fix(graph): bounds-check local-file records and reject unsafe names by bryan-minimal · Pull Request #656 · gominimal/minimal

bryan-minimal · 2026-07-07T20:52:11Z

Second batch of graph wire-decoder hardening, found by continuing the fuzz→fix→re-fuzz loop on top of #653 (which this stacks on and which fixed the two allocation OOMs).

Findings (both in `materialize_local_file`)

H1 — out-of-bounds slice panic (NET-reachable). The payload was sliced with a wire-supplied header_len (varint) and data_len (JSON) without bounds checks:

serde_json::from_slice(&payload[pos..pos + header_len])   // pos+header_len can exceed payload.len()
let data_end = data_start + file_header.data_len as usize; // unchecked, can overflow
let file_data = &payload[data_start..data_end];

Once #653 removed the allocation OOMs that masked it, seeding the fuzz corpus with a valid graph-that-inlines-a-local-file let libFuzzer reach and corrupt these fields:

panicked at wire.rs: range end index 315 out of range for slice of length 207
==ERROR: libFuzzer: deadly signal

H3 — filename path traversal. The wire-controlled filename was joined onto the temp dir unsanitised (subdir.join(&file_header.filename)), so ../absolute names could escape the per-file directory.

Fix

Validate every wire-controlled offset stays within payload (checked_add + <= len), returning a clean WireError instead of slicing out of bounds — covers both the overflow and the out-of-range cases.
Reject filenames that are absolute or contain ../root/prefix components before joining.
Regression tests drive materialize_local_file directly for each; the existing local-file round-trips still pass (legit bare names are unaffected).

Fuzzing status

After these fixes, re-fuzzing runs clean: 1.49M execs, coverage 3405 (up from 152 at the start of the campaign), RSS stable ~290 MB, no new crashes — the fuzzer-reachable crash surface of the decoder is now hardened. The harness lives on test/fuzz-graph-wire.

Validation

cargo test -p graph --lib wire → 21 passed; cargo clippy -p graph --lib -- -D warnings clean.

🤖 Generated with Claude Code

Summary by CodeRabbit

Bug Fixes
- Improved validation of uploaded file data to prevent crashes from malformed inputs.
- Rejected unsafe file paths, including absolute paths and directory traversal attempts, so extracted files stay within their intended temporary location.

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>

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>

…names materialize_local_file sliced the payload with a wire-supplied header_len and data_len without bounds checks, so a malformed TAG_LOCAL_FILE_DATA record panicked with an out-of-bounds slice (a fuzzer hit index 315 on a 207-byte payload). It also joined the wire-controlled filename onto the temp dir unsanitised, allowing `..`/absolute escape. Validate every offset stays within the payload (checked_add + <= len) and reject filenames that are absolute or contain `..`/root components; both now surface as a clean WireError. Regression tests cover each. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

coderabbitai · 2026-07-07T20:52:18Z

📝 Walkthrough

Walkthrough

Adds bounds-checked arithmetic and filename sanitization to materialize_local_file in the wire parsing code, preventing out-of-bounds slicing panics and directory traversal via crafted filenames. Adds two unit tests covering these validation paths.

Changes

Wire Input Validation Hardening

Layer / File(s)	Summary
Header length bounds check `crates/graph/src/wire.rs`	Uses checked_add to compute header end offset and validates it against payload length, returning InvalidData instead of panicking on out-of-bounds slicing.
Filename traversal sanitization `crates/graph/src/wire.rs`	Rejects absolute filenames and paths containing `..`, root, or prefix components before joining into the temp subdirectory.
Validation tests `crates/graph/src/wire.rs`	Adds tests asserting rejection of an oversized header_len payload and a filename with directory traversal.

Estimated code review effort: 2 (Simple) | ~10 minutes

Poem

A rabbit checked each byte with care,
No overflow slips past my lair,
"../escape" gets turned away,
Safe little burrows, come what may. 🐇🔒

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main hardening change: bounds checks for local-file records and rejection of unsafe filenames.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

_{Comment @coderabbitai help to get the list of available commands.}

The io::Error::new calls in materialize_local_file's bounds checks were too wide for rustfmt; wrap them so `cargo fmt --check` passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

coderabbitai

🧹 Nitpick comments (1)

crates/graph/src/wire.rs (1)
1174-1204: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider asserting the specific error for these security regressions.

Both new tests only check .is_err(), unlike the surrounding regression tests which use matches! on the concrete variant. A future refactor could make either path fail for an unrelated reason (e.g. local_file_name_traversal_is_rejected failing on a hash mismatch rather than the traversal guard) while the assertion still passes, silently eroding the regression's value. Pinning the expected error (kind/variant) keeps these guardrail tests honest.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/graph/src/wire.rs` around lines 1174 - 1204, The new regression tests
in materialize_local_file are too broad because they only assert .is_err(),
which can pass for the wrong failure reason. Update
local_file_header_length_is_bounds_checked and
local_file_name_traversal_is_rejected to match the specific error variant or
kind returned by materialize_local_file, following the pattern used by the
surrounding regression tests, so each guardrail stays pinned to the intended
bounds-check and traversal rejection behavior.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@crates/graph/src/wire.rs`:
- Around line 1174-1204: The new regression tests in materialize_local_file are
too broad because they only assert .is_err(), which can pass for the wrong
failure reason. Update local_file_header_length_is_bounds_checked and
local_file_name_traversal_is_rejected to match the specific error variant or
kind returned by materialize_local_file, following the pattern used by the
surrounding regression tests, so each guardrail stays pinned to the intended
bounds-check and traversal rejection behavior.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 34a03363-25ed-4288-9e81-3bb13777889b

📥 Commits

Reviewing files that changed from the base of the PR and between e8fe0af and d73bd40.

📒 Files selected for processing (1)

crates/graph/src/wire.rs

…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>

bryan-minimal and others added 3 commits July 7, 2026 13:23

Base automatically changed from fix/graph-wire-record-oom to main July 7, 2026 20:55

twitchyliquid64 approved these changes Jul 7, 2026

View reviewed changes

Comment thread crates/graph/src/wire.rs

bryan-minimal mentioned this pull request Jul 7, 2026

fix(common): return an error on malformed JSON in jq::parse_file #661

Merged

bryan-minimal enabled auto-merge (squash) July 7, 2026 22:39

bryan-minimal and others added 2 commits July 7, 2026 15:41

Merge branch 'main' into fix/graph-wire-local-file-bounds

13de9d3

style(graph): rustfmt the local-file bounds-check error calls

d73bd40

The io::Error::new calls in materialize_local_file's bounds checks were too wide for rustfmt; wrap them so `cargo fmt --check` passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

coderabbitai Bot reviewed Jul 7, 2026

View reviewed changes

bryan-minimal merged commit 6895d58 into main Jul 7, 2026
32 checks passed

bryan-minimal deleted the fix/graph-wire-local-file-bounds branch July 7, 2026 23:33

bryan-minimal mentioned this pull request Jul 30, 2026

test: cargo-fuzz harnesses for the decode boundary + bitrot guard + fuzzing guide #1106

Merged

Navigation Menu

Uh oh!

fix(graph): bounds-check local-file records and reject unsafe names - #656

fix(graph): bounds-check local-file records and reject unsafe names#656
bryan-minimal merged 5 commits into
mainfrom
fix/graph-wire-local-file-bounds

bryan-minimal commented Jul 7, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 7, 2026 •

edited

Loading

Walkthrough

Changes

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

bryan-minimal commented Jul 7, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Findings (both in materialize_local_file)

Fix

Fuzzing status

Validation

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bryan-minimal commented Jul 7, 2026 •

edited by coderabbitai Bot

Loading

Findings (both in `materialize_local_file`)

coderabbitai Bot commented Jul 7, 2026 •

edited

Loading