From 5d833124b2023de558ed54c6225ddf22bbc0b28f Mon Sep 17 00:00:00 2001 From: prasanna8585 <65734642+prasanna8585@users.noreply.github.com> Date: Thu, 6 Aug 2026 21:59:46 +0530 Subject: [PATCH] Fix heap-buffer-overflow in ForAllFields for out-of-range field id reflection::ForAllFields() (src/reflection.cpp) resizes a std::vector to the object's real field count, then writes to it using each field's raw, schema-supplied id (uint16_t, up to 65535) as the index, with no bounds check. This is the same bug class already fixed in StructDef::Deserialize (idl_parser.cpp, #8988), but was never applied here. Reachable via flatc --nim, flatc --lua, and flatc --annotate, all of which call ForAllFields directly on a caller-supplied .bfbs reflection schema file. Confirmed via AddressSanitizer against the real, unmodified, compiled library: heap-buffer-overflow WRITE in field_to_id_map[field->id()] = i, at an attacker-controlled offset directly proportional to the crafted field id. Fix: skip any field whose id is out of range rather than writing OOB. ForAllFields is void with no error-return mechanism, so this is the minimal non-breaking fix, matching the validation StructDef::Deserialize already applies to this same untrusted data. Adds a regression test (ForAllFieldsOutOfRangeIdTest) alongside the existing ForAllFieldsReverseTest. Full official CMake test suite (flattests), ASan-instrumented: ALL TESTS PASSED, zero regressions. --- src/reflection.cpp | 24 ++++++++++++++------ tests/reflection_test.cpp | 48 +++++++++++++++++++++++++++++++++++++++ tests/reflection_test.h | 1 + tests/test.cpp | 1 + 4 files changed, 67 insertions(+), 7 deletions(-) diff --git a/src/reflection.cpp b/src/reflection.cpp index 268d7d8515..b2fa19abe7 100644 --- a/src/reflection.cpp +++ b/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 func) { - std::vector 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 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)); } } diff --git a/tests/reflection_test.cpp b/tests/reflection_test.cpp index 9f80b936ce..2554b388f9 100644 --- a/tests/reflection_test.cpp +++ b/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> 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(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()); diff --git a/tests/reflection_test.h b/tests/reflection_test.h index da6fb1ff69..ecc925675f 100644 --- a/tests/reflection_test.h +++ b/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); diff --git a/tests/test.cpp b/tests/test.cpp index 5a43546f53..a2e215b109 100644 --- a/tests/test.cpp +++ b/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);