reflection: reject out-of-bounds shrink in ResizeContext#9132
Open
foodlook wants to merge 1 commit into
Open
Conversation
ResizeContext shrinks a buffer with
buf_.erase(buf_.begin() + start + delta_, buf_.begin() + start);
When delta_ is negative (shrinking) and start + delta_ < 0, the first
iterator points before begin(); std::vector::erase then memmoves the
trailing elements to a destination before the allocation, an out-of-bounds
heap write (CWE-787).
This is reachable from the public reflection helper SetString(): it sets
start = str_start + 4 and delta = new_len - old_len, so a string whose length
exceeds its own byte offset + 4 + the replacement length underflows. A buffer
that passes flatbuffers::Verify() is sufficient to trigger it, e.g. shrinking
a large string that sits near the front of the buffer.
Add a bounds check before any offset rewriting so an out-of-range request is
rejected and the buffer is left untouched, instead of corrupting the heap.
Legitimate grow/shrink edits are unaffected (the vector resize path keeps
working; its erase range start + delta is always >= 0).
Adds ResizeContextShrinkBoundsTest, which under AddressSanitizer aborts on the
unpatched code and passes with this fix, and also checks that a normal vector
shrink still succeeds.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
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.
What
ResizeContext(insrc/reflection.cpp) shrinks a buffer withWhen
start + delta_is negative, the first iterator points beforebegin()and
std::vector::erasememmoves the trailing elements to a destination beforethe allocation — a heap out-of-bounds write.
How it's reached
The public reflection helper
SetString()setsstart = str_start + 4anddelta = new_len - old_len. A string longer than its own byte offset + 4 + thereplacement length underflows on shrink. The buffer does not need to be
malformed — a normal buffer that passes
flatbuffers::Verify()(e.g. onedominated by a large string near the front) triggers it when an application
edits it in place with
SetString(schema, "x", big_string, &buf).Fix
Bounds-check the resize range before rewriting any offsets, so an out-of-range
request is rejected without partially mutating the buffer. The check must
precede
Straddle()/ResizeTable(), which would otherwise already haverewritten every offset by
delta_and left the buffer inconsistent.Legitimate grow/shrink edits are unaffected — the
ResizeAnyVectorpathcomputes
startat the end of the vector, so itsstart + deltais always>= 0.Tests
Adds
ResizeContextShrinkBoundsTest(intests/reflection_test.cpp, registeredin
tests/reflection_test.h, called fromtests/test.cpp). Built with-DFLATBUFFERS_CODE_SANITIZE=ON, the test aborts with an AddressSanitizer heapout-of-bounds on the current code and passes with this change; it also asserts
that a normal vector shrink still works.
flattestsis green with the fix(
ALL TESTS PASSED).