security: fix integer overflow and OOB writes in reflection.cpp#9065
Open
sharadboni wants to merge 2 commits intogoogle:masterfrom
Open
security: fix integer overflow and OOB writes in reflection.cpp#9065sharadboni wants to merge 2 commits intogoogle:masterfrom
sharadboni wants to merge 2 commits intogoogle:masterfrom
Conversation
Three security fixes for the reflection mutation API: 1. ResizeAnyVector: signed integer overflow in delta_bytes The computation `delta_elem * elem_size` used int × int which overflows when the product exceeds INT_MAX (e.g., 1M elements × 4096-byte elem). Use int64_t arithmetic and reject overflow before passing to ResizeContext. 2. SetString: unchecked memset length before ResizeContext The memset clearing the old string used str->size() from the FlatBuffer without validating that start+str->size() <= flatbuf->size(). Added the bounds check so only valid in-bounds bytes are zeroed. 3. ForAllFields: unchecked field->id() index field_to_id_map.resize(object->fields()->size()) but field->id() values from an untrusted FlatBuffer can exceed that size, causing OOB write. Added a bounds guard before the assignment. All three bugs are ASan/UBSan verified.
Cast index to uint64_t before multiplying by element_stride to prevent uint32_t overflow when computing large offsets in IndirectHelper<OffsetT<T>>::Read. Previously, i * element_stride (both uint32_t) could wrap around, producing a pointer far outside the buffer and causing a SIGSEGV.
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
Three security fixes for the reflection mutation API, all verified with ASan/UBSan.
Fix 1: Signed integer overflow in
ResizeAnyVector(CVE candidate)delta_bytes = delta_elem * elem_sizeusedint × intarithmetic which overflows whendelta_elem × elem_size > INT_MAX. For example, a vector with 1M elements of 4096-byte structs triggers the overflow. The resulting negativedelta_bytesis passed toResizeContext, causing a massive heap underallocation followed by OOB write.Fix: Use
int64_tfor the multiplication and reject overflow:Fix 2: Unchecked
memsetlength inSetStringBefore resizing,
SetStringcallsmemset(flatbuf->data() + start, 0, str->size())to clear the old string. Ifstart + str->size() > flatbuf->size(), this writes past the buffer. Added bounds check before thememset.Fix 3: Unchecked
field->id()index inForAllFieldsfield_to_id_mapis allocated withobject->fields()->size()entries, butfield->id()values from an attacker-controlled FlatBuffer can exceed that size, causing OOB write. Added a bounds guard.Test
All three bugs reproduced with ASan/UBSan on the existing
flatbuffersreflection library. This PR is complementary to #9046 which covers verifier OOB reads.