diff --git a/src/runtime/shell/builtin/mv.rs b/src/runtime/shell/builtin/mv.rs index 0f047bc29eb8..73e8aa64d7ef 100644 --- a/src/runtime/shell/builtin/mv.rs +++ b/src/runtime/shell/builtin/mv.rs @@ -507,6 +507,9 @@ 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. + 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 +548,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, @@ -561,23 +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)] - let _ = bun_sys::fchown(dd.fd(), st.st_uid as _, st.st_gid as _); - let _ = bun_sys::fchmod(dd.fd(), mode & 0o7777); + 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); } @@ -604,7 +596,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()) { @@ -613,15 +605,47 @@ 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) } + 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) { + 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); + } + } + /// `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..a49d89de2df1 100644 --- a/test/js/bun/shell/commands/mv.test.ts +++ b/test/js/bun/shell/commands/mv.test.ts @@ -1,19 +1,22 @@ import { $ } from "bun"; import { describe, expect, test } from "bun:test"; -import { isPosix } from "harness"; +import { bunEnv, bunExe, isLinux, isPosix } from "harness"; import { accessSync, chmodSync, + chownSync, constants, existsSync, lstatSync, mkdirSync, + readdirSync, readFileSync, readlinkSync, rmSync, statSync, symlinkSync, writeFileSync, + type Stats, } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "path"; @@ -87,10 +90,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 +236,283 @@ describe("mv", async () => { rmSync(dst, { recursive: true, force: true }); } }); + + 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 }); + } + }); + + // 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 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 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)); + } + 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 }); + } + if (inFlightModes.size > 0) break; + } + expect([...inFlightModes]).toEqual(["700"]); + }); + + 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 { + 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}`]; + }), + ); + } + + function canAccess(path: string, mode: number) { + try { + accessSync(path, mode); + return true; + } catch { + return false; + } + } + + // 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"]) { + // `lstat`: `mv` copies a symlink as a symlink, and Alpine links these names to busybox. + let st: Stats; + try { + st = lstatSync(path); + } catch { + continue; + } + 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; + 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(); + // `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!()}`, + }); + 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 }); + } + }); + + // 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 { + 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 || !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); + 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 }); + } + }, + ); }); });