Repository navigation
fix(graph): bound untrusted sizes in the wire decoder (record length + build_count) - #653
Conversation
GraphReader::read_record decoded a record's payload length from an untrusted varint and passed it straight to vec![0u8; len], so a malformed graph could drive a multi-gigabyte allocation and abort the process. A fuzzer reached it with a 6-byte input (03 ad ad ad ad 0a) declaring a ~2.8 GiB payload. The graph arrives over the remote-execution channel and this fires before the stream checksum is verified. Cap a single record at MAX_RECORD_LEN (512 MiB) via a shared check_record_len applied in both the sync and async readers, returning WireError::RecordTooLarge. Seed a regression test with the fuzzer's input. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
Re-fuzzing on top of the record-length bound surfaced a second OOM: the header's untrusted build_count fed Arena/HashMap::with_capacity directly, so a 35-byte input declaring ~97M builds drove a ~280 GiB allocation. Cap the pre-size hint at MAX_PREALLOC_BUILDS; the collections still grow to fit the specs actually decoded. Regression test seeded with the fuzzer input. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Folded in a second fix. Re-fuzzing on top of the record-length bound (which held: 1.3M execs, stable ~200 MB RSS, ~9x more coverage) surfaced a second, distinct OOM — the header's untrusted Fix: cap the pre-size hint at
|
…uzzing guide (#1106) * test: add cargo-fuzz harnesses for the untrusted-decode boundary Adds eight fuzz targets over the code that turns attacker-influenceable bytes into in-memory structures — the sharpest trust boundary in the tree: graph_from_bytes Graph::from_bytes (remote-execution wire format) graph_roundtrip structure-aware from_bytes(to_bytes(g)) == g remote_index_from_reader IndexFile::from_reader (index.shisha) [Linux only] spec_hash_from_hex SpecHash::from_hex target_from_str Target::from_str mfile_from_toml minimal.toml through the custom serde visitors arg_schema_parse ArgSchema::try_from jq_parse_json jq::parse_file, JSON branch Each fuzz/ dir is its own workspace so the nightly + sanitizer build cannot perturb the main one. Three small production hooks are needed: * graph: `Graph::fuzz_roundtrip` behind a new off-by-default `fuzzing` feature (graphs are not hand-constructible from outside the crate), and `insert_build` gated on `any(test, feature = "fuzzing")` rather than test alone. Default builds are unchanged. * mfile: `File::from_toml_bytes`, the pure filesystem-free core of `from_dir`, so the harness need not re-implement it. * workspace: `arbitrary`, optional and only enabled by graph's `fuzzing`. Also carries the graph corpus seeds; a valid graph-with-local-file seed is what let the fuzzer reach the local-file frame decoder. This is the tooling behind the six decoder fixes already merged in #651, #653, #656, #661 and #693. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * build(justfile): add fuzz-check bitrot guard and a fuzz runner Each fuzz/ dir declares its own [workspace], so no workspace-wide build ever compiles the fuzz targets and they rot silently as the crates they fuzz evolve. The first version of this suite went 253 commits before anyone rebuilt it, by which point a target referenced a renamed type. `just fuzz-check` is the guard: a plain `cargo check` over every fuzz workspace. No nightly, no sanitizer, no libFuzzer runtime — just "does this still compile against today's API", so it runs anywhere and is cheap enough to treat like a red build. `just fuzz <crate> <target> [args]` runs one target, applying the RSS cap that turns an unbounded-allocation bug into a catchable crash rather than an ambient OOM. The rcache target is excluded on macOS (rcache -> lcache -> the Linux-only common::renameat2), following the existing `scope` idiom. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: add the fuzzing guide Covers prerequisites, the eight targets and their trust levels, running via the just recipes, corpus seeding (and why seeding is what unlocks the deep decode paths), reproducing and minimizing a crash, keeping targets from bitrotting, the bugs the campaign found, and where to take it next. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
What
GraphReader::read_recorddecoded a record's payload length from an untrusted LEB128 varint and passed it straight tovec, with no upper bound. A malformed graph could therefore declare a huge length and drive a multi-gigabyte allocation that aborts the process.This is NET-reachable: the build graph arrives over the remote-execution channel (
Graph::from_bytes), and the allocation happens before the stream checksum is verified, so a valid checksum is not required to trigger it.How it was found
The new
graph_from_bytescargo-fuzz target (separate branch) hit it in ~672 executions with a 6-byte input:0x03is a record tag;ad ad ad ad 0ais the length varint, which decodes to ~2.8 GiB →libFuzzer: out-of-memory (malloc(2779469485)).Fix
MAX_RECORD_LEN(512 MiB) — comfortably above any real build-spec or inlined local-file record.check_record_len, shared by the sync and async readers so they stay in lock-step, returning a newWireError::RecordTooLarge { len, max }before allocating.oversized_record_length_is_rejected_not_allocatedseeded with the exact fuzzer input, asserting a cleanWireErrorrather than an OOM.Validation
cargo test -p graph --lib wire— 18 passed (including the new regression test);cargo clippy -p graph --lib -- -D warningsclean. Relying on CI for the full sandbox run.Follow-ups (not in this PR)
materialize_local_file) and an unsanitised entryfilename; the fuzzer couldn't reach them until this OOM was fixed. Re-fuzzing on top of this should surface them next.🤖 Generated with Claude Code