fix(common): contain path traversal in prefix-stripped tar extraction - #651

…#651 follow-up) (#753)

#651 contained a real write-outside-dest vuln in prefix-stripped extraction,
but its symlink/hardlink handling is stricter than it needs to be: a link
whose *target* escapes dest_dir aborts the whole extraction. Legitimate
upstream tarballs ship exactly that in test fixtures — next.js
(test/.../node_modules symlinks) and syft (symlink-resolution testdata) —
so any rebuild of those packages now fails to extract at all (first hit by
gominimal/pkgs#394/#402, whose dep bumps cascade into next/syft).

Skip the offending link (with a warning) instead of erroring. This is
equally safe against the write-through vector: the link is never created,
so a later entry that would have written through it resolves to a contained
regular path under dest_dir. The entry-path escape stays a hard error — a
file writing to ../x has no benign form.

Tests: extract_rejects_escaping_symlink becomes extract_skips_escaping_symlink
(link skipped, siblings still extract), plus a new
skipping_escaping_symlink_still_contains_write_through proving the tar-slip
defense holds through the skip.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

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

`archive::extract_compressed_tar` decodes attacker-influenced bytes for
build sources, OCI image layers, and remote-cache artifacts (NET and
SUPPLY trust), and already carried one path-traversal fix (#651), but had
no fuzz coverage.

The harness asserts containment after every iteration rather than relying
on a panic oracle. The #651 bug was a *successful* extraction that wrote
outside the destination root: no panic, no sanitizer trip, so a
panic-only target runs past it. `assert_contained` walks the extracted
tree and fails on any entry — or symlink target — resolving outside the
root. Symlinks are judged without being followed, since a link merely
pointing outside is already the escape primitive.

Input layout is hand-rolled rather than `#[derive(Arbitrary)]` so that a
seed is two control bytes prepended to a real tarball. Seeding decides
whether this target works at all: unseeded it spends ~7M executions
before constructing its first valid ustar header. `scripts/gen-seeds.sh`
builds the corpus with the system `tar` across all five compressions and
every strip_prefix selector, including adversarial trees (escaping and
absolute-target symlinks, setuid bits).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…1165)

`extract_raw_file` had no containment check at all. It did

    let rel_path = path.strip_prefix('/').unwrap_or(path);
    let candidate = dir.path().join(rel_path);
    if !candidate.exists() { continue; }
    return deliver_file(&candidate, sink);

`strip_prefix('/')` removes one leading slash and nothing else, so `..`
survived untouched into the join and walked straight out of the cache
directory. `.exists()` then greenlit the result and `deliver_file` streamed
it back to the caller.

The path comes from `[outputs.x] type = "raw-file"` in `minimal.toml`, which
the daemon reads out of a client-uploaded workspace, so it is
attacker-influenceable: `path = "../../../etc/shadow"` is a daemon-side
arbitrary file read delivered over the client's own channel. No symlink
needed, and it bypasses `lcache::LocalDir`'s guards entirely by taking the
raw `&Path` from `DirCacheEntry::path()`.

Two checks, because either alone is insufficient:

- Lexically normalize the request and reject an escape before touching the
  filesystem. Covers both `../../etc/passwd` and `/../../etc/passwd`.
- Decide containment on the *resolved* path too: a package shipping
  `escape -> /etc` makes `<pkg>/escape/passwd` lexically contained while
  resolving outside it.

A containment violation is an `Err`, deliberately not a `continue`:
falling through to the next package would hide the fact that a package
tried to serve a file it does not own, and make the refusal indistinguishable
from a cache miss.

`common::archive::normalize_within_root` becomes public rather than gaining a
second implementation here — that is how this class of bug got reintroduced
before (#651). Its doc now says plainly that it is lexical, so a caller that
touches the filesystem must also check the resolved path.

Regression tests verified to fail without the fix; the symlink case delivers
10 bytes of a file outside the package when the check is removed.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

…#1162)

* test(common): add archive_extract fuzz target with generated seeds

`archive::extract_compressed_tar` decodes attacker-influenced bytes for
build sources, OCI image layers, and remote-cache artifacts (NET and
SUPPLY trust), and already carried one path-traversal fix (#651), but had
no fuzz coverage.

The harness asserts containment after every iteration rather than relying
on a panic oracle. The #651 bug was a *successful* extraction that wrote
outside the destination root: no panic, no sanitizer trip, so a
panic-only target runs past it. `assert_contained` walks the extracted
tree and fails on any entry — or symlink target — resolving outside the
root. Symlinks are judged without being followed, since a link merely
pointing outside is already the escape primitive.

Input layout is hand-rolled rather than `#[derive(Arbitrary)]` so that a
seed is two control bytes prepended to a real tarball. Seeding decides
whether this target works at all: unseeded it spends ~7M executions
before constructing its first valid ustar header. `scripts/gen-seeds.sh`
builds the corpus with the system `tar` across all five compressions and
every strip_prefix selector, including adversarial trees (escaping and
absolute-target symlinks, setuid bits).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(common): check tar link targets when no strip_prefix is set

`extract_tar_impl` had two extraction paths with different security
properties. With `strip_prefix: Some(..)` it ran a per-entry loop that
normalizes the entry path and skips any symlink/hardlink whose target
escapes the destination. With `None` it delegated to a whole-archive
`tar::Archive::unpack`, which writes link targets verbatim.

So the same tarball was safe with a prefix set and unsafe without one:
an entry `link -> ../../../etc` was skipped in one case and created on
disk in the other. Every existing symlink test passed
`Some(&"prefix")`, so nothing covered the gap.

Callers reach the `None` path with untrusted input — a package source
that declares no `strip_prefix` (op::sources) and remote-cache artifact
extraction (rcache::remote), both NET trust.

Route both cases through the same hardened loop, skipping only the strip
step when there is no prefix. Same-tarball write-through was already
contained by tar-rs's `validate_inside_dst`, so the exposure was
attacker-controlled symlinks left in the extracted tree rather than a
direct arbitrary write; the regression test for write-through asserts
that containment explicitly.

Found by the `archive_extract` fuzz target.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(paths): stop EitherPath minting an unvalidated RelPath

Holding a `RelPath` is meant to be proof that a path is relative and free
of `..` — that is what lets the daemon composer trust a `SandboxRelPath`
arriving over the wire, since joining it against the sandbox home cannot
escape. `EitherPath::new` built its `Rel` variant with a struct literal,
bypassing `RelPath::try_new`, so `EitherPath::new("../../etc/passwd")`
forged one: `/srv/sandbox/home` joined with it resolves to
`/srv/etc/passwd`. Its doc comment claimed the opposite.

Reachable from more than the CLI: `EitherPath` has a `Deserialize` impl
and `HostPath`/`SandboxPath` alias it, so `WireSource::Project { path }`
mints one straight from wire JSON.

The fix is not to validate harder. `EitherPath` is built from inputs that
legitimately climb — `min materialize --output ../artifacts` typed in a
sandbox cwd, `--minimal-state-dir ../state` on the daemon command line —
so rejecting `..` would break real usage. Instead it stops claiming a
guarantee it cannot keep: `Rel` now holds a bare `Utf8PathBuf`. The realm
is already carried by `EitherPath<R>`, so the inner tag contributed
nothing but the false promise. `RelPath::try_new` is once again the only
door to a `RelPath`.

No live traversal resulted: `resolve_output` normalizes explicitly (its
comment names this exact shape) and `WireSource::Project.path` is used for
identity, not joins. Defense in depth held while the invariant did not.

Adds the `path_invariants` fuzz target that found it — a differential
asserting every route to a `RelPath` enforces the same rule, plus
join-containment, `FromStr`/`try_new` agreement, and absoluteness
partitioning. It ships with the fix because its assertions are written
against the corrected API. Minimal reproducer: ",/..".

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(common): cap the xz arm's decompressed output

The gzip, zstd, and bz2 arms hand a streaming decoder straight to the tar
reader, so their disk use is bounded by what the archive actually
unpacks. `lzma_rs` exposes only a one-shot `xz_decompress(reader,
writer)`, so the xz arm has to materialize the entire decompressed tar to
a tempfile first — before a single entry has been validated. Uncapped,
that makes a few KB of hostile `.tar.xz` a disk-fill primitive on a
NET/SUPPLY-trust path.

Bound it with a counting writer and report the overflow as
`ArchiveError::DecompressedTooLarge`, following the named-const plus
explicit "too large" shape already used by `MAX_RECORD_LEN` and
`MAX_REQUEST_BYTES`. 4 GiB clears the largest source tarballs in common
use while still bounding the damage.

The cap is checked before the decompress result: `lzma_rs` wraps the
write error in its own type, and were it ever to swallow one, proceeding
would extract a silently truncated tar.

Tests cover `LimitedWriter` directly — driving 4 GiB through
`xz_decompress` in a unit test is not practical, so the bound is tested
where it lives and wired into the xz arm by construction.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(minvmd): bound oneshot RPC response reads

`call_oneshot` read the in-VM daemon's response with an unbounded
`read_to_end`. Every response on this path is a small JSON status object,
but the peer is across the vsock bridge: a wedged or hostile guest could
make the host buffer without limit.

`take` the cap plus one byte so hitting the cap stays distinguishable
from a response that merely fills it, then reject explicitly rather than
letting a truncated body surface as a confusing JSON decode error.
Mirrors `MAX_REQUEST_BYTES` in `minimald::diag`, which bounds the same
shape of read in the other direction.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* docs(fuzzing): record the paths target and what was audited but skipped

Adds `path_invariants` to the target table and its finding to the track
record.

Rewrites the "More targets" list, which was misleading in two ways. It
named `SpecHash`, `Target::from_str`, and `mfile::File` as future work
when all three have had targets for some time — the same silent rot the
"Keeping the targets alive" section warns about, one section over, in the
prose that a guard cannot check. And it conflated being *possible* to
fuzz with being *worth* fuzzing: two of its suggestions turn out to be
hardened by construction.

Replaces it with the surfaces genuinely left, plus a table of five that
were audited and deliberately skipped, each with the reason. Recording
the negative results is the point — otherwise the next person re-derives
them, as this campaign did.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(common): resolve tar hardlink targets against the destination

Unifying both `strip_prefix` branches onto the per-entry loop closed the
symlink hole and opened a hardlink one, because the two extraction APIs
harden against opposite halves of the threat:

  Archive::unpack   -> unpack_in, target_base = Some(dst): joins the link
                       name to dst and validates it. Hardlink-safe.
  Entry::unpack     -> fields.unpack(None, ..): uses the link name verbatim,
                       so the kernel resolves it against the process CWD.

tar's own source comments on exactly this asymmetry. The link-target check
was therefore necessary but not sufficient: a target of `etc/shadow`
normalizes cleanly, passes the check, and then hardlinks the real file into
the destination. Sharing an inode is an escape no path-based containment can
see -- the resulting path genuinely is inside the root -- so it defeats the
SFTP resolver and every other check in the daemon at once, and yields read
*and* write.

Reachable from a remote cache mirror (rcache::remote::materialize) and from
upstream tarballs (op::sources), both of which extract with
`strip_prefix: None` -- the branch that was hardlink-safe before.

Create hardlinks explicitly, anchored to `dest_dir`, mirroring what
`unpack_in`'s `Some(dst)` arm does. A link whose target is missing is
skipped with a warning rather than fatal, consistent with the policy for
escaping links.

The regression test asserts through the inode: it plants a secret in the
process CWD and requires the extracted entry not to read back as that file.
Verified to fail without this change. Note the `archive_extract` fuzz oracle
cannot catch this class -- it asserts path containment, and this escape has
a contained path.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* test(fuzz): seed the archive walk from the canonical root

The containment walk descended from `root` while comparing against
`canonical_root`. On macOS the tempdir is `/var/folders/...` and its
canonical form is `/private/var/folders/...` (`/var` is a symlink), so
`dir.join(target)` carried a prefix `canonical_root` never matched and
`escapes()` fired on in-tree symlinks. Linux `/tmp` is not symlinked, which
is why the campaign that wrote this target never saw it.

Reported by CodeRabbit on #1162.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* fix(fuzz): stop seed archives leaking the generating account

The checked-in seed tarballs carried the generating host's tar metadata:
uid=1000, gid=100, and the literal account names uname='bryan',
gname='users'. 16 of 32 seeds contained the username, now in a public repo.
It also made regeneration non-reproducible, since mtimes and readdir order
vary per host.

Normalize in gen-seeds.sh (umask 022, LC_ALL=C, ustar format, zeroed
numeric owner, epoch mtimes) and regenerate. GNU tar and bsdtar spell these
flags differently, so the script now picks per flavour; byte-identical
output ACROSS the two is still not guaranteed (padding and entry ordering
differ) and is documented as such.

Also fixes the SC2059 shellcheck info in the same file, verified to leave
the generated seeds byte-identical.

Reported by CodeRabbit on #1162.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* fix(paths): keep the realm on EitherPath and fix the callers instead

Reverts the previous approach, which dropped `EitherPath::Rel` to a bare
`Utf8PathBuf` so it could hold a climbing path. That weakened the
abstraction to accommodate its misuse: the realm marker is what makes
misinterpreting a path a compile error, and it is lost the moment anything
destructures the variant, however the enum itself is parameterised. The
generic also became vestigial, justified only by the `Abs` arm.

`Rel` carries a validated `RelPath<R>` again. `EitherPath::new` — infallible,
and struct-literalling BOTH variants, so it forged an `AbsPath` or `RelPath`
that the real constructors would have rejected — becomes `try_new`, routing
through `AbsPath::try_new`/`RelPath::try_new`. `Deserialize` is fallible too,
so a climbing path is now refused at the wire boundary rather than becoming a
forged `RelPath` downstream.

A path that legitimately climbs is unresolved user input, which is what
`CwdRelative` is for. It stops wrapping `EitherPath` — that wrapping was what
forced the `Rel` variant to degrade — and owns the raw string instead, with
`resolve` as the only door out. `FromStr` stays infallible, so clap is
unaffected.

rustc then enumerated the callers that had been relying on the forge:

- `minimald::env::resolve_output` took a `SandboxPath` only to classify
  absolute-vs-relative, then normalized `..` itself two lines later. It never
  wanted the guarantee; it now takes the raw path, which says so.
- `sessions`' glob literal-prefix extraction: a climbing prefix is not a
  usable base, so it is dropped like an absent one.
- The rest were absolute literals in tests.

The test asserting the old behaviour now asserts the new: `EitherPath` must
refuse `../../etc/passwd`, and such a path is `CwdRelative`'s job.

Review feedback from @twitchyliquid64 on #1162.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* fix(minimald): pass the raw output path as a &str

`Utf8Path::new` takes a reference; `output` is an owned `String` at this
call site. Missed because minimald does not build on macOS (procfs), so the
change was made by inspection. Verified this time by running clippy in the
Linux sandbox before pushing.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* docs(paths,minimald): document behaviour, not the change that produced it

Several doc comments argued against a shape that is no longer in the tree —
the `Rel` variant explaining why it is not a bare path, `try_new` explaining
what the old infallible `new` did wrong, `CwdRelative` explaining what it
stopped wrapping. A reader of the merged code has no idea any of that
existed, so it reads as narration rather than documentation.

Replaced with what each thing is and when to reach for it. `resolve_output`
keeps its original docstring, which already said it plainly.

Review feedback from @twitchyliquid64 on #1162.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>