From e32cbfd92b703e696f7e456d34101e5079257ef5 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 18 Sep 2026 21:27:11 +0000 Subject: [PATCH 1/8] shell mv: drop set-uid and set-gid across devices when the owner cannot be kept The EXDEV fallback ignored the result of fchown and then gave the copy the full source mode. A user who moved a set-uid file that another user owns got a set-uid file that the mover owns. POSIX mv and coreutils drop S_ISUID and S_ISGID in that case. Do the same for files and directories. --- src/runtime/shell/builtin/mv.rs | 21 +++++-- test/js/bun/shell/commands/mv.test.ts | 91 +++++++++++++++++++++++++-- 2 files changed, 102 insertions(+), 10 deletions(-) diff --git a/src/runtime/shell/builtin/mv.rs b/src/runtime/shell/builtin/mv.rs index 0f047bc29eb8..ea032bd8b97f 100644 --- a/src/runtime/shell/builtin/mv.rs +++ b/src/runtime/shell/builtin/mv.rs @@ -575,7 +575,8 @@ impl ShellMvBatchedTask { Self::move_across_devices(sd.fd(), name_z, dd.fd(), name_z)?; } #[cfg(unix)] - let _ = bun_sys::fchown(dd.fd(), st.st_uid as _, st.st_gid as _); + Self::copy_owner_and_mode(dd.fd(), &st); + #[cfg(windows)] let _ = bun_sys::fchmod(dd.fd(), mode & 0o7777); drop((sd, dd)); return bun_sys::rmdirat(src_dir, src); @@ -613,15 +614,23 @@ impl ShellMvBatchedTask { return Err(e); } #[cfg(unix)] - { - // `fchown` first: Linux clears S_ISUID/S_ISGID on chown. - let _ = bun_sys::fchown(out.fd(), st.st_uid as _, st.st_gid as _); - let _ = bun_sys::fchmod(out.fd(), mode & 0o7777); - } + Self::copy_owner_and_mode(out.fd(), &st); drop((in_, out)); bun_sys::unlinkat(src_dir, src) } + /// Gives `fd` the owner and mode of `st`. Without the owner, set-uid and + /// set-gid would act for a different user, so they are dropped (POSIX `mv`). + #[cfg(unix)] + fn copy_owner_and_mode(fd: bun_sys::Fd, st: &bun_sys::Stat) { + let mut mode = st.st_mode as bun_core::Mode & 0o7777; + // `fchown` first: Linux clears S_ISUID/S_ISGID on chown. + if bun_sys::fchown(fd, st.st_uid as _, st.st_gid as _).is_err() { + mode &= !(bun_sys::S::ISUID | bun_sys::S::ISGID); + } + let _ = bun_sys::fchmod(fd, mode); + } + /// `renameat(cwd, src, target_fd, basename(src))`. A free fn over the /// fields it touches so `src` can borrow `self.sources[_]` while `self.err` /// is written by the caller. diff --git a/test/js/bun/shell/commands/mv.test.ts b/test/js/bun/shell/commands/mv.test.ts index 4389044ae861..53497aaf09fe 100644 --- a/test/js/bun/shell/commands/mv.test.ts +++ b/test/js/bun/shell/commands/mv.test.ts @@ -1,6 +1,6 @@ import { $ } from "bun"; import { describe, expect, test } from "bun:test"; -import { isPosix } from "harness"; +import { bunEnv, bunExe, isPosix } from "harness"; import { accessSync, chmodSync, @@ -8,6 +8,7 @@ import { existsSync, lstatSync, mkdirSync, + readdirSync, readFileSync, readlinkSync, rmSync, @@ -87,10 +88,10 @@ describe("mv", async () => { const other = findCrossDeviceDir(); const skip = other === undefined; - function crossDevicePair(name: string): [src: string, dst: string] { + function crossDevicePair(name: string, roots: [string, string] = [tmp, other!]): [src: string, dst: string] { const base = `bun-mv-xdev-${process.pid}-${name}`; - const a = join(tmp, base); - const b = join(other!, base); + const a = join(roots[0], base); + const b = join(roots[1], base); for (const d of [a, b]) { rmSync(d, { recursive: true, force: true }); mkdirSync(d, { recursive: true }); @@ -233,5 +234,87 @@ describe("mv", async () => { rmSync(dst, { recursive: true, force: true }); } }); + + // Needs root: only root can create a file that another user owns. The move + // runs as `nobody`, who cannot enter the harness TMPDIR (mode 0700), so both + // ends are world-writable mounts. + const nobody = 65534; + const publicRoots: [string, string] = ["/tmp", "/dev/shm"]; + const publicRootsDiffer = (() => { + try { + return statSync(publicRoots[0]).dev !== statSync(publicRoots[1]).dev; + } catch { + return false; + } + })(); + + function ownerAndMode(root: string, names: string[]) { + return Object.fromEntries( + names.map(name => { + const { uid, mode } = statSync(join(root, name)); + return [name, `${(mode & 0o7777).toString(8)} uid=${uid}`]; + }), + ); + } + + test.skipIf(!isRoot || !publicRootsDiffer)( + "set-uid and set-gid are dropped across devices when the owner cannot be kept", + async () => { + const [src, dst] = crossDevicePair("setid", publicRoots); + try { + writeFileSync(join(src, "suid"), "suid"); + chmodSync(join(src, "suid"), 0o4755); + writeFileSync(join(src, "sgid"), "sgid"); + chmodSync(join(src, "sgid"), 0o2755); + mkdirSync(join(src, "dir")); + writeFileSync(join(src, "dir", "suid"), "dir/suid"); + chmodSync(join(src, "dir", "suid"), 0o4755); + // `nobody` removes the sources and creates the copies, so it needs write access. + chmodSync(join(src, "dir"), 0o2777); + chmodSync(src, 0o777); + chmodSync(dst, 0o777); + expect(ownerAndMode(src, ["suid", "sgid", "dir", "dir/suid"])).toEqual({ + "suid": "4755 uid=0", + "sgid": "2755 uid=0", + "dir": "2777 uid=0", + "dir/suid": "4755 uid=0", + }); + + // `own` belongs to the mover, so its owner is kept and its bits must be too. + const script = ` + import { $ } from "bun"; + import { chmodSync, writeFileSync } from "node:fs"; + const [src, dst] = ${JSON.stringify([src, dst])}; + writeFileSync(src + "/own", "own"); + chmodSync(src + "/own", 0o6755); + const r = await $\`mv \${src}/suid \${src}/sgid \${src}/own \${src}/dir \${dst}\`.nothrow(); + process.exit(r.exitCode); + `; + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", script], + env: bunEnv, + cwd: "/", + uid: nobody, + gid: nobody, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stdout, stderr, exitCode }).toEqual({ stdout: "", stderr: "", exitCode: 0 }); + + expect(ownerAndMode(dst, ["suid", "sgid", "dir", "dir/suid", "own"])).toEqual({ + "suid": `755 uid=${nobody}`, + "sgid": `755 uid=${nobody}`, + "dir": `777 uid=${nobody}`, + "dir/suid": `755 uid=${nobody}`, + "own": `6755 uid=${nobody}`, + }); + expect(readdirSync(src)).toEqual([]); + } finally { + rmSync(src, { recursive: true, force: true }); + rmSync(dst, { recursive: true, force: true }); + } + }, + ); }); }); From a4fc2c0ec52b1c023293e207aa846e0e7e7b3435 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 19 Sep 2026 04:37:37 +0000 Subject: [PATCH 2/8] test(shell mv): add a non-root case for the set-uid rule --- test/js/bun/shell/commands/mv.test.ts | 69 ++++++++++++++++++++++++++- 1 file changed, 68 insertions(+), 1 deletion(-) diff --git a/test/js/bun/shell/commands/mv.test.ts b/test/js/bun/shell/commands/mv.test.ts index 53497aaf09fe..bc90b5b020df 100644 --- a/test/js/bun/shell/commands/mv.test.ts +++ b/test/js/bun/shell/commands/mv.test.ts @@ -1,6 +1,6 @@ import { $ } from "bun"; import { describe, expect, test } from "bun:test"; -import { bunEnv, bunExe, isPosix } from "harness"; +import { bunEnv, bunExe, isLinux, isPosix } from "harness"; import { accessSync, chmodSync, @@ -257,6 +257,73 @@ describe("mv", async () => { ); } + function canAccess(path: string, mode: number) { + try { + accessSync(path, mode); + return true; + } catch { + return false; + } + } + + // The same rule without root, so that a CI lane runs it. A set-id file that + // another user owns needs root to create, so this takes one that the system + // already has. `mv` copies it first, then fails to remove it, which leaves + // the system file in place. Linux only: macOS protects /usr/bin, so the + // rename can fail before it reports EXDEV. + const foreignSetId = (() => { + if (!isLinux || isRoot) return undefined; + const mover = process.getuid!(); + for (const path of ["/usr/bin/passwd", "/usr/bin/sudo", "/bin/su", "/usr/bin/chsh", "/usr/bin/gpasswd"]) { + let uid: number; + let mode: number; + try { + ({ uid, mode } = statSync(path)); + } catch { + continue; + } + mode &= 0o7777; + // `fchown` fails only for an owner that this user cannot set. + if ((mode & (0o4000 | 0o2000)) === 0 || uid === mover) continue; + if (!canAccess(path, constants.R_OK)) continue; + // A writable parent would let `mv` delete the system file. + if (canAccess(join(path, ".."), constants.W_OK)) continue; + const { dev } = statSync(path); + for (const root of [other, "/dev/shm", tmp]) { + if (root === undefined) continue; + try { + if (statSync(root).dev === dev) continue; + } catch { + continue; + } + if (canAccess(root, constants.W_OK | constants.X_OK)) return { path, uid, mode, root }; + } + } + return undefined; + })(); + + test.skipIf(!foreignSetId)("set-uid is dropped across devices for a file that another user owns", async () => { + const { path, uid, mode, root } = foreignSetId!; + const dst = join(root, `bun-mv-xdev-${process.pid}-foreign`); + rmSync(dst, { recursive: true, force: true }); + mkdirSync(dst, { recursive: true }); + try { + const r = await $`mv ${path} ${join(dst, "copy")}`.quiet(); + // The copy comes first and the removal last, so a removal that fails + // with EACCES still leaves the copy. + expect(r.stderr.toString()).toBe(`mv: ${path}: Permission denied\n`); + expect(r.exitCode).toBe(13); + expect(ownerAndMode(dst, ["copy"])).toEqual({ + copy: `${(mode & ~(0o4000 | 0o2000)).toString(8)} uid=${process.getuid!()}`, + }); + expect(ownerAndMode(join(path, ".."), [path.slice(path.lastIndexOf("/") + 1)])).toEqual({ + [path.slice(path.lastIndexOf("/") + 1)]: `${mode.toString(8)} uid=${uid}`, + }); + } finally { + rmSync(dst, { recursive: true, force: true }); + } + }); + test.skipIf(!isRoot || !publicRootsDiffer)( "set-uid and set-gid are dropped across devices when the owner cannot be kept", async () => { From dd107ab2a2e5faa4f115916d5e4de4c1b3c647bc Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 19 Sep 2026 05:09:18 +0000 Subject: [PATCH 3/8] shell mv: keep the cross-device copy private to the mover until it has its owner The destination was created with the full source mode, so a root mover held a root-owned set-uid file while it copied a user's content. Create the file and the directory with owner-only bits and no set-id bits. copy_owner_and_mode applies the source mode after fchown, as before. The non-root test now selects its system binary with lstat and requires a regular file, because mv copies a symlink as a symlink. --- src/runtime/shell/builtin/mv.rs | 14 ++++--- test/js/bun/shell/commands/mv.test.ts | 56 ++++++++++++++++++++++++--- 2 files changed, 59 insertions(+), 11 deletions(-) diff --git a/src/runtime/shell/builtin/mv.rs b/src/runtime/shell/builtin/mv.rs index ea032bd8b97f..13b912890825 100644 --- a/src/runtime/shell/builtin/mv.rs +++ b/src/runtime/shell/builtin/mv.rs @@ -507,6 +507,10 @@ impl ShellMvBatchedTask { ) -> Result<(), bun_sys::Error> { use bun_sys::{Dir, E, File, O, S, Tag}; + // The copy belongs to the mover until `copy_owner_and_mode` runs. Until then + // no other user can reach it, and it has no set-id bits (coreutils `omitted_permissions`). + const OWNER_ONLY: bun_core::Mode = 0o700; + let st = bun_sys::lstatat(src_dir, src)?; let mode = st.st_mode as bun_core::Mode; @@ -545,15 +549,13 @@ impl ShellMvBatchedTask { return Err(bun_sys::Error::from_code(E::ENOENT, Tag::rename)); } let st = sst; - let mode = st.st_mode as bun_core::Mode; - // `| 0o700` so children can be written even when the source mode is read-only; restored via `fchmod` below. - if let Err(e) = bun_sys::mkdirat(dst_dir, dst, (mode & 0o7777) | 0o700) { + if let Err(e) = bun_sys::mkdirat(dst_dir, dst, OWNER_ONLY) { if e.get_errno() != E::EEXIST { return Err(e); } // Refuse to merge into a non-empty dest (matches same-device `ENOTEMPTY`). bun_sys::rmdirat(dst_dir, dst)?; - bun_sys::mkdirat(dst_dir, dst, (mode & 0o7777) | 0o700)?; + bun_sys::mkdirat(dst_dir, dst, OWNER_ONLY)?; } let dd = Dir::from_fd(shell_openat( dst_dir, @@ -577,7 +579,7 @@ impl ShellMvBatchedTask { #[cfg(unix)] Self::copy_owner_and_mode(dd.fd(), &st); #[cfg(windows)] - let _ = bun_sys::fchmod(dd.fd(), mode & 0o7777); + let _ = bun_sys::fchmod(dd.fd(), st.st_mode as bun_core::Mode & 0o7777); drop((sd, dd)); return bun_sys::rmdirat(src_dir, src); } @@ -605,7 +607,7 @@ impl ShellMvBatchedTask { dst_dir, dst.as_bytes(), O::WRONLY | O::CREAT | O::TRUNC | O::CLOEXEC | O::NOFOLLOW, - mode & 0o7777, + mode & OWNER_ONLY, )?; let _ = bun_sys::preallocate_file(out.fd().native(), 0, st.st_size as _); if let Err(e) = bun_sys::copy_file(in_.fd(), out.fd()) { diff --git a/test/js/bun/shell/commands/mv.test.ts b/test/js/bun/shell/commands/mv.test.ts index bc90b5b020df..a3d793f56ce3 100644 --- a/test/js/bun/shell/commands/mv.test.ts +++ b/test/js/bun/shell/commands/mv.test.ts @@ -15,6 +15,7 @@ import { statSync, symlinkSync, writeFileSync, + type Stats, } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "path"; @@ -235,6 +236,50 @@ describe("mv", async () => { } }); + test.skipIf(skip)("directory and file modes are kept across devices", async () => { + const [src, dst] = crossDevicePair("modes"); + try { + const srcDir = join(src, "tree"); + mkdirSync(srcDir); + writeFileSync(join(srcDir, "f.txt"), "F\n"); + chmodSync(join(srcDir, "f.txt"), 0o604); + chmodSync(srcDir, 0o750); + + const r = await $`mv ${srcDir} ${dst}`.quiet(); + expect(r.stderr.toString()).toBe(""); + expect(r.exitCode).toBe(0); + expect({ + tree: (statSync(join(dst, "tree")).mode & 0o7777).toString(8), + file: (statSync(join(dst, "tree", "f.txt")).mode & 0o7777).toString(8), + }).toEqual({ tree: "750", file: "604" }); + } finally { + rmSync(src, { recursive: true, force: true }); + rmSync(dst, { recursive: true, force: true }); + } + }); + + // The copy belongs to the mover until the move gives it its owner. The FIFO + // stops the move partway, which shows the mode the copy has until then. + test.skipIf(skip)("a directory in flight across devices is private to the mover", async () => { + const [src, dst] = crossDevicePair("in-flight"); + try { + const srcDir = join(src, "tree"); + mkdirSync(srcDir); + chmodSync(srcDir, 0o755); + const { exitCode: mk } = Bun.spawnSync({ cmd: ["mkfifo", join(srcDir, "pipe")] }); + expect(mk).toBe(0); + + const r = await $`mv ${srcDir} ${dst}`.quiet(); + expect(r.stderr.toString()).toContain("not supported"); + expect(r.exitCode).not.toBe(0); + expect((statSync(join(dst, "tree")).mode & 0o7777).toString(8)).toBe("700"); + expect(lstatSync(join(srcDir, "pipe")).isFIFO()).toBe(true); + } finally { + rmSync(src, { recursive: true, force: true }); + rmSync(dst, { recursive: true, force: true }); + } + }); + // Needs root: only root can create a file that another user owns. The move // runs as `nobody`, who cannot enter the harness TMPDIR (mode 0700), so both // ends are world-writable mounts. @@ -275,20 +320,21 @@ describe("mv", async () => { if (!isLinux || isRoot) return undefined; const mover = process.getuid!(); for (const path of ["/usr/bin/passwd", "/usr/bin/sudo", "/bin/su", "/usr/bin/chsh", "/usr/bin/gpasswd"]) { - let uid: number; - let mode: number; + // `lstat`: `mv` copies a symlink as a symlink, and Alpine links these names to busybox. + let st: Stats; try { - ({ uid, mode } = statSync(path)); + st = lstatSync(path); } catch { continue; } - mode &= 0o7777; + if (!st.isFile()) continue; + const { uid, dev } = st; + const mode = st.mode & 0o7777; // `fchown` fails only for an owner that this user cannot set. if ((mode & (0o4000 | 0o2000)) === 0 || uid === mover) continue; if (!canAccess(path, constants.R_OK)) continue; // A writable parent would let `mv` delete the system file. if (canAccess(join(path, ".."), constants.W_OK)) continue; - const { dev } = statSync(path); for (const root of [other, "/dev/shm", tmp]) { if (root === undefined) continue; try { From c1e4a4aaf47d8fa0b439328af7c0ff01d7ac00b5 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 19 Sep 2026 05:22:14 +0000 Subject: [PATCH 4/8] shell mv: shorten the comments in the cross-device fallback and its tests --- src/runtime/shell/builtin/mv.rs | 6 ++---- test/js/bun/shell/commands/mv.test.ts | 18 ++++++------------ 2 files changed, 8 insertions(+), 16 deletions(-) diff --git a/src/runtime/shell/builtin/mv.rs b/src/runtime/shell/builtin/mv.rs index 13b912890825..933837d26c7e 100644 --- a/src/runtime/shell/builtin/mv.rs +++ b/src/runtime/shell/builtin/mv.rs @@ -507,8 +507,7 @@ impl ShellMvBatchedTask { ) -> Result<(), bun_sys::Error> { use bun_sys::{Dir, E, File, O, S, Tag}; - // The copy belongs to the mover until `copy_owner_and_mode` runs. Until then - // no other user can reach it, and it has no set-id bits (coreutils `omitted_permissions`). + // The copy belongs to the mover until `copy_owner_and_mode` runs. const OWNER_ONLY: bun_core::Mode = 0o700; let st = bun_sys::lstatat(src_dir, src)?; @@ -621,8 +620,7 @@ impl ShellMvBatchedTask { bun_sys::unlinkat(src_dir, src) } - /// Gives `fd` the owner and mode of `st`. Without the owner, set-uid and - /// set-gid would act for a different user, so they are dropped (POSIX `mv`). + /// POSIX `mv`: the copy gets set-uid and set-gid only if it also gets the owner. #[cfg(unix)] fn copy_owner_and_mode(fd: bun_sys::Fd, st: &bun_sys::Stat) { let mut mode = st.st_mode as bun_core::Mode & 0o7777; diff --git a/test/js/bun/shell/commands/mv.test.ts b/test/js/bun/shell/commands/mv.test.ts index a3d793f56ce3..87695277730a 100644 --- a/test/js/bun/shell/commands/mv.test.ts +++ b/test/js/bun/shell/commands/mv.test.ts @@ -258,8 +258,7 @@ describe("mv", async () => { } }); - // The copy belongs to the mover until the move gives it its owner. The FIFO - // stops the move partway, which shows the mode the copy has until then. + // The FIFO stops the move partway, which shows the mode the copy has while the move runs. test.skipIf(skip)("a directory in flight across devices is private to the mover", async () => { const [src, dst] = crossDevicePair("in-flight"); try { @@ -280,10 +279,8 @@ describe("mv", async () => { } }); - // Needs root: only root can create a file that another user owns. The move - // runs as `nobody`, who cannot enter the harness TMPDIR (mode 0700), so both - // ends are world-writable mounts. const nobody = 65534; + // `nobody` runs the move and cannot enter the harness TMPDIR (mode 0700). const publicRoots: [string, string] = ["/tmp", "/dev/shm"]; const publicRootsDiffer = (() => { try { @@ -311,12 +308,9 @@ describe("mv", async () => { } } - // The same rule without root, so that a CI lane runs it. A set-id file that - // another user owns needs root to create, so this takes one that the system - // already has. `mv` copies it first, then fails to remove it, which leaves - // the system file in place. Linux only: macOS protects /usr/bin, so the - // rename can fail before it reports EXDEV. + // Only root can create a set-id file that another user owns, so the non-root case moves one the system has. const foreignSetId = (() => { + // macOS protects /usr/bin, so the rename can fail before it reports EXDEV. if (!isLinux || isRoot) return undefined; const mover = process.getuid!(); for (const path of ["/usr/bin/passwd", "/usr/bin/sudo", "/bin/su", "/usr/bin/chsh", "/usr/bin/gpasswd"]) { @@ -355,8 +349,7 @@ describe("mv", async () => { mkdirSync(dst, { recursive: true }); try { const r = await $`mv ${path} ${join(dst, "copy")}`.quiet(); - // The copy comes first and the removal last, so a removal that fails - // with EACCES still leaves the copy. + // `mv` copies first and removes last, so the failed removal leaves the copy. expect(r.stderr.toString()).toBe(`mv: ${path}: Permission denied\n`); expect(r.exitCode).toBe(13); expect(ownerAndMode(dst, ["copy"])).toEqual({ @@ -370,6 +363,7 @@ describe("mv", async () => { } }); + // Root only: no other user can create a file that someone else owns. test.skipIf(!isRoot || !publicRootsDiffer)( "set-uid and set-gid are dropped across devices when the owner cannot be kept", async () => { From a587ccf914f77fd60d716cd8ec5d9348f5a5a7ce Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 19 Sep 2026 05:36:44 +0000 Subject: [PATCH 5/8] test(shell mv): skip the root case when nobody cannot run bun, accept EROFS in the non-root case --- test/js/bun/shell/commands/mv.test.ts | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/test/js/bun/shell/commands/mv.test.ts b/test/js/bun/shell/commands/mv.test.ts index 87695277730a..34b256354708 100644 --- a/test/js/bun/shell/commands/mv.test.ts +++ b/test/js/bun/shell/commands/mv.test.ts @@ -349,9 +349,9 @@ describe("mv", async () => { mkdirSync(dst, { recursive: true }); try { const r = await $`mv ${path} ${join(dst, "copy")}`.quiet(); - // `mv` copies first and removes last, so the failed removal leaves the copy. - expect(r.stderr.toString()).toBe(`mv: ${path}: Permission denied\n`); - expect(r.exitCode).toBe(13); + // `mv` copies first and removes last, so the failed removal (EACCES, or EROFS on a read-only /usr) leaves the copy. + expect(r.stderr.toString()).toStartWith(`mv: ${path}: `); + expect(r.exitCode).not.toBe(0); expect(ownerAndMode(dst, ["copy"])).toEqual({ copy: `${(mode & ~(0o4000 | 0o2000)).toString(8)} uid=${process.getuid!()}`, }); @@ -363,8 +363,18 @@ describe("mv", async () => { } }); + // A checkout under /root is out of reach for `nobody`. + function nobodyCanRunBun() { + try { + const asNobody = { env: bunEnv, cwd: "/", uid: nobody, gid: nobody }; + return Bun.spawnSync({ cmd: [bunExe(), "--revision"], ...asNobody }).success; + } catch { + return false; + } + } + // Root only: no other user can create a file that someone else owns. - test.skipIf(!isRoot || !publicRootsDiffer)( + test.skipIf(!isRoot || !publicRootsDiffer || !nobodyCanRunBun())( "set-uid and set-gid are dropped across devices when the owner cannot be kept", async () => { const [src, dst] = crossDevicePair("setid", publicRoots); From f43c46f071e3aa6e991f3a0089937a5aebfb6258 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 19 Sep 2026 05:59:00 +0000 Subject: [PATCH 6/8] shell mv: give a partial cross-device directory copy its owner and mode A directory move that fails partway returned before copy_owner_and_mode ran, so the partial copy kept the private in-flight mode and the mover as its owner. The entries that moved before the failure exist only in that copy. Apply the owner and mode to the directory also when the move of its entries fails, as coreutils does. --- src/runtime/shell/builtin/mv.rs | 39 ++++++++++++++------- test/js/bun/shell/commands/mv.test.ts | 50 ++++++++++++++++++++++----- 2 files changed, 68 insertions(+), 21 deletions(-) diff --git a/src/runtime/shell/builtin/mv.rs b/src/runtime/shell/builtin/mv.rs index 933837d26c7e..c278ed18a330 100644 --- a/src/runtime/shell/builtin/mv.rs +++ b/src/runtime/shell/builtin/mv.rs @@ -562,24 +562,14 @@ impl ShellMvBatchedTask { O::RDONLY | O::DIRECTORY | O::NOFOLLOW, 0, )?); - // Boxed: `WrappedIterator` embeds an 8 KB inline readdir buffer. - let mut iter = Box::new(bun_sys::dir_iterator::iterate(sd.fd())); - let mut nbuf = bun_paths::path_buffer_pool::get(); - while let Some(entry) = iter.next()? { - let name = entry.name.slice_u8(); - if name.len() >= bun_paths::MAX_PATH_BYTES { - return Err(bun_sys::Error::from_code(E::ENAMETOOLONG, Tag::rename)); - } - nbuf[..name.len()].copy_from_slice(name); - nbuf[name.len()] = 0; - let name_z = ZStr::from_buf(&nbuf[..], name.len()); - Self::move_across_devices(sd.fd(), name_z, dd.fd(), name_z)?; - } + let moved = Self::move_entries_across_devices(sd.fd(), dd.fd()); + // Also after a failure: the entries already moved exist only in the copy. #[cfg(unix)] Self::copy_owner_and_mode(dd.fd(), &st); #[cfg(windows)] let _ = bun_sys::fchmod(dd.fd(), st.st_mode as bun_core::Mode & 0o7777); drop((sd, dd)); + moved?; return bun_sys::rmdirat(src_dir, src); } @@ -620,6 +610,29 @@ impl ShellMvBatchedTask { bun_sys::unlinkat(src_dir, src) } + fn move_entries_across_devices( + src_dir: bun_sys::Fd, + dst_dir: bun_sys::Fd, + ) -> Result<(), bun_sys::Error> { + // Boxed: `WrappedIterator` embeds an 8 KB inline readdir buffer. + let mut iter = Box::new(bun_sys::dir_iterator::iterate(src_dir)); + let mut nbuf = bun_paths::path_buffer_pool::get(); + while let Some(entry) = iter.next()? { + let name = entry.name.slice_u8(); + if name.len() >= bun_paths::MAX_PATH_BYTES { + return Err(bun_sys::Error::from_code( + bun_sys::E::ENAMETOOLONG, + bun_sys::Tag::rename, + )); + } + nbuf[..name.len()].copy_from_slice(name); + nbuf[name.len()] = 0; + let name_z = ZStr::from_buf(&nbuf[..], name.len()); + Self::move_across_devices(src_dir, name_z, dst_dir, name_z)?; + } + Ok(()) + } + /// POSIX `mv`: the copy gets set-uid and set-gid only if it also gets the owner. #[cfg(unix)] fn copy_owner_and_mode(fd: bun_sys::Fd, st: &bun_sys::Stat) { diff --git a/test/js/bun/shell/commands/mv.test.ts b/test/js/bun/shell/commands/mv.test.ts index 34b256354708..1900eb702025 100644 --- a/test/js/bun/shell/commands/mv.test.ts +++ b/test/js/bun/shell/commands/mv.test.ts @@ -258,21 +258,55 @@ describe("mv", async () => { } }); - // The FIFO stops the move partway, which shows the mode the copy has while the move runs. + // What moved before the failure exists only in the copy, so the copy must not keep its private in-flight mode. + test.skipIf(skip)("a directory move that fails partway across devices gives the copy its mode", async () => { + const [src, dst] = crossDevicePair("partial"); + const dirs = [join(src, "tree"), join(dst, "tree")]; + try { + mkdirSync(dirs[0]); + const { exitCode: mk } = Bun.spawnSync({ cmd: ["mkfifo", join(dirs[0], "pipe")] }); + expect(mk).toBe(0); + chmodSync(dirs[0], 0o555); + + const r = await $`mv ${dirs[0]} ${dst}`.quiet(); + expect(r.stderr.toString()).toContain("not supported"); + expect(r.exitCode).not.toBe(0); + expect((statSync(dirs[1]).mode & 0o7777).toString(8)).toBe("555"); + expect(lstatSync(join(dirs[0], "pipe")).isFIFO()).toBe(true); + } finally { + for (const dir of dirs) if (existsSync(dir)) chmodSync(dir, 0o700); + rmSync(src, { recursive: true, force: true }); + rmSync(dst, { recursive: true, force: true }); + } + }); + + // The copy gets its mode only after the last entry moves, so a sample can never fail on a correct build. test.skipIf(skip)("a directory in flight across devices is private to the mover", async () => { const [src, dst] = crossDevicePair("in-flight"); try { const srcDir = join(src, "tree"); mkdirSync(srcDir); + const entries = Array.from({ length: 200 }, (_, i) => join(srcDir, `f${i}`)); + for (const entry of entries) writeFileSync(entry, ""); chmodSync(srcDir, 0o755); - const { exitCode: mk } = Bun.spawnSync({ cmd: ["mkfifo", join(srcDir, "pipe")] }); - expect(mk).toBe(0); - const r = await $`mv ${srcDir} ${dst}`.quiet(); - expect(r.stderr.toString()).toContain("not supported"); - expect(r.exitCode).not.toBe(0); - expect((statSync(join(dst, "tree")).mode & 0o7777).toString(8)).toBe("700"); - expect(lstatSync(join(srcDir, "pipe")).isFIFO()).toBe(true); + let done = false; + const moving = $`mv ${srcDir} ${dst}`.quiet().then(r => ((done = true), r)); + const otherModes = new Set(); + let next = 0; + while (!done) { + const copy = statSync(join(dst, "tree"), { throwIfNoEntry: false }); + // An entry that is still in the source after the sample proves that the move was in flight. + while (next < entries.length && !existsSync(entries[next])) next++; + if (copy && next < entries.length && (copy.mode & 0o7777) !== 0o700) { + otherModes.add((copy.mode & 0o7777).toString(8)); + } + await new Promise(resolve => setImmediate(resolve)); + } + const r = await moving; + expect(r.stderr.toString()).toBe(""); + expect(r.exitCode).toBe(0); + expect([...otherModes]).toEqual([]); } finally { rmSync(src, { recursive: true, force: true }); rmSync(dst, { recursive: true, force: true }); From 846125c55796771682564ebdf51ca63be84d2cce Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 19 Sep 2026 06:14:33 +0000 Subject: [PATCH 7/8] test(shell mv): require a sample of the move in flight, end the poll on any settlement --- test/js/bun/shell/commands/mv.test.ts | 58 ++++++++++++++------------- 1 file changed, 31 insertions(+), 27 deletions(-) diff --git a/test/js/bun/shell/commands/mv.test.ts b/test/js/bun/shell/commands/mv.test.ts index 1900eb702025..804f8b06216c 100644 --- a/test/js/bun/shell/commands/mv.test.ts +++ b/test/js/bun/shell/commands/mv.test.ts @@ -280,37 +280,41 @@ describe("mv", async () => { } }); - // The copy gets its mode only after the last entry moves, so a sample can never fail on a correct build. + // The copy gets its mode only after the last entry moves, so each sample of a move in flight must show 700. test.skipIf(skip)("a directory in flight across devices is private to the mover", async () => { - const [src, dst] = crossDevicePair("in-flight"); - try { - const srcDir = join(src, "tree"); - mkdirSync(srcDir); - const entries = Array.from({ length: 200 }, (_, i) => join(srcDir, `f${i}`)); - for (const entry of entries) writeFileSync(entry, ""); - chmodSync(srcDir, 0o755); - - let done = false; - const moving = $`mv ${srcDir} ${dst}`.quiet().then(r => ((done = true), r)); - const otherModes = new Set(); - let next = 0; - while (!done) { - const copy = statSync(join(dst, "tree"), { throwIfNoEntry: false }); - // An entry that is still in the source after the sample proves that the move was in flight. - while (next < entries.length && !existsSync(entries[next])) next++; - if (copy && next < entries.length && (copy.mode & 0o7777) !== 0o700) { - otherModes.add((copy.mode & 0o7777).toString(8)); + const inFlightModes = new Set(); + // A move can end before the first sample. Then a larger tree gives the sampler more time. + for (const count of [200, 2000]) { + const [src, dst] = crossDevicePair("in-flight"); + try { + const srcDir = join(src, "tree"); + mkdirSync(srcDir); + const entries = Array.from({ length: count }, (_, i) => join(srcDir, `f${i}`)); + for (const entry of entries) writeFileSync(entry, ""); + chmodSync(srcDir, 0o755); + + let done = false; + const moving = $`mv ${srcDir} ${dst}`.quiet().finally(() => (done = true)); + let next = 0; + while (!done) { + for (let burst = 0; burst < 64; burst++) { + const copy = statSync(join(dst, "tree"), { throwIfNoEntry: false }); + // An entry that is still in the source after the sample proves that the move was in flight. + while (next < entries.length && !existsSync(entries[next])) next++; + if (copy && next < entries.length) inFlightModes.add((copy.mode & 0o7777).toString(8)); + } + await new Promise(resolve => setImmediate(resolve)); } - await new Promise(resolve => setImmediate(resolve)); + const r = await moving; + expect(r.stderr.toString()).toBe(""); + expect(r.exitCode).toBe(0); + } finally { + rmSync(src, { recursive: true, force: true }); + rmSync(dst, { recursive: true, force: true }); } - const r = await moving; - expect(r.stderr.toString()).toBe(""); - expect(r.exitCode).toBe(0); - expect([...otherModes]).toEqual([]); - } finally { - rmSync(src, { recursive: true, force: true }); - rmSync(dst, { recursive: true, force: true }); + if (inFlightModes.size > 0) break; } + expect([...inFlightModes]).toEqual(["700"]); }); const nobody = 65534; From ea8afc84c65f1423b98c6fb97ccbae3f2a0c47a6 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 19 Sep 2026 06:49:32 +0000 Subject: [PATCH 8/8] shell mv: set the permission bits of a cross-device copy before it changes owner A mover with CAP_CHOWN but without CAP_FOWNER cannot fchmod a copy after it gives the copy away. With the owner-only creation mode, that left files at 600 and directories at 700. Apply the mode without the set-id bits first, then fchown, then add the set-id bits only if fchown succeeded. --- src/runtime/shell/builtin/mv.rs | 12 +++--- test/js/bun/shell/commands/mv.test.ts | 53 ++++++++++++++++++++++++--- 2 files changed, 55 insertions(+), 10 deletions(-) diff --git a/src/runtime/shell/builtin/mv.rs b/src/runtime/shell/builtin/mv.rs index c278ed18a330..73e8aa64d7ef 100644 --- a/src/runtime/shell/builtin/mv.rs +++ b/src/runtime/shell/builtin/mv.rs @@ -636,12 +636,14 @@ impl ShellMvBatchedTask { /// POSIX `mv`: the copy gets set-uid and set-gid only if it also gets the owner. #[cfg(unix)] fn copy_owner_and_mode(fd: bun_sys::Fd, st: &bun_sys::Stat) { - let mut mode = st.st_mode as bun_core::Mode & 0o7777; - // `fchown` first: Linux clears S_ISUID/S_ISGID on chown. - if bun_sys::fchown(fd, st.st_uid as _, st.st_gid as _).is_err() { - mode &= !(bun_sys::S::ISUID | bun_sys::S::ISGID); + let mode = st.st_mode as bun_core::Mode & 0o7777; + let set_id = mode & (bun_sys::S::ISUID | bun_sys::S::ISGID); + // Before `fchown`: a mover with CAP_CHOWN but no CAP_FOWNER cannot `fchmod` a copy it gave away. + let _ = bun_sys::fchmod(fd, mode & !set_id); + // After `fchown`: Linux clears S_ISUID/S_ISGID on chown. + if bun_sys::fchown(fd, st.st_uid as _, st.st_gid as _).is_ok() && set_id != 0 { + let _ = bun_sys::fchmod(fd, mode); } - let _ = bun_sys::fchmod(fd, mode); } /// `renameat(cwd, src, target_fd, basename(src))`. A free fn over the diff --git a/test/js/bun/shell/commands/mv.test.ts b/test/js/bun/shell/commands/mv.test.ts index 804f8b06216c..a49d89de2df1 100644 --- a/test/js/bun/shell/commands/mv.test.ts +++ b/test/js/bun/shell/commands/mv.test.ts @@ -4,6 +4,7 @@ import { bunEnv, bunExe, isLinux, isPosix } from "harness"; import { accessSync, chmodSync, + chownSync, constants, existsSync, lstatSync, @@ -401,18 +402,60 @@ describe("mv", async () => { } }); - // A checkout under /root is out of reach for `nobody`. - function nobodyCanRunBun() { + // A checkout under /root is out of reach for another user, and `setpriv` can be missing. + function canRunBun(prefix: string[], ids: { uid?: number; gid?: number }) { try { - const asNobody = { env: bunEnv, cwd: "/", uid: nobody, gid: nobody }; - return Bun.spawnSync({ cmd: [bunExe(), "--revision"], ...asNobody }).success; + return Bun.spawnSync({ cmd: [...prefix, bunExe(), "--revision"], env: bunEnv, cwd: "/", ...ids }).success; } catch { return false; } } + // This mover has CAP_CHOWN but not CAP_FOWNER: it can give a copy away, but it cannot chmod the copy after that. + const chownOnly = + "setpriv --reuid=65533 --regid=65533 --clear-groups --inh-caps=+chown --ambient-caps=+chown".split(" "); + + test.skipIf(!isRoot || !publicRootsDiffer || !canRunBun(chownOnly, {}))( + "modes are kept across devices when the mover cannot chmod a copy that it gave away", + async () => { + const [src, dst] = crossDevicePair("chown-only", publicRoots); + try { + writeFileSync(join(src, "file"), "file"); + chmodSync(join(src, "file"), 0o644); + mkdirSync(join(src, "dir")); + chmodSync(join(src, "dir"), 0o755); + for (const name of ["file", "dir"]) chownSync(join(src, name), nobody, nobody); + chmodSync(src, 0o777); + chmodSync(dst, 0o777); + + const script = ` + import { $ } from "bun"; + const [src, dst] = ${JSON.stringify([src, dst])}; + const r = await $\`mv \${src}/file \${src}/dir \${dst}\`.nothrow(); + process.exit(r.exitCode); + `; + await using proc = Bun.spawn({ + cmd: [...chownOnly, bunExe(), "-e", script], + env: bunEnv, + cwd: "/", + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stdout, stderr, exitCode }).toEqual({ stdout: "", stderr: "", exitCode: 0 }); + expect(ownerAndMode(dst, ["file", "dir"])).toEqual({ + file: `644 uid=${nobody}`, + dir: `755 uid=${nobody}`, + }); + } finally { + rmSync(src, { recursive: true, force: true }); + rmSync(dst, { recursive: true, force: true }); + } + }, + ); + // Root only: no other user can create a file that someone else owns. - test.skipIf(!isRoot || !publicRootsDiffer || !nobodyCanRunBun())( + test.skipIf(!isRoot || !publicRootsDiffer || !canRunBun([], { uid: nobody, gid: nobody }))( "set-uid and set-gid are dropped across devices when the owner cannot be kept", async () => { const [src, dst] = crossDevicePair("setid", publicRoots);