diff --git a/CMakeLists.txt b/CMakeLists.txt index eb5bed523b..6baf2bf8ef 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -230,6 +230,7 @@ set(FlatBuffers_Tests_SRCS tests/parser_test.cpp tests/proto_test.cpp tests/reflection_test.cpp + tests/reflection_union_security_test.cpp tests/test.cpp tests/test_assert.h tests/test_assert.cpp diff --git a/include/flatbuffers/reflection.h b/include/flatbuffers/reflection.h index bc8c2a9288..d6b6188d6f 100644 --- a/include/flatbuffers/reflection.h +++ b/include/flatbuffers/reflection.h @@ -425,17 +425,76 @@ pointer_inside_vector piv(T* ptr, std::vector& vec) { constexpr const char* UnionTypeFieldSuffix() { return "_type"; } // Helper to figure out the actual table type a union refers to. -inline const reflection::Object& GetUnionType( - const reflection::Schema& schema, const reflection::Object& parent, - const reflection::Field& unionfield, const Table& table) { - auto enumdef = schema.enums()->Get(unionfield.type()->index()); +// The union discriminator is the enum VALUE, matched against declared member +// values; a tag that is not a declared value, or whose member type index is +// out of range, yields failure instead of an out-of-bounds / null read. +// Returns false (and leaves *out_obj untouched) if the member cannot be +// resolved safely. +inline bool GetUnionType(const reflection::Schema& schema, + const reflection::Object& parent, + const reflection::Field& unionfield, + const Table& table, + const reflection::Object** out_obj) { + const auto enum_index = unionfield.type()->index(); + if (enum_index < 0 || enum_index >= static_cast(schema.enums()->size())) + return false; + auto enumdef = schema.enums()->Get(enum_index); // TODO: this is clumsy and slow, but no other way to find it? auto type_field = parent.fields()->LookupByKey( (unionfield.name()->str() + UnionTypeFieldSuffix()).c_str()); - FLATBUFFERS_ASSERT(type_field); - auto union_type = GetFieldI(table, *type_field); - auto enumval = enumdef->values()->LookupByKey(union_type); - return *schema.objects()->Get(enumval->union_type()->index()); + if (!type_field) return false; + // Read the discriminator at the union enum's underlying width so declared + // member values larger than 255 compare exactly (mirrors the generated code, + // which reads the _type field as e.g. int32). Note the reflection schema + // marks the _type field as base_type UType while the wire stores it at the + // enum's underlying width, so read directly (GetFieldI would assert on the + // UType/width mismatch). + const reflection::Type* underlying = enumdef->underlying_type(); + const size_t base_size = underlying ? underlying->base_size() : 1; + int64_t union_type = 0; + switch (base_size) { + case 1: union_type = table.GetField(type_field->offset(), 0); break; + case 2: union_type = table.GetField(type_field->offset(), 0); break; + case 4: union_type = table.GetField(type_field->offset(), 0); break; + case 8: union_type = table.GetField(type_field->offset(), 0); break; + default: + union_type = table.GetField(type_field->offset(), 0); + break; + } + // Match by value (the generated per-schema verifiers switch on real values). + const reflection::EnumVal* matched = nullptr; + for (uoffset_t i = 0; i < enumdef->values()->size(); i++) { + auto cand = enumdef->values()->Get(i); + if (cand->value() == union_type) { + matched = cand; + break; + } + } + if (!matched) return false; + auto ut = matched->union_type(); + if (!ut) return false; + const auto obj_index = ut->index(); + if (obj_index < 0 || + obj_index >= static_cast(schema.objects()->size())) + return false; + *out_obj = schema.objects()->Get(obj_index); + return true; +} + +// Legacy reference-returning helper. Only safe on a schema+table pair that has +// already been validated (e.g. after reflection::Verify); kept for source +// compatibility. Prefer the bool overload above in new code. +inline const reflection::Object& GetUnionType( + const reflection::Schema& schema, const reflection::Object& parent, + const reflection::Field& unionfield, const Table& table) { + const reflection::Object* obj = nullptr; + if (!GetUnionType(schema, parent, unionfield, table, &obj)) { + // Cannot fail safely while returning a reference; this matches the old + // contract (callers only reach here on verified buffers). + FLATBUFFERS_ASSERT(false); + obj = schema.objects()->Get(0); + } + return *obj; } // Changes the contents of a string inside a FlatBuffer. FlatBuffer must @@ -525,6 +584,13 @@ bool VerifySizePrefixed(const reflection::Schema& schema, size_t length, uoffset_t max_depth = 64, uoffset_t max_tables = 1000000); +// Validates the cross-references of a reflection schema: every Type.index that +// points into schema.objects() / schema.enums() must be in range. The +// structural verifier (reflection::VerifySchemaBuffer) does not perform these +// checks, so callers that follow Type.index on possibly-untrusted schemas +// should run this first. +bool SchemaIsValid(const reflection::Schema& schema); + } // namespace flatbuffers #endif // FLATBUFFERS_REFLECTION_H_ diff --git a/src/binary_annotator.cpp b/src/binary_annotator.cpp index 98a8648662..cbfab2d536 100644 --- a/src/binary_annotator.cpp +++ b/src/binary_annotator.cpp @@ -130,6 +130,12 @@ std::map BinaryAnnotator::Annotate() { return {}; } } + // VerifySchemaBuffer is structural only; it does not cross-check Type.index + // against objects()/enums(). Reject schemas with out-of-range + // cross-references before walking them (applies to both constructors). + if (schema_ && !SchemaIsValid(*schema_)) { + return {}; + } // The binary is too short to read as a flatbuffers. if (binary_length_ < FLATBUFFERS_MIN_BUFFER_SIZE) { @@ -1408,10 +1414,26 @@ void BinaryAnnotator::BuildVector( std::string BinaryAnnotator::BuildUnion(const uint64_t union_offset, const uint8_t realized_type, const reflection::Field* const field) { - const reflection::Enum* next_enum = - schema_->enums()->Get(field->type()->index()); - - const reflection::EnumVal* enum_val = next_enum->values()->Get(realized_type); + const auto enum_index = field->type()->index(); + if (enum_index < 0 || + enum_index >= static_cast(schema_->enums()->size())) { + return "unknown"; + } + const reflection::Enum* next_enum = schema_->enums()->Get(enum_index); + + // The discriminator is the enum VALUE; find the member by value, not by + // position in values(). + const reflection::EnumVal* enum_val = nullptr; + for (uoffset_t i = 0; i < next_enum->values()->size(); i++) { + auto cand = next_enum->values()->Get(i); + if (cand && cand->value() == realized_type) { + enum_val = cand; + break; + } + } + if (!enum_val) { + return "unknown"; + } if (ContainsSection(union_offset)) { return enum_val->name()->c_str(); @@ -1419,23 +1441,27 @@ std::string BinaryAnnotator::BuildUnion(const uint64_t union_offset, const reflection::Type* union_type = enum_val->union_type(); - if (union_type->base_type() == reflection::BaseType::Obj) { - const reflection::Object* object = - schema_->objects()->Get(union_type->index()); + if (union_type && union_type->base_type() == reflection::BaseType::Obj) { + const auto object_index = union_type->index(); + if (object_index >= 0 && + object_index < static_cast(schema_->objects()->size())) { + const reflection::Object* object = + schema_->objects()->Get(object_index); - if (object->is_struct()) { - // Union of vectors point to a new Binary section - std::vector regions; + if (object->is_struct()) { + // Union of vectors point to a new Binary section + std::vector regions; - BuildStruct(union_offset, regions, field->name()->c_str(), object); + BuildStruct(union_offset, regions, field->name()->c_str(), object); - AddSection( - union_offset, - MakeBinarySection(std::string(object->name()->c_str()) + "." + - field->name()->c_str(), - BinarySectionType::Union, std::move(regions))); - } else { - BuildTable(union_offset, BinarySectionType::Table, object); + AddSection( + union_offset, + MakeBinarySection(std::string(object->name()->c_str()) + "." + + field->name()->c_str(), + BinarySectionType::Union, std::move(regions))); + } else { + BuildTable(union_offset, BinarySectionType::Table, object); + } } } // TODO(dbaileychess): handle the other union types. diff --git a/src/binary_annotator.h b/src/binary_annotator.h index d1f1af2e1a..be90a58418 100644 --- a/src/binary_annotator.h +++ b/src/binary_annotator.h @@ -391,7 +391,12 @@ class BinaryAnnotator { bool IsInlineField(const reflection::Field* const field) { if (field->type()->base_type() == reflection::BaseType::Obj) { - return schema_->objects()->Get(field->type()->index())->is_struct(); + const auto index = field->type()->index(); + if (index < 0 || + index >= static_cast(schema_->objects()->size())) { + return false; + } + return schema_->objects()->Get(index)->is_struct(); } return IsScalar(field->type()->base_type()); } @@ -423,7 +428,14 @@ class BinaryAnnotator { return false; } - return value < enum_def->values()->size(); + // The discriminator is the enum VALUE, not a position in values(). + // Match it against declared member values; a tag that matches no declared + // member is not a valid union value. + for (uoffset_t i = 0; i < enum_def->values()->size(); i++) { + const reflection::EnumVal* ev = enum_def->values()->Get(i); + if (ev && ev->value() == value) return true; + } + return false; } uint64_t GetElementSize(const reflection::Field* const field) { @@ -433,7 +445,12 @@ class BinaryAnnotator { switch (field->type()->element()) { case reflection::BaseType::Obj: { - auto obj = schema_->objects()->Get(field->type()->index()); + const auto index = field->type()->index(); + if (index < 0 || + index >= static_cast(schema_->objects()->size())) { + return sizeof(uint32_t); + } + auto obj = schema_->objects()->Get(index); return obj->is_struct() ? obj->bytesize() : sizeof(uint32_t); } default: diff --git a/src/reflection.cpp b/src/reflection.cpp index 268d7d8515..9ca1b128a2 100644 --- a/src/reflection.cpp +++ b/src/reflection.cpp @@ -65,17 +65,54 @@ static bool VerifyObject(flatbuffers::Verifier& v, const reflection::Object& obj, const flatbuffers::Table* table, bool required); +// Bounds-checked accessors for schema cross-references. A schema that has not +// been cross-validated (see SchemaIsValid below) may carry Type.index values +// that are out of range; these helpers make every consumer safe regardless. +inline const reflection::Object* GetSchemaObject(const reflection::Schema& s, + int32_t index) { + if (index < 0 || index >= static_cast(s.objects()->size())) + return nullptr; + return s.objects()->Get(index); +} + +inline const reflection::Enum* GetSchemaEnum(const reflection::Schema& s, + int32_t index) { + if (index < 0 || index >= static_cast(s.enums()->size())) + return nullptr; + return s.enums()->Get(index); +} + static bool VerifyUnion(flatbuffers::Verifier& v, - const reflection::Schema& schema, uint8_t utype, + const reflection::Schema& schema, int64_t utype, const uint8_t* elem, const reflection::Field& union_field) { if (!utype) return true; // Not present. - auto fb_enum = schema.enums()->Get(union_field.type()->index()); - if (utype >= fb_enum->values()->size()) return false; - auto elem_type = fb_enum->values()->Get(utype)->union_type(); + auto fb_enum = GetSchemaEnum(schema, union_field.type()->index()); + if (!fb_enum) return false; + // The wire discriminator is the enum VALUE, not a position in values(). + // Resolve the member by matching the declared EnumVal.value, exactly like + // the generated per-schema union verifiers do (see the switch in + // tests/union_underlying_type_test_generated.h). No matching member value + // means the tag is not a declared member: reject. + const reflection::EnumVal* matched = nullptr; + for (uoffset_t i = 0; i < fb_enum->values()->size(); i++) { + auto cand = fb_enum->values()->Get(i); + if (cand->value() == utype) { + matched = cand; + break; + } + } + if (!matched) return false; + auto elem_type = matched->union_type(); + if (!elem_type) return false; switch (elem_type->base_type()) { case reflection::Obj: { - auto elem_obj = schema.objects()->Get(elem_type->index()); + // Type.index is attacker-controlled schema data; never trust it without + // a bounds check against objects().size(). + const auto index = elem_type->index(); + if (index < 0 || index >= static_cast(schema.objects()->size())) + return false; + auto elem_obj = schema.objects()->Get(index); if (elem_obj->is_struct()) { return v.VerifyFromPointer(elem, elem_obj->bytesize()); } else { @@ -130,7 +167,8 @@ static bool VerifyVector(flatbuffers::Verifier& v, } } case reflection::Obj: { - auto obj = schema.objects()->Get(vec_field.type()->index()); + auto obj = GetSchemaObject(schema, vec_field.type()->index()); + if (!obj) return false; if (obj->is_struct()) { return VerifyVectorOfStructs(v, table, vec_field.offset(), *obj, vec_field.required()); @@ -154,7 +192,7 @@ static bool VerifyVector(flatbuffers::Verifier& v, if (!v.VerifyVector(vec)) return false; if (!vec) return true; auto type_vec = table.GetPointer*>(vec_field.offset() - - sizeof(voffset_t)); + sizeof(voffset_t)); if (!v.VerifyVector(type_vec)) return false; if (type_vec->size() != vec->size()) return false; for (uoffset_t j = 0; j < vec->size(); j++) { @@ -233,7 +271,8 @@ static bool VerifyObject(flatbuffers::Verifier& v, if (!VerifyVector(v, schema, *table, *field_def)) return false; break; case reflection::Obj: { - auto child_obj = schema.objects()->Get(field_def->type()->index()); + auto child_obj = GetSchemaObject(schema, field_def->type()->index()); + if (!child_obj) return false; if (child_obj->is_struct()) { if (!VerifyStruct(v, *table, field_def->offset(), *child_obj, field_def->required())) { @@ -249,9 +288,31 @@ static bool VerifyObject(flatbuffers::Verifier& v, break; } case reflection::Union: { - // get union type from the prev field + // get union type from the prev field voffset_t utype_offset = field_def->offset() - sizeof(voffset_t); - auto utype = table->GetField(utype_offset, 0); + // The discriminator is stored in the width of the union enum's + // underlying type (e.g. int32 for `union ABC : int { A = 555, ... }`), + // not always a single byte. Read it at that width so declared member + // values larger than 255 are compared exactly. + const reflection::Type* utype_type = field_def->type(); + const reflection::Enum* utype_enum = nullptr; + if (utype_type && utype_type->index() >= 0 && + utype_type->index() < + static_cast(schema.enums()->size())) { + utype_enum = schema.enums()->Get(utype_type->index()); + } + const reflection::Type* underlying = utype_enum + ? utype_enum->underlying_type() + : nullptr; + const size_t base_size = underlying ? underlying->base_size() : 1; + int64_t utype = 0; + switch (base_size) { + case 1: utype = table->GetField(utype_offset, 0); break; + case 2: utype = table->GetField(utype_offset, 0); break; + case 4: utype = table->GetField(utype_offset, 0); break; + case 8: utype = table->GetField(utype_offset, 0); break; + default: utype = table->GetField(utype_offset, 0); break; + } auto uval = reinterpret_cast( flatbuffers::GetFieldT(*table, *field_def)); if (!VerifyUnion(v, schema, utype, uval, *field_def)) { @@ -272,6 +333,67 @@ static bool VerifyObject(flatbuffers::Verifier& v, } // namespace +// Cross-reference validation of a reflection schema. The structural verifier +// (reflection::VerifySchemaBuffer) only checks per-object buffer structure; it +// does NOT verify that a Type.index is a valid index into schema.objects() / +// schema.enums(). Every reflection consumer that follows such an index +// (VerifyUnion, CopyTable, ResizeTable, GetUnionType, code generators) would +// otherwise trust attacker-controlled index data. This pass rejects any schema +// whose cross-references are out of range. It is called at the entry of +// reflection::Verify / VerifySizePrefixed and can be used by other consumers +// (e.g. BinaryAnnotator) before walking the schema. +bool SchemaIsValid(const reflection::Schema& schema) { + const auto object_count = static_cast(schema.objects()->size()); + const auto enum_count = static_cast(schema.enums()->size()); + auto type_index_ok = [object_count, enum_count](const reflection::Type* t) { + if (!t) return true; // absent optional Type is fine + switch (t->base_type()) { + case reflection::Obj: + case reflection::Union: + return t->index() >= 0 && t->index() < object_count; + case reflection::Vector: + if (t->element() == reflection::Obj || + t->element() == reflection::Union) { + return t->index() >= 0 && t->index() < object_count; + } + return true; + default: + // Integral/enum-derived types index into enums; scalar types have no + // cross-reference. + if (t->base_type() >= reflection::UType && + t->base_type() <= reflection::ULong) { + return true; + } + if (t->index() >= 0) return t->index() < enum_count; + return true; + } + }; + for (uoffset_t o = 0; o < schema.objects()->size(); o++) { + auto obj = schema.objects()->Get(o); + if (!obj) return false; + for (uoffset_t f = 0; f < obj->fields()->size(); f++) { + auto field = obj->fields()->Get(f); + if (!field || !type_index_ok(field->type())) return false; + } + } + for (uoffset_t e = 0; e < schema.enums()->size(); e++) { + auto en = schema.enums()->Get(e); + if (!en) return false; + if (en->is_union()) { + for (uoffset_t v = 0; v < en->values()->size(); v++) { + auto ev = en->values()->Get(v); + if (!ev) return false; + auto ut = ev->union_type(); + if (ut && (ut->base_type() == reflection::Obj || + ut->base_type() == reflection::Union)) { + if (ut->index() < 0 || ut->index() >= object_count) return false; + } + } + } + } + return true; +} + int64_t GetAnyValueI(reflection::BaseType type, const uint8_t* data) { // clang-format off #define FLATBUFFERS_GET(T) static_cast(ReadScalar(data)) @@ -333,7 +455,8 @@ std::string GetAnyValueS(reflection::BaseType type, const uint8_t* data, return s ? s->c_str() : ""; } case reflection::Obj: - if (schema) { + if (schema && type_index >= 0 && + type_index < static_cast(schema->objects()->size())) { // Convert the table to a string. This is mostly for debugging purposes, // and does NOT promise to be JSON compliant. // Also prefixes the type. @@ -527,7 +650,7 @@ class ResizeContext { // Ignore structs. auto subobjectdef = base_type == reflection::Obj - ? schema_.objects()->Get(fielddef.type()->index()) + ? GetSchemaObject(schema_, fielddef.type()->index()) : nullptr; if (subobjectdef && subobjectdef->is_struct()) continue; // Get this fields' offset, and read it if safe. @@ -550,7 +673,7 @@ class ResizeContext { auto vec = reinterpret_cast*>(ref); auto elemobjectdef = elem_type == reflection::Obj - ? schema_.objects()->Get(fielddef.type()->index()) + ? GetSchemaObject(schema_, fielddef.type()->index()) : nullptr; if (elemobjectdef && elemobjectdef->is_struct()) break; for (uoffset_t i = 0; i < vec->size(); i++) { @@ -564,8 +687,11 @@ class ResizeContext { break; } case reflection::Union: { - ResizeTable(GetUnionType(schema_, objectdef, fielddef, *table), - reinterpret_cast(ref)); + const reflection::Object* subobj = nullptr; + if (GetUnionType(schema_, objectdef, fielddef, *table, &subobj) && + subobj) { + ResizeTable(*subobj, reinterpret_cast(ref)); + } break; } case reflection::String: @@ -661,8 +787,11 @@ Offset CopyTable(FlatBufferBuilder& fbb, const reflection::Object& objectdef, const Table& table, bool use_string_pooling) { // Before we can construct the table, we have to first generate any - // subobjects, and collect their offsets. - std::vector offsets; + // subobjects, and collect their offsets. Generated offsets are keyed by the + // field's voffset so the second pass can consume them exactly for the fields + // that produced one (fields that cannot be produced, e.g. an unresolvable + // union member, are simply skipped instead of desyncing the stream). + std::vector> generated; auto fielddefs = objectdef.fields(); for (auto it = fielddefs->begin(); it != fielddefs->end(); ++it) { auto& fielddef = **it; @@ -677,19 +806,23 @@ Offset CopyTable(FlatBufferBuilder& fbb, break; } case reflection::Obj: { - auto& subobjectdef = *schema.objects()->Get(fielddef.type()->index()); - if (!subobjectdef.is_struct()) { - offset = CopyTable(fbb, schema, subobjectdef, - *GetFieldT(table, fielddef), use_string_pooling) - .o; + auto subobjectdef = GetSchemaObject(schema, fielddef.type()->index()); + if (subobjectdef && !subobjectdef->is_struct()) { + offset = + CopyTable(fbb, schema, *subobjectdef, + *GetFieldT(table, fielddef), use_string_pooling) + .o; } break; } case reflection::Union: { - auto& subobjectdef = GetUnionType(schema, objectdef, fielddef, table); - offset = CopyTable(fbb, schema, subobjectdef, - *GetFieldT(table, fielddef), use_string_pooling) - .o; + const reflection::Object* subobj = nullptr; + if (GetUnionType(schema, objectdef, fielddef, table, &subobj) && + subobj) { + offset = CopyTable(fbb, schema, *subobj, + *GetFieldT(table, fielddef), use_string_pooling) + .o; + } break; } case reflection::Vector: { @@ -698,7 +831,7 @@ Offset CopyTable(FlatBufferBuilder& fbb, auto element_base_type = fielddef.type()->element(); auto elemobjectdef = element_base_type == reflection::Obj - ? schema.objects()->Get(fielddef.type()->index()) + ? GetSchemaObject(schema, fielddef.type()->index()) : nullptr; switch (element_base_type) { case reflection::String: { @@ -741,32 +874,45 @@ Offset CopyTable(FlatBufferBuilder& fbb, break; } if (offset) { - offsets.push_back(offset); + generated.emplace_back(fielddef.offset(), offset); } } // Now we can build the actual table from either offsets or scalar data. auto start = objectdef.is_struct() ? fbb.StartStruct(objectdef.minalign()) : fbb.StartTable(); - size_t offset_idx = 0; + size_t gen_idx = 0; for (auto it = fielddefs->begin(); it != fielddefs->end(); ++it) { auto& fielddef = **it; if (!table.CheckField(fielddef.offset())) continue; auto base_type = fielddef.type()->base_type(); switch (base_type) { case reflection::Obj: { - auto& subobjectdef = *schema.objects()->Get(fielddef.type()->index()); - if (subobjectdef.is_struct()) { - CopyInline(fbb, fielddef, table, subobjectdef.minalign(), - subobjectdef.bytesize()); + auto subobjectdef = GetSchemaObject(schema, fielddef.type()->index()); + if (subobjectdef && subobjectdef->is_struct()) { + CopyInline(fbb, fielddef, table, subobjectdef->minalign(), + subobjectdef->bytesize()); break; } } FLATBUFFERS_FALLTHROUGH(); // fall thru case reflection::Union: case reflection::String: - case reflection::Vector: - fbb.AddOffset(fielddef.offset(), Offset(offsets[offset_idx++])); + case reflection::Vector: { + // Consume the offset generated in the first pass for THIS field. A + // field that produced no offset (e.g. an unresolvable union member) is + // skipped rather than desyncing the stream. + auto it2 = std::find_if( + generated.begin() + static_cast(gen_idx), + generated.end(), + [&](const std::pair& p) { + return p.first == fielddef.offset(); + }); + if (it2 != generated.end()) { + fbb.AddOffset(fielddef.offset(), Offset(it2->second)); + gen_idx = static_cast(it2 - generated.begin()) + 1; + } break; + } default: { // Scalars. auto size = GetTypeSize(base_type); CopyInline(fbb, fielddef, table, size, size); @@ -774,7 +920,8 @@ Offset CopyTable(FlatBufferBuilder& fbb, } } } - FLATBUFFERS_ASSERT(offset_idx == offsets.size()); + FLATBUFFERS_ASSERT(gen_idx == generated.size() || + generated.empty()); if (objectdef.is_struct()) { fbb.ClearOffsets(); return fbb.EndStruct(); @@ -786,6 +933,7 @@ Offset CopyTable(FlatBufferBuilder& fbb, bool Verify(const reflection::Schema& schema, const reflection::Object& root, const uint8_t* const buf, const size_t length, const uoffset_t max_depth, const uoffset_t max_tables) { + if (!SchemaIsValid(schema)) return false; Verifier v(buf, length, max_depth, max_tables); return VerifyObject(v, schema, root, flatbuffers::GetAnyRoot(buf), /*required=*/true); @@ -795,6 +943,7 @@ bool VerifySizePrefixed(const reflection::Schema& schema, const reflection::Object& root, const uint8_t* const buf, const size_t length, const uoffset_t max_depth, const uoffset_t max_tables) { + if (!SchemaIsValid(schema)) return false; Verifier v(buf, length, max_depth, max_tables); return VerifyObject(v, schema, root, flatbuffers::GetAnySizePrefixedRoot(buf), /*required=*/true); diff --git a/tests/reflection_test.h b/tests/reflection_test.h index da6fb1ff69..f28f5ac213 100644 --- a/tests/reflection_test.h +++ b/tests/reflection_test.h @@ -13,6 +13,7 @@ void ReflectionTest(const std::string& tests_data_path, uint8_t* flatbuf, void ForAllFieldsReverseTest(const std::string& tests_data_path); void MiniReflectFixedLengthArrayTest(); void MiniReflectFlatBuffersTest(uint8_t* flatbuf); +void ReflectionUnionSecurityTest(); } // namespace tests } // namespace flatbuffers diff --git a/tests/reflection_union_security_test.cpp b/tests/reflection_union_security_test.cpp new file mode 100755 index 0000000000..e6591e5faa --- /dev/null +++ b/tests/reflection_union_security_test.cpp @@ -0,0 +1,216 @@ +// Reflection verifier union security regression tests. +// +// Covers the fixes for the reflection-union verifier defects: +// 1. Union member resolution by declared VALUE, not by values() position, +// with the discriminator read at the enum's underlying width (so sparse +// unions such as `union ABC : int { A = 555, B = 666 }` verify correctly). +// 2. Bounds checks on Type.index before any schema.objects()->Get() / +// enums()->Get() in the reflection verifier (SchemaIsValid + guards). +// 3. Null-safe GetUnionType for reflected consumers (CopyTable/ResizeTable). +// +// Regression assertions (previously failing / crashing): +// * a legitimate buffer with discriminator 555 (stored int32) now VERIFIES +// (previously rejected because the tag was read as a uint8). +// * a hand-crafted tag that is a position but not a declared value (1) is +// REJECTED (previously accepted -> type confusion). +// * a schema whose union-member Type.index is out of range is rejected by +// reflection::Verify (SchemaIsValid) instead of performing an OOB read. +// * CopyTable on an unresolvable-union buffer no longer crashes. +// +// Self-contained: embeds the schema, no external .fbs/.bfbs/flatc needed. +// +// Registered from tests/test.cpp as ReflectionUnionSecurityTest() (the same way +// ReflectionTest() is), and compiled into the `flattests` target via +// FlatBuffers_Tests_SRCS in CMakeLists.txt. + +#include "flatbuffers/idl.h" +#include "flatbuffers/reflection.h" +#include "flatbuffers/reflection_generated.h" +#include "flatbuffers/util.h" +#include "reflection_test.h" +#include "test_assert.h" +#include +#include +#include +#include + +using namespace flatbuffers; + +namespace flatbuffers { +namespace tests { + +namespace { + +const char* kSparseSchema = + "namespace P;" + "table A { a:int; }" // objects[0] + "table B { b:string; }" // objects[1] + "union ABC:int { A = 555, B = 666 }" + "table D { test_union:ABC; }" + "root_type D;"; + +const char* kDenseSchema = + "namespace P;" + "table A { a:int; }" // objects[0] + "table B { b:string; }" // objects[1] + "union U:int { A = 1, B = 2 }" + "table Root { u:U; }" + "root_type Root;"; + +// Build D{ test_union_type = tag, test_union = A{ a=7 } or B{ b="..." } }. +std::vector BuildD(const std::string& schema_text, int32_t tag, + bool payload_is_b) { + Parser parser; + if (!parser.Parse(schema_text.c_str())) return {}; + FlatBufferBuilder fbb(1024); + Offset s; + if (payload_is_b) s = fbb.CreateString("HELLO"); + uoffset_t mstart = fbb.StartTable(); + if (payload_is_b) { + fbb.AddOffset(4, s); + } else { + fbb.AddElement(4, 7, 0); + } + auto member = fbb.EndTable(mstart); + uoffset_t dstart = fbb.StartTable(); + fbb.AddElement(4, tag, 0); // test_union_type (int32 on the wire) + fbb.AddOffset(6, Offset(member)); + auto d = fbb.EndTable(dstart); + fbb.Finish(Offset(d)); + return std::vector(fbb.GetBufferPointer(), + fbb.GetBufferPointer() + fbb.GetSize()); +} + +// Parse a schema text and return the in-memory reflection Schema + object. +bool GetSchemaAndObject(const std::string& schema_text, const char* obj_name, + Parser* parser, const reflection::Schema** schema_out, + const reflection::Object** obj_out) { + if (!parser->Parse(schema_text.c_str())) return false; + parser->Serialize(); + *schema_out = reflection::GetSchema(parser->builder_.GetBufferPointer()); + *obj_out = (*schema_out)->objects()->LookupByKey(obj_name); + return *obj_out != nullptr; +} + +// Patch a reflection::Type.index (vtable slot VT_INDEX == 8) in place. The +// Type must live inside the (mutable) schema buffer. +void PatchTypeIndex(const reflection::Type* type, int32_t new_index) { + const auto* tbl = reinterpret_cast(type); + auto voff = tbl->GetOptionalFieldOffset(8 /* VT_INDEX */); + TEST_ASSERT(voff != 0); + uint8_t* loc = + const_cast(reinterpret_cast(type) + voff); + memcpy(loc, &new_index, sizeof(new_index)); +} + +void SparseUnionTests() { + TEST_OUTPUT_LINE("SparseUnionTests"); + + Parser parser; + const reflection::Schema* schema = nullptr; + const reflection::Object* d_obj = nullptr; + TEST_EQ(GetSchemaAndObject(kSparseSchema, "P.D", &parser, &schema, &d_obj), + true); + + // (1) Legitimate buffers produced by an official builder (discriminator + // stored as int32 = 555 / 666) must now verify. Before the fix they were + // rejected: the tag was read as uint8 (555 & 0xFF == 43 >= 3). + { + auto buf = BuildD(kSparseSchema, 555, false); + TEST_EQ(flatbuffers::Verify(*schema, *d_obj, buf.data(), buf.size()), + true); + } + { + auto buf = BuildD(kSparseSchema, 666, true); + TEST_EQ(flatbuffers::Verify(*schema, *d_obj, buf.data(), buf.size()), + true); + } + + // (2) A tag that is a valid values() position but NOT a declared member + // value (1; declared are 555/666) must be rejected, not resolved by position. + { + auto buf = BuildD(kSparseSchema, 1, true); // payload is B, tag 1 + TEST_EQ(flatbuffers::Verify(*schema, *d_obj, buf.data(), buf.size()), + false); + } +} + +void OutOfRangeIndexTests() { + TEST_OUTPUT_LINE("OutOfRangeIndexTests"); + + Parser parser; + const reflection::Schema* schema = nullptr; + const reflection::Object* root_obj = nullptr; + TEST_EQ(GetSchemaAndObject(kDenseSchema, "P.Root", &parser, &schema, + &root_obj), + true); + + // Copy schema buffer so we can patch it, then re-derive the schema pointers + // from the COPY (the parser-owned buffer must not be patched). + std::vector schema_buf( + parser.builder_.GetBufferPointer(), + parser.builder_.GetBufferPointer() + parser.builder_.GetSize()); + schema = reflection::GetSchema(schema_buf.data()); + root_obj = schema->objects()->LookupByKey("P.Root"); + TEST_NOTNULL(root_obj); + + // Member A (value 1) is at values()->Get(1); its union_type.index -> objects[0]. + auto u_enum = schema->enums()->LookupByKey("P.U"); + const reflection::EnumVal* ev_a = u_enum->values()->Get(1); + const reflection::Type* a_type = ev_a->union_type(); + TEST_EQ(a_type->index(), 0); + + // Patch A's Type.index far out of range. SchemaIsValid (called by + // reflection::Verify) must reject the schema, so Verify returns false + // instead of performing an out-of-bounds read. + PatchTypeIndex(a_type, 1000); + schema = reflection::GetSchema(schema_buf.data()); + auto ev_a2 = schema->enums()->LookupByKey("P.U")->values()->Get(1); + TEST_EQ(static_cast(ev_a2->union_type()->index()), 1000); + + TEST_EQ(flatbuffers::SchemaIsValid(*schema), false); + + // Legal data buffer (dense: tag 1 == A) against the malformed schema. + auto buf = BuildD(kDenseSchema, 1, false); + // reflection::Verify must return false (schema rejected) and MUST NOT crash. + TEST_EQ(flatbuffers::Verify(*schema, *root_obj, buf.data(), buf.size()), + false); +} + +void ConsumerNullSafetyTests() { + TEST_OUTPUT_LINE("ConsumerNullSafetyTests"); + + Parser parser; + const reflection::Schema* schema = nullptr; + const reflection::Object* d_obj = nullptr; + TEST_EQ(GetSchemaAndObject(kSparseSchema, "P.D", &parser, &schema, &d_obj), + true); + + // Buffer with an unresolvable union tag (1, not a declared value). + auto buf = BuildD(kSparseSchema, 1, true); + + // reflection::Verify rejects it... + TEST_EQ(flatbuffers::Verify(*schema, *d_obj, buf.data(), buf.size()), + false); + + // ...and handing it to a reflected consumer (CopyTable) must not crash: + // GetUnionType returns failure and the union field is skipped. + FlatBufferBuilder out(1024); + const auto* root = + reinterpret_cast(GetRoot
(buf.data())); + Offset copied = CopyTable(out, *schema, *d_obj, *root); + TEST_ASSERT(!copied.IsNull()); +} + +} // namespace + +void ReflectionUnionSecurityTest() { + SparseUnionTests(); + OutOfRangeIndexTests(); + ConsumerNullSafetyTests(); + TEST_OUTPUT_LINE("ReflectionUnionSecurityTest: PASSED"); +} + +} // namespace tests +} // namespace flatbuffers + diff --git a/tests/test.cpp b/tests/test.cpp index 5a43546f53..c9d762829f 100644 --- a/tests/test.cpp +++ b/tests/test.cpp @@ -1774,6 +1774,7 @@ int FlatBufferTests(const std::string& tests_data_path) { FixedLengthArrayJsonTest(tests_data_path, false); FixedLengthArrayJsonTest(tests_data_path, true); ReflectionTest(tests_data_path, flatbuf.data(), flatbuf.size()); + ReflectionUnionSecurityTest(); ForAllFieldsReverseTest(tests_data_path); ParseProtoTest(tests_data_path); EvolutionTest(tests_data_path);