fix: prevent OOB read in GetBufferStartFromRootPointer backward walk#9154
Open
samrigby64 wants to merge 1 commit into
Open
fix: prevent OOB read in GetBufferStartFromRootPointer backward walk#9154samrigby64 wants to merge 1 commit into
samrigby64 wants to merge 1 commit into
Conversation
|
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. |
The backward pointer walk can read before the start of the buffer when `root` is close to the beginning of the allocation and the loop iterates the full FLATBUFFERS_MAX_ALIGNMENT / sizeof(uoffset_t) + 1 steps. Changes: - Add a `search_limit` lower bound in the existing single-argument overload so the loop breaks before the pointer can underflow below the region that could plausibly contain the buffer start. - Add a new two-argument overload `GetBufferStartFromRootPointer(root, buf, buf_size)` that accepts the known buffer boundaries and uses the exact buffer start as the hard lower bound, making the walk provably safe. Reported by: Sam Rigby (samrigby432@outlook.com)
samrigby64
force-pushed
the
fix/get-buffer-start-oob-read
branch
from
71a0bc7 to
92ca218
Compare
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.
Problem
GetBufferStartFromRootPointerwalks backward fromstartbysizeof(uoffset_t)each iteration, up toFLATBUFFERS_MAX_ALIGNMENT / sizeof(uoffset_t) + 1steps (~65 iterations on typical platforms). There is no lower bound check, so on buffers located close to the start of the address space the pointer can underflow past the allocation's beginning, triggering an out-of-bounds read (CWE-125).The existing comment already calls this out: "Assert, because calling this function with bad data may cause reads outside of buffer boundaries." This PR addresses it.
Changes
Single-argument overload (existing API, no breaking change)
Adds a
search_limitlower bound calculated from the initialstartposition so the loop terminates before walking more thanFLATBUFFERS_MAX_ALIGNMENTbytes backward — the maximum distance a valid FlatBuffer start could ever be fromstart.Two-argument overload (new, safe API)
Adds
GetBufferStartFromRootPointer(root, buf, buf_size)that accepts the known buffer boundaries and uses the exactbuf_startpointer as the hard lower bound. This makes the function safe to call even for buffers at low addresses.Testing
No behavioral change for valid buffers; the loop exits the same way as before on well-formed input. The new overload adds a compile-time–detectable safety improvement for callers that have the buffer size available.