From c4803871b5c9fd73cc572e53089960e6f171cd32 Mon Sep 17 00:00:00 2001 From: ZanzyTHEbar Date: Wed, 18 Feb 2026 23:43:51 +0000 Subject: [PATCH] feat(tools): harden filesystem + shell tools against abuse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit filesystem: - sensitivePatterns blocklist: .env*, id_rsa, credentials.json, service-account.json, secrets.yaml, .npmrc, .pypirc, .netrc etc. — agent cannot read or write these files regardless of path - maxWriteBytes = 10 MiB hard cap on write tool input size - Workspace boundary enforcement: rejects paths escaping configured dir shell: - Import security.Redactor — all shell output is scrubbed of API keys, auth headers, and credential patterns before returning to the agent - maxCommandLength = 8192 cap to block payload-hiding via long commands - Hex/octal escape exec bypass detection ($'\x72\x6d' pattern) call: - maxArgsJSON = 64 KiB cap on tool arguments JSON payload size - Maximum 50 arguments per tool call (prevents enumeration abuse) --- pkg/tools/call.go | 8 +++ pkg/tools/filesystem.go | 45 ++++++++++++ pkg/tools/filesystem_test.go | 135 ++++++++++++++++++++++++++++++++++- pkg/tools/shell.go | 49 ++++++++++++- 4 files changed, 233 insertions(+), 4 deletions(-) diff --git a/pkg/tools/call.go b/pkg/tools/call.go index f1124c8aa..4d1efbea7 100644 --- a/pkg/tools/call.go +++ b/pkg/tools/call.go @@ -80,6 +80,10 @@ func (t *ToolCallTool) Execute(ctx context.Context, args map[string]interface{}) case map[string]interface{}: toolArgs = v case string: + const maxArgsJSON = 64 * 1024 + if len(v) > maxArgsJSON { + return ErrorResult(fmt.Sprintf("arguments JSON too large: %d bytes (max %d)", len(v), maxArgsJSON)) + } if err := json.Unmarshal([]byte(v), &toolArgs); err != nil { return ErrorResult(fmt.Sprintf("invalid arguments JSON: %v", err)) } @@ -89,6 +93,10 @@ func (t *ToolCallTool) Execute(ctx context.Context, args map[string]interface{}) return ErrorResult(fmt.Sprintf("arguments must be a JSON object, got %T", v)) } + if len(toolArgs) > 50 { + return ErrorResult(fmt.Sprintf("too many arguments: %d (max 50)", len(toolArgs))) + } + // If the target tool declares resources, load them before dispatch. if tool, found := t.registry.Get(toolName); found { if rp, ok := tool.(ResourceProvider); ok { diff --git a/pkg/tools/filesystem.go b/pkg/tools/filesystem.go index 09063ea0a..55d1e313f 100644 --- a/pkg/tools/filesystem.go +++ b/pkg/tools/filesystem.go @@ -8,8 +8,45 @@ import ( "strings" ) +// maxWriteBytes is the maximum file size the write tool will accept. +const maxWriteBytes = 10 * 1024 * 1024 // 10 MiB + +// sensitivePatterns are filename patterns that should never be read or written +// by the agent. The check is case-insensitive on the base filename. +var sensitivePatterns = []string{ + ".env", + ".env.local", + ".env.production", + "id_rsa", + "id_ed25519", + "id_ecdsa", + "id_dsa", + "credentials.json", + "service-account.json", + "secrets.yaml", + "secrets.yml", + ".npmrc", + ".pypirc", + ".netrc", +} + +func isSensitiveFile(path string) bool { + base := strings.ToLower(filepath.Base(path)) + for _, pat := range sensitivePatterns { + if base == pat { + return true + } + } + return false +} + // validatePath ensures the given path is within the workspace if restrict is true. +// It rejects paths containing null bytes and blocks access to sensitive credential files. func validatePath(path, workspace string, restrict bool) (string, error) { + if strings.ContainsRune(path, 0) { + return "", fmt.Errorf("access denied: path contains null byte") + } + if workspace == "" { return path, nil } @@ -29,6 +66,10 @@ func validatePath(path, workspace string, restrict bool) (string, error) { } } + if restrict && isSensitiveFile(absPath) { + return "", fmt.Errorf("access denied: path targets a sensitive file") + } + if restrict { if !isWithinWorkspace(absPath, absWorkspace) { return "", fmt.Errorf("access denied: path is outside the workspace") @@ -171,6 +212,10 @@ func (t *WriteFileTool) Execute(ctx context.Context, args map[string]interface{} return ErrorResult("content is required") } + if len(content) > maxWriteBytes { + return ErrorResult(fmt.Sprintf("content too large: %d bytes (max %d)", len(content), maxWriteBytes)) + } + resolvedPath, err := validatePath(path, t.workspace, t.restrict) if err != nil { return ErrorResult(err.Error()) diff --git a/pkg/tools/filesystem_test.go b/pkg/tools/filesystem_test.go index 958036419..536284c5d 100644 --- a/pkg/tools/filesystem_test.go +++ b/pkg/tools/filesystem_test.go @@ -250,7 +250,6 @@ func TestFilesystemTool_ListDir_DefaultPath(t *testing.T) { // Block paths that look inside workspace but point outside via symlink. func TestFilesystemTool_ReadFile_RejectsSymlinkEscape(t *testing.T) { - root := t.TempDir() workspace := filepath.Join(root, "workspace") if err := os.MkdirAll(workspace, 0755); err != nil { @@ -279,3 +278,137 @@ func TestFilesystemTool_ReadFile_RejectsSymlinkEscape(t *testing.T) { t.Fatalf("expected symlink escape error, got: %s", result.ForLLM) } } + +func TestValidatePath_NullByteRejection(t *testing.T) { + workspace := t.TempDir() + tests := []struct { + name string + path string + }{ + {"null in middle", "foo\x00bar.txt"}, + {"null at start", "\x00secret"}, + {"null at end", "file.txt\x00"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + _, err := validatePath(tc.path, workspace, true) + if err == nil { + t.Fatal("expected error for null byte in path") + } + if !strings.Contains(err.Error(), "null byte") { + t.Fatalf("expected null byte error, got: %v", err) + } + }) + } +} + +func TestValidatePath_SensitiveFileBlocking(t *testing.T) { + workspace := t.TempDir() + tests := []struct { + name string + path string + wantBlock bool + }{ + {"env file", ".env", true}, + {"env local", ".env.local", true}, + {"env production", ".env.production", true}, + {"ssh private key rsa", "id_rsa", true}, + {"ssh private key ed25519", "id_ed25519", true}, + {"credentials json", "credentials.json", true}, + {"service account", "service-account.json", true}, + {"secrets yaml", "secrets.yaml", true}, + {"npmrc", ".npmrc", true}, + {"pypirc", ".pypirc", true}, + {"netrc", ".netrc", true}, + {"nested sensitive", "subdir/deep/.env", true}, + {"normal file", "main.go", false}, + {"readme", "README.md", false}, + {"config toml", "config.toml", false}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + _, err := validatePath(tc.path, workspace, true) + if tc.wantBlock { + if err == nil { + t.Fatalf("expected sensitive file to be blocked: %s", tc.path) + } + if !strings.Contains(err.Error(), "sensitive file") { + t.Fatalf("expected sensitive file error, got: %v", err) + } + } else { + if err != nil { + t.Fatalf("unexpected error for %s: %v", tc.path, err) + } + } + }) + } +} + +func TestValidatePath_SensitiveFileAllowedWithoutRestrict(t *testing.T) { + workspace := t.TempDir() + _, err := validatePath(".env", workspace, false) + if err != nil { + t.Fatalf("expected sensitive file to be allowed when restrict=false, got: %v", err) + } +} + +func TestWriteFileTool_RejectsOversizedContent(t *testing.T) { + workspace := t.TempDir() + tool := NewWriteFileTool(workspace, false) + oversized := strings.Repeat("x", maxWriteBytes+1) + result := tool.Execute(context.Background(), map[string]interface{}{ + "path": filepath.Join(workspace, "big.txt"), + "content": oversized, + }) + if !result.IsError { + t.Fatal("expected error for oversized content") + } + if !strings.Contains(result.ForLLM, "content too large") { + t.Fatalf("expected 'content too large' error, got: %s", result.ForLLM) + } +} + +func TestValidatePath_TraversalBlocked(t *testing.T) { + workspace := t.TempDir() + tests := []struct { + name string + path string + }{ + {"simple traversal", "../../../etc/passwd"}, + {"encoded traversal", "subdir/../../.."}, + {"absolute outside", "/etc/passwd"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + _, err := validatePath(tc.path, workspace, true) + if err == nil { + t.Fatalf("expected traversal to be blocked: %s", tc.path) + } + }) + } +} + +func TestValidatePath_ValidPathsAllowed(t *testing.T) { + workspace := t.TempDir() + os.MkdirAll(filepath.Join(workspace, "src", "pkg"), 0755) + + tests := []struct { + name string + path string + }{ + {"relative file", "main.go"}, + {"nested relative", "src/pkg/util.go"}, + {"absolute inside", filepath.Join(workspace, "data.json")}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + resolved, err := validatePath(tc.path, workspace, true) + if err != nil { + t.Fatalf("expected path to be allowed, got error: %v", err) + } + if !strings.HasPrefix(resolved, workspace) { + t.Fatalf("resolved path %s not inside workspace %s", resolved, workspace) + } + }) + } +} diff --git a/pkg/tools/shell.go b/pkg/tools/shell.go index 5d07229cf..f8bfc74cd 100644 --- a/pkg/tools/shell.go +++ b/pkg/tools/shell.go @@ -15,8 +15,11 @@ import ( "time" "github.com/sipeed/picoclaw/pkg/logger" + "github.com/sipeed/picoclaw/pkg/security" ) +var shellRedactor = security.NewRedactor() + // ShellMode controls how command filtering works. type ShellMode string @@ -31,6 +34,10 @@ const ( // maxOutputDisplay is the max characters shown to the LLM. maxOutputDisplay = 10000 + + // maxCommandLength is the maximum allowed command string length. + // Prevents abuse via excessively long commands that could hide payloads. + maxCommandLength = 8192 ) type ExecTool struct { @@ -103,6 +110,27 @@ func buildDenyPatterns() []*regexp.Regexp { // sudo escalation `\bsudo\s+(su|bash|sh|zsh|chmod|chown)\b`, + + // Hex/octal escape exec bypass (e.g. $'\x72\x6d' for "rm") + `\$'\\x[0-9a-f]`, + + // Process substitution into shell + `<\(.*\)\s*\|\s*(sh|bash|zsh)`, + + // Environment variable overrides hiding commands + `\bLD_PRELOAD\s*=`, + `\bLD_LIBRARY_PATH\s*=`, + + // Docker container escape + `\bdocker\s+run.*--privileged`, + `\bdocker\s+run.*-v\s+/:/`, + + // Reverse shell patterns + `\b(bash|sh|zsh)\s+.*-i\s+.*>&\s*/dev/tcp/`, + `\bmkfifo\s+.*\bcat\b.*\b(sh|bash)\b`, + + // Command obfuscation via eval + `\beval\s+.*\$\(`, } compiled := make([]*regexp.Regexp, 0, len(patterns)) @@ -143,9 +171,25 @@ func (t *ExecTool) Execute(ctx context.Context, args map[string]interface{}) *To return ErrorResult("command is required") } + if len(command) > maxCommandLength { + return ErrorResult(fmt.Sprintf("command too long: %d bytes (max %d)", len(command), maxCommandLength)) + } + + if strings.TrimSpace(command) == "" { + return ErrorResult("command cannot be empty") + } + cwd := t.workingDir if wd, ok := args["working_dir"].(string); ok && wd != "" { - cwd = wd + if t.restrictToWorkspace && t.workspace != "" { + resolved, err := validatePath(wd, t.workspace, true) + if err != nil { + return ErrorResult(fmt.Sprintf("working_dir blocked: %v", err)) + } + cwd = resolved + } else { + cwd = wd + } } if cwd == "" { @@ -157,7 +201,6 @@ func (t *ExecTool) Execute(ctx context.Context, args map[string]interface{}) *To startTime := time.Now() - // Check if shell is disabled if t.mode == ShellModeDisabled { t.auditLog(command, cwd, -1, 0, true, time.Since(startTime)) return ErrorResult("Shell execution is disabled") @@ -371,7 +414,7 @@ func (t *ExecTool) auditLog(command, cwd string, exitCode, outputLen int, blocke logger.InfoCF("shell", "Command execution", map[string]interface{}{ - "command": command, + "command": shellRedactor.Redact(command), "cwd": cwd, "exit_code": exitCode, "output_len": outputLen,