Fix heap-buffer-overflow in ForAllFields for out-of-range field id

src/reflection.cpp

@@ -378,18 +378,28 @@ std::string GetAnyValueS(reflection::BaseType type, const uint8_t* data,
 
 void ForAllFields(const reflection::Object* object, bool reverse,
                   std::function<void(const reflection::Field*)> func) {
-  std::vector<uint32_t> field_to_id_map;
-  field_to_id_map.resize(object->fields()->size());
-
-  // Create the mapping of field ID to the index into the vector.
-  for (uint32_t i = 0; i < object->fields()->size(); ++i) {
+  const uint32_t field_count = object->fields()->size();
+  std::vector<uint32_t> field_to_id_map(field_count, UINT32_MAX);
+
+  // Create the mapping of field ID to the index into the vector. Field IDs
+  // come from the (possibly untrusted) reflection schema and are not
+  // guaranteed to be < field_count -- skip any field whose ID does not
+  // correspond to a valid slot, matching the validation
+  // StructDef::Deserialize already applies to this same untrusted data
+  // (see idl_parser.cpp).
+  for (uint32_t i = 0; i < field_count; ++i) {
     auto field = object->fields()->Get(i);
+    if (field->id() >= field_count) {
+      continue;
+    }
     field_to_id_map[field->id()] = i;
   }
 
   for (size_t i = 0; i < field_to_id_map.size(); ++i) {
-    func(object->fields()->Get(
-        field_to_id_map[reverse ? field_to_id_map.size() - (i + 1) : i]));
+    uint32_t idx =
+        field_to_id_map[reverse ? field_to_id_map.size() - (i + 1) : i];
+    if (idx == UINT32_MAX) continue;  // slot never filled: invalid/duplicate id
+    func(object->fields()->Get(idx));
   }
 }
 

tests/reflection_test.cpp

@@ -335,6 +335,54 @@ void ForAllFieldsReverseTest(const std::string& tests_data_path) {
   }
 }
 
+// Test that ForAllFields does not write out of bounds when a Field's `id`
+// (which comes from the, possibly untrusted, reflection schema) is >= the
+// object's field count. Reachable via flatc --nim, flatc --lua, and
+// flatc --annotate, all of which call ForAllFields directly on a
+// caller-supplied .bfbs file. Confirmed via AddressSanitizer:
+// heap-buffer-overflow WRITE in field_to_id_map[field->id()] before this
+// fix.
+void ForAllFieldsOutOfRangeIdTest() {
+  flatbuffers::FlatBufferBuilder fbb;
+
+  reflection::TypeBuilder tb(fbb);
+  tb.add_base_type(reflection::Int);
+  auto type_offset = tb.Finish();
+
+  auto field_name = fbb.CreateString("evil_field");
+  reflection::FieldBuilder field_builder(fbb);
+  field_builder.add_name(field_name);
+  field_builder.add_type(type_offset);
+  field_builder.add_id(50);  // out of range: object has only 1 field
+  auto field_offset = field_builder.Finish();
+
+  std::vector<flatbuffers::Offset<reflection::Field>> fields_vec = {
+      field_offset};
+  auto fields = fbb.CreateVectorOfSortedTables(&fields_vec);
+
+  auto object_name = fbb.CreateString("EvilObject");
+  reflection::ObjectBuilder object_builder(fbb);
+  object_builder.add_name(object_name);
+  object_builder.add_fields(fields);
+  auto object_offset = object_builder.Finish();
+  fbb.Finish(object_offset);
+
+  auto* object =
+      flatbuffers::GetRoot<reflection::Object>(fbb.GetBufferPointer());
+  TEST_EQ(object->fields()->size(), 1u);
+  TEST_EQ(object->fields()->Get(0)->id(), 50);
+
+  // Must not crash / write out of bounds. The malformed field is skipped
+  // rather than visited, since it has no valid slot in a
+  // field_count-sized table.
+  int visited = 0;
+  flatbuffers::ForAllFields(object, /*reverse=*/false,
+                            [&visited](const reflection::Field*) {
+                              visited++;
+                            });
+  TEST_EQ(visited, 0);
+}
+
 void MiniReflectFlatBuffersTest(uint8_t* flatbuf) {
   auto s =
       flatbuffers::FlatBufferToString(flatbuf, Monster::MiniReflectTypeTable());

tests/reflection_test.h

@@ -11,6 +11,7 @@ namespace tests {
 void ReflectionTest(const std::string& tests_data_path, uint8_t* flatbuf,
                     size_t length);
 void ForAllFieldsReverseTest(const std::string& tests_data_path);
+void ForAllFieldsOutOfRangeIdTest();
 void MiniReflectFixedLengthArrayTest();
 void MiniReflectFlatBuffersTest(uint8_t* flatbuf);
 

tests/test.cpp

@@ -1775,6 +1775,7 @@ int FlatBufferTests(const std::string& tests_data_path) {
   FixedLengthArrayJsonTest(tests_data_path, true);
   ReflectionTest(tests_data_path, flatbuf.data(), flatbuf.size());
   ForAllFieldsReverseTest(tests_data_path);
+  ForAllFieldsOutOfRangeIdTest();
   ParseProtoTest(tests_data_path);
   EvolutionTest(tests_data_path);
   UnionDeprecationTest(tests_data_path);