From 41a57927c74e3627c87fd91cdd3d841eab69089a Mon Sep 17 00:00:00 2001 From: Alice T'Poteat Date: Mon, 30 Mar 2026 15:42:43 -0700 Subject: [PATCH] fix: allow filesystem root traversal when denyRead includes '/' (#190) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix allowRead carve-outs when denyRead covers filesystem root denyRead: ['/'] + allowRead: [] denied everything on both platforms (follow-up to #166, closes #10). macOS: (deny file-read* (subpath "/")) blocks the root inode itself; no allowWithinDeny subpath covers "/", so dyld SIGABRTs before exec. Emit (allow file-read* (literal "/")) so path traversal through root works. Exposes `ls /` dirent names but no subtree contents. Linux: two issues. --tmpfs / wipes every prior mount (ro-bind /, write binds, denyWrite ro-binds), and the carve-out prefix check startsWith('/' + '/') never matches. Expand a root deny into its children (minus /proc, /dev, /sys) so the existing per-dir tmpfs + re-bind logic applies. Also re-bind any allowWrite paths that land under a tmpfs'd deny dir (previously they went read-only), and buffer denyWrite ro-binds until after denyRead processing so .git/hooks protection survives a tmpfs over an ancestor. * Dedup denyWrite entries post-normalization to prevent bwrap failure Two denyWrite entries that converge to the same path after normalizePathForSandbox() produced a duplicate --ro-bind /dev/null . On the second bind, is a char device (the first bind's mount); bwrap's ensure_file() only short-circuits on S_ISREG, so it falls through to creat() and fails on the now-read-only mount. Every sandboxed command errors out. Common trigger: same path specified in two config surfaces that feed into denyWrite, e.g. a file-edit permission deny and a sandbox.filesystem.denyWrite entry for the same file. linuxGetMandatoryDenyPaths() already does [...new Set(denyPaths)] but that's pre-normalization — ~/.foo and the expanded absolute path differ as strings there. * Wire allow-read and wrap-with-sandbox tests into CI test:integration only listed integration.test.ts explicitly; these two files were never run despite containing sandbox-exec/bwrap integration tests. The root-deny and dedup tests added in this PR need CI coverage. * Fix Linux test assumptions: seccomp binary path, ro-bind src+dest count Root deny hides the repo's vendor/seccomp/ dir, so apply-seccomp can't load inside the sandbox and bash returns 127. Bypass seccomp with allowAllUnixSockets: true — socket blocking is orthogonal here. Dedup assertion was off by 2x: --ro-bind

contains

twice. The fix works (was 4 occurrences, now 2). * Narrow allowRead skip check to writes actually re-bound under this tmpfs The skip check introduced in 3cdb468 was too broad: it skipped any allowPath under any allowWrite, not just writes that were re-bound in the current tmpfs iteration. With allowWrite as an ancestor of denyRead (e.g. allowWrite: [~], denyRead: [~/.ssh], allowRead: [~/.ssh/known_hosts]) the write path isn't wiped and isn't re-bound, but the skip check still matched — known_hosts was left sitting in the empty tmpfs. Narrow the predicate: only skip if the write path itself is under the tmpfs'd deny dir (i.e. it was re-bound just above). --- package.json | 2 +- src/sandbox/linux-sandbox-utils.ts | 78 ++++++-- src/sandbox/macos-sandbox-utils.ts | 10 ++ test/sandbox/allow-read.test.ts | 238 +++++++++++++++++++++++-- test/sandbox/wrap-with-sandbox.test.ts | 40 ++++- 5 files changed, 344 insertions(+), 24 deletions(-) diff --git a/package.json b/package.json index 25e14bc..43941eb 100644 --- a/package.json +++ b/package.json @@ -18,7 +18,7 @@ "clean": "rm -rf dist", "test": "bun test", "test:unit": "bun test test/config-validation.test.ts test/sandbox/seccomp-filter.test.ts", - "test:integration": "bun test test/sandbox/integration.test.ts", + "test:integration": "bun test test/sandbox/integration.test.ts test/sandbox/allow-read.test.ts test/sandbox/wrap-with-sandbox.test.ts", "typecheck": "tsc --noEmit", "lint": "eslint 'src/**/*.ts' --fix --cache --cache-location=node_modules/.cache/.eslintcache", "lint:check": "eslint 'src/**/*.ts' --cache --cache-location=node_modules/.cache/.eslintcache", diff --git a/src/sandbox/linux-sandbox-utils.ts b/src/sandbox/linux-sandbox-utils.ts index d3336cd..16851a1 100644 --- a/src/sandbox/linux-sandbox-utils.ts +++ b/src/sandbox/linux-sandbox-utils.ts @@ -644,14 +644,18 @@ async function generateFilesystemArgs( const args: string[] = [] // fs already imported + // Collect normalized allowed write paths. Populated in the writeConfig + // block, read again in the denyRead loop to re-bind writes under tmpfs. + const allowedWritePaths: string[] = [] + // denyWrite binds are buffered and emitted after denyRead processing so that + // a denyRead tmpfs over an ancestor directory doesn't wipe them out. + const denyWriteArgs: string[] = [] + // Determine initial root mount based on write restrictions if (writeConfig) { // Write restrictions: Start with read-only root, then allow writes to specific paths args.push('--ro-bind', '/', '/') - // Collect normalized allowed write paths for later checking - const allowedWritePaths: string[] = [] - // Allow writes to specific paths for (const pathPattern of writeConfig.allowOnly || []) { const normalizedPath = normalizePathForSandbox(pathPattern) @@ -714,8 +718,15 @@ async function generateFilesystemArgs( )), ] + // Dedup post-normalization: entries like ['~/.foo', '/home/user/.foo'] + // converge to the same path here. A duplicate --ro-bind /dev/null + // hits a char device on the second pass and bwrap's ensure_file() falls + // through to creat() on a read-only mount. + const seenDenyWrite = new Set() for (const pathPattern of denyPaths) { const normalizedPath = normalizePathForSandbox(pathPattern) + if (seenDenyWrite.has(normalizedPath)) continue + seenDenyWrite.add(normalizedPath) // Skip /dev/* paths since --dev /dev already handles them if (normalizedPath.startsWith('/dev/')) { @@ -728,7 +739,7 @@ async function generateFilesystemArgs( // symlink and creates real .claude/settings.json with malicious hooks. const symlinkInPath = findSymlinkInPath(normalizedPath, allowedWritePaths) if (symlinkInPath) { - args.push('--ro-bind', '/dev/null', symlinkInPath) + denyWriteArgs.push('--ro-bind', '/dev/null', symlinkInPath) logForDebugging( `[Sandbox Linux] Mounted /dev/null at symlink ${symlinkInPath} to prevent symlink replacement attack`, ) @@ -780,14 +791,14 @@ async function generateFilesystemArgs( const emptyDir = fs.mkdtempSync( path.join(tmpdir(), 'claude-empty-'), ) - args.push('--ro-bind', emptyDir, firstNonExistent) + denyWriteArgs.push('--ro-bind', emptyDir, firstNonExistent) bwrapMountPoints.add(firstNonExistent) registerExitCleanupHandler() logForDebugging( `[Sandbox Linux] Mounted empty dir at ${firstNonExistent} to block creation of ${normalizedPath}`, ) } else { - args.push('--ro-bind', '/dev/null', firstNonExistent) + denyWriteArgs.push('--ro-bind', '/dev/null', firstNonExistent) bwrapMountPoints.add(firstNonExistent) registerExitCleanupHandler() logForDebugging( @@ -811,7 +822,7 @@ async function generateFilesystemArgs( ) if (isWithinAllowedPath) { - args.push('--ro-bind', normalizedPath, normalizedPath) + denyWriteArgs.push('--ro-bind', normalizedPath, normalizedPath) } else { logForDebugging( `[Sandbox Linux] Skipping deny path not within allowed paths: ${normalizedPath}`, @@ -822,13 +833,30 @@ async function generateFilesystemArgs( // No write restrictions: Allow all writes args.push('--bind', '/', '/') } + // denyWriteArgs is emitted after the denyRead loop below. // Handle read restrictions by mounting tmpfs over denied paths - const readDenyPaths = [...(readConfig?.denyOnly || [])] + const readDenyPaths: string[] = [] const readAllowPaths = (readConfig?.allowWithinDeny || []).map(p => normalizePathForSandbox(p), ) + // --tmpfs / would wipe all prior mounts (ro-bind /, write binds, deny binds). + // Expand a root deny into its direct children so the existing per-dir tmpfs + // + re-bind logic applies. Skip /proc and /dev: they're remounted by the + // caller after this function returns. Skip /sys: kernel interface, tmpfs + // over it breaks tooling and the host /sys is already read-only via ro-bind. + const rootSkip = new Set(['proc', 'dev', 'sys']) + for (const p of readConfig?.denyOnly || []) { + if (normalizePathForSandbox(p) === '/') { + for (const child of fs.readdirSync('/')) { + if (!rootSkip.has(child)) readDenyPaths.push('/' + child) + } + } else { + readDenyPaths.push(p) + } + } + // Always hide /etc/ssh/ssh_config.d to avoid permission issues with OrbStack // SSH is very strict about config file permissions and ownership, and they can // appear wrong inside the sandbox causing "Bad owner or permissions" errors @@ -845,24 +873,45 @@ async function generateFilesystemArgs( continue } + const denySep = normalizedPath === '/' ? '/' : normalizedPath + '/' const readDenyStat = fs.statSync(normalizedPath) if (readDenyStat.isDirectory()) { args.push('--tmpfs', normalizedPath) + // tmpfs wiped any earlier write binds under this path — restore them. + for (const writePath of allowedWritePaths) { + if (writePath.startsWith(denySep) || writePath === normalizedPath) { + args.push('--bind', writePath, writePath) + logForDebugging( + `[Sandbox Linux] Re-bound write path wiped by denyRead tmpfs: ${writePath}`, + ) + } + } + // Re-allow specific paths within the denied directory (allowRead overrides denyRead). // After mounting tmpfs over the denied dir, bind back the allowed subdirectories // so they are readable again. for (const allowPath of readAllowPaths) { - if ( - allowPath.startsWith(normalizedPath + '/') || - allowPath === normalizedPath - ) { + if (allowPath.startsWith(denySep) || allowPath === normalizedPath) { if (!fs.existsSync(allowPath)) { logForDebugging( `[Sandbox Linux] Skipping non-existent read allow path: ${allowPath}`, ) continue } + // Skip only if a write path was re-bound just above AND covers + // allowPath. A write path that's an ancestor of the deny dir isn't + // re-bound (it wasn't wiped), so allowPath under it still needs + // its own ro-bind here. + if ( + allowedWritePaths.some( + w => + (w.startsWith(denySep) || w === normalizedPath) && + (allowPath === w || allowPath.startsWith(w + '/')), + ) + ) { + continue + } // Bind the allowed path back over the tmpfs so it's readable args.push('--ro-bind', allowPath, allowPath) logForDebugging( @@ -888,6 +937,11 @@ async function generateFilesystemArgs( } } + // Emitting denyWrite last means these ro-binds layer on top of any write + // paths the denyRead loop just re-bound. Before this ordering, tmpfs over + // an ancestor of cwd would wipe the .git/hooks protection. + args.push(...denyWriteArgs) + return args } diff --git a/src/sandbox/macos-sandbox-utils.ts b/src/sandbox/macos-sandbox-utils.ts index b88df28..cde516f 100644 --- a/src/sandbox/macos-sandbox-utils.ts +++ b/src/sandbox/macos-sandbox-utils.ts @@ -213,6 +213,7 @@ function generateReadRules( } const rules: string[] = [] + let deniesRoot = false // Start by allowing everything rules.push(`(allow file-read*)`) @@ -221,6 +222,8 @@ function generateReadRules( for (const pathPattern of config.denyOnly || []) { const normalizedPath = normalizePathForSandbox(pathPattern) + if (normalizedPath === '/') deniesRoot = true + if (containsGlobChars(normalizedPath)) { // Use regex matching for glob patterns const regexPattern = globToRegex(normalizedPath) @@ -239,6 +242,13 @@ function generateReadRules( } } + // (subpath "/") denies the root inode itself; allowWithinDeny subpaths don't + // cover "/", so dyld aborts before exec. Re-allow the literal root so path + // traversal works. This exposes `ls /` dirent names but no subtree contents. + if (deniesRoot) { + rules.push(`(allow file-read* (literal "/"))`) + } + // Re-allow specific paths within denied regions (allowWithinDeny takes precedence) for (const pathPattern of config.allowWithinDeny || []) { const normalizedPath = normalizePathForSandbox(pathPattern) diff --git a/test/sandbox/allow-read.test.ts b/test/sandbox/allow-read.test.ts index 8eeaa13..55ea6e5 100644 --- a/test/sandbox/allow-read.test.ts +++ b/test/sandbox/allow-read.test.ts @@ -1,19 +1,12 @@ import { describe, it, expect, beforeAll, afterAll } from 'bun:test' import { spawnSync } from 'node:child_process' -import { - existsSync, - mkdirSync, - rmSync, - writeFileSync, -} from 'node:fs' -import { tmpdir } from 'node:os' +import { existsSync, mkdirSync, rmSync, writeFileSync } from 'node:fs' +import { homedir, tmpdir } from 'node:os' import { join } from 'node:path' import { getPlatform } from '../../src/utils/platform.js' import { wrapCommandWithSandboxMacOS } from '../../src/sandbox/macos-sandbox-utils.js' import { wrapCommandWithSandboxLinux } from '../../src/sandbox/linux-sandbox-utils.js' -import type { - FsReadRestrictionConfig, -} from '../../src/sandbox/sandbox-schemas.js' +import type { FsReadRestrictionConfig } from '../../src/sandbox/sandbox-schemas.js' function skipIfNotMacOS(): boolean { return getPlatform() !== 'macos' @@ -219,6 +212,231 @@ describe('allowRead precedence over denyRead', () => { expect(result.status).not.toBe(0) expect(result.stdout).not.toContain(TEST_SECRET_CONTENT) }) + + // Regression: the write-path skip check in the allowRead re-bind loop was + // too broad — it skipped any allowPath under ANY allowWrite, not just + // writes actually re-bound under this tmpfs. With allowWrite as an + // ancestor of denyRead (not wiped, not re-bound), allowRead under it was + // skipped and left sitting in the empty tmpfs. + // Shape: allowWrite: [~], denyRead: [~/.ssh], allowRead: [~/.ssh/known_hosts]. + it('should re-allow under denyRead when allowWrite is an ancestor of the deny', async () => { + if (skipIfNotLinux()) { + return + } + + const wrappedCommand = await wrapCommandWithSandboxLinux({ + command: `cat ${TEST_ALLOWED_FILE}`, + needsNetworkRestriction: false, + readConfig: { + denyOnly: [TEST_DENIED_DIR], + allowWithinDeny: [TEST_ALLOWED_SUBDIR], + }, + writeConfig: { + allowOnly: [TEST_BASE_DIR], // ancestor of denyRead + denyWithinAllow: [], + }, + }) + + const result = spawnSync(wrappedCommand, { + shell: true, + encoding: 'utf8', + timeout: 5000, + }) + + expect(result.status).toBe(0) + expect(result.stdout).toContain(TEST_ALLOWED_CONTENT) + }) + }) +}) + +/** + * Regression: denyRead: ['/'] + allowRead: [] used to deny everything. + * + * macOS: (subpath "/") denies the root inode; no allowWithinDeny subpath covers + * "/", so dyld SIGABRTs before exec. Fix emits (allow file-read* (literal "/")). + * Linux: --tmpfs / wiped all prior mounts, and the carve-out prefix check + * startsWith('/' + '/') never matched. Fix expands '/' into its children. + * + * Test dir lives under $HOME (not tmpdir) so the macOS /tmp → /private/tmp + * symlink doesn't confuse Seatbelt path matching. + */ +describe('allowRead carve-out with denyRead at filesystem root (issue #10)', () => { + const TEST_DIR = join( + homedir(), + '.sandbox-runtime-test-root-deny-' + Date.now(), + ) + const TEST_FILE = join(TEST_DIR, 'visible.txt') + const TEST_CONTENT = 'ROOT_CARVE_OUT' + // Paths needed for sh/cat to load at all when the whole filesystem is denied. + // /private covers /tmp and /var (macOS symlinks). /lib* for Linux ld.so. + const EXEC_DEPS = [ + '/bin', + '/usr', + '/lib', + '/lib64', + '/System', + '/private', + '/dev', + '/etc', + ] + + beforeAll(() => { + if (getPlatform() !== 'macos' && getPlatform() !== 'linux') { + return + } + mkdirSync(TEST_DIR, { recursive: true }) + writeFileSync(TEST_FILE, TEST_CONTENT) + }) + + afterAll(() => { + if (existsSync(TEST_DIR)) { + rmSync(TEST_DIR, { recursive: true, force: true }) + } + }) + + it('macOS: re-allows carve-out under a root-level deny', () => { + if (skipIfNotMacOS()) { + return + } + + const readConfig: FsReadRestrictionConfig = { + denyOnly: ['/'], + allowWithinDeny: [TEST_DIR, ...EXEC_DEPS], + } + + const wrappedCommand = wrapCommandWithSandboxMacOS({ + command: `cat ${TEST_FILE}`, + needsNetworkRestriction: false, + readConfig, + writeConfig: undefined, + }) + + const result = spawnSync(wrappedCommand, { + shell: true, + encoding: 'utf8', + timeout: 5000, + }) + + expect(result.status).toBe(0) + expect(result.stdout).toContain(TEST_CONTENT) + }) + + it('macOS: still denies paths outside the carve-out under a root-level deny', () => { + if (skipIfNotMacOS()) { + return + } + + const outside = join(homedir(), '.bashrc') + const readConfig: FsReadRestrictionConfig = { + denyOnly: ['/'], + allowWithinDeny: [TEST_DIR, ...EXEC_DEPS], + } + + const wrappedCommand = wrapCommandWithSandboxMacOS({ + command: `cat ${outside} 2>/dev/null; true`, + needsNetworkRestriction: false, + readConfig, + writeConfig: undefined, + }) + + const result = spawnSync(wrappedCommand, { + shell: true, + encoding: 'utf8', + timeout: 5000, + }) + + // Process must exec (no SIGABRT) and stdout must be empty (cat denied) + expect(result.status).toBe(0) + expect(result.stdout).toBe('') + }) + + it('Linux: re-allows carve-out under a root-level deny', async () => { + if (skipIfNotLinux()) { + return + } + + const readConfig: FsReadRestrictionConfig = { + denyOnly: ['/'], + allowWithinDeny: [TEST_DIR, ...EXEC_DEPS], + } + + // allowAllUnixSockets: true bypasses the seccomp path — otherwise the + // apply-seccomp binary under /vendor/ is hidden by the root deny. + const wrappedCommand = await wrapCommandWithSandboxLinux({ + command: `cat ${TEST_FILE}`, + needsNetworkRestriction: false, + readConfig, + writeConfig: undefined, + allowAllUnixSockets: true, + }) + + const result = spawnSync(wrappedCommand, { + shell: true, + encoding: 'utf8', + timeout: 5000, + }) + + expect(result.status).toBe(0) + expect(result.stdout).toContain(TEST_CONTENT) + }) + + it('Linux: still denies paths outside the carve-out under a root-level deny', async () => { + if (skipIfNotLinux()) { + return + } + + const outside = join(homedir(), '.bashrc') + const readConfig: FsReadRestrictionConfig = { + denyOnly: ['/'], + allowWithinDeny: [TEST_DIR, ...EXEC_DEPS], + } + + const wrappedCommand = await wrapCommandWithSandboxLinux({ + command: `cat ${outside} 2>/dev/null; true`, + needsNetworkRestriction: false, + readConfig, + writeConfig: undefined, + allowAllUnixSockets: true, + }) + + const result = spawnSync(wrappedCommand, { + shell: true, + encoding: 'utf8', + timeout: 5000, + }) + + expect(result.status).toBe(0) + expect(result.stdout).toBe('') + }) + + it('Linux: preserves write binds when denyRead ancestor wipes them', async () => { + if (skipIfNotLinux()) { + return + } + + const writeTarget = join(TEST_DIR, 'written.txt') + const wrappedCommand = await wrapCommandWithSandboxLinux({ + command: `echo WRITE_OK > ${writeTarget} && cat ${writeTarget}`, + needsNetworkRestriction: false, + readConfig: { + denyOnly: ['/'], + allowWithinDeny: [...EXEC_DEPS], + }, + writeConfig: { + allowOnly: [TEST_DIR], + denyWithinAllow: [], + }, + allowAllUnixSockets: true, + }) + + const result = spawnSync(wrappedCommand, { + shell: true, + encoding: 'utf8', + timeout: 5000, + }) + + expect(result.status).toBe(0) + expect(result.stdout).toContain('WRITE_OK') }) }) diff --git a/test/sandbox/wrap-with-sandbox.test.ts b/test/sandbox/wrap-with-sandbox.test.ts index 0c7581b..aed8799 100644 --- a/test/sandbox/wrap-with-sandbox.test.ts +++ b/test/sandbox/wrap-with-sandbox.test.ts @@ -4,7 +4,7 @@ import type { SandboxRuntimeConfig } from '../../src/sandbox/sandbox-config.js' import { getPlatform } from '../../src/utils/platform.js' import { wrapCommandWithSandboxLinux } from '../../src/sandbox/linux-sandbox-utils.js' import { wrapCommandWithSandboxMacOS } from '../../src/sandbox/macos-sandbox-utils.js' -import { mkdirSync, rmSync } from 'node:fs' +import { mkdirSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' @@ -717,4 +717,42 @@ describe('allowWrite glob suffix handling', () => { rmSync(parentDir, { recursive: true, force: true }) } }) + + // Regression: two denyWrite entries that converge to the same path after + // normalizePathForSandbox() produced a duplicate --ro-bind /dev/null . + // Second bind finds a char device at ; bwrap's ensure_file() doesn't + // short-circuit on S_ISCHR and falls through to creat() on a read-only mount. + it('dedups denyWrite entries that normalize to the same path (Linux)', async () => { + if (getPlatform() !== 'linux') { + return + } + + const parentDir = join(tmpdir(), `srt-test-dup-deny-${Date.now()}`) + const childFile = join(parentDir, 'dup.txt') + mkdirSync(parentDir, { recursive: true }) + writeFileSync(childFile, '') + + try { + await SandboxManager.reset() + await SandboxManager.initialize({ + network: { allowedDomains: [], deniedDomains: [] }, + filesystem: { + denyRead: [], + allowWrite: [parentDir], + // Trailing slash and bare form both realpath to childFile + denyWrite: [childFile, childFile + '/'], + }, + }) + + const result = await SandboxManager.wrapWithSandbox(command) + + // One --ro-bind contains the path twice (src + dest). + // Without dedup this was 4 occurrences (two binds). + const occurrences = result.split(childFile).length - 1 + expect(occurrences).toBe(2) + } finally { + await SandboxManager.reset() + rmSync(parentDir, { recursive: true, force: true }) + } + }) })