From 54eb65aad894aff8270f6bc626e9916243a3041a Mon Sep 17 00:00:00 2001 From: Boris Bliznioukov Date: Wed, 4 Mar 2026 15:59:55 +0100 Subject: [PATCH] feat(security): fix reviewer comments enhance risk classification and sandboxing for shell commands Signed-off-by: Boris Bliznioukov --- docs/design/DDR-shell-tool-hardening.md | 6 +- docs/tools_configuration.md | 32 ++++++-- pkg/tools/shell/risk.go | 102 ++++++++++++++--------- pkg/tools/shell/risk_test.go | 105 +++++++++++++++++++++++- pkg/tools/shell/sandbox.go | 13 ++- pkg/tools/shell/sandbox_test.go | 67 +++++++++++++++ pkg/tools/shell_tool.go | 5 -- 7 files changed, 270 insertions(+), 60 deletions(-) diff --git a/docs/design/DDR-shell-tool-hardening.md b/docs/design/DDR-shell-tool-hardening.md index 3d5ed80cc..2dfbae08a 100644 --- a/docs/design/DDR-shell-tool-hardening.md +++ b/docs/design/DDR-shell-tool-hardening.md @@ -90,7 +90,11 @@ | `high` | Block | Destructive, system-modifying (rm, chmod, git push) | | `critical` | Block | Privilege escalation, always dangerous (sudo, dd, eval) | -5. The risk classifier MUST apply argument-aware modifiers (e.g., `curl` is `medium`, but `curl -X POST` is `high`; `git` is `medium`, but `git push` is `high`). + Shell interpreters (`sh`, `bash`, `zsh`, `dash`, `fish`, `ksh`, `csh`, `tcsh`, `powershell`, `pwsh`, `cmd`) MUST be classified as `critical` because they can execute arbitrary nested commands that bypass the risk classifier entirely (e.g., `sh -c 'rm -rf /'`). + +5. The risk classifier MUST apply argument-aware modifiers (e.g., `curl` is `medium`, but `curl -X POST` is `high`; `git` is `medium`, but `git push` is `high`). All matching modifiers MUST be scanned and the highest level that exceeds the base level is applied (highest-match-wins, not first-match-wins). + +5a. `risk_overrides` sets the **base level** for a command (replacing the built-in table entry). Argument modifiers MUST still be applied on top of the overridden base level and can elevate it further. This means `risk_overrides: {"rm": "medium"}` allows plain `rm` but `rm -rf` is still elevated to `critical` by the built-in modifier. 6. When a command is blocked, the `ToolResult` MUST include: - Risk level of the command. diff --git a/docs/tools_configuration.md b/docs/tools_configuration.md index 09470fdeb..e16af6e4b 100644 --- a/docs/tools_configuration.md +++ b/docs/tools_configuration.md @@ -50,13 +50,13 @@ AST-based risk classification, environment sanitization, and file-access sandbox ### Configuration -| Config | Type | Default | Description | -| ---------------- | ------ | ---------- | ----------------------------------------------------------------------- | -| `risk_threshold` | string | `"medium"` | Maximum allowed risk level: `"low"`, `"medium"`, `"high"`, `"critical"` | -| `risk_overrides` | object | `{}` | Per-command risk level overrides (command name → level) | -| `arg_modifiers` | object | `{}` | Per-command argument patterns that adjust risk level | -| `env_allowlist` | array | `[]` | Extra environment variables to expose (extends built-in defaults) | -| `env_set` | object | `{}` | Explicit `VAR=value` pairs injected into every command | +| Config | Type | Default | Description | +| ---------------- | ------ | ---------- | ------------------------------------------------------------------------------- | +| `risk_threshold` | string | `"medium"` | Maximum allowed risk level: `"low"`, `"medium"`, `"high"`, `"critical"` | +| `risk_overrides` | object | `{}` | Per-command base risk level (command name → level); modifiers can still elevate | +| `arg_modifiers` | object | `{}` | Per-command argument patterns that adjust risk level | +| `env_allowlist` | array | `[]` | Extra environment variables to expose (extends built-in defaults) | +| `env_set` | object | `{}` | Explicit `VAR=value` pairs injected into every command | ### Risk Classification @@ -91,6 +91,24 @@ must all be present (order-independent) and the resulting level: The **highest matching** modifier wins (built-in and custom are merged). +#### Precedence + +The final risk level is computed as: + +1. **Base level**: `risk_overrides` entry if present, else built-in table, else `medium`. +2. **Modifiers**: All matching argument modifiers (built-in + custom) are scanned. + The highest level that exceeds the base is applied. Modifiers can only elevate, + never lower. + +This means `"risk_overrides": {"rm": "medium"}` allows plain `rm` at the `medium` +threshold, but `rm -rf` is still elevated to `critical` by the built-in modifier. + +#### Shell wrappers + +Shell interpreters (`sh`, `bash`, `zsh`, `dash`, `fish`, `ksh`, `csh`, `tcsh`, +`powershell`, `pwsh`, `cmd`) are classified as `critical` because they can execute +arbitrary nested commands that bypass the risk classifier (e.g., `sh -c 'rm -rf /'`). + ### Environment Sanitization The shell interpreter runs with a sanitized environment. Only a safe allowlist diff --git a/pkg/tools/shell/risk.go b/pkg/tools/shell/risk.go index f8dc11edc..cb4e0c8c3 100644 --- a/pkg/tools/shell/risk.go +++ b/pkg/tools/shell/risk.go @@ -1,6 +1,9 @@ package shell -import "fmt" +import ( + "fmt" + "path/filepath" +) // RiskLevel represents the potential danger of a shell command. type RiskLevel int @@ -203,6 +206,21 @@ var commandRiskTable = map[string]RiskLevel{ ".": RiskCritical, "format": RiskCritical, "diskpart": RiskCritical, + + // Critical — shell wrappers can execute arbitrary nested commands, + // bypassing the risk classifier entirely (e.g. sh -c 'rm -rf /'). + "sh": RiskCritical, + "bash": RiskCritical, + "zsh": RiskCritical, + "dash": RiskCritical, + "fish": RiskCritical, + "csh": RiskCritical, + "tcsh": RiskCritical, + "ksh": RiskCritical, + "powershell": RiskCritical, + "pwsh": RiskCritical, + "cmd": RiskCritical, + "cmd.exe": RiskCritical, } // ArgModifier describes a condition that elevates a command's risk level. @@ -214,7 +232,7 @@ type ArgModifier struct { } // argumentModifiers maps command names to their argument-aware risk adjustments. -// Checked in order; first match wins. +// All matching modifiers are scanned; the highest level wins. // // Patterns use individual flags (e.g., "-r", "-f") rather than combined forms // ("-rf") because normalizeFlags splits combined flags before matching. This @@ -291,9 +309,17 @@ var argumentModifiers = map[string][]ArgModifier{ // ClassifyCommand determines the risk level of a resolved command. // args[0] is the command name (basename), args[1:] are the arguments. -// overrides allows per-command level overrides from config. -// extraModifiers are checked after built-in argumentModifiers. -// The highest matching level across all sources wins. +// +// Precedence (highest wins): +// 1. Argument modifiers (built-in, then user-supplied extraModifiers) — +// all matching modifiers are scanned; the maximum level is kept. +// 2. risk_overrides from config — sets the base level for the command, +// replacing the built-in table entry. Modifiers can still elevate above it. +// 3. Built-in commandRiskTable — default base level per command. +// 4. Commands not in any table default to RiskMedium. +// +// This means risk_overrides: {"rm": "medium"} allows plain `rm` but +// `rm -rf` is still elevated to critical by the built-in modifier. func ClassifyCommand(args []string, overrides map[string]string, extraModifiers ...map[string][]ArgModifier) RiskLevel { if len(args) == 0 { return RiskMedium @@ -301,60 +327,59 @@ func ClassifyCommand(args []string, overrides map[string]string, extraModifiers cmdName := baseCommand(args[0]) - if overrides != nil { - if levelStr, ok := overrides[cmdName]; ok { - level, err := ParseRiskLevel(levelStr) - if err == nil { - return level - } - // Invalid risk level in override: fall through to default classification. - // The parse error is descriptive, but we can't log from here without - // injecting a logger. Config-time validation catches user errors. - } - } - + // Determine base level: override > table > medium default. level, known := commandRiskTable[cmdName] if !known { level = RiskMedium } + if overrides != nil { + if levelStr, ok := overrides[cmdName]; ok { + parsed, err := ParseRiskLevel(levelStr) + if err == nil { + level = parsed + } + // Invalid risk level in override: keep table/default level. + // Config-time validation catches user errors. + } + } // Normalize args: expand combined short flags like -rf → -r, -f // so that modifiers match regardless of how flags were grouped or ordered. normalizedArgs := normalizeFlags(args[1:]) - // Check built-in modifiers, then user-supplied. Keep the highest match. - if elevated, ok := applyModifiers(normalizedArgs, cmdName, level, argumentModifiers); ok { - level = elevated - } + // Apply built-in modifiers, then user-supplied. Keep the highest match + // across all sources. Modifiers can only elevate, never lower. + level = applyModifiers(normalizedArgs, cmdName, level, argumentModifiers) for _, extra := range extraModifiers { if extra == nil { continue } - if elevated, ok := applyModifiers(normalizedArgs, cmdName, level, extra); ok { - level = elevated - } + level = applyModifiers(normalizedArgs, cmdName, level, extra) } return level } -// applyModifiers checks whether any modifier for cmdName matches the -// normalised args and would elevate the risk. Returns (newLevel, true) -// on first match, or (0, false) if nothing matched. +// applyModifiers scans all modifiers for cmdName, matches them against +// normalizedArgs, and returns the highest level that exceeds baseLevel. +// If no modifier elevates, returns baseLevel unchanged. func applyModifiers( normalizedArgs []string, cmdName string, baseLevel RiskLevel, mods map[string][]ArgModifier, -) (RiskLevel, bool) { - if entries, ok := mods[cmdName]; ok { - for _, mod := range entries { - if matchArgs(normalizedArgs, mod.Args) && mod.Level > baseLevel { - return mod.Level, true - } +) RiskLevel { + entries, ok := mods[cmdName] + if !ok { + return baseLevel + } + result := baseLevel + for _, mod := range entries { + if matchArgs(normalizedArgs, mod.Args) && mod.Level > result { + result = mod.Level } } - return 0, false + return result } // IsAllowed returns true if the given risk level is at or below the threshold. @@ -388,13 +413,10 @@ func BlockedCommandError(args []string, level, threshold RiskLevel, reason strin } // baseCommand extracts the basename from a command path. +// Uses filepath.Base so both forward slashes and Windows backslashes +// are handled correctly. func baseCommand(cmd string) string { - for i := len(cmd) - 1; i >= 0; i-- { - if cmd[i] == '/' { - return cmd[i+1:] - } - } - return cmd + return filepath.Base(cmd) } // normalizeFlags expands combined short flags (e.g., "-rf" → "-r", "-f") diff --git a/pkg/tools/shell/risk_test.go b/pkg/tools/shell/risk_test.go index 383b14439..d67da0fd4 100644 --- a/pkg/tools/shell/risk_test.go +++ b/pkg/tools/shell/risk_test.go @@ -97,26 +97,53 @@ func TestClassifyCommand_ArgumentModifiers(t *testing.T) { func TestClassifyCommand_Overrides(t *testing.T) { overrides := map[string]string{ - "rm": "low", + "rm": "medium", "curl": "critical", } + // Override sets the BASE level, but modifiers still elevate. + // rm is overridden to medium, but rm -rf triggers the built-in + // modifier that elevates to critical. got := ClassifyCommand([]string{"rm", "-rf", "/"}, overrides) - if got != RiskLow { - t.Errorf("override rm to low: got %s", got) + if got != RiskCritical { + t.Errorf("override rm to medium + rm -rf modifier should be critical: got %s", got) } + // Plain rm (no -rf) stays at the overridden level. + got = ClassifyCommand([]string{"rm", "file.txt"}, overrides) + if got != RiskMedium { + t.Errorf("override rm to medium (no modifier match): got %s", got) + } + + // Override elevates curl to critical unconditionally. got = ClassifyCommand([]string{"curl", "https://example.com"}, overrides) if got != RiskCritical { t.Errorf("override curl to critical: got %s", got) } + // No override for ls — uses table as before. got = ClassifyCommand([]string{"ls"}, overrides) if got != RiskLow { t.Errorf("ls (no override) should be low: got %s", got) } } +func TestClassifyCommand_OverrideLowers_ModifierStillElevates(t *testing.T) { + // Scenario: user sets rm to low ("I want rm allowed"), but rm -rf + // still hits the built-in modifier → critical. + overrides := map[string]string{"rm": "low"} + + got := ClassifyCommand([]string{"rm", "file.txt"}, overrides) + if got != RiskLow { + t.Errorf("plain rm with override=low should be low: got %s", got) + } + + got = ClassifyCommand([]string{"rm", "-rf", "/"}, overrides) + if got != RiskCritical { + t.Errorf("rm -rf should still be critical despite override=low: got %s", got) + } +} + func TestClassifyCommand_UnknownCommand(t *testing.T) { got := ClassifyCommand([]string{"some_unknown_tool", "--flag"}, nil) if got != RiskMedium { @@ -136,6 +163,25 @@ func TestClassifyCommand_FullPath(t *testing.T) { } } +func TestClassifyCommand_BackslashPath(t *testing.T) { + // Forward-slash paths at various depths. + got := ClassifyCommand([]string{"/usr/sbin/shutdown", "-h"}, nil) + if got != RiskCritical { + t.Errorf("/usr/sbin/shutdown should be critical, got %s", got) + } + + got = ClassifyCommand([]string{"/usr/local/bin/sudo", "ls"}, nil) + if got != RiskCritical { + t.Errorf("/usr/local/bin/sudo should be critical, got %s", got) + } + + // Bare command still works after the filepath.Base change. + got = ClassifyCommand([]string{"dd", "if=/dev/zero"}, nil) + if got != RiskCritical { + t.Errorf("bare dd should be critical, got %s", got) + } +} + func TestIsAllowed(t *testing.T) { tests := []struct { level RiskLevel @@ -283,7 +329,8 @@ func TestClassifyCommand_ExtraArgModifiers(t *testing.T) { func TestClassifyCommand_ExtraArgModifiers_NoOverrideBuiltIn(t *testing.T) { // Extra modifier tries to set "rm -rf" to medium, but built-in already - // elevates to critical and built-in is checked first. + // elevates to critical. Since we take the max across all matching + // modifiers, the built-in critical wins. extra := map[string][]ArgModifier{ "rm": { {Args: []string{"-r", "-f"}, Level: RiskMedium}, @@ -295,3 +342,53 @@ func TestClassifyCommand_ExtraArgModifiers_NoOverrideBuiltIn(t *testing.T) { t.Errorf("built-in should win over extra for rm -rf: got %s", got) } } + +func TestClassifyCommand_ShellWrappers(t *testing.T) { + // Shell wrappers must be critical to prevent classifier bypass. + shells := []string{"sh", "bash", "zsh", "dash", "fish", "ksh", "csh", "tcsh", "powershell", "pwsh", "cmd", "cmd.exe"} + for _, sh := range shells { + t.Run(sh, func(t *testing.T) { + got := ClassifyCommand([]string{sh, "-c", "echo hi"}, nil) + if got != RiskCritical { + t.Errorf("%s should be critical, got %s", sh, got) + } + }) + } +} + +func TestClassifyCommand_ShellWrapperFullPath(t *testing.T) { + // /bin/sh, /usr/bin/bash etc. should also be caught via baseCommand. + got := ClassifyCommand([]string{"/bin/sh", "-c", "rm -rf /"}, nil) + if got != RiskCritical { + t.Errorf("/bin/sh should be critical, got %s", got) + } + + got = ClassifyCommand([]string{"/usr/bin/bash", "-c", "sudo rm -rf /"}, nil) + if got != RiskCritical { + t.Errorf("/usr/bin/bash should be critical, got %s", got) + } +} + +func TestApplyModifiers_HighestMatchWins(t *testing.T) { + // When multiple modifiers match, the highest level should win. + // Scenario: git push matches both ["push"] → High and ["push", "-f"] → Critical + args := normalizeFlags([]string{"push", "-f", "origin"}) + result := applyModifiers(args, "git", RiskMedium, argumentModifiers) + if result != RiskCritical { + t.Errorf("git push -f should resolve to critical (highest match), got %s", result) + } + + // Only ["push"] matches → High + args2 := normalizeFlags([]string{"push", "origin"}) + result2 := applyModifiers(args2, "git", RiskMedium, argumentModifiers) + if result2 != RiskHigh { + t.Errorf("git push (no -f) should resolve to high, got %s", result2) + } + + // No modifier matches → base level unchanged + args3 := normalizeFlags([]string{"status"}) + result3 := applyModifiers(args3, "git", RiskMedium, argumentModifiers) + if result3 != RiskMedium { + t.Errorf("git status should stay medium, got %s", result3) + } +} diff --git a/pkg/tools/shell/sandbox.go b/pkg/tools/shell/sandbox.go index 43729de81..8de7d0c69 100644 --- a/pkg/tools/shell/sandbox.go +++ b/pkg/tools/shell/sandbox.go @@ -45,10 +45,17 @@ func SandboxedOpenHandler(workspaceDir string) interp.OpenHandlerFunc { return os.OpenFile(path, flag, perm) } - absPath, err := filepath.Abs(path) - if err != nil { - return nil, fmt.Errorf("sandbox: cannot resolve path %q: %w", path, err) + // Resolve relative paths against the interpreter's working directory, + // not the process CWD. The interpreter tracks its own CWD via + // interp.Dir() and internal cd commands without calling os.Chdir(). + var absPath string + if filepath.IsAbs(path) { + absPath = path + } else { + hctx := interp.HandlerCtx(ctx) + absPath = filepath.Join(hctx.Dir, path) } + // filepath.Join already returns a clean path; no extra Abs needed. // Resolve symlinks to prevent escape. // If the file doesn't exist yet, resolve the parent. diff --git a/pkg/tools/shell/sandbox_test.go b/pkg/tools/shell/sandbox_test.go index 2b008ed29..3240c15c7 100644 --- a/pkg/tools/shell/sandbox_test.go +++ b/pkg/tools/shell/sandbox_test.go @@ -4,7 +4,9 @@ import ( "context" "os" "path/filepath" + "strings" "testing" + "time" ) func TestSandboxedOpenHandler_AllowsInsideWorkspace(t *testing.T) { @@ -80,6 +82,71 @@ func TestSandboxedOpenHandler_AllowsNewFileInWorkspace(t *testing.T) { os.Remove(newFile) } +// TestSandboxedOpenHandler_RelativePathUsesInterpreterCwd verifies that +// relative paths in shell redirections resolve against the interpreter's +// working directory (from interp.HandlerCtx), not the process CWD. +func TestSandboxedOpenHandler_RelativePathUsesInterpreterCwd(t *testing.T) { + workspace := t.TempDir() + + // Write to a relative path inside the workspace via the interpreter. + // The interpreter's Dir is set to workspace, so "output.txt" should + // resolve to workspace/output.txt regardless of the process CWD. + result := Run(context.Background(), RunConfig{ + Command: "echo sandbox_relative > output.txt", + Dir: workspace, + Timeout: 5 * time.Second, + Restrict: true, + WorkspaceDir: workspace, + RiskThreshold: RiskMedium, + }) + + if result.IsError { + t.Fatalf("relative redirect inside workspace should succeed: %s", result.Output) + } + + content, err := os.ReadFile(filepath.Join(workspace, "output.txt")) + if err != nil { + t.Fatalf("output.txt should exist in workspace: %v", err) + } + if !strings.Contains(string(content), "sandbox_relative") { + t.Errorf("expected 'sandbox_relative' in file, got: %s", content) + } +} + +// TestSandboxedOpenHandler_RelativePathBlocksEscapeViaCd verifies that +// if a script uses cd to move outside the workspace, a subsequent relative +// redirect is blocked by the sandbox. +func TestSandboxedOpenHandler_RelativePathBlocksEscapeViaCd(t *testing.T) { + root := t.TempDir() + workspace := filepath.Join(root, "workspace") + outside := filepath.Join(root, "outside") + os.MkdirAll(workspace, 0o755) + os.MkdirAll(outside, 0o755) + + // cd to outside dir, then try to write a relative path. + // The sandbox should block because the resolved path is outside workspace. + result := Run(context.Background(), RunConfig{ + Command: "cd " + outside + " && echo escaped > leak.txt", + Dir: workspace, + Timeout: 5 * time.Second, + Restrict: true, + WorkspaceDir: workspace, + RiskThreshold: RiskHigh, // allow cd + }) + + if !result.IsError { + // If it didn't error, check that the file was NOT written outside + if _, err := os.Stat(filepath.Join(outside, "leak.txt")); err == nil { + t.Fatal("sandbox should have blocked write outside workspace") + } + } + + // Verify nothing leaked + if _, err := os.Stat(filepath.Join(outside, "leak.txt")); err == nil { + t.Error("leak.txt should not exist outside workspace") + } +} + // TestSandboxedOpenHandler_AllowsDottedFiles verifies that files with // names starting with ".." (like ".../file", "....txt", "..something") // are NOT incorrectly blocked by the escape check. diff --git a/pkg/tools/shell_tool.go b/pkg/tools/shell_tool.go index 4276fd819..dd5b0d7ae 100644 --- a/pkg/tools/shell_tool.go +++ b/pkg/tools/shell_tool.go @@ -71,11 +71,6 @@ func NewExecToolWithConfig(workingDir string, restrict bool, cfg *config.Config) } func warnDeprecatedExecConfig(cfg config.ExecConfig) { - if !cfg.EnableDenyPatterns { - fmt.Println("Warning: 'enable_deny_patterns' is deprecated and ignored. " + - "The new shell tool uses AST-based risk classification. " + - "Use 'risk_threshold' to control command blocking.") - } if len(cfg.CustomDenyPatterns) > 0 { fmt.Println("Warning: 'custom_deny_patterns' is deprecated and ignored. " + "Use 'risk_overrides' to adjust per-command risk levels.")