fix(install): guard against overlong PAX paths in tarball extraction - #31160
fix(install): guard against overlong PAX paths in tarball extraction#31160pc-style wants to merge 1 commit into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThis PR hardens tar processing by adding early bounds checks against fixed-size path buffers during streaming and extraction, plus a post-normalization check that rejects paths whose first component is ChangesTarball path validation security hardening
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@test/cli/install/bun-install.test.ts`:
- Around line 130-131: The test asserts the subprocess exit code (expect(await
exited).toBe(0)) before consuming/asserting output which hampers diagnostics;
reorder the assertions so you first read/assert output (e.g., expect(await new
Response(stderr).text()).not.toContain("Bun has crashed") and any stdout checks)
and only after those checks await and assert the exit code (exited) to be
0—locate the usages of exited, stderr and any stdout assertions in the test and
move the exit-code assertion to the very end.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 51aeaa0d-b2fe-4e0f-9094-b040d9997c05
📒 Files selected for processing (5)
src/install/TarballStream.rssrc/install/TarballStream.zigsrc/libarchive/lib.rssrc/libarchive/libarchive.zigtest/cli/install/bun-install.test.ts
927838a to
8774f0f
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/install/TarballStream.zig`:
- Around line 551-558: The added guard in TarballStream.zig that checks `if
(pathname.len >= norm_buf.len) { this.phase = .want_data; this.out_fd = null;
return; }` must not be kept in the Zig port; remove or revert this change so the
Zig reference remains unchanged and leave the security/length-guard fix only in
the Rust implementation. Locate the code around `normalizeBufT` and the
references to `pathname`, `norm_buf`, `this.phase`, and `this.out_fd` in
`TarballStream.zig` and restore the original behavior (no new length-check
sentinel rejection) so the Zig file remains a non-shipping porting reference
while the .rs sibling contains the actual fix.
In `@src/libarchive/libarchive.zig`:
- Around line 415-419: Remove the mirrored hardening checks from the Zig
reference implementation: delete the bounds-check branches that reject long PAX
paths (the `if (pathname.len >= normalized_buf.len) continue :loop;` check and
the similar checks around lines 425-431) in the
`normalizeBufT`/path-normalization section so the .zig file remains a
non-compiled reference; ensure no behavioral changes are made in the shipped
Rust port (the Rust code should retain the actual runtime checks), and leave a
brief comment noting that bounds/hardening belongs in the Rust implementation
rather than the .zig reference.
In `@test/cli/install/bun-install.test.ts`:
- Line 114: In the test fixture where you build the long package path (the
tarEntry call that uses paxEntry and currently uses "a".repeat(5000)), replace
the `.repeat()` usage with Buffer.alloc(count, fill).toString() to generate the
repetitive string; update the tarEntry("PaxHeader",
paxEntry(`package/${"a".repeat(5000)}`), "x") invocation to use
Buffer.alloc(5000, "a").toString() (keeping the same template and arguments) so
the test follows the repo pattern and avoids `.repeat()` in test fixtures.
- Line 131: Replace the negative crash-banner assertion that inspects stderr
(the line using new Response(stderr).text() and .not.toContain("Bun has
crashed")) with deterministic positive assertions: assert the process exit code
is 0 (use the test's exit/status variable, e.g., exitCode or status) and assert
stdout contains the expected install success message (read via new
Response(stdout).text()); remove the not.toContain("Bun has crashed") check and
use these two positive checks instead, updating the assertion targets (stderr ->
stdout) and the referenced variables in the current test.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c3fb06a9-ac2d-4cb0-9059-fe3aabacd7ba
📒 Files selected for processing (5)
src/install/TarballStream.rssrc/install/TarballStream.zigsrc/libarchive/lib.rssrc/libarchive/libarchive.zigtest/cli/install/bun-install.test.ts
12f2596 to
bad2c42
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
test/cli/install/bun-install.test.ts (2)
130-131:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid negative crash-banner assertion; assert deterministic success output and keep exit code last.
The
not.toContain("Bun has crashed")check is disallowed and can be non-actionable in CI. Assert expected success output first, then assert exit code.Suggested diff
- expect(await exited).toBe(0); - expect(await new Response(stderr).text()).not.toContain("Bun has crashed"); + expect(await new Response(stderr).text()).toContain("Saved lockfile"); + expect(await exited).toBe(0);As per coding guidelines: "Never write tests that check for no 'panic' or 'uncaught exception' or similar in output - these will never fail in CI" and "Assert the exit code last in tests - this gives a more useful error message on test failure".
🤖 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 `@test/cli/install/bun-install.test.ts` around lines 130 - 131, The test currently asserts a negative crash-banner check using expect(await new Response(stderr).text()).not.toContain("Bun has crashed") and then checks exit code via exited; remove the negative assertion and instead assert a deterministic success message from the CLI (use expect(await new Response(stderr).text()).toContain("<EXPECTED_SUCCESS_TEXT>") or similar) before finally asserting the exit code with expect(await exited).toBe(0); update the test to reference the same symbols (stderr, new Response(stderr).text(), and exited) so the success output is checked deterministically and the exit code assertion remains last.
114-114: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winReplace
.repeat()in test fixture string generation.Use the repo-required
Buffer.alloc(...).toString()pattern for long repeated strings.Suggested diff
- tarEntry("PaxHeader", paxEntry(`package/${"a".repeat(5000)}`), "x"), + tarEntry("PaxHeader", paxEntry(`package/${Buffer.alloc(5000, "a").toString()}`), "x"),As per coding guidelines: "To create repetitive strings in tests, use
Buffer.alloc(count, fill).toString()instead of"string".repeat(count)for better performance in debug builds".🤖 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 `@test/cli/install/bun-install.test.ts` at line 114, In the test fixture where tarEntry("PaxHeader", paxEntry(`package/${"a".repeat(5000)}`), "x") is used, replace the `"a".repeat(5000)` call with the repo-approved pattern using Buffer.alloc to build the long string (e.g. Buffer.alloc(5000, "a").toString()) so the paxEntry input uses Buffer.alloc(...).toString() instead of String.prototype.repeat; update the expression passed into paxEntry accordingly while keeping the surrounding tarEntry and paxEntry calls unchanged.
🤖 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.
Duplicate comments:
In `@test/cli/install/bun-install.test.ts`:
- Around line 130-131: The test currently asserts a negative crash-banner check
using expect(await new Response(stderr).text()).not.toContain("Bun has crashed")
and then checks exit code via exited; remove the negative assertion and instead
assert a deterministic success message from the CLI (use expect(await new
Response(stderr).text()).toContain("<EXPECTED_SUCCESS_TEXT>") or similar) before
finally asserting the exit code with expect(await exited).toBe(0); update the
test to reference the same symbols (stderr, new Response(stderr).text(), and
exited) so the success output is checked deterministically and the exit code
assertion remains last.
- Line 114: In the test fixture where tarEntry("PaxHeader",
paxEntry(`package/${"a".repeat(5000)}`), "x") is used, replace the
`"a".repeat(5000)` call with the repo-approved pattern using Buffer.alloc to
build the long string (e.g. Buffer.alloc(5000, "a").toString()) so the paxEntry
input uses Buffer.alloc(...).toString() instead of String.prototype.repeat;
update the expression passed into paxEntry accordingly while keeping the
surrounding tarEntry and paxEntry calls unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1ff9910e-daaf-4fc7-aaf0-a961e5cc63d7
📒 Files selected for processing (5)
src/install/TarballStream.rssrc/install/TarballStream.zigsrc/libarchive/lib.rssrc/libarchive/libarchive.zigtest/cli/install/bun-install.test.ts
bad2c42 to
4c6995c
Compare
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 `@test/cli/install/bun-install.test.ts`:
- Around line 130-132: Move the exit-code assertion to the end of the test:
after you compute err from stderr and assert output contents (e.g.,
expect(err).toContain("Saved lockfile")), defer expect(await exited).toBe(0)
until after those output assertions so the test reports output mismatches before
exit-code failures; update the assertions around the variables stderr, err and
exited accordingly in the test block.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5864e9c2-540c-408b-8483-9def4ba65dea
📒 Files selected for processing (3)
src/install/TarballStream.rssrc/libarchive/lib.rstest/cli/install/bun-install.test.ts
Tarballs with PAX pathnames longer than OSPathBuffer (4096 bytes) cause normalizeBufT to write past the buffer, crashing Bun with SIGBUS. Add bounds checks before calling normalizeBufT in both buffered (Archiver.extractToDir) and streaming (TarballStream) extraction paths. Also add leading '..' rejection to the buffered extraction path to match the existing guard in TarballStream.beginEntry. Adds regression test for bun install on tarballs with 5000-byte PAX filenames.
4c6995c to
d4538b5
Compare
|
Thanks for this, and sorry it sat for so long. Both halves of the fix have since landed on main through the hardening passes: the buffered (libarchive) extractor got the same length guard in cd1ad59 (#31339), and the streaming extractor got it in ff512ea (#36165), each with a regression test ( The additional leading Closing since the branch now conflicts with main and there is nothing left for it to change, but the fix itself is in. Thanks again for the report and the patch. |
2 participants
Summary
Fixes a crash in
bun installwhen extracting a malicious npm tarball containing an overlong PAX pathname.normalizeBufTwrites into a fixed-sizeOSPathBuffer(4096 bytes on macOS) and assumes callers provide enough space. A tarball can provide an arbitrarily long PAXpath, causing extraction to write past the buffer and crash with a bus error. In local testing with Bun 1.3.12, the fault address was attacker-controlled-looking (0x6161616161616161).This PR adds bounds checks before
normalizeBufTin both extraction paths:Archiver.extractToDirbuffered extractionTarballStreamstreaming extractionIt also adds leading
..rejection to the buffered extraction path to match the existingTarballStream.beginEntrytraversal guard.Security impact
Crash-only / denial of service. This does not claim RCE.
Requirements:
bun installon a project depending on a malicious npm package tarballpathlonger thanOSPathBufferTest
Added a regression test that generates a gzipped tarball with a 5000-byte PAX pathname and verifies
bun installexits successfully without printing the crash banner.Validation
git diff --checkpassed for touched fileszig fmt --stdinrustup run nightly-2026-05-06 cargo fmtI also attempted
bun bd, but currentmainfailed in an unrelated subsystem before reaching these files: