Filesystem allow-list bypass via naive prefix check in isPathAllowed

[jeanlaurent]

Summary

• Context: The isPathAllowed function in pkg/tools/builtin/filesystem.go enforces filesystem access control by checking if file paths are within allowed directories.
• Bug: The function uses a simple string prefix check (strings.HasPrefix) to validate paths, which incorrectly allows access to directories with similar name prefixes.
• Actual vs. expected: A path like /home/user/project-secrets/file.txt is incorrectly allowed when only /home/user/project is in the allowed directories list, because the former string starts with the latter. The expected behavior is to only allow paths that are actually within the allowed directory hierarchy.
• Impact: An agent or user could bypass filesystem access restrictions to read, write, edit, or search files in directories that should be forbidden, leading to unauthorized access to sensitive files.

Code with bug

func (t *FilesystemTool) isPathAllowed(path string) error {
	absPath, err := filepath.Abs(path)
	if err != nil {
		return fmt.Errorf("unable to resolve absolute path: %w", err)
	}

	if len(t.allowedDirectories) == 0 {
		return fmt.Errorf("no allowed directories configured")
	}

	for _, allowedDir := range t.allowedDirectories {
		allowedAbs, err := filepath.Abs(allowedDir)
		if err != nil {
			continue
		}

		if strings.HasPrefix(absPath, allowedAbs) {  // <-- BUG 🔴 Uses simple string prefix without path separator check
			return nil
		}
	}

	return fmt.Errorf("path %s is not within allowed directories", path)
}

Evidence

Example

Consider the following scenario:

1. Allowed directory: /home/user/project
2. Attempted access path: /home/user/project-secrets/confidential.txt

Current buggy behavior:

• absPath = /home/user/project-secrets/confidential.txt
• allowedAbs = /home/user/project
• strings.HasPrefix("/home/user/project-secrets/confidential.txt", "/home/user/project") returns true
• Access is incorrectly granted

Expected behavior:

• The path /home/user/project-secrets/confidential.txt should be denied because /home/user/project-secrets is a sibling directory to /home/user/project, not a subdirectory of it.
• The check should verify that either:
  • The paths are equal, OR
  • The checked path starts with the allowed path followed by a path separator

Additional examples where the bug manifests:

• Allowed: /home/user/project
  • /home/user/project2/file.txt → incorrectly allowed (should be denied)
  • /home/user/project_backup/file.txt → incorrectly allowed (should be denied)
  • /home/user/projectX/file.txt → incorrectly allowed (should be denied)

Failing test

Test script

package builtin

import (
	"context"
	"os"
	"path/filepath"
	"strings"
	"testing"
)

// TestPathAllowedPrefixBug tests for a bug where paths with similar prefixes
// are incorrectly allowed due to simple string prefix matching
func TestPathAllowedPrefixBug(t *testing.T) {
	// Create test directories
	tmpDir := t.TempDir()

	// Create two directories with similar names
	allowedDir := filepath.Join(tmpDir, "project")
	similarDir := filepath.Join(tmpDir, "project-secrets")

	if err := os.MkdirAll(allowedDir, 0755); err != nil {
		t.Fatalf("Failed to create allowed dir: %v", err)
	}

	if err := os.MkdirAll(similarDir, 0755); err != nil {
		t.Fatalf("Failed to create similar dir: %v", err)
	}

	// Create test files
	allowedFile := filepath.Join(allowedDir, "allowed.txt")
	secretFile := filepath.Join(similarDir, "secret.txt")

	if err := os.WriteFile(allowedFile, []byte("This should be accessible"), 0644); err != nil {
		t.Fatalf("Failed to create allowed file: %v", err)
	}

	if err := os.WriteFile(secretFile, []byte("This should NOT be accessible"), 0644); err != nil {
		t.Fatalf("Failed to create secret file: %v", err)
	}

	// Create filesystem tool with only the "project" directory allowed
	fsTool := NewFilesystemTool([]string{allowedDir})

	// Test 1: Access file in allowed directory (should succeed)
	t.Run("AccessAllowedFile", func(t *testing.T) {
		err := fsTool.isPathAllowed(allowedFile)
		if err != nil {
			t.Errorf("Expected access to allowed file to succeed, got error: %v", err)
		}
	})

	// Test 2: Access file in directory with similar name (should fail but currently succeeds due to bug)
	t.Run("AccessSimilarNamedDirectory", func(t *testing.T) {
		err := fsTool.isPathAllowed(secretFile)
		if err == nil {
			t.Errorf("BUG DETECTED: Access to file in similar-named directory should be denied, but was allowed!")
			t.Logf("Allowed directory: %s", allowedDir)
			t.Logf("Secret file path: %s", secretFile)

			// Show the buggy logic
			allowedAbs, _ := filepath.Abs(allowedDir)
			secretAbs, _ := filepath.Abs(secretFile)
			t.Logf("strings.HasPrefix(%q, %q) = %v", secretAbs, allowedAbs, strings.HasPrefix(secretAbs, allowedAbs))
		} else {
			t.Logf("Access correctly denied (bug may be fixed): %v", err)
		}
	})

	// Test 3: Try to read the secret file using the tool's handler
	t.Run("ReadSecretFileViaHandler", func(t *testing.T) {
		args := ReadFileArgs{Path: secretFile}
		result, err := fsTool.handleReadFile(context.Background(), args)

		if err != nil {
			t.Fatalf("Handler returned unexpected error: %v", err)
		}

		// Check if the result contains an error message or the actual content
		if !strings.Contains(result.Output, "Error:") {
			t.Errorf("BUG DETECTED: Successfully read secret file that should have been blocked!")
			t.Logf("Result output: %s", result.Output)
		} else {
			t.Logf("Access correctly denied in handler: %s", result.Output)
		}
	})
}

// TestPathAllowedEdgeCases tests various edge cases for path validation
func TestPathAllowedEdgeCases(t *testing.T) {
	tmpDir := t.TempDir()

	testCases := []struct {
		name          string
		allowedDir    string
		testPath      string
		shouldSucceed bool
		description   string
	}{
		{
			name:          "ExactMatch",
			allowedDir:    filepath.Join(tmpDir, "project"),
			testPath:      filepath.Join(tmpDir, "project"),
			shouldSucceed: true,
			description:   "Exact match of allowed directory",
		},
		{
			name:          "ChildFile",
			allowedDir:    filepath.Join(tmpDir, "project"),
			testPath:      filepath.Join(tmpDir, "project", "file.txt"),
			shouldSucceed: true,
			description:   "File inside allowed directory",
		},
		{
			name:          "SimilarPrefix",
			allowedDir:    filepath.Join(tmpDir, "project"),
			testPath:      filepath.Join(tmpDir, "project-secrets", "file.txt"),
			shouldSucceed: false,
			description:   "Directory with similar prefix should be denied",
		},
		{
			name:          "SimilarPrefixNoSeparator",
			allowedDir:    filepath.Join(tmpDir, "project"),
			testPath:      filepath.Join(tmpDir, "project2", "file.txt"),
			shouldSucceed: false,
			description:   "Directory with similar prefix (alphanumeric) should be denied",
		},
		{
			name:          "ParentDirectory",
			allowedDir:    filepath.Join(tmpDir, "project", "subdir"),
			testPath:      filepath.Join(tmpDir, "project", "file.txt"),
			shouldSucceed: false,
			description:   "Parent of allowed directory should be denied",
		},
	}

	for _, tc := range testCases {
		t.Run(tc.name, func(t *testing.T) {
			// Create directories
			if err := os.MkdirAll(tc.allowedDir, 0755); err != nil {
				t.Fatalf("Failed to create allowed dir: %v", err)
			}
			if err := os.MkdirAll(filepath.Dir(tc.testPath), 0755); err != nil {
				t.Fatalf("Failed to create test path dir: %v", err)
			}

			// Create file if it doesn't exist
			if filepath.Ext(tc.testPath) != "" {
				if err := os.WriteFile(tc.testPath, []byte("test"), 0644); err != nil {
					t.Fatalf("Failed to create test file: %v", err)
				}
			}

			fsTool := NewFilesystemTool([]string{tc.allowedDir})
			err := fsTool.isPathAllowed(tc.testPath)

			if tc.shouldSucceed && err != nil {
				t.Errorf("%s: Expected access to succeed but got error: %v", tc.description, err)
			} else if !tc.shouldSucceed && err == nil {
				t.Errorf("%s: Expected access to be denied but it was allowed! (BUG)", tc.description)
				allowedAbs, _ := filepath.Abs(tc.allowedDir)
				testAbs, _ := filepath.Abs(tc.testPath)
				t.Logf("Allowed: %q, Test: %q, HasPrefix: %v", allowedAbs, testAbs, strings.HasPrefix(testAbs, allowedAbs))
			}
		})
	}
}

Test output

=== RUN   TestPathAllowedPrefixBug
=== RUN   TestPathAllowedPrefixBug/AccessAllowedFile
=== RUN   TestPathAllowedPrefixBug/AccessSimilarNamedDirectory
    filesystem_pathcheck_test.go:56: BUG DETECTED: Access to file in similar-named directory should be denied, but was allowed!
    filesystem_pathcheck_test.go:57: Allowed directory: /tmp/TestPathAllowedPrefixBug3131898510/001/project
    filesystem_pathcheck_test.go:58: Secret file path: /tmp/TestPathAllowedPrefixBug3131898510/001/project-secrets/secret.txt
    filesystem_pathcheck_test.go:63: strings.HasPrefix("/tmp/TestPathAllowedPrefixBug3131898510/001/project-secrets/secret.txt", "/tmp/TestPathAllowedPrefixBug3131898510/001/project") = true
=== RUN   TestPathAllowedPrefixBug/ReadSecretFileViaHandler
    filesystem_pathcheck_test.go:80: BUG DETECTED: Successfully read secret file that should have been blocked!
    filesystem_pathcheck_test.go:81: Result output: This should NOT be accessible
--- FAIL: TestPathAllowedPrefixBug (0.00s)
    --- PASS: TestPathAllowedPrefixBug/AccessAllowedFile (0.00s)
    --- FAIL: TestPathAllowedPrefixBug/AccessSimilarNamedDirectory (0.00s)
    --- FAIL: TestPathAllowedPrefixBug/ReadSecretFileViaHandler (0.00s)
FAIL
FAIL	github.com/docker/cagent	0.012s
FAIL

The edge case tests also confirm multiple failure scenarios:

=== RUN   TestPathAllowedEdgeCases
=== RUN   TestPathAllowedEdgeCases/ExactMatch
=== RUN   TestPathAllowedEdgeCases/ChildFile
=== RUN   TestPathAllowedEdgeCases/SimilarPrefix
    filesystem_pathcheck_test.go:159: Directory with similar prefix should be denied: Expected access to be denied but it was allowed! (BUG)
    filesystem_pathcheck_test.go:162: Allowed: "/tmp/TestPathAllowedEdgeCases3084968122/001/project", Test: "/tmp/TestPathAllowedEdgeCases3084968122/001/project-secrets/file.txt", HasPrefix: true
=== RUN   TestPathAllowedEdgeCases/SimilarPrefixNoSeparator
    filesystem_pathcheck_test.go:159: Directory with similar prefix (alphanumeric) should be denied: Expected access to be denied but it was allowed! (BUG)
    filesystem_pathcheck_test.go:162: Allowed: "/tmp/TestPathAllowedEdgeCases3084968122/001/project", Test: "/tmp/TestPathAllowedEdgeCases3084968122/001/project2/file.txt", HasPrefix: true
=== RUN   TestPathAllowedEdgeCases/ParentDirectory
--- FAIL: TestPathAllowedEdgeCases (0.00s)
    --- PASS: TestPathAllowedEdgeCases/ExactMatch (0.00s)
    --- PASS: TestPathAllowedEdgeCases/ChildFile (0.00s)
    --- FAIL: TestPathAllowedEdgeCases/SimilarPrefix (0.00s)
    --- FAIL: TestPathAllowedEdgeCases/SimilarPrefixNoSeparator (0.00s)
    --- PASS: TestPathAllowedEdgeCases/ParentDirectory (0.00s)
FAIL
FAIL	github.com/docker/cagent	0.016s
FAIL

Full context

The FilesystemTool is a core security component in the cagent system that provides filesystem access to AI agents with built-in restrictions. The isPathAllowed function is the central gatekeeper that validates all filesystem operations (read, write, edit, search, list, etc.) to ensure they only access permitted directories.

This tool is instantiated throughout the codebase:

• In pkg/creator/agent.go: Creates filesystem tools with the working directory as the allowed directory
• In pkg/teamloader/registry.go: Creates filesystem tools for team agents
• In pkg/acp/filesystem.go: Creates ACP-specific filesystem toolsets

The function is called by every filesystem operation handler:

• handleReadFile - Reading file contents
• handleReadMultipleFiles - Reading multiple files
• handleWriteFile - Writing/creating files
• handleEditFile - Editing existing files
• handleListDirectory - Listing directory contents
• handleSearchFiles - Searching for files by pattern
• handleSearchFilesContent - Searching file contents
• handleDirectoryTree - Building directory trees

The security model, as documented in the tool's instructions, states:

• "All operations are restricted to allowed directories only"
• "Subdirectories within allowed directories are accessible"
• Users can request additional directory access via add_allowed_directory

The bug undermines this security model by allowing access to directories that happen to share a string prefix with allowed directories, even though they are not subdirectories.

Why has this bug gone undetected?

1. Requires specific directory naming: The bug only manifests when sibling directories exist with names that share a prefix with an allowed directory (e.g., project and project-secrets, or data and data-backup).

2. Common cases work correctly: The most common use case - accessing files within subdirectories of the allowed directory - works correctly because those paths naturally have the allowed directory as a true prefix followed by a path separator.

3. Typical usage patterns: In development environments, directories are often uniquely named without such prefix collisions. Developers might have directories like src, tests, docs which don't share prefixes.

4. Limited production testing: Since the tool is typically configured with the working directory as the allowed directory, and most operations happen within the project structure, the bug may not surface in normal usage.

5. No comprehensive path validation tests: The existing test suite in pkg/tools/builtin/filesystem_test.go does not include test cases for sibling directories with similar name prefixes.

6. Privilege escalation is subtle: When the bug does trigger, the access is granted silently - there's no error or warning that would alert users to the security bypass.

Recommended fix

Replace the simple string prefix check with a proper path hierarchy check that ensures a path separator exists after the allowed directory prefix (or that the paths are equal):

func (t *FilesystemTool) isPathAllowed(path string) error {
	absPath, err := filepath.Abs(path)
	if err != nil {
		return fmt.Errorf("unable to resolve absolute path: %w", err)
	}

	if len(t.allowedDirectories) == 0 {
		return fmt.Errorf("no allowed directories configured")
	}

	for _, allowedDir := range t.allowedDirectories {
		allowedAbs, err := filepath.Abs(allowedDir)
		if err != nil {
			continue
		}

		// Check if paths are equal or if absPath is within allowedAbs
		if absPath == allowedAbs {
			return nil
		}

		// Ensure allowedAbs ends with separator for proper prefix matching
		allowedWithSep := allowedAbs
		if !strings.HasSuffix(allowedWithSep, string(filepath.Separator)) {
			allowedWithSep += string(filepath.Separator)
		}

		if strings.HasPrefix(absPath, allowedWithSep) {  // <-- FIX 🟢 Now requires separator after allowed dir
			return nil
		}
	}

	return fmt.Errorf("path %s is not within allowed directories", path)
}

Alternatively, use filepath.Rel to check if the relative path doesn't escape the allowed directory:

// Check if testPath is within allowedAbs
relPath, err := filepath.Rel(allowedAbs, absPath)
if err == nil && !strings.HasPrefix(relPath, "..") {  // <-- FIX 🟢 Alternative approach
	return nil
}