diff --git a/src/sandbox/linux-sandbox-utils.ts b/src/sandbox/linux-sandbox-utils.ts index cc28c9f..94823b1 100644 --- a/src/sandbox/linux-sandbox-utils.ts +++ b/src/sandbox/linux-sandbox-utils.ts @@ -55,11 +55,19 @@ export interface LinuxSandboxParams { seccompConfig?: { bpfPath?: string; applyPath?: string } /** Abort signal to cancel the ripgrep scan */ abortSignal?: AbortSignal + /** Callback for warnings about unprotected paths */ + onWarnings?: (warnings: string[]) => void } /** Default max depth for searching dangerous files */ const DEFAULT_MANDATORY_DENY_SEARCH_DEPTH = 3 +function defaultWarningHandler(warnings: string[]): void { + for (const warning of warnings) { + console.warn(`[Sandbox] Unprotected path: ${warning}`) + } +} + /** * Find if any component of the path is a symlink within the allowed write paths. * Returns the symlink path if found, or null if no symlinks. @@ -100,30 +108,6 @@ function findSymlinkInPath( return null } -/** - * Find the first non-existent path component. - * E.g., for "/existing/parent/nonexistent/child/file.txt" where /existing/parent exists, - * returns "/existing/parent/nonexistent" - * - * This is used to block creation of non-existent deny paths by mounting /dev/null - * at the first missing component, preventing mkdir from creating the parent directories. - */ -function findFirstNonExistentComponent(targetPath: string): string { - const parts = targetPath.split(path.sep) - let currentPath = '' - - for (const part of parts) { - if (!part) continue // Skip empty parts (leading /) - const nextPath = currentPath + path.sep + part - if (!fs.existsSync(nextPath)) { - return nextPath - } - currentPath = nextPath - } - - return targetPath // Shouldn't reach here if called correctly -} - /** * Get mandatory deny paths using ripgrep (Linux only). * Uses a SINGLE ripgrep call with multiple glob patterns for efficiency. @@ -532,8 +516,10 @@ async function generateFilesystemArgs( mandatoryDenySearchDepth: number = DEFAULT_MANDATORY_DENY_SEARCH_DEPTH, allowGitConfig = false, abortSignal?: AbortSignal, + onWarnings?: (warnings: string[]) => void, ): Promise { const args: string[] = [] + const warnings: string[] = [] // fs already imported // Determine initial root mount based on write restrictions @@ -570,8 +556,12 @@ async function generateFilesystemArgs( } // Deny writes within allowed paths (user-specified + mandatory denies) + const userDenyPaths = writeConfig.denyWithinAllow || [] + const userDenyPathsNormalized = new Set( + userDenyPaths.map(p => normalizePathForSandbox(p)), + ) const denyPaths = [ - ...(writeConfig.denyWithinAllow || []), + ...userDenyPaths, ...(await linuxGetMandatoryDenyPaths( ripgrepConfig, mandatoryDenySearchDepth, @@ -588,50 +578,27 @@ async function generateFilesystemArgs( continue } - // Check for symlinks in the path - if any parent component is a symlink, - // mount /dev/null there to prevent symlink replacement attacks. - // Attack scenario: .claude is a symlink to ./decoy/, attacker deletes - // symlink and creates real .claude/settings.json with malicious hooks. - const symlinkInPath = findSymlinkInPath(normalizedPath, allowedWritePaths) - if (symlinkInPath) { - args.push('--ro-bind', '/dev/null', symlinkInPath) + // Skip non-existent paths. Using --ro-bind /dev/null on non-existent paths + // creates mount point inodes that persist on host after sandbox exits. + // Warn only for user-specified paths (mandatory denies are expected to vary). + if (!fs.existsSync(normalizedPath)) { + if (userDenyPathsNormalized.has(normalizedPath)) { + warnings.push(`cannot protect non-existent path: ${normalizedPath}`) + } logForDebugging( - `[Sandbox Linux] Mounted /dev/null at symlink ${symlinkInPath} to prevent symlink replacement attack`, + `[Sandbox Linux] Skipping non-existent deny path: ${normalizedPath}`, ) continue } - // Handle non-existent paths by mounting /dev/null to block creation - if (!fs.existsSync(normalizedPath)) { - // Find the deepest existing ancestor directory - let ancestorPath = path.dirname(normalizedPath) - while (ancestorPath !== '/' && !fs.existsSync(ancestorPath)) { - ancestorPath = path.dirname(ancestorPath) - } - - // Only protect if the existing ancestor is within an allowed write path - const ancestorIsWithinAllowedPath = allowedWritePaths.some( - allowedPath => - ancestorPath.startsWith(allowedPath + '/') || - ancestorPath === allowedPath || - normalizedPath.startsWith(allowedPath + '/'), + // Warn about symlinks in path (potential symlink replacement attack vector). + // Only warn for user-specified paths (mandatory denies are expected to vary). + const symlinkInPath = findSymlinkInPath(normalizedPath, allowedWritePaths) + if (symlinkInPath && userDenyPathsNormalized.has(normalizedPath)) { + warnings.push(`symlink in path may be replaced: ${normalizedPath}`) + logForDebugging( + `[Sandbox Linux] Found symlink in deny path: ${symlinkInPath}`, ) - - if (ancestorIsWithinAllowedPath) { - // Mount /dev/null at the first non-existent path component - // This blocks creation of the entire path by making the first - // missing component appear as an empty file (mkdir will fail) - const firstNonExistent = findFirstNonExistentComponent(normalizedPath) - args.push('--ro-bind', '/dev/null', firstNonExistent) - logForDebugging( - `[Sandbox Linux] Mounted /dev/null at ${firstNonExistent} to block creation of ${normalizedPath}`, - ) - } else { - logForDebugging( - `[Sandbox Linux] Skipping non-existent deny path not within allowed paths: ${normalizedPath}`, - ) - } - continue } // Only add deny binding if this path is within an allowed write path @@ -683,6 +650,12 @@ async function generateFilesystemArgs( } } + const uniqueWarnings = [...new Set(warnings)] + if (uniqueWarnings.length > 0) { + const handler = onWarnings ?? defaultWarningHandler + handler(uniqueWarnings) + } + return args } @@ -752,6 +725,7 @@ export async function wrapCommandWithSandboxLinux( allowGitConfig = false, seccompConfig, abortSignal, + onWarnings, } = params // Determine if we have restrictions to apply @@ -880,6 +854,7 @@ export async function wrapCommandWithSandboxLinux( mandatoryDenySearchDepth, allowGitConfig, abortSignal, + onWarnings, ) bwrapArgs.push(...fsArgs) diff --git a/test/sandbox/mandatory-deny-paths.test.ts b/test/sandbox/mandatory-deny-paths.test.ts index f43c5fa..830781b 100644 --- a/test/sandbox/mandatory-deny-paths.test.ts +++ b/test/sandbox/mandatory-deny-paths.test.ts @@ -6,7 +6,6 @@ import { writeFileSync, readFileSync, symlinkSync, - existsSync, } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' @@ -486,142 +485,28 @@ describe('Mandatory Deny Paths - Integration Tests', () => { }) }) - describe('Non-existent deny path protection (Linux only)', () => { - // This tests the fix for sandbox escape via creating non-existent deny paths - // Only applicable to Linux since it uses /dev/null mounting - - async function runSandboxedWriteWithDenyPaths( - command: string, - denyPaths: string[], - ): Promise<{ success: boolean; stdout: string; stderr: string }> { - const platform = getPlatform() - if (platform !== 'linux') { - return { success: true, stdout: '', stderr: '' } - } - - const writeConfig = { - allowOnly: ['.'], - denyWithinAllow: denyPaths, - } - - const wrappedCommand = await wrapCommandWithSandboxLinux({ - command, - needsNetworkRestriction: false, - readConfig: undefined, - writeConfig, - enableWeakerNestedSandbox: true, - }) - - const result = spawnSync(wrappedCommand, { - shell: true, - encoding: 'utf8', - timeout: 10000, - }) - - return { - success: result.status === 0, - stdout: result.stdout || '', - stderr: result.stderr || '', - } - } - - it('blocks creation of non-existent file when parent dir exists', async () => { - if (getPlatform() !== 'linux') return - - // .claude directory exists from beforeAll setup - // .claude/settings.json does NOT exist - const nonExistentFile = '.claude/settings.json' - - const result = await runSandboxedWriteWithDenyPaths( - `echo '{"hooks":{}}' > '${nonExistentFile}'`, - [join(TEST_DIR, nonExistentFile)], - ) - - expect(result.success).toBe(false) - // Verify file content was NOT written (bwrap may create empty mount point file) - const content = readFileSync(nonExistentFile, 'utf8') - expect(content).toBe('') - }) - - it('blocks creation of non-existent file when parent dir also does not exist', async () => { - if (getPlatform() !== 'linux') return - - // nonexistent-dir does NOT exist - const nonExistentPath = 'nonexistent-dir/settings.json' - - const result = await runSandboxedWriteWithDenyPaths( - `mkdir -p nonexistent-dir && echo '{"hooks":{}}' > '${nonExistentPath}'`, - [join(TEST_DIR, nonExistentPath)], - ) - - expect(result.success).toBe(false) - // bwrap mounts /dev/null at first non-existent component, blocking mkdir - // The mount point file is created but is empty (from /dev/null) - const content = readFileSync('nonexistent-dir', 'utf8') - expect(content).toBe('') - }) - - it('blocks creation of deeply nested non-existent path', async () => { + describe('Protected path warnings (Linux only)', () => { + it('calls onWarnings for user-specified non-existent deny paths', async () => { if (getPlatform() !== 'linux') return - // a/b/c/file.txt does NOT exist - const nonExistentPath = 'a/b/c/file.txt' - - const result = await runSandboxedWriteWithDenyPaths( - `mkdir -p a/b/c && echo 'test' > '${nonExistentPath}'`, - [join(TEST_DIR, nonExistentPath)], - ) - - expect(result.success).toBe(false) - // bwrap mounts /dev/null at 'a' (first non-existent component), blocking mkdir - // The mount point file is created but is empty (from /dev/null) - const content = readFileSync('a', 'utf8') - expect(content).toBe('') - }) - }) - - describe('Symlink replacement attack protection (Linux only)', () => { - // This tests the fix for symlink replacement attacks where an attacker - // could delete a symlink and create a real directory with malicious content + const nonExistentPath = join(TEST_DIR, 'does-not-exist.txt') + const warnings: string[] = [] - async function runSandboxedCommandWithDenyPaths( - command: string, - denyPaths: string[], - ): Promise<{ success: boolean; stdout: string; stderr: string }> { - const platform = getPlatform() - if (platform !== 'linux') { - return { success: true, stdout: '', stderr: '' } - } - - const writeConfig = { - allowOnly: ['.'], - denyWithinAllow: denyPaths, - } - - const wrappedCommand = await wrapCommandWithSandboxLinux({ - command, + await wrapCommandWithSandboxLinux({ + command: 'echo test', needsNetworkRestriction: false, readConfig: undefined, - writeConfig, + writeConfig: { allowOnly: ['.'], denyWithinAllow: [nonExistentPath] }, + onWarnings: w => warnings.push(...w), }) - const result = spawnSync(wrappedCommand, { - shell: true, - encoding: 'utf8', - timeout: 10000, - }) - - return { - success: result.status === 0, - stdout: result.stdout || '', - stderr: result.stderr || '', - } - } + expect(warnings.length).toBeGreaterThan(0) + expect(warnings[0]).toContain('non-existent') + }) - it('blocks symlink replacement attack on .claude directory', async () => { + it('calls onWarnings for symlinks in protected paths', async () => { if (getPlatform() !== 'linux') return - // Setup: Create a symlink .claude -> decoy (simulating malicious git repo) const decoyDir = 'symlink-decoy' const claudeSymlink = 'symlink-claude' mkdirSync(decoyDir, { recursive: true }) @@ -629,57 +514,48 @@ describe('Mandatory Deny Paths - Integration Tests', () => { symlinkSync(decoyDir, claudeSymlink) try { - // The deny path is the settings.json through the symlink const denyPath = join(TEST_DIR, claudeSymlink, 'settings.json') + const warnings: string[] = [] - // Attacker tries to: - // 1. Delete the symlink - // 2. Create a real directory - // 3. Create malicious settings.json - const result = await runSandboxedCommandWithDenyPaths( - `rm ${claudeSymlink} && mkdir ${claudeSymlink} && echo '{"hooks":{}}' > ${claudeSymlink}/settings.json`, - [denyPath], - ) - - // The attack should fail - symlink is protected with /dev/null mount - expect(result.success).toBe(false) + await wrapCommandWithSandboxLinux({ + command: 'echo test', + needsNetworkRestriction: false, + readConfig: undefined, + writeConfig: { allowOnly: ['.'], denyWithinAllow: [denyPath] }, + onWarnings: w => warnings.push(...w), + }) - // Verify the symlink still exists on host (was not deleted) - expect(existsSync(claudeSymlink)).toBe(true) + expect(warnings.length).toBeGreaterThan(0) + expect(warnings[0]).toContain('symlink') } finally { - // Cleanup rmSync(claudeSymlink, { force: true }) rmSync(decoyDir, { recursive: true, force: true }) } }) - it('blocks deletion of symlink in protected path', async () => { + it('does not call onWarnings for existing non-symlink paths', async () => { if (getPlatform() !== 'linux') return - // Setup: Create a symlink - const targetDir = 'symlink-target-dir' - const symlinkPath = 'protected-symlink' - mkdirSync(targetDir, { recursive: true }) - writeFileSync(join(targetDir, 'file.txt'), 'content') - symlinkSync(targetDir, symlinkPath) + const existingFile = 'existing-deny-file.txt' + writeFileSync(existingFile, 'content') try { - const denyPath = join(TEST_DIR, symlinkPath, 'file.txt') + const denyPath = join(TEST_DIR, existingFile) + let callbackCalled = false - // Try to just delete the symlink - const result = await runSandboxedCommandWithDenyPaths( - `rm ${symlinkPath}`, - [denyPath], - ) - - // Should fail - symlink is mounted with /dev/null - expect(result.success).toBe(false) + await wrapCommandWithSandboxLinux({ + command: 'echo test', + needsNetworkRestriction: false, + readConfig: undefined, + writeConfig: { allowOnly: ['.'], denyWithinAllow: [denyPath] }, + onWarnings: () => { + callbackCalled = true + }, + }) - // Symlink should still exist - expect(existsSync(symlinkPath)).toBe(true) + expect(callbackCalled).toBe(false) } finally { - rmSync(symlinkPath, { force: true }) - rmSync(targetDir, { recursive: true, force: true }) + rmSync(existingFile, { force: true }) } }) })