diff --git a/src/install/TarballStream.rs b/src/install/TarballStream.rs index 2eccd74be538..5cc3c478eb5c 100644 --- a/src/install/TarballStream.rs +++ b/src/install/TarballStream.rs @@ -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); } } diff --git a/src/install/extract_tarball.rs b/src/install/extract_tarball.rs index b8d3a8b091b5..6f1bb1d7c5f9 100644 --- a/src/install/extract_tarball.rs +++ b/src/install/extract_tarball.rs @@ -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 @@ -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, @@ -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 diff --git a/src/libarchive/lib.rs b/src/libarchive/lib.rs index 83b9b2a6312e..a36c21fa9f63 100644 --- a/src/libarchive/lib.rs +++ b/src/libarchive/lib.rs @@ -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()); + 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"), ); } } diff --git a/test/cli/install/bun-install-streaming-extract.test.ts b/test/cli/install/bun-install-streaming-extract.test.ts index c77868293ea2..b4d17aa6e448 100644 --- a/test/cli/install/bun-install-streaming-extract.test.ts +++ b/test/cli/install/bun-install-streaming-extract.test.ts @@ -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 `.-.` 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); + }); +});