diff --git a/docs/tools_configuration.md b/docs/tools_configuration.md index 2e0a22d3b..dc5feb298 100644 --- a/docs/tools_configuration.md +++ b/docs/tools_configuration.md @@ -94,10 +94,38 @@ PICOCLAW_TOOLS_EXEC_ENABLED=false > **Note:** When disabled, the agent will not be able to execute shell commands. This also affects the Cron tool's ability to run scheduled shell commands. -### 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: `HTTP_PROXY`, `HTTPS_PROXY`, `NO_PROXY` + - Plus locale: `LC_ALL`, `LC_CTYPE`, `LC_MESSAGES`, `LC_MONETARY`, `LC_NUMERIC`, `LC_TIME`, `LC_PAPER`, `LC_NAME`, `LC_ADDRESS`, `LC_TELEPHONE`, `LC_MEASUREMENT`, `LC_IDENTIFICATION`, `LC_COLLATE` + - Plus: `PICOCLAW_HOME`, `PICOCLAW_CONFIG`, `PICOCLAW_AGENT_WORKSPACE`, `PICOCLAW_EXE`, `PICOCLAW_SERVICE_NAME`, `PICOCLAW_EXEC_TIME`, `PICOCLAW_EXEC_TIMEOUT` + +2. **Config env_set** — Variables from config are merged (can override inherited values) + +3. **Config env_allowlist** — Additional explicit variable names to allow (extends default) + +4. **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` + - All `PICOCLAW_*` variables ### Default Blocked Command Patterns diff --git a/pkg/config/config.go b/pkg/config/config.go index 33a5db8ae..d26f9d9d1 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -751,12 +751,14 @@ type CronToolsConfig struct { } type ExecConfig struct { - ToolConfig ` envPrefix:"PICOCLAW_TOOLS_EXEC_"` - EnableDenyPatterns bool ` env:"PICOCLAW_TOOLS_EXEC_ENABLE_DENY_PATTERNS" json:"enable_deny_patterns"` - AllowRemote bool ` env:"PICOCLAW_TOOLS_EXEC_ALLOW_REMOTE" json:"allow_remote"` - CustomDenyPatterns []string ` env:"PICOCLAW_TOOLS_EXEC_CUSTOM_DENY_PATTERNS" json:"custom_deny_patterns"` - CustomAllowPatterns []string ` env:"PICOCLAW_TOOLS_EXEC_CUSTOM_ALLOW_PATTERNS" json:"custom_allow_patterns"` - TimeoutSeconds int ` env:"PICOCLAW_TOOLS_EXEC_TIMEOUT_SECONDS" json:"timeout_seconds"` // 0 means use default (60s) + ToolConfig ` envPrefix:"PICOCLAW_TOOLS_EXEC_"` + EnableDenyPatterns bool ` env:"PICOCLAW_TOOLS_EXEC_ENABLE_DENY_PATTERNS" json:"enable_deny_patterns"` + AllowRemote bool ` env:"PICOCLAW_TOOLS_EXEC_ALLOW_REMOTE" json:"allow_remote"` + CustomDenyPatterns []string ` env:"PICOCLAW_TOOLS_EXEC_CUSTOM_DENY_PATTERNS" json:"custom_deny_patterns"` + CustomAllowPatterns []string ` env:"PICOCLAW_TOOLS_EXEC_CUSTOM_ALLOW_PATTERNS" json:"custom_allow_patterns"` + TimeoutSeconds int ` env:"PICOCLAW_TOOLS_EXEC_TIMEOUT_SECONDS" json:"timeout_seconds"` // 0 means use default (60s) + EnvSet map[string]string ` json:"env_set"` // env vars to set for all exec commands + EnvAllowlist []string ` json:"env_allowlist"` // additional env vars to allow (extends default) - use explicit names, not wildcards } type SkillsToolsConfig struct { diff --git a/pkg/tools/shell.go b/pkg/tools/shell.go index 78ad2b26d..bb0a769e4 100644 --- a/pkg/tools/shell.go +++ b/pkg/tools/shell.go @@ -15,6 +15,7 @@ import ( "github.com/sipeed/picoclaw/pkg/config" "github.com/sipeed/picoclaw/pkg/constants" + "github.com/sipeed/picoclaw/pkg/tools/shell" ) type ExecTool struct { @@ -26,6 +27,7 @@ type ExecTool struct { allowedPathPatterns []*regexp.Regexp restrictToWorkspace bool allowRemote bool + cachedEnv map[string]string // cached sanitized env from os.Getenv() at init } var ( @@ -150,6 +152,22 @@ func NewExecToolWithConfig( timeout = time.Duration(config.Tools.Exec.TimeoutSeconds) * time.Second } + // Get envSet and envAllowlist from config (if provided) + var envSet map[string]string + var envAllowlist []string + if config != nil && config.Tools.Exec.EnvSet != nil { + envSet = config.Tools.Exec.EnvSet + } + if config != nil && config.Tools.Exec.EnvAllowlist != nil { + envAllowlist = config.Tools.Exec.EnvAllowlist + } + + // Ensure PICOCLAW_* vars are set for child processes + envSet = shell.WithPicoclawEnvVars(envSet, workingDir) + + // Build cached env: start with envSet (PICOCLAW_*), then add allowed inherited vars + cachedEnv := shell.WithAllowedEnv(envSet, envAllowlist) + return &ExecTool{ workingDir: workingDir, timeout: timeout, @@ -158,7 +176,11 @@ func NewExecToolWithConfig( customAllowPatterns: customAllowPatterns, allowedPathPatterns: allowedPathPatterns, restrictToWorkspace: restrict, +<<<<<<< HEAD allowRemote: allowRemote, +======= + cachedEnv: cachedEnv, +>>>>>>> 4b11ef32fe1c501baafc615da77c63b952339684 }, nil } @@ -182,6 +204,13 @@ func (t *ExecTool) Parameters() map[string]any { "type": "string", "description": "Optional working directory for the command", }, + "env": map[string]any{ + "type": "object", + "description": "Additional environment variables to set for this command. Available: PICOCLAW_HOME, PICOCLAW_CONFIG, PICOCLAW_AGENT_WORKSPACE, PICOCLAW_EXE, PICOCLAW_SERVICE_NAME, PICOCLAW_EXEC_TIME (RFC3339), PICOCLAW_EXEC_TIMEOUT. Cannot override: PATH, HOME, USER, LOGNAME, SHELL, LD_PRELOAD, LD_LIBRARY_PATH, LD_AUDIT, LD_DEBUG, PICOCLAW_*", + "additionalProperties": map[string]any{ + "type": "string", + }, + }, }, "required": []string{"command"}, } @@ -269,6 +298,28 @@ func (t *ExecTool) Execute(ctx context.Context, args map[string]any) *ToolResult } else { cmd = exec.CommandContext(cmdCtx, "sh", "-c", command) } + + // Parse env param from LLM (if provided) + var extraEnv map[string]string + if envArg, ok := args["env"].(map[string]any); ok && envArg != nil { + extraEnv = make(map[string]string) + for k, v := range envArg { + if strVal, ok := v.(string); ok { + extraEnv[k] = strVal + } + } + } + + // Add PICOCLAW_EXEC_TIME - timestamp when command is executed + execTimeEnv := map[string]string{ + "PICOCLAW_EXEC_TIME": time.Now().Format(time.RFC3339), + "PICOCLAW_EXEC_TIMEOUT": t.timeout.String(), + } + + // Use sanitized environment - merge cached env with exec time vars and LLM extra env + // Note: cachedEnv is NOT re-filtered - PICOCLAW_* vars are preserved + cmd.Env = shell.MapToEnvSlice(shell.MergeEnvVars(t.cachedEnv, execTimeEnv, extraEnv)) + 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..49739f418 --- /dev/null +++ b/pkg/tools/shell/env.go @@ -0,0 +1,239 @@ +package shell + +import ( + "os" + "path/filepath" + "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, + + // Locale + "LC_ALL": true, + "LC_CTYPE": true, + "LC_MESSAGES": true, + "LC_MONETARY": true, + "LC_NUMERIC": true, + "LC_TIME": true, + "LC_PAPER": true, + "LC_NAME": true, + "LC_ADDRESS": true, + "LC_TELEPHONE": true, + "LC_MEASUREMENT": true, + "LC_IDENTIFICATION": true, + "LC_COLLATE": true, +} + +// 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 + + // PICOCLAW_* vars - controlled by the agent, not LLM + "PICOCLAW_HOME": true, + "PICOCLAW_CONFIG": true, + "PICOCLAW_AGENT_WORKSPACE": true, + "PICOCLAW_EXE": true, + "PICOCLAW_SERVICE_NAME": true, + "PICOCLAW_EXEC_TIME": true, + "PICOCLAW_EXEC_TIMEOUT": true, +} + +// 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, +} + +// WithAllowedEnv builds a map of allowed environment variables by looking them up. +// This is more efficient than filtering os.Environ() with string parsing. +// It starts with the provided env map, then adds allowed inherited vars (if not set). +// extraAllowlist adds to the default allowlist. +func WithAllowedEnv(envSet map[string]string, extraAllowlist []string) map[string]string { + // Start with provided envSet map + result := envSet + if result == nil { + result = make(map[string]string) + } + + // Add default allowlist (only if not already set) + for k := range DefaultEnvAllowlist { + if _, exists := result[k]; !exists { + if val := os.Getenv(k); val != "" { + result[k] = val + } + } + } + // Add Windows-specific vars + if runtime.GOOS == "windows" { + for k := range windowsEnvAllowlist { + if _, exists := result[k]; !exists { + if val := os.Getenv(k); val != "" { + result[k] = val + } + } + } + } + // Add extra allowlist from config + for _, k := range extraAllowlist { + if _, exists := result[k]; !exists { + if val := os.Getenv(k); val != "" { + result[k] = val + } + } + } + + return result +} + +// LLMBlocklistPrefixes are env var prefixes that the LLM cannot override. +var LLMBlocklistPrefixes = []string{ + "PICOCLAW_", +} + +// isBlocked returns true if the key is in the blocklist or matches a blocked prefix. +func isBlocked(key string) bool { + norm := envKey(key) + if LLMBlocklist[norm] { + return true + } + for _, prefix := range LLMBlocklistPrefixes { + if strings.HasPrefix(norm, prefix) { + return true + } + } + return false +} + +// MergeEnvVars merges multiple env sources into a map. +// baseEnv is the cached map from AllowedEnv. +// envSet provides explicit key=value pairs (config, not filtered). +// extraEnv provides additional key=value pairs from LLM (filtered by blocklist). +func MergeEnvVars(baseEnv map[string]string, envSet, extraEnv map[string]string) map[string]string { + vars := make(map[string]string, len(baseEnv)+len(envSet)+len(extraEnv)) + + // Start with base env (already filtered) + for k, v := range baseEnv { + vars[envKey(k)] = v + } + + // Add envSet (config-provided, not filtered) + if envSet != nil { + for k, v := range envSet { + vars[envKey(k)] = v + } + } + + // Merge extraEnv (LLM-provided) - filtered by blocklist + if extraEnv != nil { + for k, v := range extraEnv { + if isBlocked(k) { + continue // Skip blocked vars + } + vars[envKey(k)] = v + } + } + + return vars +} + +// MapToEnvSlice converts a map of environment variables to a []string +// in the format "KEY=value" suitable for exec.Cmd.Env. +func MapToEnvSlice(vars map[string]string) []string { + 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 +} + +// WithPicoclawEnvVars ensures PICOCLAW_* vars are set in envSet. +// These are needed for child processes to locate config, workspace, etc. +func WithPicoclawEnvVars(envSet map[string]string, workspace string) map[string]string { + if envSet == nil { + envSet = make(map[string]string) + } + + // Always compute PICOCLAW_* vars - priority: env var > default + if v := os.Getenv("PICOCLAW_HOME"); v != "" { + envSet["PICOCLAW_HOME"] = v + } else if home, _ := os.UserHomeDir(); home != "" { + envSet["PICOCLAW_HOME"] = filepath.Join(home, ".picoclaw") + } + + if v := os.Getenv("PICOCLAW_CONFIG"); v != "" { + envSet["PICOCLAW_CONFIG"] = v + } else if home := envSet["PICOCLAW_HOME"]; home != "" { + envSet["PICOCLAW_CONFIG"] = filepath.Join(home, "config.json") + } + + // Workspace - this is the agent's working directory + if workspace != "" { + envSet["PICOCLAW_AGENT_WORKSPACE"] = workspace + } + + if exe, err := os.Executable(); err == nil { + envSet["PICOCLAW_EXE"] = exe + } + + if v := os.Getenv("PICOCLAW_SERVICE_NAME"); v != "" { + envSet["PICOCLAW_SERVICE_NAME"] = v + } else { + envSet["PICOCLAW_SERVICE_NAME"] = "picoclaw" + } + + return envSet +}