Skip to content

Fix out-of-bounds write in BFBS field iteration - #9203

Open
maxvanamersfort wants to merge 1 commit into
google:masterfrom
maxvanamersfort:fix-bfbs-field-id-oob
Open

Fix out-of-bounds write in BFBS field iteration#9203
maxvanamersfort wants to merge 1 commit into
google:masterfrom
maxvanamersfort:fix-bfbs-field-id-oob

Conversation

@maxvanamersfort

Copy link
Copy Markdown

Summary

  • treat field IDs read from BFBS input as untrusted values
  • sort field pointers by ID instead of indexing a field-count-sized vector
  • reuse the centralized ForAllFields implementation in BaseBfbsGenerator
  • add regression coverage for a verifier-accepted BFBS containing field ID 65535

Problem

ForAllFields allocated a vector with object->fields()->size() entries and
then used field->id() as the write index. A structurally valid BFBS can contain
a sparse or oversized field ID, so processing such a schema could write past the
end of that vector.

Fix

Collect the field pointers, sort them by ID, and iterate over the sorted pointers
in forward or reverse order. This preserves the expected order for normal BFBS
schemas without assuming that IDs form a dense range. The duplicate vulnerable
mapping in BaseBfbsGenerator is removed in favor of the centralized helper.

Testing

  • reproduced the original out-of-bounds access with libstdc++ assertions enabled
  • confirmed the crafted BFBS passes VerifySchemaBuffer
  • added a regression test using field ID 65535
  • ran the full upstream C++ test suite: ALL TESTS PASSED

Fixes #8950

Treat field IDs from reflection schemas as untrusted when iterating. Sort field pointers by ID instead of indexing a field-count-sized vector, and add regression coverage for a verified BFBS containing ID 65535.
@github-actions github-actions Bot added c++ codegen Involving generating code from schema labels Aug 20, 2026
@xhon-pelushi

Copy link
Copy Markdown

Verified the regression path locally. On PR head 7e35e54defc1064448d447d8794bdc22e5770891, both of these pass:

cmake -S . -B build-review-9203-a -G "Unix Makefiles" -DCMAKE_BUILD_TYPE=Release -DFLATBUFFERS_STRICT_MODE=ON -DFLATBUFFERS_BUILD_CPP17=ON
cmake --build build-review-9203-a --target flattests -j$(nproc)
./build-review-9203-a/flattests

cmake -S . -B build-review-9203-assert -G "Unix Makefiles" -DCMAKE_BUILD_TYPE=Release -DFLATBUFFERS_STRICT_MODE=ON -DFLATBUFFERS_CXX_FLAGS="-D_GLIBCXX_ASSERTIONS"
cmake --build build-review-9203-assert --target flattests -j$(nproc)
./build-review-9203-assert/flattests

Then I checked out the parent 5761d6e67af841d15ee21bc1ce9a78ffa9cf939e, transplanted only tests/reflection_test.cpp from this PR, and rebuilt with -D_GLIBCXX_ASSERTIONS. That run aborts in the old implementation at the vector subscript:

std::vector<unsigned int>::operator[](size_type): Assertion '__n < this->size()' failed.

I also grepped the tree for the same direct field->id()-as-vector-index shape. The duplicated helper in bfbs_gen.h is gone, BaseBfbsGenerator::ForAllFields() now goes through the centralized flatbuffers::ForAllFields(), and I didn't find another remaining copy of this specific unchecked BFBS field-id mapping nearby.

I did not run the full multi-language tests/TestAll.sh; this verification is scoped to the C++ flattests path affected by src/reflection.cpp, src/bfbs_gen.h, and tests/reflection_test.cpp.

Tried to submit this as an approving review, but GitHub rejects formal PR reviews here for accounts without explicit repository access, so leaving the verified result as a comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ codegen Involving generating code from schema

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Heap Out-of-Bounds Write via Unchecked field->id() Index in BFBS Schema Processing

2 participants