Bound out-of-range Object/Enum index lookups in BinaryAnnotator - #9212
Open
unpredictable21 wants to merge 1 commit into
Open
Bound out-of-range Object/Enum index lookups in BinaryAnnotator#9212unpredictable21 wants to merge 1 commit into
unpredictable21 wants to merge 1 commit into
Conversation
When flatc --annotate consumes a .bfbs schema, BinaryAnnotator walks
each Field's Type.index and dereferences
schema_->objects()->Get(field->type()->index()) (and the matching enums
vector) without first validating that the index is in range. The
reflection::VerifySchemaBuffer that runs before annotation only checks
structural integrity of the schema (offsets, sizes, alignment, vector
bounds); it never validates that a Field.type.index references a slot in
schema->objects() or schema->enums().
A schema with Type { base_type = Obj, index = N } where N >=
schema->objects()->size() (and similarly for unions and enum-driven
indexes) reaches one of eight unguarded sinks in binary_annotator.cpp /
binary_annotator.h. Each sink reads Vector::Get(N) where N is far
beyond the underlying heap allocation. In a release build without
FLATBUFFERS_ASSERT, the read proceeds past the heap buffer and into
adjacent heap pages; depending on the index value this either crosses
into unmapped memory (SEGV) or reads attacker-influenced heap contents.
This change adds two helpers, BinaryAnnotator::GetObject and
BinaryAnnotator::GetEnum, which return nullptr when the index is
negative or beyond the schema's objects() / enums() size. Every
unguarded sink is rewired through these helpers and bails out cleanly
when the index is out of bounds. The BuildStruct / BuildVector /
BuildUnion paths stop traversing when the bound is exceeded; the
BuildTable Obj path emits a generic (unknown) annotation for the
offending field so downstream regions remain well-formed.
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
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.
Bound out-of-range Object/Enum index lookups in
BinaryAnnotatorWhen
flatc --annotateconsumes a.bfbsschema,BinaryAnnotatorwalkseach
Field'sType.indexand dereferencesschema_->objects()->Get(field->type()->index())(and the matching enumsvector) without first validating that the index is in range. The
reflection::VerifySchemaBufferthat runs before annotation only checksstructural integrity of the schema (offsets, sizes, alignment, vector
bounds); it never validates that a
Field.type.indexreferences a slot inschema->objects()orschema->enums().A schema with
Type { base_type = Obj, index = N }whereN >= schema->objects()->size()(and similarly for unions and enum-drivenindexes) reaches one of six unguarded sinks in
binary_annotator.cpp/binary_annotator.h. Each sink readsVector::Get(N)whereNis farbeyond the underlying heap allocation. In a release build without
FLATBUFFERS_ASSERT, the read proceeds past the heap buffer and intoadjacent heap pages; depending on the index value this either crosses into
unmapped memory (SEGV) or reads attacker-influenced heap contents.
The reproducer builds a minimal reflection
Schemawith oneObjectwhose single
FieldcarriesType { base_type = Obj, index = 99999 }.The schema passes
reflection::VerifySchemaBufferbecause everystructural constraint is satisfied. Running
on a debug build with assertions enabled trips
Vector::Get(i < size()); on a release build (or with assertionscompiled out under ASan) the read lands past the end of the
schema->objects()vector and producesSix sinks dereference the unchecked index:
src/binary_annotator.hIsInlineField(Obj base type, struct vs. table)src/binary_annotator.cppBuildTableObj casesrc/binary_annotator.cppBuildStructnested Objsrc/binary_annotator.cppBuildStructarray-of-struct Objsrc/binary_annotator.cppBuildVectorObj elementsrc/binary_annotator.cppBuildUnionenum lookupsrc/binary_annotator.cppBuildUnionObj lookup afterenum_val->union_typesrc/binary_annotator.hGetElementSizeObj element branchThis change adds two helpers,
BinaryAnnotator::GetObjectandBinaryAnnotator::GetEnum, which returnnullptrwhen the index isnegative or beyond the schema's
objects()/enums()size. Everyunguarded sink is rewired through these helpers and bails out cleanly
when the index is out of bounds. The
BuildStruct/BuildVector/BuildUnionpaths stop traversing when the bound is exceeded; theBuildTableObj path emits a generic(unknown)annotation for theoffending field so downstream regions remain well-formed.
The existing
BinaryAnnotator::IsValidUnionValuealready performs thesame kind of check for union value ids; the new
GetObject/GetEnumhelpers extend the same pattern to all object/enum index dereferences.
Reproducer
poc/make_evil_bfbs.cppemits the malicious schema. Build withinclude/on the include path:The produced
evil.bfbsvalidates withreflection::VerifySchemaBufferand crashes an unpatched
flatc --annotate evil.bfbs evil.bfbs(debugbuilds abort on the
Vector::Getassert; release builds with ASanreport a SEGV in
ReadScalarreached throughBinaryAnnotator::IsInlineField). Applying the diff in this changemakes the same invocation exit cleanly.