diff --git a/sandbox/platform/bubblewrap_translator_linux.go b/sandbox/platform/bubblewrap_translator_linux.go index 1facf96..018e459 100644 --- a/sandbox/platform/bubblewrap_translator_linux.go +++ b/sandbox/platform/bubblewrap_translator_linux.go @@ -224,14 +224,7 @@ func (t *bubblewrapPolicyTranslator) translateFilesystem(policy *sandbox.Sandbox args = append(args, writeArgs...) } - // Collect allowed write paths for deny rule processing - // This is used to determine if non-existent deny paths need to be blocked - allowedWritePaths := make([]string, 0, len(writeBoundPaths)) - for path := range writeBoundPaths { - allowedWritePaths = append(allowedWritePaths, path) - } - - // 3. Process deny_write rules (mount /dev/null to prevent creation) + // 3. Process deny_write rules (mount /dev/null to prevent access) allowGitConfig := utils.SafelyGetValue(policy.AllowGitConfig) denyPatterns := append([]string{}, policy.Filesystem.DenyWrite...) @@ -246,7 +239,7 @@ func (t *bubblewrapPolicyTranslator) translateFilesystem(policy *sandbox.Sandbox continue } - denyArgs, err := t.processDenyRule(expanded, allowedWritePaths) + denyArgs, err := t.processDenyRule(expanded) if err != nil { log.Debugf("Deny rule '%s' skipped: %v", expanded, err) continue @@ -437,10 +430,9 @@ func (t *bubblewrapPolicyTranslator) processWriteRule(path string, boundPaths ma return args, nil } -// processDenyRule handles deny rules by mounting /dev/null to prevent file creation. +// processDenyRule handles deny rules by mounting /dev/null to prevent file access. // This technique is borrowed from Anthropic's sandbox-runtime. -// allowedWritePaths is used to determine if non-existent paths need to be blocked. -func (t *bubblewrapPolicyTranslator) processDenyRule(path string, allowedWritePaths []string) ([]string, error) { +func (t *bubblewrapPolicyTranslator) processDenyRule(path string) ([]string, error) { args := []string{} // For glob patterns, expand and deny each path @@ -481,18 +473,14 @@ func (t *bubblewrapPolicyTranslator) processDenyRule(path string, allowedWritePa args = append(args, "--ro-bind", "/dev/null", path) } } else if os.IsNotExist(err) { - // File doesn't exist - check if it's within an allowed write path - // If so, we need to block creation by mounting /dev/null at first non-existent component - if isWithinAllowedWritePath(path, allowedWritePaths) { - firstNonExistent := t.findFirstNonExistentPath(path) - if firstNonExistent != "" { - args = append(args, "--ro-bind", "/dev/null", firstNonExistent) - log.Debugf("Deny rule: blocking creation of '%s' by mounting /dev/null at '%s'", path, firstNonExistent) - } - } else { - // Not within allowed write paths - already protected by deny-by-default - log.Debugf("Deny rule: skipping '%s' - not within allowed write paths (protected by deny-by-default)", path) - } + // File doesn't exist - skip it + // IMPORTANT: We cannot use --ro-bind /dev/null for non-existent paths because + // bwrap creates the file on the host filesystem as a mount point, which leaves + // empty files (.env, .aws, etc.) in the user's directory after sandbox exits. + // Non-existent files are harmless (no secrets to leak), and blocking creation + // in writable directories isn't critical since an attacker creating an empty + // .env is not a security threat. + log.Debugf("Deny rule: skipping non-existent path '%s' (bwrap would create empty file as mount point)", path) } } @@ -522,19 +510,6 @@ func (t *bubblewrapPolicyTranslator) findFirstNonExistentPath(path string) strin return "" } -// isWithinAllowedWritePath checks if a target path is within any of the allowed write paths. -// This is used to determine if a non-existent deny path needs to be blocked. -func isWithinAllowedWritePath(targetPath string, allowedWritePaths []string) bool { - targetPath = filepath.Clean(targetPath) - for _, allowedPath := range allowedWritePaths { - allowedPath = filepath.Clean(allowedPath) - if targetPath == allowedPath || strings.HasPrefix(targetPath, allowedPath+"/") { - return true - } - } - return false -} - // expandGlobPattern expands a glob pattern to a list of concrete paths. // Implements depth limiting and path count limiting to prevent DoS. // Returns (paths, useFallback, error) where useFallback indicates if diff --git a/sandbox/platform/bubblewrap_translator_linux_test.go b/sandbox/platform/bubblewrap_translator_linux_test.go index 1123ab9..dca942b 100644 --- a/sandbox/platform/bubblewrap_translator_linux_test.go +++ b/sandbox/platform/bubblewrap_translator_linux_test.go @@ -506,35 +506,11 @@ func TestBubblewrapTranslatorProcessDenyRule(t *testing.T) { assert.Contains(t, argsStr, testFile) }) - t.Run("non-existent file outside allowed write paths is skipped", func(t *testing.T) { - // Use a path that is truly outside any allowed write path - // Note: /tmp is always in allowedWritePaths due to tmpdir support - // So we use a path under /nonexistent which doesn't exist - nonExistentPath := "/nonexistent/completely/fake/path/.env" - - policy := &sandbox.SandboxPolicy{ - Filesystem: sandbox.FilesystemPolicy{ - DenyWrite: []string{nonExistentPath}, - // No AllowWrite - path is not within any allowed write area - }, - } - - config := newDefaultBubblewrapConfig() - translator := newBubblewrapPolicyTranslator(config) - args, err := translator.translate(policy) - require.NoError(t, err) - - argsStr := argSliceToString(args) - - // Non-existent file should NOT be in args (protected by deny-by-default) - // Path is not within allowed write paths, so no need to block it - assert.NotContains(t, argsStr, nonExistentPath) - }) - - t.Run("non-existent file within allowed write path is blocked", func(t *testing.T) { + t.Run("non-existent file is skipped to avoid creating empty files", func(t *testing.T) { tmpDir := t.TempDir() - // Non-existent path WITHIN an allowed write path + // Non-existent file - should be skipped because using --ro-bind /dev/null + // on non-existent paths causes bwrap to create the file as a mount point nonExistentPath := filepath.Join(tmpDir, ".env") policy := &sandbox.SandboxPolicy{ @@ -551,29 +527,21 @@ func TestBubblewrapTranslatorProcessDenyRule(t *testing.T) { argsStr := argSliceToString(args) - // Non-existent file within allowed write path should be blocked - // by mounting /dev/null at the first non-existent component - assert.Contains(t, argsStr, "--ro-bind") - assert.Contains(t, argsStr, "/dev/null") - assert.Contains(t, argsStr, nonExistentPath) + // Non-existent file should NOT be in args + // Using --ro-bind /dev/null on non-existent paths creates empty files + assert.NotContains(t, argsStr, nonExistentPath) }) - t.Run("deeply nested non-existent path is blocked at first missing component", func(t *testing.T) { + t.Run("existing directory is mounted read-only", func(t *testing.T) { tmpDir := t.TempDir() - // Create a subdirectory that exists - existingDir := filepath.Join(tmpDir, "existing") - require.NoError(t, os.MkdirAll(existingDir, 0755)) - - // Deeply nested non-existent path: existing/nonexistent/deep/.env - // First non-existent component is "nonexistent" - nonExistentPath := filepath.Join(existingDir, "nonexistent", "deep", ".env") - firstNonExistent := filepath.Join(existingDir, "nonexistent") + // Create a directory + testDir := filepath.Join(tmpDir, "secrets") + require.NoError(t, os.MkdirAll(testDir, 0755)) policy := &sandbox.SandboxPolicy{ Filesystem: sandbox.FilesystemPolicy{ - AllowWrite: []string{tmpDir}, // Allow writes to tmpDir - DenyWrite: []string{nonExistentPath}, + DenyWrite: []string{testDir}, }, } @@ -584,10 +552,9 @@ func TestBubblewrapTranslatorProcessDenyRule(t *testing.T) { argsStr := argSliceToString(args) - // Should mount /dev/null at the first non-existent component - assert.Contains(t, argsStr, "--ro-bind") - assert.Contains(t, argsStr, "/dev/null") - assert.Contains(t, argsStr, firstNonExistent) + // Existing directory should be mounted read-only + assert.Contains(t, argsStr, "--ro-bind-try") + assert.Contains(t, argsStr, testDir) }) }