From b4e18abcb873f2c5fbd694f63c41c642ac0fee0c Mon Sep 17 00:00:00 2001 From: Keith Patrick Date: Sun, 8 Mar 2026 19:41:12 +0000 Subject: [PATCH] feat(security): add environment sanitization for shell execution - Add BuildSanitizedEnv() with default allowlist (PATH, HOME, USER, etc) - Add PICOCLAW_* vars to allowlist for config propagation - Add LLMBlocklist for vars LLM cannot override - Cache sanitized env at ExecTool init, pass to exec.Cmd - Add tests and docs Part of PR1: Environment Sanitization for External Execution --- docs/tools_configuration.md | 31 ++++- pkg/tools/shell.go | 7 ++ pkg/tools/shell/env.go | 156 ++++++++++++++++++++++++ pkg/tools/shell/env_test.go | 180 ++++++++++++++++++++++++++++ pkg/tools/shell/env_windows_test.go | 68 +++++++++++ 5 files changed, 439 insertions(+), 3 deletions(-) create mode 100644 pkg/tools/shell/env.go create mode 100644 pkg/tools/shell/env_test.go create mode 100644 pkg/tools/shell/env_windows_test.go diff --git a/docs/tools_configuration.md b/docs/tools_configuration.md index e64a3a107..f1d11dfb5 100644 --- a/docs/tools_configuration.md +++ b/docs/tools_configuration.md @@ -52,10 +52,35 @@ The exec tool is used to execute shell commands. | `enable_deny_patterns` | bool | true | Enable default dangerous command blocking | | `custom_deny_patterns` | array | [] | Custom deny patterns (regular expressions) | -### Functionality +### Environment Sanitization -- **`enable_deny_patterns`**: Set to `false` to completely disable the default dangerous command blocking patterns -- **`custom_deny_patterns`**: Add custom deny regex patterns; commands matching these will be blocked +The exec tool sanitizes the environment passed to child processes: + +1. **Default allowlist** — Only these variables are inherited from the parent process: + - `PATH`, `HOME`, `USER`, `LANG`, `SHELL`, `TERM`, `PWD`, `OLDPWD`, `HOSTNAME`, `LOGNAME`, `TZ`, `DISPLAY`, `TMPDIR`, `EDITOR`, `PAGER` + - Plus: `PICOCLAW_CONFIG`, `PICOCLAW_HOME`, `PICOCLAW_SERVICE_NAME`, `PICOCLAW_EXE`, `PICOCLAW_WORKSPACE` + - Plus: `HTTP_PROXY`, `HTTPS_PROXY`, `NO_PROXY` + - Plus any variable starting with `LC_` + +2. **Config env_set** — Variables from config are merged (can override inherited values) + +3. **LLM env injection** — The LLM can inject additional variables per-call via the `env` parameter: + + ```json + { + "name": "exec", + "arguments": { + "command": "echo $DEBUG_MODE", + "env": { + "DEBUG_MODE": "true" + } + } + } + ``` + + **Blocked variables** — The LLM cannot override these sensitive variables: + - `PATH`, `HOME`, `USER`, `LOGNAME`, `SHELL` + - `LD_PRELOAD`, `LD_LIBRARY_PATH`, `LD_AUDIT`, `LD_DEBUG` ### Default Blocked Command Patterns diff --git a/pkg/tools/shell.go b/pkg/tools/shell.go index b8a811d03..6d8b786cb 100644 --- a/pkg/tools/shell.go +++ b/pkg/tools/shell.go @@ -14,6 +14,7 @@ import ( "time" "github.com/sipeed/picoclaw/pkg/config" + "github.com/sipeed/picoclaw/pkg/tools/shell" ) type ExecTool struct { @@ -23,6 +24,7 @@ type ExecTool struct { allowPatterns []*regexp.Regexp customAllowPatterns []*regexp.Regexp restrictToWorkspace bool + cachedEnv []string // cached sanitized env from os.Environ() at init } var ( @@ -143,6 +145,7 @@ func NewExecToolWithConfig(workingDir string, restrict bool, config *config.Conf allowPatterns: nil, customAllowPatterns: customAllowPatterns, restrictToWorkspace: restrict, + cachedEnv: shell.BuildSanitizedEnv(os.Environ(), nil, nil, nil), }, nil } @@ -217,6 +220,10 @@ func (t *ExecTool) Execute(ctx context.Context, args map[string]any) *ToolResult } else { cmd = exec.CommandContext(cmdCtx, "sh", "-c", command) } + + // Use sanitized environment - strips secrets, prevents env-based attacks + cmd.Env = t.cachedEnv + if cwd != "" { cmd.Dir = cwd } diff --git a/pkg/tools/shell/env.go b/pkg/tools/shell/env.go new file mode 100644 index 000000000..b259ba645 --- /dev/null +++ b/pkg/tools/shell/env.go @@ -0,0 +1,156 @@ +package shell + +import ( + "os" + "runtime" + "strings" +) + +// DefaultEnvAllowlist is the set of environment variable names that are safe +// to propagate to child processes. Everything else is stripped to prevent +// accidental credential leakage. +// +// To add a new variable: +// 1. Add to this map if it's safe to pass through +// 2. Or add a prefix to defaultEnvAllowPrefixes for pattern matching +// Note: Do NOT add wildcard patterns like "*_API_KEY" here - use explicit names +// to avoid accidentally leaking secrets. +var DefaultEnvAllowlist = map[string]bool{ + "PATH": true, + "HOME": true, + "USER": true, + "LANG": true, + "SHELL": true, + "TERM": true, + "PWD": true, + "OLDPWD": true, + "HOSTNAME": true, + "LOGNAME": true, + "TZ": true, + "DISPLAY": true, + "TMPDIR": true, + "EDITOR": true, + "PAGER": true, + "HTTP_PROXY": true, + "HTTPS_PROXY": true, + "NO_PROXY": true, + // PICOCLAW_* vars - inherited from parent process if set + "PICOCLAW_HOME": true, + "PICOCLAW_CONFIG": true, + "PICOCLAW_SERVICE_NAME": true, + "PICOCLAW_EXE": true, + "PICOCLAW_WORKSPACE": true, +} + +// defaultEnvAllowPrefixes are env var prefixes that are always allowed. +var defaultEnvAllowPrefixes = []string{ + "LC_", +} + +// LLMBlocklist is the set of environment variable names that the LLM +// cannot override, even if passed via the env parameter. These vars +// control fundamental process behavior and could be exploited. +var LLMBlocklist = map[string]bool{ + "PATH": true, // Could hijack command resolution + "HOME": true, // Could redirect file access + "USER": true, // Could impersonate user + "LOGNAME": true, // Could impersonate user + "SHELL": true, // Could change shell behavior + "LD_PRELOAD": true, // Could inject code + "LD_LIBRARY_PATH": true, // Could hijack library resolution + "LD_AUDIT": true, // Could inject code + "LD_DEBUG": true, // Could leak info +} + +// windowsEnvAllowlist contains additional variables needed on Windows. +var windowsEnvAllowlist = map[string]bool{ + "PATHEXT": true, + "SYSTEMROOT": true, + "SYSTEMDRIVE": true, + "COMSPEC": true, + "APPDATA": true, + "USERPROFILE": true, + "HOMEDRIVE": true, + "HOMEPATH": true, +} + +// BuildSanitizedEnv constructs a sanitized environment []string suitable for +// exec.Cmd.Env. It filters the inherited environment to only allowlisted variables. +// +// baseEnv is the inherited environment (e.g., from os.Environ() or cached). +// If nil, os.Environ() will be used for backwards compatibility. +// extraAllowlist adds additional variable names to the default allowlist. +// envSet provides explicit key=value pairs from config (override inherited). +// extraEnv provides additional key=value pairs from tool call (merged with envSet). +func BuildSanitizedEnv(baseEnv []string, extraAllowlist []string, envSet, extraEnv map[string]string) []string { + // Use provided env or fall back to os.Environ + inherited := baseEnv + if inherited == nil { + inherited = os.Environ() + } + + allowed := make(map[string]bool, len(DefaultEnvAllowlist)+len(extraAllowlist)+len(windowsEnvAllowlist)) + for k := range DefaultEnvAllowlist { + allowed[envKey(k)] = true + } + if runtime.GOOS == "windows" { + for k := range windowsEnvAllowlist { + allowed[envKey(k)] = true + } + } + for _, k := range extraAllowlist { + allowed[envKey(k)] = true + } + + vars := make(map[string]string, len(allowed)+len(envSet)+len(extraEnv)) + + for _, entry := range inherited { + k, v, ok := strings.Cut(entry, "=") + if !ok { + continue + } + norm := envKey(k) + if allowed[norm] || isAllowedPrefix(norm) { + vars[norm] = v + } + } + + for k, v := range envSet { + vars[envKey(k)] = v + } + + // Merge extraEnv (tool call) - highest priority + // Filter against LLM blocklist to prevent override of sensitive vars + for k, v := range extraEnv { + if LLMBlocklist[envKey(k)] { + continue // Skip blocked vars + } + vars[envKey(k)] = v + } + + // Convert to []string for exec.Cmd.Env + result := make([]string, 0, len(vars)) + for k, v := range vars { + result = append(result, k+"="+v) + } + return result +} + +// envKey normalizes an environment variable name. On Windows, where env +// vars are case-insensitive, it uppercases the key so that "Path" and +// "PATH" map to the same entry. On other platforms it's a no-op. +func envKey(k string) string { + if runtime.GOOS == "windows" { + return strings.ToUpper(k) + } + return k +} + +func isAllowedPrefix(name string) bool { + for _, prefix := range defaultEnvAllowPrefixes { + if strings.HasPrefix(name, prefix) { + return true + } + } + return false +} diff --git a/pkg/tools/shell/env_test.go b/pkg/tools/shell/env_test.go new file mode 100644 index 000000000..7c24dc727 --- /dev/null +++ b/pkg/tools/shell/env_test.go @@ -0,0 +1,180 @@ +package shell + +import ( + "strings" + "testing" +) + +func TestBuildSanitizedEnv_FiltersSecrets(t *testing.T) { + // Set some secret env vars. + t.Setenv("OPENAI_API_KEY", "sk-secret") + t.Setenv("ANTHROPIC_API_KEY", "anthro-secret") + t.Setenv("AWS_SECRET_ACCESS_KEY", "aws-secret") + t.Setenv("DATABASE_URL", "postgres://secret") + // Set something that should pass. + t.Setenv("PATH", "/usr/bin") + t.Setenv("HOME", "/home/test") + t.Setenv("LC_ALL", "en_US.UTF-8") + + env := BuildSanitizedEnv(nil, nil, nil, nil) + + assertEnvPresent(t, env, "PATH") + assertEnvPresent(t, env, "HOME") + assertEnvPresent(t, env, "LC_ALL") + + assertEnvAbsent(t, env, "OPENAI_API_KEY") + assertEnvAbsent(t, env, "ANTHROPIC_API_KEY") + assertEnvAbsent(t, env, "AWS_SECRET_ACCESS_KEY") + assertEnvAbsent(t, env, "DATABASE_URL") +} + +func TestBuildSanitizedEnv_ExtraAllowlist(t *testing.T) { + t.Setenv("MY_CUSTOM_VAR", "hello") + + env := BuildSanitizedEnv(nil, []string{"MY_CUSTOM_VAR"}, nil, nil) + assertEnvPresent(t, env, "MY_CUSTOM_VAR") +} + +func TestBuildSanitizedEnv_EnvSet(t *testing.T) { + env := BuildSanitizedEnv(nil, nil, map[string]string{ + "INJECTED": "value123", + }, nil) + + v := getEnvValue(env, "INJECTED") + if v == "" { + t.Fatal("expected INJECTED to be present") + } + if v != "value123" { + t.Errorf("INJECTED = %q, want %q", v, "value123") + } +} + +func TestBuildSanitizedEnv_EnvSetOverridesInherited(t *testing.T) { + t.Setenv("PATH", "/original") + + env := BuildSanitizedEnv(nil, nil, map[string]string{ + "PATH": "/overridden", + }, nil) + + v := getEnvValue(env, "PATH") + if v != "/overridden" { + t.Errorf("PATH = %q, want %q", v, "/overridden") + } +} + +func TestBuildSanitizedEnv_DefaultAllowlist(t *testing.T) { + for name := range DefaultEnvAllowlist { + t.Setenv(name, "test-"+name) + } + + env := BuildSanitizedEnv(nil, nil, nil, nil) + + for name := range DefaultEnvAllowlist { + v := getEnvValue(env, name) + if v == "" { + t.Errorf("expected %s to be in sanitized env", name) + } + } +} + +func TestBuildSanitizedEnv_ReturnsSlice(t *testing.T) { + env := BuildSanitizedEnv(nil, nil, map[string]string{ + "TEST_A": "a", + "TEST_B": "b", + }, nil) + + if env == nil { + t.Fatal("env should not be nil") + } + + // Should be a slice + if len(env) == 0 { + t.Error("expected non-empty env slice") + } + + // Check format + found := make(map[string]string) + for _, entry := range env { + k, v, ok := strings.Cut(entry, "=") + if !ok { + t.Errorf("invalid env entry: %q", entry) + continue + } + found[k] = v + } + + if found["TEST_A"] != "a" { + t.Errorf("TEST_A = %q, want %q", found["TEST_A"], "a") + } + if found["TEST_B"] != "b" { + t.Errorf("TEST_B = %q, want %q", found["TEST_B"], "b") + } +} + +func TestLLMBlocklist_BlocksSensitiveVars(t *testing.T) { + // Set up inherited env + t.Setenv("PATH", "/original/path") + t.Setenv("HOME", "/original/home") + + // LLM tries to override these via extraEnv + extraEnv := map[string]string{ + "PATH": "/malicious/path", + "HOME": "/etc", + "LD_PRELOAD": "/evil.so", + "MY_CUSTOM_VAR": "allowed", // Not blocked + } + + env := BuildSanitizedEnv(nil, nil, nil, extraEnv) + + // Blocked vars should keep their inherited value, not the LLM override + v := getEnvValue(env, "PATH") + if v != "/original/path" { + t.Errorf("PATH should be /original/path, got %q - LLM override was not blocked", v) + } + + v = getEnvValue(env, "HOME") + if v != "/original/home" { + t.Errorf("HOME should be /original/home, got %q - LLM override was not blocked", v) + } + + // LD_PRELOAD was not in inherited, so should still be absent (blocked) + v = getEnvValue(env, "LD_PRELOAD") + if v != "" { + t.Error("LD_PRELOAD should be blocked by LLMBlocklist") + } + + // Non-blocked var should be present + v = getEnvValue(env, "MY_CUSTOM_VAR") + if v == "" { + t.Error("MY_CUSTOM_VAR should be allowed") + } + if v != "allowed" { + t.Errorf("MY_CUSTOM_VAR = %q, want %q", v, "allowed") + } +} + +// Helper to get value from env slice +func getEnvValue(env []string, name string) string { + prefix := name + "=" + for _, entry := range env { + if strings.HasPrefix(entry, prefix) { + return strings.TrimPrefix(entry, prefix) + } + } + return "" +} + +func assertEnvPresent(t *testing.T, env []string, name string) { + t.Helper() + if getEnvValue(env, name) == "" { + t.Errorf("expected %s to be present in sanitized env", name) + } +} + +func assertEnvAbsent(t *testing.T, env []string, name string) { + t.Helper() + v := getEnvValue(env, name) + if v != "" { + t.Errorf("expected %s to be absent from sanitized env, got %q", name, v) + } +} diff --git a/pkg/tools/shell/env_windows_test.go b/pkg/tools/shell/env_windows_test.go new file mode 100644 index 000000000..3395d8db5 --- /dev/null +++ b/pkg/tools/shell/env_windows_test.go @@ -0,0 +1,68 @@ +package shell + +import ( + "runtime" + "testing" +) + +func TestBuildSanitizedEnv_WindowsCaseInsensitive(t *testing.T) { + // Skip on non-Windows + if runtime.GOOS != "windows" { + t.Skip("Windows-specific test") + } + + // On Windows the OS typically stores "Path" not "PATH". + // Verify that mixed-case inherited vars still pass the allowlist. + t.Setenv("Path", `C:\Windows\system32`) + + env := BuildSanitizedEnv(nil, nil, nil, nil) + + v := getEnvValue(env, "PATH") + if v == "" { + t.Fatal("expected PATH to be present when OS provides 'Path'") + } + if v != `C:\Windows\system32` { + t.Errorf("PATH = %q, want %q", v, `C:\Windows\system32`) + } + + // Also verify lookup with original casing works. + v2 := getEnvValue(env, "Path") + if v2 == "" { + t.Fatal("expected Get('Path') to resolve via case-insensitive lookup") + } +} + +func TestBuildSanitizedEnv_WindowsEnvSetCaseInsensitive(t *testing.T) { + // Skip on non-Windows + if runtime.GOOS != "windows" { + t.Skip("Windows-specific test") + } + + env := BuildSanitizedEnv(nil, nil, map[string]string{ + "path": `C:\custom\bin`, + }, nil) + + v := getEnvValue(env, "PATH") + if v == "" { + t.Fatal("expected PATH to be set via lowercase 'path' envSet key") + } + if v != `C:\custom\bin` { + t.Errorf("PATH = %q, want %q", v, `C:\custom\bin`) + } +} + +func TestBuildSanitizedEnv_WindowsExtraAllowlistCaseInsensitive(t *testing.T) { + // Skip on non-Windows + if runtime.GOOS != "windows" { + t.Skip("Windows-specific test") + } + + t.Setenv("my_custom_var", "hello") + + env := BuildSanitizedEnv(nil, []string{"MY_CUSTOM_VAR"}, nil, nil) + + v := getEnvValue(env, "my_custom_var") + if v == "" { + t.Fatal("expected my_custom_var to be found via case-insensitive allowlist") + } +}