From dce5220241c3cc8574b14985682080ed9967feeb Mon Sep 17 00:00:00 2001 From: abhisek Date: Wed, 14 Jan 2026 21:50:04 +0530 Subject: [PATCH] fix: Non-existent path handling bug --- .../platform/bubblewrap_translator_linux.go | 41 +++++- .../bubblewrap_translator_linux_test.go | 129 ++++++++++++++---- 2 files changed, 137 insertions(+), 33 deletions(-) diff --git a/sandbox/platform/bubblewrap_translator_linux.go b/sandbox/platform/bubblewrap_translator_linux.go index 69a40d9..5383388 100644 --- a/sandbox/platform/bubblewrap_translator_linux.go +++ b/sandbox/platform/bubblewrap_translator_linux.go @@ -218,6 +218,13 @@ 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) allowGitConfig := utils.SafelyGetValue(policy.AllowGitConfig) denyPatterns := append([]string{}, policy.Filesystem.DenyWrite...) @@ -233,7 +240,7 @@ func (t *bubblewrapPolicyTranslator) translateFilesystem(policy *sandbox.Sandbox continue } - denyArgs, err := t.processDenyRule(expanded) + denyArgs, err := t.processDenyRule(expanded, allowedWritePaths) if err != nil { // Deny rules failing is not critical (file may not exist yet) log.Debugf("Deny rule '%s' skipped: %v", expanded, err) @@ -425,7 +432,8 @@ func (t *bubblewrapPolicyTranslator) processWriteRule(path string, boundPaths ma // processDenyRule handles deny rules by mounting /dev/null to prevent file creation. // This technique is borrowed from Anthropic's sandbox-runtime. -func (t *bubblewrapPolicyTranslator) processDenyRule(path string) ([]string, error) { +// allowedWritePaths is used to determine if non-existent paths need to be blocked. +func (t *bubblewrapPolicyTranslator) processDenyRule(path string, allowedWritePaths []string) ([]string, error) { args := []string{} // For glob patterns, expand and deny each path @@ -466,10 +474,18 @@ func (t *bubblewrapPolicyTranslator) processDenyRule(path string) ([]string, err args = append(args, "--ro-bind", "/dev/null", path) } } else if os.IsNotExist(err) { - // File doesn't exist - skip blocking it - // Rationale: bwrap cannot create mount points in read-only parent directories. - // Non-existent files are already protected by deny-by-default (not in allow_write). - log.Debugf("Deny rule: skipping non-existent path '%s' (already protected by deny-by-default)", path) + // 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) + } } } @@ -499,6 +515,19 @@ 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 8ef1244..1123ab9 100644 --- a/sandbox/platform/bubblewrap_translator_linux_test.go +++ b/sandbox/platform/bubblewrap_translator_linux_test.go @@ -480,40 +480,115 @@ func TestBubblewrapConfigEssentialDevices(t *testing.T) { } func TestBubblewrapTranslatorProcessDenyRule(t *testing.T) { - tmpDir := t.TempDir() + t.Run("existing file is blocked with /dev/null", func(t *testing.T) { + tmpDir := t.TempDir() - // Create a test file that exists - testFile := filepath.Join(tmpDir, "existing.txt") - require.NoError(t, os.WriteFile(testFile, []byte("test"), 0644)) + // Create a test file that exists + testFile := filepath.Join(tmpDir, "existing.txt") + require.NoError(t, os.WriteFile(testFile, []byte("test"), 0644)) - // Create a path that doesn't exist - nonExistentPath := filepath.Join(tmpDir, ".env") - - policy := &sandbox.SandboxPolicy{ - Filesystem: sandbox.FilesystemPolicy{ - DenyWrite: []string{ - testFile, // Existing file - nonExistentPath, // Non-existent file + policy := &sandbox.SandboxPolicy{ + Filesystem: sandbox.FilesystemPolicy{ + DenyWrite: []string{testFile}, }, - }, - } + } - config := newDefaultBubblewrapConfig() - translator := newBubblewrapPolicyTranslator(config) - args, err := translator.translate(policy) - require.NoError(t, err) + config := newDefaultBubblewrapConfig() + translator := newBubblewrapPolicyTranslator(config) + args, err := translator.translate(policy) + require.NoError(t, err) - argsStr := argSliceToString(args) + argsStr := argSliceToString(args) - // Existing file should be mounted with /dev/null - assert.Contains(t, argsStr, "--ro-bind") - assert.Contains(t, argsStr, "/dev/null") - assert.Contains(t, argsStr, testFile) + // Existing file should be mounted with /dev/null + assert.Contains(t, argsStr, "--ro-bind") + assert.Contains(t, argsStr, "/dev/null") + assert.Contains(t, argsStr, testFile) + }) - // Non-existent file should NOT be in args (protected by deny-by-default) - // In bubblewrap, paths that are not explicitly mounted are inaccessible, - // so we don't need to add deny rules for non-existent files - assert.NotContains(t, argsStr, nonExistentPath) + 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) { + tmpDir := t.TempDir() + + // Non-existent path WITHIN an allowed write path + nonExistentPath := filepath.Join(tmpDir, ".env") + + policy := &sandbox.SandboxPolicy{ + Filesystem: sandbox.FilesystemPolicy{ + AllowWrite: []string{tmpDir}, // Allow writes to tmpDir + DenyWrite: []string{nonExistentPath}, + }, + } + + config := newDefaultBubblewrapConfig() + translator := newBubblewrapPolicyTranslator(config) + args, err := translator.translate(policy) + require.NoError(t, err) + + 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) + }) + + t.Run("deeply nested non-existent path is blocked at first missing component", 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") + + policy := &sandbox.SandboxPolicy{ + Filesystem: sandbox.FilesystemPolicy{ + AllowWrite: []string{tmpDir}, // Allow writes to tmpDir + DenyWrite: []string{nonExistentPath}, + }, + } + + config := newDefaultBubblewrapConfig() + translator := newBubblewrapPolicyTranslator(config) + args, err := translator.translate(policy) + require.NoError(t, err) + + 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) + }) } func TestBubblewrapTranslatorTmpdirSupport(t *testing.T) {