Fix type confusion and out-of-bounds read in reflection union verification

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

include/flatbuffers/reflection.h

@@ -425,17 +425,76 @@ pointer_inside_vector<T, U> piv(T* ptr, std::vector<U>& 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<int32_t>(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<uint8_t>(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<uint8_t>(type_field->offset(), 0); break;
+    case 2: union_type = table.GetField<uint16_t>(type_field->offset(), 0); break;
+    case 4: union_type = table.GetField<uint32_t>(type_field->offset(), 0); break;
+    case 8: union_type = table.GetField<uint64_t>(type_field->offset(), 0); break;
+    default:
+      union_type = table.GetField<uint8_t>(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<int32_t>(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_

src/binary_annotator.cpp

@@ -130,6 +130,12 @@ std::map<uint64_t, BinarySection> 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,34 +1414,54 @@ 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<int32_t>(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();
   }
 
   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<int32_t>(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<BinaryRegion> regions;
+      if (object->is_struct()) {
+        // Union of vectors point to a new Binary section
+        std::vector<BinaryRegion> 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.

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<int32_t>(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<int32_t>(schema_->objects()->size())) {
+          return sizeof(uint32_t);
+        }
+        auto obj = schema_->objects()->Get(index);
         return obj->is_struct() ? obj->bytesize() : sizeof(uint32_t);
       }
       default: