diff --git a/src/sandbox/linux-sandbox-utils.ts b/src/sandbox/linux-sandbox-utils.ts index e31685cfe..7523aa040 100644 --- a/src/sandbox/linux-sandbox-utils.ts +++ b/src/sandbox/linux-sandbox-utils.ts @@ -17,6 +17,8 @@ import { isSymlinkOutsideBoundary, encodeSandboxedCommand, DANGEROUS_FILES, + isAtOrUnder, + isStrictlyUnder, getDangerousDirectories, } from './sandbox-utils.js' import type { @@ -927,7 +929,9 @@ async function generateFilesystemArgs( // path whose deepest existing ancestor lies within one of these is already // uncreatable, and must not get a /dev/null stub: bwrap would have to // creat() the mount point inside that read-only mount and abort ("Can't - // create file at : Read-only file system"). The spellings matter + // create file at : Read-only file system"). An EXISTING deny path + // strictly beneath one is likewise already unwritable and its own + // --ro-bind

is skipped as redundant. The spellings matter // because the emission filter and the denyRead re-application compare raw // spellings as well as the resolved dest, so the stub-skip guard tests a // covering directory in its canonical form AND every recorded spelling. @@ -1005,13 +1009,14 @@ async function generateFilesystemArgs( allowedWritePaths.push(normalizedPath) } - // Inputs for the stub-skip guard's vetoes, computed at most once and only - // when an absent deny path actually has a covering read-only deny dir (an - // uncommon configuration) — ordinary commands skip the extra - // stat/realpath/readdir syscalls entirely. Lazy evaluation also means the - // derivation runs from inside the deny loop, AFTER the (unbounded) - // mandatory-deny ripgrep await below, keeping the snapshot as close as - // possible to the denyRead loop that later acts on the real filesystem. + // Inputs for the covering-directory vetoes, computed at most once and + // only when a deny path (absent or existing) lies strictly beneath a + // recorded read-only deny dir — commands with no such covering directory + // skip the extra stat/realpath/readdir syscalls entirely. Lazy + // evaluation also means the derivation runs from inside the deny loop, + // AFTER the (unbounded) mandatory-deny ripgrep await below, keeping the + // snapshot as close as possible to the denyRead loop that later acts on + // the real filesystem. // // allowedWritePathsBothForms: allowWrite paths in their recorded and // realpath-canonical spellings. The canonical form is re-resolved HERE, @@ -1194,11 +1199,16 @@ async function generateFilesystemArgs( // Per-covering-dir veto verdict, computed once per recorded directory // (the inputs never change during the deny loop) instead of per absent // deny entry. - // INVARIANT: a stub is skipped only under a recorded covering deny - // directory that has no allowed write path strictly beneath it and is - // INCOMPARABLE with every read-deny tmpfs directory (neither - // at-or-beneath it nor containing it or any spelling it was reached - // through). Rationale: the only writable emissions that land after the + // INVARIANT: a stub, or an existing deny path's own bind, is skipped + // only under a recorded covering deny directory that has no allowed + // write path strictly beneath it and is INCOMPARABLE with every + // read-deny tmpfs directory (neither at-or-beneath it nor containing it + // or any spelling it was reached through). Containment is root-aware + // (isAtOrUnder): '/' is a recordable covering directory when allowOnly + // and denyWithinAllow both name it, and '/' + '/' is a prefix of + // nothing, so a string-prefix test would judge it safe for every path + // and drop the binds the re-application passes below key off. + // Rationale: the only writable emissions that land after the // buffered read-only binds are the denyRead re-applications // (pushReadDenyDirMounts), which mount a tmpfs and re-bind allowed write // paths beneath it WITHOUT re-emitting the binds it buries — so a @@ -1229,13 +1239,12 @@ async function generateFilesystemArgs( // (i) an allowed write path strictly beneath the dir: the // re-application's effect would re-bind it writable. allowedWritePathsBothForms.some(writePath => - writePath.startsWith(denyDir + '/'), + isStrictlyUnder(writePath, denyDir), ) || // (ii) a read-deny tmpfs at or beneath the dir: the re-application's // trigger. - prospectiveReadDenyTmpfsDirsBothForms.some( - tmpfsDir => - tmpfsDir === denyDir || tmpfsDir.startsWith(denyDir + '/'), + prospectiveReadDenyTmpfsDirsBothForms.some(tmpfsDir => + isAtOrUnder(tmpfsDir, denyDir), ) || // (iii) a read-deny tmpfs CONTAINING the dir or any raw spelling it // was reached through: the dir's own --ro-bind can be dropped as @@ -1244,8 +1253,7 @@ async function generateFilesystemArgs( // is not reliably read-only in the sandbox. prospectiveReadDenyTmpfsDirsBothForms.some(tmpfsDir => [denyDir, ...(readOnlyDenyDirSpellings.get(denyDir) ?? [])].some( - spelling => - spelling === tmpfsDir || spelling.startsWith(tmpfsDir + '/'), + spelling => isAtOrUnder(spelling, tmpfsDir), ), ) coveringDirUnsafeVerdicts.set(denyDir, unsafe) @@ -1254,6 +1262,30 @@ async function generateFilesystemArgs( // Materialized once: the pre-pass above fully populates the map and the // deny loop never mutates it. const readOnlyDenyDirs = [...readOnlyDenyDirSpellings.keys()] + // Is `candidate` already unwritable in the sandbox: strictly under a + // recorded read-only deny directory that survives every + // coveringDirIsUnsafe veto? Strictly, because a deny equal to a recorded + // directory IS that covering bind and must be emitted (an absent path + // never equals one). Stops at the first vetoed covering directory. + const coveredBySafeReadOnlyDenyDir = (candidate: string): boolean => { + let covered = false + for (const denyDir of readOnlyDenyDirs) { + if (!isStrictlyUnder(candidate, denyDir)) continue + if (coveringDirIsUnsafe(denyDir)) { + // A vetoed '/' neither covers a path nor disqualifies an inner + // recorded directory: everything lies beneath it, so it would + // veto every skip and stub each absent mandatory-deny path of a + // write-denied cwd after that cwd's own bind — the startup abort. + // Its descendants are decided by their own recorded directories, + // as before, when the string-prefix filter matched '/' only for a + // path directly beneath it and the vetoes never fired for it. + if (denyDir === '/') continue + return false + } + covered = true + } + return covered + } for (const pathPattern of denyPaths) { const rawPath = normalizePathForSandbox(pathPattern) @@ -1371,13 +1403,11 @@ async function generateFilesystemArgs( // regardless of where it appears in denyPaths. A recorded covering // directory is evidence for skipping only if it survives the // coveringDirIsUnsafe vetoes (see the INVARIANT at its definition). - const coveringReadOnlyDenyDirs = readOnlyDenyDirs.filter( - denyDir => - ancestorPath === denyDir || ancestorPath.startsWith(denyDir + '/'), - ) + // (Tested on the absent path itself: a recorded directory that + // covers it is at-or-above its deepest existing ancestor, since + // recorded directories exist.) const ancestorIsWithinReadOnlyDeny = - coveringReadOnlyDenyDirs.length > 0 && - !coveringReadOnlyDenyDirs.some(coveringDirIsUnsafe) + coveredBySafeReadOnlyDenyDir(normalizedPath) if (ancestorIsWithinAllowedPath && !ancestorIsWithinReadOnlyDeny) { const firstNonExistent = findFirstNonExistentComponent(normalizedPath) @@ -1423,6 +1453,20 @@ async function generateFilesystemArgs( const isWithinAllowedPath = isWithinAnyAllowedWritePath(normalizedPath) if (isWithinAllowedPath) { + // Already unwritable under a read-only denied directory (the + // existing-path twin of the stub skip above). Veto (iii) keeps the + // covering bind through the emission filter; a symlinked spelling + // keeps its own bind because the re-application passes below key + // off emitted raw spellings. + if ( + rawPath === normalizedPath && + coveredBySafeReadOnlyDenyDir(normalizedPath) + ) { + logForDebugging( + `[Sandbox Linux] Skipping deny path already under read-only denied directory: ${normalizedPath}`, + ) + continue + } denyWriteArgs.push('--ro-bind', normalizedPath, normalizedPath) denyWriteRawDests.set(normalizedPath, rawPath) } else { @@ -1594,9 +1638,11 @@ async function generateFilesystemArgs( // The inverse stacking problem: a denyWrite ro-bind whose dest strictly // contains a read-denied dir re-exposes that dir's real contents (the bind // landed after the tmpfs). Re-apply the tmpfs on top, with the same write - // and allowRead re-binds the denyRead loop emitted. + // and allowRead re-binds the denyRead loop emitted. A bind of '/' itself + // (allowOnly and denyWithinAllow both naming it) contains every one of + // them, so containment is root-aware. for (const tmpfsDir of tmpfsDirs) { - if (emittedDenyWriteDests.some(dest => tmpfsDir.startsWith(dest + '/'))) { + if (emittedDenyWriteDests.some(dest => isStrictlyUnder(tmpfsDir, dest))) { logForDebugging( `[Sandbox Linux] Re-applying denyRead tmpfs re-exposed by denyWrite bind: ${tmpfsDir}`, ) @@ -1607,7 +1653,7 @@ async function generateFilesystemArgs( // ancestor bind, so the real file is back. Re-apply the mask with its // original source (/dev/null for read-deny, the fake for credential mask). for (const [maskedFile, source] of maskedFiles) { - if (emittedDenyWriteDests.some(dest => maskedFile.startsWith(dest + '/'))) { + if (emittedDenyWriteDests.some(dest => isStrictlyUnder(maskedFile, dest))) { // maskedFiles holds both the symlink path and its resolved target so // the denyWrite skip-check above matches either. Re-emission must go // to the target only — bwrap rejects a symlink bind dest (see diff --git a/src/sandbox/sandbox-utils.ts b/src/sandbox/sandbox-utils.ts index 933585175..f5d9bd8f1 100644 --- a/src/sandbox/sandbox-utils.ts +++ b/src/sandbox/sandbox-utils.ts @@ -52,6 +52,19 @@ export function normalizeCaseForComparison(pathStr: string): string { return pathStr.toLowerCase() } +/** + * `p` is `dir` itself or lies beneath it, by path segment ('/x' is not under + * '/xy'); root-aware, since '/' + '/' is a prefix of nothing. + */ +export function isAtOrUnder(p: string, dir: string): boolean { + return p === dir || p.startsWith(dir === '/' ? '/' : dir + '/') +} + +/** `p` lies strictly beneath `dir` (isAtOrUnder, excluding `dir` itself). */ +export function isStrictlyUnder(p: string, dir: string): boolean { + return p !== dir && isAtOrUnder(p, dir) +} + /** * Check if a path pattern contains glob characters */ diff --git a/test/sandbox/readonly-deny-dir-binds.test.ts b/test/sandbox/readonly-deny-dir-binds.test.ts new file mode 100644 index 000000000..bf1b4ee1d --- /dev/null +++ b/test/sandbox/readonly-deny-dir-binds.test.ts @@ -0,0 +1,270 @@ +import { describe, it, expect, beforeEach, afterEach } from 'bun:test' +import { + mkdirSync, + mkdtempSync, + readFileSync, + realpathSync, + rmSync, + symlinkSync, + writeFileSync, +} from 'node:fs' +import { spawnSync } from 'node:child_process' +import { join } from 'node:path' +import { tmpdir } from 'node:os' +import { + wrapCommandWithSandboxLinux, + cleanupBwrapMountPoints, +} from '../../src/sandbox/linux-sandbox-utils.js' +import { isLinux } from '../helpers/platform.js' + +/** + * A deny path strictly beneath a directory that denyWithinAllow re-binds + * read-only gets no --ro-bind of its own, under the same evidence and vetoes + * as the absent-path stub skip (readonly-deny-dir-stubs.test.ts); a deny + * equal to an allowOnly root, or with no covering directory, is still bound. + */ +describe.if(isLinux)('Deny binds under a read-only denied directory', () => { + let BASE: string + let AREA: string // allowed write area + let PROJ: string // project dir inside AREA + let FILE: string // existing file under PROJ/sub + + const savedCwd = process.cwd() + + // Runtime arm, as in readonly-deny-dir-stubs.test.ts: only where bwrap can + // run the namespace/proc surface the wrapped commands use. + const BWRAP_CAN_NAMESPACE = + spawnSync( + 'bwrap', + [ + '--unshare-pid', + '--unshare-user', + '--cap-drop', + 'ALL', + '--ro-bind', + '/', + '/', + '--proc', + '/proc', + 'true', + ], + { timeout: 5000 }, + ).status === 0 + + beforeEach(() => { + BASE = realpathSync(mkdtempSync(join(tmpdir(), 'ro-deny-bind-'))) + AREA = join(BASE, 'area') + PROJ = join(AREA, 'proj') + FILE = join(PROJ, 'sub', 'settings.json') + mkdirSync(join(PROJ, 'sub'), { recursive: true }) + writeFileSync(FILE, '{}\n') + // Keep cwd outside the allowlist so the mandatory-deny scan adds no + // binds of its own to reason about. + process.chdir(BASE) + }) + + afterEach(() => { + process.chdir(savedCwd) + cleanupBwrapMountPoints({ force: true }) + rmSync(BASE, { recursive: true, force: true }) + }) + + // Same parameter order as readonly-deny-dir-stubs.test.ts, whose fixture + // and covering-directory predicate this suite shares. + async function wrap( + denyPaths: string[], + readDenyPaths: string[] = [], + allowPaths: string[] = [AREA], + command = 'echo hello', + ): Promise { + return wrapCommandWithSandboxLinux({ + command, + needsNetworkRestriction: false, + readConfig: { denyOnly: readDenyPaths }, + writeConfig: { allowOnly: allowPaths, denyWithinAllow: denyPaths }, + }) + } + + const countOccurrences = (haystack: string, needle: string): number => + haystack.split(needle).length - 1 + + it('binds the denied allow-root once and skips the existing file beneath it', async () => { + // allowOnly=[proj], denyWithinAllow=[proj, proj/sub/file]: the directory + // deny equals the allow root and must still be emitted; the file is a + // strict descendant of that read-only bind and needs nothing. + const command = await wrap([PROJ, FILE], [], [PROJ]) + + expect(countOccurrences(command, `--ro-bind ${PROJ} ${PROJ}`)).toBe(1) + expect(command).not.toContain(`--ro-bind ${FILE} ${FILE}`) + + // Where the host can run bwrap, prove the covering bind alone still + // holds: the file reads, and a write through it fails and changes + // nothing on the host. + if (BWRAP_CAN_NAMESPACE) { + const run = (wrapped: string) => + spawnSync(wrapped, { + shell: true, + encoding: 'utf8', + timeout: 15000, + cwd: BASE, + }) + const read = run(await wrap([PROJ, FILE], [], [PROJ], `cat ${FILE}`)) + expect(read.status).toBe(0) + expect(read.stdout).toContain('{}') + + const write = run( + await wrap([PROJ, FILE], [], [PROJ], `sh -c 'echo x >> ${FILE}'`), + ) + expect(write.status).not.toBe(0) + expect(readFileSync(FILE, 'utf8')).toBe('{}\n') + } + }) + + it('is independent of the order the denies are listed in', async () => { + const command = await wrap([FILE, PROJ], [], [PROJ]) + + expect(countOccurrences(command, `--ro-bind ${PROJ} ${PROJ}`)).toBe(1) + expect(command).not.toContain(`--ro-bind ${FILE} ${FILE}`) + }) + + it('still binds the file when its directory is not itself denied', async () => { + const command = await wrap([FILE], [], [PROJ]) + + expect(command).toContain(`--bind ${PROJ} ${PROJ}`) + expect(command).toContain(`--ro-bind ${FILE} ${FILE}`) + }) + + it('collapses a chain of nested directory denies to the outermost bind', async () => { + const sub = join(PROJ, 'sub') + const command = await wrap([PROJ, sub, FILE]) + + expect(countOccurrences(command, `--ro-bind ${PROJ} ${PROJ}`)).toBe(1) + expect(command).not.toContain(`--ro-bind ${sub} ${sub}`) + expect(command).not.toContain(`--ro-bind ${FILE} ${FILE}`) + }) + + it('keeps the descendant bind when an allowed write path sits strictly beneath the covering dir (veto)', async () => { + // Same veto as the stub skip: with an allowWrite under PROJ the denyRead + // re-application machinery could re-open part of the subtree, so the + // covering bind is not trusted and the explicit deny keeps its own. + const nestedAllow = join(PROJ, 'w') + mkdirSync(nestedAllow) + + const command = await wrap([PROJ, FILE], [], [AREA, nestedAllow]) + + expect(command).toContain(`--ro-bind ${PROJ} ${PROJ}`) + expect(command).toContain(`--ro-bind ${FILE} ${FILE}`) + }) + + it('keeps the descendant bind when a denyRead tmpfs sits under the covering dir (veto)', async () => { + const readDenied = join(PROJ, 'secrets') + mkdirSync(readDenied) + + const command = await wrap([PROJ, FILE], [readDenied]) + + expect(command).toContain(`--ro-bind ${PROJ} ${PROJ}`) + expect(command).toContain(`--ro-bind ${FILE} ${FILE}`) + }) + + it('does not trust a recorded "/" as a covering directory', async () => { + // allowOnly and denyWithinAllow both naming '/' records it as a + // read-only deny directory that every path lies beneath. A string-prefix + // veto ('/' + '/') could never fire, so PROJ's own bind would be dropped + // as covered; the recursive --ro-bind / / emitted later then shadows the + // FILE mask with no bind left to key its re-application off, and the + // read-denied file is readable. Root-aware containment vetoes '/' (AREA + // is an allowed write path beneath it), keeps PROJ's bind, and re-applies + // the mask after the root bind. + const command = await wrap(['/', PROJ], [FILE], ['/', AREA]) + + expect(command).toContain(`--ro-bind ${PROJ} ${PROJ}`) + const rootBind = command.lastIndexOf('--ro-bind / /') + const mask = command.lastIndexOf(`--ro-bind /dev/null ${FILE}`) + expect(rootBind).toBeGreaterThan(-1) + expect(mask).toBeGreaterThan(rootBind) + }) + + it('skips the stubs under a write-denied cwd even when a recorded "/" is vetoed', async () => { + // '/' recorded and vetoed (AREA is writable beneath it). A veto that + // disqualified every skip would stub each absent mandatory-deny dotfile + // of the write-denied cwd after the cwd's own bind — the startup abort + // readonly-deny-dir-stubs.test.ts documents. The cwd's recorded bind + // decides instead, as on main. + process.chdir(PROJ) + const command = await wrap(['/', PROJ], [], ['/', AREA]) + + expect(command).toContain(`--ro-bind ${PROJ} ${PROJ}`) + expect(command).not.toContain(`/dev/null ${PROJ}/`) + expect(command).not.toMatch(/--ro-bind \S*claude-empty-\S+ \S*\/proj\//) + }) + + it('re-applies a read-deny mask and tmpfs shadowed by a bind of "/" alone', async () => { + // '/' is the only emitted deny bind. main compared the bind by string + // prefix ('/' + '/') and re-applied nothing after --ro-bind / /, so the + // recursive root bind left the file and the directory readable. + const secrets = join(PROJ, 'secrets') + mkdirSync(secrets) + const command = await wrap(['/'], [FILE, secrets], ['/']) + + const rootBind = command.lastIndexOf('--ro-bind / /') + expect(rootBind).toBeGreaterThan(-1) + expect(command.lastIndexOf(`--ro-bind /dev/null ${FILE}`)).toBeGreaterThan( + rootBind, + ) + expect(command.lastIndexOf(`--tmpfs ${secrets}`)).toBeGreaterThan(rootBind) + }) + + it('does not treat a string-prefix sibling as covered', async () => { + // AREA/proj2/x.txt shares a prefix with AREA/proj without lying beneath + // it: a plain startsWith would drop its bind and leave it writable. + const proj2 = join(AREA, 'proj2') + mkdirSync(proj2) + const sibling = join(proj2, 'x.txt') + writeFileSync(sibling, '') + + const command = await wrap([PROJ, sibling]) + + expect(command).toContain(`--ro-bind ${PROJ} ${PROJ}`) + expect(command).toContain(`--ro-bind ${sibling} ${sibling}`) + }) + + it('vetoes "/" for an allowed write path beneath it even with no read policy', async () => { + // Veto (i) alone must be root-aware: with readConfig undefined there is + // no read-deny tmpfs for veto (ii) to catch '/' with. + const command = await wrapCommandWithSandboxLinux({ + command: 'echo hello', + needsNetworkRestriction: false, + readConfig: undefined, + writeConfig: { allowOnly: ['/', AREA], denyWithinAllow: ['/', PROJ] }, + }) + + expect(command).toContain(`--ro-bind ${PROJ} ${PROJ}`) + }) + + it('does not re-apply a tmpfs over the bind that denies the same directory', async () => { + // X in allowOnly, denyWithinAllow and denyRead: the read-only bind of X + // is not "an ancestor that re-exposes X", so no --tmpfs X --bind X X may + // follow it and make X writable again. + const X = join(AREA, 'both') + mkdirSync(X) + const command = await wrap([X], [X], [AREA, X]) + + const lastRoBind = command.lastIndexOf(`--ro-bind ${X} ${X}`) + expect(lastRoBind).toBeGreaterThan(-1) + expect(command.indexOf(`--bind ${X} ${X}`, lastRoBind)).toBe(-1) + }) + + it('keeps the bind for a deny reached through a symlinked spelling', async () => { + // The re-application passes key off emitted raw spellings; a dest that + // was reached via a symlink keeps its bind so that breadcrumb survives. + const realSub = join(PROJ, 'sub') + const linkSub = join(PROJ, 'link') + symlinkSync(realSub, linkSub) + const viaLink = join(linkSub, 'settings.json') + + const command = await wrap([PROJ, viaLink]) + + expect(command).toContain(`--ro-bind ${PROJ} ${PROJ}`) + expect(command).toContain(`--ro-bind ${FILE} ${FILE}`) + }) +}) diff --git a/test/sandbox/symlink-boundary.test.ts b/test/sandbox/symlink-boundary.test.ts index 692926166..ed5b9ca3b 100644 --- a/test/sandbox/symlink-boundary.test.ts +++ b/test/sandbox/symlink-boundary.test.ts @@ -4,6 +4,7 @@ import { existsSync, mkdirSync, rmSync, unlinkSync, lstatSync } from 'node:fs' import { join } from 'node:path' import { wrapCommandWithSandboxMacOS } from '../../src/sandbox/macos-sandbox-utils.js' import { + isAtOrUnder, isSymlinkOutsideBoundary, normalizePathForSandbox, } from '../../src/sandbox/sandbox-utils.js' @@ -390,6 +391,17 @@ describe('isSymlinkOutsideBoundary Unit Tests', () => { }) }) +describe('isAtOrUnder', () => { + it('contains by path segment, root included', () => { + expect(isAtOrUnder('/a/b', '/a')).toBe(true) + expect(isAtOrUnder('/a', '/a')).toBe(true) + expect(isAtOrUnder('/ab', '/a')).toBe(false) + expect(isAtOrUnder('/x', '/')).toBe(true) + expect(isAtOrUnder('/', '/')).toBe(true) + expect(isAtOrUnder('/', '/a')).toBe(false) + }) +}) + /** * Tests for glob pattern symlink boundary validation */