From 0f80fd1bebdbf6adf8a315884a21372db5493789 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 1 Jul 2026 11:05:44 +0000 Subject: [PATCH 1/2] Bun.mmap, Bun.Glob, Bun.FileSystemRouter: reject interior null bytes in path arguments Bun.mmap(path), Glob.scan/scanSync (both the pattern and the cwd option), and new FileSystemRouter({ dir }) passed their path arguments through to C-string syscalls without checking for interior NUL bytes. C paths stop at the first NUL, so a string that passed JS-level validation reached the OS as a different, shorter path (CWE-158). For example: new Bun.Glob("*.ts").scanSync({ cwd: safe + "\0/../secret" }) scans `safe/`, while every JS-level check (and path.resolve) saw a path resolving to `secret/`. Bun.file() and node:fs already reject these inputs with ERR_INVALID_ARG_VALUE. Generalize the existing Valid::path_null_bytes helper (the one Bun.file uses) to take the argument name, and call it from the three entry points. Glob's match() and constructor are intentionally unchanged: match() never touches the filesystem, so the pattern is only validated when scan/scanSync turn it into a path. Also make the pre-existing FileSystemRouter 32-bit-hash-collision test deterministic; its 600k-iteration birthday search alone exceeded the default per-test timeout on debug+ASAN builds. --- src/runtime/api/BunObject.rs | 1 + src/runtime/api/filesystem_router.rs | 2 + src/runtime/api/glob.rs | 6 +++ src/runtime/node/types.rs | 34 ++++++++++------ test/js/bun/glob/scan.test.ts | 46 ++++++++++++++++++++++ test/js/bun/util/filesystem_router.test.ts | 36 +++++++++-------- test/js/bun/util/mmap.test.js | 7 ++++ 7 files changed, 105 insertions(+), 27 deletions(-) diff --git a/src/runtime/api/BunObject.rs b/src/runtime/api/BunObject.rs index 1a64301a51ee..948b91a3c71b 100644 --- a/src/runtime/api/BunObject.rs +++ b/src/runtime/api/BunObject.rs @@ -1749,6 +1749,7 @@ pub(crate) fn mmap_file(global_this: &JSGlobalObject, callframe: &CallFrame) -> global_this.throw_invalid_arguments(format_args!("Path too long")) ); } + node::Valid::no_null_bytes(path_str.slice(), "path", "a string", global_this)?; let paths = &[path_str.slice()]; break 'brk bun_paths::resolve_path::join_abs_string_buf::< bun_paths::resolve_path::platform::Auto, diff --git a/src/runtime/api/filesystem_router.rs b/src/runtime/api/filesystem_router.rs index d39d6d12d922..48afb2fc5c3d 100644 --- a/src/runtime/api/filesystem_router.rs +++ b/src/runtime/api/filesystem_router.rs @@ -41,6 +41,7 @@ use bun_router::{self as Router, Match as RouterMatch, RouteConfig}; use bun_url::{CombinedScanner, QueryStringMap, URL, route_param}; use crate::api::bun_object; +use crate::node::Valid; use crate::webcore::{Request, Response}; use bun_bundler as Transpiler; @@ -157,6 +158,7 @@ impl FileSystemRouter { .throw_invalid_arguments(format_args!("Expected dir to be a string"))); } let root_dir_path_ = dir.to_slice(global_this)?; + Valid::no_null_bytes(root_dir_path_.slice(), "dir", "a string", global_this)?; if !(root_dir_path_.slice().is_empty() || root_dir_path_.slice() == b".") { // resolve relative path if needed let path_ = root_dir_path_.slice(); diff --git a/src/runtime/api/glob.rs b/src/runtime/api/glob.rs index f0e6bd58a993..030b0707831a 100644 --- a/src/runtime/api/glob.rs +++ b/src/runtime/api/glob.rs @@ -13,6 +13,8 @@ use bun_paths::resolve_path::join_string_buf; use bun_paths::{self as resolve_path, MAX_PATH_BYTES, PathBuffer, platform}; use bun_sys as syscall; +use crate::node::Valid; + // Codegen hooks (JSGlob): toJS / fromJS / fromJSDirect are provided by the // generated C++ wrapper. See PORTING.md §JSC ".classes.ts-backed types". #[bun_jsc::JsClass] @@ -52,6 +54,7 @@ impl ScanOpts { fn_name, MAX_PATH_BYTES ))); } + Valid::no_null_bytes(cwd_utf8.slice(), "cwd", "a string", global_this)?; // If its absolute return as is if resolve_path::Platform::AUTO.is_absolute(cwd_utf8.slice()) { @@ -301,6 +304,9 @@ impl Glob { fn_name: &'static str, arena: &mut Arena, ) -> JsResult>> { + // The pattern's literal prefix is opened as a C path; `match()` never + // touches the filesystem, so only scan/scanSync reject interior NULs. + Valid::no_null_bytes(&self.pattern, "pattern", "a string", global_this)?; let Some(match_opts) = ScanOpts::from_js(global_this, arguments, fn_name, arena)? else { return Ok(None); }; diff --git a/src/runtime/node/types.rs b/src/runtime/node/types.rs index cce2e751b754..5b455279e79c 100644 --- a/src/runtime/node/types.rs +++ b/src/runtime/node/types.rs @@ -1436,18 +1436,30 @@ impl Valid { } pub fn path_null_bytes(slice: &[u8], global: &JSGlobalObject) -> JsResult<()> { - if strings::index_of_char(slice, 0).is_some() { - return Err(global - .err( - jsc::ErrorCode::INVALID_ARG_VALUE, - format_args!( - "The argument 'path' must be a string, Uint8Array, or URL without null bytes. Received {}", - bun_fmt::quote(slice) - ), - ) - .throw()); + Self::no_null_bytes(slice, "path", "a string, Uint8Array, or URL", global) + } + + /// Reject interior NUL bytes in a path-like string argument with Node's + /// `ERR_INVALID_ARG_VALUE`. C paths stop at the first NUL, so a value + /// that passed JS-level validation would reach the syscall truncated. + pub fn no_null_bytes( + slice: &[u8], + name: &str, + accepts: &str, + global: &JSGlobalObject, + ) -> JsResult<()> { + if strings::index_of_char(slice, 0).is_none() { + return Ok(()); } - Ok(()) + Err(global + .err( + jsc::ErrorCode::INVALID_ARG_VALUE, + format_args!( + "The argument '{name}' must be {accepts} without null bytes. Received {}", + bun_fmt::quote(slice) + ), + ) + .throw()) } } diff --git a/test/js/bun/glob/scan.test.ts b/test/js/bun/glob/scan.test.ts index 01a4b6ec6fa4..464c12ef8524 100644 --- a/test/js/bun/glob/scan.test.ts +++ b/test/js/bun/glob/scan.test.ts @@ -1125,3 +1125,49 @@ describe.skipIf(!canCreateDirSymlink)("literal path segment through a symlinked expect(norm(result)).toEqual(["linkdir/file.txt"]); }); }); + +// The cwd and the pattern's literal prefix both reach the OS as C strings, +// which stop at the first NUL: "/safe\0/../secret" would scan "/safe" +// while JS-level validation saw a path resolving to "/secret". +describe("glob scan rejects interior null bytes", () => { + const nulError = expect.objectContaining({ code: "ERR_INVALID_ARG_VALUE" }); + + function makeNulTree(prefix: string) { + return tempDir(prefix, { + "safe/inside.ts": "x", + "secret/leaked.ts": "x", + }); + } + + test("scanSync rejects a cwd option containing a null byte", () => { + using dir = makeNulTree("glob-nul-cwd-sync"); + const evil = `${dir}/safe\0/../secret`; + expect(() => Array.from(new Glob("*.ts").scanSync({ cwd: evil }))).toThrow(nulError); + expect(() => Array.from(new Glob("*.ts").scanSync(evil))).toThrow(nulError); + }); + + // `scan` runs the same argument validation as `scanSync` before scheduling + // any work, so a bad cwd/pattern throws from the `scan()` call itself. + test("scan rejects a cwd option containing a null byte", () => { + using dir = makeNulTree("glob-nul-cwd-async"); + const evil = `${dir}/safe\0/../secret`; + expect(() => new Glob("*.ts").scan({ cwd: evil })).toThrow(nulError); + expect(() => new Glob("*.ts").scan(evil)).toThrow(nulError); + }); + + test("scanSync rejects a pattern containing a null byte", () => { + using dir = makeNulTree("glob-nul-pattern-sync"); + expect(() => Array.from(new Glob(`${dir}/secret\0trailer/*.ts`).scanSync())).toThrow(nulError); + }); + + test("scan rejects a pattern containing a null byte", () => { + using dir = makeNulTree("glob-nul-pattern-async"); + expect(() => new Glob(`${dir}/secret\0trailer/*.ts`).scan()).toThrow(nulError); + }); + + test("match never touches the filesystem so null bytes stay plain characters", () => { + expect(new Glob("a\0b").match("a\0b")).toBe(true); + expect(new Glob("a\0b").match("ab")).toBe(false); + expect(new Glob("*.ts").match("a\0b.ts")).toBe(true); + }); +}); diff --git a/test/js/bun/util/filesystem_router.test.ts b/test/js/bun/util/filesystem_router.test.ts index 51248c37db71..268d4d5ad61d 100644 --- a/test/js/bun/util/filesystem_router.test.ts +++ b/test/js/bun/util/filesystem_router.test.ts @@ -456,6 +456,17 @@ it("dir should be validated", async () => { }).toThrow("Expected dir to be a string"); }); +it("dir with an interior null byte is rejected", () => { + const { dir } = make(["index.tsx"]); + // The C path stops at the first NUL, so "\0suffix" would otherwise + // list while route resolution used a different JS-level path. + const nulError = expect.objectContaining({ code: "ERR_INVALID_ARG_VALUE" }); + expect(() => new FileSystemRouter({ style: "nextjs", dir: `${dir}\0suffix` })).toThrow(nulError); + expect(() => new FileSystemRouter({ style: "nextjs", dir: `${dir}\0suffix` })).toThrow( + "must be a string without null bytes", + ); +}); + it("origin should be validated", async () => { const { dir } = make(["posts.tsx"]); @@ -682,21 +693,14 @@ it("caps the number of parsed query string parameters instead of crashing", asyn }); it("does not match a dynamic route whose static segment merely collides on length and 32-bit hash", () => { - const low32 = (input: string) => Number(BigInt.asUintN(32, BigInt(Bun.hash.wyhash(input)))); - const seen = new Map(); - let pair: [string, string] | null = null; - for (let i = 0; i < 600_000; i++) { - const candidate = "s" + i.toString(36).padStart(9, "0"); - const h = low32(candidate); - const prev = seen.get(h); - if (prev !== undefined) { - pair = [prev, candidate]; - break; - } - seen.set(h, candidate); - } - expect(pair).not.toBeNull(); - const [routeSegment, collidingSegment] = pair!; + // Route segment matching must compare bytes, not just (length, truncated + // 32-bit wyhash). Bun.hash.wyhash(s, 0) is the same hash the router stores + // for static route segments. This equal-length pair was found by a birthday + // search; the hash32 assertion keeps the test honest if wyhash ever changes. + const routeSegment = "s000000io9"; + const collidingSegment = "s000001eqf"; + const hash32 = (s: string) => Number(BigInt.asUintN(32, BigInt(Bun.hash.wyhash(s)))); + expect(hash32(collidingSegment)).toBe(hash32(routeSegment)); expect(collidingSegment).not.toBe(routeSegment); expect(collidingSegment.length).toBe(routeSegment.length); @@ -708,7 +712,7 @@ it("does not match a dynamic route whose static segment merely collides on lengt expect(router.match(`/${routeSegment}/42`)?.name).toBe(`/${routeSegment}/[id]`); expect(router.match(`/${collidingSegment}/42`)).toBeNull(); -}, 60_000); +}); it("match() does not panic on a leading '?' or a path that percent-decodes to empty", async () => { // URLPath::parse assumed the decoded pathname was non-empty and had a leading diff --git a/test/js/bun/util/mmap.test.js b/test/js/bun/util/mmap.test.js index 6d507d3b968e..1651a8b79707 100644 --- a/test/js/bun/util/mmap.test.js +++ b/test/js/bun/util/mmap.test.js @@ -85,6 +85,13 @@ describe.skipIf(isWindows)("Bun.mmap", async () => { expect(() => Bun.mmap(path, null)).not.toThrow(); }); + it("mmap rejects paths with interior null bytes", () => { + // The C path stops at the first NUL, so "a\0b" would silently map "a". + const evil = path + "\0.does-not-exist"; + expect(() => Bun.mmap(evil)).toThrow(expect.objectContaining({ code: "ERR_INVALID_ARG_VALUE" })); + expect(() => Bun.mmap(evil)).toThrow("must be a string without null bytes"); + }); + it("mmap handles non-number offset/size without crashing", () => { // These should not crash - non-number values coerce to 0 per JavaScript semantics // Previously these caused assertion failures (issue ENG-22413) From 3b69257d6d56b0e938f8d4518055f24eeecaacc5 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 1 Jul 2026 12:44:16 +0000 Subject: [PATCH 2/2] ci: retrigger