Repository navigation
Harden reflection Verifier against out-of-bounds reads on truncated buffers, vtable offsets, and unions - #9430
Open
professor-moody wants to merge 1 commit into
Conversation
…uffers, vtable offsets, and unions ## Summary This PR hardens the binary-schema reflection verifier in `src/reflection.cpp` (`flatbuffers::Verify`, `flatbuffers::VerifySizePrefixed`, `VerifyObject`, `VerifyVector`, and `VerifyUnion`) against out-of-bounds reads, null pointer dereferences, and verification bypasses when validating untrusted FlatBuffer payloads against a `reflection::Schema`: 1. **`flatbuffers::Verify` and `flatbuffers::VerifySizePrefixed` root offset and size-prefix validation (`src/reflection.cpp`)**: - `flatbuffers::Verify` previously called `flatbuffers::GetAnyRoot(buf)` before verifying `buf != nullptr`, `length >= FLATBUFFERS_MIN_BUFFER_SIZE`, or `v.VerifyOffset<uoffset_t>(0U)`. Passing `nullptr` or a buffer smaller than 4 bytes performed an out-of-bounds 4-byte read in `GetAnyRoot(buf)`, and a self-referential (`0`) or negative root offset was not checked by `VerifyOffset<uoffset_t>(0U)`. - `flatbuffers::VerifySizePrefixed` previously called `flatbuffers::GetAnySizePrefixedRoot(buf)` without verifying `buf != nullptr`, `length >= sizeof(uoffset_t) + FLATBUFFERS_MIN_BUFFER_SIZE`, or checking that the 4-byte size prefix `ReadScalar<uoffset_t>(buf)` lies within `[FLATBUFFERS_MIN_BUFFER_SIZE, length - sizeof(uoffset_t)]`. It also verified offsets against the outer `length` rather than clamping the verifier length to `sizeof(uoffset_t) + static_cast<size_t>(prefixed_size)`. - Updated `Verify` to check `if (!buf || length < FLATBUFFERS_MIN_BUFFER_SIZE) return false;` and `if (!v.VerifyOffset<uoffset_t>(0U)) return false;` before calling `GetAnyRoot(buf)`. Updated `VerifySizePrefixed` to validate `prefixed_size` against `[FLATBUFFERS_MIN_BUFFER_SIZE, length - sizeof(uoffset_t)]`, keep `Verifier v(buf, sizeof(uoffset_t) + static_cast<size_t>(prefixed_size), max_depth, max_tables)` anchored at `buf` (so 8-byte field alignment is measured from the true start of a `FinishSizePrefixed` buffer, matching `VerifierTemplate::VerifySizePrefixedBuffer`), and check `if (!v.VerifyOffset<uoffset_t>(sizeof(uoffset_t))) return false;` before calling `VerifyObject(v, schema, root, flatbuffers::GetAnySizePrefixedRoot(buf), true)`. 2. **`VerifyObject` offset and `required()` validation on `Obj`, `Union`, and `String` fields (`src/reflection.cpp`)**: - In `VerifyObject`, `case reflection::Obj:` (when `!child_obj->is_struct()`) called `flatbuffers::GetFieldT(*table, *field_def)` directly without first calling `table->VerifyOffset(v, field_def->offset())` (or `VerifyOffsetRequired`). A vtable slot pointing outside the buffer caused `table->GetPointer<const Table*>(field.offset())` to read a 4-byte `uoffset_t` out of bounds before recursive `VerifyObject` ran. - `case reflection::Union:` called `table->GetField<uint8_t>(utype_offset, 0)` and `flatbuffers::GetFieldT(*table, *field_def)` without verifying `utype_offset` or `field_def->offset()`. Because `obj.fields()` in `.bfbs` schemas is sorted alphabetically by field name (`key`), a union value field `"u"` is visited before its companion `"u_type"` field in `VerifyObject`, so `utype_offset` has not yet been verified when `case reflection::Union:` executes. - `case reflection::String:` used `table->VerifyField<uoffset_t>(v, field_def->offset(), sizeof(uoffset_t))` instead of `VerifyOffset` / `VerifyOffsetRequired`, which did not enforce `field_def->required()` when a required string field was omitted from the vtable and did not validate the `uoffset_t` target before `GetFieldS` resolved the pointer. - Updated `String`, non-struct `Obj`, and `Union` fields in `VerifyObject` to validate field offsets with `VerifyOffsetRequired` / `VerifyOffset` (and `VerifyField<uint8_t>` for `utype_offset`, after checking `field_def->offset() >= sizeof(voffset_t)`) prior to dereferencing. 3. **`VerifyVector` offset validation, `required()` enforcement, and union type-vector verification (`src/reflection.cpp`)**: - `VerifyVector` previously checked `table.VerifyField<uoffset_t>(v, vec_field.offset(), sizeof(uoffset_t))` rather than `VerifyOffsetRequired` / `VerifyOffset`, ignoring `vec_field.required()` when a required non-struct vector was omitted from the vtable and not validating the vector offset before `GetFieldV` / `GetFieldAnyV` resolved the pointer. - In `case reflection::Union:`, `VerifyVector` called `table.GetPointer<Vector<uint8_t>*>(vec_field.offset() - sizeof(voffset_t))` without verifying the companion `_type` vector offset (`table.VerifyOffsetRequired(v, utype_vec_offset)`). When the companion `_type` vector vtable slot pointed out of bounds, `GetPointer` read 4 bytes out of bounds; when the `_type` vector vtable slot was `0` (`type_vec == nullptr`), `v.VerifyVector(type_vec)` returned `true` and `type_vec->size()` immediately dereferenced a null pointer. - In `case reflection::String:`, `case reflection::Obj:`, and `case reflection::Union:`, element offset entries inside the vector (`vec->Data() + j * sizeof(uoffset_t)`) are now validated with `v.VerifyOffset(vec->Data() + j * sizeof(uoffset_t), 0)` before `vec->Get(j)` resolves the element offset. 4. **`VerifyUnion` consistency, sparse enum lookup, and struct alignment (`src/reflection.cpp`)**: - `VerifyUnion` now checks `if (!utype) return !elem;` and `if (!elem) return false;`, rejecting both `(utype == 0 && elem != nullptr)` and `(utype != 0 && elem == nullptr)`. Note that rejecting both inconsistent states is intentionally stricter than `flatc`-generated union verifiers: existing reflection consumers in `src/reflection.cpp` (`CopyTable` and `ResizeContext`) check `table.CheckField(fielddef.offset())` on the union value field and call the existing `GetUnionType` helper (which indexes `schema.objects()->Get(-1)` when `utype == 0`, as in google#9416 and google#9417), and when `utype != 0 && elem == nullptr`, `v.VerifyString(nullptr)` previously returned `true` for null `String` union members. - `VerifyUnion` previously indexed `fb_enum->values()->Get(utype)` by raw position instead of `fb_enum->values()->LookupByKey(utype)` (consistent with existing `GetUnionType` in `src/reflection.cpp`) and did not check `enumval->union_type() != nullptr`. - For struct union members (`elem_obj->is_struct()`), `VerifyUnion` now calls `v.VerifyFieldStruct(elem, 0, elem_obj->bytesize(), elem_obj->minalign())` so struct alignment is verified in addition to byte size. ## Testing - Added regression tests in `tests/reflection_test.cpp` (`ReflectionTest`) covering null, `< 4`-byte, and `< 8`-byte buffers, positive `FinishSizePrefixed` `Monster` verification and size-prefixed frame mismatches, out-of-bounds vtable slots on sub-table (`Obj`) and union (`UType` / `Union`) fields, omitted required `String` fields, and inconsistent union `utype` / `elem` pairs. - Verified with a standalone C++17 regression suite across Release (`-O2 -DNDEBUG`) and AddressSanitizer + UndefinedBehaviorSanitizer (`-fsanitize=address,undefined -fno-omit-frame-pointer -g -O1`) builds.
This branch has not been deployed
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.
Summary
This PR hardens the binary-schema reflection verifier in
src/reflection.cpp(flatbuffers::Verify,flatbuffers::VerifySizePrefixed,VerifyObject,VerifyVector, andVerifyUnion) against out-of-bounds reads, null pointer dereferences, and verification bypasses when validating untrusted FlatBuffer payloads against areflection::Schema:flatbuffers::Verifyandflatbuffers::VerifySizePrefixedroot offset and size-prefix validation (src/reflection.cpp):flatbuffers::Verifypreviously calledflatbuffers::GetAnyRoot(buf)before verifyingbuf != nullptr,length >= FLATBUFFERS_MIN_BUFFER_SIZE, orv.VerifyOffset<uoffset_t>(0U). Passingnullptror a buffer smaller than 4 bytes performed an out-of-bounds 4-byte read inGetAnyRoot(buf), and a self-referential (0) or negative root offset was not checked byVerifyOffset<uoffset_t>(0U).flatbuffers::VerifySizePrefixedpreviously calledflatbuffers::GetAnySizePrefixedRoot(buf)without verifyingbuf != nullptr,length >= sizeof(uoffset_t) + FLATBUFFERS_MIN_BUFFER_SIZE, or checking that the 4-byte size prefixReadScalar<uoffset_t>(buf)lies within[FLATBUFFERS_MIN_BUFFER_SIZE, length - sizeof(uoffset_t)]. It also verified offsets against the outerlengthrather than clamping the verifier length tosizeof(uoffset_t) + static_cast<size_t>(prefixed_size).Verifyto checkif (!buf || length < FLATBUFFERS_MIN_BUFFER_SIZE) return false;andif (!v.VerifyOffset<uoffset_t>(0U)) return false;before callingGetAnyRoot(buf). UpdatedVerifySizePrefixedto validateprefixed_sizeagainst[FLATBUFFERS_MIN_BUFFER_SIZE, length - sizeof(uoffset_t)], keepVerifier v(buf, sizeof(uoffset_t) + static_cast<size_t>(prefixed_size), max_depth, max_tables)anchored atbuf(so 8-byte field alignment is measured from the true start of aFinishSizePrefixedbuffer, matchingVerifierTemplate::VerifySizePrefixedBuffer), and checkif (!v.VerifyOffset<uoffset_t>(sizeof(uoffset_t))) return false;before callingVerifyObject(v, schema, root, flatbuffers::GetAnySizePrefixedRoot(buf), true).VerifyObjectoffset andrequired()validation onObj,Union, andStringfields (src/reflection.cpp):VerifyObject,case reflection::Obj:(when!child_obj->is_struct()) calledflatbuffers::GetFieldT(*table, *field_def)directly without first callingtable->VerifyOffset(v, field_def->offset())(orVerifyOffsetRequired). A vtable slot pointing outside the buffer causedtable->GetPointer<const Table*>(field.offset())to read a 4-byteuoffset_tout of bounds before recursiveVerifyObjectran.case reflection::Union:calledtable->GetField<uint8_t>(utype_offset, 0)andflatbuffers::GetFieldT(*table, *field_def)without verifyingutype_offsetorfield_def->offset(). Becauseobj.fields()in.bfbsschemas is sorted alphabetically by field name (key), a union value field"u"is visited before its companion"u_type"field inVerifyObject, soutype_offsethas not yet been verified whencase reflection::Union:executes.case reflection::String:usedtable->VerifyField<uoffset_t>(v, field_def->offset(), sizeof(uoffset_t))instead ofVerifyOffset/VerifyOffsetRequired, which did not enforcefield_def->required()when a required string field was omitted from the vtable and did not validate theuoffset_ttarget beforeGetFieldSresolved the pointer.String, non-structObj, andUnionfields inVerifyObjectto validate field offsets withVerifyOffsetRequired/VerifyOffset(andVerifyField<uint8_t>forutype_offset, after checkingfield_def->offset() >= sizeof(voffset_t)) prior to dereferencing.VerifyVectoroffset validation,required()enforcement, and union type-vector verification (src/reflection.cpp):VerifyVectorpreviously checkedtable.VerifyField<uoffset_t>(v, vec_field.offset(), sizeof(uoffset_t))rather thanVerifyOffsetRequired/VerifyOffset, ignoringvec_field.required()when a required non-struct vector was omitted from the vtable and not validating the vector offset beforeGetFieldV/GetFieldAnyVresolved the pointer.case reflection::Union:,VerifyVectorcalledtable.GetPointer<Vector<uint8_t>*>(vec_field.offset() - sizeof(voffset_t))without verifying the companion_typevector offset (table.VerifyOffsetRequired(v, utype_vec_offset)). When the companion_typevector vtable slot pointed out of bounds,GetPointerread 4 bytes out of bounds; when the_typevector vtable slot was0(type_vec == nullptr),v.VerifyVector(type_vec)returnedtrueandtype_vec->size()immediately dereferenced a null pointer.case reflection::String:,case reflection::Obj:, andcase reflection::Union:, element offset entries inside the vector (vec->Data() + j * sizeof(uoffset_t)) are now validated withv.VerifyOffset(vec->Data() + j * sizeof(uoffset_t), 0)beforevec->Get(j)resolves the element offset.VerifyUnionconsistency, sparse enum lookup, and struct alignment (src/reflection.cpp):VerifyUnionnow checksif (!utype) return !elem;andif (!elem) return false;, rejecting both(utype == 0 && elem != nullptr)and(utype != 0 && elem == nullptr). Note that rejecting both inconsistent states is intentionally stricter thanflatc-generated union verifiers: existing reflection consumers insrc/reflection.cpp(CopyTableandResizeContext) checktable.CheckField(fielddef.offset())on the union value field and call the existingGetUnionTypehelper (which indexesschema.objects()->Get(-1)whenutype == 0, as in [C++] Object API: Pack() dereferences null when a union has its type set but no value (buffer passes the verifier) #9416 and Fix null dereference in generated C++ union Pack when value is absent #9417), and whenutype != 0 && elem == nullptr,v.VerifyString(nullptr)previously returnedtruefor nullStringunion members.VerifyUnionpreviously indexedfb_enum->values()->Get(utype)by raw position instead offb_enum->values()->LookupByKey(utype)(consistent with existingGetUnionTypeinsrc/reflection.cpp) and did not checkenumval->union_type() != nullptr.elem_obj->is_struct()),VerifyUnionnow callsv.VerifyFieldStruct(elem, 0, elem_obj->bytesize(), elem_obj->minalign())so struct alignment is verified in addition to byte size.Testing
tests/reflection_test.cpp(ReflectionTest) covering null,< 4-byte, and< 8-byte buffers, positiveFinishSizePrefixedMonsterverification and size-prefixed frame mismatches, out-of-bounds vtable slots on sub-table (Obj) and union (UType/Union) fields, omitted requiredStringfields, and inconsistent unionutype/elempairs.-O2 -DNDEBUG) and AddressSanitizer + UndefinedBehaviorSanitizer (-fsanitize=address,undefined -fno-omit-frame-pointer -g -O1) builds.