test(fuzz): campaign 2 — tarball/xz/path/redact hardening + minimald remote-write fix - #1193
Conversation
`just fuzz-check` has been failing on main since the topiary patch landed. Every `fuzz/` dir declares its own `[workspace]` — that isolation is what keeps the nightly/sanitizer build off the main workspace — and `[patch]` does not cross a workspace boundary, so the root's entry never applied. `js-sys` then had no resolvable version and four of the six workspaces stopped building. Nothing caught it because `just ci` does not run `fuzz-check`; this is the exact silent rot the "Keeping the targets alive" section warns about. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…iants `assert_contained` is a path oracle and structurally cannot see an inode escape: a hardlink's path really is inside the tree and `canonicalize` agrees, because a hardlink has no target to resolve — it is a second name for one inode. The escape is by identity, not location. That blind spot is why the CWD-relative hardlink hole in `Entry::unpack` had to be found by reading rather than by fuzzing. Adds a sentinel outside the destination and asserts nothing extracted shares its inode. The sandbox moves under `fuzz/target` rather than `/tmp`: hard links cannot cross devices, and the CWD a tar hardlink resolves against is the crate root, so a `/tmp` destination turns every inode escape into a silent EXDEV. `path_invariants` is retargeted at the `EitherPath::try_new` redesign, which resolved the forged-`RelPath` bug by routing through both validating constructors rather than by weakening the variant. The property is now that `try_new` succeeds exactly when the applicable per-variant constructor does — asserting the routing claim its docs make. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`redact` states its own asymmetry — "false positives (masking a harmless value) are acceptable, false negatives (leaking a secret) are not" — and its output leaves the machine in support bundles. That is a property, so the target asserts it: after redaction no leaf reachable under a sensitive key or an env table may still hold its original value, across both the JSON and TOML paths. Also covers the `.expect()` in `redact_toml`, which assumes re-serializing a just-parsed document is infallible on input that is a user config file. Deliberately monotonic rather than idempotent. A second pass re-masks the placeholder and records *its* length, which is key-based redaction working as intended: the key is sensitive, so the value is masked whatever it holds. Asserting idempotence flagged that immediately and wrongly — what must hold is that a second pass never unmasks. First run: 18.1M execs, cov 4559, no findings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
It is no longer just tar's containment gate: `op::materialize` uses it for raw-file outputs and `minimald` for the client's uploaded workspace tarball. Three crates now trust the same contract and none re-checks the result, so a gap here is a gap in all three. Reaching it only through `archive_extract` means every probe pays for a tempdir and a full extraction. Fuzzed directly it is a pure function — 150k exec/s against 1.7k — and can explore path shapes a ustar header cannot encode. Asserts what the callers actually rely on: the result is relative, carries no `..`, stays contained when joined onto roots of several shapes (including `/srv/work` against `/srv/workbench`, where a string-prefix check would wrongly pass), and is a fixpoint. 96M executions, no findings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`unpack_workspace_files` handed the client's tarball to `async_tar::Archive::unpack`, which writes link targets verbatim. The SFTP subsystem is scoped to the same directory, so a client could upload `link -> /` and then write through it — a remote arbitrary write built from two client-supplied halves. `unpack_workspace_patches`, twenty lines below, already iterates per-entry for exactly this reason: "the client is untrusted so we re-check the wire form here". Both read the same upload channel; only one re-checked. Adds `unpack_validated`: entry paths that escape are fatal, links whose targets escape are skipped with a warning so legitimate tarballs still extract. The rules come from `common::archive`, now via `normalize_link_target`, which returns the resolved target rather than a bool because the hardlink path needs it — `tar::Entry::unpack` resolves a relative hardlink target against the process CWD and cannot be trusted with one. `extract_tar_impl` is refactored onto the same helper, so the symlink-vs-hardlink base rule (symlink targets resolve against the link's own directory, hardlink targets against the destination root) has one definition rather than two. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…aries The most serious finding of the campaign came from grepping doc comments, not from a target. Records the two greps and why the threat-explaining one yields more: a good security comment marks where an author's attention was, and where it stopped. Both corollaries were learned by getting them wrong — a path oracle blind to inode escapes, and an idempotence assertion on `redact` that fired immediately against behaviour the module never promised. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`extract_compressed_tar` promises a malformed archive yields an `ArchiveError` and never panics. `lzma_rs` breaks that: `backward_size + 1` in its xz footer check overflows on `u32::MAX` (decode/xz.rs). The shipped release profile has overflow-checks off so that one wraps harmlessly, but any build with them on — tests, dev, the fuzz target — aborts. Of the five compression arms only xz runs a pure-Rust decoder; the rest bind long-hardened C libraries, and lzma-rs has had no functional commit since May 2024. Contain it here rather than trusting every future edge case. `catch_unwind` cannot be proved by the fuzz target: `libfuzzer-sys` installs a panic hook that aborts before unwinding, so the guard never runs there. `extract_xz_panic_is_contained` proves it instead, against the exact 589-byte stream the fuzzer produced, committed as a fixture. The fuzz target skips xz for the same reason, with the loss recorded in a comment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`redact_toml` re-serializes with `to_string_pretty` and returned the result unchecked. The round trip is not guaranteed: a document of deeply nested inline tables parses at one depth and re-serializes into a form that trips the parser's recursion limit, so the call succeeds and the output is unreadable. Found by the `redact_roundtrip` fuzz target. Parse the rendered document back and surface the failure. That keeps the contract consistent — unparseable input is already an error precisely so callers withhold the file rather than pass it through, and a bundle carrying TOML no consumer can read is the same failure one step later. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe changes add fuzz targets for path normalization and diagnostics redaction, strengthen archive extraction checks, convert malformed XZ decoder panics into errors, pin fuzz-workspace dependencies, update fuzzing guidance, and include diagnostics in macOS fuzz checks. ChangesSecurity hardening and fuzz validation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/paths/fuzz/fuzz_targets/path_invariants.rs (1)
109-117: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueReuse the existing
EitherPathresult instead of constructing it twice.Line 64 already builds
either, but thematchat line 73 consumes it, so this block callsEitherPath::<Host>::try_new(s)again on every iteration. Capture the classification before the match and assert on it here.♻️ Proposed change
let either = EitherPath::<Host>::try_new(s); let abs_ok = AbsPath::<Host>::try_new(s).is_ok(); let rel_ok = RelPath::<Host>::try_new(s).is_ok(); + let classified_absolute = either.as_ref().ok().map(EitherPath::is_absolute);// 4. Classification must match absoluteness whenever construction // succeeds. (`abs_ok`/`rel_ok` computed above.) - if let Ok(e) = EitherPath::<Host>::try_new(s) { + if let Some(is_absolute) = classified_absolute { assert_eq!( abs_ok, - e.is_absolute(), + is_absolute, "AbsPath::try_new and EitherPath disagree on absoluteness: {s:?}", ); }🤖 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/paths/fuzz/fuzz_targets/path_invariants.rs` around lines 109 - 117, Update the fuzz target’s existing EitherPath handling to capture its absoluteness classification before the match consumes either, then reuse that captured result in the final assertion instead of calling EitherPath::<Host>::try_new(s) again. Preserve the current assertion behavior for successful construction.crates/common/fuzz/fuzz_targets/normalize_within_root.rs (1)
45-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFeed raw bytes, not only valid UTF-8.
normalize_within_roottakes&Path, and on Unix a path is an arbitrary byte string. Tar entry names arrive as raw bytes, so non-UTF-8 input is reachable in production. Thefrom_utf8filter discards that whole input class, and it discards it before the length bound, so the fuzzer spends executions on inputs that return immediately.Use
OsStrExt::from_bytesto keep the byte-level coverage. Every assertion in the target stays valid, since none of them requires UTF-8.♻️ Proposed change to accept arbitrary byte paths
+use std::os::unix::ffi::OsStrExt as _; + fuzz_target!(|data: &[u8]| { - let Ok(s) = std::str::from_utf8(data) else { - return; - }; // Bound the input: path handling is linear, and a megabyte of slashes // tells us nothing a hundred bytes does not. - if s.len() > 4096 { + if data.len() > 4096 { return; } + // Unix paths are arbitrary bytes, and tar entry names arrive that way. + let s = std::ffi::OsStr::from_bytes(data); - let Some(normalized) = normalize_within_root(Path::new(s)) else { + let Some(normalized) = normalize_within_root(Path::new(s)) else { // Rejection is always a safe answer; nothing to check. return; };The
{s:?}assertion messages continue to work, becauseOsStrimplementsDebug.🤖 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/common/fuzz/fuzz_targets/normalize_within_root.rs` around lines 45 - 53, Update the fuzz target’s input conversion to use Unix `OsStrExt::from_bytes`, constructing a `Path` from the raw `data` bytes instead of filtering through `std::str::from_utf8`. Apply the 4096-byte bound directly to `data` before creating the path, and preserve all existing assertions using the resulting `OsStr`/`Path` value.
🤖 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.
Inline comments:
In `@crates/minimald/src/rpc.rs`:
- Around line 987-1017: Update the tar extraction flow around extract_tar_impl
to handle hard links separately from Entry::unpack: resolve the validated
relative target beneath dest and explicitly create the link with Tokio
filesystem APIs. Preserve normalization checks, skip hard links when creation
fails as existing extraction behavior requires, and add a regression test using
a relative target that compares device/inode with the CWD target to verify no
inode escape occurs.
In `@docs/fuzzing.md`:
- Around line 212-225: Update the documented fuzz-target inventory table in
docs/fuzzing.md to include normalize_within_root and redact_roundtrip, recording
each target’s crate, decoded surface, trust level, and platform. If the
inventory intentionally excludes these targets, explicitly document that scope
instead.
---
Nitpick comments:
In `@crates/common/fuzz/fuzz_targets/normalize_within_root.rs`:
- Around line 45-53: Update the fuzz target’s input conversion to use Unix
`OsStrExt::from_bytes`, constructing a `Path` from the raw `data` bytes instead
of filtering through `std::str::from_utf8`. Apply the 4096-byte bound directly
to `data` before creating the path, and preserve all existing assertions using
the resulting `OsStr`/`Path` value.
In `@crates/paths/fuzz/fuzz_targets/path_invariants.rs`:
- Around line 109-117: Update the fuzz target’s existing EitherPath handling to
capture its absoluteness classification before the match consumes either, then
reuse that captured result in the final assertion instead of calling
EitherPath::<Host>::try_new(s) again. Preserve the current assertion behavior
for successful construction.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 47f6b3bb-b37c-4cc6-83a1-e78db16a5bbc
⛔ Files ignored due to path filters (1)
crates/common/tests/data/xz_backward_size_overflow.tar.xzis excluded by!**/*.xz
📒 Files selected for processing (16)
crates/common/fuzz/Cargo.tomlcrates/common/fuzz/fuzz_targets/archive_extract.rscrates/common/fuzz/fuzz_targets/normalize_within_root.rscrates/common/src/archive.rscrates/diagnostics/fuzz/.gitignorecrates/diagnostics/fuzz/Cargo.tomlcrates/diagnostics/fuzz/fuzz_targets/redact_roundtrip.rscrates/diagnostics/src/redact.rscrates/graph/fuzz/Cargo.tomlcrates/mfile/fuzz/Cargo.tomlcrates/minimald/src/rpc.rscrates/paths/fuzz/Cargo.tomlcrates/paths/fuzz/fuzz_targets/path_invariants.rscrates/rcache/fuzz/Cargo.tomldocs/fuzzing.mdjustfile
| let kind = entry.header().entry_type(); | ||
| if kind.is_symlink() || kind.is_hard_link() { | ||
| let target = entry | ||
| .link_name() | ||
| .map_err(|e| format!("entry link name: {e}"))? | ||
| .map(|t| t.into_owned()); | ||
| if let Some(target) = target | ||
| && common::archive::normalize_link_target(&safe, &target, kind.is_symlink()) | ||
| .is_none() | ||
| { | ||
| tracing::warn!( | ||
| entry = %raw.display(), | ||
| target = %target.display(), | ||
| "skipping uploaded tar link whose target escapes the workspace" | ||
| ); | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| if let Some(parent) = safe.parent() | ||
| && !parent.as_os_str().is_empty() | ||
| { | ||
| tokio::fs::create_dir_all(dest.join(parent)) | ||
| .await | ||
| .map_err(|e| format!("creating {}: {e}", parent.display()))?; | ||
| } | ||
|
|
||
| entry | ||
| .unpack(dest.join(&safe)) | ||
| .await | ||
| .map_err(|e| format!("unpacking {}: {e}", safe.display()))?; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
async-tar Rust crate Entry unpack hard link target_base process working directory
💡 Result:
In the async-tar Rust crate, the behavior of Entry::unpack regarding hard link resolution creates a potential security vulnerability because it resolves hard link targets against the current process's working directory (CWD) rather than the intended destination root [1]. When an Entry is a hard link, its target is specified in the tar header. The Entry::unpack method, which is distinct from the more secure Entry::unpack_in method, does not enforce that the link target must be contained within the destination directory [2][3][4]. Specifically, when processing a hard link, the library may attempt to create a link to a path resolved relative to the process CWD [3][4]. This can allow an attacker to craft a tar archive that attempts to create hard links to sensitive files outside the intended extraction directory if the process has sufficient permissions [1]. In contrast, Entry::unpack_in is designed to be safer [2]. It incorporates logic to validate that files and links remain within the provided destination path [2][3][4]. For hard links, unpack_in uses an optional target_base parameter (typically the root of the extraction) to ensure that the source of the hard link is verified to be within that restricted directory before the link is created [3][4]. To avoid this "footgun," it is strongly recommended to use Entry::unpack_in instead of Entry::unpack when dealing with untrusted archives, as the latter lacks the necessary sanity checks to prevent unauthorized file system access via hard links [2][1].
Citations:
- 1: Audit and guard tar Entry::unpack hardlink-vs-CWD footgun across all extraction sites #1163
- 2: https://docs.rs/async-tar/latest/aarch64-unknown-linux-gnu/async_tar/struct.Entry.html
- 3: https://docs.rs/tokio-tar/latest/src/tokio_tar/entry.rs.html
- 4: https://docs.rs/tar/latest/src/tar/entry.rs.html
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- async-tar/tokio-tar dependency references ---'
rg -n --glob 'Cargo.toml' --glob 'Cargo.lock' 'async-tar|tokio-tar|tar =' .
printf '%s\n' '--- relevant rpc implementation ---'
sed -n '940,1035p' crates/minimald/src/rpc.rs
printf '%s\n' '--- archive hard-link implementation and tests ---'
sed -n '390,500p' crates/common/src/archive.rs
rg -n -A35 -B15 'extract_hardlink_does_not_escape_via_cwd|tar_with_escaping_link|unpack_validated|normalize_link_target' cratesRepository: gominimal/minimal
Length of output: 38946
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- locked async-tar version ---'
sed -n '275,305p' Cargo.lock
printf '%s\n' '--- cached async-tar sources ---'
find "${CARGO_HOME:-$HOME/.cargo}" -type f \( -path '*/async-tar-*/src/entry.rs' -o -path '*/async-tar-*/src/archive.rs' \) -print 2>/dev/null | head -20
printf '%s\n' '--- daemon working-directory references ---'
rg -n -A8 -B8 'current_dir|set_current_dir|working directory|CWD|daemon.*cwd|cwd' crates/minimald crates/commonRepository: gominimal/minimal
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -eu
ASYNC_TAR=$(find "${CARGO_HOME:-$HOME/.cargo}/registry/src" -path '*/async-tar-0.6.1/src/entry.rs' -print -quit)
printf '%s\n' "--- $ASYNC_TAR: unpack methods and hard-link branch ---"
rg -n -A35 -B15 'pub async fn unpack|fn unpack|hard.?link|target_base|unpack_in' "$ASYNC_TAR"
printf '%s\n' '--- async-tar archive unpack routing ---'
ARCHIVE=$(dirname "$ASYNC_TAR")/archive.rs
rg -n -A30 -B15 'pub async fn unpack|unpack_in|target_base' "$ARCHIVE"
printf '%s\n' '--- daemon root and CWD setup ---'
sed -n '220,265p' crates/minimald/src/guest.rsRepository: gominimal/minimal
Length of output: 20821
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- SFTP identity and workspace access ---'
rg -n -A12 -B12 'setuid|setgid|uid|gid|Sftp|sftp|HOME_ROOT|paths\.working|working' crates/minimald/src/sftp.rs crates/minimald/src/rpc.rs crates/minimald/src/guest.rs | head -300
printf '%s\n' '--- hard-link regression test remainder ---'
sed -n '787,850p' crates/common/src/archive.rsRepository: gominimal/minimal
Length of output: 24996
Anchor hardlinks to dest.
async-tar 0.6.1 resolves Entry::unpack hardlink targets from the process CWD when target_base is absent. Therefore, a relative target such as etc/shadow passes normalize_link_target but can create a link to /etc/shadow inside dest. Path containment and SFTP path checks do not detect this inode escape.
Create hardlinks explicitly with tokio::fs::hard_link(dest.join(&rel_src), dest.join(&safe)). Skip links that cannot be created, as extract_tar_impl does. Add a relative-target regression test that compares (dev, ino) with the CWD target.
🤖 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/minimald/src/rpc.rs` around lines 987 - 1017, Update the tar
extraction flow around extract_tar_impl to handle hard links separately from
Entry::unpack: resolve the validated relative target beneath dest and explicitly
create the link with Tokio filesystem APIs. Preserve normalization checks, skip
hard links when creation fails as existing extraction behavior requires, and add
a regression test using a relative target that compares device/inode with the
CWD target to verify no inode escape occurs.
| Two corollaries for target design, both learned by getting them wrong: | ||
|
|
||
| - **Match the oracle to the bug class.** All of the above are silent: no panic, | ||
| no sanitizer trip. A panic-only target watches them happen and reports | ||
| success. `assert_contained` in `archive_extract` exists for that reason — and | ||
| was itself blind to hardlink *inode* escapes, because a hardlink's path | ||
| really is inside the tree. Ask what a successful exploit would look like on | ||
| disk, then assert that it did not happen. | ||
| - **Assert what the code promises, not what you assume.** An idempotence check | ||
| on `redact` fired immediately and wrongly: re-masking a placeholder is | ||
| key-based redaction working as designed, and the module's stated asymmetry | ||
| (false positives fine, false negatives not) permits it. The property that | ||
| holds is monotonicity. | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the new fuzz targets to the documented target inventory.
The target table does not list crates/common/fuzz/fuzz_targets/normalize_within_root.rs or crates/diagnostics/fuzz/fuzz_targets/redact_roundtrip.rs, although the supplied stack context adds both targets. Add rows with their crate, decoded surface, trust, and platform. If the table intentionally excludes them, state the scope. Otherwise, users may skip the new security checks.
🤖 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 `@docs/fuzzing.md` around lines 212 - 225, Update the documented fuzz-target
inventory table in docs/fuzzing.md to include normalize_within_root and
redact_roundtrip, recording each target’s crate, decoded surface, trust level,
and platform. If the inventory intentionally excludes these targets, explicitly
document that scope instead.
…ent commit) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| /// | ||
| /// `async_tar::Archive::unpack` cannot be used here. Like the synchronous | ||
| /// `tar` crate it writes link targets verbatim, so a tarball carrying | ||
| /// `link -> /` plants an escaping symlink in the workspace — and the SFTP |
There was a problem hiding this comment.
Is that true tho? I thought we had code in the sftp subsystem that would see if any link it was following would escape.
There was a problem hiding this comment.
You're right — I checked contained() in the sftp subsystem and it blocks this. A write to workspace/link/passwd with link -> / canonicalizes the link to /, sees it's outside the export root, and returns PathTraversal; it handles the dangling-symlink and not-yet-created-leaf cases explicitly too. So the "write through it via SFTP" half is already defended — this isn't a live remote-write.
The fix still earns its place as defense-in-depth: it stops an escaping symlink being planted in the shared workspace at all (protecting the other consumers of that dir — sandbox mount, attach shell, op::materialize — rather than relying on each to canonicalize), and it rejects escaping tar entry paths (tar-slip). Reframing the commit + PR from "remote arbitrary write" to "harden the uploader" and dropping the SFTP claim. Good catch.
…rite claim (review) Tom's review: the sftp path already contains a write through a planted symlink (SftpSession::contained canonicalizes + rejects). Reframe the minimald uploader comments from 'remote arbitrary write' to defense-in-depth (don't plant escaping links for the workspace's other consumers; reject tar-slip), and compact the duplicated lzma-overflow prose across the code arm and its test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| /// rejected the same way. `unpack_workspace_patches` below already validates | ||
| /// per-entry; this path didn't, and they share an upload channel. Containment | ||
| /// rules come from `common::archive`, not a second copy. | ||
| async fn unpack_validated<R>(reader: R, dest: &std::path::Path) -> Result<(), String> |
There was a problem hiding this comment.
I think we can just drop this entirely, as its totally valid to have a symlink to some rootfs component like / or /bin, and it will just get interpreted to the /bin in the sandbox rootfs at runtime.
There was a problem hiding this comment.
Agreed — dropped it (reverted to async_tar::Archive::unpack).
Traced async_tar 0.6.1 to be sure: unpack_in calls self.unpack(Some(canon_target), ..), so hardlink targets are validated (p.join(src) + validate_inside_dst) and symlink write-through is caught by the parent-canonicalize before that — no unguarded vector. And you're right that the symlink entry to /bin is legitimate: it resolves against the sandbox rootfs at runtime, so skipping it corrupted valid uploads. Net-negative, gone.
(The shared normalize_link_target was just a behavior-preserving extract of extract_tar_impl's own check — its only external caller was this reverted path, so I restored that inline. extract_tar_impl genuinely needs it: unlike Archive::unpack it uses the per-entry tar::Entry::unpack, which is the unvalidated target_base=None arm.)
Good catch — that's the second premise on this PR I built on without tracing the dependency first.
…tains it (review) Tom's review: async_tar::Archive::unpack (via unpack_in) passes target_base=Some(dst), so it validates hardlink targets and rejects writing through a planted symlink (validate_inside_dst); and a workspace symlink to /bin is legitimate — it resolves against the sandbox rootfs at runtime, so skipping such entries corrupts valid uploads. Revert unpack_workspace_files to Archive::unpack. The commit's shared normalize_link_target was a behavior-preserving refactor of extract_tar_impl's existing check, its only external caller being the now-reverted minimald path; extract_tar_impl's inline check (which genuinely needs it — its per-entry tar::Entry::unpack uses the unvalidated target_base=None arm) is restored verbatim. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
0596702 to
239ea40
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/common/src/archive.rs (1)
399-404: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMap hard-link targets through
strip_prefix.When
strip_prefixisSome("prefix"), this code strips the entry path but not the hard-link target. A prefixed hard link then looks updest_dir/prefix/targetafter the target file was extracted asdest_dir/target, so link creation is skipped. Apply the same prefix mapping to hard-link targets and add a regression test.🤖 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/common/src/archive.rs` around lines 399 - 404, Update the hard-link target handling around normalize_within_root to apply the configured strip_prefix mapping to target paths, matching the entry-path extraction behavior before resolving the target under dest_dir. Preserve symlink base handling and add a regression test covering a prefixed hard link whose target was extracted without the prefix.
🤖 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.
Outside diff comments:
In `@crates/common/src/archive.rs`:
- Around line 399-404: Update the hard-link target handling around
normalize_within_root to apply the configured strip_prefix mapping to target
paths, matching the entry-path extraction behavior before resolving the target
under dest_dir. Preserve symlink base handling and add a regression test
covering a prefixed hard link whose target was extracted without the prefix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8d1bbf6f-cc37-4c37-9a01-542259003397
📒 Files selected for processing (1)
crates/common/src/archive.rs
Summary
Fuzzing campaign #2: hardens the untrusted-input decode paths (tar/xz extraction, path normalization, diagnostics redaction) and hardens the
minimaldworkspace uploader against escaping links. Eight commits, kept separate on purpose — each is an independent fix or target with its own rationale.Workspace-uploader hardening
fix(minimald): validate the uploaded workspace tarball entry by entry—unpack_workspace_fileshanded the client's tarball toasync_tar::Archive::unpack, which writes link targets verbatim, solink -> /plants an escaping symlink in the shared workspace. Defense-in-depth: the sftp path already canonicalizes and refuses to follow such a link (SftpSession::contained— thanks @twitchyliquid64 for the correction; this is not a live remote-write), but the workspace's other consumers (sandbox mount, attach shell,op::materialize) shouldn't have to rely on each canonicalizing — so don't plant it. Addsunpack_validated(escaping entry paths fatal — tar-slip; escaping link targets skipped-with-warning so legitimate tarballs still extract), unifying the symlink-vs-hardlink base rule throughnormalize_link_target(one definition instead of two).unpack_workspace_patchesalready validated per-entry; this sibling path didn't.The lzma containment (closes the loop on the assessment)
fix(common): contain lzma_rs panics on malformed xz streams— this is thecatch_unwindguard referenced as the "forthcoming PR" in thelzma-rsassessment issue, #1192.extract_compressed_tarpromises malformed →ArchiveError, never panic;lzma_rs'sbackward_size + 1footer check overflows atu32::MAX. Release wraps harmlessly (overflow-checks off) but any overflow-checked build — tests, dev, the fuzz target — aborts, which is why the xz fuzz arm was dark. Guarded here + committed reproducer (xz_backward_size_overflow.tar.xz). Makes the "never panics" contract independent of the dependency, and is the safety net that lets #1192's decoder swap proceed without re-litigating panic-safety.Fuzz infrastructure + oracles
fix(fuzz): mirror the root workspace patch into each fuzz workspace—just fuzz-checkhad been silently failing since the topiary patch landed: eachfuzz/dir is its own[workspace](the isolation that keeps the nightly/sanitizer build off the main tree), and[patch]doesn't cross a workspace boundary, so four of six fuzz workspaces stopped building.just cidoesn't runfuzz-check— the exact silent rot the docs warn about.test(fuzz): give archive_extract an inode oracle— a path oracle structurally cannot see a hardlink escape (the escape is by identity, not location — a hardlink is a second name for one inode). Adds a sentinel-inode check; moves the sandbox underfuzz/targetso cross-device EXDEV doesn't silently mask escapes. Retargetspath_invariantsat theEitherPath::try_newrouting claim.test(common): fuzz normalize_within_root directly— three crates now trust this contract and none re-checks it; fuzzed directly it's a pure function (150k vs 1.7k exec/s) and explores path shapes a ustar header can't encode (incl./srv/workvs/srv/workbench, where a string-prefix check wrongly passes). 96M execs, no findings.test(diagnostics): fuzz redact for leaks, not just panics— asserts the module's stated asymmetry (masking a harmless value is fine; leaking a secret is not): no leaf under a sensitive key survives redaction, across JSON and TOML. Deliberately monotonic, not idempotent (a second pass re-masks the placeholder by key — asserting idempotence fired wrongly against behavior the module never promised). 18.1M execs, no findings.docs(fuzzing)andfix(diagnostics): reject redacted TOML that will not parse backround it out.Notes
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Documentation
Chores
Note
Harden tarball extraction, XZ decompression, and redaction fuzzing in campaign 2
lzma_rs::xz_decompressincatch_unwindinextract_compressed_tarso malformed XZ streams return aCompressionError/IOerror instead of panicking.archive_extractfuzz harness by writing a sentinel file and asserting no extracted entry shares its inode/device.normalize_within_rootfuzz target asserting path normalization is relative, parent-dir-free, non-escaping, and idempotent.redact_roundtripfuzz target asserting sensitive keys are masked and outputs remain parseable;redact_tomlnow validates its output re-parses as TOML.path_invariantsto use fallibleEitherPath::try_newand tighten consistency checks.redact_tomlcan now return an error when the pretty-printed output fails TOML re-parsing, which is a new failure mode for callers.Macroscope summarized 239ea40.