Fix heap OOB read in reflection-based Verify() from unchecked Type.index() - #9198
Open
prasanna8585 wants to merge 2 commits into
Open
Fix heap OOB read in reflection-based Verify() from unchecked Type.index()#9198prasanna8585 wants to merge 2 commits into
prasanna8585 wants to merge 2 commits into
Conversation
…dex() VerifyUnion, VerifyVector, and VerifyObject in reflection.cpp use field.type()->index() to index into schema.objects()/schema.enums() without validating the index is in range. Vector::Get()'s only bounds check is an assert that is compiled out in NDEBUG (release) builds, so a schema with an out-of-range Type.index() causes an out-of-bounds read when verified via the reflection API. This affects callers using flatbuffers::Verify() with a schema from an untrusted or externally supplied source. Adds bounds checks at all three affected call sites so verification fails closed instead, plus a regression test that builds a schema with an empty objects vector and an out-of-range field type.index(), and confirms Verify() now returns false instead of reading out of bounds. Note: CopyTable/CopyInline (~L680-757) have the same unchecked index() pattern but operate on data expected to already be verified; leaving those out of this fix to keep it minimal and reviewable, flagging for a maintainer look separately.
Author
|
Hi @dbaileychess, |
…nchecked Type.index()
Independent finding, found by investigating a lead my own prior fix
(heap OOB read in reflection-based Verify(), VerifyUnion/VerifyVector/
VerifyObject) explicitly left open in its own commit message:
CopyTable/CopyInline have the same unchecked index() pattern but were
assumed to 'operate on data expected to already be verified.' That
assumption was never actually verified.
Neither CopyTable nor ResizeContext::ResizeTable (reached via the
public SetString/ResizeAnyVector mutation APIs) requires the caller
to have verified the schema against the data -- and even a caller
that did verify may have done so against a different, benign schema
than the one passed here. An out-of-range Type.index() reaches
Vector::Get(), whose only bounds check is an assert compiled out in
release (NDEBUG) builds.
Found and fixed 5 instances of the identical pattern:
- 3 sites in CopyTable (the top-level Obj field case, the
vector-of-Obj element case, and the second loop that builds the
actual output table)
- 2 sites in ResizeContext::ResizeTable (the direct Obj field case
and the vector-of-Obj element case)
Dynamically confirmed both as real, independent crashes with real
ASan runs: a malicious schema (empty objects vector, a field with
Type.index()=999999) passed to CopyTable produced a genuine
ASan-reported SEGV from an out-of-bounds read; the same schema shape,
reached via SetString (whose resize triggers ResizeTable's
whole-object-graph walk as a side effect of resizing an unrelated
string field), produced a second, independent ASan-reported SEGV
inside ResizeTable.
Fix: CopyTable's sites skip the malformed field entirely, mirroring
how the same function already tolerates an absent field. The second
loop specifically must rather than on an
out-of-range index, since would fall through to code
expecting to consume an entry from the offsets vector that was never
pushed for this field, desynchronizing every field processed after
it. ResizeContext::ResizeTable's sites are simpler: the existing
downstream code already null-safely handles a missing object
definition, so resolving it safely (nullptr on out-of-range) is
sufficient on its own.
Dedup checked: fetched the one existing, superficially-similar public
report (google#9040, Heap Buffer Overflow in FlatBuffers Reflection
Verifier) in full and confirmed it is a genuinely different bug --
its root cause is GetFieldT reading a table field offset without
checking it against the table's own size, not an out-of-range index
into schema.objects(). Searched this repo's history (git log -S on
the exact vulnerable line) and found no prior fix attempt.
Verified: both PoCs re-run against the patched code now complete
cleanly with no crash. Added 2 new regression tests
(CopyTableInvalidObjectIndexTest, ResizeTableInvalidObjectIndexTest),
matching the file's own existing ReflectionInvalidObjectIndexTest
convention. Built the full existing test suite with ASan and ran it
end to end: ALL TESTS PASSED, including both new tests, with no
regressions.
Author
|
Hi @dbaileychess, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
VerifyUnion,VerifyVector, andVerifyObjectinreflection.cppusefield.type()->index()to index intoschema.objects()/schema.enums()without validating the index is in range.Vector::Get()'s only bounds check is an assert that is compiled out inNDEBUG(release) builds, so a schema with an out-of-rangeType.index()causes an out-of-bounds read when verified via the reflection API.Who is affected
Applications that call
flatbuffers::Verify(schema, root, buf, len)with a schema from an untrusted or externally supplied source (e.g. tools that load.bfbsschemas at runtime, per the documented reflection use case).Fix
Adds a range check (
0 <= index() < size()) at each of the three call sites in the verification path before callingGet(), returningfalse(verification failure) on an out-of-range index rather than reading out of bounds.Testing
ReflectionInvalidObjectIndexTesttotests/reflection_test.cpp, which builds a schema with an emptyobjectsvector and a fieldtype.index()far out of range, and confirmsVerify()now fails closed.Vector::Getassert) against the unfixed source, and passes cleanly with the fix applied.flattests) passes with no regressions.Note for maintainers
CopyTable/CopyInline(roughly lines 680-757) have the same uncheckedindex()pattern, but they operate on data that callers are expected to have already run throughVerify(). Left out of this fix to keep the change minimal and focused on the verification path itself; flagging here in case it's worth a follow-up.