[Rust] Verify nested_flatbuffer fields - #9230
Open
sanil18 wants to merge 1 commit into
Open
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. |
sanil18
force-pushed
the
fix-nested-flatbuffer-verification
branch
2 times, most recently
from
September 9, 2026 03:46
b352dec to
b51c883
Compare
A field carrying the `nested_flatbuffer` attribute was verified only as `Vector<u8>`. The generated `..._nested_flatbuffer()` accessor follows those bytes as a FlatBuffer root without any further checking, so on a buffer that `root()` returned Ok for, every offset inside the nested buffer was still attacker-controlled. The consequence is that safe generated accessors read outside the buffer's allocation. Under Miri, `Monster::hp()` on such a buffer reports "attempting to access 2 bytes, but got alloc+0x9f which is only 1 byte from the end of the allocation". `read_scalar` guards that read with a debug_assert only, so release builds read past the end. A present-but- empty nested vector was likewise accepted and then panicked in the accessor. This contradicts docs/source/languages/rust.md, which states that the safe APIs are "intended to be safe for use on flatbuffers from untrusted sources", that "All of the safe Rust APIs ensure the verifier is run over these flatbuffers before accessing them", and that the generated accessors "access memory without any further bounds checking". C++ has verified nested buffers by default for years via Verifier::VerifyNestedFlatBuffer, so buffers Rust accepted here are already rejected by any C++ peer. Adds VerifierOptions::check_nested_flatbuffers (default true, matching C++), a NestedFlatBuffer<T> verification marker, and emits it from the Rust generator for the verifier slot of such fields. Accessor return types are unchanged. Offsets inside a nested buffer are relative to its own start, so the verifier runs against the nested slice: `self.buffer` is swapped for it and restored afterwards. Swapping rather than building a second Verifier keeps depth, num_tables and apparent_size accumulating in one place, so a chain of nested buffers cannot reset them. That matters: porting the C++ design literally, with a fresh verifier and fresh budgets per level, would introduce a new DoS. Measured on a 200-level chain of nested Monsters, budgets reset gives "thread 'main' has overflowed its stack"; budgets carried gives DepthLimitReached on the same input. Nested bytes are also billed to apparent_size rather than verified for free, so nesting cannot multiply the verification work a small message buys. Tested: Rust suite and no_std suite pass (332 each); Miri clean on the nested paths; serde and both alloc checks unchanged; generated output verified byte-identical to patched-flatc output. Fixes the root cause reported in google#8291.
sanil18
force-pushed
the
fix-nested-flatbuffer-verification
branch
from
September 9, 2026 06:25
b51c883 to
438030c
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
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.
The problem
A field with the
nested_flatbufferattribute is verified only asVector<u8>.The generated
..._nested_flatbuffer()accessor then follows those bytes as aFlatBuffer root with no further checking — so on a buffer
flatbuffers::root()has already returned
Okfor, every offset inside the nested buffer is stillattacker-controlled.
The result is that safe generated accessors read outside the buffer's
allocation.
Reproduction
Against the published
flatbuffers = "25.12.19"crate, from a binary that is#![forbid(unsafe_code)]:Under Miri, with
-Zmiri-disable-stacked-borrowsso this is a spatial error andnot an aliasing technicality:
read_scalarbounds-checks with adebug_assert!only, so release builds readpast the end. In debug the same input trips that assert with
insufficient capacity for emplace_scalar, needed 2 got 1— showing directly that 2 bytes areread from a 1-byte slice.
A present-but-empty nested vector has the same root cause: accepted by the
verifier, then panics in the accessor with
range end index 4 out of range for slice of length 0.Why this is a soundness bug rather than hardening
docs/source/languages/rust.mdpromises exactly what this breaks:And the generated accessor carries this safety comment today (unchanged by this
PR):
Nothing established "contains a valid flatbuffer in this slot". This PR makes
that comment true.
C++ already does this
Verifier::VerifyNestedFlatBufferruns a nested verifier, gated oncheck_nested_flatbuffers, which defaults totrue. A buffer Rust acceptstoday is already rejected by every C++ peer, so this brings Rust in line rather
than inventing a policy.
The change
Runtime (
rust/flatbuffers/src/verifier.rs)VerifierOptions::check_nested_flatbuffers, defaulttrue— same name anddefault as C++. It replaces the existing
// Ignore nested flatbuffers, etc?TODO.
NestedFlatBuffer<T>: a verification marker implementingVerifiablethatchecks the
[ubyte]vector, then the buffer inside it. It deliberately doesnot implement
Follow, so it can only ever be used for verification.Verifier::verify_nested_buffer, which runs a verifier over the nested slice.Generator (
src/idl_gen_rust.cpp)Emit
ForwardsUOffset<NestedFlatBuffer<Root>>instead ofForwardsUOffset<Vector<u8>>for the verifier slot of such a field. Accessorreturn types are untouched — 4 lines change across the two generated files.
Generated code was regenerated with the patched
flatcso the effect of thegenerator change is visible in the diff, and
scripts/clang-format-git.shleaves the C++ change unmodified.
Two details worth reviewing
Budgets cross the nesting boundary. Offsets inside a nested buffer are
relative to its own start, so the verifier runs against the nested slice:
self.bufferis swapped for it and restored afterwards. Swapping rather thanbuilding a second
Verifierkeepsdepth,num_tablesandapparent_size—and anything added later — accumulating in one place, with no per-field copy to
keep in sync.
That matters, because porting the C++ design literally (fresh verifier, fresh
budgets per level) would introduce a new DoS: every level resets
max_depth.Measured on a 200-level chain of nested Monsters — budgets reset:
thread 'main' has overflowed its stack; budgets carried:Nested table depth limit reached.on the same input.The same carry stops nesting becoming a work amplifier:
range_in_buffercharges
apparent_sizeandvisit_tablechargesnum_tables, so nested bytesare billed rather than verified for free.
nested_flatbuffer_work_counts_toward_apparent_size,nested_flatbuffer_depth_counts_toward_max_depthandnested_flatbuffer_tables_count_toward_max_tablesassert this by measuring thebudgets rather than hard-coding thresholds.
No lookup that can fail quietly. The generator uses
field.nested_flatbuffer, the root type the parser already resolved, ratherthan re-resolving the attribute string. A name lookup that missed would fall
back to
Vector<u8>and silently emit an unverified field — the exact failuremode this PR exists to remove.
Compatibility
Verifiableslot differs.unsafeadded: the patch adds zero lines containingunsafe.(
>= FLATBUFFERS_MIN_BUFFER_SIZE), and it previously panicked in the accessor,so nothing working depended on it.
puts the attribute on a field but stores bytes that are not a flatbuffer of
that type, and only ever reads them through the plain
[ubyte]accessor. Ithink rejecting that is correct — the attribute is a declaration about the
contents, and C++ has enforced it by default for years — but it is a real
change, and it is a one-line opt-out.
VerifierOptionsis not#[non_exhaustive], so adding a public field istechnically breaking for struct-literal construction. Everything in-tree uses
..Default::default(). Flagging rather than adding#[non_exhaustive]unilaterally — happy to if you'd prefer.
Tests
integration_test.rs— valid nested buffer still accepted; invalid UTF-8,out-of-bounds offsets and truncation rejected; opt-out works; empty/absent
pinned; a >200 KB nested payload still verifies; depth,
max_tablesandmax_apparent_sizeall shown to count nested work.nested_flatbuffer_invariant.rs— a deterministic 4,000-mutation sweep over theattacker-controlled payload asserting every buffer the verifier accepts survives
a full traversal without panicking or yielding a non-UTF-8
&str(231 accepted,0 misbehaved). A second sweep corrupts the outer buffer 20,000 times and
asserts verification always returns rather than panicking.
The budget tests each carry a control asserting the same budget suffices when
the nested check is off, and the invariant sweep carries
control_disabling_nested_check_reintroduces_the_problem, which asserts the oldpath still does misbehave. If these ever stop reaching the behaviour they
guard, the suite goes red instead of passing vacuously.
The four sweep/large-payload tests are
#[cfg(not(miri))] // slow., matchingexisting use in this crate, since
RustTest.shrunscargo miri testover thewhole suite. The targeted nested tests still run under Miri and pass clean.
Credit
#8291 reported this root cause in April 2024, framed as a panic, and was
auto-closed as stale. Credit to @Nekrolm.
Noticed while working here, not addressed:
Verifier::reset()assignsnum_tablestwice and never clearsapparent_size. It has no in-tree callersand only makes a reused verifier stricter, so it is not a soundness issue —
happy to send separately.