From 6053baaf82e5eab191815a3489a1f8951abf3cf4 Mon Sep 17 00:00:00 2001 From: Huang Rui Date: Mon, 2 Mar 2026 00:57:02 +0800 Subject: [PATCH] fix(tools): allow /dev/null redirection and add read/write sandbox split - Remove deny pattern that incorrectly blocked redirects to /dev/null - Expand block device write pattern to cover nvme, mmcblk, vd, xvd, hd, loop, dm-, md, sr and nbd in addition to sd - Add safe path whitelist for kernel pseudo-devices so workspace path check does not reject /dev/null, /dev/zero, /dev/random, /dev/urandom, /dev/stdin, /dev/stdout and /dev/stderr - Add allow_read_outside_workspace config option (default true) so file read and list tools are unrestricted while write tools stay sandboxed Closes https://github.com/sipeed/picoclaw/issues/964 Closes https://github.com/sipeed/picoclaw/issues/965 Signed-off-by: Huang Rui --- pkg/agent/instance.go | 5 +-- pkg/config/config.go | 23 ++++++------ pkg/tools/shell.go | 23 ++++++++++-- pkg/tools/shell_test.go | 78 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 114 insertions(+), 15 deletions(-) diff --git a/pkg/agent/instance.go b/pkg/agent/instance.go index e597542e4..796ce2dea 100644 --- a/pkg/agent/instance.go +++ b/pkg/agent/instance.go @@ -48,10 +48,11 @@ func NewAgentInstance( fallbacks := resolveAgentFallbacks(agentCfg, defaults) restrict := defaults.RestrictToWorkspace + readRestrict := restrict && !defaults.AllowReadOutsideWorkspace toolsRegistry := tools.NewToolRegistry() - toolsRegistry.Register(tools.NewReadFileTool(workspace, restrict)) + toolsRegistry.Register(tools.NewReadFileTool(workspace, readRestrict)) toolsRegistry.Register(tools.NewWriteFileTool(workspace, restrict)) - toolsRegistry.Register(tools.NewListDirTool(workspace, restrict)) + toolsRegistry.Register(tools.NewListDirTool(workspace, readRestrict)) execTool, err := tools.NewExecToolWithConfig(workspace, restrict, cfg) if err != nil { log.Fatalf("Critical error: unable to initialize exec tool: %v", err) diff --git a/pkg/config/config.go b/pkg/config/config.go index 55d0cfb2c..ff37b7ac4 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -168,17 +168,18 @@ type SessionConfig struct { } type AgentDefaults struct { - Workspace string `json:"workspace" env:"PICOCLAW_AGENTS_DEFAULTS_WORKSPACE"` - RestrictToWorkspace bool `json:"restrict_to_workspace" env:"PICOCLAW_AGENTS_DEFAULTS_RESTRICT_TO_WORKSPACE"` - Provider string `json:"provider" env:"PICOCLAW_AGENTS_DEFAULTS_PROVIDER"` - ModelName string `json:"model_name,omitempty" env:"PICOCLAW_AGENTS_DEFAULTS_MODEL_NAME"` - Model string `json:"model" env:"PICOCLAW_AGENTS_DEFAULTS_MODEL"` // Deprecated: use model_name instead - ModelFallbacks []string `json:"model_fallbacks,omitempty"` - ImageModel string `json:"image_model,omitempty" env:"PICOCLAW_AGENTS_DEFAULTS_IMAGE_MODEL"` - ImageModelFallbacks []string `json:"image_model_fallbacks,omitempty"` - MaxTokens int `json:"max_tokens" env:"PICOCLAW_AGENTS_DEFAULTS_MAX_TOKENS"` - Temperature *float64 `json:"temperature,omitempty" env:"PICOCLAW_AGENTS_DEFAULTS_TEMPERATURE"` - MaxToolIterations int `json:"max_tool_iterations" env:"PICOCLAW_AGENTS_DEFAULTS_MAX_TOOL_ITERATIONS"` + Workspace string `json:"workspace" env:"PICOCLAW_AGENTS_DEFAULTS_WORKSPACE"` + RestrictToWorkspace bool `json:"restrict_to_workspace" env:"PICOCLAW_AGENTS_DEFAULTS_RESTRICT_TO_WORKSPACE"` + AllowReadOutsideWorkspace bool `json:"allow_read_outside_workspace" env:"PICOCLAW_AGENTS_DEFAULTS_ALLOW_READ_OUTSIDE_WORKSPACE"` + Provider string `json:"provider" env:"PICOCLAW_AGENTS_DEFAULTS_PROVIDER"` + ModelName string `json:"model_name,omitempty" env:"PICOCLAW_AGENTS_DEFAULTS_MODEL_NAME"` + Model string `json:"model" env:"PICOCLAW_AGENTS_DEFAULTS_MODEL"` // Deprecated: use model_name instead + ModelFallbacks []string `json:"model_fallbacks,omitempty"` + ImageModel string `json:"image_model,omitempty" env:"PICOCLAW_AGENTS_DEFAULTS_IMAGE_MODEL"` + ImageModelFallbacks []string `json:"image_model_fallbacks,omitempty"` + MaxTokens int `json:"max_tokens" env:"PICOCLAW_AGENTS_DEFAULTS_MAX_TOKENS"` + Temperature *float64 `json:"temperature,omitempty" env:"PICOCLAW_AGENTS_DEFAULTS_TEMPERATURE"` + MaxToolIterations int `json:"max_tool_iterations" env:"PICOCLAW_AGENTS_DEFAULTS_MAX_TOOL_ITERATIONS"` } // GetModelName returns the effective model name for the agent defaults. diff --git a/pkg/tools/shell.go b/pkg/tools/shell.go index 2fd22353f..6b1638105 100644 --- a/pkg/tools/shell.go +++ b/pkg/tools/shell.go @@ -34,7 +34,10 @@ var ( `\b(format|mkfs|diskpart)\b\s`, ), regexp.MustCompile(`\bdd\s+if=`), - regexp.MustCompile(`>\s*/dev/sd[a-z]\b`), // Block writes to disk devices (but allow /dev/null) + // Block writes to block devices (all common naming schemes). + regexp.MustCompile( + `>\s*/dev/(sd[a-z]|hd[a-z]|vd[a-z]|xvd[a-z]|nvme\d|mmcblk\d|loop\d|dm-\d|md\d|sr\d|nbd\d)`, + ), regexp.MustCompile(`\b(shutdown|reboot|poweroff)\b`), regexp.MustCompile(`:\(\)\s*\{.*\};\s*:`), regexp.MustCompile(`\$\([^)]+\)`), @@ -45,7 +48,6 @@ var ( regexp.MustCompile(`;\s*rm\s+-[rf]`), regexp.MustCompile(`&&\s*rm\s+-[rf]`), regexp.MustCompile(`\|\|\s*rm\s+-[rf]`), - regexp.MustCompile(`>\s*/dev/null\s*>&?\s*\d?`), regexp.MustCompile(`<<\s*EOF`), regexp.MustCompile(`\$\(\s*cat\s+`), regexp.MustCompile(`\$\(\s*curl\s+`), @@ -75,6 +77,19 @@ var ( // absolutePathPattern matches absolute file paths in commands (Unix and Windows). absolutePathPattern = regexp.MustCompile(`[A-Za-z]:\\[^\\\"']+|/[^\s\"']+`) + + // safePaths are kernel pseudo-devices that are always safe to reference in + // commands, regardless of workspace restriction. They contain no user data + // and cannot cause destructive writes. + safePaths = map[string]bool{ + "/dev/null": true, + "/dev/zero": true, + "/dev/random": true, + "/dev/urandom": true, + "/dev/stdin": true, + "/dev/stdout": true, + "/dev/stderr": true, + } ) func NewExecTool(workingDir string, restrict bool) (*ExecTool, error) { @@ -303,6 +318,10 @@ func (t *ExecTool) guardCommand(command, cwd string) string { continue } + if safePaths[p] { + continue + } + rel, err := filepath.Rel(cwdPath, p) if err != nil { continue diff --git a/pkg/tools/shell_test.go b/pkg/tools/shell_test.go index 1a179547a..91f93039c 100644 --- a/pkg/tools/shell_test.go +++ b/pkg/tools/shell_test.go @@ -309,3 +309,81 @@ func TestShellTool_RestrictToWorkspace(t *testing.T) { ) } } + +// TestShellTool_DevNullAllowed verifies that /dev/null redirections are not blocked (issue #964). +func TestShellTool_DevNullAllowed(t *testing.T) { + tmpDir := t.TempDir() + tool, err := NewExecTool(tmpDir, true) + if err != nil { + t.Fatalf("unable to configure exec tool: %s", err) + } + + commands := []string{ + "echo hello 2>/dev/null", + "echo hello >/dev/null", + "echo hello > /dev/null", + "echo hello 2> /dev/null", + "echo hello >/dev/null 2>&1", + "find " + tmpDir + " -name '*.go' 2>/dev/null", + } + + for _, cmd := range commands { + result := tool.Execute(context.Background(), map[string]any{"command": cmd}) + if result.IsError && strings.Contains(result.ForLLM, "blocked") { + t.Errorf("command should not be blocked: %s\n error: %s", cmd, result.ForLLM) + } + } +} + +// TestShellTool_BlockDevices verifies that writes to block devices are blocked (issue #965). +func TestShellTool_BlockDevices(t *testing.T) { + tool, err := NewExecTool("", false) + if err != nil { + t.Fatalf("unable to configure exec tool: %s", err) + } + + blocked := []string{ + "echo x > /dev/sda", + "echo x > /dev/hda", + "echo x > /dev/vda", + "echo x > /dev/xvda", + "echo x > /dev/nvme0n1", + "echo x > /dev/mmcblk0", + "echo x > /dev/loop0", + "echo x > /dev/dm-0", + "echo x > /dev/md0", + "echo x > /dev/sr0", + "echo x > /dev/nbd0", + } + + for _, cmd := range blocked { + result := tool.Execute(context.Background(), map[string]any{"command": cmd}) + if !result.IsError { + t.Errorf("expected block device write to be blocked: %s", cmd) + } + } +} + +// TestShellTool_SafePathsInWorkspaceRestriction verifies that safe kernel pseudo-devices +// are allowed even when workspace restriction is active. +func TestShellTool_SafePathsInWorkspaceRestriction(t *testing.T) { + tmpDir := t.TempDir() + tool, err := NewExecTool(tmpDir, true) + if err != nil { + t.Fatalf("unable to configure exec tool: %s", err) + } + + // These reference paths outside workspace but should be allowed via safePaths. + commands := []string{ + "cat /dev/urandom | head -c 16 | od", + "echo test > /dev/null", + "dd if=/dev/zero bs=1 count=1", + } + + for _, cmd := range commands { + result := tool.Execute(context.Background(), map[string]any{"command": cmd}) + if result.IsError && strings.Contains(result.ForLLM, "path outside working dir") { + t.Errorf("safe path should not be blocked by workspace check: %s\n error: %s", cmd, result.ForLLM) + } + } +}