From 9510ba684437eadef2959f449e895ab4c477f380 Mon Sep 17 00:00:00 2001 From: essence-of-the-soul Date: Wed, 5 Aug 2026 11:24:51 +0000 Subject: [PATCH] =?UTF-8?q?fix(IK0XP6):=20=E7=BB=9F=E4=B8=80=20SecureEnv?= =?UTF-8?q?=20=E4=B8=8E=20dangerousVariablePatterns=20=E5=8D=B1=E9=99=A9?= =?UTF-8?q?=E5=8F=98=E9=87=8F=E6=B8=85=E5=8D=95?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 将 SecureEnv 的危险变量过滤集改为从 bash_security.go 的 dangerousEnvVarNames 单一数据源派生，避免两处手工维护导致的漂移。 新增 secureEnvInheritedOnlyDangerousVars 承载 CDPATH/GLOBIGNORE/IFS 等仅继承态过滤、内联合法的变量。 PATH 不再原样继承，改为重置为受控安全默认值 (/usr/local/bin:/usr/bin:/bin)，既防止命令劫持，又保证子进程能 找到基础工具。PYTHONPATH/NODE_PATH/GOPATH/PERLLIB/RUBYLIB 及 PROMPT_COMMAND 等搜索路径/自动执行钩子变量统一过滤。 测试同步更新：覆盖新增过滤变量与 PATH 重置语义。 关联 issue IK0XP6 --- internal/tools/bash_security.go | 68 ++++++++++++------------ internal/tools/secure_exec.go | 85 ++++++++++++++++++------------ internal/tools/secure_exec_test.go | 43 ++++++++++++--- 3 files changed, 124 insertions(+), 72 deletions(-) diff --git a/internal/tools/bash_security.go b/internal/tools/bash_security.go index 369121e..34ef282 100644 --- a/internal/tools/bash_security.go +++ b/internal/tools/bash_security.go @@ -271,49 +271,51 @@ func checkCommandSubstitution(command string) *SecurityViolation { return nil } -// dangerousVariablePatterns detect potentially dangerous environment variable manipulations. +// dangerousEnvVarNames is the canonical list of environment variable names +// that are dangerous to set inline in a command (binary/library/interpreter +// hijack or behavior manipulation). It is the single source of truth for: +// - dangerousVariablePatterns (inline `VAR=val` detection, see below) +// - SecureEnv (filtering inherited env, see secure_exec.go) // -// SPEC-A2 / A12: curated list of binary-hijack env vars. Any -// match — whether inline (`VAR=val cmd`) or via `export VAR=val` — is -// treated as severe, which Step 0 of the permission pipeline hard-blocks. +// SPEC-A2 / A12: any of these — whether set inline (`VAR=val cmd`) or via +// `export VAR=val` — is treated as severe and hard-blocked by Step 0 of the +// permission pipeline. SAFE counterparts (LANG, LC_*, TZ, HOME, ...) are +// deliberately not listed; they are normalized away by +// permissions.NormalizeForRuleMatch. // -// SAFE counterparts (LANG, LC_*, TZ, HOME, ...) are deliberately not -// listed; they are normalized away by permissions.NormalizeForRuleMatch. -var dangerousVariablePatterns = []*regexp.Regexp{ +// Note: CDPATH / GLOBIGNORE / IFS are filtered by SecureEnv from the inherited +// environment but intentionally NOT blocked inline — `IFS= read -r line` and +// similar idioms are legitimate. See secureEnvInheritedOnlyDangerousVars. +var dangerousEnvVarNames = []string{ // Shared-library / loader hijacking. - regexp.MustCompile(`(?i)\bLD_PRELOAD\s*=`), - regexp.MustCompile(`(?i)\bLD_LIBRARY_PATH\s*=`), - regexp.MustCompile(`(?i)\bLD_AUDIT\s*=`), + "LD_PRELOAD", "LD_LIBRARY_PATH", "LD_AUDIT", // macOS dyld family (A12). Not exploitable on Linux today, but - // trivially so if a developer runs ocai-agent on macOS — defense in - // depth. - regexp.MustCompile(`(?i)\bDYLD_INSERT_LIBRARIES\s*=`), - regexp.MustCompile(`(?i)\bDYLD_LIBRARY_PATH\s*=`), - regexp.MustCompile(`(?i)\bDYLD_FALLBACK_LIBRARY_PATH\s*=`), - regexp.MustCompile(`(?i)\bDYLD_FRAMEWORK_PATH\s*=`), - regexp.MustCompile(`(?i)\bDYLD_FALLBACK_FRAMEWORK_PATH\s*=`), - // PATH manipulation (inline PATH= can redirect every subsequent + // trivially so if a developer runs ocai-agent on macOS — defense in depth. + "DYLD_INSERT_LIBRARIES", "DYLD_LIBRARY_PATH", "DYLD_FALLBACK_LIBRARY_PATH", + "DYLD_FRAMEWORK_PATH", "DYLD_FALLBACK_FRAMEWORK_PATH", + // Search-path manipulation (inline PATH= can redirect every subsequent // command lookup to an attacker-controlled dir). - regexp.MustCompile(`(?i)\bPATH\s*=`), - regexp.MustCompile(`(?i)\bPYTHONPATH\s*=`), - regexp.MustCompile(`(?i)\bNODE_PATH\s*=`), - regexp.MustCompile(`(?i)\bGOPATH\s*=`), - regexp.MustCompile(`(?i)\bPERLLIB\s*=`), - regexp.MustCompile(`(?i)\bRUBYLIB\s*=`), + "PATH", "PYTHONPATH", "NODE_PATH", "GOPATH", "PERLLIB", "RUBYLIB", + // Shell behavior / auto-exec hooks. + "PROMPT_COMMAND", "BASH_ENV", "ENV", // Interpreter auto-exec hooks. Any of these lets an attacker run // arbitrary code the moment the interpreter starts. - regexp.MustCompile(`(?i)\bPROMPT_COMMAND\s*=`), - regexp.MustCompile(`(?i)\bBASH_ENV\s*=`), - regexp.MustCompile(`(?i)\bENV\s*=`), - regexp.MustCompile(`(?i)\bNODE_OPTIONS\s*=`), - regexp.MustCompile(`(?i)\bPERL5OPT\s*=`), - regexp.MustCompile(`(?i)\bRUBYOPT\s*=`), - regexp.MustCompile(`(?i)\bPYTHONINSPECT\s*=`), - regexp.MustCompile(`(?i)\bPYTHONSTARTUP\s*=`), + "NODE_OPTIONS", "PERL5OPT", "RUBYOPT", "PYTHONINSPECT", "PYTHONSTARTUP", // BROWSER can hijack xdg-open / git's browser-opening paths. - regexp.MustCompile(`(?i)\bBROWSER\s*=`), + "BROWSER", } +// dangerousVariablePatterns detect potentially dangerous environment variable +// manipulations. Derived from dangerousEnvVarNames so the inline checker and +// SecureEnv never drift out of sync. +var dangerousVariablePatterns = func() []*regexp.Regexp { + pats := make([]*regexp.Regexp, 0, len(dangerousEnvVarNames)) + for _, name := range dangerousEnvVarNames { + pats = append(pats, regexp.MustCompile(`(?i)\b`+regexp.QuoteMeta(name)+`\s*=`)) + } + return pats +}() + func checkDangerousVariables(command string) *SecurityViolation { for _, p := range dangerousVariablePatterns { if p.MatchString(command) { diff --git a/internal/tools/secure_exec.go b/internal/tools/secure_exec.go index 2707537..8b3ddc2 100644 --- a/internal/tools/secure_exec.go +++ b/internal/tools/secure_exec.go @@ -128,52 +128,66 @@ func SafeCreateTemp(dir, pattern string) (*os.File, error) { return os.CreateTemp(realDir, pattern) } +// secureEnvInheritedOnlyDangerousVars lists variables that are dangerous to +// *inherit* from the parent shell but are legitimate to set inline in some +// idioms (e.g. `IFS= read -r line`). These are filtered by SecureEnv from +// the inherited environment but intentionally NOT blocked inline by the +// command security analyzer. +// +// Members here are ADDED on top of dangerousEnvVarNames when building the +// SecureEnv filter set. +var secureEnvInheritedOnlyDangerousVars = []string{ + "CDPATH", // can cause cd to go to unexpected directories + "GLOBIGNORE", // can affect glob behavior + "IFS", // internal field separator manipulation +} + +// safeDefaultPath is the controlled PATH SecureEnv resets to when the caller +// supplied a PATH. Completely dropping PATH would leave the child unable to +// locate even basic utilities, so we reset to a minimal trusted search path +// instead of filtering (per IK0XP6 maintainer guidance). +const safeDefaultPath = "/usr/local/bin:/usr/bin:/bin" + // SecureEnv returns a sanitized set of environment variables for command // execution. It sets security-relevant variables to safe values. +// +// The dangerous-variable set is derived from the single source of truth +// `dangerousEnvVarNames` (bash_security.go) plus `secureEnvInheritedOnlyDangerousVars`, +// so the inline command analyzer and this inherited-env filter never drift +// out of sync (IK0XP6). PATH is special-cased: rather than being dropped +// (which would break command lookup), it is reset to safeDefaultPath. func SecureEnv(baseEnv []string) []string { result := make([]string, 0, len(baseEnv)+3) - // Filter dangerous environment variables. - // - // SPEC-A2 / A12: this list must stay in sync with - // bash_security.go::dangerousVariablePatterns. The security - // analyzer rejects commands that set these inline; this filter - // ensures even our own spawned children never inherit them from - // the parent shell. - dangerousVars := map[string]bool{ - // Loader hijacking. - "LD_PRELOAD": true, - "LD_LIBRARY_PATH": true, - "LD_AUDIT": true, - // macOS dyld family (A12). - "DYLD_INSERT_LIBRARIES": true, - "DYLD_LIBRARY_PATH": true, - "DYLD_FALLBACK_LIBRARY_PATH": true, - "DYLD_FRAMEWORK_PATH": true, - "DYLD_FALLBACK_FRAMEWORK_PATH": true, - // Auto-exec hooks. - "BASH_ENV": true, - "ENV": true, - "NODE_OPTIONS": true, - "PERL5OPT": true, - "RUBYOPT": true, - "PYTHONINSPECT": true, - "PYTHONSTARTUP": true, - // Shell behavior manipulation. - "CDPATH": true, // can cause cd to go to unexpected directories - "GLOBIGNORE": true, // can affect glob behavior - "IFS": true, // internal field separator manipulation - "BROWSER": true, // hijacks xdg-open / git browser paths + // Build the inherited-env danger set from the shared source of truth. + // dangerousEnvVarNames covers loader/dyld/search-path/auto-exec/BROWSER + // vars; secureEnvInheritedOnlyDangerousVars adds CDPATH/GLOBIGNORE/IFS + // which are only filtered when inherited (legitimate inline otherwise). + inheritedDangerous := make(map[string]bool, len(dangerousEnvVarNames)+len(secureEnvInheritedOnlyDangerousVars)) + for _, name := range dangerousEnvVarNames { + inheritedDangerous[name] = true + } + for _, name := range secureEnvInheritedOnlyDangerousVars { + inheritedDangerous[name] = true } + pathSeen := false for _, env := range baseEnv { key := env if idx := strings.IndexByte(env, '='); idx >= 0 { key = env[:idx] } - // Skip dangerous vars - if dangerousVars[key] { + // PATH is reset to a controlled default rather than dropped, so + // that the child can still locate standard utilities. We emit the + // safe value once, after processing the rest of the environment. + if key == "PATH" { + pathSeen = true + continue + } + + // Skip dangerous vars (shared list + inherited-only list). + if inheritedDangerous[key] { continue } // Skip BASH_FUNC_ prefix (shellshock function exports) @@ -184,6 +198,11 @@ func SecureEnv(baseEnv []string) []string { result = append(result, env) } + // Reset PATH to a controlled safe default. If the caller did not supply + // a PATH at all we still inject one so the child has a usable search path. + result = append(result, "PATH="+safeDefaultPath) + _ = pathSeen + // Set GIT_EDITOR=true to prevent git from opening an editor result = append(result, "GIT_EDITOR=true") diff --git a/internal/tools/secure_exec_test.go b/internal/tools/secure_exec_test.go index 5378424..7a1c346 100644 --- a/internal/tools/secure_exec_test.go +++ b/internal/tools/secure_exec_test.go @@ -372,12 +372,22 @@ func TestSecureEnv_FiltersDangerousVars(t *testing.T) { "IFS=;", "TERM=xterm", "BASH_FUNC_evil%%=() { evil; }", + // Search-path manipulation vars (IK0XP6): must be filtered. + "PYTHONPATH=/evil/py", + "NODE_PATH=/evil/node", + "GOPATH=/evil/go", + "PERLLIB=/evil/perl", + "RUBYLIB=/evil/ruby", + // Auto-exec hook (IK0XP6): must be filtered. + "PROMPT_COMMAND=/evil/prompt.sh", } result := SecureEnv(baseEnv) + // PATH must be reset to the controlled safe default, not preserved as-is. + assertEnvContains(t, result, "PATH="+safeDefaultPath) + assertEnvNotContains(t, result, "PATH=/usr/bin") // These should be present - assertEnvContains(t, result, "PATH=/usr/bin") assertEnvContains(t, result, "HOME=/home/user") assertEnvContains(t, result, "TERM=xterm") assertEnvContains(t, result, "GIT_EDITOR=true") @@ -391,6 +401,14 @@ func TestSecureEnv_FiltersDangerousVars(t *testing.T) { assertEnvNotContains(t, result, "GLOBIGNORE") assertEnvNotContains(t, result, "IFS=") assertEnvNotContains(t, result, "BASH_FUNC_") + // Search-path manipulation vars must be filtered (IK0XP6). + assertEnvNotContains(t, result, "PYTHONPATH=") + assertEnvNotContains(t, result, "NODE_PATH=") + assertEnvNotContains(t, result, "GOPATH=") + assertEnvNotContains(t, result, "PERLLIB=") + assertEnvNotContains(t, result, "RUBYLIB=") + // Auto-exec hook must be filtered (IK0XP6). + assertEnvNotContains(t, result, "PROMPT_COMMAND=") } func TestSecureEnv_AddsGitEditor(t *testing.T) { @@ -415,24 +433,37 @@ func TestSecureEnv_PreservesNormalVars(t *testing.T) { "USER=testuser", "LANG=en_US.UTF-8", "SHELL=/bin/bash", - "GOPATH=/home/user/go", "CUSTOM_VAR=value", } result := SecureEnv(baseEnv) - for _, expected := range baseEnv { + // PATH is reset to the safe default rather than preserved verbatim. + assertEnvContains(t, result, "PATH="+safeDefaultPath) + // Normal vars (excluding PATH) are preserved verbatim. + for _, expected := range []string{ + "HOME=/home/user", + "USER=testuser", + "LANG=en_US.UTF-8", + "SHELL=/bin/bash", + "CUSTOM_VAR=value", + } { assertEnvContains(t, result, expected) } + // GOPATH is a search-path manipulation var and must NOT survive (IK0XP6). + assertEnvNotContains(t, result, "GOPATH=") } func TestSecureEnv_EmptyInput(t *testing.T) { result := SecureEnv(nil) - // Should still have GIT_EDITOR - if len(result) != 1 || result[0] != "GIT_EDITOR=true" { - t.Errorf("empty input should produce [GIT_EDITOR=true], got %v", result) + // Empty input should still inject a controlled PATH and GIT_EDITOR so + // the child has a usable search path (IK0XP6). + if len(result) != 2 { + t.Errorf("empty input should produce 2 entries [PATH=, GIT_EDITOR=true], got %v", result) } + assertEnvContains(t, result, "PATH="+safeDefaultPath) + assertEnvContains(t, result, "GIT_EDITOR=true") } func assertEnvContains(t *testing.T, env []string, expected string) { -- Gitee