Repository navigation
Conversation
…nknown discriminants The Rust verifier accepted vtables shorter than the two mandatory header VOffsetTs, and the generated verifier for unions silently accepted values whose discriminant was unknown. Both could be read out of bounds through the safe API.
|
@vglavnyy - flagging this for review since you're listed as a known contributor for the flatbuffers project in OSS-Fuzz. Context: the corresponding fuzz target for this fix is google/oss-fuzz#16202 - a Rust target that exercises the verifier plus the public safe reader path (root::() -> table -> vtable -> inline size). On the unfixed crate it reproduces an out-of-bounds read (Miri: Both PRs are small and self-contained: this one adds the verifier hardening plus regression tests, and the OSS-Fuzz one only adds the target for |
|
@aardappel - adding a maintainer ping here (I'd pinged @vglavnyy earlier). Summary: the Rust verifier accepts a buffer that then makes the public SAFE API read out of bounds (degenerate vtable / union value with unknown discriminant); the fix rejects those and verifies the union value slot, with 6 regression tests. Corresponding oss-fuzz target: google/oss-fuzz#16202. Full ASan reproduction is on the Buganizer report. Thanks for the look. |
|
A small update that might help move this along: the sibling fix from the same batch just landed - google/filament#10493 (same class of bug: an out-of-range binding reaching a fixed-size descriptor structure) was merged today after three maintainer approvals. This one is the Rust verifier soundness fix, with 6 regression tests, green CLA and green checks - it has been waiting for a reviewer for a while now. Would one of you be able to take a look? Happy to adjust anything you'd like. |
|
@dbaileychess @aardappel - one more nudge here, with some context that may help: the sibling fix from the same batch (google/filament#10493, same class of bug) was merged two days after three maintainer approvals, so this kind of fix does land. This one is a two-file change in the Rust path (verifier.rs + the generator) with six regression tests; CLA and all checks are green, and it has been waiting for a first review since Sept 29. Would one of you have a moment to look? Also, if it is more useful, I am happy to contribute the equivalent coverage as an in-repo Rust fuzz target (we already have a working one that drives the safe verifier path) - just say which you prefer. |
Problem
A buffer that passes the official Rust verifier
flatbuffers::root::<T>()can still make thepublic safe API read out of bounds. Under Miri this surfaces as
Undefined Behavior: memory access failed ... beyond the end of the allocation.That violates the core soundness contract of a verifier: once verification succeeds, every
read performed through the safe accessors must stay inside the buffer.
Root cause
Two independent holes in the Rust path.
Degenerate vtables.
Verifier::visit_tablereadsvtable_lenfrom the buffer and thencalls
range_in_buffer(vtable_pos, vtable_len). Forvtable_len == 0that range checktrivially succeeds (an empty range lies inside any buffer), so the table is accepted. The
safe
Table/VTableAPI then readsobject_inline_num_bytes()atloc + 2(andnum_fields, which would underflow) — addresses never covered by the range check, whichcan lie past the end of the buffer.
Union values with an unknown discriminant. The generated verifier for unions ended in
_ => Ok(()): when the discriminant was not one of the known variants, the union payloadwas never verified. A Rust reader still interprets a union value as
ForwardsUOffset<Table>— that is what the generated_as_*/ union accessors construct —so the safe API builds and traverses a
Tableat an offset the verifier never checked.Fix (3 changes)
rust/flatbuffers/src/verifier.rs:visit_tablenow rejects a vtable shorter than the twomandatory header
VOffsetTs(vtable_len < 2 * SIZE_VOFFSET), returning arange-out-of-bounds error before anything is read through that vtable.
rust/flatbuffers/src/verifier.rs: newimpl Verifiable for Table<'a>, which verifies atable without knowing its schema — the
soffsetatposmust dereference in bounds and thevtable it points to must lie completely inside the buffer.
src/idl_gen_rust.cpp: for unions, the unknown-discriminant arm now emitsv.verify_union_variant::<::flatbuffers::ForwardsUOffset<::flatbuffers::Table<'static>>>("unknown", pos)instead of
_ => Ok(()), so the payload is verified as a table before any safe accessor canreach it.
How it was found
A differential harness over generated Rust readers: the same inputs are run against the
generated verifier and against the safe accessors under Miri, and cases where verification
succeeds but a safe accessor reads out of bounds are minimized.
Verification status
by verification.
cargo test --libin therust/flatbufferscrate: 6 passed, 1 failed. That failing testfails identically on unpatched upstream
master, i.e. it is pre-existing and unrelated tothis change.
Happy to add regression tests for both cases if you would like them — say the word and I will
push them to this branch.