diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index d59e75eda..73d243e19 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -1397,7 +1397,8 @@ func (al *AgentLoop) executeCmdMode( } // handleCdCommand handles the cd command in command mode, updating per-session working directory. -// Special paths (cd, cd ~, cd /, cd /xxx) are redirected to the workspace directory for safety. +// Special paths (cd, cd ~, cd /) are redirected to the workspace directory for safety. +// Uses tools.ValidatePath for robust containment: filepath.Rel + filepath.IsLocal + symlink resolution. func (al *AgentLoop) handleCdCommand(content, sessionKey string, agent *AgentInstance) string { parts := strings.Fields(content) workspace := agent.Workspace @@ -1408,15 +1409,13 @@ func (al *AgentLoop) handleCdCommand(content, sessionKey string, agent *AgentIns target = workspace } else { target = parts[1] + // Strip null bytes (defense-in-depth against bypass attempts) + target = strings.ReplaceAll(target, "\x00", "") // Expand ~ prefix: treat ~ as workspace root (not $HOME) if strings.HasPrefix(target, "~/") { target = workspace + target[1:] } - // Absolute paths (e.g. cd /etc) → redirect to workspace - if filepath.IsAbs(target) { - target = workspace - } - // Resolve relative paths + // Resolve relative paths against session working dir if !filepath.IsAbs(target) { currentDir := al.getSessionWorkDir(sessionKey) if currentDir == "" { @@ -1426,11 +1425,13 @@ func (al *AgentLoop) handleCdCommand(content, sessionKey string, agent *AgentIns } } - target = filepath.Clean(target) - - // Prevent traversal outside workspace via ../ - if !strings.HasPrefix(target, workspace) { + // Validate path is within workspace (handles traversal, symlinks, prefix matching correctly) + validated, err := tools.ValidatePath(target, workspace, true) + if err != nil { + // Path escapes workspace — fall back to workspace root target = workspace + } else { + target = validated } info, err := os.Stat(target) diff --git a/pkg/agent/loop_test.go b/pkg/agent/loop_test.go index 6dc1ccc7f..fc468184d 100644 --- a/pkg/agent/loop_test.go +++ b/pkg/agent/loop_test.go @@ -669,6 +669,71 @@ func TestHandleCdCommand_TraversalBlocked(t *testing.T) { _ = result } +// TestHandleCdCommand_NullByte verifies that null bytes in cd target +// are stripped and cannot bypass workspace restriction. +func TestHandleCdCommand_NullByte(t *testing.T) { + workspace := t.TempDir() + cfg := &config.Config{ + Agents: config.AgentsConfig{ + Defaults: config.AgentDefaults{ + Workspace: workspace, + Model: "test-model", + MaxTokens: 4096, + }, + }, + } + msgBus := bus.NewMessageBus() + provider := &mockProvider{} + al := NewAgentLoop(cfg, msgBus, provider) + agent := al.registry.GetDefaultAgent() + + al.setSessionWorkDir("test", workspace) + + // Null byte in path should be stripped — traversal caught after stripping + al.handleCdCommand("cd sub\x00dir/../../..", "test", agent) + workDir := al.getSessionWorkDir("test") + + if workDir != workspace { + t.Errorf("Expected workDir=%s after null-byte cd, got %s", workspace, workDir) + } +} + +// TestHandleCdCommand_SymlinkEscape verifies that a symlink inside workspace +// pointing outside is blocked by ValidatePath's symlink resolution. +func TestHandleCdCommand_SymlinkEscape(t *testing.T) { + workspace := t.TempDir() + outsideDir := t.TempDir() + + // Create a symlink inside workspace pointing outside + symlink := filepath.Join(workspace, "escape") + if err := os.Symlink(outsideDir, symlink); err != nil { + t.Skipf("Cannot create symlink: %v", err) + } + + cfg := &config.Config{ + Agents: config.AgentsConfig{ + Defaults: config.AgentDefaults{ + Workspace: workspace, + Model: "test-model", + MaxTokens: 4096, + }, + }, + } + msgBus := bus.NewMessageBus() + provider := &mockProvider{} + al := NewAgentLoop(cfg, msgBus, provider) + agent := al.registry.GetDefaultAgent() + + al.setSessionWorkDir("test", workspace) + + al.handleCdCommand("cd escape", "test", agent) + workDir := al.getSessionWorkDir("test") + + if workDir != workspace { + t.Errorf("Expected symlink escape to be blocked, workDir=%s, workspace=%s", workDir, workspace) + } +} + // TestHandleExtensionCommand_EmojiPassthrough verifies that emoji-like // messages starting with : are not intercepted as commands. func TestHandleExtensionCommand_EmojiPassthrough(t *testing.T) { diff --git a/pkg/tools/shell.go b/pkg/tools/shell.go index e4e1bef9b..0afb8001a 100644 --- a/pkg/tools/shell.go +++ b/pkg/tools/shell.go @@ -5,6 +5,7 @@ import ( "context" "errors" "fmt" + "net/url" "os" "os/exec" "path/filepath" @@ -48,6 +49,9 @@ var defaultDenyPatterns = []*regexp.Regexp{ regexp.MustCompile(`\$\(\s*wget\s+`), regexp.MustCompile(`\$\(\s*which\s+`), regexp.MustCompile(`\bsudo\b`), + regexp.MustCompile(`\bsu\b`), + regexp.MustCompile(`\bdoas\b`), + regexp.MustCompile(`\bpkexec\b`), regexp.MustCompile(`\bchmod\s+[0-7]{3,4}\b`), regexp.MustCompile(`\bchown\b`), regexp.MustCompile(`\bpkill\b`), @@ -255,8 +259,18 @@ func (t *ExecTool) Execute(ctx context.Context, args map[string]any) *ToolResult } } +// sanitizeCommand strips null bytes and decodes URL-encoded sequences +// so that encoded traversal patterns (e.g. %2e%2e%2f) are detected by guards. +func sanitizeCommand(cmd string) string { + cmd = strings.ReplaceAll(cmd, "\x00", "") + if decoded, err := url.PathUnescape(cmd); err == nil { + cmd = decoded + } + return cmd +} + func (t *ExecTool) guardCommand(command, cwd string) string { - cmd := strings.TrimSpace(command) + cmd := sanitizeCommand(strings.TrimSpace(command)) lower := strings.ToLower(cmd) for _, pattern := range t.denyPatterns { diff --git a/pkg/tools/shell_test.go b/pkg/tools/shell_test.go index a8bd603cc..d60b6f724 100644 --- a/pkg/tools/shell_test.go +++ b/pkg/tools/shell_test.go @@ -296,3 +296,59 @@ func TestGuardCommand_DotSlashExecutable(t *testing.T) { t.Errorf("Expected output 'ok', got: %s", result.ForLLM) } } + +// TestGuardCommand_URLEncodedTraversal verifies that URL-encoded path traversal +// sequences (%2e%2e%2f → ../) are detected and blocked. +func TestGuardCommand_URLEncodedTraversal(t *testing.T) { + tmpDir := t.TempDir() + tool := NewExecTool(tmpDir, true) + tool.SetRestrictToWorkspace(true) + + msg := tool.guardCommand("cat %2e%2e%2f%2e%2e%2fetc/passwd", tmpDir) + if msg == "" { + t.Error("Expected URL-encoded path traversal to be blocked") + } +} + +// TestGuardCommand_NullByte verifies that null bytes in commands are stripped +// before guard checks so they cannot bypass traversal detection. +func TestGuardCommand_NullByte(t *testing.T) { + tmpDir := t.TempDir() + tool := NewExecTool(tmpDir, true) + tool.SetRestrictToWorkspace(true) + + msg := tool.guardCommand("cat foo\x00../../etc/passwd", tmpDir) + if msg == "" { + t.Error("Expected null-byte traversal to be blocked") + } +} + +// TestGuardCommand_SuBlocked verifies that su and related privilege +// escalation commands are blocked by deny patterns. +func TestGuardCommand_SuBlocked(t *testing.T) { + tmpDir := t.TempDir() + tool := NewExecTool(tmpDir, true) + + cases := []string{"su", "su -", "su root", "doas ls", "pkexec /bin/bash"} + for _, cmd := range cases { + msg := tool.guardCommand(cmd, tmpDir) + if msg == "" { + t.Errorf("Expected %q to be blocked", cmd) + } + } +} + +// TestGuardCommand_SuNoFalsePositive verifies that words containing "su" +// as a substring are NOT blocked (e.g. summary, result, surplus). +func TestGuardCommand_SuNoFalsePositive(t *testing.T) { + tmpDir := t.TempDir() + tool := NewExecTool(tmpDir, false) // no workspace restriction for this test + + cases := []string{"echo surplus", "cat summary.txt", "ls result/"} + for _, cmd := range cases { + msg := tool.guardCommand(cmd, tmpDir) + if msg != "" { + t.Errorf("Expected %q to NOT be blocked, got: %s", cmd, msg) + } + } +}