install: skip overlong PAX paths in streaming tarball extraction - #36533
install: skip overlong PAX paths in streaming tarball extraction#36533robobun wants to merge 4 commits into
Conversation
TarballStream::begin_entry copies each entry's pathname into a fixed-size OSPathBuffer via normalize_buf_t without checking the input length. PAX extended headers can carry paths of arbitrary length, so a crafted registry tarball with a path longer than the buffer (4096 bytes on Linux, 1024 on macOS, 32767 u16 units on Windows) aborts the install with a slice-index panic. The buffered extractor (Archiver::extract_to_dir) already has this guard; add the same check to the streaming path and skip the entry, matching the behaviour for other unrepresentable entries. Adopts the remaining piece of #31160; the buffered-path guard and the leading-.. guard for the streaming path have already landed separately. Co-authored-by: pc-style <pcstyle@duck.com>
|
Warning Review limit reached
Next review available in: 6 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Reproduced on a release build by serving a tarball with a 40 000-byte PAX With the guard the overlong entry is skipped and the install completes; the entry placed after it in the tarball is still extracted correctly. Follow-up from self-review: tightened the bound by one in both extractors ( CI: the new test passes on every lane in build 86128. The remaining red is unrelated flake (bitbucket.org TLS handshake in |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Intentional overlap with #31160: this adopts the remaining piece of that PR (the streaming-path length guard), rebased onto current main. The buffered-path length guard from #31160 already landed via #31339, and the streaming-path leading- |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mirrors the existing overlong-path guard from the buffered extractor into the streaming path.
What was reviewed:
- Guard placement matches
Archiver::extract_to_dirat src/libarchive/lib.rs:1497 (>=reserves the NUL slot written atnorm_buf[norm_len] = 0). - Skip path (
phase = WantData; out_fd = None; return Ok(())) is identical to the other skip branches inbegin_entry(empty,., leading.., Windows absolute). - Test forces the streaming path via
BUN_INSTALL_STREAMING_MIN_SIZE=1, asserts theStreamedverbose line, and checks the entry after the overlong one is extracted — so a broken state-machine reset would fail it.
Extended reasoning...
Overview
Adds a single length guard in TarballStream::begin_entry (src/install/TarballStream.rs) that skips tarball entries whose path (after stripping the leading package/ component) is at least as long as the fixed-size OSPathBuffer. Without it, normalize_buf_t panics with a slice-bounds error on PAX-header paths longer than the buffer, aborting the whole install. Also adds a regression test in test/cli/install/bun-install-streaming-extract.test.ts.
Security risks
The change removes a DoS vector: a malicious registry package with a >4096-byte (Linux) or >32767-unit (Windows) PAX path could crash any bun install that streams it. The skip behavior is identical to what the buffered extractor already does at src/libarchive/lib.rs:1497 and to the other unrepresentable-entry branches in the same function, so no new attack surface is introduced. The overlong entry's data is drained via WantData with out_fd = None, so nothing is written to disk.
Level of scrutiny
Low-to-medium. The Rust change is 5 lines that copy an existing, cited guard from the sibling extractor into the streaming path, using the same comparison (>=) and the same skip idiom used throughout begin_entry. norm_buf.len() on OSPathBuffer is the compile-time array length, matching how the buffered path uses normalized_buf.len().
Other factors
The test follows the file's established harness (makeRegistry, runInstall, tempDir, bunEnv), uses a 40 000-char hash-chained hex path so it exceeds the buffer on every platform and gzip can't collapse it, forces streaming with BUN_INSTALL_STREAMING_MIN_SIZE=1, and asserts both the Streamed verbose marker and that the entry placed after the overlong one lands correctly (proving the per-entry state machine recovered). PR description confirms the test fails without the fix and the full file passes with it. No outstanding reviewer comments; no prior claude reviews.
normalize_buf_t can return one character more than its input on Windows (a bare UNC volume name gains a trailing separator; bare C: becomes C:.), so a crafted path of length OSPathBuffer.len()-1 would still overflow the trailing-NUL write. Reserve one slot of headroom in both the streaming and buffered extractors, and assert the overlong entry produced no on-disk artifact in the test.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small defensive guard that mirrors the existing buffered-extractor check, with a solid regression test.
What was reviewed:
- The new length check in
begin_entryuses the same skip pattern (phase = WantData; out_fd = None; return Ok) as every other unrepresentable-entry case in that function. - Bound math:
rest.len() + 1 >= norm_buf.len()leaves room for the +1 Windows UNC/drive growth plus the trailing NUL write; the buffered-path tightening is conservative-only. - Test: 40 000-char hex path (no
/) survivestokenize_rest_after_firstat full length; asserts the streaming path ran, the following entry extracted, andreaddirSortedshows no truncated-name artifact.
Extended reasoning...
Overview
Adds a length guard in TarballStream::begin_entry before normalize_buf_t copies a PAX-supplied path into the fixed-size OSPathBuffer, and tightens the equivalent existing guard in Archiver::extract_to_dir by one to account for the Windows UNC/drive growth plus the NUL terminator written immediately after normalization. A new test in bun-install-streaming-extract.test.ts serves a tarball with a 40 000-char PAX path from a drip-feed registry and asserts the install completes with the overlong entry silently skipped.
Security risks
This is security-adjacent (untrusted tarball extraction) but strictly defensive: it converts a Rust slice-bounds panic (DoS) into a silent per-entry skip, matching how the buffered extractor and every other unrepresentable-entry case in begin_entry already behave. No new attack surface is added; the skipped entry's data blocks are consumed and discarded via the existing out_fd = None path in Phase::WantData.
Level of scrutiny
Low-to-medium. The Rust change is 7 added lines that copy the exact skip pattern used four other times in the same function, plus a one-character tightening of an existing bound in the sibling extractor. The bound arithmetic checks out: proceed only when buflen >= rest.len() + 2, which covers worst-case output length (input + 1 on Windows) plus the NUL byte written at norm_buf[norm_len]. Over-rejection is at most one character at the platform max-path boundary, which is harmless.
Other factors
- The PR description shows the test failing on both debug/ASAN and release builds without the fix and passing with it.
- The test's long name is hex (no
/), so after thepackage/prefix is stripped the full 40 000 chars reach the guard on every platform (>1024 macOS, >4096 Linux, >32767 Windows). - All comment-cop inline threads are resolved; the two-line comments that remain justify the non-obvious
+1and are appropriate per REVIEW.md's "durable non-obvious content" rule. - This adopts the last outstanding hunk from #31160; the buffered-path guard already landed via #31339.
|
Updated 3:38 AM PT - Jul 31st, 2026
✅ @robobun, your commit 9ff04fdd88c5885b689fd236ccb7dd64ee3c5e29 passed in 🧪 To try this PR locally: bunx bun-pr 36533That installs a local version of the PR into your bun-36533 --bun |
|
Superseded. The streaming extractor's length guard landed in ff512ea (#36165) shortly after this PR was opened, and the buffered extractor has had the same guard since cd1ad59 (#31339). The test added here passes unmodified against current main, so there is no longer a fail-before case for it. The one thing left in this diff, the |
1 participant
What
TarballStream::begin_entrycopies each entry's pathname into a fixed-sizeOSPathBuffervianormalize_buf_twithout checking the input length first. PAX extended headers can carry paths of arbitrary length, so a crafted registry tarball with a path longer than the buffer aborts the install with:The buffered extractor (
Archiver::extract_to_dir) already has this guard atsrc/libarchive/lib.rs:1497; this adds the same check to the streaming path and skips the entry, matching the behaviour for other unrepresentable entries (empty,./.., absolute on Windows).Why
The streaming extractor is the default for any registry tarball above 2 MiB, so a malicious package can crash
bun installfor anyone depending on it. Since the Rust port it is a clean panic/abort (DoS only, not memory corruption), but it still takes down the whole install.This adopts the remaining piece of #31160 by @pc-style. The buffered-path length guard from that PR landed separately in #31339, and the leading-
..guard for the streaming path was already present; only the streaming-path length guard remained.How verified
New test in
test/cli/install/bun-install-streaming-extract.test.tsserves a tarball containing a 40 000-character PAXpath(longer thanOSPathBufferon every platform) from a drip-feed local registry so the streaming extractor commits to it, then asserts the install completes, the entry following the overlong one is extracted correctly, and theStreamedverbose line confirms the streaming path ran.Without the fix (
bun bdwithsrc/stashed) the spawned install aborts with the panic above and the test fails; with the fix the full file (10 tests) passes.[review] gate passed · iteration 1 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file