Fix out-of-bounds write in BFBS field iteration

src/bfbs_gen.h

@@ -20,7 +20,7 @@
 #include <cstdint>
 
 #include "flatbuffers/code_generator.h"
-#include "flatbuffers/reflection_generated.h"
+#include "flatbuffers/reflection.h"
 
 namespace flatbuffers {
 
@@ -63,21 +63,6 @@ static void ForAllDocumentation(
   }
 }
 
-// Maps the field index into object->fields() to the field's ID (the ith element
-// in the return vector).
-static std::vector<uint32_t> FieldIdToIndex(const reflection::Object* object) {
-  std::vector<uint32_t> field_index_by_id;
-  field_index_by_id.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) {
-    auto field = object->fields()->Get(i);
-    field_index_by_id[field->id()] = i;
-  }
-
-  return field_index_by_id;
-}
-
 static bool IsStructOrTable(const reflection::BaseType base_type) {
   return base_type == reflection::Obj;
 }
@@ -186,11 +171,7 @@ class BaseBfbsGenerator : public CodeGenerator {
 
   void ForAllFields(const reflection::Object* object, bool reverse,
                     std::function<void(const reflection::Field*)> func) const {
-    const std::vector<uint32_t> field_to_id_map = FieldIdToIndex(object);
-    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]));
-    }
+    flatbuffers::ForAllFields(object, reverse, func);
   }
 
   bool IsTable(const reflection::Type* type, bool use_element = false) const {

src/reflection.cpp

@@ -16,6 +16,8 @@
 
 #include "flatbuffers/reflection.h"
 
+#include <algorithm>
+
 #include "flatbuffers/util.h"
 
 // Helper functionality for reflection.
@@ -378,18 +380,26 @@ 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) {
-    auto field = object->fields()->Get(i);
-    field_to_id_map[field->id()] = i;
+  // Field IDs can originate in an untrusted BFBS buffer. Sort field pointers
+  // instead of using those IDs as indexes into a field-count-sized vector.
+  std::vector<const reflection::Field*> fields_by_id;
+  fields_by_id.reserve(object->fields()->size());
+  for (const auto* field : *object->fields()) {
+    fields_by_id.push_back(field);
   }
-
-  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]));
+  std::sort(fields_by_id.begin(), fields_by_id.end(),
+            [](const reflection::Field* lhs, const reflection::Field* rhs) {
+              return lhs->id() < rhs->id();
+            });
+
+  if (reverse) {
+    for (auto it = fields_by_id.rbegin(); it != fields_by_id.rend(); ++it) {
+      func(*it);
+    }
+  } else {
+    for (const auto* field : fields_by_id) {
+      func(field);
+    }
   }
 }
 

tests/reflection_test.cpp

@@ -333,6 +333,50 @@ void ForAllFieldsReverseTest(const std::string& tests_data_path) {
       TEST_ASSERT(reverse_ids[i - 1] > reverse_ids[i]);
     }
   }
+
+  // A structurally valid BFBS can contain a field ID that is larger than the
+  // number of fields. Iteration must not use that untrusted ID as a vector
+  // index.
+  {
+    std::string crafted_bfbs = bfbsfile;
+    auto crafted_schema = reflection::GetSchema(crafted_bfbs.data());
+    auto crafted_object =
+        crafted_schema->objects()->LookupByKey("MyGame.Example.Stat");
+    TEST_NOTNULL(crafted_object);
+    const reflection::Field* crafted_field = nullptr;
+    for (const auto* field : *crafted_object->fields()) {
+      if (field->id() != 0) {
+        crafted_field = field;
+        break;
+      }
+    }
+    TEST_NOTNULL(crafted_field);
+
+    auto field_table = reinterpret_cast<uint8_t*>(
+        const_cast<reflection::Field*>(crafted_field));
+    auto field_vtable = field_table - ReadScalar<soffset_t>(field_table);
+    auto id_offset =
+        ReadScalar<voffset_t>(field_vtable + reflection::Field::VT_ID);
+    TEST_NE(id_offset, 0);
+    WriteScalar<uint16_t>(field_table + id_offset, 0xffff);
+
+    flatbuffers::Verifier crafted_verifier(
+        reinterpret_cast<const uint8_t*>(crafted_bfbs.data()),
+        crafted_bfbs.size());
+    TEST_EQ(reflection::VerifySchemaBuffer(crafted_verifier), true);
+
+    std::vector<uint16_t> field_ids;
+    flatbuffers::ForAllFields(
+        crafted_object, /*reverse=*/false,
+        [&field_ids](const reflection::Field* field) {
+          field_ids.push_back(field->id());
+        });
+    TEST_EQ(field_ids.size(), 3u);
+    TEST_EQ(field_ids[0], 0);
+    TEST_ASSERT(field_ids[0] < field_ids[1]);
+    TEST_ASSERT(field_ids[1] < field_ids[2]);
+    TEST_EQ(field_ids[2], 0xffff);
+  }
 }
 
 void MiniReflectFlatBuffersTest(uint8_t* flatbuf) {