From 9c88e8d46bc9a4e9cf749d875298256fa566bce1 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Tue, 11 Aug 2026 22:45:46 +0200 Subject: [PATCH 01/12] fix(sandbox): classify git push as network access Classify `git push` and remote `git archive` as network-sensitive through one shared, positional Git option parser, and fail closed on the launcher forms whose command source cannot be resolved statically: shell functions and subshells, CMD (CALL/IF/FOR /F/START/caret and nested launchers), PowerShell and pwsh, GNU `env -S`, shell `-c` clusters, BusyBox, and strace. Source that exists but cannot be read is not evidence that a command is local: an undecodable `-EncodedCommand` payload, a PowerShell Command operand built from an expansion, and an `env -S` split string the shell expands all now classify as network rather than parsing cleanly to nothing. Bounded, valid encoded payloads are decoded and read instead of guessed at. START keeps taking switches after its optional window title, and git's bare `--exec-path` is terminal (`--exec-path[=]`) so it no longer consumes the following token and mislabels a local informational command as network. An integration regression proves an approved `git push` receives the existing turn-scoped network overlay, including the `gitlawb://` transport. Co-Authored-By: Claude Opus 5 (1M context) --- internal/agent/command_prefix.go | 16 +- internal/agent/command_prefix_test.go | 11 + internal/agent/loop_test.go | 20 +- internal/sandbox/analyzer.go | 416 ++++++++- internal/sandbox/analyzer_test.go | 132 ++- internal/sandbox/engine_test.go | 206 +++++ internal/sandbox/risk.go | 1058 ++++++++++++++++++++++- internal/sandbox/risk_hardening_test.go | 564 +++++++++++- internal/sandbox/safe_command.go | 493 ++++++++++- internal/sandbox/safe_command_test.go | 11 + 10 files changed, 2852 insertions(+), 75 deletions(-) diff --git a/internal/agent/command_prefix.go b/internal/agent/command_prefix.go index 2d0f5a606..6279b284d 100644 --- a/internal/agent/command_prefix.go +++ b/internal/agent/command_prefix.go @@ -395,7 +395,7 @@ func safeGitCommand(command []string) bool { func gitSubcommand(command []string) (int, string, bool) { for index := 1; index < len(command); index++ { arg := command[index] - if gitOptionConsumesValue(arg) { + if sandbox.GitGlobalOptionConsumesValue(arg) { index++ continue } @@ -412,17 +412,9 @@ func gitSubcommand(command []string) (int, string, bool) { return 0, "", false } -func gitOptionConsumesValue(arg string) bool { - switch arg { - case "-C", "-c", "--config-env", "--exec-path", "--git-dir", "--namespace", "--super-prefix", "--work-tree": - return true - default: - return false - } -} - func gitOptionHasInlineValue(arg string) bool { - return strings.HasPrefix(arg, "--config-env=") || + return strings.HasPrefix(arg, "--attr-source=") || + strings.HasPrefix(arg, "--config-env=") || strings.HasPrefix(arg, "--exec-path=") || strings.HasPrefix(arg, "--git-dir=") || strings.HasPrefix(arg, "--namespace=") || @@ -439,7 +431,7 @@ func gitHasUnsafeGlobalOption(args []string) bool { return true case arg == "-C" || strings.HasPrefix(arg, "-C"): return true - case gitOptionConsumesValue(arg): + case sandbox.GitGlobalOptionConsumesValue(arg): index++ } } diff --git a/internal/agent/command_prefix_test.go b/internal/agent/command_prefix_test.go index a6e2adfac..69c9160db 100644 --- a/internal/agent/command_prefix_test.go +++ b/internal/agent/command_prefix_test.go @@ -35,6 +35,17 @@ func TestProposedCommandPrefixHonorsValidatedRequestedPrefix(t *testing.T) { } } +func TestSafeGitCommandConsumesAttrSourceOptionValue(t *testing.T) { + for _, command := range [][]string{ + {"git", "--attr-source", "HEAD", "status"}, + {"git", "--attr-source=HEAD", "status"}, + } { + if !safeGitCommand(command) { + t.Errorf("safeGitCommand(%q) = false; want true", command) + } + } +} + func TestProposedCommandPrefixSupportsSegmentedCommands(t *testing.T) { got := proposedCommandPrefix("bash", map[string]any{"command": "ps aux | head -5"}) if runtime.GOOS == "windows" { diff --git a/internal/agent/loop_test.go b/internal/agent/loop_test.go index f17e9be46..5223cc557 100644 --- a/internal/agent/loop_test.go +++ b/internal/agent/loop_test.go @@ -2441,22 +2441,22 @@ func TestRunPersistentCommandPrefixStillPromptsForNetwork(t *testing.T) { } } -func TestRunApprovedNetworkBashPromptAppliesTurnNetworkGrant(t *testing.T) { +func TestRunApprovedGitPushPromptAppliesTurnNetworkGrant(t *testing.T) { root := t.TempDir() - command := "PATH=.:$PATH curl https://example.com" + command := "PATH=.:$PATH git push gitlawb://example.com/repo.git main" if runtime.GOOS == "windows" { if windowsTestUsesPowerShell() { - command = `$env:PATH = '.;' + $env:PATH; curl.cmd https://example.com` + command = `$env:PATH = '.;' + $env:PATH; git.cmd push gitlawb://example.com/repo.git main` } else { - command = "set PATH=.;%PATH% && curl https://example.com" + command = "set PATH=.;%PATH% && git push gitlawb://example.com/repo.git main" } - fakeCurl := filepath.Join(root, "curl.cmd") - if err := os.WriteFile(fakeCurl, []byte("@echo fake curl %*\r\n"), 0o755); err != nil { + fakeGit := filepath.Join(root, "git.cmd") + if err := os.WriteFile(fakeGit, []byte("@echo fake git %*\r\n"), 0o755); err != nil { t.Fatal(err) } } else { - fakeCurl := filepath.Join(root, "curl") - if err := os.WriteFile(fakeCurl, []byte("#!/bin/sh\necho fake curl \"$@\"\n"), 0o755); err != nil { + fakeGit := filepath.Join(root, "git") + if err := os.WriteFile(fakeGit, []byte("#!/bin/sh\necho fake git \"$@\"\n"), 0o755); err != nil { t.Fatal(err) } } @@ -2478,7 +2478,7 @@ func TestRunApprovedNetworkBashPromptAppliesTurnNetworkGrant(t *testing.T) { } var requests []PermissionRequest var events []PermissionEvent - result, err := Run(context.Background(), "curl", provider, Options{ + result, err := Run(context.Background(), "push changes", provider, Options{ Registry: registry, PermissionMode: PermissionModeAsk, Autonomy: "medium", @@ -2520,7 +2520,7 @@ func TestRunApprovedNetworkBashPromptAppliesTurnNetworkGrant(t *testing.T) { t.Fatalf("expected tool result to be sent back to provider, got %d requests", len(provider.requests)) } lastMessage := provider.requests[1].Messages[len(provider.requests[1].Messages)-1] - if !strings.Contains(lastMessage.Content, "fake curl https://example.com") { + if !strings.Contains(lastMessage.Content, "fake git push gitlawb://example.com/repo.git main") { t.Fatalf("expected approved network command output after degraded execution, got %q", lastMessage.Content) } } diff --git a/internal/sandbox/analyzer.go b/internal/sandbox/analyzer.go index 7973fa951..a31199181 100644 --- a/internal/sandbox/analyzer.go +++ b/internal/sandbox/analyzer.go @@ -39,7 +39,8 @@ var powerShellRemoveItemPrograms = map[string]bool{ // networkPrograms are commands that perform network egress/ingress. var networkPrograms = map[string]bool{ - "curl": true, "wget": true, "ssh": true, "scp": true, "sftp": true, + "curl": true, "wget": true, "fetch": true, "aria2c": true, + "ssh": true, "scp": true, "sftp": true, "rsync": true, "nc": true, "ncat": true, "netcat": true, "telnet": true, "ftp": true, "iwr": true, "irm": true, "invoke-webrequest": true, "invoke-restmethod": true, @@ -162,6 +163,23 @@ func analyzeInto(script string, result *AnalysisResult, seen map[string]bool, de // Resolve the real program behind wrapper prefixes (sudo, env, nice, ...) // so `sudo rm -rf`, `env curl …`, and `bash -c 'vim x'` are classified on // the payload, not the launcher — matching DetectInteractiveCommand. + // GNU env -S/--split-string can consume the complete payload, leaving no + // ordinary effective program, so resolve its argv before that scan. + if fields, ok := literalCallFields(call.Args); ok { + if split := envSplitCommandFields(fields); split.recognized { + if split.executableEnvironmentDependent || fallbackBodyUsesNetwork(split.command, depth+1) { + result.Network = true + } + return true + } + } else if envSplitSourceDynamic(call.Args) { + // The split string comes from an expansion, so its argv — including the + // executable — is unknowable here. effectiveProgram would consume -S with + // its operand and report no executable at all, which reads an + // uninspectable command as a clean one. + result.Network = true + return true + } prog, rest := effectiveProgram(call.Args) if prog == "" { return true @@ -172,15 +190,48 @@ func analyzeInto(script string, result *AnalysisResult, seen map[string]bool, de } // `sh -c ` runs the payload as a fresh command; recurse into it so // a program hidden behind a shell launcher is still classified. - if depth < maxAnalyzerDepth && shellPrograms[prog] { - if payload := dashCPayload(rest); payload != "" { - analyzeInto(payload, result, seen, depth+1) + if shellPrograms[prog] { + if payloadIndex, found := shellCommandPayloadIndex(prog, wordTexts(rest)); found && payloadIndex < len(rest) { + if !isLiteralWord(rest[payloadIndex]) { + result.Network = true + } else if payload := wordText(rest[payloadIndex]); payload != "" && depth < maxAnalyzerDepth { + analyzeInto(payload, result, seen, depth+1) + } else if payload != "" { + result.Network = true + } + } + } + // PowerShell's Command flag also carries textual source. Analyze it in an + // isolated result because PowerShell syntax that the POSIX parser rejects + // must not make the outer command TooComplex; only fold the network fact. + if prog == "powershell" || prog == "pwsh" { + source := fallbackPowerShellPayload(prog, wordTexts(rest)) + switch { + case source.opaque, powerShellSourceDynamic(source, rest): + // Source the scan cannot read — an undecodable encoded payload, or a + // Command operand built from an expansion — must not be reported as a + // command that makes no network call. + result.Network = true + case source.payload != "": + if depth >= maxAnalyzerDepth || textualPayloadUsesNetwork(source.payload, depth+1) { + result.Network = true + } } } if _, interactive := interactivePrograms[prog]; interactive && !replSuppressed(prog, rest) { result.Interactive = true } - if commandUsesNetwork(prog, rest) { + // BusyBox and strace delegate to a child executable named in their own + // argv. Both resolvers below (busyboxCommandArgs, straceCommandArgs) + // operate on wordTexts, which silently drops any expansion — an + // unresolvable child-program token would otherwise read as a clean, + // unrecognized token rather than as "unknown, so assume the worst." + switch { + case prog == "busybox" && busyboxSourceDynamic(rest): + result.Network = true + case prog == "strace" && straceSourceDynamic(rest): + result.Network = true + case commandUsesNetwork(prog, rest): result.Network = true } if destructivePrograms[prog] || @@ -194,11 +245,22 @@ func analyzeInto(script string, result *AnalysisResult, seen map[string]bool, de } func commandUsesNetwork(prog string, args []*syntax.Word) bool { - if networkPrograms[prog] { - return true + return commandWordsUseNetwork(prog, wordTexts(args)) +} + +func commandWordsUseNetwork(prog string, words []string) bool { + return commandWordsUseNetworkAt(prog, words, 0) +} + +func commandWordsUseNetworkAt(prog string, words []string, depth int) bool { + prog = normalizeProgramToken(prog) + originalWords := words + normalized := make([]string, len(words)) + for index := range words { + normalized[index] = strings.ToLower(strings.TrimSpace(words[index])) } - words := literalWordTexts(args) - if localServerPrograms[prog] { + words = normalized + if networkPrograms[prog] || localServerPrograms[prog] { return true } switch prog { @@ -237,9 +299,18 @@ func commandUsesNetwork(prog string, args []*syntax.Word) bool { return gitUsesNetwork(words) case "gh": return ghUsesNetwork(words) + case "busybox": + if command := busyboxCommandArgs(originalWords); len(command) > 0 { + return fallbackBodyUsesNetwork(command, depth+1) + } + case "strace": + if command := straceCommandArgs(originalWords); len(command) > 0 { + return fallbackBodyUsesNetwork(command, depth+1) + } default: return false } + return false } func packageManagerUsesNetwork(words []string, aliases map[string]string) bool { @@ -278,8 +349,153 @@ func packageManagerOffline(words []string) bool { } func gitUsesNetwork(words []string) bool { - switch firstSubcommand(words, nil) { - case "clone", "fetch", "pull", "push", "ls-remote", "archive": + invocation := parseGitInvocation(words) + if invocation.kind != gitCommandSubcommand { + // No subcommand at all, or a global option that makes git print locally + // and exit before any subcommand runs. + return false + } + switch invocation.subcommand { + case "clone", "fetch", "pull", "push", "ls-remote": + return true + case "archive": + // `git archive HEAD` streams a tree out of the local object store and needs + // no egress at all; only `--remote=` sends the request to another + // host. Classifying every archive as network cost a proactive network + // prompt on a purely local command. + return gitTargetsRemoteArchive(words, invocation.subcommandIndex) + default: + return false + } +} + +// gitTargetsRemoteArchive reports whether an archive subcommand has an active +// --remote option. Git accepts archive options after positional operands, so +// this must keep scanning until an unconsumed `--`. Value-taking options are +// consumed first: in `archive -o -- --remote=origin HEAD`, `-o` owns the first +// `--` and the later remote is active; in `archive -o --remote HEAD`, --remote +// is only the output filename. +func gitTargetsRemoteArchive(words []string, subcommandIndex int) bool { + for index := subcommandIndex + 1; index < len(words); index++ { + word := strings.ToLower(words[index]) + switch { + case word == "--": + return false + case strings.HasPrefix(word, "--remote="): + return true + case word == "--remote": + return index+1 < len(words) + case gitArchivePreliminaryOptionConsumesValue(word): + index++ + } + } + return false +} + +// gitArchivePreliminaryOptionConsumesValue models the first parse-options pass +// in git archive. That pass consumes only transport/output options and retains +// format/prefix/mtime options for a later parser, so those later options must +// not hide an immediately following --remote. +func gitArchivePreliminaryOptionConsumesValue(option string) bool { + if strings.Contains(option, "=") { + return false + } + switch option { + case "-o", "--output", "--exec": + return true + default: + return false + } +} + +// gitCommandKind distinguishes the three outcomes of reading a git command +// line, which callers must treat differently. +type gitCommandKind int + +const ( + // gitCommandNone: no subcommand was found (`git`, `git -C repo`). + gitCommandNone gitCommandKind = iota + // gitCommandTerminalGlobal: a global option that makes git print locally + // and exit — no subcommand runs, whatever words follow it. + gitCommandTerminalGlobal + // gitCommandSubcommand: a subcommand git will actually execute. + gitCommandSubcommand +) + +type gitInvocation struct { + kind gitCommandKind + // subcommand is set only for gitCommandSubcommand. + subcommand string + // subcommandIndex is the position in the original argument slice. + subcommandIndex int + // terminalOption is set only for gitCommandTerminalGlobal. + terminalOption string +} + +// gitTerminalGlobalOptions are git's global options that print something from +// the local installation and exit. Everything after one of them is help/version +// output text, not a command: `git -C repo --help push` prints git-push's +// manual page without contacting a remote, so a subcommand scan that walked +// past them would classify a purely local command as network. +// Bare `--exec-path` belongs here for the same reason: git documents it as +// `--exec-path[=]`, so without an inline value it prints the compiled-in +// exec path and exits. `git --exec-path /tmp push` neither reads /tmp as the +// option's value nor runs push, so treating it as a value-taking option made a +// local informational command request egress. +var gitTerminalGlobalOptions = map[string]bool{ + "-h": true, "--help": true, + "-v": true, "--version": true, + "--html-path": true, "--man-path": true, "--info-path": true, + "--list-cmds": true, "--exec-path": true, +} + +// parseGitInvocation resolves what a git command line actually does, past git's +// GLOBAL options. It is the single reader both classification paths use — the +// AST path through gitUsesNetwork and the unparseable fallback through +// matchesUnparseableGitNetwork — so the two cannot disagree about an option, as +// they did while each carried its own skip list. +// +// The generic firstSubcommand cannot do this job: git's value-taking globals put +// their value in the next token, so scanning for the first non-dash token returns +// that value instead — `git -C repo push origin main` looked like the subcommand +// "repo" and so classified as no-network, dropping the proactive network prompt +// for the most common form of the command. internal/agent/command_prefix.go +// resolves the same option set for its own prefix matching. +func parseGitInvocation(words []string) gitInvocation { + for index := 0; index < len(words); index++ { + word := strings.ToLower(words[index]) + if word == "" { + continue + } + if gitTerminalGlobalOptions[word] || strings.HasPrefix(word, "--list-cmds=") { + return gitInvocation{kind: gitCommandTerminalGlobal, terminalOption: word} + } + if strings.HasPrefix(word, "-") { + // A joined value (--git-dir=/x, -C/x) is one token and needs no skip; + // a separated one puts its value in the next token. + if GitGlobalOptionConsumesValue(word) { + index++ + } + continue + } + if isNumericToken(word) { + continue + } + return gitInvocation{kind: gitCommandSubcommand, subcommand: word, subcommandIndex: index} + } + return gitInvocation{kind: gitCommandNone} +} + +// GitGlobalOptionConsumesValue lists git's global options whose value is a +// separate token. It is shared with internal/agent's command-prefix parser so +// the two security-sensitive scans cannot drift. +// +// `--exec-path` is deliberately absent: its value is inline-only +// (`--exec-path=`), and the bare spelling is terminal — see +// gitTerminalGlobalOptions. +func GitGlobalOptionConsumesValue(option string) bool { + switch strings.ToLower(option) { + case "-c", "--attr-source", "--config-env", "--git-dir", "--namespace", "--super-prefix", "--work-tree": return true default: return false @@ -290,14 +506,177 @@ func npxUsesNetwork(_ []string) bool { return true } -func literalWordTexts(args []*syntax.Word) []string { +func wordTexts(args []*syntax.Word) []string { words := make([]string, 0, len(args)) for _, arg := range args { - words = append(words, strings.ToLower(strings.TrimSpace(wordText(arg)))) + words = append(words, strings.TrimSpace(wordText(arg))) } return words } +// powerShellSourceDynamic reports whether the words carrying a PowerShell +// host's command source contain an expansion. wordText silently drops those, +// so `powershell -Command "$PAYLOAD"` otherwise extracts an empty payload and +// classifies as clean while the host runs whatever the shell expanded. +func powerShellSourceDynamic(source powerShellPayload, args []*syntax.Word) bool { + if source.sourceIndex < 0 { + return false + } + for index := source.sourceIndex; index < len(args); index++ { + if !isLiteralWord(args[index]) { + return true + } + } + return false +} + +// envSplitSourceDynamic reports whether an env invocation takes its +// -S/--split-string argv from a word this scan cannot resolve statically. +// +// The literal reconstruction used by envSplitCommandFields is an optimization, +// not a proof of safety: `PAYLOAD='curl https://…'; env -S "$PAYLOAD"` runs the +// expanded argv, so an unreadable operand must classify as network-sensitive +// rather than fall through to wrapper handling that reports no executable. +func envSplitSourceDynamic(args []*syntax.Word) bool { + texts := make([]string, len(args)) + for index, arg := range args { + texts[index] = wordText(arg) + } + start, ok := envArgumentStart(texts) + if !ok { + return false + } + seenSplit := false + for index := start; index < len(texts); index++ { + text := texts[index] + if text == "--" { + return false + } + if _, _, _, split := envSplitOption([]string{text}, 0); split { + // A joined operand (-S"$PAYLOAD") reconstructs to the option alone, so + // the option token itself carries the dynamic source. + if !isLiteralWord(args[index]) { + return true + } + seenSplit = true + continue + } + if !isLiteralWord(args[index]) { + if seenSplit { + return true + } + continue + } + if seenSplit { + // The operand of a separated -S is literal; the ordinary literal path + // already reads it. + return false + } + if strings.Contains(text, "=") && !strings.HasPrefix(text, "=") && !strings.HasPrefix(text, "-") { + continue + } + if strings.HasPrefix(text, "-") { + if wrapperConsumesValue("env", text) && index+1 < len(texts) { + index++ + } + continue + } + // A command position was reached without a split string; ordinary wrapper + // resolution classifies this invocation. + return false + } + return false +} + +// busyboxSourceDynamic reports whether a BusyBox invocation's applet-name +// operand — the token busyboxCommandArgs treats as the delegated child +// executable — comes from a word this scan cannot resolve statically. +// +// busyboxCommandArgs runs on wordTexts, which silently drops expansions: +// `APPLET=curl; busybox "$APPLET" https://…` would otherwise resolve the +// applet position to an empty string, which is neither a recognized BusyBox +// flag nor the executable this scan can name, and the invocation reads as an +// ordinary unrecognized command rather than as "unknown, assume network." +func busyboxSourceDynamic(args []*syntax.Word) bool { + if len(args) == 0 { + return false + } + return !isLiteralWord(args[0]) +} + +// straceSourceDynamic is busyboxSourceDynamic's counterpart for strace: it +// reports whether the operand straceCommandArgs would treat as the traced +// child command comes from a word this scan cannot resolve statically. +// straceChildIndex is shared with straceCommandArgs so this check walks +// strace's option grammar exactly once, rather than duplicating it and +// risking the two silently drifting apart. +func straceSourceDynamic(args []*syntax.Word) bool { + index, ok := straceChildIndex(wordTexts(args)) + if !ok || index >= len(args) { + return false + } + return !isLiteralWord(args[index]) +} + +// envArgumentStart returns the index just past an `env` program token, allowing +// the wrapper prefixes (sudo, nice, ...) that may precede it. +func envArgumentStart(texts []string) (int, bool) { + wrapper := "" + for index := 0; index < len(texts); index++ { + text := texts[index] + if text == "" { + if wrapper == "" { + return 0, false + } + continue + } + if strings.Contains(text, "=") && !strings.HasPrefix(text, "=") && !strings.HasPrefix(text, "-") { + continue + } + if strings.HasPrefix(text, "-") { + if wrapperConsumesValue(wrapper, text) && index+1 < len(texts) { + index++ + } + continue + } + if isNumericToken(text) { + continue + } + token := normalizeProgramToken(text) + if token == "env" { + return index + 1, true + } + if wrapperPrograms[token] { + wrapper = token + continue + } + return 0, false + } + return 0, false +} + +func literalCallFields(args []*syntax.Word) ([]string, bool) { + fields := make([]string, 0, len(args)) + for _, arg := range args { + if !isLiteralWord(arg) { + return nil, false + } + fields = append(fields, wordText(arg)) + } + return fields, true +} + +// textualPayloadUsesNetwork classifies source carried by another interpreter +// without leaking the nested parser's TooComplex bit into the outer command. +func textualPayloadUsesNetwork(payload string, depth int) bool { + if depth > maxAnalyzerDepth { + return false + } + result := AnalysisResult{} + analyzeInto(payload, &result, map[string]bool{}, depth) + return result.Network || (result.TooComplex && matchesUnparseableNetworkAt(payload, depth)) +} + func pythonModuleUsesNetwork(words []string) bool { for index := 0; index < len(words); index++ { if words[index] != "-m" || index+1 >= len(words) { @@ -423,17 +802,6 @@ func effectiveProgram(args []*syntax.Word) (string, []*syntax.Word) { return "", nil } -// dashCPayload returns the literal text of the word following `-c` in an AST arg -// list (the command a shell launcher will run), or "" when there is none. -func dashCPayload(args []*syntax.Word) string { - for index := 0; index < len(args); index++ { - if wordText(args[index]) == "-c" && index+1 < len(args) { - return wordText(args[index+1]) - } - } - return "" -} - // replSuppressed reports whether a REPL program (python/node/...) was invoked // non-interactively — with an inline-eval flag or a script argument — mirroring // nonInteractiveREPLFlags used by the regex detector. Non-REPL interactive diff --git a/internal/sandbox/analyzer_test.go b/internal/sandbox/analyzer_test.go index 9f44eca25..3d8362084 100644 --- a/internal/sandbox/analyzer_test.go +++ b/internal/sandbox/analyzer_test.go @@ -1,8 +1,12 @@ package sandbox -import "testing" +import ( + "strings" + "testing" +) func TestAnalyzeCommand(t *testing.T) { + t.Setenv("ZERO_TEST_ENV_COMMAND", "curl") cases := []struct { name string script string @@ -85,6 +89,122 @@ func TestAnalyzeCommand(t *testing.T) { {name: "git clone", script: "git clone https://example.com/repo.git", network: true}, {name: "git fetch", script: "git fetch origin", network: true}, {name: "git status is offline", script: "git status", network: false}, + {name: "git pull", script: "git pull origin main", network: true}, + {name: "git push custom transport", script: "git push gitlawb://example.com/repo.git main", network: true}, + {name: "git ls-remote", script: "git ls-remote origin", network: true}, + {name: "git remote archive", script: "git archive --remote=origin HEAD", network: true}, + {name: "git remote archive separated value", script: "git archive --remote origin HEAD", network: true}, + // Only --remote leaves the machine; a local archive streams the object + // store and must not cost a network prompt (issue #703 review). + {name: "git local archive", script: "git archive HEAD", network: false}, + {name: "git -C local archive", script: "git -C repo archive HEAD -o out.tar", network: false}, + // After `--` every token is a pathspec, so this archives a tree entry + // named "--remote" from the local object store (issue #703 review). + {name: "git archive pathspec named --remote", script: "git archive HEAD -- --remote", network: false}, + {name: "git -C archive pathspec named --remote", script: "git -C repo archive HEAD -- --remote=origin", network: false}, + {name: "git archive missing remote operand", script: "git archive HEAD --remote", network: false}, + {name: "git archive post-tree --remote value", script: "git archive HEAD --remote=origin", network: true}, + // A real --remote before the separator still is one. + {name: "git remote archive with pathspec", script: "git archive --remote=origin HEAD -- src", network: true}, + {name: "git remote archive after output option", script: "git archive -o out.tar --remote origin HEAD", network: true}, + {name: "git remote archive after mtime option", script: "git archive --mtime 2024-01-01 --remote origin HEAD", network: true}, + {name: "git remote archive after format without value", script: "git archive --format --remote=origin HEAD", network: true}, + {name: "git remote archive after mtime without value", script: "git archive --mtime --remote=origin HEAD", network: true}, + {name: "git output consumes --remote", script: "git archive -o --remote HEAD", network: false}, + {name: "git joined output named --remote", script: "git archive --output=--remote HEAD", network: false}, + {name: "git global -C consumes --remote", script: "git -C --remote archive HEAD", network: false}, + {name: "git output consumes separator before remote", script: "git archive -o -- --remote=origin HEAD", network: true}, + {name: "bash clustered login command", script: `bash -lc 'curl https://x.test'`, network: true}, + {name: "bash clustered command erexit", script: `bash -ce 'git push origin main'`, network: true}, + {name: "dash clustered command erexit", script: `dash -ce 'curl https://x.test'`, network: true}, + {name: "bash long command flag is invalid", script: `bash --command 'curl https://x.test'`, network: false}, + {name: "bash args after command payload are positional", script: `bash -c 'echo ok' curl https://x.test`, network: false}, + {name: "bash script makes later command flag positional", script: `bash /dev/null -c 'curl https://x.test'`, network: false}, + {name: "bash option terminator makes command flag positional", script: `bash -- -c 'curl https://x.test'`, network: false}, + {name: "bash invalid command cluster", script: `bash -Zc 'curl https://x.test'`, network: false}, + {name: "bash noexec command cluster", script: `bash -nc 'curl https://x.test'`, network: false}, + {name: "bash plus option before command", script: `bash +n -c 'curl https://x.test'`, network: true}, + {name: "bash plus command flag", script: `bash +c 'curl https://x.test'`, network: true}, + {name: "bash dump strings does not execute", script: `bash --dump-strings -c 'curl https://x.test'`, network: false}, + {name: "shell dynamic command payload fails closed", script: `sh -c "$ZERO_TEST_SHELL_COMMAND"`, network: true}, + {name: "nested shell dynamic command payload fails closed", script: `sh -c 'sh -c "$1"' sh 'curl https://x.test'`, network: true}, + {name: "PowerShell slash command", script: `powershell /Command Invoke-WebRequest https://x.test`, network: true}, + {name: "PowerShell abbreviated command", script: `pwsh -co iwr https://x.test`, network: true}, + {name: "PowerShell local command", script: `pwsh /Command Get-ChildItem`, network: false}, + {name: "PowerShell abbreviated execution policy", script: `powershell -ep RemoteSigned curl https://x.test`, network: true}, + {name: "pwsh abbreviated execution policy before command", script: `pwsh -ep Bypass -Command curl https://x.test`, network: true}, + {name: "Windows PowerShell bare command", script: `powershell Invoke-WebRequest https://x.test`, network: true}, + {name: "pwsh bare token is file mode", script: `pwsh Invoke-WebRequest https://x.test`, network: false}, + {name: "PowerShell joined command flag is invalid", script: `powershell -Command:Invoke-WebRequest https://x.test`, network: false}, + {name: "pwsh command with args", script: `pwsh -CommandWithArgs Invoke-WebRequest https://x.test`, network: true}, + {name: "pwsh abbreviated command with args", script: `pwsh -cwa Invoke-RestMethod https://x.test`, network: true}, + {name: "PowerShell invalid startup option", script: `powershell -DefinitelyInvalid Invoke-WebRequest https://x.test`, network: false}, + {name: "env split curl argv", script: `env -S 'curl https://x.test'`, network: true}, + {name: "env joined split git argv", script: `env --split-string='git push origin main'`, network: true}, + {name: "env clustered split option", script: `env -iS 'curl https://x.test'`, network: true}, + {name: "env split introduces split option", script: `env -S '-S "curl https://x.test"'`, network: true}, + {name: "env split introduces nested env", script: `env -S 'env -S "curl https://x.test"'`, network: true}, + {name: "env split underscore separator", script: `env -S 'curl\_https://x.test'`, network: true}, + {name: "env split environment executable", script: `env -S '${ZERO_TEST_ENV_COMMAND} https://x.test'`, network: true}, + {name: "env split metacharacters stay arguments", script: `env -S 'printf x; curl https://x.test'`, network: false}, + {name: "env split trailing args stay arguments", script: `env -S 'printf ok' curl https://x.test`, network: false}, + {name: "env split environment argument", script: `env -S 'printf ${ZERO_TEST_ENV_COMMAND}'`, network: false}, + {name: "env split shell command", script: `env -S 'sh -c "curl https://x.test"'`, network: true}, + {name: "env split argv0 before curl", script: `env -S '--argv0 harmless curl https://x.test'`, network: true}, + {name: "env split argv0 named curl", script: `env -S '--argv0 curl printf ok'`, network: false}, + // GNU env accepts unambiguous long-option abbreviations: "--split" and + // "--split-strin" name --split-string exactly like the full spelling does, + // so the launcher's argv reconstruction must not depend on that one spelling. + {name: "env abbreviated split option", script: `env --split 'curl https://x.test'`, network: true}, + {name: "env abbreviated split option with value", script: `env --split='curl https://x.test'`, network: true}, + {name: "env minimal abbreviated split option", script: `env --s 'curl https://x.test'`, network: true}, + {name: "env near-full abbreviated split option", script: `env --split-strin 'curl https://x.test'`, network: true}, + {name: "env abbreviated split metacharacters stay arguments", script: `env --split 'printf x; curl https://x.test'`, network: false}, + {name: "env non-split long option is not split-string", script: `env --unset FOO curl https://x.test`, network: true}, + {name: "env non-split long option leaves literal payload alone", script: `env --chdir=/tmp printf ok`, network: false}, + {name: "busybox wget applet", script: `busybox wget https://x.test`, network: true}, + {name: "busybox shell command", script: `busybox sh -c 'curl https://x.test'`, network: true}, + {name: "busybox echo network text", script: `busybox echo wget https://x.test`, network: false}, + {name: "busybox has no option terminator", script: `busybox -- curl https://x.test`, network: false}, + {name: "busybox unknown option", script: `busybox -x curl https://x.test`, network: false}, + // The applet name comes from a shell expansion this scan cannot read + // statically; wordText silently drops it, so the AST resolver must fail + // closed here rather than reading the blanked token as a clean unknown. + {name: "busybox dynamic applet fails closed", script: `APPLET=curl; busybox "$APPLET" https://x.test`, network: true}, + {name: "busybox literal applet stays classified on content", script: `busybox echo "not a program" https://x.test`, network: false}, + {name: "strace curl command", script: `strace -f -o trace.log curl https://x.test`, network: true}, + {name: "strace shell command", script: `strace sh -c 'git push origin main'`, network: true}, + {name: "strace trace path before curl", script: `strace -P /tmp curl https://x.test`, network: true}, + {name: "strace trace path named curl", script: `strace -P curl true`, network: false}, + {name: "strace attach and curl", script: `strace -p 123 curl https://x.test`, network: true}, + {name: "strace long trace option", script: `strace --trace network curl https://x.test`, network: true}, + {name: "strace tips traces curl", script: `strace --tips curl https://x.test`, network: true}, + {name: "strace joined tips traces git", script: `strace --tips=full git push origin main`, network: true}, + {name: "strace clustered options", script: `strace -fqo trace.log curl https://x.test`, network: true}, + {name: "strace output named curl", script: `strace -o curl true`, network: false}, + {name: "strace invalid option", script: `strace --definitely-invalid curl https://x.test`, network: false}, + // The traced command comes from a shell expansion this scan cannot read + // statically; straceSourceDynamic must fail closed the same way + // busyboxSourceDynamic does above, rather than reading the blanked token + // as a clean unknown command. + {name: "strace dynamic child fails closed", script: `APPLET=curl; strace "$APPLET" https://x.test`, network: true}, + {name: "strace literal child stays classified on content", script: `strace true "not a program" https://x.test`, network: false}, + {name: "git local commit", script: `git commit -m "local change"`, network: false}, + // git's value-taking global options put their value in the NEXT token, so a + // generic "first non-dash token" scan reads the value as the subcommand and + // misses the network verb entirely (issue #703 review). + {name: "git -C push", script: "git -C repo push origin main", network: true}, + {name: "git -c config push", script: "git -c http.sslVerify=false push origin main", network: true}, + {name: "git --git-dir fetch", script: "git --git-dir /repo/.git fetch origin", network: true}, + {name: "git --work-tree pull", script: "git --work-tree /repo pull origin main", network: true}, + {name: "git --attr-source push", script: "git --attr-source HEAD push origin main", network: true}, + {name: "git -C local commit", script: `git -C repo commit -m "local change"`, network: false}, + {name: "git help topic is offline", script: "git --help push", network: false}, + {name: "git -C help topic is offline", script: "git -C repo --help push", network: false}, + {name: "git -c version topic is offline", script: "git -c user.name=test --version push", network: false}, + {name: "git.exe push", script: "git.exe push origin main", network: true}, + {name: "git.cmd push", script: "git.cmd push origin main", network: true}, + {name: "git.exe local commit", script: `git.exe commit -m "local change"`, network: false}, {name: "gh release download", script: "gh release download v1.0.0", network: true}, {name: "no network", script: "ls -la && echo done", network: false}, {name: "process pattern is not network", script: `pkill -f "python3 -m http.server 8000"`, network: false}, @@ -104,6 +224,16 @@ func TestAnalyzeCommand(t *testing.T) { } } +func TestAnalyzeCommandFailsClosedAtShellLauncherDepthLimit(t *testing.T) { + command := "curl https://x.test" + for range maxAnalyzerDepth + 1 { + command = "sh -c '" + strings.ReplaceAll(command, "'", `'"'"'`) + "'" + } + if got := AnalyzeCommand(command); !got.Network { + t.Fatalf("AnalyzeCommand(%q) = %#v; want network", command, got) + } +} + func TestAnalyzeCommandEmptyIsClean(t *testing.T) { if got := AnalyzeCommand(" "); got.Interactive || got.Destructive || got.Network || got.TooComplex { t.Fatalf("empty script should be clean, got %#v", got) diff --git a/internal/sandbox/engine_test.go b/internal/sandbox/engine_test.go index 5f9633a40..1cd8ee4b3 100644 --- a/internal/sandbox/engine_test.go +++ b/internal/sandbox/engine_test.go @@ -45,6 +45,51 @@ func TestEvaluatePromptsForNetworkToolsButExemptsThemFromShellNetworkPolicy(t *t } } +// TestEvaluatePromptsForUnparseableNetworkBehindWrapper is the engine-level +// regression for jatmn's #726 P2 finding. Classification is what decides egress +// here: when the fallback stopped recognizing a network program hidden behind a +// wrapper, an already-permission-granted shell command went from ActionPrompt / +// ReasonNetworkBlocked to a plain ActionAllow — silently granting network to a +// command too obfuscated to parse. +func TestEvaluatePromptsForUnparseableNetworkBehindWrapper(t *testing.T) { + engine := NewEngine(EngineOptions{Policy: Policy{Mode: ModeEnforce, Network: NetworkDeny}}) + for _, command := range []string{ + `sudo curl https://evil.test && "unterminated`, + `env git push origin main && "unterminated`, + `curl.exe https://evil.test && "unterminated`, + "true\ncurl https://evil.test && \"unterminated", + `echo $(curl https://evil.test) && "unterminated`, + `if true; then git push; fi && "unterminated`, + `eval "curl https://evil.test" && "unterminated`, + `f(){ curl https://evil.test; }; f && "unterminated`, + `function f() (git push origin main); f && "unterminated`, + `cmd.exe /c curl https://evil.test & rem '`, + `cmd.exe /d /c git push origin main & rem '`, + `cmd.exe /k git push origin main & rem '`, + `powershell.exe -Command curl https://evil.test & rem '`, + `powershell.exe -c curl https://evil.test & rem '`, + `pwsh.exe -c git push origin main & rem '`, + `if 1==1 (curl https://evil.test) & rem '`, + `if 1==1 ((git push origin main)) & rem '`, + `for %i in (x) do (curl https://evil.test) & rem '`, + `for /f %i in ('curl https://evil.test') do echo %i & rem '`, + "for /f \"usebackq\" %i in (`git push origin main`) do echo %i & rem '", + } { + t.Run(command, func(t *testing.T) { + if analysis := AnalyzeCommand(command); !analysis.TooComplex { + t.Fatalf("AnalyzeCommand(%q) = %#v; test must exercise the fallback", command, analysis) + } + decision := engine.Evaluate(context.Background(), Request{ + ToolName: "bash", SideEffect: SideEffectShell, PermissionGranted: true, + Args: map[string]any{"command": command}, + }) + if decision.Action != ActionPrompt || decision.Reason != ReasonNetworkBlocked { + t.Fatalf("Evaluate(%q) = action %q reason %q, want a network prompt", command, decision.Action, decision.Reason) + } + }) + } +} + func TestEngineBashAllowGrantDoesNotBypassNetworkPrompt(t *testing.T) { store, err := NewGrantStore(StoreOptions{ FilePath: filepath.Join(t.TempDir(), "sandbox-grants.json"), @@ -93,6 +138,167 @@ func TestEngineClassifiesPowerShellCurlCommandAsNetwork(t *testing.T) { } } +// TestEngineClassifiesCMDInvocationFormsAsNetwork covers jatmn's #726 finding +// at the decision layer it actually costs: Evaluate only reaches the network +// prompt when the risk carries the "network" category, so a CMD form the +// fallback resolved to a keyword instead of its program reached ActionAllow (or +// ran under the deny profile) with no approval flow. On the Windows unelevated +// backend that prompt is the network boundary. +func TestEngineClassifiesCMDInvocationFormsAsNetwork(t *testing.T) { + for _, command := range []string{ + `@curl https://evil.test & rem '`, + `cmd.exe /c call curl https://evil.test & rem '`, + `if not 1==2 curl https://evil.test & rem '`, + `start "" curl https://evil.test & rem '`, + `start "" c^u^r^l https://evil.test & rem '`, + `call start "" curl https://evil.test & rem '`, + `if 1==1 start "" curl https://evil.test & rem '`, + `cmd /c /d curl https://evil.test & rem '`, + `powershell /Command curl https://evil.test & rem '`, + `powershell -co curl https://evil.test & rem '`, + `if 1==1 (curl https://evil.test) & rem '`, + } { + t.Run(command, func(t *testing.T) { + if analysis := AnalyzeCommand(command); !analysis.TooComplex { + t.Fatalf("AnalyzeCommand(%q) parsed; this case must exercise the fallback", command) + } + engine := NewEngine(EngineOptions{WorkspaceRoot: t.TempDir(), Policy: DefaultPolicy()}) + decision := engine.Evaluate(context.Background(), Request{ + ToolName: "bash", + SideEffect: SideEffectShell, + Permission: PermissionPrompt, + PermissionMode: PermissionModeAsk, + Args: map[string]any{"command": command}, + }) + if decision.Action != ActionPrompt || decision.Reason != ReasonNetworkBlocked || + !HasRiskCategory(decision.Risk, "network") { + t.Fatalf("Evaluate(%q) = %#v, want a network prompt", command, decision) + } + }) + } +} + +func TestEnginePromptsForReviewedUnparseableNetworkForms(t *testing.T) { + t.Setenv("ZERO_TEST_ENV_COMMAND", "curl") + for _, command := range []string{ + `cu^rl https://evil.test & rem '`, + `cmd /c cu^rl https://evil.test & rem '`, + `%ComSpec% /c curl https://evil.test & rem '`, + `start "x" curl https://evil.test & rem '`, + `cmd /c start "x" curl https://evil.test & rem '`, + `start /b "x" curl https://evil.test & rem '`, + `if cmdextversion 1 curl https://evil.test & rem '`, + `if not cmdextversion 1 git push origin main & rem '`, + `git pus^h origin main & rem '`, + `git archive --rem^ote=origin HEAD & rem '`, + `git archive HEAD --remote=origin`, + `git archive --format --remote=origin HEAD`, + `bash -lc 'curl https://evil.test' && "unterminated`, + `dash -ce 'git push origin main' && "unterminated`, + `bash +n -c 'curl https://evil.test' && "unterminated`, + `sh -c "$ZERO_TEST_SHELL_COMMAND"`, + `env -S 'curl https://evil.test' && "unterminated`, + `env -S 'curl\_https://evil.test' && "unterminated`, + `env -S '${ZERO_TEST_ENV_COMMAND} https://evil.test' && "unterminated`, + `env -iS 'curl https://evil.test' && "unterminated`, + `env -S '-S "curl https://evil.test"' && "unterminated`, + `env -S 'env -S "curl https://evil.test"' && "unterminated`, + `env -S '--argv0 harmless curl https://evil.test' && "unterminated`, + `env --split-string 'git push origin main' && "unterminated`, + `exec -a harmless curl https://evil.test && "unterminated`, + `powershell -NoProfile curl https://evil.test`, + `pwsh -cwa Invoke-WebRequest https://evil.test & rem '`, + `busybox sh -c 'curl https://evil.test' && "unterminated`, + `strace -P /tmp sh -c 'git push origin main' && "unterminated`, + `strace --trace network curl https://evil.test && "unterminated`, + `strace --tips curl https://evil.test && "unterminated`, + `strace -fqo trace.log curl https://evil.test && "unterminated`, + `powershell -ep RemoteSigned curl https://evil.test & rem '`, + // PowerShell source that exists but cannot be read statically: an + // undecodable encoded payload, valid encoded network source, and a + // Command operand supplied by an expansion. + `powershell -EncodedCommand curl & rem '`, + `powershell -EncodedCommand YwB1AHIAbAAgAGgAdAB0AHAAcwA6AC8ALwBlAHYAaQBsAC4AdABlAHMAdAA=`, + `pwsh -ec YwB1AHIAbAAgAGgAdAB0AHAAcwA6AC8ALwBlAHYAaQBsAC4AdABlAHMAdAA= & rem '`, + `PAYLOAD='curl https://evil.test'; powershell -Command "$PAYLOAD"`, + `PAYLOAD='curl https://evil.test'; powershell "$PAYLOAD"`, + // GNU env -S receives the shell-expanded value and executes its argv, so + // an operand this scan cannot resolve must not read as network-free. + `PAYLOAD='curl https://evil.test'; env -S "$PAYLOAD"`, + `PAYLOAD='curl https://evil.test'; env -S"$PAYLOAD"`, + `PAYLOAD='curl https://evil.test'; env --split-string="$PAYLOAD"`, + `env -S "$PAYLOAD" && "unterminated`, + // CMD's START continues taking switches after its optional window title. + `start "" /b curl https://evil.test & rem '`, + `start "" /wait curl https://evil.test & rem '`, + `start "" /d C:\ curl https://evil.test & rem '`, + `start "" /b git push origin main & rem '`, + } { + t.Run(command, func(t *testing.T) { + decision := NewEngine(EngineOptions{Policy: Policy{Mode: ModeEnforce, Network: NetworkDeny}}).Evaluate(context.Background(), Request{ + ToolName: "bash", + SideEffect: SideEffectShell, + Args: map[string]any{"command": command}, + Permission: PermissionAllow, + }) + if decision.Action != ActionPrompt || decision.Reason != ReasonNetworkBlocked { + t.Fatalf("Evaluate(%q) = action %q reason %q, want prompt/network blocked", command, decision.Action, decision.Reason) + } + }) + } +} + +// TestEngineDoesNotPromptForNonNetworkCommandForms is the negative half: text +// that resembles a network invocation but is actually a local argument, +// encoded payload, script name, or malformed remote option must not cost a +// network grant. +func TestEngineDoesNotPromptForNonNetworkCommandForms(t *testing.T) { + for _, command := range []string{ + "git -C repo --help push", + "git -h push", + `git -C repo -h push & rem '`, + "git archive HEAD -- --remote=origin", + `git archive -o --remote HEAD & rem '`, + `git archive HEAD --remote & rem '`, + // A decodable encoded payload is read, not guessed at: this one is the + // UTF-16LE source `evil`, a local program name. + `powershell -e ZQB2AGkAbAA= & rem '`, + `pwsh -File curl & rem '`, + // Bare --exec-path prints the local exec path and exits; git never reaches + // push, and /tmp is an operand rather than the option's value. + `git --exec-path`, + `git --exec-path /tmp push`, + `git --exec-path /tmp push & rem '`, + `pwsh Invoke-WebRequest https://evil.test`, + `powershell -Command:Invoke-WebRequest https://evil.test`, + `powershell -DefinitelyInvalid Invoke-WebRequest https://evil.test`, + `env -S 'printf curl; git push' && "unterminated`, + `env -S 'printf ok' curl https://evil.test && "unterminated`, + `env -S '--argv0 curl printf ok' && "unterminated`, + `start MyTitle curl https://evil.test & rem '`, + `busybox -- curl https://evil.test && "unterminated`, + `strace --definitely-invalid curl https://evil.test && "unterminated`, + `bash /dev/null -c 'curl https://evil.test' && "unterminated`, + `bash -- -c 'curl https://evil.test' && "unterminated`, + `bash -Zc 'curl https://evil.test' && "unterminated`, + `bash -nc 'curl https://evil.test' && "unterminated`, + } { + t.Run(command, func(t *testing.T) { + engine := NewEngine(EngineOptions{WorkspaceRoot: t.TempDir(), Policy: DefaultPolicy()}) + decision := engine.Evaluate(context.Background(), Request{ + ToolName: "bash", + SideEffect: SideEffectShell, + Permission: PermissionPrompt, + PermissionMode: PermissionModeAsk, + Args: map[string]any{"command": command}, + }) + if decision.Reason == ReasonNetworkBlocked || HasRiskCategory(decision.Risk, "network") { + t.Fatalf("Evaluate(%q) = %#v, want no network classification", command, decision) + } + }) + } +} + func TestUnsandboxedExecutionAllowedPreservesDeniedReads(t *testing.T) { root := t.TempDir() policy := DefaultPolicy() diff --git a/internal/sandbox/risk.go b/internal/sandbox/risk.go index faa4e09bc..b45b87b8e 100644 --- a/internal/sandbox/risk.go +++ b/internal/sandbox/risk.go @@ -1,12 +1,15 @@ package sandbox import ( + "encoding/base64" "fmt" "path/filepath" "regexp" "sort" "strconv" "strings" + "unicode/utf16" + "unicode/utf8" ) var ( @@ -33,7 +36,9 @@ var ( // unparseableNetworkPattern is used only after the shell parser fails. At // that point the command is already marked too complex, so this intentionally // favors catching obvious network programs over proving exact shell syntax. - unparseableNetworkPattern = regexp.MustCompile(`(?i)\b(curl|wget|fetch|aria2c|ssh|scp|sftp|rsync|nc|ncat|netcat|telnet|ftp|npx|http-server|vite|next|nuxt|astro)\b|\b(npm|pnpm|yarn|bun|pip|pip2|pip3)\s+(install|add|publish|login|start|serve|dev|preview|run\s+(start|serve|dev|preview)|exec|x|dlx)\b|\bgo\s+get\b|\bgit\s+clone\b|\bpython(2|3)?\s+-m\s+(http\.server|pip\s+install)\b|\bgh\s+(api|repo\s+clone|release\s+download)\b`) + // Git needs token-aware handling below: a regex cannot reliably distinguish + // option values and executable path components from subcommands. + unparseableNetworkPattern = regexp.MustCompile(`(?i)^(?:(curl|wget|fetch|aria2c|ssh|scp|sftp|rsync|nc|ncat|netcat|telnet|ftp|npx|http-server|vite|next|nuxt|astro)(\s|$)|(npm|pnpm|yarn|bun|pip|pip2|pip3)\s+(install|add|publish|login|start|serve|dev|preview|run\s+(start|serve|dev|preview)|exec|x|dlx)\b|go\s+get\b|python(2|3)?\s+-m\s+(http\.server|pip\s+install)\b|gh\s+(api|repo\s+clone|release\s+download)\b)`) // destructiveExtraPatterns hold high-severity patterns that the legacy // destructiveCommandPattern does not already cover. Folded in from the // blueprint safe_bash.go without duplicating existing matches. @@ -52,6 +57,8 @@ var ( // mkfs. form (e.g. mkfs.ext4) not caught by the bare \bmkfs\b above when followed by a dot. regexp.MustCompile(`(?i)\bmkfs\.[a-z0-9]+\b`), } + cmdForFSingleQuotedCommandPattern = regexp.MustCompile(`(?is)(?:^|[&|;\r\n]\s*)@?(?:call\s+)?for\s+/f\b(.*?)\bin\s*\(\s*'([^']*)'\s*\)\s+do\b`) + cmdForFBacktickCommandPattern = regexp.MustCompile("(?is)(?:^|[&|;\\r\\n]\\s*)@?(?:call\\s+)?for\\s+/f\\b(.*?)\\bin\\s*\\(\\s*`([^`]*)`\\s*\\)\\s+do\\b") ) func matchesDestructive(command string) bool { @@ -66,8 +73,1055 @@ func matchesDestructive(command string) bool { return false } +// maxUnparseableShellDepth bounds `sh -c ` recursion in the fallback. +// It IS maxAnalyzerDepth rather than a copy of its value: a fallback that gave +// up a level earlier than the parseable path would drop the network category on +// exactly the deeply-nested launcher chains this path exists to fail closed on. +const maxUnparseableShellDepth = maxAnalyzerDepth + func matchesUnparseableNetwork(command string) bool { - return unparseableNetworkPattern.MatchString(command) + return matchesUnparseableNetworkAt(command, 0) +} + +// matchesUnparseableNetworkAt scans each fallback segment for a network program. +// It resolves the segment's real program the same way the parseable path does — +// past environment assignments and wrapper prefixes (sudo, env, timeout, nice, +// xargs, ...) and the option values those wrappers consume — because a fallback +// that only looked at the first token would let `sudo curl …`, `env git fetch …`, +// or `PATH=.:$PATH git push …` through. The point of this path is to fail closed +// on a command too obfuscated to parse; a wrapper prefix is the cheapest possible +// obfuscation. +// +// Resolving the program (rather than matching the network name anywhere in the +// string, as an earlier revision did) is what keeps `git status push` and +// `echo https://example.com/repo.git push` out: a network verb only counts when +// it belongs to a program actually being invoked. +func matchesUnparseableNetworkAt(command string, depth int) bool { + if depth < maxUnparseableShellDepth { + for _, payload := range fallbackCMDForFCommands(command) { + if textualPayloadUsesNetwork(payload, depth+1) || matchesUnparseableNetworkAt(payload, depth+1) { + return true + } + } + } + command = maskFallbackCMDForFLiteralBackticks(command) + for _, tokenInfo := range fallbackCommandTokenInfo(command) { + tokens := fallbackTokenValues(tokenInfo) + if depth < maxUnparseableShellDepth { + if split := envSplitCommandFields(tokens); split.recognized { + if split.executableEnvironmentDependent || fallbackBodyUsesNetwork(split.command, depth+1) { + return true + } + continue + } + } + for _, body := range fallbackCommandBodies(tokens) { + if fallbackBodyUsesNetwork(body, depth) { + return true + } + } + for _, body := range cmdCommandBodyTokenInfoCandidates(tokenInfo) { + if fallbackBodyUsesNetwork(fallbackTokenValues(body), depth) { + return true + } + } + } + return false +} + +// fallbackCommandBodies resolves the executable position of one segment under +// every command language the text may belong to. The fallback runs on input the +// POSIX parser rejected, and on Windows that input is frequently valid CMD +// source rather than broken sh — `@curl …`, `call curl …`, `if not 1==2 curl …` +// and `start "" curl …` all execute curl under cmd.exe while resolving to a +// non-program token ("@curl", "call", "not", "start") under POSIX rules. Each +// runtime's resolver therefore gets its own shot at the segment; a resolution +// can only ADD the network category, which is the direction this fail-closed +// path is allowed to err in. +func fallbackCommandBodies(tokens []string) [][]string { + if body := fallbackCommandBodyFields(tokens); len(body) > 0 { + return [][]string{body} + } + return nil +} + +// fallbackBodyUsesNetwork classifies one resolved command body, recursing into +// the payloads of launchers that run command text of their own. +func fallbackBodyUsesNetwork(body []string, depth int) bool { + if len(body) == 0 { + return false + } + program, args := executableTokenBase(body[0]), body[1:] + for program == "exec" { + body = fallbackExecCommandArgs(args) + if len(body) == 0 { + return false + } + program, args = executableTokenBase(body[0]), body[1:] + } + if program == "%comspec%" { + program = "cmd" + } + if networkPrograms[program] || localServerPrograms[program] { + return true + } + if program == "git" && matchesUnparseableGitNetwork(args) { + return true + } + if program == "busybox" { + if command := busyboxCommandArgs(args); len(command) > 0 { + if fallbackTokenLooksDynamic(command[0]) { + return true + } + if depth >= maxUnparseableShellDepth || fallbackBodyUsesNetwork(command, depth+1) { + return true + } + } + } + if program == "strace" { + if command := straceCommandArgs(args); len(command) > 0 { + if fallbackTokenLooksDynamic(command[0]) { + return true + } + if depth >= maxUnparseableShellDepth || fallbackBodyUsesNetwork(command, depth+1) { + return true + } + } + } + if unparseableNetworkPattern.MatchString(strings.Join(append([]string{program}, args...), " ")) { + return true + } + // eval executes its remaining arguments as shell source. Recurse into that + // source just as we do for `sh -c`; otherwise quoting the same curl/git + // invocation behind eval would hide it from this fail-closed path. + if program == "eval" && len(args) > 0 { + if depth >= maxUnparseableShellDepth || matchesUnparseableNetworkAt(strings.Join(args, " "), depth+1) { + return true + } + } + if program == "env" { + if split := envSplitCommand(args); split.recognized { + return split.executableEnvironmentDependent || + (len(split.command) > 0 && (depth >= maxUnparseableShellDepth || fallbackBodyUsesNetwork(split.command, depth+1))) + } + } + // `sh -c ` runs the payload as a fresh command. The fallback + // tokenizer keeps a quoted payload as ONE token, so the network program + // inside it is not a token of this segment at all — recurse the way + // analyzeInto does on the parseable path. + if shellPrograms[program] { + if payloadIndex, found := shellCommandPayloadIndex(program, args); found && payloadIndex < len(args) { + if payload := args[payloadIndex]; payload != "" { + if depth >= maxUnparseableShellDepth || matchesUnparseableNetworkAt(payload, depth+1) { + return true + } + } + } + } + // PowerShell source that exists but cannot be read is not evidence that the + // command is local. Fail closed on it here, before payload extraction drops + // it as empty. + if program == "powershell" || program == "pwsh" { + if fallbackPowerShellPayload(program, args).opaque { + return true + } + } + // Windows command interpreters carry command text after their command + // flag. That payload is valid shell input even when the POSIX parser that + // sent us here cannot parse it (for example, `cmd /c curl ... & rem '`). + if payload := fallbackCommandInterpreterPayload(program, args); payload != "" { + if depth >= maxUnparseableShellDepth || + matchesUnparseableNetworkAt(payload, depth+1) || + fallbackPayloadUsesNetwork(strings.Join(fallbackCommandInterpreterArgs(program, args), " "), depth+1) { + return true + } + } + return false +} + +// fallbackTokenLooksDynamic reports whether a token resolved as a wrapper's +// delegated child program (busybox's applet, strace's traced command) still +// carries unresolved shell syntax — a bare $VAR, ${VAR}, $(...), or a +// backtick substitution. This fallback tokenizer runs on text the POSIX +// parser rejected and never expands anything, so `busybox "$APPLET" …` +// arrives with the literal token `$APPLET` rather than a blank one. Matching +// that token against known program names silently reads "cannot resolve" as +// "not a network program"; fail closed instead, the same direction +// busyboxSourceDynamic/straceSourceDynamic already fail closed in on the AST +// path for the equivalent gap. +func fallbackTokenLooksDynamic(token string) bool { + return strings.ContainsAny(token, "$`") +} + +func fallbackPayloadUsesNetwork(payload string, depth int) bool { + for _, tokenInfo := range fallbackCommandTokenInfo(payload) { + if len(tokenInfo) > 0 && strings.EqualFold(tokenInfo[0].value, "start") { + if body := cmdStartPayloadTokenInfo(tokenInfo[1:]); len(body) > 0 && fallbackBodyUsesNetwork(fallbackTokenValues(body), depth) { + return true + } + } + for _, body := range cmdCommandBodyTokenInfoCandidates(tokenInfo) { + if fallbackBodyUsesNetwork(fallbackTokenValues(body), depth) { + return true + } + } + } + return false +} + +var fallbackLeadingShellKeywords = map[string]bool{ + "!": true, "if": true, "while": true, "until": true, "then": true, + "do": true, "else": true, "elif": true, "in": true, "coproc": true, + "{": true, "}": true, +} + +// fallbackCommandBodyFields resolves a command at a shell-control boundary. +// In addition to ordinary wrappers, an unparseable compound command can put +// control-flow keywords and redirections before the executable (`then curl`, +// `do wget`, `>out git push`). Those prefixes do not change the program being +// invoked and must not make the fail-closed network check lose sight of it. +func fallbackCommandBodyFields(fields []string) []string { + // Shell keywords are syntax only at the original command boundary. Do not + // reinterpret a wrapper's payload as syntax: `command if curl` attempts to + // execute a program named "if" and does not invoke curl. + for len(fields) > 0 && fallbackLeadingShellKeywords[strings.ToLower(fields[0])] { + fields = fields[1:] + } + body := commandBodyFields(fields) + for len(body) > 0 { + word := strings.ToLower(body[0]) + if consumesRedirectTarget(word) { + if len(body) == 1 { + return nil + } + body = commandBodyFields(body[2:]) + continue + } + if isRedirectToken(word) { + body = commandBodyFields(body[1:]) + continue + } + return body + } + return nil +} + +// cmdComparisonOperators are CMD's IF comparison operators, which sit between +// the two values being compared (`if %x% equ 1 curl …`). +var cmdComparisonOperators = map[string]bool{ + "equ": true, "neq": true, "lss": true, "leq": true, "gtr": true, "geq": true, +} + +type fallbackCommandToken struct { + value string + quoted bool +} + +func fallbackTokenValues(tokens []fallbackCommandToken) []string { + values := make([]string, len(tokens)) + for index := range tokens { + values[index] = tokens[index].value + } + return values +} + +func cmdCommandBodyTokenInfoCandidates(fields []fallbackCommandToken) [][]fallbackCommandToken { + fields = normalizeCMDCommandTokens(fields) + for len(fields) > 0 { + fields = trimCMDEchoPrefix(fields) + if len(fields) == 0 { + return nil + } + switch strings.ToLower(fields[0].value) { + case "call", "else", "then", "do": + fields = fields[1:] + case "cmd", "cmd.exe", "%comspec%": + payload := cmdInterpreterPayloadTokenInfo(fields[1:]) + if len(payload) == 0 { + return nil + } + fields = payload + case "start": + body := cmdStartPayloadTokenInfo(fields[1:]) + if len(body) == 0 { + return nil + } + body = trimCMDEchoPrefix(body) + if len(body) == 0 { + return nil + } + return [][]fallbackCommandToken{body} + case "if": + values := cmdConditionPayload(fallbackTokenValues(fields[1:])) + if len(values) == 0 { + return nil + } + fields = fields[len(fields)-len(values):] + case "for": + values := cmdForPayload(fallbackTokenValues(fields[1:])) + if len(values) == 0 { + return nil + } + fields = fields[len(fields)-len(values):] + default: + return [][]fallbackCommandToken{fields} + } + } + return nil +} + +func cmdInterpreterPayloadTokenInfo(fields []fallbackCommandToken) []fallbackCommandToken { + for index, field := range fields { + flag := strings.ToLower(field.value) + if !strings.HasPrefix(flag, "/c") && !strings.HasPrefix(flag, "/k") { + continue + } + if len(field.value) > 2 { + joined := field + joined.value = field.value[2:] + return append([]fallbackCommandToken{joined}, fields[index+1:]...) + } + index++ + for index < len(fields) && isCMDInterpreterSwitch(fields[index].value) { + index++ + } + return fields[index:] + } + return nil +} + +func normalizeCMDCommandTokens(fields []fallbackCommandToken) []fallbackCommandToken { + normalized := make([]fallbackCommandToken, 0, len(fields)) + for _, field := range fields { + if field.quoted { + normalized = append(normalized, field) + continue + } + // The shared tokenizer reads `\` as a POSIX escape, so a Windows path that + // ends in a separator swallows the following word: `start "" /d C:\ curl …` + // arrives as the single token `C:\ curl`, which lets a value-taking switch + // consume the executable. CMD has no such escape, so split those apart + // before resolving CMD's own grammar. + for _, part := range splitCMDEscapedWhitespace(field.value) { + normalized = append(normalized, fallbackCommandToken{value: normalizeCMDToken(part)}) + } + } + return normalized +} + +func splitCMDEscapedWhitespace(value string) []string { + var parts []string + start := 0 + for index := 0; index+1 < len(value); index++ { + if value[index] != '\\' || (value[index+1] != ' ' && value[index+1] != '\t') { + continue + } + if part := value[start : index+1]; part != "" { + parts = append(parts, part) + } + start = index + 2 + index++ + } + if part := value[start:]; part != "" || len(parts) == 0 { + parts = append(parts, part) + } + return parts +} + +func trimCMDEchoPrefix(fields []fallbackCommandToken) []fallbackCommandToken { + for len(fields) > 0 { + fields[0].value = strings.TrimLeft(fields[0].value, "@") + if fields[0].value != "" { + break + } + fields = fields[1:] + } + return fields +} + +// cmdStartPayloadTokenInfo resolves the command START will launch. START's +// grammar is switches, an optional quoted window title, then more switches: +// `start "" /b curl …` is a real invocation of curl. A single prefix strip left +// `/b` in the executable position, so neither the CMD nor the POSIX resolver +// contributed the network category for that form. +func cmdStartPayloadTokenInfo(fields []fallbackCommandToken) []fallbackCommandToken { + fields = cmdStartOperandsTokenInfo(fields) + if len(fields) > 0 && fields[0].quoted { + fields = cmdStartOperandsTokenInfo(fields[1:]) + } + return fields +} + +func cmdStartOperandsTokenInfo(fields []fallbackCommandToken) []fallbackCommandToken { + for len(fields) > 0 && strings.HasPrefix(fields[0].value, "/") { + switch strings.ToLower(fields[0].value) { + case "/d", "/node", "/affinity": + if len(fields) < 2 { + return nil + } + fields = fields[2:] + default: + fields = fields[1:] + } + } + return fields +} + +// cmdConditionPayload skips an IF condition and returns the guarded command. +func cmdConditionPayload(fields []string) []string { + for len(fields) > 0 { + word := strings.ToLower(strings.Trim(fields[0], `"`)) + switch { + case strings.HasPrefix(word, "/"): // /I, /D — case-insensitivity switches + fields = fields[1:] + case word == "not": + fields = fields[1:] + case word == "errorlevel" || word == "exist" || word == "defined" || word == "cmdextversion": + // Keyword plus its single operand. + if len(fields) < 2 { + return nil + } + return fields[2:] + case len(fields) >= 2 && (fields[1] == "==" || cmdComparisonOperators[strings.ToLower(fields[1])]): + // A spaced comparison: . + if len(fields) < 3 { + return nil + } + return fields[3:] + case strings.Contains(word, "=="): + // The whole comparison arrived as one token (`1==1`). + return fields[1:] + default: + // Unknown condition grammar cannot establish a command boundary. + return nil + } + } + return nil +} + +// cmdForPayload returns the command after DO, which is the only part of a FOR +// loop that executes. `for %i in (curl) do echo %i` must not resolve to curl. +func cmdForPayload(fields []string) []string { + for index, field := range fields { + if strings.EqualFold(strings.Trim(field, `"`), "do") { + return fields[index+1:] + } + } + return nil +} + +// fallbackCMDForFCommands extracts the command source that CMD FOR /F executes +// from its IN clause. Without usebackq, single quotes denote command text; with +// usebackq, backticks do. The other quote form is literal input and must not be +// classified as an invocation. +func fallbackCMDForFCommands(command string) []string { + var payloads []string + commandBoundaries := fallbackCMDCommandBoundaries(command) + for _, indexes := range cmdForFSingleQuotedCommandPattern.FindAllStringSubmatchIndex(command, -1) { + if len(indexes) < 6 || !commandBoundaries[indexes[0]] { + continue + } + match := []string{command[indexes[0]:indexes[1]], command[indexes[2]:indexes[3]], command[indexes[4]:indexes[5]]} + if len(match) == 3 && !cmdForFUsesBackq(match[1]) { + payloads = append(payloads, match[2]) + } + } + for _, indexes := range cmdForFBacktickCommandPattern.FindAllStringSubmatchIndex(command, -1) { + if len(indexes) < 6 || !commandBoundaries[indexes[0]] { + continue + } + match := []string{command[indexes[0]:indexes[1]], command[indexes[2]:indexes[3]], command[indexes[4]:indexes[5]]} + if len(match) == 3 && cmdForFUsesBackq(match[1]) { + payloads = append(payloads, match[2]) + } + } + return payloads +} + +func cmdForFUsesBackq(options string) bool { + for _, option := range strings.Fields(strings.Trim(options, ` "`)) { + if strings.EqualFold(strings.Trim(option, `"`), "usebackq") { + return true + } + } + return false +} + +func maskFallbackCMDForFLiteralBackticks(command string) string { + masked := []byte(command) + commandBoundaries := fallbackCMDCommandBoundaries(command) + for _, indexes := range cmdForFBacktickCommandPattern.FindAllStringSubmatchIndex(command, -1) { + if len(indexes) < 6 || !commandBoundaries[indexes[0]] || + cmdForFUsesBackq(command[indexes[2]:indexes[3]]) { + continue + } + for index := indexes[4]; index < indexes[5]; index++ { + masked[index] = ' ' + } + } + return string(masked) +} + +// fallbackCMDCommandBoundaries records whether each byte offset is outside a +// quoted or caret-escaped CMD region. FOR /F regex matches can then check their +// start in O(1) instead of rescanning every preceding byte for every match. +func fallbackCMDCommandBoundaries(command string) []bool { + boundaries := make([]bool, len(command)+1) + quoted := false + escaped := false + for index := 0; index < len(command); index++ { + boundaries[index] = !quoted && !escaped + switch { + case escaped: + escaped = false + case command[index] == '^': + escaped = true + case command[index] == '"': + quoted = !quoted + } + } + boundaries[len(command)] = !quoted && !escaped + return boundaries +} + +func consumesRedirectTarget(word string) bool { + word = strings.TrimLeft(word, "0123456789") + return word == ">" || word == ">>" || word == "<" || word == "<<" || word == "<<-" || + word == "<<<" || word == "<>" || word == ">|" +} + +func isRedirectToken(word string) bool { + word = strings.TrimLeft(word, "0123456789") + return strings.HasPrefix(word, ">") || strings.HasPrefix(word, "<") +} + +// matchesUnparseableGitNetwork reports whether git's arguments (everything after +// the executable) name a subcommand that talks to a remote. +// +// It defers to gitUsesNetwork rather than reading the option list a second time. +// The two paths disagreeing is not a theoretical risk: while each kept its own +// terminal-option rule, `git -h push` was network on one path and local on the +// other, and every future option would have had to be added to both. +func matchesUnparseableGitNetwork(args []string) bool { + return gitUsesNetwork(args) +} + +// shellCommandPayloadIndex returns the one argv element a shell launcher will +// execute for -c. It parses only the leading option region: a script operand or +// `--` makes later `-c` text positional, and an invalid option cluster is not +// treated as executable source. +func shellCommandPayloadIndex(program string, args []string) (int, bool) { + noExecute, dumpStrings := false, false + for index := 0; index < len(args); index++ { + option := args[index] + if option == "--" || option == "-" || option == "+" || + (!strings.HasPrefix(option, "-") && !strings.HasPrefix(option, "+")) { + return 0, false + } + if strings.HasPrefix(option, "--") { + if program != "bash" { + return 0, false + } + name := option + if equals := strings.IndexByte(option, '='); equals >= 0 { + name = option[:equals] + } + switch name { + case "--dump-po-strings", "--dump-strings": + return 0, false + case "--debug", "--debugger", "--login", "--noediting", "--noprofile", "--norc", "--posix", + "--pretty-print", "--restricted", "--verbose": + if name != option { + return 0, false + } + case "--init-file", "--rcfile": + if name == option { + index++ + if index >= len(args) { + return 0, false + } + } + case "--help", "--version": + return 0, false + default: + return 0, false + } + continue + } + + validOptions := "abefhkmnptuvxBCEHPTilrsDc" + valueOptions := "o" + switch program { + case "bash": + valueOptions += "O" + case "dash", "sh": + validOptions = "abCefnuvxIimspc" + default: + // ksh/zsh share the common invocation flags used here, including -l. + validOptions = "abefhkmnptuvxBCilrsc" + } + command, values := false, 0 + enable := option[0] == '-' + for _, flag := range option[1:] { + switch { + case flag == 'c': + command = true + case flag == 'n': + noExecute = enable + case program == "bash" && flag == 'D': + dumpStrings = true + case strings.ContainsRune(valueOptions, flag): + values++ + case strings.ContainsRune(validOptions, flag): + default: + return 0, false + } + } + index += values + if index >= len(args) { + return 0, false + } + if command { + if noExecute || dumpStrings { + return 0, false + } + return index + 1, true + } + } + return 0, false +} + +func fallbackCommandInterpreterPayload(program string, args []string) string { + for index, arg := range args { + flag := strings.ToLower(arg) + if program == "cmd" && (strings.HasPrefix(flag, "/c") || strings.HasPrefix(flag, "/k")) { + if len(arg) > 2 { + return strings.TrimSpace(strings.Join(append([]string{arg[2:]}, args[index+1:]...), " ")) + } + index++ + for index < len(args) && isCMDInterpreterSwitch(args[index]) { + index++ + } + return strings.Join(args[index:], " ") + } + } + if program == "powershell" || program == "pwsh" { + return fallbackPowerShellPayload(program, args).payload + } + return "" +} + +func fallbackCommandInterpreterArgs(program string, args []string) []string { + for index, arg := range args { + flag := strings.ToLower(arg) + if program == "cmd" && (strings.HasPrefix(flag, "/c") || strings.HasPrefix(flag, "/k")) { + if len(arg) > 2 { + return append([]string{arg[2:]}, args[index+1:]...) + } + index++ + for index < len(args) && isCMDInterpreterSwitch(args[index]) { + index++ + } + return args[index:] + } + } + return nil +} + +func isCMDInterpreterSwitch(arg string) bool { + flag := strings.ToLower(arg) + switch flag { + case "/d", "/s", "/q", "/a", "/u": + return true + default: + return strings.HasPrefix(flag, "/e:") || strings.HasPrefix(flag, "/f:") || strings.HasPrefix(flag, "/v:") + } +} + +// powerShellPayload describes what a PowerShell host invocation will execute. +// +// The three outcomes are distinct and must not be collapsed: readable source +// (payload), source that exists but cannot be read statically (opaque), and no +// inline source at all (a script File, a version query, an invalid switch). +// Treating the middle case as the last one is how `-EncodedCommand ` parsed cleanly and was allowed without a +// network grant. +type powerShellPayload struct { + payload string + opaque bool + // sourceIndex is where the command source begins in args, or -1 when the + // invocation carries none. Callers on the AST path use it to check whether + // the source words are static literals. + sourceIndex int +} + +// maxPowerShellEncodedCommandBytes bounds the attacker-controlled base64 this +// path will decode. +const maxPowerShellEncodedCommandBytes = 64 << 10 + +func fallbackPowerShellPayload(program string, args []string) powerShellPayload { + none := powerShellPayload{sourceIndex: -1} + for index := 0; index < len(args); index++ { + arg := args[index] + if !strings.HasPrefix(arg, "-") && !strings.HasPrefix(arg, "/") { + // Windows PowerShell's first positional parameter is Command. PowerShell + // 6+ changed pwsh's default to File, a script path rather than source. + if program == "pwsh" { + return none + } + return powerShellPayload{payload: strings.Join(args[index:], " "), sourceIndex: index} + } + flag := strings.TrimLeft(strings.ToLower(arg), "-/") + if strings.Contains(flag, ":") { + // The native host does not accept colon-joined option values. + return none + } + switch { + case program == "pwsh" && (flag == "commandwithargs" || flag == "cwa"): + if index+1 < len(args) { + return powerShellPayload{payload: args[index+1], sourceIndex: index + 1} + } + return none + case powerShellFlagAbbreviates(flag, "command"): + if index+1 < len(args) { + return powerShellPayload{payload: strings.Join(args[index+1:], " "), sourceIndex: index + 1} + } + return none + case flag == "ec" || powerShellFlagAbbreviates(flag, "encodedcommand"): + if index+1 >= len(args) { + return none + } + // Encoded source is still source. Decode the bounded, valid forms so a + // benign payload stays quiet, and fail closed on anything that does not + // decode rather than reading it as network-free. + if decoded, ok := decodePowerShellEncodedCommand(args[index+1]); ok { + return powerShellPayload{payload: decoded, sourceIndex: index + 1} + } + return powerShellPayload{opaque: true, sourceIndex: index + 1} + case powerShellFlagAbbreviates(flag, "file"): + // A script path, not inline source: nothing here to classify. + return none + case program == "pwsh" && (flag == "v" || flag == "version"): + return none + case powerShellOptionConsumesValue(program, flag): + if index+1 >= len(args) { + return none + } + index++ + case powerShellValuelessOption(program, flag): + case flag == "h" || flag == "help" || flag == "?": + return none + default: + // Unknown host switches terminate option parsing with an error; do not + // reinterpret a later network-looking argument as PowerShell source. + return none + } + } + return none +} + +// decodePowerShellEncodedCommand decodes the base64 UTF-16LE source carried by +// -EncodedCommand. A value that is not valid base64, not an even number of +// bytes, or larger than the bound is not decodable here and is reported as +// such so the caller can fail closed. +func decodePowerShellEncodedCommand(value string) (string, bool) { + value = strings.Trim(value, `"'`) + if value == "" || len(value) > maxPowerShellEncodedCommandBytes { + return "", false + } + raw, err := base64.StdEncoding.DecodeString(value) + if err != nil { + return "", false + } + if len(raw) == 0 || len(raw)%2 != 0 { + return "", false + } + units := make([]uint16, 0, len(raw)/2) + for index := 0; index+1 < len(raw); index += 2 { + units = append(units, uint16(raw[index])|uint16(raw[index+1])<<8) + } + decoded := string(utf16.Decode(units)) + if strings.ContainsRune(decoded, 0) || !utf8.ValidString(decoded) { + return "", false + } + return decoded, true +} + +func powerShellFlagAbbreviates(flag, fullName string) bool { + return flag != "" && strings.HasPrefix(fullName, flag) +} + +func powerShellOptionConsumesValue(program, flag string) bool { + switch flag { + case "configurationfile", "configurationname", "config", "custompipename", + "encodedarguments", "ea", + "executionpolicy", "ex", "ep", "inputformat", "inp", "if", + "outputformat", "o", "of", "psconsolefile", "settingsfile", "settings", + "windowstyle", "w", "workingdirectory", "wd", "wo": + return true + case "version", "v": + return program == "powershell" + default: + return false + } +} + +func powerShellValuelessOption(program, flag string) bool { + switch flag { + case "mta", "sta", "noexit", "noe", "nologo", "nol", "noninteractive", "noni", + "noprofile", "nop": + return true + case "interactive", "login", "noprofileloadtime", "sshservermode": + return program == "pwsh" + default: + return false + } +} + +func fallbackExecCommandArgs(args []string) []string { + for index := 0; index < len(args); index++ { + if args[index] == "--" { + return args[index+1:] + } + if args[index] == "-a" || args[index] == "--argv0" { + if index+1 >= len(args) { + return nil + } + index++ + continue + } + if strings.HasPrefix(args[index], "-") { + continue + } + return args[index:] + } + return nil +} + +func normalizeCMDToken(token string) string { + var out strings.Builder + for index := 0; index < len(token); index++ { + if token[index] == '^' && index+1 < len(token) { + index++ + } + out.WriteByte(token[index]) + } + return out.String() +} + +// executableTokenBase reduces a raw fallback token to a comparable program name. +// It strips quoting, any directory prefix, and a Windows executable suffix, so +// this path recognizes curl.exe and git.cmd exactly as normalizeProgramToken does +// on the parseable path — a token that normalized differently here used to be how +// `curl.exe https://… && "unterminated` lost its network classification. +// +// Drive-relative spellings go through windowsExecutablePathBasename for the same +// reason: `C:git.exe` has no separator to cut on, so a plain basename scan leaves +// `c:git` and the deny never matches a program the parseable path classifies. +func executableTokenBase(token string) string { + token = strings.Trim(token, `\"'`) + if basename, ok := windowsExecutablePathBasename(token); ok { + token = basename + } else if slash := strings.LastIndexAny(token, `/\`); slash >= 0 { + token = token[slash+1:] + } + return trimExecutableSuffix(strings.ToLower(token)) +} + +type fallbackDelimiterFrame struct { + opener rune + quote rune +} + +// fallbackCommandTokens performs deliberately small shell/cmd tokenization. +// It preserves quoted spaces even when the command's trailing quote is +// unmatched (the condition that sends classification down this fallback). +func fallbackCommandTokens(command string) [][]string { + infos := fallbackCommandTokenInfo(command) + commands := make([][]string, len(infos)) + for index := range infos { + commands[index] = fallbackTokenValues(infos[index]) + } + return commands +} + +func fallbackCommandTokenInfo(command string) [][]fallbackCommandToken { + // Command strings commonly preserve cmd.exe's escaped quote spelling. + command = strings.ReplaceAll(command, `\"`, `"`) + var commands [][]fallbackCommandToken + var tokens []fallbackCommandToken + var word strings.Builder + var quote rune + wordQuoted := false + var delimiters []fallbackDelimiterFrame + backtick := false + escaped := false + flush := func() { + if word.Len() > 0 || wordQuoted { + tokens = append(tokens, fallbackCommandToken{value: word.String(), quoted: wordQuoted}) + word.Reset() + wordQuoted = false + } + } + flushCommand := func() { + flush() + if len(tokens) > 0 { + commands = append(commands, tokens) + tokens = nil + } + } + runes := []rune(command) + for index, r := range runes { + if escaped { + word.WriteRune(r) + escaped = false + continue + } + if r == '\\' { + word.WriteRune(r) + escaped = true + continue + } + // Backticks execute inside unquoted and double-quoted text, but are + // literal inside single quotes. Preserve the surrounding quote while the + // substitution body is scanned as its own command. + if r == '`' && quote != '\'' { + flushCommand() + if backtick { + if len(delimiters) > 0 && delimiters[len(delimiters)-1].opener == '`' { + quote = delimiters[len(delimiters)-1].quote + delimiters = delimiters[:len(delimiters)-1] + } + } else { + delimiters = append(delimiters, fallbackDelimiterFrame{opener: '`', quote: quote}) + quote = 0 + } + backtick = !backtick + continue + } + if r == '\'' || r == '"' { + switch quote { + case 0: + quote = r + wordQuoted = true + case r: + quote = 0 + default: + word.WriteRune(r) + } + continue + } + // A verified function declaration makes its brace or subshell body a new + // executable region. Without this boundary, `f(){ curl ...; }; f` resolves + // the whole segment to the syntax token `f()` and hides the body. + if r == '{' && quote == 0 && fallbackFunctionDeclaration(tokens, word.String()) { + flushCommand() + continue + } + // Command and process substitutions execute even inside double quotes. + // Ordinary/arithmetic/array parentheses do not: splitting all parens made + // `${curl}`, `$((curl))`, and `arr=(curl)` look like curl invocations. + if r == '(' && quote != '\'' { + current := word.String() + if quote == 0 && current == "" && fallbackFunctionDeclaration(tokens, "") { + flushCommand() + delimiters = append(delimiters, fallbackDelimiterFrame{opener: '(', quote: 0}) + continue + } + nextIsParen := index+1 < len(runes) && runes[index+1] == '(' + substitution := (strings.HasSuffix(current, "$") && !nextIsParen) || + strings.HasSuffix(current, "<") || strings.HasSuffix(current, ">") + // CMD conditionals put the command group after condition tokens, e.g. + // `if 1==1 (curl ...)`; the opening parenthesis is still a command + // boundary even though it is not the segment's first token. + grouping := quote == 0 && word.Len() == 0 && + (len(tokens) == 0 || startsCMDCommandGroup(fallbackTokenValues(tokens))) + if substitution || grouping { + flushCommand() + delimiters = append(delimiters, fallbackDelimiterFrame{opener: '(', quote: quote}) + quote = 0 + continue + } + } + if r == ')' && quote == 0 && len(delimiters) > 0 && delimiters[len(delimiters)-1].opener == '(' { + flushCommand() + quote = delimiters[len(delimiters)-1].quote + delimiters = delimiters[:len(delimiters)-1] + continue + } + // A case pattern's closing parenthesis starts the command body. It is not + // paired with an opening command-group parenthesis. + if r == ')' && quote == 0 && len(tokens) > 0 && tokens[0].value == "case" { + flushCommand() + continue + } + // A newline separates commands exactly as ;/&/| do. Treating it as mere + // whitespace kept a multi-line script as one segment, so anything after the + // first line was scanned as arguments of the first line's program and the + // program on line two was never resolved. + if quote == 0 && (r == ';' || r == '&' || r == '|' || r == '\n' || r == '\r') { + flushCommand() + continue + } + if quote == 0 && (r == ' ' || r == '\t') { + flush() + continue + } + word.WriteRune(r) + } + flushCommand() + return commands +} + +func fallbackFunctionDeclaration(tokens []fallbackCommandToken, current string) bool { + fields := fallbackTokenValues(tokens) + if current != "" { + fields = append(fields, current) + } + switch len(fields) { + case 1: + return fallbackFunctionNameToken(fields[0]) + case 2: + return (isShellFunctionName(fields[0]) && fields[1] == "()") || + (strings.EqualFold(fields[0], "function") && + (fallbackFunctionNameToken(fields[1]) || isShellFunctionName(fields[1]))) + case 3: + return strings.EqualFold(fields[0], "function") && isShellFunctionName(fields[1]) && fields[2] == "()" + default: + return false + } +} + +func fallbackFunctionNameToken(token string) bool { + return strings.HasSuffix(token, "()") && isShellFunctionName(strings.TrimSuffix(token, "()")) +} + +func isShellFunctionName(name string) bool { + if name == "" { + return false + } + for index, r := range name { + if (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || r == '_' || (index > 0 && r >= '0' && r <= '9') { + continue + } + return false + } + return true +} + +func startsCMDCommandGroup(tokens []string) bool { + if len(tokens) == 0 { + return false + } + switch strings.TrimPrefix(strings.ToLower(tokens[0]), "@") { + case "if", "else": + return true + case "for": + // Parentheses after IN contain data; only the group after DO executes. + return strings.EqualFold(tokens[len(tokens)-1], "do") + default: + return false + } } func Classify(request Request) Risk { diff --git a/internal/sandbox/risk_hardening_test.go b/internal/sandbox/risk_hardening_test.go index ce2f89994..b529a780c 100644 --- a/internal/sandbox/risk_hardening_test.go +++ b/internal/sandbox/risk_hardening_test.go @@ -1,6 +1,9 @@ package sandbox -import "testing" +import ( + "strings" + "testing" +) func classifyCommand(command string) Risk { return Classify(Request{ @@ -273,6 +276,9 @@ func TestClassifyASTCatchesNetworkProgramsRegexMisses(t *testing.T) { "ftp ftp.example.com", "sftp user@host", "sudo telnet example.com 23", + "git fetch origin", + "git pull origin main", + "git push gitlawb://example.com/repo.git main", } { risk := classifyCommand(command) if risk.Level != RiskCritical || !HasRiskCategory(risk, "network") { @@ -291,12 +297,558 @@ func TestClassifyFlagsUnparseableCommand(t *testing.T) { } func TestClassifyUnparseableNetworkCommandFailsClosed(t *testing.T) { - risk := classifyCommand(`curl https://example.com && "unterminated`) - if !HasRiskCategory(risk, "unparseable_command") { - t.Fatalf("Classify of unparseable network command = categories %v; want unparseable_command", risk.Categories) + for _, command := range []string{ + `curl https://example.com && "unterminated`, + `git fetch origin && "unterminated`, + `git pull origin main && "unterminated`, + `git push gitlawb://example.com/repo.git main && "unterminated`, + `git ls-remote gitlawb://example.com/repo.git & rem '`, + `git archive --remote=gitlawb://example.com/repo.git HEAD & rem '`, + `git -C repo push gitlawb://example.com/repo.git main && "unterminated`, + // git.exe runs under cmd.exe, which has no notion of a trailing single + // quote — this parses fine there but fails the POSIX shell parser used + // by AnalyzeCommand, so it must still be caught by the regex fallback. + `git.exe push origin main & rem '`, + `git.cmd push origin main & rem '`, + // cmd.exe accepts quoted executable paths, option values, and verbs. + // Preserve those token boundaries when the trailing REM quote forces the + // fallback path, including joined short and long option-value forms. + `"C:\Program Files\Git\cmd\git.exe" "push" origin main & rem '`, + `git.exe -C "C:\Program Files\repo" push origin main & rem '`, + `git.exe -C "C:\Program Files\repo" "push" origin main & rem '`, + `git.exe -C"C:\Program Files\repo" "push" origin main & rem '`, + `git.exe --git-dir="C:\Program Files\repo\.git" push origin main & rem '`, + `git.exe "--git-dir=C:\Program Files\repo\.git" push origin main & rem '`, + `git -C repo push origin main & rem '`, + `git -c user.name=test fetch origin & rem '`, + `git -C "C:\Program Files\repo" push origin main & rem '`, + // More value-taking global options than the fallback regex used to cap + // its generic-token scan at (formerly {0,8}) — every option here still + // precedes the actual subcommand. + `git -c a=1 -c b=2 -c c=3 -c d=4 -c e=5 push gitlawb://example.com/repo.git main && "unterminated`, + } { + t.Run(command, func(t *testing.T) { + risk := classifyCommand(command) + if !HasRiskCategory(risk, "unparseable_command") { + t.Errorf("Classify(%q) = categories %v; want unparseable_command", command, risk.Categories) + } + if risk.Level != RiskCritical || !HasRiskCategory(risk, "network") { + t.Errorf("Classify(%q) = level %s, categories %v; want critical network", command, risk.Level, risk.Categories) + } + }) + } +} + +// TestClassifyUnparseableNetworkBehindWrapperFailsClosed is the regression test +// for jatmn's #726 P2/P3 findings: resolving the fallback's program from +// tokens[0] alone dropped the network category whenever the real program sat +// behind a wrapper (sudo/env/timeout/xargs), an environment assignment, a shell +// launcher's -c payload, a Windows executable suffix, or a newline — each of +// which base main still caught with its whole-string match. An unparseable +// command is already too obfuscated to analyze, so a wrapper prefix must not be +// enough to buy egress without a prompt. +func TestClassifyUnparseableNetworkBehindWrapperFailsClosed(t *testing.T) { + for _, command := range []string{ + // Wrapper programs, including ones whose options consume a value. + `sudo curl https://example.com && "unterminated`, + `sudo -u root curl https://example.com && "unterminated`, + `env curl https://example.com && "unterminated`, + `env git fetch origin && "unterminated`, + `sudo git push origin main && "unterminated`, + `sudo npm install && "unterminated`, + `timeout 5 curl https://example.com && "unterminated`, + `xargs curl https://example.com && "unterminated`, + // Environment-assignment prefixes. + `PATH=.:$PATH git push origin main && "unterminated`, + `GIT_SSH_COMMAND=ssh git push origin main && "unterminated`, + // A shell launcher's payload is a single token to the fallback tokenizer, + // so the program inside it is only visible by recursing into it. + `sh -c 'curl https://example.com' && "unterminated`, + `bash -c "git push origin main" && "unterminated`, + // Windows executable suffixes normalize on the parseable path already. + `curl.exe https://example.com && "unterminated`, + `wget.exe https://example.com && "unterminated`, + `sudo curl.exe https://example.com && "unterminated`, + // A newline separates commands; the network program is on its own line. + "true\ncurl https://example.com && \"unterminated", + "echo start\r\ngit push origin main && \"unterminated", + // Shell short-option clusters still carry one command payload. + `bash -lc 'curl https://example.com' && "unterminated`, + `bash -ce "git push origin main" && "unterminated`, + `dash -ce 'curl https://example.com' && "unterminated`, + `bash +n -c 'curl https://example.com' && "unterminated`, + // A drive-relative Windows spelling has no separator to cut on, so the + // basename scan alone left "c:git" and never matched (same review). + `C:git.exe push origin main & rem '`, + `C:curl.exe https://example.com & rem '`, + // Recursion goes through more than one launcher layer. + `sh -c "sh -c 'curl https://example.com'" && "unterminated`, + } { + t.Run(command, func(t *testing.T) { + risk := classifyCommand(command) + if !HasRiskCategory(risk, "unparseable_command") { + t.Errorf("Classify(%q) = categories %v; want unparseable_command", command, risk.Categories) + } + if risk.Level != RiskCritical || !HasRiskCategory(risk, "network") { + t.Errorf("Classify(%q) = level %s, categories %v; want critical network", command, risk.Level, risk.Categories) + } + }) + } +} + +// TestClassifyUnparseableNetworkInShellConstructFailsClosed covers compound +// shell forms where the invoked program is not the first token after a basic +// ;/&/| separator. The fallback must still find the real invocation without +// returning to an anywhere-in-the-string regex that would misclassify +// `git status push` and URL/path text containing `.git push`. +func TestClassifyUnparseableNetworkInShellConstructFailsClosed(t *testing.T) { + for _, command := range []string{ + `echo $(curl https://evil.test) && "unterminated`, + "echo `curl https://evil.test` && \"unterminated", + `x=$(curl https://evil.test) && "unterminated`, + `echo "$(curl https://evil.test)" && "unterminated`, + "echo \"`git push`\" && \"unterminated", + `x="$(curl https://evil.test)" && "unterminated`, + `(curl https://evil.test) && "unterminated`, + `( curl https://evil.test ) && "unterminated`, + `{ curl https://evil.test ; } && "unterminated`, + `cat <(curl https://evil.test) && "unterminated`, + `if true; then curl https://evil.test; fi && "unterminated`, + `for i in 1 2; do curl https://evil.test; done && "unterminated`, + `while :; do wget https://evil.test; done && "unterminated`, + `case x in x) curl https://evil.test;; esac && "unterminated`, + `>out curl https://evil.test && "unterminated`, + `2>err curl https://evil.test && "unterminated`, + `<<< payload curl https://evil.test && "unterminated`, + `<<- EOF curl https://evil.test && "unterminated`, + `coproc curl https://evil.test; wait && "unterminated`, + `eval "curl https://evil.test" && "unterminated`, + `! curl https://evil.test && "unterminated`, + `if true; then git push; fi && "unterminated`, + `(git -C repo push) && "unterminated`, + `echo $(git push) && "unterminated`, + `eval "git push" && "unterminated`, + `f() { curl https://evil.test; }; f && "unterminated`, + `f () { curl https://evil.test; }; f && "unterminated`, + `f(){ curl https://evil.test; }; f && "unterminated`, + `function f { curl https://evil.test; }; f && "unterminated`, + `function f() { curl https://evil.test; }; f && "unterminated`, + `f() ( curl https://evil.test ); f && "unterminated`, + `f() { git push origin main; }; f && "unterminated`, + // CMD command groups follow condition tokens rather than beginning a + // segment, and may themselves contain nested groups. + `if 1==1 (curl https://evil.test) & rem '`, + `if 1==1 ((git push origin main)) & rem '`, + `for %i in (x) do (curl https://evil.test) & rem '`, + } { + t.Run(command, func(t *testing.T) { + risk := classifyCommand(command) + if !HasRiskCategory(risk, "unparseable_command") { + t.Errorf("Classify(%q) = categories %v; want unparseable_command", command, risk.Categories) + } + if risk.Level != RiskCritical || !HasRiskCategory(risk, "network") { + t.Errorf("Classify(%q) = level %s, categories %v; want critical network", command, risk.Level, risk.Categories) + } + }) + } +} + +// TestClassifyUnparseableCMDInvocationFormsFailClosed covers jatmn's #726 +// finding that the fallback used POSIX rules to find the executable position in +// what is often valid CMD source. Each command below runs a network program +// under cmd.exe and is rejected by the POSIX parser only because CMD's `rem` +// comment swallows the trailing apostrophe; under POSIX resolution each one +// stops at a token that is not the program ("@curl", "call", "not", "start"), +// which dropped the network category and with it the engine's network prompt. +func TestClassifyUnparseableCMDInvocationFormsFailClosed(t *testing.T) { + for _, command := range []string{ + `@curl https://evil.test & rem '`, + `@@curl https://evil.test & rem '`, + `call curl https://evil.test & rem '`, + `call @curl https://evil.test & rem '`, + `cmd.exe /c call curl https://evil.test & rem '`, + `@cmd.exe /c curl https://evil.test & rem '`, + `start "" curl https://evil.test & rem '`, + `start "download window" curl https://evil.test & rem '`, + `start "" c^u^r^l https://evil.test & rem '`, + `start /b /wait curl https://evil.test & rem '`, + `start /d C:\tmp curl https://evil.test & rem '`, + `if not 1==2 curl https://evil.test & rem '`, + `if /i "%mode%" == "fetch" git push origin main & rem '`, + `if exist repo git push origin main & rem '`, + `if errorlevel 1 curl https://evil.test & rem '`, + `if defined PROXY curl https://evil.test & rem '`, + `if %retries% gtr 0 curl https://evil.test & rem '`, + `for %i in (x) do call curl https://evil.test & rem '`, + `for /f %i in ('curl https://evil.test') do echo %i & rem '`, + `for /f %i in ('cu^rl https://evil.test') do echo %i & rem '`, + "for /f \"usebackq\" %i in (`git ls-remote origin`) do echo %i & rem '", + `cu^rl https://evil.test & rem '`, + `cmd /c cu^rl https://evil.test & rem '`, + `cmd /c"curl https://evil.test" & rem '`, + `cmd /c /d curl https://evil.test & rem '`, + `%ComSpec% /c curl https://evil.test & rem '`, + `git pus^h origin main & rem '`, + `git archive --rem^ote=origin HEAD & rem '`, + `powershell /Command curl https://evil.test & rem '`, + `powershell -Command Invoke-WebRequest https://evil.test & rem '`, + `pwsh -co iwr https://evil.test & rem '`, + `powershell -co curl https://evil.test & rem '`, + `powershell -ep RemoteSigned curl https://evil.test & rem '`, + `pwsh -ep Bypass -Command curl https://evil.test & rem '`, + `start "x" curl https://evil.test & rem '`, + `cmd /c start "x" curl https://evil.test & rem '`, + `start /b /wait "x" curl https://evil.test & rem '`, + `if cmdextversion 1 curl https://evil.test & rem '`, + `if not cmdextversion 1 git push origin main & rem '`, + } { + t.Run(command, func(t *testing.T) { + if analysis := AnalyzeCommand(command); !analysis.TooComplex { + t.Fatalf("AnalyzeCommand(%q) parsed; this case must exercise the fallback", command) + } + risk := classifyCommand(command) + if !HasRiskCategory(risk, "unparseable_command") { + t.Errorf("Classify(%q) = categories %v; want unparseable_command", command, risk.Categories) + } + if risk.Level != RiskCritical || !HasRiskCategory(risk, "network") { + t.Errorf("Classify(%q) = level %s, categories %v; want critical network", command, risk.Level, risk.Categories) + } + }) + } +} + +// TestGitGlobalOptionsResolveIdenticallyOnBothPaths pins the whole result table +// of git's global-option scan, on the AST path and the fallback path at once. +// Terminal globals print from the local installation and exit, so the words +// after them are output text rather than a subcommand: `git -h push` renders +// git-push's usage without contacting a remote. Both paths read the same +// parser, so a future option cannot be taught to one and not the other. +func TestGitGlobalOptionsResolveIdenticallyOnBothPaths(t *testing.T) { + for _, testCase := range []struct { + args string + network bool + }{ + {"push origin main", true}, + {"-C repo push origin main", true}, + {"--git-dir /repo/.git fetch origin", true}, + {"--no-pager push origin main", true}, + {"PUSH origin main", true}, + {"--Git-Dir repo PUSH origin main", true}, + {"archive --mtime 2024-01-01 --remote origin HEAD", true}, + {"archive --format --remote=origin HEAD", true}, + {"archive --mtime --remote=origin HEAD", true}, + {"archive HEAD --remote", false}, + {"archive HEAD --remote=origin", true}, + {"archive -o -- --remote=origin HEAD", true}, + {"archive -o --remote HEAD", false}, + {"archive HEAD -- --remote=origin", false}, + {"--help push", false}, + {"-h push", false}, + {"--version push", false}, + {"-v push", false}, + {"--html-path push", false}, + {"--man-path push", false}, + {"--info-path push", false}, + {"-C repo --help push", false}, + {"-C repo -h push", false}, + {"--list-cmds=main push", false}, + // `--exec-path[=]`: bare, it prints the local exec path and exits, so + // it neither takes /tmp as a value nor reaches push. With an inline value + // it is an ordinary nonterminal global. + {"--exec-path", false}, + {"--exec-path /tmp push", false}, + {"--exec-path=/tmp push", true}, + {"-C repo --exec-path push", false}, + {"-C repo --list-cmds=main push", false}, + {"-C repo --version push", false}, + {"status", false}, + {"", false}, + } { + t.Run(testCase.args, func(t *testing.T) { + parseable := "git " + testCase.args + if analysis := AnalyzeCommand(parseable); analysis.TooComplex { + t.Fatalf("AnalyzeCommand(%q) reported TooComplex; this case must exercise the AST path", parseable) + } + if got := HasRiskCategory(classifyCommand(parseable), "network"); got != testCase.network { + t.Errorf("AST Classify(%q) network = %v, want %v", parseable, got, testCase.network) + } + + // The same words through the fallback, made unparseable by a CMD + // comment the POSIX parser cannot close. + unparseable := parseable + " & rem '" + if analysis := AnalyzeCommand(unparseable); !analysis.TooComplex { + t.Fatalf("AnalyzeCommand(%q) parsed; this case must exercise the fallback", unparseable) + } + if got := HasRiskCategory(classifyCommand(unparseable), "network"); got != testCase.network { + t.Errorf("fallback Classify(%q) network = %v, want %v", unparseable, got, testCase.network) + } + }) + } +} + +func TestClassifyUnparseableLocalPowerShellCommandStaysNonNetwork(t *testing.T) { + command := `powershell -Command Get-ChildItem -File . & rem '` + risk := classifyCommand(command) + if HasRiskCategory(risk, "network") { + t.Fatalf("Classify(%q) = categories %v; want no network category", command, risk.Categories) + } +} + +func TestClassifyUnparseableCommandBearingWrapperValuesFailClosed(t *testing.T) { + t.Setenv("ZERO_TEST_ENV_COMMAND", "curl") + for _, command := range []string{ + `env -S 'curl https://evil.test' && "unterminated`, + `env --split-string 'git push origin main' && "unterminated`, + `env -S 'curl\_https://evil.test' && "unterminated`, + `env -S '${ZERO_TEST_ENV_COMMAND} https://evil.test' && "unterminated`, + `env -iS 'curl https://evil.test' && "unterminated`, + `env -S '-S "curl https://evil.test"' && "unterminated`, + `env -S 'env -S "curl https://evil.test"' && "unterminated`, + `env -S '--argv0 harmless curl https://evil.test' && "unterminated`, + `busybox sh -c 'curl https://evil.test' && "unterminated`, + `strace -P /tmp sh -c 'git push origin main' && "unterminated`, + `strace -p 123 curl https://evil.test && "unterminated`, + `strace --trace network curl https://evil.test && "unterminated`, + `strace --tips curl https://evil.test && "unterminated`, + `strace -fqo trace.log curl https://evil.test && "unterminated`, + `pwsh -cwa Invoke-WebRequest https://evil.test & rem '`, + `exec -a harmless curl https://evil.test && "unterminated`, + `exec --argv0 harmless git push origin main && "unterminated`, + // Source that exists but cannot be read statically: an encoded PowerShell + // payload that does not decode, and an env split string the shell expands. + `powershell -EncodedCommand curl & rem '`, + `env -S "$PAYLOAD" && "unterminated`, + `env --split-string="$PAYLOAD" && "unterminated`, + // GNU env accepts an unambiguous abbreviation of --split-string; the + // fallback tokenizer must recognize it exactly like the full spelling, + // not fall through to ordinary wrapper handling that reports no executable. + `env --split 'curl https://evil.test' && "unterminated`, + `env --split='curl https://evil.test' && "unterminated`, + // The delegated child program is a shell expansion this fallback tokenizer + // preserves verbatim (it never expands anything); matching the literal + // "$APPLET" token against known program names must not read as clean. + `APPLET=curl; busybox "$APPLET" https://evil.test && "unterminated`, + `APPLET=curl; strace "$APPLET" https://evil.test && "unterminated`, + // START keeps taking switches after its optional window title. + `start "" /b curl https://evil.test & rem '`, + `start "" /d C:\ curl https://evil.test & rem '`, + `start "" /wait git push origin main & rem '`, + } { + t.Run(command, func(t *testing.T) { + if analysis := AnalyzeCommand(command); !analysis.TooComplex { + t.Fatalf("AnalyzeCommand(%q) parsed; this case must exercise the fallback", command) + } + risk := classifyCommand(command) + if risk.Level != RiskCritical || !HasRiskCategory(risk, "network") { + t.Fatalf("Classify(%q) = level %s, categories %v; want critical network", command, risk.Level, risk.Categories) + } + }) } - if risk.Level != RiskCritical || !HasRiskCategory(risk, "network") { - t.Fatalf("Classify of unparseable network command = level %s, categories %v; want critical network", risk.Level, risk.Categories) +} + +// These malformed forms contain network-looking text in non-executing variable, +// arithmetic, array, escaped-backtick, or ordinary argument contexts. They must +// stay non-network so fallback tokenization does not over-flag inert text. +func TestClassifyUnparseableShellSyntaxTextStaysNonNetwork(t *testing.T) { + for _, command := range []string{ + `echo ${curl} && "unterminated`, + `echo $((curl)) && "unterminated`, + `arr=(curl) && "unterminated`, + "echo \\`curl\\` && \"unterminated", + `command if curl https://evil.test && "unterminated`, + `env then git push && "unterminated`, + `for %i in (curl) do echo %i & rem '`, + `env -S 'printf curl; git push' && "unterminated`, + `env -S 'printf ok' curl https://evil.test && "unterminated`, + // A decodable encoded payload is read rather than guessed at: these decode + // to the UTF-16LE source `evil`, a local program name. + `powershell -e ZQB2AGkAbAA= & rem '`, + `pwsh -ec ZQB2AGkAbAA= & rem '`, + `pwsh -File curl & rem '`, + `powershell -DefinitelyInvalid Invoke-WebRequest https://evil.test & rem '`, + // CMD's START runs nothing when its only quoted argument is the window + // title, and neither ECHO nor REM executes the text that follows it. + `start "curl https://evil.test" & rem '`, + // An unquoted first START operand is the executable, not a window title; + // the later curl token is only an argument to MyTitle. + `start MyTitle curl https://evil.test & rem '`, + `call start MyTitle curl https://evil.test & rem '`, + `echo for /f %i in ('curl https://evil.test') do echo %i & rem '`, + `rem for /f %i in ('curl https://evil.test') do echo %i & rem '`, + `echo "x & for /f %i in ('curl https://evil.test') do echo %i" & rem '`, + `echo x ^& for /f %i in ('curl https://evil.test') do echo %i & rem '`, + "for /f \"delims=usebackq\" %i in (`curl https://evil.test`) do echo %i & rem '", + `busybox -- curl https://evil.test && "unterminated`, + `busybox -x curl https://evil.test && "unterminated`, + `strace --definitely-invalid curl https://evil.test && "unterminated`, + `env -S '--argv0 curl printf ok' && "unterminated`, + // An abbreviated long option that isn't --split-string must not be + // misread as one; env's own resolution of these flags is unaffected. + `env --unset curl printf ok && "unterminated`, + `env --chdir=/tmp printf ok && "unterminated`, + // The delegated child program resolves to ordinary literal text, not an + // unread expansion; it must still be classified on its own content. + `busybox echo curl https://evil.test && "unterminated`, + `strace true curl https://evil.test && "unterminated`, + `bash /dev/null -c 'curl https://evil.test' && "unterminated`, + `bash -- -c 'curl https://evil.test' && "unterminated`, + `bash -Zc 'curl https://evil.test' && "unterminated`, + `bash -nc 'curl https://evil.test' && "unterminated`, + `cmd /c start & rem '`, + `echo call curl https://evil.test & rem '`, + `rem start curl https://evil.test & rem '`, + } { + t.Run(command, func(t *testing.T) { + risk := classifyCommand(command) + if !HasRiskCategory(risk, "unparseable_command") { + t.Errorf("Classify(%q) = categories %v; want unparseable_command", command, risk.Categories) + } + if HasRiskCategory(risk, "network") { + t.Errorf("Classify(%q) = level %s, categories %v; want no network category", command, risk.Level, risk.Categories) + } + }) + } +} + +func TestClassifyUnparseableMismatchedDelimitersDoesNotPanic(t *testing.T) { + for _, command := range []string{ + "echo `curl)` && \"unterminated", + "echo `curl(` && \"unterminated", + "echo )`curl` && \"unterminated", + } { + risk := classifyCommand(command) + if !HasRiskCategory(risk, "unparseable_command") { + t.Errorf("Classify(%q) = categories %v; want unparseable_command", command, risk.Categories) + } + } +} + +func FuzzFallbackCommandTokensDoesNotPanic(f *testing.F) { + for _, command := range []string{"", "`", ")", "(`)", "echo `curl)`"} { + f.Add(command) + } + f.Fuzz(func(t *testing.T, command string) { + fallbackCommandTokens(command) + }) +} + +// FuzzFallbackNetworkResolutionDoesNotPanic covers the resolvers layered on top +// of the tokenizer. They index into token slices around keywords, conditions and +// option values, and this path exists to be total over input the shell parser +// already rejected: a panic here is a request-triggerable crash. +func FuzzFallbackNetworkResolutionDoesNotPanic(f *testing.F) { + for _, command := range []string{ + "", "if", "if not", "start", "start /d", "start @", "start @@", "call", "@", "@curl", + "for %i in (x) do", "if errorlevel", "if %x% equ", "git -C", + "echo `curl)` && \"unterminated", `cmd /c`, `cmd /c start & rem '`, `if 1==1 (curl https://evil.test`, + } { + f.Add(command) + } + f.Fuzz(func(t *testing.T, command string) { + matchesUnparseableNetwork(command) + }) +} + +func TestClassifyDeepCMDLaunchersDoesNotRecurseUnboundedly(t *testing.T) { + command := strings.Repeat("cmd /c ", 512) + `curl https://evil.test & rem '` + risk := classifyCommand(command) + if !HasRiskCategory(risk, "network") { + t.Fatalf("Classify(deep cmd chain) = categories %v; want network", risk.Categories) + } +} + +func TestFallbackBodyDeepExecChainDoesNotRecurseUnboundedly(t *testing.T) { + body := append(make([]string, 512), "curl", "https://evil.test") + for index := 0; index < 512; index++ { + body[index] = "exec" + } + if !fallbackBodyUsesNetwork(body, 0) { + t.Fatal("fallbackBodyUsesNetwork(deep exec chain) = false; want network") + } +} + +// TestUnparseableShellDepthMatchesAnalyzerDepth pins the two launcher-recursion +// caps together, which is the property jatmn's #703 review asked for: a fallback +// that gave up a level earlier than the AST path would drop the network category +// on exactly the deeply-nested chains it exists to fail closed on. +// +// Asserted as constants rather than by driving a four-deep command: the fallback +// tokenizer is deliberately small and does not model nested escaped quotes, so +// a literal four-layer `sh -c` string would be testing the tokenizer's escaping +// rather than the depth limit. The behavior that recursion happens at all, and +// through more than one layer, is covered above. +func TestUnparseableShellDepthMatchesAnalyzerDepth(t *testing.T) { + if maxUnparseableShellDepth != maxAnalyzerDepth { + t.Fatalf("maxUnparseableShellDepth = %d, maxAnalyzerDepth = %d; the fallback must not give up before the parseable path", + maxUnparseableShellDepth, maxAnalyzerDepth) + } +} + +// TestClassifyUnparseableLocalGitArchiveStaysNonNetwork pins the other half of +// the archive gate: the fallback must agree with the AST path that only a +// --remote archive talks to another host. +func TestClassifyUnparseableLocalGitArchiveStaysNonNetwork(t *testing.T) { + for _, command := range []string{ + `git archive HEAD & rem '`, + `git archive -o out.tar HEAD & rem '`, + `git -C repo archive HEAD & rem '`, + `git.exe archive HEAD & rem '`, + // A pathspec named --remote after the end-of-options separator is a + // local tree entry, not a remote (issue #703 review). + `git archive HEAD -- --remote & rem '`, + `git archive HEAD -- --remote=origin & rem '`, + } { + t.Run(command, func(t *testing.T) { + risk := classifyCommand(command) + if !HasRiskCategory(risk, "unparseable_command") { + t.Errorf("Classify(%q) = categories %v; want unparseable_command", command, risk.Categories) + } + if HasRiskCategory(risk, "network") { + t.Errorf("Classify(%q) = categories %v; want no network category for a local archive", command, risk.Categories) + } + }) + } +} + +// TestClassifyUnparseableNonGitOptionTokenStaysNonNetwork guards against the +// fallback regex treating an arbitrary bare token before a network verb as if +// it were a git global option. `status` in `git status push` is a pathspec +// argument to `git status`, not a value-taking global option, so `push` here +// is not the git subcommand and must not be classified as network — even +// though the trailing unmatched quote (a cmd.exe REM comment, invalid under +// the POSIX parser AnalyzeCommand uses) still forces the unparseable-command +// fallback path. +func TestClassifyUnparseableNonGitOptionTokenStaysNonNetwork(t *testing.T) { + for _, command := range []string{ + `git status push & rem '`, + `git "status" push & rem '`, + `git 'status' push & rem "`, + `git.exe -C "C:\Program Files\push" status & rem '`, + `git.exe --git-dir="C:\Program Files\push\.git" status & rem '`, + `git.exe "--git-dir=C:\Program Files\push\.git" status & rem '`, + `git -C push status & rem '`, + `git -c push status & rem '`, + `git -C "push" status & rem '`, + `git -c "push" status & rem '`, + `git --help push & rem '`, + `git --version push & rem '`, + `echo https://example.com/repo.git push & rem '`, + `echo ssh://git@example.com/repo.git push & rem '`, + `echo C:\repos\repo.git push & rem '`, + `echo git.example.com push & rem '`, + } { + t.Run(command, func(t *testing.T) { + risk := classifyCommand(command) + if !HasRiskCategory(risk, "unparseable_command") { + t.Errorf("Classify(%q) = categories %v; want unparseable_command", command, risk.Categories) + } + if HasRiskCategory(risk, "network") { + t.Errorf("Classify(%q) = level %s, categories %v; want no network category", command, risk.Level, risk.Categories) + } + if risk.Level != RiskHigh { + t.Errorf("Classify(%q) = level %s; want high (unparseable_command only)", command, risk.Level) + } + }) } } diff --git a/internal/sandbox/safe_command.go b/internal/sandbox/safe_command.go index 807004710..73e60aada 100644 --- a/internal/sandbox/safe_command.go +++ b/internal/sandbox/safe_command.go @@ -1,6 +1,7 @@ package sandbox import ( + "os" "runtime" "strings" ) @@ -147,6 +148,13 @@ var interactiveSegments = []struct { // that would block a non-interactive agent. goos selects platform-specific // rules (pass "" to use the host runtime.GOOS). func DetectInteractiveCommand(command string, goos string) InteractiveCommandResult { + return detectInteractiveCommandAt(command, goos, 0) +} + +func detectInteractiveCommandAt(command string, goos string, depth int) InteractiveCommandResult { + if depth > maxAnalyzerDepth { + return InteractiveCommandResult{} + } command = strings.TrimSpace(command) if command == "" { return InteractiveCommandResult{} @@ -192,7 +200,7 @@ func DetectInteractiveCommand(command string, goos string) InteractiveCommandRes // command; recurse into it so an interactive program inside the payload // is detected (e.g. `sh -c 'vim x'`). if payload := shellDashCPayload(first, fields); payload != "" { - if inner := DetectInteractiveCommand(payload, goos); inner.Interactive { + if inner := detectInteractiveCommandAt(payload, goos, depth+1); inner.Interactive { return inner } continue @@ -229,7 +237,7 @@ func DetectInteractiveCommand(command string, goos string) InteractiveCommandRes // the parser cannot handle (Windows cmd.exe, obfuscation) yields no commands // and falls through unchanged — the guard never hard-blocks on a parse error. for _, fields := range astCommandFields(command) { - if result, ok := inspectCommandFields(fields, goos); ok { + if result, ok := inspectCommandFields(fields, goos, depth); ok { return result } } @@ -243,7 +251,7 @@ func DetectInteractiveCommand(command string, goos string) InteractiveCommandRes // non-interactive suppressions (hasNonInteractiveFlag covers REPL flags and // trailing-command clients like ssh). It mirrors the hand-written passes above // so an AST-extracted command is classified identically to a plainly-split one. -func inspectCommandFields(fields []string, goos string) (InteractiveCommandResult, bool) { +func inspectCommandFields(fields []string, goos string, depth int) (InteractiveCommandResult, bool) { body := strings.ToLower(commandBody(fields)) for _, seg := range interactiveSegments { if body == seg.match || strings.HasPrefix(body, seg.match+" ") { @@ -259,7 +267,7 @@ func inspectCommandFields(fields []string, goos string) (InteractiveCommandResul return InteractiveCommandResult{}, false } if payload := shellDashCPayload(first, fields); payload != "" { - if inner := DetectInteractiveCommand(payload, goos); inner.Interactive { + if inner := detectInteractiveCommandAt(payload, goos, depth+1); inner.Interactive { return inner, true } return InteractiveCommandResult{}, false @@ -297,7 +305,7 @@ var wrapperPrograms = map[string]bool{ var wrapperValueOptionsByProg = map[string]map[string]bool{ "sudo": {"-u": true, "--user": true, "-g": true, "--group": true, "-p": true, "--prompt": true, "-C": true, "--close-from": true, "-r": true, "--role": true, "-T": true, "--command-timeout": true, "-U": true, "--other-user": true, "-h": true, "--host": true, "-D": true, "--chdir": true, "-R": true, "--chroot": true}, "doas": {"-u": true, "-C": true}, - "env": {"-u": true, "--unset": true, "-S": true, "--split-string": true, "-C": true, "--chdir": true}, + "env": {"-u": true, "--unset": true, "-S": true, "--split-string": true, "-C": true, "--chdir": true, "-a": true, "--argv0": true}, "timeout": {"-s": true, "--signal": true, "-k": true, "--kill-after": true}, "nice": {"-n": true, "--adjustment": true}, "ionice": {"-c": true, "--class": true, "-n": true, "--classdata": true, "-p": true, "--pid": true}, @@ -361,10 +369,20 @@ func firstProgram(fields []string) string { // command boundary (e.g. `sudo git rebase -i` -> "git rebase -i") instead of // matching the segment text anywhere as a raw substring. func commandBody(fields []string) string { + return strings.Join(commandBodyFields(fields), " ") +} + +// commandBodyFields is commandBody's scan, returning the fields from the first +// real command token onward (nil when the segment is nothing but assignments and +// wrappers). Callers that need to reason about the program and its arguments +// separately — the unparseable-command fallback in risk.go — use this rather +// than re-splitting commandBody's joined string, which would lose the token +// boundaries the fallback tokenizer worked to preserve. +func commandBodyFields(fields []string) []string { wrapper := "" for index := 0; index < len(fields); index++ { field := fields[index] - if strings.Contains(field, "=") && !strings.HasPrefix(field, "=") { + if strings.Contains(field, "=") && !strings.HasPrefix(field, "=") && !strings.HasPrefix(field, "-") { continue } if strings.HasPrefix(field, "-") { @@ -381,9 +399,437 @@ func commandBody(fields []string) string { continue } // First real command token: the body starts here. - return strings.Join(fields[index:], " ") + return fields[index:] } - return "" + return nil +} + +// envSplitCommandFields resolves GNU env's -S/--split-string launcher when env +// appears at an actual command position (possibly behind another wrapper). The +// split string is argv syntax, not shell source, so metacharacters remain inert +// unless the resulting executable is itself a shell with -c. +type envSplitCommandResult struct { + command []string + recognized bool + executableEnvironmentDependent bool +} + +func envSplitCommandFields(fields []string) envSplitCommandResult { + wrapper := "" + for index := 0; index < len(fields); index++ { + field := fields[index] + if strings.Contains(field, "=") && !strings.HasPrefix(field, "=") && !strings.HasPrefix(field, "-") { + continue + } + if strings.HasPrefix(field, "-") { + if wrapperConsumesValue(wrapper, field) && index+1 < len(fields) { + index++ + } + continue + } + if isNumericToken(field) { + continue + } + token := normalizeProgramToken(field) + if token == "env" { + return envSplitCommand(fields[index+1:]) + } + if wrapperPrograms[token] { + wrapper = token + continue + } + return envSplitCommandResult{} + } + return envSplitCommandResult{} +} + +func envSplitCommand(args []string) envSplitCommandResult { + current := append([]string(nil), args...) + dependent := make([]bool, len(current)) + recognized := false + for rewrite := 0; rewrite < 16; rewrite++ { + commandIndex := len(current) + rewritten := false + for index := 0; index < len(current); index++ { + arg := current[index] + if arg == "--" { + commandIndex = index + 1 + break + } + if strings.Contains(arg, "=") && !strings.HasPrefix(arg, "=") && !strings.HasPrefix(arg, "-") { + continue + } + value, consumed, retainedOption, split := envSplitOption(current, index) + if split { + recognized = true + if consumed == 0 { + return envSplitCommandResult{recognized: true} + } + fields, fieldDependencies, ok := splitEnvString(value) + if !ok { + // The split string is executable argv this scan cannot read. + // "Cannot inspect" is not "does not use the network": env still + // runs whatever the shell expanded into it, so keep the gate. + return envSplitCommandResult{recognized: true, executableEnvironmentDependent: true} + } + replacement := fields + replacementDependencies := fieldDependencies + if retainedOption != "" { + replacement = append([]string{retainedOption}, replacement...) + replacementDependencies = append([]bool{false}, replacementDependencies...) + } + current = append(append(append([]string(nil), current[:index]...), replacement...), current[index+consumed:]...) + dependent = append(append(append([]bool(nil), dependent[:index]...), replacementDependencies...), dependent[index+consumed:]...) + rewritten = true + break + } + if strings.HasPrefix(arg, "-") { + if wrapperConsumesValue("env", arg) { + index++ + } + continue + } + commandIndex = index + break + } + if rewritten { + continue + } + if !recognized { + return envSplitCommandResult{} + } + if commandIndex >= len(current) { + return envSplitCommandResult{recognized: true} + } + command := current[commandIndex:] + // Preserve a nested env invocation so fallbackBodyUsesNetwork can process + // its own -S grammar. Other ordinary wrappers can be resolved directly. + if normalizeProgramToken(command[0]) != "env" { + command = commandBodyFields(command) + commandIndex = len(current) - len(command) + } + return envSplitCommandResult{ + command: command, + recognized: true, + executableEnvironmentDependent: len(command) > 0 && dependent[commandIndex], + } + } + // Excessive rewrites are attacker-controlled ambiguity. Keep the network + // gate rather than recursing without a bound. + return envSplitCommandResult{recognized: true, executableEnvironmentDependent: true} +} + +// envSplitLongOptionName is GNU env's only long option beginning with "s", so +// any unambiguous prefix of it ("--s", "--split", "--split-strin", ...) names +// the same option under GNU getopt_long's abbreviation rule. +const envSplitLongOptionName = "split-string" + +// matchesEnvSplitLongOption reports whether name (the text of a "--name" or +// "--name=value" option, with "--" and any "=value" suffix already stripped) +// is an unambiguous abbreviation of --split-string. Modelling the runtime +// option grammar here, rather than the single exact spelling, matters: `env +// --split 'curl https://…'` is accepted by GNU env exactly like +// `--split-string`, and treating only the full spelling as recognized let a +// shortened form fall through to ordinary wrapper handling with no network +// classification at all. +func matchesEnvSplitLongOption(name string) bool { + if name == "" { + return false + } + return strings.HasPrefix(envSplitLongOptionName, name) +} + +func envSplitOption(args []string, index int) (value string, consumed int, retainedOption string, ok bool) { + arg := args[index] + switch { + case arg == "-S": + if index+1 >= len(args) { + return "", 0, "", true + } + return args[index+1], 2, "", true + case strings.HasPrefix(arg, "--"): + name := arg[2:] + hasValue := false + optionValue := "" + if equals := strings.IndexByte(name, '='); equals >= 0 { + hasValue = true + optionValue = name[equals+1:] + name = name[:equals] + } + if !matchesEnvSplitLongOption(name) { + return "", 0, "", false + } + if hasValue { + return optionValue, 1, "", true + } + if index+1 >= len(args) { + return "", 0, "", true + } + return args[index+1], 2, "", true + case strings.HasPrefix(arg, "-") && !strings.HasPrefix(arg, "--"): + position := strings.IndexByte(arg[1:], 'S') + if position < 0 { + return "", 0, "", false + } + position++ + for _, option := range arg[1:position] { + if option != 'i' && option != '0' && option != 'v' { + return "", 0, "", false + } + } + retained := "" + if position > 1 { + retained = "-" + arg[1:position] + } + if position+1 < len(arg) { + return arg[position+1:], 1, retained, true + } + if index+1 >= len(args) { + return "", 0, "", true + } + return args[index+1], 2, retained, true + default: + return "", 0, "", false + } +} + +func splitEnvString(value string) ([]string, []bool, bool) { + var fields []string + var dependent []bool + var word strings.Builder + var quote rune + started := false + wordDependent := false + flush := func() { + if started { + fields = append(fields, word.String()) + dependent = append(dependent, wordDependent) + word.Reset() + started = false + wordDependent = false + } + } + runes := []rune(value) + for index := 0; index < len(runes); index++ { + r := runes[index] + if r == '\\' { + if index+1 >= len(runes) { + return nil, nil, false + } + index++ + escaped := runes[index] + if quote == '\'' && escaped != '\\' && escaped != '\'' { + word.WriteRune('\\') + word.WriteRune(escaped) + started = true + continue + } + switch escaped { + case 'c': + if quote != 0 { + return nil, nil, false + } + flush() + return fields, dependent, true + case '_': + if quote == '"' { + word.WriteRune(' ') + started = true + } else { + flush() + } + case 'f': + word.WriteRune('\f') + started = true + case 'n': + word.WriteRune('\n') + started = true + case 'r': + word.WriteRune('\r') + started = true + case 't': + word.WriteRune('\t') + started = true + case 'v': + word.WriteRune('\v') + started = true + case '#', '$', '"', '\'', '\\': + word.WriteRune(escaped) + started = true + default: + return nil, nil, false + } + continue + } + if r == '\'' || r == '"' { + if quote == 0 { + quote = r + started = true + continue + } + if quote == r { + quote = 0 + continue + } + } + if r == '$' && quote != '\'' { + if index+2 >= len(runes) || runes[index+1] != '{' { + return nil, nil, false + } + end := index + 2 + for end < len(runes) && runes[end] != '}' { + end++ + } + if end >= len(runes) || !validEnvSplitVariableName(string(runes[index+2:end])) { + return nil, nil, false + } + word.WriteString(os.Getenv(string(runes[index+2 : end]))) + started = true + wordDependent = true + index = end + continue + } + if r == '#' && quote == 0 && !started { + break + } + if quote == 0 && (r == ' ' || r == '\t' || r == '\n' || r == '\r' || r == '\v' || r == '\f') { + flush() + continue + } + word.WriteRune(r) + started = true + } + if quote != 0 { + return nil, nil, false + } + flush() + return fields, dependent, true +} + +func validEnvSplitVariableName(name string) bool { + if name == "" { + return false + } + for index, r := range name { + if (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || r == '_' || + (index > 0 && r >= '0' && r <= '9') { + continue + } + return false + } + return true +} + +func busyboxCommandArgs(args []string) []string { + if len(args) == 0 { + return nil + } + switch args[0] { + case "--help", "--list", "--list-full", "--install", "--show": + return nil + } + if strings.HasPrefix(args[0], "-") { + return nil + } + return args +} + +var ( + straceRequiredLongOptions = map[string]bool{ + "--columns": true, "--detach-on": true, "--env": true, "--interruptible": true, + "--stack-trace-frame-limit": true, "--syscall-limit": true, "--output": true, + "--summary-syscall-overhead": true, "--attach": true, "--trace-path": true, + "--string-limit": true, "--summary-sort-by": true, "--user": true, + "--summary-columns": true, "--const-print-style": true, "--argv0": true, + "--color": true, "--trace": true, "--trace-fds": true, "--abbrev": true, + "--verbose": true, "--raw": true, "--signals": true, "--status": true, + "--read": true, "--write": true, "--fault": true, "--inject": true, + "--kvm": true, "--decode-pids": true, + } + straceOptionalLongOptions = map[string]bool{ + "--daemonize": true, "--daemonised": true, "--daemonized": true, + "--stack-trace": true, "--stack-traces": true, "--relative-timestamps": true, + "--absolute-timestamps": true, "--timestamps": true, "--syscall-times": true, + "--strings-in-hex": true, "--tips": true, "--namespace": true, + "--quiet": true, "--silent": true, "--silence": true, "--decode-fds": true, + "--secontext": true, + } + straceValuelessLongOptions = map[string]bool{ + "--output-append-mode": true, "--summary-only": true, "--summary": true, + "--debug": true, "--follow-forks": true, "--output-separately": true, + "--instruction-pointer": true, "--kill-on-exit": true, "--syscall-number": true, + "--arg-names": true, "--no-abbrev": true, "--summary-wall-clock": true, + "--pidns-translation": true, "--successful-only": true, "--failed-only": true, + "--failing-only": true, "--seccomp-bpf": true, "--always-show-pid": true, + } +) + +func straceCommandArgs(args []string) []string { + index, ok := straceChildIndex(args) + if !ok { + return nil + } + return args[index:] +} + +// straceChildIndex walks strace's own argv exactly as straceCommandArgs does, +// returning the index of the traced child command instead of the resolved +// slice. straceSourceDynamic shares this walk so its literalness check stays +// aligned with straceCommandArgs's option grammar by construction, instead of +// via a second hand-maintained copy that could silently drift from it. +func straceChildIndex(args []string) (int, bool) { + for index := 0; index < len(args); index++ { + arg := args[index] + switch arg { + case "--help", "--version": + return 0, false + case "--": + return index + 1, true + case "-": + return index, true + } + if strings.HasPrefix(arg, "--") { + name, hasValue := arg, false + if equals := strings.IndexByte(arg, '='); equals >= 0 { + name, hasValue = arg[:equals], true + } + switch { + case straceRequiredLongOptions[name]: + if !hasValue { + index++ + } + case straceOptionalLongOptions[name]: + // getopt_long accepts optional values only in --option=value form. + case straceValuelessLongOptions[name] && !hasValue: + default: + return 0, false + } + continue + } + if strings.HasPrefix(arg, "-") { + cluster := []rune(arg[1:]) + for clusterIndex, option := range cluster { + switch { + case option == 'h' || option == 'V': + return 0, false + case strings.ContainsRune("abeEIoOpPsSuUX", option): + if clusterIndex+1 == len(cluster) { + index++ + } + clusterIndex = len(cluster) + case strings.ContainsRune("AcCdDfFiknNqrtTvwxyYzZ", option): + default: + return 0, false + } + if clusterIndex == len(cluster) { + break + } + } + continue + } + return index, true + } + return 0, false } // isNumericToken reports whether a token is purely digits (e.g. the duration @@ -415,13 +861,8 @@ func shellDashCPayload(program string, fields []string) string { return "" } args := fields[start+1:] - for i, arg := range args { - if arg == "-c" || arg == "--command" { - if i+1 < len(args) { - return strings.Join(args[i+1:], " ") - } - return "" - } + if payloadIndex, found := shellCommandPayloadIndex(program, args); found && payloadIndex < len(args) { + return strings.Trim(args[payloadIndex], `"'`) } return "" } @@ -454,8 +895,20 @@ func normalizeProgramToken(field string) string { token = token[i+1:] } } - token = strings.ToLower(token) - for _, suffix := range []string{".exe", ".cmd", ".bat", ".com"} { + return trimExecutableSuffix(strings.ToLower(token)) +} + +// executableSuffixes are the Windows executable extensions stripped from a +// program token so curl.exe, git.cmd, and curl all normalize to one name. Both +// the parseable path (normalizeProgramToken) and the unparseable fallback +// (executableTokenBase) strip the same set; a token that normalizes differently +// on the two paths is exactly how a command slips past the fallback. +var executableSuffixes = []string{".exe", ".cmd", ".bat", ".com"} + +// trimExecutableSuffix removes one trailing Windows executable extension from an +// already-lowercased token. +func trimExecutableSuffix(token string) string { + for _, suffix := range executableSuffixes { if strings.HasSuffix(token, suffix) { return strings.TrimSuffix(token, suffix) } @@ -487,9 +940,9 @@ func windowsExecutablePathBasename(token string) (string, bool) { } func hasWindowsExecutableSuffix(token string) bool { - token = strings.ToLower(token) - for _, suffix := range []string{".exe", ".cmd", ".bat", ".com"} { - if strings.HasSuffix(token, suffix) { + lowered := strings.ToLower(token) + for _, suffix := range executableSuffixes { + if strings.HasSuffix(lowered, suffix) { return true } } diff --git a/internal/sandbox/safe_command_test.go b/internal/sandbox/safe_command_test.go index 15a2c5d6f..ec2ad7bca 100644 --- a/internal/sandbox/safe_command_test.go +++ b/internal/sandbox/safe_command_test.go @@ -1,6 +1,7 @@ package sandbox import ( + "strconv" "strings" "testing" ) @@ -407,6 +408,16 @@ func TestDetectInteractiveMongoEvalAndFullPaths(t *testing.T) { } } +func TestDetectInteractiveCommandBoundsNestedShellLaunchers(t *testing.T) { + command := "vim file" + for range maxAnalyzerDepth + 2 { + command = "bash -c " + strconv.Quote(command) + } + if got := DetectInteractiveCommand(command, "linux"); got.Interactive { + t.Fatalf("DetectInteractiveCommand(deep shell chain) = %+v; want bounded inspection", got) + } +} + // The hand-written segment splitter misses interactive programs hidden by // constructs only a real shell parser resolves (a newline separator collapsed // to a space; a brace group that shifts the real command position). The AST From 74003e35b6c134d51edc119be0cd5dbc769bf4c7 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Thu, 13 Aug 2026 16:11:43 +0200 Subject: [PATCH 02/12] fix(sandbox): close dynamic launcher gaps --- internal/sandbox/analyzer.go | 43 +++++++++++++++- internal/sandbox/engine_test.go | 38 ++++++++++++++ internal/sandbox/risk.go | 68 ++++++++++++++++++++----- internal/sandbox/risk_hardening_test.go | 66 ++++++++++++++++++++++++ internal/sandbox/safe_command.go | 44 +++++++++++++--- 5 files changed, 236 insertions(+), 23 deletions(-) diff --git a/internal/sandbox/analyzer.go b/internal/sandbox/analyzer.go index a31199181..8966abb98 100644 --- a/internal/sandbox/analyzer.go +++ b/internal/sandbox/analyzer.go @@ -167,7 +167,8 @@ func analyzeInto(script string, result *AnalysisResult, seen map[string]bool, de // ordinary effective program, so resolve its argv before that scan. if fields, ok := literalCallFields(call.Args); ok { if split := envSplitCommandFields(fields); split.recognized { - if split.executableEnvironmentDependent || fallbackBodyUsesNetwork(split.command, depth+1) { + if split.executableEnvironmentDependent || split.commandSourceEnvironmentDependent || + fallbackBodyUsesNetwork(split.command, depth+1) { result.Network = true } return true @@ -218,6 +219,9 @@ func analyzeInto(script string, result *AnalysisResult, seen map[string]bool, de } } } + if cmdLauncherUsesNetwork(prog, rest, depth) { + result.Network = true + } if _, interactive := interactivePrograms[prog]; interactive && !replSuppressed(prog, rest) { result.Interactive = true } @@ -666,6 +670,43 @@ func literalCallFields(args []*syntax.Word) ([]string, bool) { return fields, true } +func cmdLauncherUsesNetwork(program string, args []*syntax.Word, depth int) bool { + switch program { + case "cmd", "call", "start", "%comspec%": + default: + return false + } + tokens := make([]fallbackCommandToken, 0, len(args)+1) + tokens = append(tokens, fallbackCommandToken{value: program}) + for _, arg := range args { + if !isLiteralWord(arg) { + return true + } + tokens = append(tokens, fallbackCommandToken{ + value: wordText(arg), + quoted: literalWordIsQuoted(arg), + }) + } + for _, body := range cmdCommandBodyTokenInfoCandidates(tokens) { + if fallbackBodyUsesNetwork(fallbackTokenValues(body), depth) { + return true + } + } + return false +} + +func literalWordIsQuoted(word *syntax.Word) bool { + if word == nil || len(word.Parts) == 0 { + return false + } + switch word.Parts[0].(type) { + case *syntax.SglQuoted, *syntax.DblQuoted: + return true + default: + return false + } +} + // textualPayloadUsesNetwork classifies source carried by another interpreter // without leaking the nested parser's TooComplex bit into the outer command. func textualPayloadUsesNetwork(payload string, depth int) bool { diff --git a/internal/sandbox/engine_test.go b/internal/sandbox/engine_test.go index 1cd8ee4b3..cc337ba36 100644 --- a/internal/sandbox/engine_test.go +++ b/internal/sandbox/engine_test.go @@ -178,6 +178,34 @@ func TestEngineClassifiesCMDInvocationFormsAsNetwork(t *testing.T) { } } +func TestEngineClassifiesParseableCMDLaunchersAsNetwork(t *testing.T) { + for _, command := range []string{ + `cmd /c git push origin main`, + `cmd /c curl https://evil.test`, + `call git push origin main`, + `start curl https://evil.test`, + `start "" /b curl https://evil.test`, + } { + t.Run(command, func(t *testing.T) { + if analysis := AnalyzeCommand(command); analysis.TooComplex { + t.Fatalf("AnalyzeCommand(%q) reported TooComplex; this case must exercise the AST path", command) + } + engine := NewEngine(EngineOptions{WorkspaceRoot: t.TempDir(), Policy: DefaultPolicy()}) + decision := engine.Evaluate(context.Background(), Request{ + ToolName: "bash", + SideEffect: SideEffectShell, + Permission: PermissionPrompt, + PermissionMode: PermissionModeAsk, + Args: map[string]any{"command": command}, + }) + if decision.Action != ActionPrompt || decision.Reason != ReasonNetworkBlocked || + !HasRiskCategory(decision.Risk, "network") { + t.Fatalf("Evaluate(%q) = %#v, want a network prompt", command, decision) + } + }) + } +} + func TestEnginePromptsForReviewedUnparseableNetworkForms(t *testing.T) { t.Setenv("ZERO_TEST_ENV_COMMAND", "curl") for _, command := range []string{ @@ -205,6 +233,16 @@ func TestEnginePromptsForReviewedUnparseableNetworkForms(t *testing.T) { `env -S 'env -S "curl https://evil.test"' && "unterminated`, `env -S '--argv0 harmless curl https://evil.test' && "unterminated`, `env --split-string 'git push origin main' && "unterminated`, + `set "N=curl" & call %N% https://evil.test & rem '`, + `set "N=curl" & call %N:x=y% https://evil.test & rem '`, + `set "N=curl" & cmd /c %N% https://evil.test & rem '`, + `set "N=curl" & start %N% https://evil.test & rem '`, + `set "N=git" & call !N! push origin main & rem '`, + `sh -c "$PAYLOAD" & rem '`, + `sh -c "${PAYLOAD}" & rem '`, + `CMD=curl env -S '${CMD} https://evil.test'`, + `PAYLOAD='curl https://evil.test' env -S 'sh -c "${PAYLOAD}"'`, + `PAYLOAD='curl https://evil.test' env -S 'sh -c "${PAYLOAD}"' & rem '`, `exec -a harmless curl https://evil.test && "unterminated`, `powershell -NoProfile curl https://evil.test`, `pwsh -cwa Invoke-WebRequest https://evil.test & rem '`, diff --git a/internal/sandbox/risk.go b/internal/sandbox/risk.go index b45b87b8e..2a8dfa846 100644 --- a/internal/sandbox/risk.go +++ b/internal/sandbox/risk.go @@ -109,7 +109,8 @@ func matchesUnparseableNetworkAt(command string, depth int) bool { tokens := fallbackTokenValues(tokenInfo) if depth < maxUnparseableShellDepth { if split := envSplitCommandFields(tokens); split.recognized { - if split.executableEnvironmentDependent || fallbackBodyUsesNetwork(split.command, depth+1) { + if split.executableEnvironmentDependent || split.commandSourceEnvironmentDependent || + fallbackBodyUsesNetwork(split.command, depth+1) { return true } continue @@ -162,6 +163,9 @@ func fallbackBodyUsesNetwork(body []string, depth int) bool { if program == "%comspec%" { program = "cmd" } + if fallbackTokenLooksDynamic(body[0]) { + return true + } if networkPrograms[program] || localServerPrograms[program] { return true } @@ -201,7 +205,7 @@ func fallbackBodyUsesNetwork(body []string, depth int) bool { } if program == "env" { if split := envSplitCommand(args); split.recognized { - return split.executableEnvironmentDependent || + return split.executableEnvironmentDependent || split.commandSourceEnvironmentDependent || (len(split.command) > 0 && (depth >= maxUnparseableShellDepth || fallbackBodyUsesNetwork(split.command, depth+1))) } } @@ -212,7 +216,9 @@ func fallbackBodyUsesNetwork(body []string, depth int) bool { if shellPrograms[program] { if payloadIndex, found := shellCommandPayloadIndex(program, args); found && payloadIndex < len(args) { if payload := args[payloadIndex]; payload != "" { - if depth >= maxUnparseableShellDepth || matchesUnparseableNetworkAt(payload, depth+1) { + if fallbackTokenLooksDynamic(payload) || + depth >= maxUnparseableShellDepth || + matchesUnparseableNetworkAt(payload, depth+1) { return true } } @@ -239,18 +245,52 @@ func fallbackBodyUsesNetwork(body []string, depth int) bool { return false } -// fallbackTokenLooksDynamic reports whether a token resolved as a wrapper's -// delegated child program (busybox's applet, strace's traced command) still -// carries unresolved shell syntax — a bare $VAR, ${VAR}, $(...), or a -// backtick substitution. This fallback tokenizer runs on text the POSIX -// parser rejected and never expands anything, so `busybox "$APPLET" …` -// arrives with the literal token `$APPLET` rather than a blank one. Matching -// that token against known program names silently reads "cannot resolve" as -// "not a network program"; fail closed instead, the same direction -// busyboxSourceDynamic/straceSourceDynamic already fail closed in on the AST -// path for the equivalent gap. +// fallbackTokenLooksDynamic reports whether a token selected as executable +// source still contains an unresolved POSIX or CMD expansion. The fallback +// never expands these values, so treating the spelling as an ordinary unknown +// program would turn "cannot resolve" into "does not use the network." func fallbackTokenLooksDynamic(token string) bool { - return strings.ContainsAny(token, "$`") + if strings.ContainsAny(token, "$`") { + return true + } + return containsCMDVariableExpansion(token, '%') || containsCMDVariableExpansion(token, '!') +} + +func containsCMDVariableExpansion(token string, delimiter byte) bool { + for start := 0; start < len(token); start++ { + if token[start] != delimiter { + continue + } + endOffset := strings.IndexByte(token[start+1:], delimiter) + if endOffset < 0 { + return false + } + content := token[start+1 : start+1+endOffset] + if delimiter == '%' { + if colon := strings.IndexByte(content, ':'); colon >= 0 { + content = content[:colon] + } + } + if validCMDVariableName(content) { + return true + } + start += endOffset + 1 + } + return false +} + +func validCMDVariableName(name string) bool { + if name == "" { + return false + } + for index, r := range name { + if (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || r == '_' || + (index > 0 && r >= '0' && r <= '9') { + continue + } + return false + } + return true } func fallbackPayloadUsesNetwork(payload string, depth int) bool { diff --git a/internal/sandbox/risk_hardening_test.go b/internal/sandbox/risk_hardening_test.go index b529a780c..1c3aecefd 100644 --- a/internal/sandbox/risk_hardening_test.go +++ b/internal/sandbox/risk_hardening_test.go @@ -646,6 +646,72 @@ func TestClassifyUnparseableCommandBearingWrapperValuesFailClosed(t *testing.T) } } +func TestClassifyUnparseableDynamicCommandSourcesFailClosed(t *testing.T) { + for _, command := range []string{ + `set "N=curl" & call %N% https://evil.test & rem '`, + `set "N=curl" & cmd /c %N% https://evil.test & rem '`, + `set "N=curl" & start %N% https://evil.test & rem '`, + `set "N=curl" & call %N:x=y% https://evil.test & rem '`, + `set "N=git" & call !N! push origin main & rem '`, + `sh -c "$PAYLOAD" & rem '`, + `sh -c "${PAYLOAD}" & rem '`, + "sh -c \"$(printf curl)\" & rem '", + } { + t.Run(command, func(t *testing.T) { + if analysis := AnalyzeCommand(command); !analysis.TooComplex { + t.Fatalf("AnalyzeCommand(%q) parsed; this case must exercise the fallback", command) + } + risk := classifyCommand(command) + if risk.Level != RiskCritical || !HasRiskCategory(risk, "network") { + t.Fatalf("Classify(%q) = level %s, categories %v; want critical network", command, risk.Level, risk.Categories) + } + }) + } +} + +func TestClassifyUnparseableLiteralPercentBangAndShellSourceStayLocal(t *testing.T) { + for _, command := range []string{ + `call 100%local printf ok & rem '`, + `call wow!local printf ok & rem '`, + `sh -c 'printf ok' & rem '`, + } { + t.Run(command, func(t *testing.T) { + if risk := classifyCommand(command); HasRiskCategory(risk, "network") { + t.Fatalf("Classify(%q) = categories %v; want no network category", command, risk.Categories) + } + }) + } +} + +func TestEnvSplitAssignmentDependenciesMatchASTAndFallback(t *testing.T) { + for _, testCase := range []struct { + command string + network bool + }{ + {`CMD=curl env -S '${CMD} https://evil.test'`, true}, + {`PAYLOAD='curl https://evil.test' env -S 'sh -c "${PAYLOAD}"'`, true}, + {`VALUE=x env -S 'printf ${VALUE}'`, false}, + {`env -S 'sh -c "printf ok"'`, false}, + } { + t.Run(testCase.command, func(t *testing.T) { + if analysis := AnalyzeCommand(testCase.command); analysis.TooComplex { + t.Fatalf("AnalyzeCommand(%q) reported TooComplex; this case must exercise the AST path", testCase.command) + } + if got := HasRiskCategory(classifyCommand(testCase.command), "network"); got != testCase.network { + t.Errorf("AST Classify(%q) network = %v, want %v", testCase.command, got, testCase.network) + } + + unparseable := testCase.command + " & rem '" + if analysis := AnalyzeCommand(unparseable); !analysis.TooComplex { + t.Fatalf("AnalyzeCommand(%q) parsed; this case must exercise the fallback", unparseable) + } + if got := HasRiskCategory(classifyCommand(unparseable), "network"); got != testCase.network { + t.Errorf("fallback Classify(%q) network = %v, want %v", unparseable, got, testCase.network) + } + }) + } +} + // These malformed forms contain network-looking text in non-executing variable, // arithmetic, array, escaped-backtick, or ordinary argument contexts. They must // stay non-network so fallback tokenization does not over-flag inert text. diff --git a/internal/sandbox/safe_command.go b/internal/sandbox/safe_command.go index 73e60aada..8366dd1c6 100644 --- a/internal/sandbox/safe_command.go +++ b/internal/sandbox/safe_command.go @@ -1,7 +1,6 @@ package sandbox import ( - "os" "runtime" "strings" ) @@ -409,9 +408,10 @@ func commandBodyFields(fields []string) []string { // split string is argv syntax, not shell source, so metacharacters remain inert // unless the resulting executable is itself a shell with -c. type envSplitCommandResult struct { - command []string - recognized bool - executableEnvironmentDependent bool + command []string + recognized bool + executableEnvironmentDependent bool + commandSourceEnvironmentDependent bool } func envSplitCommandFields(fields []string) envSplitCommandResult { @@ -508,10 +508,12 @@ func envSplitCommand(args []string) envSplitCommandResult { command = commandBodyFields(command) commandIndex = len(current) - len(command) } + commandDependencies := dependent[commandIndex:] return envSplitCommandResult{ - command: command, - recognized: true, - executableEnvironmentDependent: len(command) > 0 && dependent[commandIndex], + command: command, + recognized: true, + executableEnvironmentDependent: len(commandDependencies) > 0 && commandDependencies[0], + commandSourceEnvironmentDependent: envSplitCommandSourceDependent(command, commandDependencies), } } // Excessive rewrites are attacker-controlled ambiguity. Keep the network @@ -519,6 +521,32 @@ func envSplitCommand(args []string) envSplitCommandResult { return envSplitCommandResult{recognized: true, executableEnvironmentDependent: true} } +func envSplitCommandSourceDependent(command []string, dependent []bool) bool { + if len(command) == 0 || len(command) != len(dependent) { + return false + } + program := normalizeProgramToken(command[0]) + if dependent[0] { + return true + } + args, argDependent := command[1:], dependent[1:] + if shellPrograms[program] { + if index, ok := shellCommandPayloadIndex(program, args); ok && index < len(argDependent) { + return argDependent[index] + } + return false + } + if program == "powershell" || program == "pwsh" || program == "cmd" || + program == "call" || program == "start" || program == "env" { + for _, value := range argDependent { + if value { + return true + } + } + } + return false +} + // envSplitLongOptionName is GNU env's only long option beginning with "s", so // any unambiguous prefix of it ("--s", "--split", "--split-strin", ...) names // the same option under GNU getopt_long's abbreviation rule. @@ -683,7 +711,7 @@ func splitEnvString(value string) ([]string, []bool, bool) { if end >= len(runes) || !validEnvSplitVariableName(string(runes[index+2:end])) { return nil, nil, false } - word.WriteString(os.Getenv(string(runes[index+2 : end]))) + word.WriteString(string(runes[index : end+1])) started = true wordDependent = true index = end From 66cf95f784cb2d43af28569a803c5860208dd8a0 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sun, 16 Aug 2026 15:16:48 +0200 Subject: [PATCH 03/12] fix(sandbox): classify interpreter source as command text MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the three merge blockers plus the architectural split behind them: the classifier treated interpreter SOURCE as argv tokens, and the parseable and unparseable paths reached different conclusions about the same launcher. One shared operation for command text classifyCommandText is now the single entry point for a string another program will run as a command line. It re-tokenizes the payload instead of taking its basename, runs both the AST scan and the unparseable matcher (a CMD one-liner is legitimately not POSIX, so the parser rejecting it is expected rather than proof of safety), and fails closed when the text cannot be read. CMD /c and /k, CALL, start, eval, shell -c, and PowerShell -Command all route through it on both paths. Quoted CMD and CALL payloads (P1) cmdBodyUsesNetwork reads a single quoted token BOTH ways, because the ambiguity is real: `cmd /c "git push origin main"` is a command line and `cmd /c "C:\Program Files\curl\curl.exe"` is one program path. CMD resolves it by trying both, so this does too and fails closed if either reading reaches the network. Shared with the fallback so the paths cannot diverge. env -S trailing argv (P1) A literal -S operand proves something about the split string, not about the invocation: GNU env appends the remaining argv to the argv the split string produced, so `env -S 'sh -c' "$PAYLOAD"` runs unreadable text. The scan now continues past the operand and fails closed on any dynamic token after it. Trailing literal argv stays local. Nested shell payloads wordText returned raw source for double-quoted parts, because the parser leaves escape removal to expansion time. For an argv token that is harmless; for a -c operand the text IS the next command, so one level of `sh -c "sh -c \"curl …\""` handed the recursion the fragment `\"sh` and lost everything after it. unescapeDoubleQuoted applies POSIX escape removal for the five characters where a backslash is special, leaving quoted Windows paths intact. Parseable-path parity eval and CMD's echo-suppression prefix were handled in the fallback but not in the AST, so identical text was network only when something else defeated the parser. Both now classify on the parseable path. git send-pack joins the network subcommand list, which both paths read. GitTerminalGlobalOption is exported so command_prefix stops where the sandbox stops — `git --help status` no longer resolves to the auto-approved prefix `git status`. Tests TestEvaluatePromptsForParseableNetworkLaunchers is the parity matrix the unparseable table lacked: every launcher, parseable (asserted), with the shell permission already granted, expecting ActionPrompt/ReasonNetworkBlocked — plus a negative table so failing closed does not mean flagging everything. The turn-grant integration test now asserts the plan's policy is NetworkAllow while the approved call runs and is not after the turn, rather than inferring the grant from the command having executed. Refs #703. Co-Authored-By: Claude Opus 5 (1M context) --- internal/agent/command_prefix.go | 8 ++ internal/agent/command_prefix_test.go | 29 +++++ internal/agent/loop_test.go | 72 +++++++++++- internal/sandbox/analyzer.go | 158 ++++++++++++++++++++++++-- internal/sandbox/analyzer_test.go | 128 +++++++++++++++++++++ internal/sandbox/engine_test.go | 102 +++++++++++++++++ internal/sandbox/risk.go | 23 ++-- 7 files changed, 494 insertions(+), 26 deletions(-) diff --git a/internal/agent/command_prefix.go b/internal/agent/command_prefix.go index 6279b284d..628a640b7 100644 --- a/internal/agent/command_prefix.go +++ b/internal/agent/command_prefix.go @@ -399,6 +399,14 @@ func gitSubcommand(command []string) (int, string, bool) { index++ continue } + // A terminal global makes git print and exit, so the token after it is + // help text rather than a subcommand. Stopping here keeps this parser + // aligned with the sandbox classifier, which already stops: without it + // `git --help status` resolved to the read-only prefix `git status` + // and was auto-approved as a command it is not. + if sandbox.GitTerminalGlobalOption(arg) { + return 0, "", false + } if gitOptionHasInlineValue(arg) || arg == "--" || strings.HasPrefix(arg, "-") { continue } diff --git a/internal/agent/command_prefix_test.go b/internal/agent/command_prefix_test.go index 69c9160db..6283e179c 100644 --- a/internal/agent/command_prefix_test.go +++ b/internal/agent/command_prefix_test.go @@ -242,3 +242,32 @@ func TestProposedCommandPrefixRejectsRequestedUnsafeLauncherPrefix(t *testing.T) t.Fatalf("unsafe requested launcher prefix should be rejected, got %#v", got) } } + +// A terminal global makes git print and exit, so nothing after it is a +// subcommand. The sandbox classifier already stopped there; this parser walked +// past it and resolved `git --help status` to the read-only prefix +// `git status`, auto-approving a command the user never ran. The two scans read +// one option grammar (sandbox.GitTerminalGlobalOption) so they cannot drift. +func TestSafeGitCommandStopsAtTerminalGlobalOptions(t *testing.T) { + for _, command := range [][]string{ + {"git", "--help", "status"}, + {"git", "-h", "status"}, + {"git", "--version", "log"}, + {"git", "--exec-path", "status"}, + {"git", "-C", "repo", "--help", "diff"}, + } { + if safeGitCommand(command) { + t.Errorf("safeGitCommand(%q) = true; a terminal global ends the subcommand scan", command) + } + } + // An inline --exec-path= is a value-carrying global, not terminal, so + // the subcommand after it is still real. + for _, command := range [][]string{ + {"git", "--exec-path=/usr/libexec/git-core", "status"}, + {"git", "--namespace", "ns", "status"}, + } { + if !safeGitCommand(command) { + t.Errorf("safeGitCommand(%q) = false; want true", command) + } + } +} diff --git a/internal/agent/loop_test.go b/internal/agent/loop_test.go index 5223cc557..849f677ac 100644 --- a/internal/agent/loop_test.go +++ b/internal/agent/loop_test.go @@ -2460,8 +2460,19 @@ func TestRunApprovedGitPushPromptAppliesTurnNetworkGrant(t *testing.T) { t.Fatal(err) } } + engine := sandbox.NewEngine(sandbox.EngineOptions{ + WorkspaceRoot: root, + Policy: sandbox.DefaultPolicy(), + Backend: sandbox.Backend{Name: sandbox.BackendUnavailable, Message: "native sandbox unavailable"}, + }) + var overlay []sandbox.NetworkMode registry := tools.NewRegistry() - registry.Register(tools.NewScopedBashTool(root, nil)) + registry.Register(&networkOverlayProbeTool{ + delegate: tools.NewScopedBashTool(root, nil), + engine: engine, + observed: &overlay, + t: t, + }) provider := &mockProvider{ turns: [][]zeroruntime.StreamEvent{ { @@ -2482,11 +2493,7 @@ func TestRunApprovedGitPushPromptAppliesTurnNetworkGrant(t *testing.T) { Registry: registry, PermissionMode: PermissionModeAsk, Autonomy: "medium", - Sandbox: sandbox.NewEngine(sandbox.EngineOptions{ - WorkspaceRoot: root, - Policy: sandbox.DefaultPolicy(), - Backend: sandbox.Backend{Name: sandbox.BackendUnavailable, Message: "native sandbox unavailable"}, - }), + Sandbox: engine, OnPermissionRequest: func(_ context.Context, request PermissionRequest) (PermissionDecision, error) { requests = append(requests, request) return PermissionDecision{Action: PermissionDecisionAllow, Reason: "approve network once"}, nil @@ -2523,6 +2530,59 @@ func TestRunApprovedGitPushPromptAppliesTurnNetworkGrant(t *testing.T) { if !strings.Contains(lastMessage.Content, "fake git push gitlawb://example.com/repo.git main") { t.Fatalf("expected approved network command output after degraded execution, got %q", lastMessage.Content) } + // The command running is not proof the GRANT was applied: under + // BackendUnavailable the degraded path executes regardless. Assert the turn + // overlay itself — the approval must have widened the plan's policy to + // NetworkAllow while the tool ran, and must not outlive the turn. + if len(overlay) != 1 { + t.Fatalf("expected one recorded overlay observation, got %#v", overlay) + } + if overlay[0] != sandbox.NetworkAllow { + t.Fatalf("plan policy network = %q during the approved call, want %q", overlay[0], sandbox.NetworkAllow) + } + if after := planNetworkMode(t, engine); after == sandbox.NetworkAllow { + t.Fatalf("turn network grant outlived the turn: policy network = %q", after) + } +} + +// planNetworkMode reads the network mode the engine would actually build a +// command plan with, which is where a turn grant's overlay becomes observable. +func planNetworkMode(t *testing.T, engine *sandbox.Engine) sandbox.NetworkMode { + t.Helper() + + plan, err := engine.BuildCommandPlan(sandbox.CommandSpec{Name: "git", Args: []string{"push"}}) + if err != nil { + t.Fatalf("BuildCommandPlan returned error: %v", err) + } + return sandbox.NormalizeNetworkMode(plan.Policy.Network) +} + +// networkOverlayProbeTool is a bash-named shell tool that records the network +// mode in effect at the moment the approved call runs, then delegates to the +// real scoped bash tool so the command still executes. +type networkOverlayProbeTool struct { + delegate tools.Tool + engine *sandbox.Engine + observed *[]sandbox.NetworkMode + t *testing.T +} + +func (tool *networkOverlayProbeTool) Name() string { return tool.delegate.Name() } +func (tool *networkOverlayProbeTool) Description() string { return tool.delegate.Description() } +func (tool *networkOverlayProbeTool) Parameters() tools.Schema { + return tool.delegate.Parameters() +} + +func (tool *networkOverlayProbeTool) Safety() tools.Safety { + if safe, ok := tool.delegate.(interface{ Safety() tools.Safety }); ok { + return safe.Safety() + } + return tools.Safety{SideEffect: tools.SideEffectShell, Permission: tools.PermissionPrompt} +} + +func (tool *networkOverlayProbeTool) Run(ctx context.Context, args map[string]any) tools.Result { + *tool.observed = append(*tool.observed, planNetworkMode(tool.t, tool.engine)) + return tool.delegate.Run(ctx, args) } func TestRunDoesNotOfferPrefixApprovalForUnsafeBashCommand(t *testing.T) { diff --git a/internal/sandbox/analyzer.go b/internal/sandbox/analyzer.go index 8966abb98..8a1b4365f 100644 --- a/internal/sandbox/analyzer.go +++ b/internal/sandbox/analyzer.go @@ -214,11 +214,27 @@ func analyzeInto(script string, result *AnalysisResult, seen map[string]bool, de // command that makes no network call. result.Network = true case source.payload != "": - if depth >= maxAnalyzerDepth || textualPayloadUsesNetwork(source.payload, depth+1) { + if classifyCommandText(source.payload, depth+1) { result.Network = true } } } + // eval runs its remaining arguments as shell source, exactly like the + // fallback path already treats it. Without this branch a parseable + // `eval git push origin main` resolved to the program "eval" and no + // network, while the same text behind an unparseable suffix was caught + // — the fallback and the AST disagreeing about the same launcher. + if prog == "eval" && len(rest) > 0 { + if payload, ok := literalCallFields(rest); ok { + if classifyCommandText(strings.Join(payload, " "), depth+1) { + result.Network = true + } + } else { + // Source assembled from an expansion is source this scan cannot + // read, and eval will run it regardless. + result.Network = true + } + } if cmdLauncherUsesNetwork(prog, rest, depth) { result.Network = true } @@ -257,7 +273,12 @@ func commandWordsUseNetwork(prog string, words []string) bool { } func commandWordsUseNetworkAt(prog string, words []string, depth int) bool { - prog = normalizeProgramToken(prog) + // CMD's echo-suppression prefix is not part of the program name: `@curl …` + // runs curl. The fallback tokenizer already strips it (trimCMDEchoPrefix), + // so without the same normalization here the AST path called the program + // "@curl" and found no network, while the identical text behind an + // unparseable suffix was flagged. Stripping it can only ADD the category. + prog = normalizeProgramToken(trimCMDEchoPrefixToken(prog)) originalWords := words normalized := make([]string, len(words)) for index := range words { @@ -360,7 +381,11 @@ func gitUsesNetwork(words []string) bool { return false } switch invocation.subcommand { - case "clone", "fetch", "pull", "push", "ls-remote": + // send-pack is push's plumbing counterpart — `git send-pack origin main` + // performs exactly the egress `git push` does, so leaving it out gave the + // same operation two different answers depending on which spelling was + // used. + case "clone", "fetch", "pull", "push", "ls-remote", "send-pack": return true case "archive": // `git archive HEAD` streams a tree out of the local object store and needs @@ -497,6 +522,18 @@ func parseGitInvocation(words []string) gitInvocation { // `--exec-path` is deliberately absent: its value is inline-only // (`--exec-path=`), and the bare spelling is terminal — see // gitTerminalGlobalOptions. +// GitTerminalGlobalOption reports whether a git global option makes git print +// something from the local installation and exit, so nothing after it is a +// subcommand. It is the exported view of gitTerminalGlobalOptions, shared with +// internal/agent's command-prefix parser for the same reason +// GitGlobalOptionConsumesValue is: while each scan carried its own option +// grammar, `git --help status` was the safe prefix `git status` to one parser +// and a terminal help invocation to the other, and every new option had to be +// remembered in two places. +func GitTerminalGlobalOption(option string) bool { + return gitTerminalGlobalOptions[strings.ToLower(strings.TrimSpace(option))] +} + func GitGlobalOptionConsumesValue(option string) bool { switch strings.ToLower(option) { case "-c", "--attr-source", "--config-env", "--git-dir", "--namespace", "--super-prefix", "--work-tree": @@ -572,8 +609,19 @@ func envSplitSourceDynamic(args []*syntax.Word) bool { continue } if seenSplit { - // The operand of a separated -S is literal; the ordinary literal path - // already reads it. + // The -S operand is literal, but that is only proof about the SPLIT + // STRING, not about the invocation: GNU env appends the remaining + // argv to the argv the split string produced, so + // `env -S 'sh -c' "$PAYLOAD"` runs an argument this scan cannot + // read. Keep scanning; a dynamic token anywhere after the split + // string extends the command unknowably and fails closed. Trailing + // LITERAL tokens are fine — the ordinary literal path reads the + // whole reconstructed argv, this one included. + for rest := index + 1; rest < len(args); rest++ { + if !isLiteralWord(args[rest]) { + return true + } + } return false } if strings.Contains(text, "=") && !strings.HasPrefix(text, "=") && !strings.HasPrefix(text, "-") { @@ -688,13 +736,35 @@ func cmdLauncherUsesNetwork(program string, args []*syntax.Word, depth int) bool }) } for _, body := range cmdCommandBodyTokenInfoCandidates(tokens) { - if fallbackBodyUsesNetwork(fallbackTokenValues(body), depth) { + if cmdBodyUsesNetwork(body, depth) { return true } } return false } +// cmdBodyUsesNetwork classifies one CMD payload candidate. +// +// A single QUOTED token is genuinely ambiguous: `cmd /c "git push origin main"` +// is a command line, while `cmd /c "C:\Program Files\curl.exe"` is one program +// path that happens to contain a space. CMD itself resolves that by trying the +// path first and falling back to parsing it as a command line, so this +// classifies it BOTH ways and fails closed if either reading reaches the +// network. Treating it only as a program name — taking its basename — is what +// made `git push origin main` look like an unrecognized executable. +func cmdBodyUsesNetwork(body []fallbackCommandToken, depth int) bool { + if len(body) == 0 { + return false + } + if fallbackBodyUsesNetwork(fallbackTokenValues(body), depth) { + return true + } + if len(body) == 1 && body[0].quoted && strings.ContainsAny(body[0].value, " \t") { + return classifyCommandText(body[0].value, depth+1) + } + return false +} + func literalWordIsQuoted(word *syntax.Word) bool { if word == nil || len(word.Parts) == 0 { return false @@ -707,15 +777,40 @@ func literalWordIsQuoted(word *syntax.Word) bool { } } -// textualPayloadUsesNetwork classifies source carried by another interpreter -// without leaking the nested parser's TooComplex bit into the outer command. -func textualPayloadUsesNetwork(payload string, depth int) bool { - if depth > maxAnalyzerDepth { +// classifyCommandText is the single entry point for interpreter SOURCE — a +// string another program will run as a command line — as opposed to argv +// tokens naming a program to execute. It is the difference between +// `cmd /c "git push origin main"` meaning "run the command line git push +// origin main" and meaning "run the program named `git push origin main`"; +// reading the second as the first is what let quoted one-liners through. +// +// Every launcher that carries command text routes through here: CMD /c and +// /k, CALL, start, eval, shell -c/--command, PowerShell -Command, and the +// fallback tokenizer's equivalents. It re-tokenizes the payload rather than +// taking its basename, runs BOTH the AST scan and the unparseable matcher (a +// CMD one-liner is legitimately not POSIX, so the parser failing on it is +// expected, not proof of safety), and fails closed when the text cannot be +// read at all. +func classifyCommandText(payload string, depth int) bool { + payload = strings.TrimSpace(payload) + if payload == "" { return false } + if depth > maxAnalyzerDepth { + // The text exists and is about to run, but the budget to inspect it is + // gone. An unread payload is not a safe one. + return true + } result := AnalysisResult{} analyzeInto(payload, &result, map[string]bool{}, depth) - return result.Network || (result.TooComplex && matchesUnparseableNetworkAt(payload, depth)) + if result.Network { + return true + } + // The fallback matcher is the reader for text the POSIX parser rejects, and + // it is also a second opinion on text the parser accepted: the two + // tokenizers disagree about CMD quoting and echo prefixes, and only one of + // them needs to see the network program. + return matchesUnparseableNetworkAt(payload, depth) } func pythonModuleUsesNetwork(words []string) bool { @@ -777,6 +872,45 @@ func firstSubcommand(words []string, aliases map[string]string) string { // wordText returns the literal text of a shell word, concatenating its plain and // quoted literal parts (so "vim", 'vim', and vim all yield "vim"). Parts that are // expansions ($x, $(...)) contribute nothing — the program name is taken as-is. +// unescapeDoubleQuoted applies POSIX double-quote escape removal to the literal +// text the parser preserves verbatim inside a double-quoted word. +// +// The parser keeps `\"` as two characters because escape removal is the +// shell's job at expansion time, not the parser's. For an argv token that is +// harmless; for the `-c` operand of a shell launcher it is not, because the +// text IS the next command. Without this, one level of +// `sh -c "sh -c \"curl …\""` handed the recursion the fragment `\"sh` — the +// rest of the payload silently dropped — and the nested curl disappeared from +// classification entirely at nesting level 3. +// +// Inside double quotes a backslash is special ONLY before $, `, ", \, and a +// newline; everywhere else it is a literal backslash and must be preserved, or +// a Windows path inside quotes would lose its separators. +func unescapeDoubleQuoted(value string) string { + if !strings.Contains(value, `\`) { + return value + } + var builder strings.Builder + builder.Grow(len(value)) + for index := 0; index < len(value); index++ { + if value[index] != '\\' || index+1 >= len(value) { + builder.WriteByte(value[index]) + continue + } + switch next := value[index+1]; next { + case '$', '`', '"', '\\': + builder.WriteByte(next) + index++ + case '\n': + // A quoted line continuation removes both characters. + index++ + default: + builder.WriteByte('\\') + } + } + return builder.String() +} + func wordText(word *syntax.Word) string { if word == nil { return "" @@ -791,7 +925,7 @@ func wordText(word *syntax.Word) string { case *syntax.DblQuoted: for _, inner := range typed.Parts { if lit, ok := inner.(*syntax.Lit); ok { - builder.WriteString(lit.Value) + builder.WriteString(unescapeDoubleQuoted(lit.Value)) } } } diff --git a/internal/sandbox/analyzer_test.go b/internal/sandbox/analyzer_test.go index 3d8362084..8825e6337 100644 --- a/internal/sandbox/analyzer_test.go +++ b/internal/sandbox/analyzer_test.go @@ -239,3 +239,131 @@ func TestAnalyzeCommandEmptyIsClean(t *testing.T) { t.Fatalf("empty script should be clean, got %#v", got) } } + +// unescapeDoubleQuoted implements POSIX double-quote escape removal, which the +// parser deliberately leaves to expansion time. For an argv token that is +// harmless; for a shell launcher's -c operand the text IS the next command, so +// keeping `\"` verbatim handed the recursion a fragment and lost everything +// after it. +func TestUnescapeDoubleQuoted(t *testing.T) { + cases := []struct{ in, want string }{ + {`plain`, `plain`}, + {`sh -c \"curl x\"`, `sh -c "curl x"`}, + {`a\\b`, `a\b`}, + {`\$HOME`, `$HOME`}, + {"\\`cmd\\`", "`cmd`"}, + // A backslash before anything else is literal: a quoted Windows path + // must survive intact. + {`C:\Users\me\file.txt`, `C:\Users\me\file.txt`}, + {`C:\temp\n`, `C:\temp\n`}, + {"line\\" + "\n" + "cont", "linecont"}, + } + for _, testCase := range cases { + if got := unescapeDoubleQuoted(testCase.in); got != testCase.want { + t.Errorf("unescapeDoubleQuoted(%q) = %q, want %q", testCase.in, got, testCase.want) + } + } +} + +// A quoted CMD payload is command SOURCE, but a quoted path containing a space +// is a program name. CMD resolves that ambiguity by trying both, so the +// classifier does too and fails closed if either reading reaches the network. +func TestAnalyzeCommandReadsQuotedCMDPayloadBothWays(t *testing.T) { + network := []string{ + `cmd /c "git push origin main"`, + `cmd /c "curl https://evil.test"`, + `cmd /k "git fetch origin"`, + `call "git push origin main"`, + `start "" "curl https://evil.test"`, + // Program-path reading: the quoted token IS the executable. + `cmd /c "C:\Program Files\curl\curl.exe"`, + } + for _, command := range network { + if analysis := AnalyzeCommand(command); !analysis.Network { + t.Errorf("AnalyzeCommand(%q).Network = false, want true", command) + } + } + local := []string{ + `cmd /c "git status"`, + `cmd /c "echo hello world"`, + `cmd /c "C:\Program Files\git\bin\git.exe status"`, + `call "git status"`, + } + for _, command := range local { + if analysis := AnalyzeCommand(command); analysis.Network { + t.Errorf("AnalyzeCommand(%q).Network = true, want false", command) + } + } +} + +// GNU env appends the remaining argv to the argv the split string produced, so +// proving the -S operand is literal proves nothing about the whole invocation: +// `env -S 'sh -c' "$PAYLOAD"` runs text this scan cannot read. +func TestAnalyzeCommandFailsClosedOnDynamicArgvAfterEnvSplit(t *testing.T) { + dynamic := []string{ + `env -S 'git push origin main' "$EXTRA"`, + `env -S 'printf ok' $PAYLOAD`, + `env -S 'printf ok' "${PAYLOAD}" https://evil.test`, + `env -S 'sh -c' "$PAYLOAD"`, + `env --split-string='printf ok' $PAYLOAD`, + `sudo env -S 'printf ok' $PAYLOAD`, + } + for _, command := range dynamic { + if analysis := AnalyzeCommand(command); !analysis.Network { + t.Errorf("AnalyzeCommand(%q).Network = false, want true", command) + } + } + // Trailing LITERAL argv is fully readable and must not cost a prompt. + literal := []string{ + `env -S 'printf ok' literal args`, + `env -S 'git status' --`, + `env -S 'echo hi'`, + } + for _, command := range literal { + if analysis := AnalyzeCommand(command); analysis.Network { + t.Errorf("AnalyzeCommand(%q).Network = true, want false", command) + } + } +} + +// eval and CMD's echo-suppression prefix were handled on the fallback path but +// not on the AST path, so the identical text was network only when something +// else in the command happened to defeat the parser. +func TestAnalyzeCommandClassifiesEvalAndEchoPrefixOnParseablePath(t *testing.T) { + network := []string{ + `eval git push origin main`, + `eval "curl https://evil.test"`, + `eval $PAYLOAD`, + `@curl https://evil.test`, + `@git push origin main`, + } + for _, command := range network { + analysis := AnalyzeCommand(command) + if analysis.TooComplex { + t.Fatalf("AnalyzeCommand(%q) is TooComplex; this test must cover the parseable path", command) + } + if !analysis.Network { + t.Errorf("AnalyzeCommand(%q).Network = false, want true", command) + } + } + for _, command := range []string{`eval echo hi`, `eval git status`, `@echo hello`} { + if analysis := AnalyzeCommand(command); analysis.Network { + t.Errorf("AnalyzeCommand(%q).Network = true, want false", command) + } + } +} + +// send-pack is push's plumbing counterpart and performs the same egress; both +// classification paths read one subcommand list, so covering gitUsesNetwork +// covers the fallback too. +func TestGitSendPackIsNetwork(t *testing.T) { + for _, command := range []string{ + `git send-pack origin main`, + `git -C repo send-pack origin main`, + `git send-pack origin main & rem '`, + } { + if analysis := AnalyzeCommand(command); !analysis.Network && !matchesUnparseableNetwork(command) { + t.Errorf("%q classified as local; send-pack pushes to a remote", command) + } + } +} diff --git a/internal/sandbox/engine_test.go b/internal/sandbox/engine_test.go index cc337ba36..34e999eaf 100644 --- a/internal/sandbox/engine_test.go +++ b/internal/sandbox/engine_test.go @@ -1120,3 +1120,105 @@ func TestEvaluateAllowsWritesInsideDefaultTempRoot(t *testing.T) { t.Fatalf("temp-root write risk=%v, must not be out_of_workspace", decision.Risk) } } + +// TestEvaluatePromptsForParseableNetworkLaunchers is the parity matrix for the +// PARSEABLE path. +// +// Every launcher covered by TestEvaluatePromptsForUnparseableNetworkBehindWrapper +// is exercised there behind an unparseable suffix (`& rem '`, `&& "unterminated`), +// which forces classification down the fallback. That proved the fallback and +// hid the real exposure: production input is usually parseable, and a launcher +// the AST path did not recurse into stayed clean no matter what the fallback +// would have said. `cmd /c "git push origin main"` and `eval git push origin main` +// were both caught by the fallback and both allowed here. +// +// So each case below must ALSO be parseable — the guard asserts it — and must +// still reach ActionPrompt/ReasonNetworkBlocked with the shell permission +// already granted. A new launcher belongs in both tables. +func TestEvaluatePromptsForParseableNetworkLaunchers(t *testing.T) { + engine := NewEngine(EngineOptions{Policy: Policy{Mode: ModeEnforce, Network: NetworkDeny}}) + for _, command := range []string{ + // Quoted CMD payloads are command SOURCE, not a program name. + `cmd /c "git push origin main"`, + `cmd /c "curl https://evil.test"`, + `cmd /d /c "git push origin main"`, + `cmd /k "curl https://evil.test"`, + `call "git push origin main"`, + `call "curl https://evil.test"`, + // Unquoted forms, which already worked, stay working. + `cmd /c git push origin main`, + `cmd.exe /d /c curl https://evil.test`, + // eval runs its arguments as shell source on this path too. + `eval git push origin main`, + `eval "curl https://evil.test"`, + `eval curl https://evil.test`, + // GNU env appends trailing argv to the split string's argv, so a dynamic + // token after a literal -S operand extends the command unreadably. + `env -S 'git push origin main' "$EXTRA"`, + `env -S 'printf ok' $PAYLOAD`, + `env -S 'printf ok' "${PAYLOAD}" https://evil.test`, + `env -S "$PAYLOAD"`, + // CMD's echo-suppression prefix is not part of the program name. + `@curl https://evil.test`, + `@git push origin main`, + // Nested shell launchers past the level where double-quote escape + // removal used to hand the recursion a fragment. + `sh -c "sh -c \"sh -c \\\"curl https://evil.test\\\"\""`, + `sh -c "sh -c \"git push origin main\""`, + // push's plumbing counterpart performs the same egress. + `git send-pack origin main`, + `git -C repo send-pack origin main`, + } { + t.Run(command, func(t *testing.T) { + if analysis := AnalyzeCommand(command); analysis.TooComplex { + t.Fatalf("AnalyzeCommand(%q) is TooComplex; this table must exercise the PARSEABLE path", command) + } + decision := engine.Evaluate(context.Background(), Request{ + ToolName: "bash", SideEffect: SideEffectShell, PermissionGranted: true, + Args: map[string]any{"command": command}, + }) + if decision.Action != ActionPrompt || decision.Reason != ReasonNetworkBlocked { + t.Fatalf("Evaluate(%q) = action %q reason %q, want a network prompt", command, decision.Action, decision.Reason) + } + }) + } +} + +// The counterpart to the matrix above: failing closed must not mean flagging +// every launcher. These run locally and must NOT cost a network prompt, or the +// classifier trains users to approve egress reflexively. +func TestEvaluateAllowsParseableLocalLaunchers(t *testing.T) { + engine := NewEngine(EngineOptions{Policy: Policy{Mode: ModeEnforce, Network: NetworkDeny}}) + for _, command := range []string{ + `cmd /c "git status"`, + `cmd /c "echo hello world"`, + // A quoted path containing a space is a program name, not a command line. + `cmd /c "C:\Program Files\git\bin\git.exe status"`, + `call "git status"`, + `eval echo hi`, + `eval git status`, + // Trailing LITERAL argv after a split string is fully readable. + `env -S 'printf ok' literal args`, + `env -S 'git status' --`, + `@echo hello`, + `@git status`, + `sh -c "sh -c \"git status\""`, + // Escape removal must not damage an ordinary quoted Windows path. + `echo "C:\Users\me\file.txt"`, + `git status`, + `git --help push`, + } { + t.Run(command, func(t *testing.T) { + if analysis := AnalyzeCommand(command); analysis.TooComplex { + t.Fatalf("AnalyzeCommand(%q) is TooComplex; this table must exercise the PARSEABLE path", command) + } + decision := engine.Evaluate(context.Background(), Request{ + ToolName: "bash", SideEffect: SideEffectShell, PermissionGranted: true, + Args: map[string]any{"command": command}, + }) + if decision.Reason == ReasonNetworkBlocked { + t.Fatalf("Evaluate(%q) requested a network prompt for a local command", command) + } + }) + } +} diff --git a/internal/sandbox/risk.go b/internal/sandbox/risk.go index 2a8dfa846..fe0e9165d 100644 --- a/internal/sandbox/risk.go +++ b/internal/sandbox/risk.go @@ -99,7 +99,7 @@ func matchesUnparseableNetwork(command string) bool { func matchesUnparseableNetworkAt(command string, depth int) bool { if depth < maxUnparseableShellDepth { for _, payload := range fallbackCMDForFCommands(command) { - if textualPayloadUsesNetwork(payload, depth+1) || matchesUnparseableNetworkAt(payload, depth+1) { + if classifyCommandText(payload, depth+1) { return true } } @@ -122,7 +122,10 @@ func matchesUnparseableNetworkAt(command string, depth int) bool { } } for _, body := range cmdCommandBodyTokenInfoCandidates(tokenInfo) { - if fallbackBodyUsesNetwork(fallbackTokenValues(body), depth) { + // Shared with the AST path's cmdLauncherUsesNetwork so a quoted + // payload cannot be command text on one path and a program name on + // the other. + if cmdBodyUsesNetwork(body, depth) { return true } } @@ -199,7 +202,7 @@ func fallbackBodyUsesNetwork(body []string, depth int) bool { // source just as we do for `sh -c`; otherwise quoting the same curl/git // invocation behind eval would hide it from this fail-closed path. if program == "eval" && len(args) > 0 { - if depth >= maxUnparseableShellDepth || matchesUnparseableNetworkAt(strings.Join(args, " "), depth+1) { + if classifyCommandText(strings.Join(args, " "), depth+1) { return true } } @@ -216,9 +219,7 @@ func fallbackBodyUsesNetwork(body []string, depth int) bool { if shellPrograms[program] { if payloadIndex, found := shellCommandPayloadIndex(program, args); found && payloadIndex < len(args) { if payload := args[payloadIndex]; payload != "" { - if fallbackTokenLooksDynamic(payload) || - depth >= maxUnparseableShellDepth || - matchesUnparseableNetworkAt(payload, depth+1) { + if fallbackTokenLooksDynamic(payload) || classifyCommandText(payload, depth+1) { return true } } @@ -236,8 +237,7 @@ func fallbackBodyUsesNetwork(body []string, depth int) bool { // flag. That payload is valid shell input even when the POSIX parser that // sent us here cannot parse it (for example, `cmd /c curl ... & rem '`). if payload := fallbackCommandInterpreterPayload(program, args); payload != "" { - if depth >= maxUnparseableShellDepth || - matchesUnparseableNetworkAt(payload, depth+1) || + if classifyCommandText(payload, depth+1) || fallbackPayloadUsesNetwork(strings.Join(fallbackCommandInterpreterArgs(program, args), " "), depth+1) { return true } @@ -468,6 +468,13 @@ func splitCMDEscapedWhitespace(value string) []string { return parts } +// trimCMDEchoPrefixToken strips CMD's echo-suppression prefix from a single +// program token. It is the one-token form of trimCMDEchoPrefix, shared so the +// AST and fallback paths cannot disagree about whether "@curl" names curl. +func trimCMDEchoPrefixToken(token string) string { + return strings.TrimLeft(strings.TrimSpace(token), "@") +} + func trimCMDEchoPrefix(fields []fallbackCommandToken) []fallbackCommandToken { for len(fields) > 0 { fields[0].value = strings.TrimLeft(fields[0].value, "@") From 47c07e07e75ea5b3f54c571ce94d10b1f3db7a48 Mon Sep 17 00:00:00 2001 From: Amp Date: Tue, 18 Aug 2026 20:48:09 +0000 Subject: [PATCH 04/12] fix(sandbox): block unresolved nested launchers Amp-Thread-ID: https://ampcode.com/threads/T-01a01695-4d5a-75a9-b8b5-7ce3cbb57943 Co-authored-by: Pierre Bruno --- internal/sandbox/safe_command.go | 19 ++++++++++++++++--- internal/sandbox/safe_command_test.go | 11 +++++++---- internal/tools/bash_tool_test.go | 23 +++++++++++++++++++++++ 3 files changed, 46 insertions(+), 7 deletions(-) diff --git a/internal/sandbox/safe_command.go b/internal/sandbox/safe_command.go index 8366dd1c6..bb9eb2cba 100644 --- a/internal/sandbox/safe_command.go +++ b/internal/sandbox/safe_command.go @@ -152,7 +152,12 @@ func DetectInteractiveCommand(command string, goos string) InteractiveCommandRes func detectInteractiveCommandAt(command string, goos string, depth int) InteractiveCommandResult { if depth > maxAnalyzerDepth { - return InteractiveCommandResult{} + return InteractiveCommandResult{ + Interactive: true, + Command: "nested shell launcher", + Reason: "shell launcher nesting exceeds the safe inspection limit, so its payload cannot be proven non-interactive", + Suggestion: "Simplify the nested shell command or run its non-interactive payload directly.", + } } command = strings.TrimSpace(command) if command == "" { @@ -202,7 +207,9 @@ func detectInteractiveCommandAt(command string, goos string, depth int) Interact if inner := detectInteractiveCommandAt(payload, goos, depth+1); inner.Interactive { return inner } - continue + // strings.Fields cannot preserve a quoted multi-word payload. If its + // best-effort read is clean, keep going so the AST pass below can + // inspect the actual payload rather than treating truncation as safe. } program, ok := interactivePrograms[first] if !ok { @@ -265,7 +272,13 @@ func inspectCommandFields(fields []string, goos string, depth int) (InteractiveC if first == "" { return InteractiveCommandResult{}, false } - if payload := shellDashCPayload(first, fields); payload != "" { + start := programIndex(first, fields) + args := fields[start+1:] + payloadIndex, hasPayload := shellCommandPayloadIndex(first, args) + if hasPayload && payloadIndex < len(args) { + // astCommandFields already removed the launcher's outer quoting. Do not + // trim again: trailing quotes can belong to a nested launcher's payload. + payload := args[payloadIndex] if inner := detectInteractiveCommandAt(payload, goos, depth+1); inner.Interactive { return inner, true } diff --git a/internal/sandbox/safe_command_test.go b/internal/sandbox/safe_command_test.go index ec2ad7bca..c5239498e 100644 --- a/internal/sandbox/safe_command_test.go +++ b/internal/sandbox/safe_command_test.go @@ -1,7 +1,6 @@ package sandbox import ( - "strconv" "strings" "testing" ) @@ -411,10 +410,14 @@ func TestDetectInteractiveMongoEvalAndFullPaths(t *testing.T) { func TestDetectInteractiveCommandBoundsNestedShellLaunchers(t *testing.T) { command := "vim file" for range maxAnalyzerDepth + 2 { - command = "bash -c " + strconv.Quote(command) + command = "bash -c '" + strings.ReplaceAll(command, "'", `'"'"'`) + "'" } - if got := DetectInteractiveCommand(command, "linux"); got.Interactive { - t.Fatalf("DetectInteractiveCommand(deep shell chain) = %+v; want bounded inspection", got) + got := DetectInteractiveCommand(command, "linux") + if !got.Interactive || got.Command != "nested shell launcher" { + t.Fatalf("DetectInteractiveCommand(deep shell chain) = %+v; want conservative depth-limit result", got) + } + if !strings.Contains(got.Reason, "cannot be proven non-interactive") { + t.Fatalf("DetectInteractiveCommand(deep shell chain).Reason = %q; want unresolved inspection reason", got.Reason) } } diff --git a/internal/tools/bash_tool_test.go b/internal/tools/bash_tool_test.go index 297f9816a..462cd10bd 100644 --- a/internal/tools/bash_tool_test.go +++ b/internal/tools/bash_tool_test.go @@ -789,6 +789,29 @@ func TestBashToolBlocksInteractiveCommandBeforeExecution(t *testing.T) { } } +func TestBashToolBlocksOverDepthShellLauncherBeforeExecution(t *testing.T) { + command := "vim main.go" + // Six launchers exceed sandbox.maxAnalyzerDepth (4) while leaving the + // interactive payload beyond the detector's inspection budget. + for range 6 { + command = "bash -c '" + strings.ReplaceAll(command, "'", `'"'"'`) + "'" + } + + result := NewScopedBashTool(t.TempDir(), nil).Run(context.Background(), map[string]any{ + "command": command, + }) + + if result.Status != StatusError || result.Meta["safety_block"] != "interactive_command" { + t.Fatalf("over-depth shell launcher result = %#v; want pre-execution safety block", result) + } + if result.Meta["exit_code"] != "-1" || result.Meta["safety_cmd"] != "nested shell launcher" { + t.Fatalf("over-depth shell launcher metadata = %#v; want unresolved launcher blocked before execution", result.Meta) + } + if !strings.Contains(result.Output, "cannot be proven non-interactive") { + t.Fatalf("over-depth shell launcher output = %q; want conservative inspection-limit reason", result.Output) + } +} + func TestBashToolBlocksInteractiveCommandThroughSandbox(t *testing.T) { root := t.TempDir() engine := sandbox.NewEngine(sandbox.EngineOptions{ From 66a253be5bbe804104e5e7598c6a32c583c5707e Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Thu, 20 Aug 2026 21:34:18 +0200 Subject: [PATCH 05/12] fix(sandbox): unify launcher network resolution --- internal/sandbox/analyzer.go | 238 +++++++++++------------- internal/sandbox/analyzer_test.go | 18 +- internal/sandbox/engine_test.go | 61 +++++- internal/sandbox/risk.go | 216 ++++++++++----------- internal/sandbox/risk_hardening_test.go | 14 +- internal/sandbox/safe_command.go | 136 +++++++++++--- 6 files changed, 407 insertions(+), 276 deletions(-) diff --git a/internal/sandbox/analyzer.go b/internal/sandbox/analyzer.go index 8a1b4365f..91389ccf3 100644 --- a/internal/sandbox/analyzer.go +++ b/internal/sandbox/analyzer.go @@ -160,26 +160,8 @@ func analyzeInto(script string, result *AnalysisResult, seen map[string]bool, de if !ok || len(call.Args) == 0 { return true } - // Resolve the real program behind wrapper prefixes (sudo, env, nice, ...) - // so `sudo rm -rf`, `env curl …`, and `bash -c 'vim x'` are classified on - // the payload, not the launcher — matching DetectInteractiveCommand. - // GNU env -S/--split-string can consume the complete payload, leaving no - // ordinary effective program, so resolve its argv before that scan. - if fields, ok := literalCallFields(call.Args); ok { - if split := envSplitCommandFields(fields); split.recognized { - if split.executableEnvironmentDependent || split.commandSourceEnvironmentDependent || - fallbackBodyUsesNetwork(split.command, depth+1) { - result.Network = true - } - return true - } - } else if envSplitSourceDynamic(call.Args) { - // The split string comes from an expansion, so its argv — including the - // executable — is unknowable here. effectiveProgram would consume -S with - // its operand and report no executable at all, which reads an - // uninspectable command as a clean one. + if resolveASTCommandNetwork(call.Args, depth).needsNetworkGate() { result.Network = true - return true } prog, rest := effectiveProgram(call.Args) if prog == "" { @@ -189,153 +171,135 @@ func analyzeInto(script string, result *AnalysisResult, seen map[string]bool, de seen[prog] = true result.Programs = append(result.Programs, prog) } - // `sh -c ` runs the payload as a fresh command; recurse into it so - // a program hidden behind a shell launcher is still classified. - if shellPrograms[prog] { - if payloadIndex, found := shellCommandPayloadIndex(prog, wordTexts(rest)); found && payloadIndex < len(rest) { - if !isLiteralWord(rest[payloadIndex]) { - result.Network = true - } else if payload := wordText(rest[payloadIndex]); payload != "" && depth < maxAnalyzerDepth { - analyzeInto(payload, result, seen, depth+1) - } else if payload != "" { - result.Network = true - } - } - } - // PowerShell's Command flag also carries textual source. Analyze it in an - // isolated result because PowerShell syntax that the POSIX parser rejects - // must not make the outer command TooComplex; only fold the network fact. - if prog == "powershell" || prog == "pwsh" { - source := fallbackPowerShellPayload(prog, wordTexts(rest)) - switch { - case source.opaque, powerShellSourceDynamic(source, rest): - // Source the scan cannot read — an undecodable encoded payload, or a - // Command operand built from an expansion — must not be reported as a - // command that makes no network call. - result.Network = true - case source.payload != "": - if classifyCommandText(source.payload, depth+1) { - result.Network = true - } - } - } - // eval runs its remaining arguments as shell source, exactly like the - // fallback path already treats it. Without this branch a parseable - // `eval git push origin main` resolved to the program "eval" and no - // network, while the same text behind an unparseable suffix was caught - // — the fallback and the AST disagreeing about the same launcher. - if prog == "eval" && len(rest) > 0 { - if payload, ok := literalCallFields(rest); ok { - if classifyCommandText(strings.Join(payload, " "), depth+1) { - result.Network = true - } - } else { - // Source assembled from an expansion is source this scan cannot - // read, and eval will run it regardless. - result.Network = true - } - } - if cmdLauncherUsesNetwork(prog, rest, depth) { - result.Network = true - } if _, interactive := interactivePrograms[prog]; interactive && !replSuppressed(prog, rest) { result.Interactive = true } - // BusyBox and strace delegate to a child executable named in their own - // argv. Both resolvers below (busyboxCommandArgs, straceCommandArgs) - // operate on wordTexts, which silently drops any expansion — an - // unresolvable child-program token would otherwise read as a clean, - // unrecognized token rather than as "unknown, so assume the worst." - switch { - case prog == "busybox" && busyboxSourceDynamic(rest): - result.Network = true - case prog == "strace" && straceSourceDynamic(rest): - result.Network = true - case commandUsesNetwork(prog, rest): - result.Network = true - } if destructivePrograms[prog] || (prog == "rm" && hasRecursiveForce(rest)) || (powerShellRemoveItemPrograms[prog] && hasPowerShellRecursiveForce(rest)) || (prog == "find" && hasFindDelete(rest)) { result.Destructive = true } + if shellPrograms[prog] { + if index, found := shellCommandPayloadIndex(prog, wordTexts(rest)); found && index < len(rest) && + isLiteralWord(rest[index]) && depth < maxAnalyzerDepth { + analyzeInto(wordText(rest[index]), result, seen, depth+1) + } + } return true }) } -func commandUsesNetwork(prog string, args []*syntax.Word) bool { - return commandWordsUseNetwork(prog, wordTexts(args)) -} - -func commandWordsUseNetwork(prog string, words []string) bool { - return commandWordsUseNetworkAt(prog, words, 0) +// resolveASTCommandNetwork maps shell AST words onto the same tri-state argv +// resolver used by the unparseable fallback. Literal invocations take exactly +// one path. For dynamic words, the literal portions still establish known +// network programs, while executable or interpreter-source expansions remain +// unresolved and therefore keep the network gate. +func resolveASTCommandNetwork(words []*syntax.Word, depth int) commandResolution { + if depth > maxAnalyzerDepth { + return commandUnresolved + } + if fields, ok := literalCallFields(words); ok { + program, args := effectiveProgram(words) + switch program { + case "cmd", "call", "start", "%comspec%": + return commandNetworkResolution(cmdLauncherUsesNetwork(program, args, depth)) + default: + return resolveCommandArgv(fields, depth) + } + } + if envSplitSourceDynamic(words) { + return commandUnresolved + } + program, args := effectiveProgram(words) + if program == "" { + return commandUnresolved + } + textArgs := wordTexts(args) + resolution := resolveCommandArgv(append([]string{program}, textArgs...), depth) + switch { + case shellPrograms[program]: + index, found := shellCommandPayloadIndex(program, textArgs) + if !found || index >= len(args) { + return resolution + } + if !isLiteralWord(args[index]) { + return commandUnresolved + } + case program == "powershell" || program == "pwsh": + source := fallbackPowerShellPayload(program, textArgs) + if source.opaque || powerShellSourceDynamic(source, args) { + return commandUnresolved + } + case program == "eval": + if _, ok := literalCallFields(args); !ok { + return commandUnresolved + } + case program == "cmd" || program == "call" || program == "start" || program == "%comspec%": + if cmdLauncherUsesNetwork(program, args, depth) { + return commandKnownNetwork + } + case program == "busybox" && busyboxSourceDynamic(args): + return commandUnresolved + case program == "strace" && straceSourceDynamic(args): + return commandUnresolved + } + return resolution } -func commandWordsUseNetworkAt(prog string, words []string, depth int) bool { - // CMD's echo-suppression prefix is not part of the program name: `@curl …` - // runs curl. The fallback tokenizer already strips it (trimCMDEchoPrefix), - // so without the same normalization here the AST path called the program - // "@curl" and found no network, while the identical text behind an - // unparseable suffix was flagged. Stripping it can only ADD the category. +// literalProgramNetworkResolution classifies a program after launcher argv has +// been resolved. Wrapper, delegated-child, and interpreter-source grammars live +// in resolveCommandArgv so AST and fallback paths cannot select different +// children before reaching this program-specific table. +func literalProgramNetworkResolution(prog string, words []string) commandResolution { prog = normalizeProgramToken(trimCMDEchoPrefixToken(prog)) - originalWords := words normalized := make([]string, len(words)) for index := range words { normalized[index] = strings.ToLower(strings.TrimSpace(words[index])) } words = normalized if networkPrograms[prog] || localServerPrograms[prog] { - return true + return commandKnownNetwork } switch prog { case "python", "python2", "python3", "py": - return pythonModuleUsesNetwork(words) + return commandNetworkResolution(pythonModuleUsesNetwork(words)) case "npm": - return packageManagerUsesNetwork(words, map[string]string{ + return commandNetworkResolution(packageManagerUsesNetwork(words, map[string]string{ "run": "run", "exec": "exec", "x": "exec", - }) + })) case "pnpm": - return packageManagerUsesNetwork(words, map[string]string{ + return commandNetworkResolution(packageManagerUsesNetwork(words, map[string]string{ "run": "run", "exec": "exec", "dlx": "exec", - }) + })) case "yarn": - return packageManagerUsesNetwork(words, map[string]string{ + return commandNetworkResolution(packageManagerUsesNetwork(words, map[string]string{ "run": "run", "exec": "exec", "dlx": "exec", - }) + })) case "bun": - return packageManagerUsesNetwork(words, map[string]string{ + return commandNetworkResolution(packageManagerUsesNetwork(words, map[string]string{ "run": "run", "x": "exec", - }) + })) case "npx": - return npxUsesNetwork(words) + return commandNetworkResolution(npxUsesNetwork(words)) case "pip", "pip2", "pip3": - return firstSubcommand(words, nil) == "install" + return commandNetworkResolution(firstSubcommand(words, nil) == "install") case "go": - return firstSubcommand(words, nil) == "get" + return commandNetworkResolution(firstSubcommand(words, nil) == "get") case "git": - return gitUsesNetwork(words) + return commandNetworkResolution(gitUsesNetwork(words)) case "gh": - return ghUsesNetwork(words) - case "busybox": - if command := busyboxCommandArgs(originalWords); len(command) > 0 { - return fallbackBodyUsesNetwork(command, depth+1) - } - case "strace": - if command := straceCommandArgs(originalWords); len(command) > 0 { - return fallbackBodyUsesNetwork(command, depth+1) - } + return commandNetworkResolution(ghUsesNetwork(words)) default: - return false + return commandKnownLocal } - return false } func packageManagerUsesNetwork(words []string, aliases map[string]string) bool { @@ -760,7 +724,7 @@ func cmdBodyUsesNetwork(body []fallbackCommandToken, depth int) bool { return true } if len(body) == 1 && body[0].quoted && strings.ContainsAny(body[0].value, " \t") { - return classifyCommandText(body[0].value, depth+1) + return classifyInterpreterSource(body[0].value, interpreterSourceCMD, depth+1).needsNetworkGate() } return false } @@ -791,26 +755,36 @@ func literalWordIsQuoted(word *syntax.Word) bool { // CMD one-liner is legitimately not POSIX, so the parser failing on it is // expected, not proof of safety), and fails closed when the text cannot be // read at all. -func classifyCommandText(payload string, depth int) bool { +type interpreterSourceLanguage uint8 + +const ( + interpreterSourcePOSIX interpreterSourceLanguage = iota + interpreterSourceCMD + interpreterSourcePowerShell +) + +// classifyInterpreterSource is the single bounded classifier for text a +// launcher will execute. Language-specific syntax that the shared parser cannot +// faithfully interpret is unresolved rather than silently treated as local. +func classifyInterpreterSource(payload string, language interpreterSourceLanguage, depth int) commandResolution { payload = strings.TrimSpace(payload) if payload == "" { - return false + return commandKnownLocal } if depth > maxAnalyzerDepth { - // The text exists and is about to run, but the budget to inspect it is - // gone. An unread payload is not a safe one. - return true + return commandUnresolved + } + if language == interpreterSourcePowerShell && strings.ContainsRune(payload, '`') { + // Backtick escaping and line continuation change PowerShell token + // boundaries but have no equivalent in the POSIX/CMD readers below. + return commandUnresolved } result := AnalysisResult{} analyzeInto(payload, &result, map[string]bool{}, depth) - if result.Network { - return true + if result.Network || matchesUnparseableNetworkAt(payload, depth) { + return commandKnownNetwork } - // The fallback matcher is the reader for text the POSIX parser rejects, and - // it is also a second opinion on text the parser accepted: the two - // tokenizers disagree about CMD quoting and echo prefixes, and only one of - // them needs to see the network program. - return matchesUnparseableNetworkAt(payload, depth) + return commandKnownLocal } func pythonModuleUsesNetwork(words []string) bool { diff --git a/internal/sandbox/analyzer_test.go b/internal/sandbox/analyzer_test.go index 8825e6337..e700fd9fa 100644 --- a/internal/sandbox/analyzer_test.go +++ b/internal/sandbox/analyzer_test.go @@ -128,9 +128,13 @@ func TestAnalyzeCommand(t *testing.T) { {name: "bash dump strings does not execute", script: `bash --dump-strings -c 'curl https://x.test'`, network: false}, {name: "shell dynamic command payload fails closed", script: `sh -c "$ZERO_TEST_SHELL_COMMAND"`, network: true}, {name: "nested shell dynamic command payload fails closed", script: `sh -c 'sh -c "$1"' sh 'curl https://x.test'`, network: true}, + {name: "shell command terminator before dynamic payload fails closed", script: `PAYLOAD='curl https://x.test'; sh -c -- "$PAYLOAD"`, network: true}, + {name: "env split shell terminator selects payload", script: `env -S 'sh -c' -- 'curl https://x.test'`, network: true}, {name: "PowerShell slash command", script: `powershell /Command Invoke-WebRequest https://x.test`, network: true}, {name: "PowerShell abbreviated command", script: `pwsh -co iwr https://x.test`, network: true}, {name: "PowerShell local command", script: `pwsh /Command Get-ChildItem`, network: false}, + {name: "PowerShell backtick command name fails closed", script: "powershell -Command 'Invoke`-WebRequest https://x.test'", network: true}, + {name: "PowerShell backtick continuation fails closed", script: "powershell -Command 'Invoke`\n-WebRequest https://x.test'", network: true}, {name: "PowerShell abbreviated execution policy", script: `powershell -ep RemoteSigned curl https://x.test`, network: true}, {name: "pwsh abbreviated execution policy before command", script: `pwsh -ep Bypass -Command curl https://x.test`, network: true}, {name: "Windows PowerShell bare command", script: `powershell Invoke-WebRequest https://x.test`, network: true}, @@ -165,13 +169,16 @@ func TestAnalyzeCommand(t *testing.T) { {name: "busybox wget applet", script: `busybox wget https://x.test`, network: true}, {name: "busybox shell command", script: `busybox sh -c 'curl https://x.test'`, network: true}, {name: "busybox echo network text", script: `busybox echo wget https://x.test`, network: false}, - {name: "busybox has no option terminator", script: `busybox -- curl https://x.test`, network: false}, - {name: "busybox unknown option", script: `busybox -x curl https://x.test`, network: false}, + {name: "busybox option terminator is unresolved", script: `busybox -- curl https://x.test`, network: true}, + {name: "busybox unknown option is unresolved", script: `busybox -x curl https://x.test`, network: true}, // The applet name comes from a shell expansion this scan cannot read // statically; wordText silently drops it, so the AST resolver must fail // closed here rather than reading the blanked token as a clean unknown. {name: "busybox dynamic applet fails closed", script: `APPLET=curl; busybox "$APPLET" https://x.test`, network: true}, {name: "busybox literal applet stays classified on content", script: `busybox echo "not a program" https://x.test`, network: false}, + {name: "busybox ordinary env wrapper", script: `busybox env curl https://x.test`, network: true}, + {name: "busybox env split wrapper", script: `busybox env -S 'git push origin main'`, network: true}, + {name: "busybox ordinary env local control", script: `busybox env true`, network: false}, {name: "strace curl command", script: `strace -f -o trace.log curl https://x.test`, network: true}, {name: "strace shell command", script: `strace sh -c 'git push origin main'`, network: true}, {name: "strace trace path before curl", script: `strace -P /tmp curl https://x.test`, network: true}, @@ -180,15 +187,20 @@ func TestAnalyzeCommand(t *testing.T) { {name: "strace long trace option", script: `strace --trace network curl https://x.test`, network: true}, {name: "strace tips traces curl", script: `strace --tips curl https://x.test`, network: true}, {name: "strace joined tips traces git", script: `strace --tips=full git push origin main`, network: true}, + {name: "strace abbreviated tips traces curl", script: `strace --tip curl https://x.test`, network: true}, + {name: "strace ambiguous long option is unresolved", script: `strace --ver curl https://x.test`, network: true}, {name: "strace clustered options", script: `strace -fqo trace.log curl https://x.test`, network: true}, {name: "strace output named curl", script: `strace -o curl true`, network: false}, - {name: "strace invalid option", script: `strace --definitely-invalid curl https://x.test`, network: false}, + {name: "strace invalid option is unresolved", script: `strace --definitely-invalid curl https://x.test`, network: true}, // The traced command comes from a shell expansion this scan cannot read // statically; straceSourceDynamic must fail closed the same way // busyboxSourceDynamic does above, rather than reading the blanked token // as a clean unknown command. {name: "strace dynamic child fails closed", script: `APPLET=curl; strace "$APPLET" https://x.test`, network: true}, {name: "strace literal child stays classified on content", script: `strace true "not a program" https://x.test`, network: false}, + {name: "strace ordinary env wrapper", script: `strace env curl https://x.test`, network: true}, + {name: "strace env split wrapper", script: `strace env -S 'git push origin main'`, network: true}, + {name: "strace ordinary env local control", script: `strace env true`, network: false}, {name: "git local commit", script: `git commit -m "local change"`, network: false}, // git's value-taking global options put their value in the NEXT token, so a // generic "first non-dash token" scan reads the value as the subcommand and diff --git a/internal/sandbox/engine_test.go b/internal/sandbox/engine_test.go index 34e999eaf..a6b3fdea5 100644 --- a/internal/sandbox/engine_test.go +++ b/internal/sandbox/engine_test.go @@ -90,6 +90,65 @@ func TestEvaluatePromptsForUnparseableNetworkBehindWrapper(t *testing.T) { } } +// TestEvaluateLauncherResolutionContract pins the tri-state resolver at the +// enforcement boundary: known network and unresolved executable source retain +// the network gate, while explicit local controls do not prompt for egress. +func TestEvaluateLauncherResolutionContract(t *testing.T) { + engine := NewEngine(EngineOptions{Policy: Policy{Mode: ModeEnforce, Network: NetworkDeny}}) + networkCases := []string{ + `PAYLOAD='curl https://evil.test'; sh -c -- "$PAYLOAD"`, + `PAYLOAD='curl https://evil.test'; env -S 'sh -c' -- "$PAYLOAD" && "unterminated`, + "powershell -Command 'Invoke`-WebRequest https://evil.test'", + "powershell -Command 'Invoke`\n-WebRequest https://evil.test'", + `powershell -Command Invoke-WebRequest https://evil.test`, + `strace --tip curl https://evil.test`, + `strace --ver curl https://evil.test`, + `strace --tips curl https://evil.test`, + `strace env curl https://evil.test`, + `strace env -S 'git push origin main'`, + `busybox env curl https://evil.test`, + `busybox env -S 'git push origin main'`, + `busybox -- curl https://evil.test`, + `busybox -x curl https://evil.test`, + `strace --definitely-invalid curl https://evil.test`, + `APPLET=curl; strace "$APPLET" https://evil.test`, + `APPLET=curl; busybox "$APPLET" https://evil.test`, + `strace env curl https://evil.test && "unterminated`, + `busybox env curl https://evil.test && "unterminated`, + } + for _, command := range networkCases { + t.Run("network/"+command, func(t *testing.T) { + decision := engine.Evaluate(context.Background(), Request{ + ToolName: "bash", SideEffect: SideEffectShell, PermissionGranted: true, + Args: map[string]any{"command": command}, + }) + if decision.Action != ActionPrompt || decision.Reason != ReasonNetworkBlocked { + t.Fatalf("Evaluate(%q) = action %q reason %q, want network prompt", command, decision.Action, decision.Reason) + } + }) + } + + localCases := []string{ + `sh -c -- 'printf ok'`, + `env -S 'sh -c' -- 'printf ok' && "unterminated`, + `powershell -Command Get-ChildItem`, + `strace --tips true`, + `strace env true`, + `busybox env true`, + } + for _, command := range localCases { + t.Run("local/"+command, func(t *testing.T) { + decision := engine.Evaluate(context.Background(), Request{ + ToolName: "bash", SideEffect: SideEffectShell, PermissionGranted: true, + Args: map[string]any{"command": command}, + }) + if decision.Reason == ReasonNetworkBlocked || HasRiskCategory(decision.Risk, "network") { + t.Fatalf("Evaluate(%q) = %#v, want proven-local network classification", command, decision) + } + }) + } +} + func TestEngineBashAllowGrantDoesNotBypassNetworkPrompt(t *testing.T) { store, err := NewGrantStore(StoreOptions{ FilePath: filepath.Join(t.TempDir(), "sandbox-grants.json"), @@ -314,8 +373,6 @@ func TestEngineDoesNotPromptForNonNetworkCommandForms(t *testing.T) { `env -S 'printf ok' curl https://evil.test && "unterminated`, `env -S '--argv0 curl printf ok' && "unterminated`, `start MyTitle curl https://evil.test & rem '`, - `busybox -- curl https://evil.test && "unterminated`, - `strace --definitely-invalid curl https://evil.test && "unterminated`, `bash /dev/null -c 'curl https://evil.test' && "unterminated`, `bash -- -c 'curl https://evil.test' && "unterminated`, `bash -Zc 'curl https://evil.test' && "unterminated`, diff --git a/internal/sandbox/risk.go b/internal/sandbox/risk.go index fe0e9165d..aaa960e4e 100644 --- a/internal/sandbox/risk.go +++ b/internal/sandbox/risk.go @@ -33,12 +33,6 @@ var ( // A purely local pipe into a shell (e.g. `printf … | sh`, `cat ./s | bash`) // is NOT a piped installer and must not be flagged. pipedInstallerPattern = regexp.MustCompile(`(?i)\b(curl|wget|fetch|aria2c)\b[^|]*\|\s*(ba|z|k|da)?sh\b`) - // unparseableNetworkPattern is used only after the shell parser fails. At - // that point the command is already marked too complex, so this intentionally - // favors catching obvious network programs over proving exact shell syntax. - // Git needs token-aware handling below: a regex cannot reliably distinguish - // option values and executable path components from subcommands. - unparseableNetworkPattern = regexp.MustCompile(`(?i)^(?:(curl|wget|fetch|aria2c|ssh|scp|sftp|rsync|nc|ncat|netcat|telnet|ftp|npx|http-server|vite|next|nuxt|astro)(\s|$)|(npm|pnpm|yarn|bun|pip|pip2|pip3)\s+(install|add|publish|login|start|serve|dev|preview|run\s+(start|serve|dev|preview)|exec|x|dlx)\b|go\s+get\b|python(2|3)?\s+-m\s+(http\.server|pip\s+install)\b|gh\s+(api|repo\s+clone|release\s+download)\b)`) // destructiveExtraPatterns hold high-severity patterns that the legacy // destructiveCommandPattern does not already cover. Folded in from the // blueprint safe_bash.go without duplicating existing matches. @@ -79,6 +73,28 @@ func matchesDestructive(command string) bool { // exactly the deeply-nested launcher chains this path exists to fail closed on. const maxUnparseableShellDepth = maxAnalyzerDepth +// commandResolution is the shared security result for executable argv and +// interpreter source. Unresolved input is distinct from a proven-local command +// and retains the network gate at every caller. +type commandResolution uint8 + +const ( + commandKnownLocal commandResolution = iota + commandKnownNetwork + commandUnresolved +) + +func (resolution commandResolution) needsNetworkGate() bool { + return resolution != commandKnownLocal +} + +func commandNetworkResolution(network bool) commandResolution { + if network { + return commandKnownNetwork + } + return commandKnownLocal +} + func matchesUnparseableNetwork(command string) bool { return matchesUnparseableNetworkAt(command, 0) } @@ -99,7 +115,7 @@ func matchesUnparseableNetwork(command string) bool { func matchesUnparseableNetworkAt(command string, depth int) bool { if depth < maxUnparseableShellDepth { for _, payload := range fallbackCMDForFCommands(command) { - if classifyCommandText(payload, depth+1) { + if classifyInterpreterSource(payload, interpreterSourceCMD, depth+1).needsNetworkGate() { return true } } @@ -152,97 +168,107 @@ func fallbackCommandBodies(tokens []string) [][]string { // fallbackBodyUsesNetwork classifies one resolved command body, recursing into // the payloads of launchers that run command text of their own. func fallbackBodyUsesNetwork(body []string, depth int) bool { + return resolveCommandArgv(body, depth).needsNetworkGate() +} + +// resolveCommandArgv is the bounded, wrapper-aware resolver shared by the AST +// and fallback paths. It distinguishes a proven-local command from network +// access and from argv/source that the supported launcher grammars cannot +// resolve. Only a proven-local result may omit the network gate. +func resolveCommandArgv(body []string, depth int) commandResolution { if len(body) == 0 { - return false + return commandKnownLocal } - program, args := executableTokenBase(body[0]), body[1:] - for program == "exec" { - body = fallbackExecCommandArgs(args) - if len(body) == 0 { - return false + if depth > maxAnalyzerDepth { + return commandUnresolved + } + if split := envSplitCommandFields(body); split.recognized { + if split.executableEnvironmentDependent || split.commandSourceEnvironmentDependent { + return commandUnresolved } - program, args = executableTokenBase(body[0]), body[1:] + if len(split.command) == 0 { + return commandKnownLocal + } + return resolveCommandArgv(split.command, depth+1) + } + if resolved := commandBodyFields(body); len(resolved) > 0 { + body = resolved + } else { + return commandKnownLocal } + program, args := executableTokenBase(body[0]), body[1:] if program == "%comspec%" { program = "cmd" } if fallbackTokenLooksDynamic(body[0]) { - return true + return commandUnresolved } - if networkPrograms[program] || localServerPrograms[program] { - return true - } - if program == "git" && matchesUnparseableGitNetwork(args) { - return true - } - if program == "busybox" { - if command := busyboxCommandArgs(args); len(command) > 0 { - if fallbackTokenLooksDynamic(command[0]) { - return true - } - if depth >= maxUnparseableShellDepth || fallbackBodyUsesNetwork(command, depth+1) { - return true - } + switch program { + case "busybox": + command, status := busyboxDelegatedCommand(args) + if status != commandKnownLocal { + return status } - } - if program == "strace" { - if command := straceCommandArgs(args); len(command) > 0 { - if fallbackTokenLooksDynamic(command[0]) { - return true - } - if depth >= maxUnparseableShellDepth || fallbackBodyUsesNetwork(command, depth+1) { - return true - } + if len(command) == 0 { + return commandKnownLocal } - } - if unparseableNetworkPattern.MatchString(strings.Join(append([]string{program}, args...), " ")) { - return true - } - // eval executes its remaining arguments as shell source. Recurse into that - // source just as we do for `sh -c`; otherwise quoting the same curl/git - // invocation behind eval would hide it from this fail-closed path. - if program == "eval" && len(args) > 0 { - if classifyCommandText(strings.Join(args, " "), depth+1) { - return true + return resolveCommandArgv(command, depth+1) + case "strace": + command, status := straceDelegatedCommand(args) + if status != commandKnownLocal { + return status } - } - if program == "env" { - if split := envSplitCommand(args); split.recognized { - return split.executableEnvironmentDependent || split.commandSourceEnvironmentDependent || - (len(split.command) > 0 && (depth >= maxUnparseableShellDepth || fallbackBodyUsesNetwork(split.command, depth+1))) + if len(command) == 0 { + return commandKnownLocal + } + return resolveCommandArgv(command, depth+1) + case "eval": + if len(args) == 0 { + return commandKnownLocal + } + return classifyInterpreterSource(strings.Join(args, " "), interpreterSourcePOSIX, depth+1) + case "call": + if len(args) == 0 { + return commandKnownLocal } + if fallbackTokenLooksDynamic(args[0]) { + return commandUnresolved + } + return resolveCommandArgv(args, depth+1) } - // `sh -c ` runs the payload as a fresh command. The fallback - // tokenizer keeps a quoted payload as ONE token, so the network program - // inside it is not a token of this segment at all — recurse the way - // analyzeInto does on the parseable path. if shellPrograms[program] { - if payloadIndex, found := shellCommandPayloadIndex(program, args); found && payloadIndex < len(args) { - if payload := args[payloadIndex]; payload != "" { - if fallbackTokenLooksDynamic(payload) || classifyCommandText(payload, depth+1) { - return true - } - } + index, found := shellCommandPayloadIndex(program, args) + if !found || index >= len(args) { + return commandKnownLocal } + payload := args[index] + if fallbackTokenLooksDynamic(payload) { + return commandUnresolved + } + return classifyInterpreterSource(payload, interpreterSourcePOSIX, depth+1) } - // PowerShell source that exists but cannot be read is not evidence that the - // command is local. Fail closed on it here, before payload extraction drops - // it as empty. if program == "powershell" || program == "pwsh" { - if fallbackPowerShellPayload(program, args).opaque { - return true + source := fallbackPowerShellPayload(program, args) + switch { + case source.opaque: + return commandUnresolved + case source.payload == "": + return commandKnownLocal + default: + return classifyInterpreterSource(source.payload, interpreterSourcePowerShell, depth+1) } } - // Windows command interpreters carry command text after their command - // flag. That payload is valid shell input even when the POSIX parser that - // sent us here cannot parse it (for example, `cmd /c curl ... & rem '`). if payload := fallbackCommandInterpreterPayload(program, args); payload != "" { - if classifyCommandText(payload, depth+1) || - fallbackPayloadUsesNetwork(strings.Join(fallbackCommandInterpreterArgs(program, args), " "), depth+1) { - return true + resolution := classifyInterpreterSource(payload, interpreterSourceCMD, depth+1) + if resolution.needsNetworkGate() { + return resolution + } + if fallbackPayloadUsesNetwork(strings.Join(fallbackCommandInterpreterArgs(program, args), " "), depth+1) { + return commandKnownNetwork } + return commandKnownLocal } - return false + return literalProgramNetworkResolution(program, args) } // fallbackTokenLooksDynamic reports whether a token selected as executable @@ -642,17 +668,6 @@ func isRedirectToken(word string) bool { return strings.HasPrefix(word, ">") || strings.HasPrefix(word, "<") } -// matchesUnparseableGitNetwork reports whether git's arguments (everything after -// the executable) name a subcommand that talks to a remote. -// -// It defers to gitUsesNetwork rather than reading the option list a second time. -// The two paths disagreeing is not a theoretical risk: while each kept its own -// terminal-option rule, `git -h push` was network on one path and local on the -// other, and every future option would have had to be added to both. -func matchesUnparseableGitNetwork(args []string) bool { - return gitUsesNetwork(args) -} - // shellCommandPayloadIndex returns the one argv element a shell launcher will // execute for -c. It parses only the leading option region: a script operand or // `--` makes later `-c` text positional, and an invalid option cluster is not @@ -732,7 +747,14 @@ func shellCommandPayloadIndex(program string, args []string) (int, bool) { if noExecute || dumpStrings { return 0, false } - return index + 1, true + sourceIndex := index + 1 + if sourceIndex < len(args) && args[sourceIndex] == "--" { + sourceIndex++ + } + if sourceIndex >= len(args) { + return 0, false + } + return sourceIndex, true } } return 0, false @@ -925,26 +947,6 @@ func powerShellValuelessOption(program, flag string) bool { } } -func fallbackExecCommandArgs(args []string) []string { - for index := 0; index < len(args); index++ { - if args[index] == "--" { - return args[index+1:] - } - if args[index] == "-a" || args[index] == "--argv0" { - if index+1 >= len(args) { - return nil - } - index++ - continue - } - if strings.HasPrefix(args[index], "-") { - continue - } - return args[index:] - } - return nil -} - func normalizeCMDToken(token string) string { var out strings.Builder for index := 0; index < len(token); index++ { diff --git a/internal/sandbox/risk_hardening_test.go b/internal/sandbox/risk_hardening_test.go index 1c3aecefd..6df429e81 100644 --- a/internal/sandbox/risk_hardening_test.go +++ b/internal/sandbox/risk_hardening_test.go @@ -611,6 +611,17 @@ func TestClassifyUnparseableCommandBearingWrapperValuesFailClosed(t *testing.T) `strace --trace network curl https://evil.test && "unterminated`, `strace --tips curl https://evil.test && "unterminated`, `strace -fqo trace.log curl https://evil.test && "unterminated`, + `strace --tip curl https://evil.test && "unterminated`, + `strace --ver curl https://evil.test && "unterminated`, + `strace --definitely-invalid curl https://evil.test && "unterminated`, + `strace env curl https://evil.test && "unterminated`, + `strace env -S 'git push origin main' && "unterminated`, + `busybox env curl https://evil.test && "unterminated`, + `busybox env -S 'git push origin main' && "unterminated`, + `busybox -- curl https://evil.test && "unterminated`, + `busybox -x curl https://evil.test && "unterminated`, + `env -S 'sh -c' -- 'curl https://evil.test' && "unterminated`, + "powershell -Command 'Invoke`-WebRequest https://evil.test' & rem '", `pwsh -cwa Invoke-WebRequest https://evil.test & rem '`, `exec -a harmless curl https://evil.test && "unterminated`, `exec --argv0 harmless git push origin main && "unterminated`, @@ -744,9 +755,6 @@ func TestClassifyUnparseableShellSyntaxTextStaysNonNetwork(t *testing.T) { `echo "x & for /f %i in ('curl https://evil.test') do echo %i" & rem '`, `echo x ^& for /f %i in ('curl https://evil.test') do echo %i & rem '`, "for /f \"delims=usebackq\" %i in (`curl https://evil.test`) do echo %i & rem '", - `busybox -- curl https://evil.test && "unterminated`, - `busybox -x curl https://evil.test && "unterminated`, - `strace --definitely-invalid curl https://evil.test && "unterminated`, `env -S '--argv0 curl printf ok' && "unterminated`, // An abbreviated long option that isn't --split-string must not be // misread as one; env's own resolution of these flags is unaffected. diff --git a/internal/sandbox/safe_command.go b/internal/sandbox/safe_command.go index bb9eb2cba..010b9910e 100644 --- a/internal/sandbox/safe_command.go +++ b/internal/sandbox/safe_command.go @@ -323,6 +323,7 @@ var wrapperValueOptionsByProg = map[string]map[string]bool{ "ionice": {"-c": true, "--class": true, "-n": true, "--classdata": true, "-p": true, "--pid": true}, "stdbuf": {"-i": true, "--input": true, "-o": true, "--output": true, "-e": true, "--error": true}, "xargs": {"-a": true, "--arg-file": true, "-d": true, "--delimiter": true, "-E": true, "-I": true, "--replace": true, "-L": true, "--max-lines": true, "-n": true, "--max-args": true, "-P": true, "--max-procs": true, "-s": true, "--max-chars": true}, + "exec": {"-a": true, "--argv0": true}, } // wrapperConsumesValue reports whether option is a value-consuming flag of the @@ -761,18 +762,21 @@ func validEnvSplitVariableName(name string) bool { return true } -func busyboxCommandArgs(args []string) []string { +func busyboxDelegatedCommand(args []string) ([]string, commandResolution) { if len(args) == 0 { - return nil + return nil, commandKnownLocal } switch args[0] { case "--help", "--list", "--list-full", "--install", "--show": - return nil + return nil, commandKnownLocal } if strings.HasPrefix(args[0], "-") { - return nil + return nil, commandUnresolved } - return args + if fallbackTokenLooksDynamic(args[0]) { + return nil, commandUnresolved + } + return args, commandKnownLocal } var ( @@ -805,45 +809,75 @@ var ( } ) -func straceCommandArgs(args []string) []string { - index, ok := straceChildIndex(args) - if !ok { - return nil +type straceLongOptionKind uint8 + +const ( + straceLongOptionInvalid straceLongOptionKind = iota + straceLongOptionRequired + straceLongOptionOptional + straceLongOptionValueless + straceLongOptionTerminal +) + +func straceDelegatedCommand(args []string) ([]string, commandResolution) { + index, status := straceChildResolution(args) + if status != commandKnownLocal || index < 0 || index >= len(args) { + return nil, status } - return args[index:] + if fallbackTokenLooksDynamic(args[index]) { + return nil, commandUnresolved + } + return args[index:], commandKnownLocal } -// straceChildIndex walks strace's own argv exactly as straceCommandArgs does, -// returning the index of the traced child command instead of the resolved -// slice. straceSourceDynamic shares this walk so its literalness check stays -// aligned with straceCommandArgs's option grammar by construction, instead of -// via a second hand-maintained copy that could silently drift from it. +// straceChildIndex retains the index surface used by AST literalness checks. +// Unresolved option grammar is deliberately not exposed as "no child"; the +// shared argv resolver consumes straceChildResolution directly and keeps the +// network gate in that state. func straceChildIndex(args []string) (int, bool) { + index, status := straceChildResolution(args) + return index, status == commandKnownLocal && index >= 0 +} + +func straceChildResolution(args []string) (int, commandResolution) { for index := 0; index < len(args); index++ { arg := args[index] switch arg { - case "--help", "--version": - return 0, false case "--": - return index + 1, true + if index+1 >= len(args) { + return -1, commandKnownLocal + } + return index + 1, commandKnownLocal case "-": - return index, true + return index, commandKnownLocal } if strings.HasPrefix(arg, "--") { name, hasValue := arg, false if equals := strings.IndexByte(arg, '='); equals >= 0 { name, hasValue = arg[:equals], true } - switch { - case straceRequiredLongOptions[name]: + kind, ok := resolveStraceLongOption(name) + if !ok { + return -1, commandUnresolved + } + switch kind { + case straceLongOptionTerminal: + return -1, commandKnownLocal + case straceLongOptionRequired: if !hasValue { index++ + if index >= len(args) { + return -1, commandUnresolved + } + } + case straceLongOptionOptional: + // GNU getopt_long accepts optional values only as --option=value. + case straceLongOptionValueless: + if hasValue { + return -1, commandUnresolved } - case straceOptionalLongOptions[name]: - // getopt_long accepts optional values only in --option=value form. - case straceValuelessLongOptions[name] && !hasValue: default: - return 0, false + return -1, commandUnresolved } continue } @@ -852,15 +886,18 @@ func straceChildIndex(args []string) (int, bool) { for clusterIndex, option := range cluster { switch { case option == 'h' || option == 'V': - return 0, false + return -1, commandKnownLocal case strings.ContainsRune("abeEIoOpPsSuUX", option): if clusterIndex+1 == len(cluster) { index++ + if index >= len(args) { + return -1, commandUnresolved + } } clusterIndex = len(cluster) case strings.ContainsRune("AcCdDfFiknNqrtTvwxyYzZ", option): default: - return 0, false + return -1, commandUnresolved } if clusterIndex == len(cluster) { break @@ -868,9 +905,50 @@ func straceChildIndex(args []string) (int, bool) { } continue } - return index, true + return index, commandKnownLocal + } + return -1, commandKnownLocal +} + +func resolveStraceLongOption(name string) (straceLongOptionKind, bool) { + if kind := exactStraceLongOption(name); kind != straceLongOptionInvalid { + return kind, true + } + matchedKind := straceLongOptionInvalid + matches := 0 + visit := func(options map[string]bool, kind straceLongOptionKind) { + for option := range options { + if strings.HasPrefix(option, name) { + matchedKind = kind + matches++ + } + } + } + visit(straceRequiredLongOptions, straceLongOptionRequired) + visit(straceOptionalLongOptions, straceLongOptionOptional) + visit(straceValuelessLongOptions, straceLongOptionValueless) + for _, option := range []string{"--help", "--version"} { + if strings.HasPrefix(option, name) { + matchedKind = straceLongOptionTerminal + matches++ + } + } + return matchedKind, matches == 1 +} + +func exactStraceLongOption(name string) straceLongOptionKind { + switch { + case straceRequiredLongOptions[name]: + return straceLongOptionRequired + case straceOptionalLongOptions[name]: + return straceLongOptionOptional + case straceValuelessLongOptions[name]: + return straceLongOptionValueless + case name == "--help" || name == "--version": + return straceLongOptionTerminal + default: + return straceLongOptionInvalid } - return 0, false } // isNumericToken reports whether a token is purely digits (e.g. the duration From 68f606cde68402b8f8afe888d389169a07fbd6cf Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Fri, 21 Aug 2026 15:25:08 +0200 Subject: [PATCH 06/12] fix(sandbox): classify sh login command clusters Amp-Thread-ID: https://ampcode.com/threads/T-01a0246b-e61a-70e8-aa71-24f1ea7804c8 Co-authored-by: Amp --- .../sandbox/combined_shell_options_test.go | 50 +++++++++++++++++++ internal/sandbox/risk.go | 2 +- 2 files changed, 51 insertions(+), 1 deletion(-) create mode 100644 internal/sandbox/combined_shell_options_test.go diff --git a/internal/sandbox/combined_shell_options_test.go b/internal/sandbox/combined_shell_options_test.go new file mode 100644 index 000000000..f209647bb --- /dev/null +++ b/internal/sandbox/combined_shell_options_test.go @@ -0,0 +1,50 @@ +package sandbox + +import ( + "context" + "testing" +) + +// Shells accept command mode inside a short-option cluster. In particular, +// `sh -lc` is a common login-shell spelling; treating its `-l` as invalid made +// the parser stop before the command payload and let network use bypass the +// network-deny prompt. +func TestEvaluateCombinedShellOptionsPreservesNetworkGate(t *testing.T) { + engine := NewEngine(EngineOptions{Policy: Policy{Mode: ModeEnforce, Network: NetworkDeny}}) + networkCases := []string{ + `bash -lc "curl https://evil.test"`, + `bash -ic "curl https://evil.test"`, + `bash -xc "curl https://evil.test"`, + `bash -ec "curl https://evil.test"`, + `sh -lc "curl https://evil.test"`, + `zsh -lc "curl https://evil.test"`, + `bash -lc "git push origin main"`, + } + for _, command := range networkCases { + t.Run(command, func(t *testing.T) { + decision := engine.Evaluate(context.Background(), Request{ + ToolName: "bash", SideEffect: SideEffectShell, PermissionGranted: true, + Args: map[string]any{"command": command}, + }) + if decision.Action != ActionPrompt || decision.Reason != ReasonNetworkBlocked { + t.Fatalf("Evaluate(%q) = action %q reason %q risk %v, want network prompt", command, decision.Action, decision.Reason, decision.Risk.Categories) + } + }) + } + + for _, command := range []string{ + `bash -lc "printf ok"`, + `sh -lc "printf ok"`, + `zsh -lc "printf ok"`, + } { + t.Run("local/"+command, func(t *testing.T) { + decision := engine.Evaluate(context.Background(), Request{ + ToolName: "bash", SideEffect: SideEffectShell, PermissionGranted: true, + Args: map[string]any{"command": command}, + }) + if decision.Reason == ReasonNetworkBlocked || HasRiskCategory(decision.Risk, "network") { + t.Fatalf("Evaluate(%q) = %#v, want proven-local network classification", command, decision) + } + }) + } +} diff --git a/internal/sandbox/risk.go b/internal/sandbox/risk.go index aaa960e4e..79ec5c95b 100644 --- a/internal/sandbox/risk.go +++ b/internal/sandbox/risk.go @@ -717,7 +717,7 @@ func shellCommandPayloadIndex(program string, args []string) (int, bool) { case "bash": valueOptions += "O" case "dash", "sh": - validOptions = "abCefnuvxIimspc" + validOptions = "abCefnuvxIimslpc" default: // ksh/zsh share the common invocation flags used here, including -l. validOptions = "abefhkmnptuvxBCilrsc" From 227e99f3f9010ffa2dd7a6657d0dec840f269449 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sun, 23 Aug 2026 16:47:01 +0200 Subject: [PATCH 07/12] fix(sandbox): fail closed on command shapes the classifier cannot read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three unresolved review threads, each the same shape: a reading the scan could not perform was reported as "no network" rather than as unresolved. Dynamic git arguments. wordText drops an expansion, so `git $VERB origin main` reconstructed as `git origin main` — an unrecognized, therefore local, subcommand — while the shell ran `git push`. gitSelectionDynamic now fails closed when an unreadable word could choose the subcommand, or could be `git archive`'s --remote, the one option that turns a local tree export into a request to another host. Words BEFORE the subcommand count too: an empty expansion or one that word-splits shifts which token git reads as the subcommand, even as the value of a global option that normally consumes it. Both readers get the same rule — the AST path supplies isLiteralWord, the fallback supplies fallbackTokenLooksDynamic — so neither can classify a shape the other refuses. Past a non-archive subcommand nothing can change git's answer, so `git commit -m "$MESSAGE"` keeps its quiet path. `git -C "$DIR" status` does not: the dynamic option value is not proven, so it now prompts. That is a real cost and the conservative direction is deliberate, but a maintainer who wants it narrowed can exempt a value that cannot word-split. CMD FOR loops. cmdForPayload selected the first `do`, which inside the IN set is an ordinary set element: `for %i in ( do x ) do curl …` resolved the loop body from `x` and never reached the curl. DO now counts only at parenthesis depth zero, and an unbalanced set — which CMD itself rejects — yields no body rather than a guessed one. The joined spelling `(do` happened to land correctly before, so the spaced form is the regression that actually failed; both are pinned. CMD quoting. The fallback tokenizer treats `'` as a quote, but CMD has no single-quote quoting at all: in `echo ' & curl https://evil.test` the POSIX reading buries `& curl` in echo's argument while cmd.exe runs curl. Text handed to a CMD reader is now also tokenized under CMD's rules, where `"` is the only quote and a backtick is ordinary text. That second tokenization is gated on the disagreement that matters — a single-quoted region containing `&` or `|` — not merely on the presence of a quote. Re-cutting every quoted POSIX construct produced segments that mean nothing under CMD and classified `env -S 'printf ok' …` as network; `;` is excluded because CMD delimits arguments with it rather than starting a command. Tests: TestEngineFailsClosedOnUnreadableCommandShapes runs all three through the engine, since the property that matters is the prompt the operator sees, with the readable forms alongside as the no-noise half. Seven of its eight network rows were verified to fail against the unfixed classifier. gitSelectionDynamic, cmdForPayload, and posixSingleQuoteHidesCMDSeparator also get direct unit coverage so a regression names the layer that broke. Validation: go build ./..., go vet ./... (also GOOS=linux and GOOS=darwin), gofmt clean, deadcode unchanged from baseline, go test ./internal/... (internal/cli provider-config failures are pre-existing on this branch and unrelated). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JgWC2FnDp5Jjdvc6cqEfEQ --- internal/sandbox/analyzer.go | 45 ++++++++ internal/sandbox/engine_test.go | 72 +++++++++++++ internal/sandbox/risk.go | 137 +++++++++++++++++++++--- internal/sandbox/risk_hardening_test.go | 97 +++++++++++++++++ 4 files changed, 339 insertions(+), 12 deletions(-) diff --git a/internal/sandbox/analyzer.go b/internal/sandbox/analyzer.go index 91389ccf3..ea90efd64 100644 --- a/internal/sandbox/analyzer.go +++ b/internal/sandbox/analyzer.go @@ -243,10 +243,55 @@ func resolveASTCommandNetwork(words []*syntax.Word, depth int) commandResolution return commandUnresolved case program == "strace" && straceSourceDynamic(args): return commandUnresolved + case program == "git" && resolution == commandKnownLocal: + if gitSelectionDynamic(textArgs, func(index int) bool { return !isLiteralWord(args[index]) }) { + return commandUnresolved + } } return resolution } +// gitSelectionDynamic reports whether an expansion this scan cannot read could +// choose what git actually does: the SUBCOMMAND itself, or `git archive`'s +// --remote, the one option that turns a local tree export into a request to +// another host. +// +// wordText drops an expansion, so the reconstructed argv shows a subcommand git +// may never run: `git $VERB origin main` reads as `git origin main` — an +// unrecognized, therefore local, subcommand — while the shell runs `git push`. +// A dynamic word BEFORE the subcommand counts too, because it can shift which +// token git reads as the subcommand: an empty expansion or one that word-splits +// changes the position, even as the value of a global option that normally +// consumes it. +// +// Past the subcommand nothing else can change the answer, so ordinary forms like +// `git commit -m "$MESSAGE"` stay proven-local and keep their quiet path. +// +// dynamic reports whether the word at an index is unreadable. The AST path +// supplies isLiteralWord and the fallback supplies fallbackTokenLooksDynamic, so +// neither can classify a shape the other fails closed on. +func gitSelectionDynamic(words []string, dynamic func(index int) bool) bool { + invocation := parseGitInvocation(words) + limit := len(words) + if invocation.kind == gitCommandSubcommand { + limit = min(invocation.subcommandIndex+1, len(words)) + } + for index := range limit { + if dynamic(index) { + return true + } + } + if invocation.kind != gitCommandSubcommand || invocation.subcommand != "archive" { + return false + } + for index := invocation.subcommandIndex + 1; index < len(words); index++ { + if dynamic(index) { + return true + } + } + return false +} + // literalProgramNetworkResolution classifies a program after launcher argv has // been resolved. Wrapper, delegated-child, and interpreter-source grammars live // in resolveCommandArgv so AST and fallback paths cannot select different diff --git a/internal/sandbox/engine_test.go b/internal/sandbox/engine_test.go index a6b3fdea5..7ec215008 100644 --- a/internal/sandbox/engine_test.go +++ b/internal/sandbox/engine_test.go @@ -149,6 +149,78 @@ func TestEvaluateLauncherResolutionContract(t *testing.T) { } } +// Three shapes where a reading the classifier could not perform was being +// reported as "no network" rather than as unresolved. Each ran through the +// engine, because the property that matters is the prompt the operator sees. +func TestEngineFailsClosedOnUnreadableCommandShapes(t *testing.T) { + engine := NewEngine(EngineOptions{Policy: Policy{Mode: ModeEnforce, Network: NetworkDeny}}) + networkCases := []struct { + name string + command string + }{ + // The expansion chooses git's subcommand. The reconstructed argv shows + // `git origin main`, an unknown local subcommand, while the shell runs + // whatever VERB holds — `push` included. + {name: "git subcommand from an expansion", command: `VERB=push; git $VERB origin main`}, + {name: "git subcommand from a quoted expansion", command: `VERB=push; git "$VERB" origin main`}, + // Same reading through the unparseable fallback rather than the AST. + {name: "git subcommand from an expansion, fallback", command: `git $VERB origin main && "unterminated`}, + // archive is local until --remote makes it a fetch from another host, so + // an unreadable option word past the subcommand counts for archive alone. + {name: "git archive options from an expansion", command: `OPTS=--remote=origin; git archive $OPTS HEAD`}, + // `do` inside the IN set is a set element, not the loop keyword. Selecting + // the first one resolved the loop body from `x` and never reached the curl + // that runs. The spaced spelling is the one that was actually wrong: with + // `(do` joined, the token is not `do` and the old scan landed correctly by + // accident, which is why the joined form is kept alongside it rather than + // instead of it. + {name: "cmd FOR with do inside the set", command: `for %i in (do x) do curl https://evil.test`}, + {name: "cmd FOR with a spaced set", command: `for %i in ( do x ) do curl https://evil.test`}, + // CMD has no single-quote quoting: the quote is literal text, `&` is a + // separator, and curl runs. POSIX tokenization hid it inside echo's + // argument. + {name: "cmd separator hidden by a POSIX quote", command: `echo ' & curl https://evil.test`}, + {name: "cmd separator hidden by a POSIX quote, piped", command: `echo ' | curl https://evil.test`}, + } + for _, testCase := range networkCases { + t.Run("network/"+testCase.name, func(t *testing.T) { + decision := engine.Evaluate(context.Background(), Request{ + ToolName: "bash", SideEffect: SideEffectShell, PermissionGranted: true, + Args: map[string]any{"command": testCase.command}, + }) + if decision.Action != ActionPrompt || decision.Reason != ReasonNetworkBlocked { + t.Fatalf("Evaluate(%q) = action %q reason %q, want network prompt", testCase.command, decision.Action, decision.Reason) + } + }) + } + + // The fail-closed rule must not swallow the ordinary readable forms: a + // literal local subcommand stays quiet even with expansions in its operands, + // a local archive stays local, and a FOR loop over a network-looking set + // still resolves from its body. + localCases := []struct { + name string + command string + }{ + {name: "literal local subcommand with a dynamic operand", command: `git commit -m "$MESSAGE"`}, + {name: "local archive", command: `git archive -o out.tar HEAD`}, + {name: "archive with a dynamic operand past --", command: `git archive -o out.tar -- HEAD`}, + {name: "for loop body is echo", command: `for %i in (curl) do echo %i`}, + {name: "posix quote hiding no separator", command: `echo 'plain text'`}, + } + for _, testCase := range localCases { + t.Run("local/"+testCase.name, func(t *testing.T) { + decision := engine.Evaluate(context.Background(), Request{ + ToolName: "bash", SideEffect: SideEffectShell, PermissionGranted: true, + Args: map[string]any{"command": testCase.command}, + }) + if decision.Reason == ReasonNetworkBlocked || HasRiskCategory(decision.Risk, "network") { + t.Fatalf("Evaluate(%q) = %#v, want proven-local network classification", testCase.command, decision) + } + }) + } +} + func TestEngineBashAllowGrantDoesNotBypassNetworkPrompt(t *testing.T) { store, err := NewGrantStore(StoreOptions{ FilePath: filepath.Join(t.TempDir(), "sandbox-grants.json"), diff --git a/internal/sandbox/risk.go b/internal/sandbox/risk.go index 79ec5c95b..4307de6e3 100644 --- a/internal/sandbox/risk.go +++ b/internal/sandbox/risk.go @@ -146,6 +146,18 @@ func matchesUnparseableNetworkAt(command string, depth int) bool { } } } + // The segments above were cut under POSIX quoting. CMD honors only `"`, so + // re-cut the same text under its rules before handing it to the CMD readers; + // otherwise a quote form CMD ignores hides a separator it acts on. + if posixSingleQuoteHidesCMDSeparator(command) { + for _, tokenInfo := range fallbackCMDCommandTokenInfo(command) { + for _, body := range cmdCommandBodyTokenInfoCandidates(tokenInfo) { + if cmdBodyUsesNetwork(body, depth) { + return true + } + } + } + } return false } @@ -268,7 +280,14 @@ func resolveCommandArgv(body []string, depth int) commandResolution { } return commandKnownLocal } - return literalProgramNetworkResolution(program, args) + resolution := literalProgramNetworkResolution(program, args) + if program == "git" && resolution == commandKnownLocal && + gitSelectionDynamic(args, func(index int) bool { return fallbackTokenLooksDynamic(args[index]) }) { + // The fallback never expands these values, so a subcommand it cannot read + // is not a subcommand it has proven local. See gitSelectionDynamic. + return commandUnresolved + } + return resolution } // fallbackTokenLooksDynamic reports whether a token selected as executable @@ -319,16 +338,25 @@ func validCMDVariableName(name string) bool { return true } +// fallbackPayloadUsesNetwork classifies CMD payload text, so it reads the text +// under CMD's quoting as well as the shared POSIX tokenization — a `'` CMD +// treats as ordinary text must not hide the separator that follows it. func fallbackPayloadUsesNetwork(payload string, depth int) bool { - for _, tokenInfo := range fallbackCommandTokenInfo(payload) { - if len(tokenInfo) > 0 && strings.EqualFold(tokenInfo[0].value, "start") { - if body := cmdStartPayloadTokenInfo(tokenInfo[1:]); len(body) > 0 && fallbackBodyUsesNetwork(fallbackTokenValues(body), depth) { - return true + tokenizations := [][][]fallbackCommandToken{fallbackCommandTokenInfo(payload)} + if posixSingleQuoteHidesCMDSeparator(payload) { + tokenizations = append(tokenizations, fallbackCMDCommandTokenInfo(payload)) + } + for _, tokenInfos := range tokenizations { + for _, tokenInfo := range tokenInfos { + if len(tokenInfo) > 0 && strings.EqualFold(tokenInfo[0].value, "start") { + if body := cmdStartPayloadTokenInfo(tokenInfo[1:]); len(body) > 0 && fallbackBodyUsesNetwork(fallbackTokenValues(body), depth) { + return true + } } - } - for _, body := range cmdCommandBodyTokenInfoCandidates(tokenInfo) { - if fallbackBodyUsesNetwork(fallbackTokenValues(body), depth) { - return true + for _, body := range cmdCommandBodyTokenInfoCandidates(tokenInfo) { + if fallbackBodyUsesNetwork(fallbackTokenValues(body), depth) { + return true + } } } } @@ -574,15 +602,45 @@ func cmdConditionPayload(fields []string) []string { // cmdForPayload returns the command after DO, which is the only part of a FOR // loop that executes. `for %i in (curl) do echo %i` must not resolve to curl. +// +// DO counts only OUTSIDE the IN (...) set. `do` is an ordinary word inside it, +// and CMD accepts it there: in `for %i in (do x) do curl https://evil.test` the +// first `do` is a set ELEMENT, so selecting it resolved the classifier from `x` +// and never reached the curl that actually runs. Tracking parenthesis depth +// picks the loop keyword instead, and an unbalanced set — which CMD itself +// rejects — yields no body rather than a guessed one. func cmdForPayload(fields []string) []string { + depth := 0 + quoted := false for index, field := range fields { - if strings.EqualFold(strings.Trim(field, `"`), "do") { + if depth == 0 && strings.EqualFold(strings.Trim(field, `"`), "do") { return fields[index+1:] } + depth, quoted = cmdParenDepth(field, depth, quoted) } return nil } +// cmdParenDepth advances the parenthesis depth across one token, ignoring +// parentheses inside double quotes. Quote state is carried between tokens +// because a quoted run with a space in it can span them. +func cmdParenDepth(field string, depth int, quoted bool) (int, bool) { + for _, r := range field { + switch { + case r == '"': + quoted = !quoted + case quoted: + case r == '(': + depth++ + case r == ')': + if depth > 0 { + depth-- + } + } + } + return depth, quoted +} + // fallbackCMDForFCommands extracts the command source that CMD FOR /F executes // from its IN clause. Without usebackq, single quotes denote command text; with // usebackq, backticks do. The other quote form is literal input and must not be @@ -995,6 +1053,54 @@ func fallbackCommandTokens(command string) [][]string { } func fallbackCommandTokenInfo(command string) [][]fallbackCommandToken { + return fallbackCommandTokenInfoIn(command, false) +} + +// fallbackCMDCommandTokenInfo tokenizes the same text under CMD's quoting rules, +// where `"` is the ONLY quote character. +// +// The POSIX reading of `echo ' & curl https://evil.test` puts `& curl …` inside +// the echo argument, so the segment resolves to echo and classifies local. CMD +// has no single-quote quoting at all: the `'` is literal text, `&` is a command +// separator, and curl runs. The same holds for backticks, which execute nothing +// under CMD. Any text handed to a CMD reader is tokenized both ways so a quote +// form one runtime ignores cannot hide a separator from the other. +func fallbackCMDCommandTokenInfo(command string) [][]fallbackCommandToken { + return fallbackCommandTokenInfoIn(command, true) +} + +// posixSingleQuoteHidesCMDSeparator reports the one disagreement that matters: +// a POSIX single-quoted region — quoted to this tokenizer, ordinary text to CMD +// — containing a CMD command separator. In `echo ' & curl https://evil.test` +// the quote swallows `& curl …` into echo's argument, while cmd.exe runs curl. +// +// Merely containing a `'` is not enough to justify re-reading the text: POSIX +// constructs whose quoted regions hide nothing (`env -S 'printf ok' …`) would +// re-segment into shapes that mean nothing under CMD, and a fail-closed pass +// that fires on them is just noise. `;` is deliberately not a separator here — +// CMD treats it as an argument delimiter, not a command boundary. Double quotes +// quote under both languages, so they hide nothing from either. +func posixSingleQuoteHidesCMDSeparator(command string) bool { + escaped := false + var quote rune + for _, r := range command { + switch { + case escaped: + escaped = false + case r == '\\': + escaped = true + case quote == 0 && (r == '\'' || r == '"'): + quote = r + case quote == r: + quote = 0 + case quote == '\'' && (r == '&' || r == '|'): + return true + } + } + return false +} + +func fallbackCommandTokenInfoIn(command string, cmdQuoting bool) [][]fallbackCommandToken { // Command strings commonly preserve cmd.exe's escaped quote spelling. command = strings.ReplaceAll(command, `\"`, `"`) var commands [][]fallbackCommandToken @@ -1033,8 +1139,9 @@ func fallbackCommandTokenInfo(command string) [][]fallbackCommandToken { } // Backticks execute inside unquoted and double-quoted text, but are // literal inside single quotes. Preserve the surrounding quote while the - // substitution body is scanned as its own command. - if r == '`' && quote != '\'' { + // substitution body is scanned as its own command. CMD has no command + // substitution, so under its rules a backtick is ordinary text. + if r == '`' && quote != '\'' && !cmdQuoting { flushCommand() if backtick { if len(delimiters) > 0 && delimiters[len(delimiters)-1].opener == '`' { @@ -1048,6 +1155,12 @@ func fallbackCommandTokenInfo(command string) [][]fallbackCommandToken { backtick = !backtick continue } + if r == '\'' && cmdQuoting { + // Literal text to CMD, and deliberately NOT a word boundary: the + // point of this pass is that the quote hides nothing. + word.WriteRune(r) + continue + } if r == '\'' || r == '"' { switch quote { case 0: diff --git a/internal/sandbox/risk_hardening_test.go b/internal/sandbox/risk_hardening_test.go index 6df429e81..8eecd36d9 100644 --- a/internal/sandbox/risk_hardening_test.go +++ b/internal/sandbox/risk_hardening_test.go @@ -953,3 +953,100 @@ func TestClassifyBenignCommandStaysClean(t *testing.T) { } } } + +// Unit-level counterparts to TestEngineFailsClosedOnUnreadableCommandShapes, +// pinning the two readers directly so a regression names the layer that broke. +func TestGitSelectionDynamicFailsClosedOnUnreadableSelection(t *testing.T) { + for _, testCase := range []struct { + name string + words []string + // dynamicIndexes are positions the caller could not read. + dynamicIndexes []int + want bool + }{ + {name: "subcommand token", words: []string{"", "origin", "main"}, dynamicIndexes: []int{0}, want: true}, + {name: "global option value shifts the subcommand", words: []string{"-C", "", "status"}, dynamicIndexes: []int{1}, want: true}, + {name: "archive option past the subcommand", words: []string{"archive", "", "HEAD"}, dynamicIndexes: []int{1}, want: true}, + {name: "no subcommand at all", words: []string{""}, dynamicIndexes: []int{0}, want: true}, + {name: "literal local subcommand", words: []string{"commit", "-m", "message"}, want: false}, + // Past a non-archive subcommand nothing can change git's answer, so the + // common `git commit -m "$MESSAGE"` keeps its quiet path. + {name: "operand past a local subcommand", words: []string{"commit", "-m", ""}, dynamicIndexes: []int{2}, want: false}, + {name: "operand past a network subcommand", words: []string{"push", ""}, dynamicIndexes: []int{1}, want: false}, + {name: "literal archive", words: []string{"archive", "-o", "out.tar", "HEAD"}, want: false}, + } { + t.Run(testCase.name, func(t *testing.T) { + dynamic := make(map[int]bool, len(testCase.dynamicIndexes)) + for _, index := range testCase.dynamicIndexes { + dynamic[index] = true + } + if got := gitSelectionDynamic(testCase.words, func(index int) bool { return dynamic[index] }); got != testCase.want { + t.Fatalf("gitSelectionDynamic(%q, dynamic=%v) = %t, want %t", + testCase.words, testCase.dynamicIndexes, got, testCase.want) + } + }) + } +} + +func TestCMDForPayloadSelectsDOOutsideTheSet(t *testing.T) { + for _, testCase := range []struct { + name string + fields []string + want []string + }{ + { + name: "do is a set element", + fields: []string{"%i", "in", "(", "do", "x", ")", "do", "curl", "https://evil.test"}, + want: []string{"curl", "https://evil.test"}, + }, + { + name: "set element joined to the paren", + fields: []string{"%i", "in", "(do", "x)", "do", "curl", "https://evil.test"}, + want: []string{"curl", "https://evil.test"}, + }, + { + name: "quoted paren is not a set boundary", + fields: []string{"%i", "in", `("a)b"`, "x)", "do", "curl"}, + want: []string{"curl"}, + }, + { + name: "ordinary loop", + fields: []string{"%i", "in", "(curl)", "do", "echo", "%i"}, + want: []string{"echo", "%i"}, + }, + // CMD rejects an unbalanced set, so there is no loop body to classify. + {name: "unterminated set", fields: []string{"%i", "in", "(", "do", "curl"}, want: nil}, + } { + t.Run(testCase.name, func(t *testing.T) { + got := cmdForPayload(testCase.fields) + if strings.Join(got, " ") != strings.Join(testCase.want, " ") { + t.Fatalf("cmdForPayload(%q) = %q, want %q", testCase.fields, got, testCase.want) + } + }) + } +} + +func TestPOSIXSingleQuoteHidesCMDSeparator(t *testing.T) { + for _, testCase := range []struct { + command string + want bool + }{ + {command: `echo ' & curl https://evil.test`, want: true}, + {command: `echo ' | curl https://evil.test`, want: true}, + {command: `echo 'a & b' ok`, want: true}, + // Double quotes quote under both languages, so they hide nothing. + {command: `echo " & curl https://evil.test`, want: false}, + // The separator is outside the quotes: both readers already see it. + {command: `env -S 'printf ${VALUE}' & rem '`, want: false}, + {command: `env -S 'printf ok' curl https://evil.test && "unterminated`, want: false}, + // `;` delimits arguments in CMD; it does not start a command. + {command: `env -S 'printf curl; git push' && "unterminated`, want: false}, + {command: `echo 'plain text'`, want: false}, + } { + t.Run(testCase.command, func(t *testing.T) { + if got := posixSingleQuoteHidesCMDSeparator(testCase.command); got != testCase.want { + t.Fatalf("posixSingleQuoteHidesCMDSeparator(%q) = %t, want %t", testCase.command, got, testCase.want) + } + }) + } +} From 59de6778c6bd228b7594f0f4c09d1aa1be37eda7 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sun, 23 Aug 2026 20:42:20 +0200 Subject: [PATCH 08/12] fix(sandbox): consolidate git global-option parsing behind one authority MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from the latest review round. ## [P1 merge readiness] Rebase onto upstream main The branch was 4 commits behind Gitlawb/zero main (ad34dc8d, the 0.8.0 release). Rebased cleanly, no conflicts. go build/vet/test all still pass post-rebase. ## [P1] git -C bypass claim — did not reproduce; consolidated anyway I could not reproduce "git -C repo push origin main selects repo instead of push." GitGlobalOptionConsumesValue lowercases its input before the switch, so "-C" already folded onto the "-c" case (config override) and was — coincidentally — correctly treated as value-consuming, since both -c and -C take a separate-token value. TestGitGlobalOptionsResolveIdenticallyOnBothPaths already carried `{"-C repo push origin main", true}` and passed before this commit; my own direct probes (joined form -Crepo, doubled -C, mixed with -c, GIT_DIR= prefix) all classified correctly too. That the answer was right by an accident of two semantically different options happening to share a lowercase spelling is exactly the kind of parser-drift risk the review's "Overall guidance" is about, so I fixed it regardless: -C is now an explicit case in GitGlobalOptionConsumesValue rather than an implicit fold, with a comment explaining why -c and -C are listed separately despite doing the same thing here. The real consolidation this finding asked for was in internal/agent/command_prefix.go, which turned out to carry a SECOND, independent implementation of git's global-option grammar (gitSubcommand's hand-rolled scan loop, plus a duplicated gitOptionHasInlineValue list) alongside the sandbox network classifier's parseGitInvocation — exactly the "parallel skip lists" pattern the review warns produces this class of bug even when today's answer happens to be correct. New exported sandbox.GitSubcommand wraps parseGitInvocation so internal/agent's prefix-approval matcher reads git's global-option grammar through the SAME parser the classifier uses, instead of a second copy that can drift independently. gitSubcommand and gitOptionHasInlineValue are deleted; gitHasUnsafeGlobalOption keeps its own -C/--upload-pack judgment (matcher- specific: -C changes which repo a read-only subcommand inspects, which is a prefix-approval concern, not a network-classification one) but now steps over an option's value via the shared GitGlobalOptionConsumesValue rather than its own copy. GitTerminalGlobalOption, whose only caller was the deleted gitSubcommand, is removed as dead code rather than left unreferenced. Tests: extended TestGitGlobalOptionsResolveIdenticallyOnBothPaths (already the canonical AST+fallback parity matrix) with every separate-value global paired against a remote verb (not just -C+push and --git-dir+fetch), local commit forms with a global in front, and a value-layer negative where a global's OWN VALUE is a token spelled like a remote verb (`--git-dir push status` must stay local — "push" is --git-dir's argument, never git's own argv). Added TestSafeGitCommandRejectsDashCEvenWithAnApprovableSubcommand (isolates the -C rejection from the terminal-global short circuit already covered elsewhere) and TestSafeGitCommandRejectsSubcommandsOutsideTheApprovedList (proves the refactor onto a shared reader that resolves ANY subcommand did not widen what gets auto-approved, and that the subIndex arithmetic across the sandbox.GitSubcommand boundary is still correct end to end). Validation: go build ./..., go vet ./... (also on native Linux via WSL, go1.26.6), gofmt clean, deadcode clean (GitTerminalGlobalOption removal resolved the deadcode hit its own removal-worthy status created), go test ./internal/{sandbox,agent}/... green on both Windows and Linux (one unrelated WSL-environment-specific failure in internal/sandbox backend detection, confirmed present on the unmodified branch in the previous PR round). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JgWC2FnDp5Jjdvc6cqEfEQ --- internal/agent/command_prefix.go | 94 ++++++++++++------------- internal/agent/command_prefix_test.go | 50 +++++++++++++ internal/sandbox/analyzer.go | 41 +++++++---- internal/sandbox/risk_hardening_test.go | 28 ++++++++ 4 files changed, 151 insertions(+), 62 deletions(-) diff --git a/internal/agent/command_prefix.go b/internal/agent/command_prefix.go index 628a640b7..7334990a4 100644 --- a/internal/agent/command_prefix.go +++ b/internal/agent/command_prefix.go @@ -373,71 +373,69 @@ func validSedPrintArg(arg string) bool { return true } +// gitApprovableReadOnlySubcommands are the only git subcommands this prefix +// matcher will ever auto-approve. Every other resolvable subcommand (push, +// commit, ...) — and the case where no subcommand resolves at all — is +// rejected by the zero value of the map lookup below, so this list is the +// single place that grants approval; nothing else needs to enumerate it. +var gitApprovableReadOnlySubcommands = map[string]bool{ + "status": true, "log": true, "diff": true, "show": true, "branch": true, +} + +// safeGitCommand approves only status/log/diff/show/branch with no global +// option this matcher has not specifically vetted as safe. +// +// Subcommand resolution goes through sandbox.GitSubcommand — the SAME reader +// the network classifier uses — instead of a parallel hand-rolled scan of +// git's global-option grammar. Two independent scans of the same grammar is +// exactly what let this path and the classifier disagree about which token is +// the subcommand (an omitted -C in one, present in the other) even though +// both were nominally checking the same thing; sharing the reader removes the +// second copy that can drift. func safeGitCommand(command []string) bool { - subIndex, subcommand, ok := gitSubcommand(command) - if !ok { + if len(command) < 2 { return false } + // sandbox.GitSubcommand indexes into command[1:] (it does not expect the + // "git" argv[0] itself); shift its answer back into command's own indexing + // so the slices below still mean what they did before this used the shared + // reader. + subIndex, subcommand, ok := sandbox.GitSubcommand(command[1:]) + if !ok || !gitApprovableReadOnlySubcommands[subcommand] { + return false + } + subIndex++ if gitHasUnsafeGlobalOption(command[1:subIndex]) { return false } args := command[subIndex+1:] - switch subcommand { - case "status", "log", "diff", "show": - return gitArgsReadOnly(args) - case "branch": + if subcommand == "branch" { return gitArgsReadOnly(args) && gitBranchReadOnly(args) - default: - return false } + return gitArgsReadOnly(args) } -func gitSubcommand(command []string) (int, string, bool) { - for index := 1; index < len(command); index++ { - arg := command[index] - if sandbox.GitGlobalOptionConsumesValue(arg) { - index++ - continue - } - // A terminal global makes git print and exit, so the token after it is - // help text rather than a subcommand. Stopping here keeps this parser - // aligned with the sandbox classifier, which already stops: without it - // `git --help status` resolved to the read-only prefix `git status` - // and was auto-approved as a command it is not. - if sandbox.GitTerminalGlobalOption(arg) { - return 0, "", false - } - if gitOptionHasInlineValue(arg) || arg == "--" || strings.HasPrefix(arg, "-") { - continue - } - switch arg { - case "status", "log", "diff", "show", "branch": - return index, arg, true - default: - return 0, "", false - } - } - return 0, "", false -} - -func gitOptionHasInlineValue(arg string) bool { - return strings.HasPrefix(arg, "--attr-source=") || - strings.HasPrefix(arg, "--config-env=") || - strings.HasPrefix(arg, "--exec-path=") || - strings.HasPrefix(arg, "--git-dir=") || - strings.HasPrefix(arg, "--namespace=") || - strings.HasPrefix(arg, "--super-prefix=") || - strings.HasPrefix(arg, "--work-tree=") || - ((strings.HasPrefix(arg, "-C") || strings.HasPrefix(arg, "-c")) && len(arg) > 2) -} - +// gitHasUnsafeGlobalOption rejects global options this matcher has not +// specifically vetted as compatible with the read-only subcommands above. +// -C changes which repository status/log/diff/show/branch inspects — a prefix +// approved against the workspace repo must not silently extend to `git -C +// /elsewhere status`, which reports on a DIFFERENT repository the approval +// was never about. --upload-pack lets a fetch/clone run an arbitrary program +// on the remote side; it is listed here defensively even though none of the +// five approvable subcommands accept it, so a future addition to that list +// does not silently inherit the gap. +// +// This still reads git's shared skip-list (GitGlobalOptionConsumesValue) to +// step over an option's separate-token value correctly; only the "-C is +// unsafe regardless" judgment is specific to this matcher, so it stays local +// rather than joining the classifier's shared grammar. func gitHasUnsafeGlobalOption(args []string) bool { for index := 0; index < len(args); index++ { arg := args[index] switch { case strings.HasPrefix(arg, "--upload-pack"): return true - case arg == "-C" || strings.HasPrefix(arg, "-C"): + case strings.HasPrefix(arg, "-C"): return true case sandbox.GitGlobalOptionConsumesValue(arg): index++ diff --git a/internal/agent/command_prefix_test.go b/internal/agent/command_prefix_test.go index 6283e179c..31bb6f943 100644 --- a/internal/agent/command_prefix_test.go +++ b/internal/agent/command_prefix_test.go @@ -271,3 +271,53 @@ func TestSafeGitCommandStopsAtTerminalGlobalOptions(t *testing.T) { } } } + +// TestSafeGitCommandRejectsDashCEvenWithAnApprovableSubcommand isolates the +// -C rejection from the terminal-global short circuit above (`-C repo --help +// diff` never reaches gitHasUnsafeGlobalOption's -C case at all, because +// --help stops the scan first). -C changes which repository the read-only +// subcommand inspects, so an approval for the workspace repo must not extend +// to a different one named this way — even for status/log/diff/show/branch, +// which are otherwise auto-approved. +func TestSafeGitCommandRejectsDashCEvenWithAnApprovableSubcommand(t *testing.T) { + for _, command := range [][]string{ + {"git", "-C", "repo", "status"}, + {"git", "-Crepo", "status"}, + {"git", "-C", "repo", "branch"}, + } { + if safeGitCommand(command) { + t.Errorf("safeGitCommand(%q) = true; -C must never be auto-approved", command) + } + } +} + +// TestSafeGitCommandRejectsSubcommandsOutsideTheApprovedList proves the +// refactor onto sandbox.GitSubcommand (a shared reader that resolves ANY +// subcommand, not just the five this matcher approves) did not widen what +// gets auto-approved: a resolvable-but-unapproved subcommand, and no +// subcommand at all, both stay rejected. +func TestSafeGitCommandRejectsSubcommandsOutsideTheApprovedList(t *testing.T) { + for _, command := range [][]string{ + {"git", "push", "origin", "main"}, + {"git", "commit", "-m", "msg"}, + {"git", "fetch", "origin"}, + {"git"}, + {"git", "-C", "repo"}, + } { + if safeGitCommand(command) { + t.Errorf("safeGitCommand(%q) = true; want rejected", command) + } + } + // The approved subcommands still resolve correctly through the same shared + // reader, with a global option in front — end-to-end proof the subIndex + // arithmetic across the sandbox.GitSubcommand boundary is still right. + for _, command := range [][]string{ + {"git", "status"}, + {"git", "--git-dir", "/repo/.git", "log"}, + {"git", "branch"}, + } { + if !safeGitCommand(command) { + t.Errorf("safeGitCommand(%q) = false; want approved", command) + } + } +} diff --git a/internal/sandbox/analyzer.go b/internal/sandbox/analyzer.go index ea90efd64..a5ca36c4c 100644 --- a/internal/sandbox/analyzer.go +++ b/internal/sandbox/analyzer.go @@ -526,32 +526,45 @@ func parseGitInvocation(words []string) gitInvocation { // GitGlobalOptionConsumesValue lists git's global options whose value is a // separate token. It is shared with internal/agent's command-prefix parser so -// the two security-sensitive scans cannot drift. +// the two security-sensitive scans cannot drift — internal/agent reaches this +// through GitSubcommand, which resolves the whole grammar (terminal globals +// included) in one place, rather than importing the terminal-global table +// separately. // // `--exec-path` is deliberately absent: its value is inline-only // (`--exec-path=`), and the bare spelling is terminal — see // gitTerminalGlobalOptions. -// GitTerminalGlobalOption reports whether a git global option makes git print -// something from the local installation and exit, so nothing after it is a -// subcommand. It is the exported view of gitTerminalGlobalOptions, shared with -// internal/agent's command-prefix parser for the same reason -// GitGlobalOptionConsumesValue is: while each scan carried its own option -// grammar, `git --help status` was the safe prefix `git status` to one parser -// and a terminal help invocation to the other, and every new option had to be -// remembered in two places. -func GitTerminalGlobalOption(option string) bool { - return gitTerminalGlobalOptions[strings.ToLower(strings.TrimSpace(option))] -} - func GitGlobalOptionConsumesValue(option string) bool { switch strings.ToLower(option) { - case "-c", "--attr-source", "--config-env", "--git-dir", "--namespace", "--super-prefix", "--work-tree": + // -c (config override, `-c name=value`) and -C (run as if started in + // ) are DIFFERENT git global options that happen to both take a + // separate-token value — they are listed explicitly rather than left to + // fold together under strings.ToLower, so this stays correct if the two + // options' value-taking behavior ever diverges, and so a reader does not + // have to notice the case-fold to see -C is covered at all. + case "-c", "-C", "--attr-source", "--config-env", "--git-dir", "--namespace", "--super-prefix", "--work-tree": return true default: return false } } +// GitSubcommand resolves the git subcommand a command line will actually run, +// or reports that none runs — no subcommand is present (e.g. bare `git`, or +// `git -C repo` with nothing after it), or a terminal global like --help/ +// --version made everything after it non-executed help/version text instead. +// It is parseGitInvocation's exported view, so a consumer outside this +// package — internal/agent's prefix-approval matcher — reads git's global +// options through the SAME grammar the network classifier does, rather than +// maintaining its own skip list that can silently drift from this one. +func GitSubcommand(words []string) (index int, subcommand string, ok bool) { + invocation := parseGitInvocation(words) + if invocation.kind != gitCommandSubcommand { + return 0, "", false + } + return invocation.subcommandIndex, invocation.subcommand, true +} + func npxUsesNetwork(_ []string) bool { return true } diff --git a/internal/sandbox/risk_hardening_test.go b/internal/sandbox/risk_hardening_test.go index 8eecd36d9..20348097a 100644 --- a/internal/sandbox/risk_hardening_test.go +++ b/internal/sandbox/risk_hardening_test.go @@ -530,10 +530,38 @@ func TestGitGlobalOptionsResolveIdenticallyOnBothPaths(t *testing.T) { }{ {"push origin main", true}, {"-C repo push origin main", true}, + // -C is a joined-value spelling too: `-Crepo` is one token, and its embedded + // value must not be mistaken for the subcommand. + {"-Crepo push origin main", true}, + {"-C repo -C repo2 push origin main", true}, {"--git-dir /repo/.git fetch origin", true}, + // Every separate-value global against every remote verb this scanner + // treats as network, not just push+fetch above: the option-consumption + // rule is shared, so one representative pairing per option is what + // actually exercises drift, but each remote VERB gets its own row too — + // a verb this scanner has never seen paired with an option is exactly + // where a hand-maintained verb list would be missed. + {"--work-tree /repo pull origin main", true}, + {"--attr-source HEAD clone https://example.com/repo.git", true}, + {"--config-env foo=bar ls-remote origin", true}, + {"--namespace ns send-pack origin main", true}, + {"--super-prefix sub/ fetch origin", true}, {"--no-pager push origin main", true}, {"PUSH origin main", true}, {"--Git-Dir repo PUSH origin main", true}, + // Local commit forms must stay quiet with a global in front, not just + // bare — a global that is handled correctly for push but breaks commit + // would be a regression this scanner would otherwise never see, since + // every existing local-form row here is bare. + {"-C repo commit -m msg", false}, + {"--git-dir /repo/.git commit -m msg", false}, + {"--work-tree /repo status", false}, + // Value-layer negative: the global's VALUE is a token that is itself a + // remote verb's spelling. A scanner that fails to skip the value (instead + // of reading it as the next word to classify) would misread this as + // local — "push" here is --git-dir's argument, never git's own argv. + {"--git-dir push status", false}, + {"-C push status", false}, {"archive --mtime 2024-01-01 --remote origin HEAD", true}, {"archive --format --remote=origin HEAD", true}, {"archive --mtime --remote=origin HEAD", true}, From 5f0f17fa85bc15e6c5590da100324e3ce6c5616c Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Mon, 24 Aug 2026 21:23:27 +0200 Subject: [PATCH 09/12] fix(sandbox): preserve git subcommand spelling --- internal/agent/command_prefix.go | 32 +++++++-------- internal/agent/command_prefix_test.go | 19 +++++++++ internal/agent/loop_test.go | 56 ++++++++++++++++++++++++++ internal/sandbox/analyzer.go | 58 ++++++++++++++++++++------- internal/sandbox/analyzer_test.go | 15 +++++++ internal/sandbox/engine_test.go | 8 ++++ 6 files changed, 156 insertions(+), 32 deletions(-) diff --git a/internal/agent/command_prefix.go b/internal/agent/command_prefix.go index 7334990a4..b2f719457 100644 --- a/internal/agent/command_prefix.go +++ b/internal/agent/command_prefix.go @@ -382,34 +382,32 @@ var gitApprovableReadOnlySubcommands = map[string]bool{ "status": true, "log": true, "diff": true, "show": true, "branch": true, } -// safeGitCommand approves only status/log/diff/show/branch with no global -// option this matcher has not specifically vetted as safe. +// safeGitCommand approves only exact lowercase spellings of +// status/log/diff/show/branch with no global option this matcher has not +// specifically vetted as safe. // -// Subcommand resolution goes through sandbox.GitSubcommand — the SAME reader -// the network classifier uses — instead of a parallel hand-rolled scan of -// git's global-option grammar. Two independent scans of the same grammar is -// exactly what let this path and the classifier disagree about which token is -// the subcommand (an omitted -C in one, present in the other) even though -// both were nominally checking the same thing; sharing the reader removes the -// second copy that can drift. +// Subcommand resolution goes through sandbox.GitSubcommand so option parsing +// stays shared with the classifier, but authorization compares Original, never +// Normalized. Git aliases are case-sensitive: STATUS can be an arbitrary alias +// even though the classifier conservatively recognizes its normalized spelling +// as status. func safeGitCommand(command []string) bool { if len(command) < 2 { return false } - // sandbox.GitSubcommand indexes into command[1:] (it does not expect the - // "git" argv[0] itself); shift its answer back into command's own indexing - // so the slices below still mean what they did before this used the shared - // reader. - subIndex, subcommand, ok := sandbox.GitSubcommand(command[1:]) - if !ok || !gitApprovableReadOnlySubcommands[subcommand] { + selection, ok := sandbox.GitSubcommand(command[1:]) + if !ok || !gitApprovableReadOnlySubcommands[selection.Original] { return false } - subIndex++ + // GitSubcommand indexes into command[1:] (it does not expect the "git" + // argv[0] itself); shift its answer back into command's own indexing so the + // slices below retain their command-relative meaning. + subIndex := selection.Index + 1 if gitHasUnsafeGlobalOption(command[1:subIndex]) { return false } args := command[subIndex+1:] - if subcommand == "branch" { + if selection.Original == "branch" { return gitArgsReadOnly(args) && gitBranchReadOnly(args) } return gitArgsReadOnly(args) diff --git a/internal/agent/command_prefix_test.go b/internal/agent/command_prefix_test.go index 31bb6f943..744b14c59 100644 --- a/internal/agent/command_prefix_test.go +++ b/internal/agent/command_prefix_test.go @@ -46,6 +46,25 @@ func TestSafeGitCommandConsumesAttrSourceOptionValue(t *testing.T) { } } +func TestSafeGitCommandRejectsCaseDistinctAliasSubcommand(t *testing.T) { + for _, command := range [][]string{ + {"git", "-c", "alias.STATUS=!curl", "STATUS", "https://example.invalid"}, + {"git", "-c", "alias.Status=!curl", "Status", "https://example.invalid"}, + } { + if safeGitCommand(command) { + t.Errorf("safeGitCommand(%q) = true; case-distinct aliases must not receive reusable prefixes", command) + } + } + if prefix := proposedCommandPrefix("bash", map[string]any{ + "command": `make test && git -c alias.STATUS=!curl STATUS https://example.invalid`, + }); prefix != nil { + t.Fatalf("case-distinct alias was accepted as a safe tail and exposed prefix %#v", prefix) + } + if !safeGitCommand([]string{"git", "--attr-source", "HEAD", "status"}) { + t.Fatal("ordinary lowercase status with a vetted global option must remain approvable") + } +} + func TestProposedCommandPrefixSupportsSegmentedCommands(t *testing.T) { got := proposedCommandPrefix("bash", map[string]any{"command": "ps aux | head -5"}) if runtime.GOOS == "windows" { diff --git a/internal/agent/loop_test.go b/internal/agent/loop_test.go index 849f677ac..a004e48cf 100644 --- a/internal/agent/loop_test.go +++ b/internal/agent/loop_test.go @@ -2628,6 +2628,62 @@ func TestRunDoesNotOfferPrefixApprovalForUnsafeBashCommand(t *testing.T) { } } +func TestRunDoesNotOfferPrefixApprovalForCaseDistinctGitAlias(t *testing.T) { + root := t.TempDir() + command := `make test && git -c alias.STATUS=!curl STATUS https://example.invalid` + retryTool := &sandboxDeniedRetryTool{} + registry := tools.NewRegistry() + registry.Register(retryTool) + provider := &mockProvider{ + turns: [][]zeroruntime.StreamEvent{ + { + {Type: zeroruntime.StreamEventToolCallStart, ToolCallID: "call-1", ToolName: "bash"}, + {Type: zeroruntime.StreamEventToolCallDelta, ToolCallID: "call-1", ArgumentsFragment: `{"command":` + quoteJSONString(command) + `}`}, + {Type: zeroruntime.StreamEventToolCallEnd, ToolCallID: "call-1"}, + {Type: zeroruntime.StreamEventDone}, + }, + { + {Type: zeroruntime.StreamEventText, Content: "done"}, + {Type: zeroruntime.StreamEventDone}, + }, + }, + } + requestCount := 0 + + _, err := Run(context.Background(), "inspect status", provider, Options{ + Registry: registry, + PermissionMode: PermissionModeAsk, + Autonomy: "medium", + Sandbox: sandbox.NewEngine(sandbox.EngineOptions{ + WorkspaceRoot: root, + Policy: sandbox.DefaultPolicy(), + Backend: sandbox.Backend{Name: sandbox.BackendUnavailable, Message: "native sandbox unavailable"}, + }), + OnPermissionRequest: func(_ context.Context, request PermissionRequest) (PermissionDecision, error) { + requestCount++ + if len(request.CommandPrefix) != 0 { + t.Fatalf("case-distinct alias must not have a command prefix: %#v", request.CommandPrefix) + } + if containsPermissionDecision(request.AvailableDecisions, PermissionDecisionAllowPrefix) || + containsPermissionDecision(request.AvailableDecisions, PermissionDecisionAlwaysAllowPrefix) { + t.Fatalf("case-distinct alias must not offer reusable prefix approval: %#v", request.AvailableDecisions) + } + return PermissionDecision{Action: PermissionDecisionDeny, Reason: "deny alias execution"}, nil + }, + }) + if err != nil { + t.Fatal(err) + } + if requestCount != 1 { + t.Fatalf("permission requests = %d, want one non-prefix request", requestCount) + } + for index, call := range retryTool.calls { + if shellCommandRequiresEscalated(call) { + t.Fatalf("call %d received require_escalated: %#v", index, call) + } + } +} + func TestRunPromptsForDestructiveShellInsteadOfSandboxDeny(t *testing.T) { root := t.TempDir() command := "echo rm -rf /" diff --git a/internal/sandbox/analyzer.go b/internal/sandbox/analyzer.go index a5ca36c4c..4553489fe 100644 --- a/internal/sandbox/analyzer.go +++ b/internal/sandbox/analyzer.go @@ -297,6 +297,7 @@ func gitSelectionDynamic(words []string, dynamic func(index int) bool) bool { // in resolveCommandArgv so AST and fallback paths cannot select different // children before reaching this program-specific table. func literalProgramNetworkResolution(prog string, words []string) commandResolution { + originalWords := words prog = normalizeProgramToken(trimCMDEchoPrefixToken(prog)) normalized := make([]string, len(words)) for index := range words { @@ -339,7 +340,7 @@ func literalProgramNetworkResolution(prog string, words []string) commandResolut case "go": return commandNetworkResolution(firstSubcommand(words, nil) == "get") case "git": - return commandNetworkResolution(gitUsesNetwork(words)) + return gitNetworkResolution(originalWords) case "gh": return commandNetworkResolution(ghUsesNetwork(words)) default: @@ -382,12 +383,19 @@ func packageManagerOffline(words []string) bool { return false } -func gitUsesNetwork(words []string) bool { +func gitNetworkResolution(words []string) commandResolution { invocation := parseGitInvocation(words) if invocation.kind != gitCommandSubcommand { // No subcommand at all, or a global option that makes git print locally // and exit before any subcommand runs. - return false + return commandKnownLocal + } + // Git builtin names and aliases are case-sensitive. A case-distinct spelling + // such as STATUS can therefore select an arbitrary alias even though status + // is a known local builtin. Classification may normalize to recognize known + // builtins, but it must not report an alias-shaped invocation as proven local. + if invocation.subcommandOriginal != invocation.subcommand { + return commandUnresolved } switch invocation.subcommand { // send-pack is push's plumbing counterpart — `git send-pack origin main` @@ -395,15 +403,15 @@ func gitUsesNetwork(words []string) bool { // same operation two different answers depending on which spelling was // used. case "clone", "fetch", "pull", "push", "ls-remote", "send-pack": - return true + return commandKnownNetwork case "archive": // `git archive HEAD` streams a tree out of the local object store and needs // no egress at all; only `--remote=` sends the request to another // host. Classifying every archive as network cost a proactive network // prompt on a purely local command. - return gitTargetsRemoteArchive(words, invocation.subcommandIndex) + return commandNetworkResolution(gitTargetsRemoteArchive(words, invocation.subcommandIndex)) default: - return false + return commandKnownLocal } } @@ -464,6 +472,10 @@ type gitInvocation struct { kind gitCommandKind // subcommand is set only for gitCommandSubcommand. subcommand string + // subcommandOriginal preserves the exact token Git will dispatch. Git alias + // lookup is case-sensitive, so authorization callers must not replace it + // with subcommand's normalized classifier spelling. + subcommandOriginal string // subcommandIndex is the position in the original argument slice. subcommandIndex int // terminalOption is set only for gitCommandTerminalGlobal. @@ -501,7 +513,8 @@ var gitTerminalGlobalOptions = map[string]bool{ // resolves the same option set for its own prefix matching. func parseGitInvocation(words []string) gitInvocation { for index := 0; index < len(words); index++ { - word := strings.ToLower(words[index]) + original := words[index] + word := strings.ToLower(original) if word == "" { continue } @@ -519,7 +532,12 @@ func parseGitInvocation(words []string) gitInvocation { if isNumericToken(word) { continue } - return gitInvocation{kind: gitCommandSubcommand, subcommand: word, subcommandIndex: index} + return gitInvocation{ + kind: gitCommandSubcommand, + subcommand: word, + subcommandOriginal: original, + subcommandIndex: index, + } } return gitInvocation{kind: gitCommandNone} } @@ -549,20 +567,30 @@ func GitGlobalOptionConsumesValue(option string) bool { } } +// GitSubcommandInfo is the shared result of resolving Git's global-option +// grammar. Original is the exact token Git dispatches; Normalized is for +// conservative classification only. Authorization must compare Original +// against an explicit allowlist because Git alias lookup is case-sensitive. +type GitSubcommandInfo struct { + Index int + Original string + Normalized string +} + // GitSubcommand resolves the git subcommand a command line will actually run, // or reports that none runs — no subcommand is present (e.g. bare `git`, or // `git -C repo` with nothing after it), or a terminal global like --help/ // --version made everything after it non-executed help/version text instead. -// It is parseGitInvocation's exported view, so a consumer outside this -// package — internal/agent's prefix-approval matcher — reads git's global -// options through the SAME grammar the network classifier does, rather than -// maintaining its own skip list that can silently drift from this one. -func GitSubcommand(words []string) (index int, subcommand string, ok bool) { +func GitSubcommand(words []string) (GitSubcommandInfo, bool) { invocation := parseGitInvocation(words) if invocation.kind != gitCommandSubcommand { - return 0, "", false + return GitSubcommandInfo{}, false } - return invocation.subcommandIndex, invocation.subcommand, true + return GitSubcommandInfo{ + Index: invocation.subcommandIndex, + Original: invocation.subcommandOriginal, + Normalized: invocation.subcommand, + }, true } func npxUsesNetwork(_ []string) bool { diff --git a/internal/sandbox/analyzer_test.go b/internal/sandbox/analyzer_test.go index e700fd9fa..a1a0eff6b 100644 --- a/internal/sandbox/analyzer_test.go +++ b/internal/sandbox/analyzer_test.go @@ -379,3 +379,18 @@ func TestGitSendPackIsNetwork(t *testing.T) { } } } + +func TestGitSubcommandPreservesOriginalAndNormalizedSpelling(t *testing.T) { + selection, ok := GitSubcommand([]string{"-c", "alias.STATUS=!curl", "STATUS"}) + if !ok { + t.Fatal("GitSubcommand did not resolve the alias-shaped subcommand") + } + if selection.Index != 2 || selection.Original != "STATUS" || selection.Normalized != "status" { + t.Fatalf("GitSubcommand = %#v, want index 2, original STATUS, normalized status", selection) + } + + lowercase, ok := GitSubcommand([]string{"--attr-source", "HEAD", "status"}) + if !ok || lowercase.Index != 2 || lowercase.Original != "status" || lowercase.Normalized != "status" { + t.Fatalf("lowercase GitSubcommand = %#v ok=%v", lowercase, ok) + } +} diff --git a/internal/sandbox/engine_test.go b/internal/sandbox/engine_test.go index 7ec215008..d37301947 100644 --- a/internal/sandbox/engine_test.go +++ b/internal/sandbox/engine_test.go @@ -165,6 +165,14 @@ func TestEngineFailsClosedOnUnreadableCommandShapes(t *testing.T) { {name: "git subcommand from a quoted expansion", command: `VERB=push; git "$VERB" origin main`}, // Same reading through the unparseable fallback rather than the AST. {name: "git subcommand from an expansion, fallback", command: `git $VERB origin main && "unterminated`}, + { + name: "case-distinct git alias supplied through config", + command: `git -c alias.STATUS=!curl STATUS https://example.invalid`, + }, + { + name: "case-distinct git alias through cmd fallback", + command: `git -c alias.STATUS=!curl STATUS https://example.invalid & rem '`, + }, // archive is local until --remote makes it a fetch from another host, so // an unreadable option word past the subcommand counts for archive alone. {name: "git archive options from an expansion", command: `OPTS=--remote=origin; git archive $OPTS HEAD`}, From 237024d64dd3de7e76b73af24a04ea2c11fb82a8 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Tue, 25 Aug 2026 21:41:14 +0200 Subject: [PATCH 10/12] fix(sandbox): fail closed on CMD FOR metavariable executables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A CMD FOR metavariable in an executable position was read as an ordinary unknown program and classified proven-local, so the network gate was dropped on a command whose executable CMD had not substituted yet. `for /f %i in (list.txt) do %i https://host` runs whatever list.txt names; the fallback resolver reported no network category for it. fallbackTokenLooksDynamic only recognized PAIRED expansions (%NAME%, !NAME!). A FOR metavariable has no closing delimiter, so %i, the batch %%i spelling, the ~-modified %~dpi forms, and a batch parameter %1 all read as literal program names. containsCMDSingleDelimiterExpansion now recognizes them, which reaches every executable and interpreter-source position through the existing resolveCommandArgv check rather than adding a FOR-specific exception. The reference must BE the token and the name is a single character, per CMD's grammar. That keeps literal spellings precise: `%local` is `%l` followed by text rather than a reference, and `100%local` does not start with the sigil — both stay proven-local, as TestClassifyUnparseableLiteralPercentBangAndShell SourceStayLocal requires. Only executable positions consult this, so literal FOR set data and echo-style bodies are untouched. Engine.Evaluate coverage lands in the existing tables: the metavariable forms in the reviewed-network table, and `for %i in (*.txt) do echo %i`, `... do type %i`, `... do copy %i backup\%i` as local controls, so the fix is proven to restore the gate without broad false prompts. The five network rows were each verified to fail against the unfixed resolver. Separately, on the git -C finding: the reported failure does not reproduce. GitGlobalOptionConsumesValue compares a normalized name, and -C folds onto the -c case, so `git -C repo push` already consumes repo and resolves push --- the four -C rows added here pass against the unfixed parser. The real defect was an unreachable uppercase case whose comment claimed it protected against the two options' behavior diverging, which it could not. Removed it, corrected the comment to say where that distinction would have to be restored (the call site, which normalizes), and pinned the fold plus the -C operand consumption with tests so a future divergence is caught deliberately. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01U389qmQUhoZB3YtXkFmiTq --- internal/sandbox/analyzer.go | 20 +++++++---- internal/sandbox/analyzer_test.go | 49 ++++++++++++++++++++++++++ internal/sandbox/engine_test.go | 26 ++++++++++++++ internal/sandbox/risk.go | 57 ++++++++++++++++++++++++++++++- 4 files changed, 144 insertions(+), 8 deletions(-) diff --git a/internal/sandbox/analyzer.go b/internal/sandbox/analyzer.go index 4553489fe..0863c6363 100644 --- a/internal/sandbox/analyzer.go +++ b/internal/sandbox/analyzer.go @@ -553,14 +553,20 @@ func parseGitInvocation(words []string) gitInvocation { // (`--exec-path=`), and the bare spelling is terminal — see // gitTerminalGlobalOptions. func GitGlobalOptionConsumesValue(option string) bool { + // The comparison is on the NORMALIZED name, so every case below must be + // written lowercase — an uppercase case here would be unreachable rather + // than protective. -c (config override, `-c name=value`) and -C (run as if + // started in ) are different git options that both take a + // separate-token value, so folding them onto one case is correct today and + // `git -C repo push` consumes repo either way. + // + // If their value-taking behavior ever diverges, this cannot be fixed by + // re-adding a -C case: the callers normalize before calling, so the + // distinction has to be restored at the call site by passing the original + // token. TestGitGlobalOptionConsumesValueFoldsShortCaseDeliberately pins + // the fold so that change is a deliberate one. switch strings.ToLower(option) { - // -c (config override, `-c name=value`) and -C (run as if started in - // ) are DIFFERENT git global options that happen to both take a - // separate-token value — they are listed explicitly rather than left to - // fold together under strings.ToLower, so this stays correct if the two - // options' value-taking behavior ever diverges, and so a reader does not - // have to notice the case-fold to see -C is covered at all. - case "-c", "-C", "--attr-source", "--config-env", "--git-dir", "--namespace", "--super-prefix", "--work-tree": + case "-c", "--attr-source", "--config-env", "--git-dir", "--namespace", "--super-prefix", "--work-tree": return true default: return false diff --git a/internal/sandbox/analyzer_test.go b/internal/sandbox/analyzer_test.go index a1a0eff6b..d987ac927 100644 --- a/internal/sandbox/analyzer_test.go +++ b/internal/sandbox/analyzer_test.go @@ -394,3 +394,52 @@ func TestGitSubcommandPreservesOriginalAndNormalizedSpelling(t *testing.T) { t.Fatalf("lowercase GitSubcommand = %#v ok=%v", lowercase, ok) } } + +// TestGitGlobalOptionConsumesValueFoldsShortCaseDeliberately pins the case-fold +// in GitGlobalOptionConsumesValue. -c and -C are DIFFERENT git options that +// today both take a separate-token value, and the callers normalize the option +// name before asking, so the two necessarily share one answer here. That is +// safe only while their value-taking behavior agrees; if git ever changes one +// of them, this test fails and the distinction has to be restored at the call +// site by passing the original token, not by adding an unreachable uppercase +// case to a switch that compares lowercased input. +func TestGitGlobalOptionConsumesValueFoldsShortCaseDeliberately(t *testing.T) { + for _, option := range []string{"-c", "-C"} { + if !GitGlobalOptionConsumesValue(option) { + t.Fatalf("GitGlobalOptionConsumesValue(%q) = false, want true", option) + } + } + // Long options are all-lowercase in git; the fold must not invent new ones. + for _, option := range []string{"-d", "-x", "--exec-path", "--not-an-option"} { + if GitGlobalOptionConsumesValue(option) { + t.Fatalf("GitGlobalOptionConsumesValue(%q) = true, want false", option) + } + } +} + +// TestGitSubcommandConsumesDashCOperand proves the -C operand is consumed in +// both the separated and joined spellings, so the subcommand git dispatches is +// the one found. Missing this would make `git -C repo push` resolve `repo` as +// the subcommand, and an unknown subcommand is classified proven-local — the +// network gate would be dropped on a real push. +func TestGitSubcommandConsumesDashCOperand(t *testing.T) { + for _, tc := range []struct { + words []string + want string + index int + }{ + {words: []string{"-C", "repo", "push", "origin", "main"}, want: "push", index: 2}, + {words: []string{"-Crepo", "push", "origin", "main"}, want: "push", index: 1}, + {words: []string{"-C", "repo", "status"}, want: "status", index: 2}, + {words: []string{"-c", "a=b", "push"}, want: "push", index: 2}, + } { + info, ok := GitSubcommand(tc.words) + if !ok || info.Normalized != tc.want || info.Index != tc.index { + t.Fatalf("GitSubcommand(%q) = %+v ok=%v, want %q at %d", tc.words, info, ok, tc.want, tc.index) + } + } + // `git -C repo` runs nothing: the operand is consumed and no subcommand is left. + if info, ok := GitSubcommand([]string{"-C", "repo"}); ok { + t.Fatalf("GitSubcommand(-C repo) = %+v, want no subcommand", info) + } +} diff --git a/internal/sandbox/engine_test.go b/internal/sandbox/engine_test.go index d37301947..99817997a 100644 --- a/internal/sandbox/engine_test.go +++ b/internal/sandbox/engine_test.go @@ -391,6 +391,22 @@ func TestEnginePromptsForReviewedUnparseableNetworkForms(t *testing.T) { `strace --tips curl https://evil.test && "unterminated`, `strace -fqo trace.log curl https://evil.test && "unterminated`, `powershell -ep RemoteSigned curl https://evil.test & rem '`, + // A CMD FOR metavariable supplies the executable. CMD substitutes it + // before launching the body, so the spelling proves nothing about what + // runs and the network gate must survive. %%i is the batch spelling of + // the same reference, and %~dpi the modified form. + `for /f %i in (list.txt) do %i https://evil.test`, + `for /f %%i in (list.txt) do %%i https://evil.test`, + `for /f %i in (list.txt) do %~dpi https://evil.test`, + `for /f %i in (list.txt) do call %i https://evil.test`, + // A batch parameter in the same position is equally unreadable. + `%1 https://evil.test & rem '`, + // git -C takes a separate-token value; the subcommand is what follows it, + // so push must still be found in both the separated and joined spellings. + `git -C repo push origin main`, + `git -Crepo push origin main`, + `git -C repo push origin main & rem '`, + `git -Crepo push origin main & rem '`, // PowerShell source that exists but cannot be read statically: an // undecodable encoded payload, valid encoded network source, and a // Command operand supplied by an expansion. @@ -457,6 +473,16 @@ func TestEngineDoesNotPromptForNonNetworkCommandForms(t *testing.T) { `bash -- -c 'curl https://evil.test' && "unterminated`, `bash -Zc 'curl https://evil.test' && "unterminated`, `bash -nc 'curl https://evil.test' && "unterminated`, + // FOR bodies whose executable IS readable keep their precision: the + // metavariable is ordinary data in an argument position, so recognizing + // it in executable position must not cost these a network prompt. + `for %i in (*.txt) do echo %i`, + `for /f %i in (list.txt) do type %i`, + `for %i in (a b c) do copy %i backup\%i`, + `for /f %i in (list.txt) do echo %i >> out.txt`, + // -C's operand is consumed, so a local subcommand after it stays local. + `git -C repo status`, + `git -Crepo status & rem '`, } { t.Run(command, func(t *testing.T) { engine := NewEngine(EngineOptions{WorkspaceRoot: t.TempDir(), Policy: DefaultPolicy()}) diff --git a/internal/sandbox/risk.go b/internal/sandbox/risk.go index 4307de6e3..252c450dd 100644 --- a/internal/sandbox/risk.go +++ b/internal/sandbox/risk.go @@ -298,7 +298,10 @@ func fallbackTokenLooksDynamic(token string) bool { if strings.ContainsAny(token, "$`") { return true } - return containsCMDVariableExpansion(token, '%') || containsCMDVariableExpansion(token, '!') + if containsCMDVariableExpansion(token, '%') || containsCMDVariableExpansion(token, '!') { + return true + } + return containsCMDSingleDelimiterExpansion(token) } func containsCMDVariableExpansion(token string, delimiter byte) bool { @@ -338,6 +341,58 @@ func validCMDVariableName(name string) bool { return true } +// containsCMDSingleDelimiterExpansion reports whether a token IS a CMD +// expansion written with one leading percent rather than a matched pair: a FOR +// metavariable (%i, the batch %%i spelling, and the ~-modified %~dpi forms) or +// a batch parameter (%1). containsCMDVariableExpansion cannot see these, +// because it looks for %NAME% and these have no closing delimiter. +// +// CMD substitutes them before launching the command, so in an EXECUTABLE +// position the spelling says nothing about what runs: `for /f %i in (list.txt) +// do %i https://host` executes whatever list.txt names. Reading %i as an +// ordinary unknown program turned "cannot resolve" into "proven local" and +// dropped the network gate. +// +// The reference must BE the token, not merely appear inside it, and the name +// is a single character. A metavariable name is one char by CMD's grammar, so +// `%local` is `%l` followed by literal text rather than a reference, and +// `100%local` does not start with the sigil at all — both are ordinary program +// spellings that keep their proven-local answer. Only executable and +// interpreter-source positions consult this (see the callers of +// fallbackTokenLooksDynamic), so literal FOR set data and an echo-style body's +// arguments keep their precision too: `for %i in (*.txt) do echo %i` still +// resolves through the literal program `echo`. +func containsCMDSingleDelimiterExpansion(token string) bool { + if !strings.HasPrefix(token, "%") { + return false + } + rest := token[1:] + // A batch file doubles the sigil (%%i); both spellings reach the same + // substitution, so step over the second one and read the name. + rest = strings.TrimPrefix(rest, "%") + if rest == "" { + return false + } + // %~dpi and friends: the modifier run only ever introduces a metavariable + // reference, so the '~' alone is conclusive. + if rest[0] == '~' { + return true + } + if !isCMDMetavariableName(rest[0]) { + return false + } + // One character names the variable; anything else that could continue a + // name means this token is literal text, not a reference. + return len(rest) == 1 || !isCMDMetavariableName(rest[1]) +} + +// isCMDMetavariableName reports whether c can name a FOR metavariable or a +// batch parameter. CMD accepts any single alphanumeric character here, and the +// name is case-sensitive (%i and %I are different variables). +func isCMDMetavariableName(c byte) bool { + return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') +} + // fallbackPayloadUsesNetwork classifies CMD payload text, so it reads the text // under CMD's quoting as well as the shared POSIX tokenization — a `'` CMD // treats as ordinary text must not hide the separator that follows it. From a5181291b56f63b097a3ce2d64052565e493af29 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Tue, 25 Aug 2026 22:53:10 +0200 Subject: [PATCH 11/12] fix(sandbox): gate PowerShell source the readers cannot prove local MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit classifyInterpreterSource ran PowerShell -Command payloads through the POSIX AST scan and the unparseable-command matcher, then returned commandKnownLocal when neither found a network program. Neither reader models PowerShell grammar, so that conclusion does not follow: they happen to read a simple `Verb-Noun -Arg` line correctly because it tokenizes like a POSIX command, but "the POSIX reader found nothing" is not evidence the source is local. Reproduced before fixing — every one of these performed the request and drew no network category: powershell -Command 'try { Invoke-WebRequest https://evil.test } catch {}' powershell -Command 'foreach ($i in 1..3) { curl https://evil.test }' powershell -Command 'Get-Content urls.txt | ForEach-Object { Invoke-WebRequest $_ }' powerShellSourceUnreadable now names the grammar the readers cannot model and returns commandUnresolved for it, extending the rule the backtick check already established rather than adding a new special case. Every block-structured form PowerShell has — try/catch/finally, if/else, foreach, for, while, do, switch, function, trap, param, and a script block handed to ForEach-Object or the call operator — is written with braces, so their presence is the one signal needed. Precision is deliberately retained: source the readers CAN tokenise and prove local keeps its quiet path, so `Write-Output hello`, `Get-Process` and `Get-ChildItem -Path .` still draw no prompt. Unresolved costs a prompt on a shell request rather than a denial, so erring this way asks a question instead of breaking a command. Enforcement-level coverage, asserting the Engine.Evaluate result rather than an analyzer boolean, in the existing tables: seven network rows (including a block whose body names nothing recognisable, so it is gated on the grammar rather than on spotting a network verb) and three local controls. Four of the seven fail against the unfixed classifier; the other three were already gated incidentally by the POSIX reader and are kept as coverage of that. Also rebased onto current main (6fe0d1ed), which the branch was four commits behind. Full sandbox suite re-run on the resolved head. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01U389qmQUhoZB3YtXkFmiTq --- internal/sandbox/analyzer.go | 44 ++++++++++++++++++++++++++++++--- internal/sandbox/engine_test.go | 19 ++++++++++++++ 2 files changed, 59 insertions(+), 4 deletions(-) diff --git a/internal/sandbox/analyzer.go b/internal/sandbox/analyzer.go index 0863c6363..bd946bd00 100644 --- a/internal/sandbox/analyzer.go +++ b/internal/sandbox/analyzer.go @@ -866,10 +866,10 @@ func classifyInterpreterSource(payload string, language interpreterSourceLanguag if depth > maxAnalyzerDepth { return commandUnresolved } - if language == interpreterSourcePowerShell && strings.ContainsRune(payload, '`') { - // Backtick escaping and line continuation change PowerShell token - // boundaries but have no equivalent in the POSIX/CMD readers below. - return commandUnresolved + if language == interpreterSourcePowerShell { + if unreadable := powerShellSourceUnreadable(payload); unreadable != "" { + return commandUnresolved + } } result := AnalysisResult{} analyzeInto(payload, &result, map[string]bool{}, depth) @@ -879,6 +879,42 @@ func classifyInterpreterSource(payload string, language interpreterSourceLanguag return commandKnownLocal } +// powerShellBlockPunctuation opens or closes a PowerShell script block or +// statement block. Every block-structured form the language has — try/catch/ +// finally, if/else, foreach, for, while, do, switch, function, trap, param, and +// a script block handed to ForEach-Object or the call operator — is written +// with braces, so their presence is the single signal that the source has +// structure the readers below do not model. +const powerShellBlockPunctuation = "{}" + +// powerShellSourceUnreadable reports why PowerShell source cannot be proven +// local by the readers classifyInterpreterSource runs, or "" when it can. +// +// Those readers are the POSIX AST scan and the unparseable-command matcher. +// Neither models PowerShell grammar. They happen to read a simple +// `Verb-Noun -Arg value` line correctly because it tokenizes like a POSIX +// command, and that precision is worth keeping — but "the POSIX reader found no +// network program in this PowerShell source" is not evidence the source is +// local. `try { Invoke-WebRequest https://host } catch {}` is valid PowerShell +// that performs the request, and neither reader models try/catch, so it drew no +// network category at all. +// +// A conservative unresolved result is the contract here: only source a reader +// can actually parse and prove local may skip the gate. Unresolved costs a +// prompt on a shell request, not a denial, so the failure mode of being wrong +// in this direction is a question rather than a broken command. +func powerShellSourceUnreadable(payload string) string { + if strings.ContainsRune(payload, '`') { + // Backtick escaping and line continuation change PowerShell token + // boundaries but have no equivalent in the POSIX/CMD readers. + return "backtick escaping" + } + if strings.ContainsAny(payload, powerShellBlockPunctuation) { + return "script or statement block" + } + return "" +} + func pythonModuleUsesNetwork(words []string) bool { for index := 0; index < len(words); index++ { if words[index] != "-m" || index+1 >= len(words) { diff --git a/internal/sandbox/engine_test.go b/internal/sandbox/engine_test.go index 99817997a..c5622c641 100644 --- a/internal/sandbox/engine_test.go +++ b/internal/sandbox/engine_test.go @@ -396,6 +396,19 @@ func TestEnginePromptsForReviewedUnparseableNetworkForms(t *testing.T) { // runs and the network gate must survive. %%i is the batch spelling of // the same reference, and %~dpi the modified form. `for /f %i in (list.txt) do %i https://evil.test`, + // PowerShell source with block structure. The POSIX AST scan and the + // unparseable matcher do not model try/catch, if, foreach, while or a + // script block piped into ForEach-Object, so "no network program found" + // is not evidence the source is local — it must stay gated. + `powershell -Command 'try { Invoke-WebRequest https://evil.test } catch {}'`, + `powershell -Command "try { Invoke-WebRequest https://evil.test } catch {}"`, + `pwsh -Command 'if ($true) { Invoke-WebRequest https://evil.test }'`, + `powershell -Command 'foreach ($i in 1..3) { curl https://evil.test }'`, + `powershell -Command 'while ($true) { iwr https://evil.test }'`, + `powershell -Command 'Get-Content urls.txt | ForEach-Object { Invoke-WebRequest $_ }'`, + // A block whose body names nothing recognisable is still unreadable, so it + // is gated on the grammar rather than on spotting a network verb. + `powershell -Command 'try { & $tool $target } catch {}'`, `for /f %%i in (list.txt) do %%i https://evil.test`, `for /f %i in (list.txt) do %~dpi https://evil.test`, `for /f %i in (list.txt) do call %i https://evil.test`, @@ -477,6 +490,12 @@ func TestEngineDoesNotPromptForNonNetworkCommandForms(t *testing.T) { // metavariable is ordinary data in an argument position, so recognizing // it in executable position must not cost these a network prompt. `for %i in (*.txt) do echo %i`, + // Simple PowerShell source the readers CAN tokenise and prove local keeps + // its quiet path: the fail-closed rule above is about block grammar, not + // about treating every PowerShell command as unknown. + `powershell -Command 'Write-Output hello'`, + `powershell -Command 'Get-Process'`, + `powershell -NoProfile -Command 'Get-ChildItem -Path .'`, `for /f %i in (list.txt) do type %i`, `for %i in (a b c) do copy %i backup\%i`, `for /f %i in (list.txt) do echo %i >> out.txt`, From b6a153a65c0684613542782bf9204a57e82e6cae Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Wed, 26 Aug 2026 15:27:49 +0200 Subject: [PATCH 12/12] fix(sandbox): preserve nested source uncertainty --- internal/sandbox/analyzer.go | 79 ++++++++++++++++++++++++++++--- internal/sandbox/analyzer_test.go | 6 +++ internal/sandbox/engine_test.go | 12 +++++ 3 files changed, 90 insertions(+), 7 deletions(-) diff --git a/internal/sandbox/analyzer.go b/internal/sandbox/analyzer.go index bd946bd00..d9ab3cea7 100644 --- a/internal/sandbox/analyzer.go +++ b/internal/sandbox/analyzer.go @@ -712,18 +712,20 @@ func busyboxSourceDynamic(args []*syntax.Word) bool { return !isLiteralWord(args[0]) } -// straceSourceDynamic is busyboxSourceDynamic's counterpart for strace: it -// reports whether the operand straceCommandArgs would treat as the traced -// child command comes from a word this scan cannot resolve statically. -// straceChildIndex is shared with straceCommandArgs so this check walks -// strace's option grammar exactly once, rather than duplicating it and -// risking the two silently drifting apart. +// straceSourceDynamic reports whether the delegated argv loses information in +// the literal reconstruction passed to resolveCommandArgv. This includes both a +// dynamic child executable and dynamic env -S source below a literal env child. +// straceChildIndex is shared with straceCommandArgs so this check walks strace's +// option grammar exactly once rather than duplicating it. func straceSourceDynamic(args []*syntax.Word) bool { index, ok := straceChildIndex(wordTexts(args)) if !ok || index >= len(args) { return false } - return !isLiteralWord(args[index]) + if !isLiteralWord(args[index]) { + return true + } + return envSplitSourceDynamic(args[index:]) } // envArgumentStart returns the index just past an `env` program token, allowing @@ -870,6 +872,9 @@ func classifyInterpreterSource(payload string, language interpreterSourceLanguag if unreadable := powerShellSourceUnreadable(payload); unreadable != "" { return commandUnresolved } + if resolution, found := classifyPowerShellExpressionEvaluation(payload, depth); found { + return resolution + } } result := AnalysisResult{} analyzeInto(payload, &result, map[string]bool{}, depth) @@ -915,6 +920,66 @@ func powerShellSourceUnreadable(payload string) string { return "" } +// classifyPowerShellExpressionEvaluation handles the PowerShell edge that +// executes a string as a second PowerShell program. The shared POSIX reader can +// locate a simple Invoke-Expression/iex call, but it cannot infer source from an +// expression that constructs the argument. Only one literal single-quoted +// operand is therefore recursively classified; every other evaluator shape is +// unresolved and retains the network gate. +func classifyPowerShellExpressionEvaluation(payload string, depth int) (commandResolution, bool) { + file, err := syntax.NewParser().Parse(strings.NewReader(payload), "") + if err != nil { + return commandUnresolved, powerShellExpressionEvaluatorToken(payload) + } + + found := false + resolution := commandKnownLocal + syntax.Walk(file, func(node syntax.Node) bool { + call, ok := node.(*syntax.CallExpr) + if !ok || len(call.Args) == 0 || !isPowerShellExpressionEvaluator(wordText(call.Args[0])) { + return true + } + found = true + if len(call.Args) != 2 { + resolution = commandUnresolved + return true + } + parts := call.Args[1].Parts + if len(parts) != 1 { + resolution = commandUnresolved + return true + } + quoted, ok := parts[0].(*syntax.SglQuoted) + if !ok { + resolution = commandUnresolved + return true + } + nested := classifyInterpreterSource(quoted.Value, interpreterSourcePowerShell, depth+1) + if nested == commandKnownNetwork || resolution == commandKnownNetwork { + resolution = commandKnownNetwork + } else if nested == commandUnresolved { + resolution = commandUnresolved + } + return true + }) + return resolution, found +} + +func powerShellExpressionEvaluatorToken(payload string) bool { + for _, command := range fallbackCommandTokenInfo(payload) { + for _, token := range command { + if !token.quoted && isPowerShellExpressionEvaluator(token.value) { + return true + } + } + } + return false +} + +func isPowerShellExpressionEvaluator(token string) bool { + return strings.EqualFold(token, "Invoke-Expression") || strings.EqualFold(token, "iex") +} + func pythonModuleUsesNetwork(words []string) bool { for index := 0; index < len(words); index++ { if words[index] != "-m" || index+1 >= len(words) { diff --git a/internal/sandbox/analyzer_test.go b/internal/sandbox/analyzer_test.go index d987ac927..335c1eea5 100644 --- a/internal/sandbox/analyzer_test.go +++ b/internal/sandbox/analyzer_test.go @@ -133,6 +133,10 @@ func TestAnalyzeCommand(t *testing.T) { {name: "PowerShell slash command", script: `powershell /Command Invoke-WebRequest https://x.test`, network: true}, {name: "PowerShell abbreviated command", script: `pwsh -co iwr https://x.test`, network: true}, {name: "PowerShell local command", script: `pwsh /Command Get-ChildItem`, network: false}, + {name: "PowerShell expression evaluation", script: `powershell -Command "Invoke-Expression 'curl https://x.test'"`, network: true}, + {name: "pwsh expression evaluation alias", script: `pwsh -Command "iex 'git push origin main'"`, network: true}, + {name: "PowerShell dynamic expression evaluation", script: `powershell -Command 'iex $PAYLOAD'`, network: true}, + {name: "PowerShell local expression evaluation", script: `powershell -Command "iex 'Write-Output hello'"`, network: false}, {name: "PowerShell backtick command name fails closed", script: "powershell -Command 'Invoke`-WebRequest https://x.test'", network: true}, {name: "PowerShell backtick continuation fails closed", script: "powershell -Command 'Invoke`\n-WebRequest https://x.test'", network: true}, {name: "PowerShell abbreviated execution policy", script: `powershell -ep RemoteSigned curl https://x.test`, network: true}, @@ -200,6 +204,8 @@ func TestAnalyzeCommand(t *testing.T) { {name: "strace literal child stays classified on content", script: `strace true "not a program" https://x.test`, network: false}, {name: "strace ordinary env wrapper", script: `strace env curl https://x.test`, network: true}, {name: "strace env split wrapper", script: `strace env -S 'git push origin main'`, network: true}, + {name: "strace dynamic env split source", script: `PAYLOAD='curl https://x.test'; strace env -S "$PAYLOAD"`, network: true}, + {name: "strace literal env split source", script: `strace env -S 'printf ok'`, network: false}, {name: "strace ordinary env local control", script: `strace env true`, network: false}, {name: "git local commit", script: `git commit -m "local change"`, network: false}, // git's value-taking global options put their value in the NEXT token, so a diff --git a/internal/sandbox/engine_test.go b/internal/sandbox/engine_test.go index c5622c641..292df1b4f 100644 --- a/internal/sandbox/engine_test.go +++ b/internal/sandbox/engine_test.go @@ -106,6 +106,8 @@ func TestEvaluateLauncherResolutionContract(t *testing.T) { `strace --tips curl https://evil.test`, `strace env curl https://evil.test`, `strace env -S 'git push origin main'`, + `PAYLOAD='curl https://evil.test'; strace env -S "$PAYLOAD"`, + `PAYLOAD='curl https://evil.test'; strace env -S "$PAYLOAD" && "unterminated`, `busybox env curl https://evil.test`, `busybox env -S 'git push origin main'`, `busybox -- curl https://evil.test`, @@ -132,6 +134,8 @@ func TestEvaluateLauncherResolutionContract(t *testing.T) { `sh -c -- 'printf ok'`, `env -S 'sh -c' -- 'printf ok' && "unterminated`, `powershell -Command Get-ChildItem`, + `strace env -S 'printf ok'`, + `strace env -S 'printf ok' && "unterminated`, `strace --tips true`, `strace env true`, `busybox env true`, @@ -409,6 +413,12 @@ func TestEnginePromptsForReviewedUnparseableNetworkForms(t *testing.T) { // A block whose body names nothing recognisable is still unreadable, so it // is gated on the grammar rather than on spotting a network verb. `powershell -Command 'try { & $tool $target } catch {}'`, + `powershell -Command "Invoke-Expression 'curl https://evil.test'"`, + `powershell -Command "iex 'git push origin main'"`, + `pwsh -Command "Invoke-Expression 'git push origin main'"`, + `pwsh -Command "iex 'curl https://evil.test'"`, + `powershell -Command 'iex $PAYLOAD'`, + `pwsh -Command "Invoke-Expression ('curl ' + 'https://evil.test')"`, `for /f %%i in (list.txt) do %%i https://evil.test`, `for /f %i in (list.txt) do %~dpi https://evil.test`, `for /f %i in (list.txt) do call %i https://evil.test`, @@ -496,6 +506,8 @@ func TestEngineDoesNotPromptForNonNetworkCommandForms(t *testing.T) { `powershell -Command 'Write-Output hello'`, `powershell -Command 'Get-Process'`, `powershell -NoProfile -Command 'Get-ChildItem -Path .'`, + `powershell -Command "Invoke-Expression 'Write-Output hello'"`, + `pwsh -Command "iex 'Get-ChildItem -Path .'"`, `for /f %i in (list.txt) do type %i`, `for %i in (a b c) do copy %i backup\%i`, `for /f %i in (list.txt) do echo %i >> out.txt`,