diff --git a/README.md b/README.md index d91cfb750..eef793bdd 100644 --- a/README.md +++ b/README.md @@ -525,6 +525,36 @@ Even with `restrict_to_workspace: false`, the `exec` tool blocks these dangerous * `shutdown`, `reboot`, `poweroff` — System shutdown * Fork bomb `:(){ :|:& };:` +#### Shell Escape Sequence Protection + +When `restrict_to_workspace: true`, the `exec` tool also blocks shell escape sequences that can bypass metacharacter detection: + +| Pattern | Example | Risk | +|---------|---------|------| +| ANSI-C quoting `$'...'` | `$'\x24(id)'` | Embeds command substitution via hex escape | +| Locale quoting `$"..."` | `$"$(cmd)"` | Alternative command substitution syntax | +| Hex escapes `\xNN` | `\x24(id)` | Encodes `$` as `\x24` to bypass `$()` check | +| Octal escapes `\NNN` | `\060` | Encodes characters via octal to bypass checks | +| Escaped metacharacters | `` \` ``, `\$` | Bypasses backtick and dollar sign detection | + +#### Working Directory Validation + +When `restrict_to_workspace: true`, the `exec` tool validates the `working_dir` parameter to ensure it stays within the configured workspace. Passing `working_dir` pointing outside the workspace (e.g. `/etc`) is blocked: + +``` +Command blocked by safety guard (working directory outside workspace) +``` + +#### Symlink TOCTOU Protection + +All file tools (`read_file`, `write_file`, `edit_file`, `append_file`) re-verify symlink targets immediately before the actual I/O operation. This closes the time-of-check-to-time-of-use (TOCTOU) window where an attacker could swap a symlink between the initial `validatePath()` check and the subsequent file operation: + +1. **Check**: `validatePath()` resolves the symlink and verifies the target is inside the workspace +2. **Re-check**: `safeReadFile` / `safeWriteFile` / `safeOpenFile` calls `Lstat` right before I/O — if the path is a symlink, it re-resolves and re-validates the target +3. **Operate**: The file operation uses the resolved path + +If the symlink target has changed to a path outside the workspace between steps 1 and 2, the operation is denied. + #### SSRF Protection (Web Fetch) The `web_fetch` tool blocks requests to internal and private network addresses to prevent Server-Side Request Forgery (SSRF) attacks. This protects against unauthorized access to cloud metadata endpoints (e.g. `169.254.169.254`), internal services, and local resources. diff --git a/pkg/tools/edit.go b/pkg/tools/edit.go index 1e7c33b45..98c30d31d 100644 --- a/pkg/tools/edit.go +++ b/pkg/tools/edit.go @@ -76,7 +76,7 @@ func (t *EditFileTool) Execute(ctx context.Context, args map[string]interface{}) return ErrorResult(fmt.Sprintf("file not found: %s", path)) } - content, err := os.ReadFile(resolvedPath) + content, err := safeReadFile(resolvedPath, t.allowedDir, t.restrict) if err != nil { return ErrorResult(fmt.Sprintf("failed to read file: %v", err)) } @@ -94,7 +94,7 @@ func (t *EditFileTool) Execute(ctx context.Context, args map[string]interface{}) newContent := strings.Replace(contentStr, oldText, newText, 1) - if err := os.WriteFile(resolvedPath, []byte(newContent), 0644); err != nil { + if err := safeWriteFile(resolvedPath, []byte(newContent), 0644, t.allowedDir, t.restrict); err != nil { return ErrorResult(fmt.Sprintf("failed to write file: %v", err)) } @@ -151,7 +151,7 @@ func (t *AppendFileTool) Execute(ctx context.Context, args map[string]interface{ return ErrorResult(err.Error()) } - f, err := os.OpenFile(resolvedPath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644) + f, err := safeOpenFile(resolvedPath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644, t.workspace, t.restrict) if err != nil { return ErrorResult(fmt.Sprintf("failed to open file: %v", err)) } diff --git a/pkg/tools/filesystem.go b/pkg/tools/filesystem.go index 09063ea0a..8649b28b1 100644 --- a/pkg/tools/filesystem.go +++ b/pkg/tools/filesystem.go @@ -77,6 +77,71 @@ func isWithinWorkspace(candidate, workspace string) bool { return err == nil && rel != ".." && !strings.HasPrefix(rel, ".."+string(os.PathSeparator)) } +// recheckSymlink verifies that path does not resolve outside workspace via symlink. +// This is called right before the actual I/O operation to close the TOCTOU window +// between validatePath and the file operation. +func recheckSymlink(path, workspace string, restrict bool) (string, error) { + if !restrict || workspace == "" { + return path, nil + } + + info, err := os.Lstat(path) + if err != nil { + // File doesn't exist yet (e.g. new file write) — nothing to recheck + if os.IsNotExist(err) { + return path, nil + } + return "", fmt.Errorf("failed to stat path: %w", err) + } + + if info.Mode()&os.ModeSymlink != 0 { + resolved, err := filepath.EvalSymlinks(path) + if err != nil { + return "", fmt.Errorf("failed to resolve symlink: %w", err) + } + absWorkspace, err := filepath.Abs(workspace) + if err != nil { + return "", fmt.Errorf("failed to resolve workspace: %w", err) + } + if wsResolved, err := filepath.EvalSymlinks(absWorkspace); err == nil { + absWorkspace = wsResolved + } + if !isWithinWorkspace(resolved, absWorkspace) { + return "", fmt.Errorf("access denied: symlink resolves outside workspace") + } + return resolved, nil + } + + return path, nil +} + +// safeReadFile re-checks symlinks right before reading to prevent TOCTOU attacks. +func safeReadFile(path, workspace string, restrict bool) ([]byte, error) { + resolved, err := recheckSymlink(path, workspace, restrict) + if err != nil { + return nil, err + } + return os.ReadFile(resolved) +} + +// safeWriteFile re-checks symlinks right before writing to prevent TOCTOU attacks. +func safeWriteFile(path string, data []byte, perm os.FileMode, workspace string, restrict bool) error { + resolved, err := recheckSymlink(path, workspace, restrict) + if err != nil { + return err + } + return os.WriteFile(resolved, data, perm) +} + +// safeOpenFile re-checks symlinks right before opening to prevent TOCTOU attacks. +func safeOpenFile(path string, flag int, perm os.FileMode, workspace string, restrict bool) (*os.File, error) { + resolved, err := recheckSymlink(path, workspace, restrict) + if err != nil { + return nil, err + } + return os.OpenFile(resolved, flag, perm) +} + type ReadFileTool struct { workspace string restrict bool @@ -118,7 +183,7 @@ func (t *ReadFileTool) Execute(ctx context.Context, args map[string]interface{}) return ErrorResult(err.Error()) } - content, err := os.ReadFile(resolvedPath) + content, err := safeReadFile(resolvedPath, t.workspace, t.restrict) if err != nil { return ErrorResult(fmt.Sprintf("failed to read file: %v", err)) } @@ -181,7 +246,7 @@ func (t *WriteFileTool) Execute(ctx context.Context, args map[string]interface{} return ErrorResult(fmt.Sprintf("failed to create directory: %v", err)) } - if err := os.WriteFile(resolvedPath, []byte(content), 0644); err != nil { + if err := safeWriteFile(resolvedPath, []byte(content), 0644, t.workspace, t.restrict); err != nil { return ErrorResult(fmt.Sprintf("failed to write file: %v", err)) } diff --git a/pkg/tools/filesystem_test.go b/pkg/tools/filesystem_test.go index 958036419..91cf6afea 100644 --- a/pkg/tools/filesystem_test.go +++ b/pkg/tools/filesystem_test.go @@ -279,3 +279,115 @@ func TestFilesystemTool_ReadFile_RejectsSymlinkEscape(t *testing.T) { t.Fatalf("expected symlink escape error, got: %s", result.ForLLM) } } + +// TestFilesystemTool_WriteFile_RejectsSymlinkEscape verifies that writing via a symlink +// that points outside the workspace is blocked (TOCTOU protection). +func TestFilesystemTool_WriteFile_RejectsSymlinkEscape(t *testing.T) { + root := t.TempDir() + workspace := filepath.Join(root, "workspace") + if err := os.MkdirAll(workspace, 0755); err != nil { + t.Fatalf("failed to create workspace: %v", err) + } + + target := filepath.Join(root, "outside.txt") + if err := os.WriteFile(target, []byte("original"), 0644); err != nil { + t.Fatalf("failed to write target file: %v", err) + } + + link := filepath.Join(workspace, "link.txt") + if err := os.Symlink(target, link); err != nil { + t.Skipf("symlink not supported in this environment: %v", err) + } + + tool := NewWriteFileTool(workspace, true) + result := tool.Execute(context.Background(), map[string]interface{}{ + "path": link, + "content": "hacked", + }) + + if !result.IsError { + t.Fatalf("expected symlink escape to be blocked for write") + } + if !strings.Contains(result.ForLLM, "symlink resolves outside workspace") { + t.Fatalf("expected symlink escape error, got: %s", result.ForLLM) + } + + // Verify original content was not overwritten + content, _ := os.ReadFile(target) + if string(content) != "original" { + t.Fatalf("expected original content to be preserved, got: %s", string(content)) + } +} + +// TestFilesystemTool_EditFile_RejectsSymlinkEscape verifies that editing via a symlink +// that points outside the workspace is blocked (TOCTOU protection). +func TestFilesystemTool_EditFile_RejectsSymlinkEscape(t *testing.T) { + root := t.TempDir() + workspace := filepath.Join(root, "workspace") + if err := os.MkdirAll(workspace, 0755); err != nil { + t.Fatalf("failed to create workspace: %v", err) + } + + target := filepath.Join(root, "outside.txt") + if err := os.WriteFile(target, []byte("original content"), 0644); err != nil { + t.Fatalf("failed to write target file: %v", err) + } + + link := filepath.Join(workspace, "link.txt") + if err := os.Symlink(target, link); err != nil { + t.Skipf("symlink not supported in this environment: %v", err) + } + + tool := NewEditFileTool(workspace, true) + result := tool.Execute(context.Background(), map[string]interface{}{ + "path": link, + "old_text": "original", + "new_text": "hacked", + }) + + if !result.IsError { + t.Fatalf("expected symlink escape to be blocked for edit") + } + + // Verify original content was not modified + content, _ := os.ReadFile(target) + if string(content) != "original content" { + t.Fatalf("expected original content to be preserved, got: %s", string(content)) + } +} + +// TestFilesystemTool_AppendFile_RejectsSymlinkEscape verifies that appending via a symlink +// that points outside the workspace is blocked (TOCTOU protection). +func TestFilesystemTool_AppendFile_RejectsSymlinkEscape(t *testing.T) { + root := t.TempDir() + workspace := filepath.Join(root, "workspace") + if err := os.MkdirAll(workspace, 0755); err != nil { + t.Fatalf("failed to create workspace: %v", err) + } + + target := filepath.Join(root, "outside.txt") + if err := os.WriteFile(target, []byte("original"), 0644); err != nil { + t.Fatalf("failed to write target file: %v", err) + } + + link := filepath.Join(workspace, "link.txt") + if err := os.Symlink(target, link); err != nil { + t.Skipf("symlink not supported in this environment: %v", err) + } + + tool := NewAppendFileTool(workspace, true) + result := tool.Execute(context.Background(), map[string]interface{}{ + "path": link, + "content": "appended", + }) + + if !result.IsError { + t.Fatalf("expected symlink escape to be blocked for append") + } + + // Verify original content was not modified + content, _ := os.ReadFile(target) + if string(content) != "original" { + t.Fatalf("expected original content to be preserved, got: %s", string(content)) + } +} diff --git a/pkg/tools/shell.go b/pkg/tools/shell.go index 48fd65d20..d46f0d1c3 100644 --- a/pkg/tools/shell.go +++ b/pkg/tools/shell.go @@ -20,6 +20,11 @@ var ( shellMetaRe = regexp.MustCompile("`|\\$\\(|\\$\\{") varReferenceRe = regexp.MustCompile(`\$[A-Za-z_][A-Za-z0-9_]*`) cdAbsoluteRe = regexp.MustCompile(`(?i)\bcd\s+/`) + ansiCQuoteRe = regexp.MustCompile(`\$'`) + ansiDQuoteRe = regexp.MustCompile(`\$"`) + hexEscapeRe = regexp.MustCompile(`\\x[0-9a-fA-F]`) + octalEscapeRe = regexp.MustCompile(`\\[0-7]{1,3}`) + escapedMetaRe = regexp.MustCompile(`\\[` + "`" + `$]`) ) type ExecTool struct { @@ -108,6 +113,20 @@ func (t *ExecTool) Execute(ctx context.Context, args map[string]interface{}) *To cwd = wd } + if t.restrictToWorkspace && cwd != t.workingDir { + absCwd, err := filepath.Abs(cwd) + if err != nil { + return ErrorResult("invalid working directory") + } + absWs, err := filepath.Abs(t.workingDir) + if err != nil { + return ErrorResult("invalid workspace directory") + } + if !isWithinWorkspace(absCwd, absWs) { + return ErrorResult("Command blocked by safety guard (working directory outside workspace)") + } + } + if cwd == "" { wd, err := os.Getwd() if err == nil { @@ -219,6 +238,18 @@ func (t *ExecTool) guardCommand(command, cwd string) string { return "Command blocked by safety guard (shell metacharacter in restricted mode)" } + // Block escape sequences that can bypass shell metacharacter detection + escapePatterns := []*regexp.Regexp{ansiCQuoteRe, ansiDQuoteRe, hexEscapeRe, octalEscapeRe, escapedMetaRe} + for _, re := range escapePatterns { + if re.MatchString(cmd) { + logger.WarnCF("shell", "Command blocked (escape sequence in restricted mode)", map[string]interface{}{ + "command_preview": truncateForLog(cmd), + "pattern": re.String(), + }) + return "Command blocked by safety guard (escape sequence in restricted mode)" + } + } + // Block variable expansion ($VAR) which can reference paths outside workspace if varReferenceRe.MatchString(cmd) { logger.WarnCF("shell", "Command blocked (variable expansion in restricted mode)", map[string]interface{}{ diff --git a/pkg/tools/shell_test.go b/pkg/tools/shell_test.go index 64aebb423..a5dd66203 100644 --- a/pkg/tools/shell_test.go +++ b/pkg/tools/shell_test.go @@ -313,3 +313,68 @@ func TestShellTool_WorkspaceAllowedCommands(t *testing.T) { }) } } + +// TestShellTool_EscapeSequenceBlocking verifies that escape sequences that bypass +// shell metacharacter detection are blocked in restricted mode. +func TestShellTool_EscapeSequenceBlocking(t *testing.T) { + tmpDir := t.TempDir() + tool := NewExecTool(tmpDir, true) + ctx := context.Background() + + cases := []struct { + name string + command string + }{ + {"ANSI-C quoting", `echo $'\x24(id)'`}, + {"locale quoting", `echo $"hello"`}, + {"hex escape", `echo \x24(id)`}, + {"octal escape", `echo \060`}, + {"escaped dollar", `echo \$HOME`}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + result := tool.Execute(ctx, map[string]interface{}{"command": tc.command}) + if !result.IsError { + t.Errorf("Expected command to be blocked: %q", tc.command) + } + if !strings.Contains(result.ForLLM, "escape sequence") { + t.Errorf("Expected 'escape sequence' in error for %q, got: %s", tc.command, result.ForLLM) + } + }) + } +} + +// TestShellTool_WorkingDirRestriction verifies that working_dir outside workspace is blocked. +func TestShellTool_WorkingDirRestriction(t *testing.T) { + tmpDir := t.TempDir() + tool := NewExecTool(tmpDir, true) + ctx := context.Background() + + // working_dir outside workspace should be blocked + t.Run("outside workspace blocked", func(t *testing.T) { + result := tool.Execute(ctx, map[string]interface{}{ + "command": "ls", + "working_dir": "/etc", + }) + if !result.IsError { + t.Errorf("Expected working_dir outside workspace to be blocked") + } + if !strings.Contains(result.ForLLM, "working directory outside workspace") { + t.Errorf("Expected 'working directory outside workspace' error, got: %s", result.ForLLM) + } + }) + + // working_dir inside workspace should be allowed + t.Run("inside workspace allowed", func(t *testing.T) { + subDir := filepath.Join(tmpDir, "subdir") + os.MkdirAll(subDir, 0755) + result := tool.Execute(ctx, map[string]interface{}{ + "command": "pwd", + "working_dir": subDir, + }) + if result.IsError && strings.Contains(result.ForLLM, "working directory outside workspace") { + t.Errorf("working_dir inside workspace should not be blocked, got: %s", result.ForLLM) + } + }) +}