diff --git a/src/sandbox/linux-sandbox-utils.ts b/src/sandbox/linux-sandbox-utils.ts index adab73a7..408fd0e4 100644 --- a/src/sandbox/linux-sandbox-utils.ts +++ b/src/sandbox/linux-sandbox-utils.ts @@ -124,9 +124,8 @@ function findSymlinkInPath( const stats = fs.lstatSync(nextPath) if (stats.isSymbolicLink()) { // Check if this symlink is within an allowed write path - const isWithinAllowedPath = allowedWritePaths.some( - allowedPath => - nextPath.startsWith(allowedPath + '/') || nextPath === allowedPath, + const isWithinAllowedPath = allowedWritePaths.some(allowedPath => + isAtOrUnder(nextPath, allowedPath), ) if (isWithinAllowedPath) { return nextPath @@ -1156,18 +1155,22 @@ async function generateFilesystemArgs( return stubSkipVetoInputs } // The ONE predicate deciding whether a deny dest lies inside the write - // allowlist. The read-only pre-pass below and the loop's --ro-bind gate - // MUST share it: the pre-pass is only sound if it records exactly the - // directories the loop re-binds read-only (a recorded directory that is - // never re-bound read-only would suppress stubs unsafely; a re-bound - // directory missing from the record only costs an abort). No spelling - // handling is needed here: allowedWritePaths entries are recorded with - // trailing slashes stripped, and candidates are resolved deny dests. + // allowlist, and so whether the deny is applied at all: a dest outside it + // is left read-only by the initial --ro-bind / /. The read-only pre-pass + // below and the loop's --ro-bind gate MUST share it, so that the pre-pass + // records the directories the loop re-binds read-only. + // + // Containment is root-aware (isAtOrUnder) because '/' is a legal + // allowOnly entry: an `allowedPath + '/'` prefix test spells it '//' and + // matches nothing, so under a '/' write root every deny that no other + // allow entry covers would be judged outside the allowlist and silently + // lose its bind — over a root the allow loop has already bound writable. + // Spelling needs no further handling here: allowedWritePaths entries are + // recorded with trailing slashes stripped, and candidates are resolved + // deny dests. const isWithinAnyAllowedWritePath = (candidatePath: string): boolean => - allowedWritePaths.some( - allowedPath => - candidatePath.startsWith(allowedPath + '/') || - candidatePath === allowedPath, + allowedWritePaths.some(allowedPath => + isAtOrUnder(candidatePath, allowedPath), ) // Deny writes within allowed paths (user-specified + mandatory denies) @@ -1196,9 +1199,13 @@ async function generateFilesystemArgs( // allowed write path strictly beneath it; incomparable with every // read-deny tmpfs in any spelling), which exclude every way its subtree // could be writable in the sandbox. Keep the two passes in lockstep: a - // directory recorded here but never re-bound read-only AND not vetoed - // would suppress stubs unsafely, while an emitted one missing from the - // record only costs a spurious abort. + // directory recorded here is either re-bound read-only by the loop or + // skipped because a recorded directory above it survived the vetoes and + // is bound in its place, so every record still stands for a bind that + // lands — unless a symlink appears in its path between the two passes, + // where the loop masks that component and emits no bind for the + // directory (the re-check below); an emitted one missing from the record + // only costs a spurious abort. for (const pathPattern of denyPaths) { const rawPath = normalizePathForSandbox(pathPattern) if (rawPath.startsWith('/dev/')) { @@ -1317,9 +1324,7 @@ async function generateFilesystemArgs( // 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. + // Its descendants are decided by their own recorded directories. if (denyDir === '/') continue return false } diff --git a/src/sandbox/macos-sandbox-utils.ts b/src/sandbox/macos-sandbox-utils.ts index 2cc6e5f9..bc9d918e 100644 --- a/src/sandbox/macos-sandbox-utils.ts +++ b/src/sandbox/macos-sandbox-utils.ts @@ -12,6 +12,7 @@ import { decodeSandboxedCommand, containsGlobChars, globToRegex, + isStrictlyUnder as isPathStrictlyUnder, DANGEROUS_FILES, getDangerousDirectories, } from './sandbox-utils.js' @@ -201,8 +202,10 @@ function denyGlobCovers(denyRegex: RegExp, entry: PathEntry): boolean { /** Is `entry`'s region strictly inside the literal directory `dir`? */ function isStrictlyUnder(entry: PathEntry, dir: string): boolean { - const probe = entry.glob ? globSamplePath(entry.path) : entry.path - return probe.startsWith(dir === '/' ? '/' : dir + '/') && probe !== dir + return isPathStrictlyUnder( + entry.glob ? globSamplePath(entry.path) : entry.path, + dir, + ) } /** @@ -270,7 +273,7 @@ function lateReadDenyFilters(resolved: ResolvedReadConfig): { const literalAllowDirs = resolved.allows.filter(a => !a.glob).map(a => a.path) for (const deny of resolved.denies) { if (!deny.glob) { - if (literalAllowDirs.some(a => deny.path.startsWith(a + '/'))) { + if (literalAllowDirs.some(a => isStrictlyUnder(deny, a))) { filters.push(denyPathFilter(deny.path)) } continue diff --git a/src/sandbox/sandbox-utils.ts b/src/sandbox/sandbox-utils.ts index f5d9bd8f..2c54c53b 100644 --- a/src/sandbox/sandbox-utils.ts +++ b/src/sandbox/sandbox-utils.ts @@ -53,8 +53,8 @@ export function normalizeCaseForComparison(pathStr: string): string { } /** - * `p` is `dir` itself or lies beneath it, by path segment ('/x' is not under - * '/xy'); root-aware, since '/' + '/' is a prefix of nothing. + * `p` is `dir` itself or lies beneath it, by path segment ('/xy' is not under + * '/x'); root-aware, since '/' + '/' is a prefix of nothing. */ export function isAtOrUnder(p: string, dir: string): boolean { return p === dir || p.startsWith(dir === '/' ? '/' : dir + '/') diff --git a/test/sandbox/macos-glob-deny-reemit.test.ts b/test/sandbox/macos-glob-deny-reemit.test.ts index 1dc31973..89e778ee 100644 --- a/test/sandbox/macos-glob-deny-reemit.test.ts +++ b/test/sandbox/macos-glob-deny-reemit.test.ts @@ -189,6 +189,19 @@ describe.if(!isWindows)('macOS read profile: glob denies vs allowRead', () => { expect(lateBlock(read)).toContain(filter) }) + it('still re-emits a literal deny nested under a literal allow of "/"', () => { + // '/' is a legal allowWithinDeny entry and re-opens every denied path, + // so the nested deny only survives if it lands after the allow block. + // Containment against it has to be root-aware: a `dir + '/'` prefix + // spells '//' and matches nothing. + const read = readSection( + wrap({ denyOnly: ['/work/proj/secrets'], allowWithinDeny: ['/'] }), + ) + const filter = '(subpath "/work/proj/secrets")' + expect(read.indexOf(filter)).toBeLessThan(allowBlockIndex(read)) + expect(lateBlock(read)).toContain(filter) + }) + it('does not re-emit a literal deny that no allow is nested in', () => { const read = readSection( wrap({ diff --git a/test/sandbox/readonly-deny-dir-binds.test.ts b/test/sandbox/readonly-deny-dir-binds.test.ts index bf1b4ee1..7d230a5c 100644 --- a/test/sandbox/readonly-deny-dir-binds.test.ts +++ b/test/sandbox/readonly-deny-dir-binds.test.ts @@ -85,9 +85,56 @@ describe.if(isLinux)('Deny binds under a read-only denied directory', () => { }) } + const run = (wrapped: string) => + spawnSync(wrapped, { + shell: true, + encoding: 'utf8', + timeout: 15000, + cwd: BASE, + }) + const countOccurrences = (haystack: string, needle: string): number => haystack.split(needle).length - 1 + /** + * Occurrences of one whole ` ` argv triple. Counting + * triples, not substrings, is what keeps a `/` assertion honest: the base + * `--ro-bind / /` root mount spells the deny-side bind of '/' exactly, so + * `lastIndexOf('--ro-bind / /')` finds the root mount and passes even when + * the deny-side bind was never emitted. + */ + const countBinds = ( + command: string, + flag: string, + source: string, + dest: string, + ): number => { + const argv = command.split(/\s+/) + let found = 0 + for (let i = 0; i + 2 < argv.length; i++) { + if (argv[i] === flag && argv[i + 1] === source && argv[i + 2] === dest) { + found++ + } + } + return found + } + + /** + * The write really hit a read-only mount, rather than the command failing + * for some other reason that also exits non-zero: bwrap refusing to start, + * or the spawn timing out. + */ + const expectDeniedByReadOnlyMount = (result: { + error?: Error + status: number | null + stderr: string + }): void => { + expect(result.error).toBeUndefined() + expect(result.status).not.toBe(0) + expect(result.stderr).not.toContain('bwrap:') + expect(result.stderr).toMatch(/Read-only file system|Permission denied/) + } + 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 @@ -101,13 +148,6 @@ describe.if(isLinux)('Deny binds under a read-only denied directory', () => { // 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('{}') @@ -115,7 +155,9 @@ describe.if(isLinux)('Deny binds under a read-only denied directory', () => { const write = run( await wrap([PROJ, FILE], [], [PROJ], `sh -c 'echo x >> ${FILE}'`), ) - expect(write.status).not.toBe(0) + // The specific failure, not merely a non-zero exit: a bwrap startup + // abort or a spawn timeout would satisfy that just as well. + expectDeniedByReadOnlyMount(write) expect(readFileSync(FILE, 'utf8')).toBe('{}\n') } }) @@ -178,9 +220,11 @@ describe.if(isLinux)('Deny binds under a read-only denied directory', () => { const command = await wrap(['/', PROJ], [FILE], ['/', AREA]) expect(command).toContain(`--ro-bind ${PROJ} ${PROJ}`) + // Two: the base root mount, then the deny bind of '/' whose position the + // mask has to beat. + expect(countBinds(command, '--ro-bind', '/', '/')).toBe(2) const rootBind = command.lastIndexOf('--ro-bind / /') const mask = command.lastIndexOf(`--ro-bind /dev/null ${FILE}`) - expect(rootBind).toBeGreaterThan(-1) expect(mask).toBeGreaterThan(rootBind) }) @@ -189,7 +233,7 @@ describe.if(isLinux)('Deny binds under a read-only denied directory', () => { // 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. + // decides instead. process.chdir(PROJ) const command = await wrap(['/', PROJ], [], ['/', AREA]) @@ -199,15 +243,16 @@ describe.if(isLinux)('Deny binds under a read-only denied directory', () => { }) 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. + // '/' is the only emitted deny bind, and it is recursive: it re-exposes + // every read-denied path underneath, so the mask and the tmpfs must be + // re-applied on top of it or the file and the directory stay readable. const secrets = join(PROJ, 'secrets') mkdirSync(secrets) const command = await wrap(['/'], [FILE, secrets], ['/']) + // Two: the base root mount, then the deny bind under test. + expect(countBinds(command, '--ro-bind', '/', '/')).toBe(2) const rootBind = command.lastIndexOf('--ro-bind / /') - expect(rootBind).toBeGreaterThan(-1) expect(command.lastIndexOf(`--ro-bind /dev/null ${FILE}`)).toBeGreaterThan( rootBind, ) @@ -267,4 +312,161 @@ describe.if(isLinux)('Deny binds under a read-only denied directory', () => { expect(command).toContain(`--ro-bind ${PROJ} ${PROJ}`) expect(command).toContain(`--ro-bind ${FILE} ${FILE}`) }) + + /** + * '/' is a legal allowOnly entry — normalization keeps it — and the allow + * loop binds it writable. Every deny then depends on the one predicate + * deciding whether its dest lies inside the write allowlist; a string + * prefix spells that '//' for a root allow and matches nothing, which + * drops every deny no other allow entry happens to cover. + * + * Mandatory denies are resolved against the cwd, and a '/' allowlist + * contains the cwd whatever it is, so unlike the suite above these cases + * reason about them too. + */ + describe('with "/" in the write allowlist', () => { + it('binds a denyWrite file and directory no other allow entry covers', async () => { + const denied = join(BASE, 'denied') + mkdirSync(denied) + + const command = await wrap([FILE, denied], [], ['/']) + + expect(countBinds(command, '--bind', '/', '/')).toBe(1) + expect(countBinds(command, '--ro-bind', FILE, FILE)).toBe(1) + expect(countBinds(command, '--ro-bind', denied, denied)).toBe(1) + }) + + it('binds the mandatory denies that exist and stubs the ones that do not', async () => { + const bashrc = join(BASE, '.bashrc') + const hooks = join(BASE, '.git', 'hooks') + const mcp = join(BASE, '.mcp.json') + writeFileSync(bashrc, '') + mkdirSync(hooks, { recursive: true }) + + const command = await wrap([], [], ['/']) + + expect(countBinds(command, '--ro-bind', bashrc, bashrc)).toBe(1) + expect(countBinds(command, '--ro-bind', hooks, hooks)).toBe(1) + // Absent: blocked from being created rather than bound read-only. + expect(countBinds(command, '--ro-bind', '/dev/null', mcp)).toBe(1) + }) + + it('skips every per-path deny when "/" is denied whole, and brings them back on the first veto', async () => { + // allowOnly ['/'] and denyWithinAllow ['/'] and nothing else: '/' is + // recorded as a covering deny directory, and nothing vetoes it — no + // allowed write path lies strictly beneath it, and there is no + // read-deny tmpfs at all. Every other deny, bind and stub alike, is + // then skipped as already covered, and the deny-side --ro-bind / / + // after the allow's writable --bind / / is the whole protection. + // readConfig is undefined rather than { denyOnly: [] } so the implicit + // /etc/ssh/ssh_config.d deny cannot make the verdict host-dependent. + const bashrc = join(BASE, '.bashrc') + const hooks = join(BASE, '.git', 'hooks') + const mcp = join(BASE, '.mcp.json') + writeFileSync(bashrc, '') + mkdirSync(hooks, { recursive: true }) + const wrapRoot = ( + allowOnly: string[], + readConfig: { denyOnly: string[] } | undefined, + ) => + wrapCommandWithSandboxLinux({ + command: 'echo hello', + needsNetworkRestriction: false, + readConfig, + writeConfig: { allowOnly, denyWithinAllow: ['/'] }, + }) + + const rootDeniedWhole = await wrapRoot(['/'], undefined) + + expect(countBinds(rootDeniedWhole, '--bind', '/', '/')).toBe(1) + // Two: the base root mount, then the deny bind of '/'. + expect(countBinds(rootDeniedWhole, '--ro-bind', '/', '/')).toBe(2) + expect(rootDeniedWhole.lastIndexOf('--ro-bind / /')).toBeGreaterThan( + rootDeniedWhole.indexOf('--bind / /'), + ) + expect(countBinds(rootDeniedWhole, '--ro-bind', bashrc, bashrc)).toBe(0) + expect(countBinds(rootDeniedWhole, '--ro-bind', hooks, hooks)).toBe(0) + expect(countBinds(rootDeniedWhole, '--ro-bind', '/dev/null', mcp)).toBe(0) + expect(rootDeniedWhole).not.toContain('claude-empty-') + + // A second allow entry vetoes '/' — it lies strictly beneath it — and + // so does a single read-deny directory, the re-application's trigger. + // Either way every per-path deny comes back. + const secondAllow = await wrapRoot(['/', AREA], undefined) + expect(countBinds(secondAllow, '--ro-bind', bashrc, bashrc)).toBe(1) + expect(countBinds(secondAllow, '--ro-bind', '/dev/null', mcp)).toBe(1) + + const readDenied = join(BASE, 'ro') + mkdirSync(readDenied) + const oneReadDeny = await wrapRoot(['/'], { denyOnly: [readDenied] }) + expect(countBinds(oneReadDeny, '--ro-bind', bashrc, bashrc)).toBe(1) + expect(countBinds(oneReadDeny, '--ro-bind', '/dev/null', mcp)).toBe(1) + }) + + it.skipIf(!BWRAP_CAN_NAMESPACE)( + 'boots with "/" denied whole, and the lone deny bind holds the write off', + async () => { + // The argv arm above pins a single `--ro-bind / /` after the allow's + // `--bind / /` as the entire protection. Prove bubblewrap agrees: + // it starts (nothing beneath the read-only root needs creating, + // because every per-path deny was skipped), the tree it re-binds is + // readable, and a write through it fails and changes nothing. + const wrapRootDeniedWhole = (command: string) => + wrapCommandWithSandboxLinux({ + command, + needsNetworkRestriction: false, + readConfig: undefined, + writeConfig: { allowOnly: ['/'], denyWithinAllow: ['/'] }, + }) + + const write = run( + await wrapRootDeniedWhole(`echo BOOTED; sh -c 'echo x >> ${FILE}'`), + ) + expect(write.stdout).toContain('BOOTED') + expectDeniedByReadOnlyMount(write) + expect(readFileSync(FILE, 'utf8')).toBe('{}\n') + + const read = run(await wrapRootDeniedWhole(`echo BOOTED; cat ${FILE}`)) + expect(read.error).toBeUndefined() + expect(read.stderr).not.toContain('bwrap:') + expect(read.status).toBe(0) + expect(read.stdout).toContain('BOOTED') + expect(read.stdout).toContain('{}') + }, + ) + + it('masks a symlinked ancestor of a deny path', async () => { + // A self-referential link: resolveSymlinkedDenyPath gives up and the + // fail-closed branch masks the symlink component, so it cannot be + // deleted and recreated as a real directory. + const loop = join(AREA, 'loop') + symlinkSync('loop', loop) + + const command = await wrap([join(loop, 'settings.json')], [], ['/']) + + expect(countBinds(command, '--ro-bind', '/dev/null', loop)).toBe(1) + }) + + it.skipIf(!BWRAP_CAN_NAMESPACE)( + 'denies the write at runtime and leaves the rest of the tree writable', + async () => { + const control = join(AREA, 'control.txt') + writeFileSync(control, '') + + const denied = run( + await wrap([FILE], [], ['/'], `sh -c 'echo x >> ${FILE}'`), + ) + expectDeniedByReadOnlyMount(denied) + expect(readFileSync(FILE, 'utf8')).toBe('{}\n') + + const allowed = run( + await wrap([FILE], [], ['/'], `sh -c 'echo ok >> ${control}'`), + ) + expect(allowed.error).toBeUndefined() + expect(allowed.stderr).not.toContain('bwrap:') + expect(allowed.status).toBe(0) + expect(readFileSync(control, 'utf8')).toBe('ok\n') + }, + ) + }) })