[rust] reflection: fix out-of-bounds read/write and verifier index corruption

[owvr27]

Out-of-bounds read in the safe reflection API — google/flatbuffers

Summary

The Rust reflection crate exposes a memory-safe API (SafeBuffer → SafeTable /
SafeStruct) that verifies a buffer once and then performs every read through unsafe
code justified by the comment "the buffer was verified during construction". That
justification did not hold. A buffer that passes verification can drive the entirely safe
SafeTable::get_any_field_string() into an out-of-bounds heap read.

Introduced	733e432b, 2025-01-15, PR #8102 — latent 626 days (~20.6 months), never functionally reviewed since
Component	rust/reflection (flatbuffers-reflection v0.1.0, published, ~128k recent downloads)
Type	Memory-safety: out-of-bounds read via a safe API, plus an out-of-bounds write in the setters
Diff	4 files, +398 / −36
Tests	5 new, all fail before / pass after

---

Finding 1 (primary) — out-of-bounds read via the 100% safe API

Location: rust/reflection/src/lib.rs, get_field_loc(), get_any_value_integer(),
get_any_value_float()

get_field_loc computed a field's absolute location from the vtable inside the buffer
being read and returned it without comparing it against the buffer length:

let field_offset = table.vtable().get(field.offset()) as usize;
if field_offset == 0 { return None; }
Some(table.loc() + field_offset)      // attacker-influenced, unchecked

All three get_any_field_* accessors then read a scalar there, and the scalar readers
dispatched straight to Follow::follow. The underlying primitive guards only with a
debug_assert (flatbuffers/src/endian_scalar.rs:173), which is compiled out in release:

debug_assert!(s.len() >= size, "insufficient capacity for emplace_scalar, ...");
core::ptr::copy_nonoverlapping(s.as_ptr(), mem.as_mut_ptr() as *mut u8, size);

So in an optimised build this is an unchecked copy_nonoverlapping reading up to 8 bytes
from an attacker-influenced offset — and those bytes are then returned to the caller inside
a String.

Reachability. get_any_value_integer is reached from the catch-all arm of
get_any_value_string (lib.rs:678), which recurses through nested tables and structs and
is called by the safe SafeTable::get_any_field_string(). No unsafe is required on the
caller's side.

Reproducer

51 bytes (harness/regress/oob_read.bin):

0800 0000 1c00 f8b0 0400 0000 0000 0000
0000 1000 0000 0000 0800 0000 1000 0000
0800 0000 0000 5959 5900 0000 0000 0100
0080 00

Built without debug assertions, under AddressSanitizer:

==77895==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x606000000413
READ of size 4 at 0x606000000413 thread T0
    #0 flatbuffers_reflection::get_any_value_integer
    #1 flatbuffers_reflection::get_any_value_string
    #2 flatbuffers_reflection::get_any_field_string
    #3 flatbuffers_reflection::get_any_value_string
    #4 flatbuffers_reflection::get_any_field_string
    #5 flatbuffers_reflection::SafeTable::get_any_field_string      <-- safe public API
    #6 reflection::__libfuzzer_sys_run

With debug assertions on, the same input trips
endian_scalar.rs:173: insufficient capacity for emplace_scalar, needed 4 got 3.

---

Finding 2 — out-of-bounds write in the setters

Location: rust/reflection/src/lib.rs, set_any_value_integer() / set_any_value_float()

The same missing bounds check as Finding 1, on the write path — and it is worse, because
it corrupts memory and reports success. The guard was:

if buf.len() < get_type_size(base_type) {   // is the buffer big enough *somewhere*?

That only proves the buffer is large enough for the type somewhere; it says nothing about
field_loc. The code then does &mut buf[field_loc..] and hands the resulting short slice
to emplace_scalar(), which copies size bytes guarded only by debug_assert!. A field
within size-1 bytes of the end of the buffer is therefore written past the end of the
slice, in release builds, while the function returns Ok(()).

Reproduced with a canary (an 0xAA-filled backing array, so writes past the validated slice
are observable), on the Root.n field of the test schema with the buffer truncated to leave
2 bytes of headroom:

table_loc=16 field_off=4 field_loc=20 size=4
set_any_field_integer -> Ok(())
canary before = [aa, aa, aa, aa, ...]
canary after  = [22, 11, aa, aa, ...]      <- 0x22, 0x11 written past the end
bytes written past end of validated buffer: 2

With the patch:

set_any_field_integer -> Err(VerificationError(RangeOutOfBounds { range: 20..24, .. }))
canary after  = [aa, aa, aa, aa, ...]
bytes written past end of validated buffer: 0

Both setters now validate field_loc + size <= buf.len(), matching the read path.

Note this one was not caught by my first pass: fixing Finding 1's get_field_loc bounds
field_loc <= buf.len() but does not guarantee field_loc + size <= buf.len(), so the
write overflow survived it. Finding it required auditing the set_* family specifically
rather than trusting the read-side fix to cover the class.

Finding 3 — verifier index corruption (schema type confusion)

Location: rust/reflection/src/reflection_verifier.rs:184

SafeBuffer maps buffer position → schema object index so by-name lookups know which
object a location belongs to. verify_table registers the parent; verify_struct then
registered nested structs with an unconditional insert:

let field_pos = struct_pos.saturating_add(field.offset().into());
buf_loc_to_obj_idx.insert(field_pos, obj_idx);   // overwrites the parent

For struct Outer { a: St; b: short; } the child a is at struct offset 0, so
field_pos == struct_pos — the key the caller had just mapped to Outer. The overwrite
rebound the parent to the child's schema object.

before:  outer.get_any_field_integer(b) => Err(FieldNotFound)
         outer.get_any_field_integer(x) => Ok(42)     <-- the child's field
after:   outer.get_any_field_integer(b) => Ok(-3)
         outer.get_any_field_integer(x) => Err(FieldNotFound)

This is not limited to offset 0 — any nested struct whose field offset resolves onto an
already-registered position triggers it. It also breaks ordinary correct usage: the
sanity test in the new test file fails before this change for exactly this reason.

---

Finding 4 — unchecked attacker-controlled index into the schema

Location: rust/reflection/src/reflection_verifier.rs, verify_union()

let enum_value = table_verifier.verifier().get_u8(enum_pos)?;   // attacker-controlled
let enum_type = union_enum.values().get(enum_value.into())      // no bounds check

Vector::get is assert!(idx < self.len()), so a discriminant beyond the enum's value
count panics from inside the safe SafeBuffer::new(). One byte is enough.

---

The fix

Finding	Location	Change
1	lib.rs get_field_loc	Range-check table.loc() + field_offset against buf.len(). Single chokepoint for all eight field accessors — three read paths and five set_* write paths.
1	lib.rs get_any_value_integer / _float	Defence-in-depth check before Follow::follow; needed because the *_in_struct accessors compute the location directly.
2	lib.rs set_any_value_integer / _float	Validate field_loc + size <= buf.len() instead of buf.len() < size, closing the out-of-bounds write.
3	reflection_verifier.rs:184	HashMap::insert → entry().or_insert() so a nested struct cannot rebind its parent.
4	reflection_verifier.rs:367	Range-check the union discriminant before indexing the schema's enum values.
—	tests/RustTest.sh	Wire rust/reflection into CI. It was not being run at all — the script only covered rust_serialize_test, rust_no_std_compilation_test and rust_usage_test.

All fixes report through the existing FlatbufferError / InvalidFlatbuffer::RangeOutOfBounds
plumbing. No API change, no new unsafe, no new dependency; get_field_loc is a private
unsafe fn.

Note the crate already had the correct pattern on the write side — set_any_value_integer
range-checks before emplacing. The read side was simply missing it.

---

Tests

rust/reflection/tests/safe_buffer_regression.rs is self-contained — the reflection
schema for mini.fbs and a valid Root seed buffer are embedded as byte literals, so no
flatc is needed to run it. Cases:

1. seed_is_valid_and_readable — sanity (fails before: Finding 2 breaks normal reads)
2. nested_struct_at_offset_zero_does_not_hijack_parent_lookup
3. out_of_range_union_discriminant_is_rejected_not_panicking
4. no_single_byte_mutation_panics_in_the_safe_read_path — asserts the general property
   rather than one magic byte
5. set_scalar_near_end_of_buffer_is_rejected_not_written — the canary check for Finding 2

	before	after
in-tree tests (5, incl. the OOB-write regression)	0 / 5	5 / 5
external harness	0 / 5	5 / 5

Fuzzing under ASan (two schemas — bench and bench2, the latter covering plain enums,
fixed-size arrays, 64-bit vectors, two independent unions, 3-level struct nesting and
required-field enforcement):

build	executions	crashes
unpatched	~750,000	3
patched	~3,760,000	0

The unpatched crashes on both schemas traced to the same root causes, and the patched
build is clean across both — evidence the fixes generalise rather than patch single inputs.

---

How this got here, and how long it has been there

All three defects arrived in a single commit — the one that introduced the crate:

• 733e432b — 2025-01-15 — "Rust full reflection (Rust full reflection #8102)". rust/reflection did not
  exist before this commit; all three issues are present in its very first version,
  including the // SAFETY: the buffer was verified during construction comments in
  safe_buffer.rs. The safety contract was unsound from day one.

Since then only three commits have touched rust/reflection at all: a dependency /
Android-build change, a bulk formatting-only change, and one unrelated verifier bug fix
(21b706b6, "swapped argument order in new_inconsistent_union calls", #9001). The four
functions this patch touches have never been functionally modified.

That combination — 20+ months, ~5,000 lines added once and then functionally untouched, with
a documented-but-false safety invariant — is why I fixed this at the chokepoint rather than
at the individual reads. There was no prior review pass that could have caught it.

Worth noting: 21b706b6 is a verifier fix in the same function area as Finding 3, merged
via GitHub PR, so there is an established path for a change of exactly this kind.

---

How the bugs were found

The crate had 13 fuzz targets, all C++, and no Rust fuzz target and no unit tests at
all. The 150-unsafe Rust path that parses untrusted buffers had no coverage whatsoever.
The harness used here is offered as that missing coverage.

---

Known limitations, stated deliberately

1. buf_loc_to_obj_idx is keyed by buffer position alone. A struct nested at offset 0
   genuinely shares a position with its parent, so one key can map to two schema objects.
   Fix 2 keeps the parent authoritative — the severe direction, silent wrong values — but
   the child at offset 0 is then no longer resolvable by name through SafeStruct. The
   complete fix is to carry the object index in SafeTable / SafeStruct (derivable from
   field.type_().index() at access time) and retire the map, removing the collision class
   outright. That is an API change and is better discussed than landed silently.
2. SafeTable and SafeStruct are not re-exported. safe_buffer.rs declares them pub
   inside a private module and re-exports only SafeBuffer, so they are returned by public
   methods but cannot be named by callers. Happy to fix in this PR if wanted.
3. Indirect targets are still assumed verified. get_any_value_string's Obj branch
   follows a uoffset and then reads the pointee's vtable, relying on the verifier having
   checked that table. The three findings above were the concrete gaps and the patched build
   is clean over ~3.8M executions, but the general principle — every indirect read
   range-checked at the point of use rather than assumed from verification — is worth a
   maintainer-level audit.

Note on scope

Finding 1 is the security issue (memory-safety, demonstrated with ASan). Finding 2 is an
integrity/correctness bug — its reads stay within the buffer. Finding 3 is a panic. The
change is framed around Finding 1; 2 and 3 are defence-in-depth from the same audit and are
not claimed as separately exploitable.