Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions src/install/TarballStream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -893,13 +893,15 @@ impl TarballStream {

#[cfg(any(target_os = "linux", target_os = "android"))]
{
let size: usize = usize::try_from(entry.size().max(0)).expect("int cast");
// The header's size field is attacker-controlled; cap so a
// size lie can't fallocate more than one entry's worth of
// real disk before the truncated body is detected. The
// buffered path bounds this by the decompressed tar length;
// here the stream is incomplete, so use a fixed ceiling.
const PREALLOCATE_CEILING: i64 = 64 * 1024 * 1024;
let size = entry.size().clamp(0, PREALLOCATE_CEILING);
if size > 1_000_000 {
let _ = bun_sys::preallocate_file(
fd.native(),
0,
i64::try_from(size).expect("int cast"),
);
let _ = bun_sys::preallocate_file(fd.native(), 0, size);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
}

Expand Down
39 changes: 38 additions & 1 deletion src/install/extract_tarball.rs
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,40 @@ pub(crate) fn uses_streaming_extraction() -> bool {
.unwrap_or(false)
}

/// RAII owner of a temporary extraction directory under `parent`. Removes
/// `parent/name` on drop unless [`commit`](Self::commit) is called after the
/// directory has been renamed into the cache. This keeps failed extractions
/// (decompression errors, truncated tarballs, rename failures) from leaking an
/// extraction directory per attempt in `$TMPDIR`.
struct TempExtractionDir<'a> {
parent: Fd,
name: Option<&'a ZStr>,
}

impl<'a> TempExtractionDir<'a> {
#[inline]
fn new(parent: Fd, name: &'a ZStr) -> Self {
Self {
parent,
name: Some(name),
}
}

/// Disarm the drop guard after the directory has been renamed away.
#[inline]
fn commit(mut self) {
self.name = None;
}
}

impl Drop for TempExtractionDir<'_> {
fn drop(&mut self) {
if let Some(name) = self.name {
let _ = Dir::borrow(&self.parent).delete_tree(name.as_bytes());
}
}
}

impl ExtractTarball {
/// Derive the display name and a filesystem-safe basename for this
/// package. Shared by the buffered `extract()` path below and the
Expand Down Expand Up @@ -256,6 +290,7 @@ impl ExtractTarball {
let mut resolved: &'static [u8] = b"";
let tmpname =
FileSystem::tmpname(tmpname_suffix, &mut tmpname_buf.0, bun_core::fast_random())?;
let tmpdir_guard = TempExtractionDir::new(self.temp_dir, tmpname);
{
let extract_destination = match bun_sys::make_path::make_open_path(
tmpdir,
Expand Down Expand Up @@ -439,7 +474,9 @@ impl ExtractTarball {
}
}

self.move_to_cache_directory(log, tmpname, name, basename, resolved)
let result = self.move_to_cache_directory(log, tmpname, name, basename, resolved)?;
tmpdir_guard.commit();
Ok(result)
}

/// Rename the freshly-extracted temp directory into the cache, read
Expand Down
15 changes: 13 additions & 2 deletions src/libarchive/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1886,11 +1886,22 @@ impl Archiver {
// #define MAX_WRITE (1024 * 1024)
#[cfg(any(target_os = "linux", target_os = "android"))]
{
if size > 1_000_000 {
// The header's size field is attacker-controlled; a
// malicious tarball can claim 8 GiB for a 100-byte body
// and fallocate that much real disk before the short body
// is detected. Bound by the input buffer length: when
// `file_buffer` is already the raw tar (pre-decompressed)
// this is exact; when it is the compressed gzip stream
// (libarchive's filter gunzips on the fly) it
// under-preallocates, which is acceptable since
// preallocation is best-effort. Either way the cap is at
// most what the caller actually supplied.
let prealloc = size.min(file_buffer.len());
Comment thread
robobun marked this conversation as resolved.
if prealloc > 1_000_000 {
let _ = bun_sys::preallocate_file(
file_handle.native(),
0,
i64::try_from(size).expect("int cast"),
i64::try_from(prealloc).expect("int cast"),
);
}
}
Expand Down
115 changes: 115 additions & 0 deletions test/cli/install/bun-install-streaming-extract.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -709,3 +709,118 @@ test("buffered extract does not hold the decompressed local tarball in memory",
expect(maxRssBytes).toBeGreaterThan(0);
expect(maxRssBytes).toBeLessThan(2 * PAYLOAD_SIZE);
});

// -------------------------------------------------------------------
// Buffered extract: a tarball whose ustar header declares a size far
// larger than the body that follows must fail to extract without
// leaving its temporary extraction directory behind, and without
// allocating the declared size on disk. The declared size is
// attacker-controlled and unbounded relative to the input.
// -------------------------------------------------------------------
describe("buffered extract: malformed tarball cleanup", () => {
function treeSize(root: string): number {
let total = 0;
for (const entry of readdirSync(root, { withFileTypes: true })) {
const full = join(root, entry.name);
if (entry.isDirectory()) total += treeSize(full);
else if (entry.isFile()) total += statSync(full).size;
}
return total;
}

function leakedExtractionDirs(root: string): string[] {
// Temp extraction dirs are `.<hex>-<n>.<name>` directly under $TMPDIR.
return readdirSync(root, { withFileTypes: true })
.filter(d => d.isDirectory() && d.name.startsWith("."))
.map(d => d.name);
}

async function runInstallIsolated(root: string) {
const tmp = join(root, "bun-tmp");
const cache = join(root, "bun-cache");
mkdirSync(tmp, { recursive: true });
mkdirSync(cache, { recursive: true });
await using proc = Bun.spawn({
cmd: [bunExe(), "install", "--linker=hoisted"],
cwd: root,
env: {
...bunEnv,
BUN_TMPDIR: tmp,
TMPDIR: tmp,
TEMP: tmp,
TMP: tmp,
BUN_INSTALL_CACHE_DIR: cache,
},
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
return { stdout, stderr, exitCode, tmp, cache };
}

test("tar header size far exceeding body fails cleanly without leaking temp disk", async () => {
// A valid package.json entry followed by a file entry whose header
// claims 16 MiB but whose body is only 100 bytes; libarchive reports
// a truncated archive once it runs out of input. 16 MiB is well above
// the 1 MB preallocation threshold, so the unfixed build fallocated
// the full declared size on Linux before failing.
const DECLARED = 16 * 1024 * 1024;
const pj = Buffer.from(JSON.stringify({ name: "pkg", version: "1.0.0" }));
const body = Buffer.alloc(100, 0x78);
const tar = Buffer.concat([
tarHeader("package/package.json", pj.length, "0"),
pj,
pad512(pj.length),
tarHeader("package/big.bin", DECLARED, "0"),
body,
pad512(body.length),
Buffer.alloc(1024, 0),
]);
const tgz = gzipSync(tar);
// The tarball itself is tiny; the damage is in the declared size.
expect(tgz.length).toBeLessThan(1024);

using dir = tempDir("tar-size-lie", {
"package.json": JSON.stringify({
name: "app",
version: "1.0.0",
dependencies: { pkg: "file:./pkg.tgz" },
}),
});
writeFileSync(join(String(dir), "pkg.tgz"), tgz);

// Run twice to confirm the leak does not accumulate per attempt.
let lastStderr = "";
for (let i = 0; i < 2; i++) {
const { stderr, exitCode, tmp, cache } = await runInstallIsolated(String(dir));
lastStderr = stderr;
expect(stderr).toContain("extracting tarball");
expect(exitCode).not.toBe(0);
// The failed extraction's temp directory must be gone.
expect({ leaked: leakedExtractionDirs(tmp) }).toEqual({ leaked: [] });
// Nothing close to the declared size was left anywhere under the
// test-isolated tmp or cache directories.
expect(treeSize(tmp) + treeSize(cache)).toBeLessThan(1024 * 1024);
}
expect(existsSync(join(String(dir), "node_modules", "pkg"))).toBe(false);
expect(lastStderr).not.toBe("");
});

test("a tarball that fails to decompress does not leak its temp directory", async () => {
// Not a gzip stream at all: the buffered extractor creates its temp
// directory, then libarchive fails to open the input as an archive.
using dir = tempDir("tar-bad-gzip", {
"package.json": JSON.stringify({
name: "app",
version: "1.0.0",
dependencies: { pkg: "file:./pkg.tgz" },
}),
});
writeFileSync(join(String(dir), "pkg.tgz"), Buffer.from("this is not a gzip stream"));

const { stderr, exitCode, tmp } = await runInstallIsolated(String(dir));
expect(stderr).toContain("error:");
expect({ leaked: leakedExtractionDirs(tmp) }).toEqual({ leaked: [] });
expect(exitCode).not.toBe(0);
});
});
Loading