From 3cdb4680147abd276610ba9a46aa2fd40fac4046 Mon Sep 17 00:00:00 2001 From: Alice Poteat Date: Mon, 30 Mar 2026 14:20:00 -0700 Subject: [PATCH] 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. --- src/sandbox/linux-sandbox-utils.ts | 66 ++++++++-- src/sandbox/macos-sandbox-utils.ts | 10 ++ test/sandbox/allow-read.test.ts | 199 +++++++++++++++++++++++++++-- 3 files changed, 253 insertions(+), 22 deletions(-) diff --git a/src/sandbox/linux-sandbox-utils.ts b/src/sandbox/linux-sandbox-utils.ts index d3336cd..6eb7ceb 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) @@ -728,7 +732,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 +784,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 +815,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 +826,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 +866,40 @@ 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 if already re-bound as writable above + if ( + allowedWritePaths.some( + w => 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 +925,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..d329820 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' @@ -222,6 +215,192 @@ describe('allowRead precedence over denyRead', () => { }) }) +/** + * 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], + } + + const wrappedCommand = await wrapCommandWithSandboxLinux({ + 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('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, + }) + + 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: [], + }, + }) + + const result = spawnSync(wrappedCommand, { + shell: true, + encoding: 'utf8', + timeout: 5000, + }) + + expect(result.status).toBe(0) + expect(result.stdout).toContain('WRITE_OK') + }) +}) + /** * Tests that allowRead-only configs (no denyRead) do not trigger sandbox overhead. */