Repository navigation
fix(common): skip tar links with escaping targets instead of aborting (#651 follow-up) - #753
Conversation
…#651 follow-up) #651 contained a real write-outside-dest vuln in prefix-stripped extraction, but its symlink/hardlink handling is stricter than it needs to be: a link whose *target* escapes dest_dir aborts the whole extraction. Legitimate upstream tarballs ship exactly that in test fixtures — next.js (test/.../node_modules symlinks) and syft (symlink-resolution testdata) — so any rebuild of those packages now fails to extract at all (first hit by gominimal/pkgs#394/#402, whose dep bumps cascade into next/syft). Skip the offending link (with a warning) instead of erroring. This is equally safe against the write-through vector: the link is never created, so a later entry that would have written through it resolves to a contained regular path under dest_dir. The entry-path escape stays a hard error — a file writing to ../x has no benign form. Tests: extract_rejects_escaping_symlink becomes extract_skips_escaping_symlink (link skipped, siblings still extract), plus a new skipping_escaping_symlink_still_contains_write_through proving the tar-slip defense holds through the skip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
Why
#651 closed a real write-outside-
dest_dirvulnerability in prefix-stripped tar extraction — that protection stays. But one corner is stricter than the threat requires: a symlink/hardlink whose target escapesdest_diraborts the entire extraction.Legitimate upstream tarballs ship exactly that in test fixtures:
test/development/app-dir/ssr-in-rsc/node_modules/*workspace symlinkssyft/internal/fileresolver/testdata/symlinks-base/*(its own symlink-resolution test data)Since #651 reached prod (via the build-servers rev bump), any build that has to actually re-extract those sources fails outright:
tar entry … escapes the destination directory. First hits: gominimal/pkgs#394 (lcms2 → libvips → sharp/next) and gominimal/pkgs#402 (sqlite → … → syft) — both dead on both arches. Cache hits mask it, so it only fires when a dep bump forces a rebuild, and it will keep intermittently killing pkgs auto-PRs (and any full-catalog rebuild) until fixed.What
Skip the offending link (with a
tracing::warn!) instead of erroring. This is equally safe against the write-through (tar-slip) vector:dest_dir(the entry-path check keeps its own path in-tree).../xhas no benign form.Tests
extract_rejects_parent_dir_traversal— unchanged, still passes (fix(common): contain path traversal in prefix-stripped tar extraction #651's core defense intact).extract_rejects_escaping_symlink→extract_skips_escaping_symlink: the link is skipped (not created — verified viasymlink_metadata), sibling entries still extract.skipping_escaping_symlink_still_contains_write_through— the classic tar-slip sequence (escaping link, then a write through it) lands contained underdest_dir, not outside.extract_allows_in_tree_symlink— unchanged.All 9 archive tests green in the sandbox;
cargo fmtclean.Deployment note
To unblock the two pkgs PRs this needs: merge → build-servers pin bump → res-servers pick it up at next lease-start.
🤖 Generated with Claude Code