[Swift] Enforce read bounds in release builds (assert -> precondition) and add missing guards - #9277
Open
fthomasella wants to merge 1 commit into
Open
fthomasella wants to merge 1 commit into
fthomasella wants to merge 1 commit into
Conversation
…) and add missing guards The FlatBuffers/Swift readers (ByteBuffer.read/readSlice/readString, FlexBuffers counterparts, skipPrefix) guarded out-of-bounds reads only with assert(), which compiles away in release builds: reading attacker- controlled FlatBuffers/FlexBuffers without a prior verifier pass then performs silent out-of-bounds reads of heap-adjacent memory (see precedent google#9081). TypedVector subscript and FlexBuffers getRootChecked had no bounds guard at all (the latter still carries a TODO verifier). - Convert the read-path asserts to precondition() so they hold in release (-O) as well; builder-side (write-path) asserts untouched. - skipPrefix: guard the prefix read against undersized buffers. - TypedVector.subscript: guard index against the vector count. - getRootChecked: fail closed on buffers too small to hold any root. - Add in-bounds regression tests for the guarded paths. Demonstration (linux, swift 6, -O): readSlice(0,64) against a 4-byte buffer returns 64 bytes on master (41 42 43 44 + heap-adjacent), traps with this patch.
|
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. |
Author
done |
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.
What
The Swift readers guarded out-of-bounds reads only with
assert(), which compiles away in release builds (-O). On buffers from untrusted input that skip (or predate) the verifier, those reads walk silently past the allocation. Additionally,TypedVector.subscriptand FlexBuffersgetRootCheckedhad no bounds guard at all (the latter still carries a verifier TODO).Follows the direction of #9081 (
[Swift] Fix verifier accepting truncated scalar vectors (OOB read/write, RCE)) — this closes the remaining unguarded read paths.Changes
FlatBuffers/ByteBuffer.swift:read/readSlice/readStringread-path asserts →precondition()(enforced in-O); builder/write-path asserts untouched.skipPrefixnow guards the prefix read on undersized buffers.FlexBuffers/ByteBuffer.swift: same conversion on the read paths.FlexBuffers/Reader/TypedVector.swift:subscriptguardsindex < count(was fully unchecked).FlexBuffers/Reader/Reference.swift:getRootCheckedfails closed on buffers too small to hold any root until a full verifier exists.ByteBufferBoundsRegressionTests).Demonstration (Linux, swift 6.0, -O)
Full before/after transcripts and the tiny demo program: happy to attach if useful.
Testing
swift build+ fullswift testsuite pass (89 existing + 4 new tests), swift 6.0 on Linux (CI matrix covers 6.0/6.1/6.2 + Windows + WASM — the guards are plainpreconditioncalls, no platform-specific behavior).Notes
preconditionkeeps a bounds branch on the hot read path in release. Given [Swift] Fix verifier accepting truncated scalar vectors (OOB read/write, RCE) #9081 established this class as worth fixing in-kernel, I've favored fail-closed over silent OOB; if the perf delta matters for verified-buffer workloads, a follow-up could expose an explicitly-unchecked fast path behind the verifier.