diff --git a/src/install/lockfile/Tree.rs b/src/install/lockfile/Tree.rs index 4f867c8e83f3..50a512c04d3b 100644 --- a/src/install/lockfile/Tree.rs +++ b/src/install/lockfile/Tree.rs @@ -90,7 +90,7 @@ pub(crate) fn depth_buf_uninit() -> DepthBuf { impl Tree { pub fn folder_name<'b>(&self, deps: &'b [Dependency], buf: &'b [u8]) -> &'b [u8] { let dep_id = self.dependency_id; - if dep_id == invalid_dependency_id { + if dep_id == invalid_dependency_id || dep_id as usize >= deps.len() { return b""; } deps[dep_id as usize].name.slice(buf) diff --git a/src/install/lockfile/bun.lockb.rs b/src/install/lockfile/bun.lockb.rs index 6dc55dca00e9..6ba3140220bb 100644 --- a/src/install/lockfile/bun.lockb.rs +++ b/src/install/lockfile/bun.lockb.rs @@ -427,6 +427,42 @@ pub(crate) fn load( )); } + // Per-package and per-tree `ExternalSlice` (off,len) pairs and tree + // indices are memcpy'd verbatim from disk; reject out-of-range ones so + // the installer warns + re-resolves instead of panicking at use. + { + let deps_len = lockfile.buffers.dependencies.len(); + let res_len = lockfile.buffers.resolutions.len(); + let hoisted_len = lockfile.buffers.hoisted_dependencies.len(); + let trees_len = lockfile.buffers.trees.len(); + for (dep_slice, res_slice) in lockfile + .packages + .items_dependencies() + .iter() + .zip(lockfile.packages.items_resolutions().iter()) + { + if !dep_slice.in_bounds(deps_len) || !res_slice.in_bounds(res_len) { + return Err(bun_core::err!("InvalidLockfile")); + } + } + for tree in lockfile.buffers.trees.iter() { + if !tree.dependencies.in_bounds(hoisted_len) { + return Err(bun_core::err!("InvalidLockfile")); + } + let dep_id = tree.dependency_id; + if dep_id != super::tree::ROOT_DEP_ID + && dep_id != crate::invalid_dependency_id + && dep_id as usize >= deps_len + { + return Err(bun_core::err!("InvalidLockfile")); + } + let parent = tree.parent; + if parent != super::tree::INVALID_ID && parent as usize >= trees_len { + return Err(bun_core::err!("InvalidLockfile")); + } + } + } + let has_workspace_name_hashes = false; // < Bun v1.0.4 stopped right here when reading the lockfile // So we add an extra 8 byte tag to say "hey, there's more data here" diff --git a/src/install_types/resolver_hooks.rs b/src/install_types/resolver_hooks.rs index b1aea8524e70..e79c36f616a8 100644 --- a/src/install_types/resolver_hooks.rs +++ b/src/install_types/resolver_hooks.rs @@ -109,18 +109,29 @@ impl ExternalSlice { #[inline] pub fn get(self, in_: &[T]) -> &[T] { - // Compute the sum in usize so - // the release-mode clamp applies instead of a debug u32-overflow panic. + // Both bounds are clamped: (off, len) may come straight off disk + // (bun.lockb, npm manifest cache), and a crafted `off > in_.len()` + // would otherwise panic in release even with `end` clamped. + let off = in_.len().min(self.off as usize); let end = in_.len().min(self.off as usize + self.len as usize); debug_assert!(self.off as usize + self.len as usize <= in_.len()); - &in_[self.off as usize..end] + &in_[off..end] } #[inline] pub fn mut_(self, in_: &mut [T]) -> &mut [T] { + let off = in_.len().min(self.off as usize); let end = in_.len().min(self.off as usize + self.len as usize); debug_assert!(self.off as usize + self.len as usize <= in_.len()); - &mut in_[self.off as usize..end] + &mut in_[off..end] + } + + /// True when `(off, len)` addresses a subrange of a buffer of length + /// `buf_len`. Used by the lockfile loader to reject crafted slices before + /// they reach `get()`/`mut_()`. + #[inline] + pub fn in_bounds(self, buf_len: usize) -> bool { + self.off as usize + self.len as usize <= buf_len } #[inline] diff --git a/test/cli/install/bun-lockb.test.ts b/test/cli/install/bun-lockb.test.ts index 69dd9e6df1c2..6ccd8eabeb48 100644 --- a/test/cli/install/bun-lockb.test.ts +++ b/test/cli/install/bun-lockb.test.ts @@ -1,5 +1,5 @@ import { file, spawn, write } from "bun"; -import { afterAll, beforeAll, expect, it } from "bun:test"; +import { afterAll, beforeAll, describe, expect, it } from "bun:test"; import { copyFile, exists, open, rm, writeFile } from "fs/promises"; import { bunExe, bunEnv as env, isWindows, runBunInstall, stderrForInstall, VerdaccioRegistry } from "harness"; import { join } from "path"; @@ -176,6 +176,134 @@ it("recovers from a corrupted binary lockfile instead of panicking", async () => expect(await exists(join(packageDir, "node_modules", "a-dep"))).toBe(true); }); +describe("rejects crafted ExternalSlice offsets and tree indices in bun.lockb instead of panicking", () => { + // Disk-loaded (off,len) slices and tree indices past their backing buffer + // must be rejected at load time so a tampered bun.lockb warns + re-resolves + // instead of panicking on the slice index. + async function lockbWithPkgs() { + const { packageDir, packageJson } = await registry.createTestDir({ bunfigOpts: { saveTextLockfile: false } }); + // Local folder deps keep this hermetic and produce a second Tree node + // (folder deps are never hoisted), so both package columns and + // tree[1]'s fields are available to corrupt. + await Promise.all([ + write( + packageJson, + JSON.stringify({ + name: "external-slice-lockb", + version: "1.0.0", + dependencies: { "pkg-a": "file:./pkg-a" }, + }), + ), + write( + join(packageDir, "pkg-a", "package.json"), + JSON.stringify({ + name: "pkg-a", + version: "1.0.0", + dependencies: { "pkg-b": "file:../pkg-b" }, + }), + ), + write(join(packageDir, "pkg-b", "package.json"), JSON.stringify({ name: "pkg-b", version: "1.0.0" })), + ]); + await runBunInstall(env, packageDir); + const lockbPath = join(packageDir, "bun.lockb"); + expect(await exists(lockbPath)).toBe(true); + const lockb = Buffer.from(await file(lockbPath).arrayBuffer()); + const fmt = lockb.readUInt32LE(42); + const N = Number(lockb.readBigUInt64LE(86)); + const begin = Number(lockb.readBigUInt64LE(110)); + const resolutionSize = fmt === 2 ? 64 : 72; + expect(N).toBeGreaterThanOrEqual(3); + // Trees are the first `Buffers` array, written as + // [start: u64][end: u64][ascii prefix][padding][20-byte records]. Each + // record is id(4) dependency_id(4) parent(4) off(4) len(4). + const prefixOff = lockb.indexOf("\n"); + expect(prefixOff).toBeGreaterThan(16); + const treesStart = Number(lockb.readBigUInt64LE(prefixOff - 16)); + const treesEnd = Number(lockb.readBigUInt64LE(prefixOff - 8)); + expect((treesEnd - treesStart) % 20).toBe(0); + expect(treesEnd - treesStart).toBeGreaterThanOrEqual(40); + return { packageDir, lockbPath, lockb, N, begin, resolutionSize, treesStart }; + } + + async function expectRecovers(packageDir: string, lockbPath: string, lockb: Buffer) { + await write(lockbPath, lockb); + await rm(join(packageDir, "node_modules"), { recursive: true, force: true }); + + const { stdout, stderr, exited } = spawn({ + cmd: [bunExe(), "install", "--no-progress"], + cwd: packageDir, + stdout: "pipe", + stderr: "pipe", + env, + }); + const [out, rawErr, code] = await Promise.all([stdout.text(), stderr.text(), exited]); + const err = stderrForInstall(rawErr); + + expect(err).toContain("Ignoring lockfile"); + expect(err).not.toContain("error:"); + expect(out).toContain("pkg-a@"); + expect(code).toBe(0); + expect(await exists(join(packageDir, "node_modules", "pkg-a", "package.json"))).toBe(true); + } + + it.concurrent("package dependencies slice with off > buffer length", async () => { + const { packageDir, lockbPath, lockb, N, begin, resolutionSize } = await lockbWithPkgs(); + // Dependencies column (ExternalSlice, 8 bytes each: off u32 + len u32). + const depsCol = begin + N * (8 + 8 + resolutionSize); + // Sanity: the root package's dependencies slice starts at 0. + expect(lockb.readUInt32LE(depsCol + 0 * 8)).toBe(0); + // Craft package[1].dependencies.off far past any plausible buffer end. + lockb.writeUInt32LE(0x7fffffff, depsCol + 1 * 8); + await expectRecovers(packageDir, lockbPath, lockb); + }); + + it.concurrent("package resolutions slice with off > buffer length", async () => { + const { packageDir, lockbPath, lockb, N, begin, resolutionSize } = await lockbWithPkgs(); + // Resolutions column (ExternalSlice, 8 bytes each) follows Dependencies. + const resCol = begin + N * (8 + 8 + resolutionSize + 8); + expect(lockb.readUInt32LE(resCol + 0 * 8)).toBe(0); + lockb.writeUInt32LE(0x7fffffff, resCol + 1 * 8); + await expectRecovers(packageDir, lockbPath, lockb); + }); + + it.concurrent("package dependencies slice with off + len > buffer length", async () => { + const { packageDir, lockbPath, lockb, N, begin, resolutionSize } = await lockbWithPkgs(); + const depsCol = begin + N * (8 + 8 + resolutionSize); + expect(lockb.readUInt32LE(depsCol + 0 * 8)).toBe(0); + // Craft package[1].dependencies.len far past the buffer end while + // leaving off in range, so `off..end` with only end clamped would + // succeed but the slice still addresses past the buffer. + lockb.writeUInt32LE(0x7fffffff, depsCol + 1 * 8 + 4); + await expectRecovers(packageDir, lockbPath, lockb); + }); + + it.concurrent("tree dependencies slice with off > hoisted buffer length", async () => { + const { packageDir, lockbPath, lockb, treesStart } = await lockbWithPkgs(); + // tree[0].dependencies.off + lockb.writeUInt32LE(0x7fffffff, treesStart + 0 * 20 + 12); + await expectRecovers(packageDir, lockbPath, lockb); + }); + + it.concurrent("tree dependency_id past dependencies buffer", async () => { + const { packageDir, lockbPath, lockb, treesStart } = await lockbWithPkgs(); + // Sanity: tree[1] is a non-root tree with a real dependency_id and + // parent; its sentinels are not in play. + expect(lockb.readUInt32LE(treesStart + 1 * 20 + 0)).toBe(1); + expect(lockb.readUInt32LE(treesStart + 1 * 20 + 8)).toBe(0); + // tree[1].dependency_id + lockb.writeUInt32LE(0x7fffffff, treesStart + 1 * 20 + 4); + await expectRecovers(packageDir, lockbPath, lockb); + }); + + it.concurrent("tree parent past trees buffer", async () => { + const { packageDir, lockbPath, lockb, treesStart } = await lockbWithPkgs(); + expect(lockb.readUInt32LE(treesStart + 1 * 20 + 8)).toBe(0); + // tree[1].parent + lockb.writeUInt32LE(0x7fffffff, treesStart + 1 * 20 + 8); + await expectRecovers(packageDir, lockbPath, lockb); + }); +}); + it("rejects a binary lockfile whose patched-dependency flag byte is out of range", async () => { const { packageDir, packageJson } = await registry.createTestDir({ bunfigOpts: { saveTextLockfile: false } });