test(fuzz): archive + paths targets, and four decoder hardening fixes - #1162
Conversation
|
|
📝 WalkthroughWalkthroughThe change hardens archive extraction, adds archive and path fuzzing targets with seed corpora, updates path validation and resolution, caps oneshot RPC responses, and updates related tests and documentation. ChangesArchive extraction safety
Path representation validation
RPC response limiting
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related issues
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
`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>
`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>
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>
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>
`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>
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>
fde54a1 to
e956c3a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/common/fuzz/fuzz_targets/archive_extract.rs (1)
52-54: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider truncating the body instead of discarding it.
Line 52 returns early for any body over
MAX_BODY. libFuzzer keeps producing longer inputs, and each one becomes a no-op execution with no coverage. Truncation keeps the cap and still exercises the decompressors. Alternatively, set-max_lenin the fuzz recipe.♻️ Proposed refactor
- if body.len() > MAX_BODY { - return; - } + let body = &body[..body.len().min(MAX_BODY)];🤖 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/archive_extract.rs` around lines 52 - 54, Update the oversized-input handling in the archive extraction fuzz target so bodies exceeding MAX_BODY are truncated to the cap rather than discarded via an early return. Preserve the existing size limit while allowing the resulting bounded input to continue through the decompressor execution path.crates/minimald/src/env.rs (1)
1181-1181: 🧹 Nitpick | 🔵 TrivialConfirm cross-platform test coverage for this change.
This file is
minimald. Per coding guidelines, changes here are unverified on macOS untiljust test-crosshas run. Confirm this recipe (or the platform-appropriatejustscope) has been run for this change before merge.As per coding guidelines: "On macOS, changes to
minimaldor theminCLI's tests are unverified untiljust test-crosshas run; use the platform-appropriatejustscopes."🤖 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/env.rs` at line 1181, Run the platform-appropriate cross-platform test scope for the minimald change, using just test-cross on macOS or the corresponding just recipe for the current platform, and confirm it passes before merge.Source: Coding guidelines
🤖 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/common/fuzz/fuzz_targets/archive_extract.rs`:
- Around line 82-114: Initialize the directory walk stack with
canonical_root.clone() instead of root.to_path_buf() in the stack setup, while
leaving the existing canonical-root containment checks and traversal logic
unchanged.
In `@crates/common/src/archive.rs`:
- Around line 727-741: Update the test around extract_compressed_tar and
escaping_symlink_tar to assert that the escaping symlink is not created, and
resolve the attempted write target from the actual dest/prefix/link path rather
than the current escaped path. Replace unwrap_or_default-based content checking
with the deterministic target assertion pattern used by
skipping_escaping_symlink_still_contains_write_through.
---
Nitpick comments:
In `@crates/common/fuzz/fuzz_targets/archive_extract.rs`:
- Around line 52-54: Update the oversized-input handling in the archive
extraction fuzz target so bodies exceeding MAX_BODY are truncated to the cap
rather than discarded via an early return. Preserve the existing size limit
while allowing the resulting bounded input to continue through the decompressor
execution path.
In `@crates/minimald/src/env.rs`:
- Line 1181: Run the platform-appropriate cross-platform test scope for the
minimald change, using just test-cross on macOS or the corresponding just recipe
for the current platform, and confirm it passes before merge.
🪄 Autofix (Beta)
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: a79b4ca2-7b5e-4e28-8673-0b7791db55c4
📒 Files selected for processing (44)
crates/common/fuzz/Cargo.tomlcrates/common/fuzz/fuzz_targets/archive_extract.rscrates/common/fuzz/scripts/gen-seeds.shcrates/common/fuzz/seeds/archive_extract/deep_c0_s0crates/common/fuzz/seeds/archive_extract/deep_c0_s1crates/common/fuzz/seeds/archive_extract/deep_c0_s2crates/common/fuzz/seeds/archive_extract/deep_c0_s3crates/common/fuzz/seeds/archive_extract/deep_c1_s1crates/common/fuzz/seeds/archive_extract/deep_c2_s1crates/common/fuzz/seeds/archive_extract/deep_c3_s1crates/common/fuzz/seeds/archive_extract/deep_c4_s1crates/common/fuzz/seeds/archive_extract/links_c0_s0crates/common/fuzz/seeds/archive_extract/links_c0_s1crates/common/fuzz/seeds/archive_extract/links_c0_s2crates/common/fuzz/seeds/archive_extract/links_c0_s3crates/common/fuzz/seeds/archive_extract/links_c1_s1crates/common/fuzz/seeds/archive_extract/links_c2_s1crates/common/fuzz/seeds/archive_extract/links_c3_s1crates/common/fuzz/seeds/archive_extract/links_c4_s1crates/common/fuzz/seeds/archive_extract/modes_c0_s0crates/common/fuzz/seeds/archive_extract/modes_c0_s1crates/common/fuzz/seeds/archive_extract/modes_c0_s2crates/common/fuzz/seeds/archive_extract/modes_c0_s3crates/common/fuzz/seeds/archive_extract/modes_c1_s1crates/common/fuzz/seeds/archive_extract/modes_c2_s1crates/common/fuzz/seeds/archive_extract/modes_c3_s1crates/common/fuzz/seeds/archive_extract/modes_c4_s1crates/common/fuzz/seeds/archive_extract/plain_c0_s0crates/common/fuzz/seeds/archive_extract/plain_c0_s1crates/common/fuzz/seeds/archive_extract/plain_c0_s2crates/common/fuzz/seeds/archive_extract/plain_c0_s3crates/common/fuzz/seeds/archive_extract/plain_c1_s1crates/common/fuzz/seeds/archive_extract/plain_c2_s1crates/common/fuzz/seeds/archive_extract/plain_c3_s1crates/common/fuzz/seeds/archive_extract/plain_c4_s1crates/common/src/archive.rscrates/minimald/src/env.rscrates/minvmd/src/rpc_client.rscrates/paths/fuzz/.gitignorecrates/paths/fuzz/Cargo.tomlcrates/paths/fuzz/fuzz_targets/path_invariants.rscrates/paths/src/lib.rsdocs/fuzzing.mdjustfile
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/common/fuzz/scripts/gen-seeds.sh`:
- Around line 69-74: Normalize archive generation in gen-seeds.sh by setting
umask 022 and LC_ALL=C, and update the tar invocation to use ustar format,
sorted names, the Unix epoch mtime, and numeric zero owner/group metadata. Then
regenerate the checked-in seed corpus using the normalized command.
- Around line 69-82: Update the seed-generation loop around emit and add a
second archive whose entries are rooted with the pkg operand, then use that
archive for selector 2 so its paths match strip_prefix("pkg"); keep the existing
base archive for selectors 0, 1, and 3.
In `@docs/fuzzing.md`:
- Around line 187-203: Add one blank line immediately after the audit table in
the documentation section, before the following ordered-list item, without
changing the table content or surrounding list structure.
🪄 Autofix (Beta)
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: 34db8c9b-0500-4510-bdb8-69e0a048f029
📒 Files selected for processing (44)
crates/common/fuzz/Cargo.tomlcrates/common/fuzz/fuzz_targets/archive_extract.rscrates/common/fuzz/scripts/gen-seeds.shcrates/common/fuzz/seeds/archive_extract/deep_c0_s0crates/common/fuzz/seeds/archive_extract/deep_c0_s1crates/common/fuzz/seeds/archive_extract/deep_c0_s2crates/common/fuzz/seeds/archive_extract/deep_c0_s3crates/common/fuzz/seeds/archive_extract/deep_c1_s1crates/common/fuzz/seeds/archive_extract/deep_c2_s1crates/common/fuzz/seeds/archive_extract/deep_c3_s1crates/common/fuzz/seeds/archive_extract/deep_c4_s1crates/common/fuzz/seeds/archive_extract/links_c0_s0crates/common/fuzz/seeds/archive_extract/links_c0_s1crates/common/fuzz/seeds/archive_extract/links_c0_s2crates/common/fuzz/seeds/archive_extract/links_c0_s3crates/common/fuzz/seeds/archive_extract/links_c1_s1crates/common/fuzz/seeds/archive_extract/links_c2_s1crates/common/fuzz/seeds/archive_extract/links_c3_s1crates/common/fuzz/seeds/archive_extract/links_c4_s1crates/common/fuzz/seeds/archive_extract/modes_c0_s0crates/common/fuzz/seeds/archive_extract/modes_c0_s1crates/common/fuzz/seeds/archive_extract/modes_c0_s2crates/common/fuzz/seeds/archive_extract/modes_c0_s3crates/common/fuzz/seeds/archive_extract/modes_c1_s1crates/common/fuzz/seeds/archive_extract/modes_c2_s1crates/common/fuzz/seeds/archive_extract/modes_c3_s1crates/common/fuzz/seeds/archive_extract/modes_c4_s1crates/common/fuzz/seeds/archive_extract/plain_c0_s0crates/common/fuzz/seeds/archive_extract/plain_c0_s1crates/common/fuzz/seeds/archive_extract/plain_c0_s2crates/common/fuzz/seeds/archive_extract/plain_c0_s3crates/common/fuzz/seeds/archive_extract/plain_c1_s1crates/common/fuzz/seeds/archive_extract/plain_c2_s1crates/common/fuzz/seeds/archive_extract/plain_c3_s1crates/common/fuzz/seeds/archive_extract/plain_c4_s1crates/common/src/archive.rscrates/minimald/src/env.rscrates/minvmd/src/rpc_client.rscrates/paths/fuzz/.gitignorecrates/paths/fuzz/Cargo.tomlcrates/paths/fuzz/fuzz_targets/path_invariants.rscrates/paths/src/lib.rsdocs/fuzzing.mdjustfile
🚧 Files skipped from review as they are similar to previous changes (41)
- justfile
- crates/common/fuzz/seeds/archive_extract/plain_c3_s1
- crates/common/fuzz/seeds/archive_extract/deep_c1_s1
- crates/common/fuzz/seeds/archive_extract/modes_c0_s2
- crates/minimald/src/env.rs
- crates/common/fuzz/seeds/archive_extract/modes_c1_s1
- crates/common/fuzz/seeds/archive_extract/links_c4_s1
- crates/common/fuzz/seeds/archive_extract/links_c2_s1
- crates/common/fuzz/seeds/archive_extract/modes_c2_s1
- crates/common/fuzz/seeds/archive_extract/deep_c3_s1
- crates/common/fuzz/seeds/archive_extract/deep_c4_s1
- crates/common/fuzz/seeds/archive_extract/deep_c0_s2
- crates/common/fuzz/seeds/archive_extract/modes_c4_s1
- crates/common/fuzz/seeds/archive_extract/deep_c2_s1
- crates/common/fuzz/seeds/archive_extract/links_c3_s1
- crates/common/fuzz/seeds/archive_extract/plain_c0_s2
- crates/common/fuzz/seeds/archive_extract/plain_c0_s1
- crates/common/fuzz/seeds/archive_extract/modes_c3_s1
- crates/common/fuzz/seeds/archive_extract/deep_c0_s0
- crates/common/fuzz/seeds/archive_extract/plain_c0_s3
- crates/common/fuzz/seeds/archive_extract/deep_c0_s3
- crates/common/fuzz/seeds/archive_extract/modes_c0_s3
- crates/common/fuzz/seeds/archive_extract/plain_c4_s1
- crates/common/fuzz/fuzz_targets/archive_extract.rs
- crates/common/fuzz/seeds/archive_extract/plain_c1_s1
- crates/common/fuzz/seeds/archive_extract/modes_c0_s0
- crates/common/fuzz/seeds/archive_extract/links_c1_s1
- crates/paths/fuzz/.gitignore
- crates/common/fuzz/seeds/archive_extract/links_c0_s3
- crates/paths/fuzz/Cargo.toml
- crates/common/fuzz/seeds/archive_extract/links_c0_s2
- crates/common/fuzz/seeds/archive_extract/deep_c0_s1
- crates/common/fuzz/seeds/archive_extract/links_c0_s0
- crates/common/fuzz/seeds/archive_extract/plain_c2_s1
- crates/common/fuzz/seeds/archive_extract/modes_c0_s1
- crates/common/fuzz/Cargo.toml
- crates/paths/fuzz/fuzz_targets/path_invariants.rs
- crates/common/fuzz/seeds/archive_extract/plain_c0_s0
- crates/minvmd/src/rpc_client.rs
- crates/paths/src/lib.rs
- crates/common/src/archive.rs
| tar -C "$work/$tree" -cf "$base" . | ||
|
|
||
| gzip -kfn "$base" # -> .tar.gz (-n: no timestamp) | ||
| zstd -qf "$base" -o "$base.zst" | ||
| xz -kfq "$base" # -> .tar.xz | ||
| bzip2 -kfq "$base" # -> .tar.bz2 | ||
|
|
||
| # strip_prefix selectors: 0 None, 1 ".", 2 "pkg", 3 "..". Seed every | ||
| # prefix against the uncompressed tar (cheapest to mutate), and the | ||
| # "." prefix against each compressed form so all five decompressor | ||
| # branches get a structurally valid entry point. | ||
| for s in 0 1 2 3; do | ||
| emit "$tree" 0 "$s" "$base" | ||
| done |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
mkdir -p "$tmp/src/pkg"
printf 'x\n' > "$tmp/src/pkg/file"
tar -C "$tmp/src" -cf "$tmp/archive.tar" .
tar -tf "$tmp/archive.tar"
rg -n -C 3 'strip_prefix|control\[1\]|"pkg"' \
crates/common/fuzz/fuzz_targets/archive_extract.rs \
crates/common/src/archive.rs \
crates/common/fuzz/scripts/gen-seeds.shRepository: gominimal/minimal
Length of output: 12035
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- seed generator ---'
cat -n crates/common/fuzz/scripts/gen-seeds.sh | sed -n '45,100p'
printf '%s\n' '--- existing archive_extract fixtures ---'
git ls-files crates/common/fuzz/seeds/archive_extract | sort | sed -n '1,120p'
printf '%s\n' '--- archive extraction path logic ---'
cat -n crates/common/src/archive.rs | sed -n '320,390p'
cat -n crates/common/fuzz/fuzz_targets/archive_extract.rs | sed -n '20,75p'
printf '%s\n' '--- fixture sizes and names ---'
find crates/common/fuzz/seeds/archive_extract -maxdepth 1 -type f -printf '%f %s bytes\n' | sortRepository: gominimal/minimal
Length of output: 10922
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- stored paths in selector-2 fixture payloads ---'
for f in crates/common/fuzz/seeds/archive_extract/*_c0_s2; do
printf '%s: ' "$f"
tail -c +3 "$f" | tar -tf - | sed -n '1,8p' | paste -sd ' ' -
done
printf '%s\n' '--- stored paths for both tar operands ---'
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
mkdir -p "$tmp/src/pkg"
printf 'x\n' > "$tmp/src/pkg/file"
tar -C "$tmp/src" -cf "$tmp/dot.tar" .
tar -C "$tmp/src" -cf "$tmp/pkg.tar" pkg
printf '%s\n' 'dot operand:'
tar -tf "$tmp/dot.tar"
printf '%s\n' 'pkg operand:'
tar -tf "$tmp/pkg.tar"
printf '%s\n' '--- relevant standard-library behavior if rustc is available ---'
if command -v rustc >/dev/null 2>&1; then
rustc --version
cat > "$tmp/path_probe.rs" <<'RS'
use std::path::Path;
fn main() {
for (path, prefix) in [("./pkg/file", "pkg"), ("./pkg/file", "."), ("pkg/file", "pkg")] {
println!("{path:?}.strip_prefix({prefix:?}) = {:?}", Path::new(path).strip_prefix(prefix));
}
}
RS
rustc "$tmp/path_probe.rs" -o "$tmp/path_probe"
"$tmp/path_probe"
else
printf '%s\n' 'rustc unavailable'
fiRepository: gominimal/minimal
Length of output: 303
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- stored paths in selector-2 fixture payloads ---'
for f in crates/common/fuzz/seeds/archive_extract/*_c0_s2; do
printf '%s:\n' "$f"
tail -c +3 "$f" | tar -tf - | sed -n '1,8p'
done
printf '%s\n' '--- stored paths for both tar operands ---'
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
mkdir -p "$tmp/src/pkg"
printf 'x\n' > "$tmp/src/pkg/file"
tar -C "$tmp/src" -cf "$tmp/dot.tar" .
tar -C "$tmp/src" -cf "$tmp/pkg.tar" pkg
printf '%s\n' 'dot operand:'
tar -tf "$tmp/dot.tar"
printf '%s\n' 'pkg operand:'
tar -tf "$tmp/pkg.tar"
printf '%s\n' '--- standard-library strip_prefix behavior ---'
if command -v rustc >/dev/null 2>&1; then
rustc --version
cat > "$tmp/path_probe.rs" <<'RS'
use std::path::Path;
fn main() {
for (path, prefix) in [("./pkg/file", "pkg"), ("./pkg/file", "."), ("pkg/file", "pkg")] {
println!("{path:?}.strip_prefix({prefix:?}) = {:?}", Path::new(path).strip_prefix(prefix));
}
}
RS
rustc "$tmp/path_probe.rs" -o "$tmp/path_probe"
"$tmp/path_probe"
else
printf '%s\n' 'rustc unavailable'
fiRepository: gominimal/minimal
Length of output: 836
🌐 Web query:
Rust tar crate Entry::path leading ./ normalization path header documentation source
💡 Result:
In the Rust tar crate, the Entry::path method returns the path name for an archive entry [1]. Regarding path normalization, the implementation includes specific behaviors when processing paths: 1. Path Component Handling: During entry processing, path components are parsed [2]. Leading '/' characters are trimmed (e.g., '///test' is treated as 'test') [2]. Additionally, '.' components are ignored [2]. 2. Security Normalization: To prevent directory traversal security issues, if any part of the filename contains '..', the file is skipped during extraction [2][3]. 3. Builder Normalization: On the writing side, the Header::set_path method historically strips out '.' components [4]. There is known community discussion regarding this behavior, as some users have noted that this can lead to unexpected path modifications [4]. It is important to note that the crate's documentation and internal source code emphasize that the paths returned by Entry::path are not fully normalized in the sense of resolving all filesystem-specific behaviors (such as case folding or Unicode normalization) [5]. Users are advised to rely on the library's built-in extraction methods, such as Entry::unpack_in, which are designed to handle these security and path-related concerns safely [2][3]. Directly inspecting the header or manually processing paths is generally discouraged in favor of using these provided library methods to ensure archive formats and security constraints are handled correctly [2][1].
Citations:
- 1: https://docs.rs/tar/latest/tar/struct.Entry.html
- 2: https://docs.rs/tar/latest/src/tar/entry.rs.html
- 3: https://docs.rs/tar/latest/tar/
- 4: Problem with built-in path normalization composefs/tar-rs#263
- 5: https://docs.rs/astral-tokio-tar/latest/tokio_tar/struct.Entry.html
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- tar dependency declarations ---'
rg -n -C 2 '(^|\s)tar\s*=|name = "tar"|source = ".*tar' \
Cargo.toml Cargo.lock crates 2>/dev/null | sed -n '1,120p'
printf '%s\n' '--- available local tar sources ---'
find "$HOME/.cargo" -path '*/tar-*' -maxdepth 6 -type f 2>/dev/null | sed -n '1,40p'
printf '%s\n' '--- path handling references in repository ---'
rg -n -C 3 'Entry::path|header\.path|strip_prefix\(' crates/common | sed -n '1,180p'Repository: gominimal/minimal
Length of output: 3832
🌐 Web query:
tar 0.4.46 src entry.rs Entry::path components dot ignored
💡 Result:
In the tar crate (including version 0.4.46), the behavior you identified in src/entry.rs regarding '.' (current directory) components is intentional and acts as part of the library's path normalization and security logic during extraction [1][2]. When the library processes entry paths—specifically during methods like unpack_in—it iterates over the path components [1][2]. Within this logic, the Component::CurDir (which represents a '.' component) is explicitly matched and ignored [1][2]. The relevant code comment explains: // Leading '/' characters, root paths, and '.' // components are just ignored and treated as "empty // components" Component::Prefix(..) | Component::RootDir | Component::CurDir => continue, This behavior serves to sanitize paths to prevent potential security vulnerabilities or invalid file system paths during extraction [1][2]. By skipping '.' components, the library avoids creating redundant directory structures or encountering issues with path resolution that could arise from ambiguous path specifications in an archive [1][2]. This mechanism is part of the broader logic in the crate to sanitize inputs and prevent directory traversal, similar to how it handles other path components like '..' (which causes the entry to be skipped to prevent security issues) and leading root directory indicators [1][2].
Citations:
- 1: https://docs.rs/tar/latest/src/tar/entry.rs.html
- 2: https://github.com/alexcrichton/tar-rs/blob/master/src/entry.rs
🌐 Web query:
site:docs.rs/crate/tar/0.4.46/source/src/entry.rs "pub fn path" "CurDir"
💡 Result:
The matching code in tar 0.4.46 is:
pub fn path(&self) -> io::Result<Cow<'_, Path>> {
self.fields.path()
}CurDir appears in unpacking logic:
Component::Prefix(..) | Component::RootDir | Component::CurDir => continue,Thus, . path components are ignored during extraction. (docs.rs)
Citations:
🌐 Web query:
Rust std Path strip_prefix "./pkg" "pkg" Components leading CurDir
💡 Result:
In Rust's standard library, Path::strip_prefix performs a lexical operation that determines if a given base path is a prefix of self [1][2]. Regarding your specific scenario with ./pkg and pkg: 1. Lexical Behavior: Path::strip_prefix does not normalize paths before comparing them [2]. It relies on the components of the path as they are structured [1]. 2. Components and CurDir: The Path::components() method treats ./ at the beginning of a path as a CurDir (current directory) component [3]. 3. Result: Because strip_prefix is a direct component-wise comparison, a path starting with ./pkg contains a CurDir component followed by a Normal("pkg") component [3]. A path just containing pkg consists only of a Normal("pkg") component. 4. Outcome: Calling strip_prefix("pkg") on a path of ./pkg will return an Err because the first component of ./pkg is CurDir, which does not match the first component of the base pkg (Normal("pkg")) [1][3]. To successfully strip such a prefix, you may need to normalize the path first (e.g., using Path::canonicalize, though that requires the file to exist on the filesystem) or manually handle the CurDir component by iterating through the path components and filtering them as needed [2][4]. Top results: [1][2][3]
Citations:
- 1: https://doc.rust-lang.org/std/path/struct.Path.html
- 2: https://users.rust-lang.org/t/drop-current-directory-from-abs-path/70047
- 3: https://docs.rs/envpath/latest/envpath/struct.EnvPath.html
- 4: https://stackoverflow.com/questions/50109230/can-i-use-stdpathpaths-strip-prefix-to-replace-a-dynamic-prefix
Use an archive with matching pkg paths for selector 2.
Entry::path() returns the raw ./pkg/... paths. Path::strip_prefix("pkg") does not normalize the leading ./, so *_c0_s2 exits before normalize_within_root. Create a second archive with operand pkg for selector 2.
🤖 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/scripts/gen-seeds.sh` around lines 69 - 82, Update the
seed-generation loop around emit and add a second archive whose entries are
rooted with the pkg operand, then use that archive for selector 2 so its paths
match strip_prefix("pkg"); keep the existing base archive for selectors 0, 1,
and 3.
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>
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>
|
Two follow-ups pushed. 1. CodeRabbit review comment — fixed ( 2. This PR introduced a hardlink escape; also fixed ( Unifying both
tar 0.4.46 comments on precisely this asymmetry in Reachable from a remote cache mirror ( Fix creates hardlinks explicitly, anchored to Worth flagging for the campaign: the |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/common/src/archive.rs`:
- Around line 741-781: Update extract_hardlink_does_not_escape_via_cwd to avoid
unsynchronized process-wide current_dir mutation: either serialize the CWD swap
with the repository’s shared test mutex/serial mechanism or redesign the fixture
to avoid changing CWD. Strengthen the regression assertion so it
deterministically verifies the unsafe loot link is not created, without
conditional exists checks or unwrap_or_default masking failures. Validate using
the repository’s just test and, on macOS, just test-cross recipes.
🪄 Autofix (Beta)
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: fabe8392-172b-4d17-827f-74ff2de13c6e
📒 Files selected for processing (2)
crates/common/fuzz/fuzz_targets/archive_extract.rscrates/common/src/archive.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/common/fuzz/fuzz_targets/archive_extract.rs
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>
| /// Relative variant. | ||
| Rel(RelPath<R>), | ||
| /// | ||
| /// Deliberately a bare [`Utf8PathBuf`] and **not** a [`RelPath<R>`]. |
There was a problem hiding this comment.
This breaks the model, paths in the paths crate carry the marker type Realm to make it difficult to accidentally interpret a path representing a sandbox as something on a host/daemon filesystem, so realm needs to be carried.
Also, it seems like this change was motivated by the whole .. thing - if thats the case then the fix is elsewhere depending on how RelPath is used/misused today:
- upstream uses through RelPath that may have traversal need to be resolved before they hit here, this might be a good fit for the
CwdResolvable/CwdRelativetypes here that were intended for that purpose. - Or if thats really not compatible with how its used today, then RelPath needs to be loosened to permit traversal
| /// cannot uphold `RelPath`'s no-`..` guarantee. Holding a `RelPath` here | ||
| /// would mint one that [`RelPath::try_new`] would have rejected, and every | ||
| /// downstream holder of a `RelPath` is entitled to assume that cannot | ||
| /// happen. The realm is already carried by `EitherPath<R>` itself, so |
There was a problem hiding this comment.
No, any nested use of the inner Utf8PathBuf for the rel variant looses the realm. The only reason its not a compile error for EitherPath to have the realm generic is because its used in the other variant, Abs. If Abs stopped being generic over realm, so would EitherPath.
So, its more correct to keep the realm as a generic on all variants.
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>
|
@twitchyliquid64 you're right, and the "got lazy" read is fair — the change weakened the abstraction to accommodate its misuse rather than fixing the misuse. Redone in What changed
Then rustc did the audit, which is the part that made your point concrete:
The test that asserted the old behaviour now asserts the new one: Green locally: One design question I'd rather ask than guess at, since it's the same shape as what you just pushed back on. The plan for the remaining containment work (the SFTP escape and friends) is a primitive whose operations return fds, bytes, or metadata — never a path, so a caller can't obtain the thing they'd misuse. That mechanic seems aligned with what The open question is placement. The analysis argues for Options as I see them:
Happy to spec (1) or (2) properly before writing any of it. |
`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>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/paths/src/lib.rs (1)
1290-1318: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression test for the climbing-path resolve contract, and simplify the redundant absolute-branch validation.
resolve()is the door where a CLI path that legitimately climbs (--minimal-state-dir ../state) turns into anAbsPath. The doc comment explains this intent, but no test resolves a..-containing relative path and checks the literal (non-normalized) join result. Add one to lock down this contract, since it is exactly the kind of input thepath_invariantsfuzz target is meant to probe.Separately, in the absolute branch,
self.raw.is_absolute()is already checked before callingAbsPath::try_new(self.raw.clone()), whose only failure mode is!is_absolute(). The.map_err(|_| CwdResolveError::CwdNotAbsolute(...))there is unreachable. The doubled{ { ... } }block in the relative branch also serves no purpose.🧪 Suggested test addition
+ #[test] + fn cwd_relative_resolve_preserves_climbing_components() { + let cli: CwdRelative<Host> = "../state".parse().unwrap(); + let resolved = cli.resolve().expect("test process cwd is well-defined"); + let cwd_std = std::env::current_dir().unwrap(); + let cwd_utf8 = Utf8PathBuf::from_path_buf(cwd_std).unwrap(); + assert_eq!(resolved.as_str(), format!("{cwd_utf8}/../state")); + }🤖 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/src/lib.rs` around lines 1290 - 1318, Add a regression test for `resolve()` that resolves a relative path containing `..` and asserts the result preserves the literal, non-normalized join against the current directory. In `resolve()`, simplify the absolute-path branch by returning the validated `AbsPath` directly without the unreachable `map_err`, and remove the redundant nested block around the relative-path logic.crates/sessions/src/core/primitives.rs (1)
852-904: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDocument the second
walk_rootfailure mode.
walk_rootalso returnsNonewhen a relative literal prefix contains..andHostPath::try_newrejects it, such as directFileSet::try_new("../shared/**/*.toml")usage. Patch sources reject..earlier inexpand_source, so they do not reach this path. Extend thewalk_rootdocumentation to describe this case.🤖 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/sessions/src/core/primitives.rs` around lines 852 - 904, Update the documentation for walk_root to mention that it can return None when a relative literal prefix contains ".." and HostPath::try_new rejects it, including direct FileSet::try_new inputs such as "../shared/**/*.toml"; note that expand_source normally rejects this earlier for patch sources.
🤖 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/common/fuzz/scripts/gen-seeds.sh`:
- Around line 93-94: Update the source timestamp normalization command in the
seed-generation script to run with TZ=UTC and use touch -h -t 197001010000,
preserving symlink timestamps without modifying targets. Remove the stderr
suppression and failure fallback so timestamp errors terminate generation, then
regenerate all affected seeds, including links_c4_s1.
- Line 94: Replace the timestamp normalization command in gen-seeds.sh so it
updates each filesystem entry under $work/$tree without following symlinks,
using a portable operation supported on macOS and Linux. Ensure symlink targets
outside $work cannot be modified, and remove the stderr suppression and
unconditional success fallback so failures remain visible.
---
Nitpick comments:
In `@crates/paths/src/lib.rs`:
- Around line 1290-1318: Add a regression test for `resolve()` that resolves a
relative path containing `..` and asserts the result preserves the literal,
non-normalized join against the current directory. In `resolve()`, simplify the
absolute-path branch by returning the validated `AbsPath` directly without the
unreachable `map_err`, and remove the redundant nested block around the
relative-path logic.
In `@crates/sessions/src/core/primitives.rs`:
- Around line 852-904: Update the documentation for walk_root to mention that it
can return None when a relative literal prefix contains ".." and
HostPath::try_new rejects it, including direct FileSet::try_new inputs such as
"../shared/**/*.toml"; note that expand_source normally rejects this earlier for
patch sources.
🪄 Autofix (Beta)
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: ed0179f4-ac0e-4ef5-b3d7-af164e4ef2b3
📒 Files selected for processing (44)
crates/common/fuzz/scripts/gen-seeds.shcrates/common/fuzz/seeds/archive_extract/deep_c0_s0crates/common/fuzz/seeds/archive_extract/deep_c0_s1crates/common/fuzz/seeds/archive_extract/deep_c0_s2crates/common/fuzz/seeds/archive_extract/deep_c0_s3crates/common/fuzz/seeds/archive_extract/deep_c1_s1crates/common/fuzz/seeds/archive_extract/deep_c2_s1crates/common/fuzz/seeds/archive_extract/deep_c3_s1crates/common/fuzz/seeds/archive_extract/deep_c4_s1crates/common/fuzz/seeds/archive_extract/links_c0_s0crates/common/fuzz/seeds/archive_extract/links_c0_s1crates/common/fuzz/seeds/archive_extract/links_c0_s2crates/common/fuzz/seeds/archive_extract/links_c0_s3crates/common/fuzz/seeds/archive_extract/links_c1_s1crates/common/fuzz/seeds/archive_extract/links_c2_s1crates/common/fuzz/seeds/archive_extract/links_c3_s1crates/common/fuzz/seeds/archive_extract/links_c4_s1crates/common/fuzz/seeds/archive_extract/modes_c0_s0crates/common/fuzz/seeds/archive_extract/modes_c0_s1crates/common/fuzz/seeds/archive_extract/modes_c0_s2crates/common/fuzz/seeds/archive_extract/modes_c0_s3crates/common/fuzz/seeds/archive_extract/modes_c1_s1crates/common/fuzz/seeds/archive_extract/modes_c2_s1crates/common/fuzz/seeds/archive_extract/modes_c3_s1crates/common/fuzz/seeds/archive_extract/modes_c4_s1crates/common/fuzz/seeds/archive_extract/plain_c0_s0crates/common/fuzz/seeds/archive_extract/plain_c0_s1crates/common/fuzz/seeds/archive_extract/plain_c0_s2crates/common/fuzz/seeds/archive_extract/plain_c0_s3crates/common/fuzz/seeds/archive_extract/plain_c1_s1crates/common/fuzz/seeds/archive_extract/plain_c2_s1crates/common/fuzz/seeds/archive_extract/plain_c3_s1crates/common/fuzz/seeds/archive_extract/plain_c4_s1crates/common/src/archive.rscrates/mfile/src/project_composable.rscrates/minimal/src/prompt.rscrates/minimald/src/env.rscrates/paths/src/lib.rscrates/sessions/src/core/compose.rscrates/sessions/src/core/primitives.rscrates/sessions/src/daemon/composer.rscrates/sessions/src/wire/primitives.rscrates/sessions/tests/client_flow2.rsdocs/fuzzing.md
🚧 Files skipped from review as they are similar to previous changes (33)
- crates/common/fuzz/seeds/archive_extract/deep_c4_s1
- crates/common/fuzz/seeds/archive_extract/links_c1_s1
- crates/common/fuzz/seeds/archive_extract/deep_c1_s1
- crates/common/fuzz/seeds/archive_extract/plain_c1_s1
- crates/common/fuzz/seeds/archive_extract/links_c0_s0
- crates/common/fuzz/seeds/archive_extract/plain_c3_s1
- crates/common/fuzz/seeds/archive_extract/modes_c0_s2
- crates/common/fuzz/seeds/archive_extract/modes_c4_s1
- crates/common/fuzz/seeds/archive_extract/links_c2_s1
- crates/common/fuzz/seeds/archive_extract/plain_c2_s1
- crates/common/fuzz/seeds/archive_extract/modes_c2_s1
- crates/common/fuzz/seeds/archive_extract/deep_c3_s1
- crates/common/fuzz/seeds/archive_extract/links_c0_s2
- crates/common/fuzz/seeds/archive_extract/modes_c0_s0
- crates/common/fuzz/seeds/archive_extract/links_c0_s1
- crates/common/fuzz/seeds/archive_extract/modes_c1_s1
- crates/common/fuzz/seeds/archive_extract/deep_c0_s3
- crates/common/fuzz/seeds/archive_extract/links_c3_s1
- crates/common/fuzz/seeds/archive_extract/plain_c0_s1
- crates/common/fuzz/seeds/archive_extract/modes_c0_s3
- crates/common/fuzz/seeds/archive_extract/deep_c0_s2
- crates/common/fuzz/seeds/archive_extract/deep_c0_s1
- crates/common/fuzz/seeds/archive_extract/deep_c0_s0
- crates/common/fuzz/seeds/archive_extract/modes_c3_s1
- crates/common/fuzz/seeds/archive_extract/deep_c2_s1
- crates/common/fuzz/seeds/archive_extract/modes_c0_s1
- crates/common/fuzz/seeds/archive_extract/plain_c0_s3
- crates/common/fuzz/seeds/archive_extract/plain_c0_s0
- crates/minimald/src/env.rs
- crates/common/fuzz/seeds/archive_extract/plain_c0_s2
- crates/common/fuzz/seeds/archive_extract/plain_c4_s1
- docs/fuzzing.md
- crates/common/src/archive.rs
| # Fixed mtime on the sources too: bsdtar has no --mtime for create. | ||
| find "$work/$tree" -exec touch -t 197001010000 {} + 2>/dev/null || true |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
TZ=UTC touch -t 197001010000 "$tmp/utc"
TZ=America/Los_Angeles touch -t 197001010000 "$tmp/pacific"
mtime() {
stat -c %Y "$1" 2>/dev/null || stat -f %m "$1"
}
test "$(mtime "$tmp/utc")" != "$(mtime "$tmp/pacific")"Repository: gominimal/minimal
Length of output: 233
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- target script ---'
sed -n '1,140p' crates/common/fuzz/scripts/gen-seeds.sh
printf '%s\n' '--- related seed and archive references ---'
rg -n -C 3 'links_c0_s3|links_c4_s1|197001010000|bsdtar|TZ=|LC_ALL' crates/common/fuzz .github 2>/dev/null || true
printf '%s\n' '--- repository files near fuzz seeds ---'
git ls-files crates/common/fuzz | sed -n '1,160p'Repository: gominimal/minimal
Length of output: 9476
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
mkdir -p "$tmp/tree"
printf x > "$tmp/tree/target"
ln -s target "$tmp/tree/link"
TZ=UTC touch -t 197001010000 "$tmp/tree/target"
TZ=America/Los_Angeles touch -t 197001010000 "$tmp/tree/link" || true
python3 - "$tmp/tree/target" "$tmp/tree/link" <<'PY'
import os
import sys
for path in sys.argv[1:]:
st = os.lstat(path)
print(path, "lstat_mtime", st.st_mtime)
if os.path.islink(path):
st_target = os.stat(path)
print(path, "target_mtime", st_target.st_mtime)
PY
printf '%s\n' '--- touch implementation ---'
command -v touch
touch --help 2>&1 | sed -n '1,100p'
printf '%s\n' '--- committed seed tar mtimes ---'
python3 - <<'PY'
from pathlib import Path
for path in sorted(Path("crates/common/fuzz/seeds/archive_extract").glob("*")):
data = path.read_bytes()[2:]
entries = []
for offset in range(0, len(data), 512):
block = data[offset:offset + 512]
if not block or not any(block):
break
name = block[0:100].split(b"\0", 1)[0].decode("utf-8", "replace")
mtime_raw = block[136:148].rstrip(b"\0 ")
try:
mtime = int(mtime_raw or b"0", 8)
except ValueError:
mtime = f"invalid:{mtime_raw!r}"
typeflag = block[156:157] or b"0"
entries.append((name, typeflag.decode("ascii", "replace"), mtime))
if path.name.startswith("links_") and path.name.endswith("_c0_s3"):
print(path, entries)
PYRepository: gominimal/minimal
Length of output: 2300
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import bz2
import gzip
import lzma
root = Path("crates/common/fuzz/seeds/archive_extract")
def decode(path):
payload = path.read_bytes()[2:]
suffix = path.name.split("_c", 1)[1].split("_", 1)[0]
if suffix == "1":
return gzip.decompress(payload)
if suffix == "3":
return lzma.decompress(payload)
if suffix == "4":
return bz2.decompress(payload)
return payload
def entries(tar):
result = []
offset = 0
while offset + 512 <= len(tar):
header = tar[offset:offset + 512]
if not any(header):
break
name = header[0:100].split(b"\0", 1)[0].decode("utf-8", "replace")
kind = (header[156:157] or b"0").decode("ascii", "replace")
raw_size = header[124:136].rstrip(b"\0 ")
raw_mtime = header[136:148].rstrip(b"\0 ")
size = int(raw_size or b"0", 8)
mtime = int(raw_mtime or b"0", 8)
result.append((name, kind, mtime))
offset += 512 + ((size + 511) // 512) * 512
return result
for path in sorted(root.glob("links_*")):
print(path.name, entries(decode(path)))
PYRepository: gominimal/minimal
Length of output: 1591
Normalize bsdtar source timestamps in UTC. Export TZ=UTC and use touch -h -t 197001010000 so symlink timestamps are updated without touching their targets. Remove 2>/dev/null || true; timestamp failures must stop seed generation. Regenerate the affected seeds, including links_c4_s1.
🤖 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/scripts/gen-seeds.sh` around lines 93 - 94, Update the
source timestamp normalization command in the seed-generation script to run with
TZ=UTC and use touch -h -t 197001010000, preserving symlink timestamps without
modifying targets. Remove the stderr suppression and failure fallback so
timestamp errors terminate generation, then regenerate all affected seeds,
including links_c4_s1.
Source: MCP tools
| for tree in "${trees[@]}"; do | ||
| base="$work/$tree.tar" | ||
| # Fixed mtime on the sources too: bsdtar has no --mtime for create. | ||
| find "$work/$tree" -exec touch -t 197001010000 {} + 2>/dev/null || true |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
mkdir -p "$tmp/tree"
printf x > "$tmp/outside"
ln -s "$tmp/outside" "$tmp/tree/link"
mtime() {
stat -c %Y "$1" 2>/dev/null || stat -f %m "$1"
}
before="$(mtime "$tmp/outside")"
find "$tmp/tree" -exec touch -t 197001010000 {} +
after="$(mtime "$tmp/outside")"
test "$before" != "$after"Repository: gominimal/minimal
Length of output: 198
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- target script ---'
sed -n '70,105p' crates/common/fuzz/scripts/gen-seeds.sh
printf '%s\n' '--- link payload references ---'
rg -n -C 2 'links_c0_s3|/etc/passwd|\\.\\./\\.\\./\\.\\./etc/passwd' crates/common/fuzz
printf '%s\n' '--- touch capabilities ---'
touch --help 2>&1 | sed -n '1,80p'
printf '%s\n' '--- isolated symlink behavior ---'
python3 - <<'PY'
import os
import subprocess
import tempfile
with tempfile.TemporaryDirectory() as d:
tree = os.path.join(d, "tree")
os.mkdir(tree)
outside = os.path.join(d, "outside")
link = os.path.join(tree, "link")
with open(outside, "wb") as f:
f.write(b"x")
os.symlink(outside, link)
before = os.stat(outside).st_mtime_ns
subprocess.run(
["find", tree, "-exec", "touch", "-t", "197001010000", "{}", "+"],
check=True,
)
followed = os.stat(outside).st_mtime_ns
link_after_follow = os.lstat(link).st_mtime_ns
subprocess.run(["touch", "-h", "-t", "197001010000", link], check=True)
outside_after_h = os.stat(outside).st_mtime_ns
link_after_h = os.lstat(link).st_mtime_ns
print({
"outside_changed_without_h": followed != before,
"outside_unchanged_with_h": outside_after_h == followed,
"link_changed_with_h": link_after_h != link_after_follow,
"outside_mtime_after_h": outside_after_h,
})
PYRepository: gominimal/minimal
Length of output: 4299
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- script setup and tree construction ---'
sed -n '1,70p' crates/common/fuzz/scripts/gen-seeds.sh
printf '%s\n' '--- references to the generator ---'
rg -n -C 3 'gen-seeds\.sh|fuzz.*seed|seed.*fuzz' README.md docs crates .github 2>/dev/null || true
printf '%s\n' '--- touch portability references ---'
rg -n -C 2 -- '--no-dereference|touch -h|touch .*197001010000|TZ=UTC|bsdtar|macOS' . --glob '!target/**' --glob '!node_modules/**' 2>/dev/null || true
printf '%s\n' '--- available touch implementations ---'
command -v touch || true
touch --version 2>&1 | sed -n '1,3p' || true
uname -srmRepository: gominimal/minimal
Length of output: 50376
🌐 Web query:
macOS BSD touch -h no-dereference symbolic link manual
💡 Result:
The standard touch utility included with macOS does not support a "no-dereference" flag (such as -h) to operate on a symbolic link itself rather than the target file it points to [1][2][3]. Although some manual pages or documentation may occasionally reference such options—often due to shared codebases with other BSD or GNU/Linux implementations—the native macOS version of touch consistently resolves symbolic links, meaning it updates the timestamp of the target file instead of the link [2][3]. If you require the functionality to update the timestamp of a symbolic link itself without dereferencing it, you can achieve this by installing the GNU coreutils package (e.g., via Homebrew), which provides a version of touch that includes the -h (--no-dereference) flag [3]. Alternatively, using custom scripts or utilities that interface with system functions like lutimes or setattrlist (with appropriate flags) can achieve the same result on macOS [3][4].
Citations:
- 1: https://leopard-adc.pepas.com/documentation/Darwin/Reference/ManPages/man1/touch.1.html
- 2: https://ss64.com/mac/touch.html
- 3: https://apple.stackexchange.com/questions/425660/i-do-not-understand-why-touch-h-is-not-working-as-documented-in-the-man-page
- 4: https://public-inbox.org/ruby-core/redmine.journal-50123.20141127035102.b1766499860f9c91@ruby-lang.org/
🌐 Web query:
libarchive bsdtar symlink mtime archive creation --mtime macOS
💡 Result:
In bsdtar, the --mtime option (available in newer versions) allows you to set a fixed modification time for files added to an archive [1][2]. When creating an archive, this option sets the modification time for all added file entries—including symbolic links—to the date specified [3][2]. Key details regarding bsdtar and mtime: 1. Behavior with --mtime: When you use --mtime in create (c), append (r), or update (u) modes, bsdtar forces the modification time of the archived entries to that specific date [1][4]. This behavior was explicitly implemented to support reproducible builds, similar to the functionality found in GNU tar [3][5]. 2. macOS Considerations: - While macOS uses specific metadata (often handled by the --mac-metadata option), the --mtime flag is a general libarchive feature and operates independently of macOS-specific attributes [1][4]. - Note that older versions of bsdtar (typically those prior to recent updates incorporating pull request #2601) may not support the --mtime flag at all, leading users to believe it is missing [6][5]. Ensure your version of libarchive/bsdtar is up to date if you encounter issues [7]. 3. Symlink Handling: By default, bsdtar preserves symlinks in an archive as symbolic link entries rather than following them to archive the target file [8][4]. When --mtime is applied, the timestamp of the symbolic link entry itself is set to the provided date, consistent with how it handles regular files [3][2]. If you are on macOS and require specific metadata preservation (such as extended attributes or ACLs), ensure you continue to use the --mac-metadata flag alongside your other options, as --mtime only affects the modification timestamp [1][4].
Citations:
- 1: https://man.freebsd.org/cgi/man.cgi?query=bsdtar
- 2: libarchive/libarchive@c26f037
- 3: bsdtar: Support
--mtimeand--clamp-mtimelibarchive/libarchive#2601 - 4: https://man.archlinux.org/man/core/libarchive/bsdtar.1.en
- 5: bsdtar: support
--mtimeand--clamp-mtimelibarchive/libarchive#971 - 6: https://stackoverflow.com/questions/70861118/is-there-an-equivalent-mtime-option-for-bsdtar
- 7: tar -C option fails to work with symlink on windows libarchive/libarchive#2705
- 8: https://github.com/libarchive/libarchive/blob/v3.7.7/tar/bsdtar.1
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- archive tool availability and options ---'
for tool in tar bsdtar gtar python3 perl; do
if command -v "$tool" >/dev/null 2>&1; then
printf '%s: %s\n' "$tool" "$(command -v "$tool")"
"$tool" --help 2>&1 | grep -E -i -- '--mtime|symlink|dereference|no-dereference|follow' | head -20 || true
fi
done
printf '%s\n' '--- repository requirements for generator tooling ---'
rg -n -C 2 'gen-seeds|bsdtar|libarchive|Python|python3|macOS.*tar|tar.*macOS' \
docs/fuzzing.md AGENTS.md CONTRIBUTING.md crates/common/fuzz justfile* 2>/dev/null || trueRepository: gominimal/minimal
Length of output: 4475
Prevent timestamp normalization from following symlinks.
Use a portable no-follow timestamp operation. Native macOS touch does not support -h. The current command can update /etc/passwd or another path outside $work during a privileged run. Remove 2>/dev/null || true.
🤖 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/scripts/gen-seeds.sh` at line 94, Replace the timestamp
normalization command in gen-seeds.sh so it updates each filesystem entry under
$work/$tree without following symlinks, using a portable operation supported on
macOS and Linux. Ensure symlink targets outside $work cannot be modified, and
remove the stderr suppression and unconditional success fallback so failures
remain visible.
Source: MCP tools
| )); | ||
| } | ||
| cwd.join(rel.as_utf8_path()) | ||
| /// Takes the raw user string, not a `SandboxPath`: `--output ../artifacts` is |
There was a problem hiding this comment.
Docstring over commentary: just say it resolves a user path that might be relative to their cwd to the absolute path within the sandbox mount namespace
| /// Relative variant — a validated [`RelPath<R>`], so it carries both the | ||
| /// realm and the no-`..` guarantee. | ||
| /// | ||
| /// It must stay a `RelPath` and not a bare path: the realm marker is the |
There was a problem hiding this comment.
Drop the commentary with claude talking to itself about the current pr
twitchyliquid64
left a comment
There was a problem hiding this comment.
LGTM, caveat nits about commentary in code comments
…d 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>
|
Both fixed in The pattern was doc comments arguing against a shape that is no longer in the tree: the
Net -35/+12. Tests and clippy green. |
A weekend fuzzing campaign on a Linux box (nightly + ASan — coverage this project has never had), landing two new fuzz targets and the four fixes they surfaced.
New targets
archive_extractextract_compressed_tar— build sources, OCI layers, remote-cache artifactspath_invariantsAbsPath/RelPath/EitherPathrealm gatesarchive_extractasserts containment after every extraction rather than trusting the return value — a successful extract that escaped the root trips neither a panic nor a sanitizer, and that is exactly the bug shape this target exists to catch. It is the surface that carried #651.Fixes
common::archive— the tar link-target check only ran on thestrip_prefix: Some(..)branch; an escaping symlink got through when no prefix was set.common::archive— cap the xz arm decompressed output (it is the one compression path that buffers the whole tar up front, so a decompression bomb was unbounded).paths—EitherPathcould mint an unvalidatedRelPath, bypassing the constructor gate.minvmd— bound oneshot RPC response reads.Notes for review
crates/paths/fuzz/is a new fuzz workspace: added tofuzz-cratesin both OS branches, with its own[workspace]and.gitignore, sojust fuzz-checkguards it.scripts/gen-seeds.sh) and committed — an unseeded byte fuzzer burns ~10^7 execs before stumbling onto a valid ustar header.cargo fmt --checkclean,cargo clippy -p common -p paths -p minvmd --all-targets -D warningsclean,cargo test -p common -p paths103 passing,just fuzz-checkgreen.A further fix from the same campaign (an SFTP/workspace-uploader chain) follows separately, along with a variant sweep for the same defect class.
Summary by CodeRabbit
Security
Bug Fixes
Tests
Documentation
Note
Add fuzz targets for archive extraction and path invariants, and fix four decoder hardening bugs
archive_extractlibFuzzer harness in crates/common/fuzz that exercisesextract_compressed_taracross five compression modes and four strip-prefix options, with a post-extraction containment check for symlinks and directory traversal.path_invariantslibFuzzer harness in crates/paths/fuzz that differentially testsRelPath,EitherPath, andFromStrconstructors for invariant consistency.extract_tar_implin archive.rs to skip escaping symlinks when nostrip_prefixis set, resolve hardlinks relative to the destination directory instead of process CWD, and skip hardlinks whose targets are missing.LimitedWriterwrapper; exceeding the cap returnsArchiveError::DecompressedTooLargebefore any extraction.EitherPath::newin paths/src/lib.rs to store a plainUtf8PathBufin theRelvariant instead of aRelPath, so inputs containing..no longer bypassRelPathvalidation.as_relnow returnsOption<&Utf8Path>.rpc_client::call_oneshotin rpc_client.rs to reject oneshot RPC responses larger than 8 MiB instead of buffering unboundedly.EitherPath::as_relreturn type changed fromOption<&RelPath<R>>toOption<&Utf8Path>; callers relying onRelPathmethods must re-validate.Changes since #1162 opened
paths::EitherPathto reject relative paths containing parent directory segments by replacing infalliblenewconstructor with fallibletry_newmethod, changing theRelvariant to storeRelPath<R>instead ofUtf8PathBuf, and updatingFromStrand serdeDeserializeimplementations to return validation errors [ddfd3e3]paths::CwdRelativeto store raw unresolved user input and allow climbing relative paths during resolution by replacing internalEitherPath<R>storage withUtf8PathBufandPhantomData<R>, and updatingresolvemethod to join climbing paths onto current working directory [ddfd3e3]minimaldcrate'sresolve_outputfunction to accept&Utf8Pathinstead ofSandboxPath, allowing relative output paths with parent directory segments, explicitly rejecting empty strings, and normalizing absolute paths withSandboxAbsPath::try_new[ddfd3e3]sessionscrate'sFileSet::walk_rootmethod to use fallibleHostPath::try_newand returnNonewhen computed root contains disallowed segments, and modifiedFileSet::walk_fsto skip matching entries that cannot form validHostPathinstances [ddfd3e3]mfile,minimal,sessionscrates and integration tests to constructHostPathvalues using fallibleHostPath::try_new(...).unwrap()instead of infallibleHostPath::new(...)[ddfd3e3]env::resolve_outputfunction [e374757]pathsmodule [e374757]Macroscope summarized 2f4772b.