From c4886ba0228e4b6bf82144b120e4c75ec7e656db Mon Sep 17 00:00:00 2001 From: Conner Kupferberg Date: Sun, 6 Sep 2026 01:50:55 +0000 Subject: [PATCH] fix(sandbox): auto-deny dangerous paths and .git/hooks across all allowedWrite roots * linux: scan all allowedWritePaths alongside process.cwd() for mandatory deny targets (.git/hooks, .git/config, dotfiles) * linux: use isAtOrUnder for isWithinAnyAllowedWritePath and findSymlinkInPath to prevent root '/' containment bypass * macos: normalize allowOnly roots before scanRoots filtering to avoid dropping tildes or symlinked paths * macos: emit root-specific static paths and dual directory/subtree globs for .git/hooks * test: add integration test for external allowedWrite repo and unit test for multi-root pattern emission --- src/sandbox/linux-sandbox-utils.ts | 222 ++++++++++++---------- src/sandbox/macos-sandbox-utils.ts | 75 ++++++-- test/sandbox/mandatory-deny-paths.test.ts | 59 ++++++ 3 files changed, 244 insertions(+), 112 deletions(-) diff --git a/src/sandbox/linux-sandbox-utils.ts b/src/sandbox/linux-sandbox-utils.ts index adab73a7..e03d2aae 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 @@ -279,115 +278,133 @@ async function linuxGetMandatoryDenyPaths( maxDepth: number = DEFAULT_MANDATORY_DENY_SEARCH_DEPTH, allowGitConfig = false, abortSignal?: AbortSignal, + scanRoots: string[] = [process.cwd()], ): Promise { - const cwd = process.cwd() // Use provided signal or create a fallback controller const fallbackController = new AbortController() const signal = abortSignal ?? fallbackController.signal const dangerousDirectories = getDangerousDirectories() - // Note: Settings files are added at the callsite in sandbox-manager.ts - const denyPaths = [ - // Dangerous files in CWD - ...DANGEROUS_FILES.map(f => path.resolve(cwd, f)), - // Dangerous directories in CWD - ...dangerousDirectories.map(d => path.resolve(cwd, d)), - ] - - // Git hooks and config are only denied when .git exists as a directory. - // In git worktrees, .git is a file (e.g., "gitdir: /path/..."), so - // .git/hooks can never exist — denying it would cause bwrap to fail. - // When .git doesn't exist at all, mounting at .git would block its - // creation and break git init. - const dotGitPath = path.resolve(cwd, '.git') - let dotGitIsDirectory = false - try { - dotGitIsDirectory = fs.statSync(dotGitPath).isDirectory() - } catch { - // .git doesn't exist + const deduplicatedRoots = [ + ...new Set(scanRoots.map(r => path.resolve(r))), + ].filter(r => { + try { + return fs.existsSync(r) && fs.statSync(r).isDirectory() + } catch { + return false + } + }) + if (deduplicatedRoots.length === 0) { + deduplicatedRoots.push(process.cwd()) } - if (dotGitIsDirectory) { - // Git hooks always blocked for security - denyPaths.push(path.resolve(cwd, '.git/hooks')) + const denyPaths: string[] = [] - // Git config conditionally blocked based on allowGitConfig setting - if (!allowGitConfig) { - denyPaths.push(path.resolve(cwd, '.git/config')) + for (const root of deduplicatedRoots) { + // Dangerous files in root + for (const f of DANGEROUS_FILES) { + denyPaths.push(path.resolve(root, f)) + } + // Dangerous directories in root + for (const d of dangerousDirectories) { + denyPaths.push(path.resolve(root, d)) } - } - // Build iglob args for all patterns in one ripgrep call - const iglobArgs: string[] = [] - for (const fileName of DANGEROUS_FILES) { - iglobArgs.push('--iglob', fileName) - } - for (const dirName of dangerousDirectories) { - iglobArgs.push('--iglob', `**/${dirName}/**`) - } - // Git hooks always blocked in nested repos - iglobArgs.push('--iglob', '**/.git/hooks/**') + // Git hooks and config are only denied when .git exists as a directory. + // In git worktrees, .git is a file (e.g., "gitdir: /path/..."), so + // .git/hooks can never exist — denying it would cause bwrap to fail. + // When .git doesn't exist at all, mounting at .git would block its + // creation and break git init. + const dotGitPath = path.resolve(root, '.git') + let dotGitIsDirectory = false + try { + dotGitIsDirectory = fs.statSync(dotGitPath).isDirectory() + } catch { + // .git doesn't exist + } - // Git config conditionally blocked in nested repos - if (!allowGitConfig) { - iglobArgs.push('--iglob', '**/.git/config') - } + if (dotGitIsDirectory) { + // Git hooks always blocked for security + denyPaths.push(path.resolve(root, '.git/hooks')) - // Single ripgrep call to find all dangerous paths in subdirectories - // Limit depth for performance - deeply nested dangerous files are rare - // and the security benefit doesn't justify the traversal cost - let matches: string[] = [] - try { - matches = await ripGrep( - [ - '--files', - '--hidden', - '--max-depth', - String(maxDepth), - ...iglobArgs, - '-g', - '!**/node_modules/**', - ], - cwd, - signal, - ripgrepConfig, - ) - } catch (error) { - logForDebugging(`[Sandbox] ripgrep scan failed: ${error}`) - } + // Git config conditionally blocked based on allowGitConfig setting + if (!allowGitConfig) { + denyPaths.push(path.resolve(root, '.git/config')) + } + } - // Process matches - for (const match of matches) { - const absolutePath = path.resolve(cwd, match) - - // File inside a dangerous directory -> add the directory path - let foundDir = false - for (const dirName of [...dangerousDirectories, '.git']) { - const normalizedDirName = normalizeCaseForComparison(dirName) - const segments = absolutePath.split(path.sep) - const dirIndex = segments.findIndex( - s => normalizeCaseForComparison(s) === normalizedDirName, + // Build iglob args for all patterns in one ripgrep call + const iglobArgs: string[] = [] + for (const fileName of DANGEROUS_FILES) { + iglobArgs.push('--iglob', fileName) + } + for (const dirName of dangerousDirectories) { + iglobArgs.push('--iglob', `**/${dirName}/**`) + } + // Git hooks always blocked in nested repos + iglobArgs.push('--iglob', '**/.git/hooks/**') + + // Git config conditionally blocked in nested repos + if (!allowGitConfig) { + iglobArgs.push('--iglob', '**/.git/config') + } + + // Single ripgrep call to find all dangerous paths in subdirectories + // Limit depth for performance - deeply nested dangerous files are rare + // and the security benefit doesn't justify the traversal cost + let matches: string[] = [] + try { + matches = await ripGrep( + [ + '--files', + '--hidden', + '--max-depth', + String(maxDepth), + ...iglobArgs, + '-g', + '!**/node_modules/**', + ], + root, + signal, + ripgrepConfig, ) - if (dirIndex !== -1) { - // For .git, we want hooks/ or config, not the whole .git dir - if (dirName === '.git') { - const gitDir = segments.slice(0, dirIndex + 1).join(path.sep) - if (match.includes('.git/hooks')) { - denyPaths.push(path.join(gitDir, 'hooks')) - } else if (match.includes('.git/config')) { - denyPaths.push(path.join(gitDir, 'config')) + } catch (error) { + logForDebugging(`[Sandbox] ripgrep scan failed: ${error}`) + } + + // Process matches + for (const match of matches) { + const absolutePath = path.resolve(root, match) + + // File inside a dangerous directory -> add the directory path + let foundDir = false + for (const dirName of [...dangerousDirectories, '.git']) { + const normalizedDirName = normalizeCaseForComparison(dirName) + const segments = absolutePath.split(path.sep) + const dirIndex = segments.findIndex( + s => normalizeCaseForComparison(s) === normalizedDirName, + ) + if (dirIndex !== -1) { + // For .git, we want hooks/ or config, not the whole .git dir + if (dirName === '.git') { + const gitDir = segments.slice(0, dirIndex + 1).join(path.sep) + if (match.includes('.git/hooks')) { + denyPaths.push(path.join(gitDir, 'hooks')) + } else if (match.includes('.git/config')) { + denyPaths.push(path.join(gitDir, 'config')) + } + } else { + denyPaths.push(segments.slice(0, dirIndex + 1).join(path.sep)) } - } else { - denyPaths.push(segments.slice(0, dirIndex + 1).join(path.sep)) + foundDir = true + break } - foundDir = true - break } - } - // Dangerous file match - if (!foundDir) { - denyPaths.push(absolutePath) + // Dangerous file match + if (!foundDir) { + denyPaths.push(absolutePath) + } } } @@ -1164,12 +1181,22 @@ async function generateFilesystemArgs( // handling is needed 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), ) + // Collect all roots to scan for mandatory deny paths (CWD + all allowedWrite paths) + const scanRoots = [ + process.cwd(), + ...allowedWritePaths.filter(p => { + try { + return fs.statSync(p).isDirectory() + } catch { + return false + } + }), + ] + // Deny writes within allowed paths (user-specified + mandatory denies) const denyPaths = [ ...(writeConfig.denyWithinAllow || []), @@ -1178,6 +1205,7 @@ async function generateFilesystemArgs( mandatoryDenySearchDepth, allowGitConfig, abortSignal, + scanRoots, )), ] diff --git a/src/sandbox/macos-sandbox-utils.ts b/src/sandbox/macos-sandbox-utils.ts index 2cc6e5f9..7b739f60 100644 --- a/src/sandbox/macos-sandbox-utils.ts +++ b/src/sandbox/macos-sandbox-utils.ts @@ -1,6 +1,7 @@ import { quote } from '../utils/shell-quote.js' import { spawn } from 'child_process' import * as path from 'path' +import * as fs from 'fs' import { logForDebugging } from '../utils/debug.js' import { whichSync } from '../utils/which.js' import { buildJavaToolOptions } from './java-proxy-agent.js' @@ -76,29 +77,60 @@ export interface MacOSSandboxParams { * Get mandatory deny patterns as glob patterns (no filesystem scanning). * macOS sandbox profile supports regex/glob matching directly via globToRegex(). */ -export function macGetMandatoryDenyPatterns(allowGitConfig = false): string[] { - const cwd = process.cwd() +export function macGetMandatoryDenyPatterns( + allowGitConfig = false, + scanRoots: string[] = [process.cwd()], +): string[] { + const deduplicatedRoots = [ + ...new Set(scanRoots.map(r => path.resolve(r))), + ].filter(r => { + try { + return fs.existsSync(r) && fs.statSync(r).isDirectory() + } catch { + return false + } + }) + if (deduplicatedRoots.length === 0) { + deduplicatedRoots.push(process.cwd()) + } + const denyPaths: string[] = [] - // Dangerous files - static paths in CWD + glob patterns for subtree + for (const root of deduplicatedRoots) { + // Dangerous files - static paths in root + glob patterns for subtree + for (const fileName of DANGEROUS_FILES) { + denyPaths.push(path.resolve(root, fileName)) + denyPaths.push(`${root}/**/${fileName}`) + } + + // Dangerous directories + for (const dirName of getDangerousDirectories()) { + denyPaths.push(path.resolve(root, dirName)) + denyPaths.push(`${root}/**/${dirName}/**`) + } + + // Git hooks are always blocked for security + denyPaths.push(path.resolve(root, '.git/hooks')) + denyPaths.push(`${root}/**/.git/hooks`) + denyPaths.push(`${root}/**/.git/hooks/**`) + + // Git config - conditionally blocked based on allowGitConfig setting + if (!allowGitConfig) { + denyPaths.push(path.resolve(root, '.git/config')) + denyPaths.push(`${root}/**/.git/config`) + } + } + + // Also include generic unrooted globs for backward compatibility for (const fileName of DANGEROUS_FILES) { - denyPaths.push(path.resolve(cwd, fileName)) denyPaths.push(`**/${fileName}`) } - - // Dangerous directories for (const dirName of getDangerousDirectories()) { - denyPaths.push(path.resolve(cwd, dirName)) denyPaths.push(`**/${dirName}/**`) } - - // Git hooks are always blocked for security - denyPaths.push(path.resolve(cwd, '.git/hooks')) + denyPaths.push('**/.git/hooks') denyPaths.push('**/.git/hooks/**') - - // Git config - conditionally blocked based on allowGitConfig setting if (!allowGitConfig) { - denyPaths.push(path.resolve(cwd, '.git/config')) denyPaths.push('**/.git/config') } @@ -776,10 +808,23 @@ function generateWriteRules( } rules.push(...renderRule('allow', ['file-write*'], allowFilters, logTag)) - // Combine user-specified and mandatory deny patterns (no ripgrep needed on macOS) + // Combine user-specified and mandatory deny patterns across all allowed write roots + const normalizedAllowRoots = (config.allowOnly || []).map( + normalizePathForSandbox, + ) + const scanRoots = [ + process.cwd(), + ...normalizedAllowRoots.filter(p => { + try { + return fs.existsSync(p) && fs.statSync(p).isDirectory() + } catch { + return false + } + }), + ] const denyPaths = [ ...(config.denyWithinAllow || []), - ...macGetMandatoryDenyPatterns(allowGitConfig), + ...macGetMandatoryDenyPatterns(allowGitConfig, scanRoots), ] const { groups, rest: ungrouped } = groupLiteralDenyPaths( diff --git a/test/sandbox/mandatory-deny-paths.test.ts b/test/sandbox/mandatory-deny-paths.test.ts index e82ca64b..e15a5750 100644 --- a/test/sandbox/mandatory-deny-paths.test.ts +++ b/test/sandbox/mandatory-deny-paths.test.ts @@ -1130,6 +1130,55 @@ describe.if(isSupportedPlatform)( rmSync(targetDir, { recursive: true, force: true }) } }) + it('blocks writes to .git/hooks in allowedWrite paths outside process.cwd()', async () => { + const externalRepoDir = join( + tmpdir(), + `external-repo-${Date.now()}-${Math.random().toString(36).slice(2, 6)}`, + ) + const hooksDir = join(externalRepoDir, '.git', 'hooks') + mkdirSync(hooksDir, { recursive: true }) + const hookFile = join(hooksDir, 'pre-commit') + writeFileSync(hookFile, 'ORIGINAL') + + try { + const platform = getPlatform() + const command = `echo 'EXPLOIT' > '${hookFile}'` + const writeConfig = { + allowOnly: [externalRepoDir], + denyWithinAllow: [] as string[], + } + + let wrappedCommand: string + if (platform === 'macos') { + wrappedCommand = wrapCommandWithSandboxMacOS({ + command, + needsNetworkRestriction: false, + readConfig: undefined, + writeConfig, + }) + } else { + wrappedCommand = await wrapCommandWithSandboxLinux({ + command, + needsNetworkRestriction: false, + readConfig: undefined, + writeConfig, + }) + } + + const result = spawnSync(wrappedCommand, { + shell: true, + encoding: 'utf8', + timeout: 10000, + }) + + // Write should be denied + expect(result.status).not.toBe(0) + expect(readFileSync(hookFile, 'utf8')).toBe('ORIGINAL') + } finally { + cleanupBwrapMountPoints({ force: true }) + rmSync(externalRepoDir, { recursive: true, force: true }) + } + }) }, ) }, @@ -1180,4 +1229,14 @@ describe('macGetMandatoryDenyPatterns - Unit Tests', () => { ) expect(hasGitConfigPattern).toBe(true) }) + + it('includes static and glob deny patterns for multiple scanRoots', () => { + const extraRoot = tmpdir() + const patterns = macGetMandatoryDenyPatterns(false, [ + process.cwd(), + extraRoot, + ]) + + expect(patterns.some(p => p.includes(extraRoot))).toBe(true) + }) })