diff --git a/internal/sandbox/analyzer.go b/internal/sandbox/analyzer.go index 9509c5a81..d674dd08e 100644 --- a/internal/sandbox/analyzer.go +++ b/internal/sandbox/analyzer.go @@ -65,6 +65,76 @@ func AnalyzeCommand(script string) AnalysisResult { return result } +// astCommandFields parses command with the shell parser and returns each simple +// command as its literal field slice (program + args as text), resolving the +// real command positions across quoting, command substitution, subshells, and +// newline separators — the constructs the hand-written splitter in +// safe_command.go mis-handles (issue #473). It returns nil when the command +// cannot be parsed (e.g. a Windows cmd.exe string), so callers fall through to +// the regex path rather than hard-blocking. +func astCommandFields(command string) [][]string { + file, err := syntax.NewParser().Parse(strings.NewReader(command), "") + if err != nil { + return nil + } + var commands [][]string + syntax.Walk(file, func(node syntax.Node) bool { + call, ok := node.(*syntax.CallExpr) + if !ok || len(call.Args) == 0 { + return true + } + // EVERY word must be a static literal, not just the program name. + // wordText keeps only the literal/quoted parts of a word and silently + // drops expansions, so a dynamic word anywhere in the call reconstructs + // to something the shell will never run: `$(printf foo)vim` (runs as + // `foovim`) collapses to "vim", and `git $(printf foo)rebase -i` (runs + // as `git foorebase -i`, non-interactive) collapses to `git rebase -i` + // and fabricates an interactive match. Since the runtime value of an + // expansion is unknowable here, skip the whole call rather than classify + // a lossy reconstruction. Skipping is the safe direction: the + // hand-written passes above already ran, and a missed detection falls + // through to the normal permission prompt instead of hard-blocking a + // command the user never wrote. + for _, word := range call.Args { + if !isLiteralWord(word) { + return true + } + } + fields := make([]string, 0, len(call.Args)) + for _, word := range call.Args { + fields = append(fields, wordText(word)) + } + commands = append(commands, fields) + return true + }) + return commands +} + +// isLiteralWord reports whether every part of word is a static literal (bare or +// quoted). A word containing a command substitution, parameter/arithmetic +// expansion, process substitution, etc. is dynamic — its runtime value is +// unknown, so its wordText (a partial literal) must not be trusted as a program +// name. +func isLiteralWord(word *syntax.Word) bool { + if word == nil { + return false + } + for _, part := range word.Parts { + switch typed := part.(type) { + case *syntax.Lit, *syntax.SglQuoted: + case *syntax.DblQuoted: + for _, inner := range typed.Parts { + if _, ok := inner.(*syntax.Lit); !ok { + return false + } + } + default: + return false + } + } + return true +} + // analyzeInto parses script and folds its interactive/destructive/network usage // into result, sharing seen so program names are de-duplicated across recursion. func analyzeInto(script string, result *AnalysisResult, seen map[string]bool, depth int) { diff --git a/internal/sandbox/safe_command.go b/internal/sandbox/safe_command.go index d6c2c8ea3..02f618824 100644 --- a/internal/sandbox/safe_command.go +++ b/internal/sandbox/safe_command.go @@ -219,9 +219,65 @@ func DetectInteractiveCommand(command string, goos string) InteractiveCommandRes } } + // AST second opinion (issue #473): the hand-written segment split above + // mis-handles interactive programs hidden by unusual quoting, command + // substitution, subshells, or newline separators. Re-extract the real simple + // commands from the parsed shell tree and apply the SAME full pipeline + // (interactive segments, sh -c payload recursion, per-program lookup), so a + // bypass is caught while every program stays classified exactly as before + // (ssh with a trailing command, python -c, etc. remain allowed). A command + // 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 { + return result + } + } + return InteractiveCommandResult{} } +// inspectCommandFields applies the full interactive-detection pipeline to one +// already-split command's fields: the multi-word interactive segments, then the +// `sh -c ` recursion, then the per-program lookup with its +// 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) { + body := strings.ToLower(commandBody(fields)) + for _, seg := range interactiveSegments { + if body == seg.match || strings.HasPrefix(body, seg.match+" ") { + suggestion := seg.suggestion + if goos == "windows" && seg.windowsSuggestion != "" { + suggestion = seg.windowsSuggestion + } + return InteractiveCommandResult{Interactive: true, Command: seg.command, Reason: seg.reason, Suggestion: suggestion}, true + } + } + first := firstProgram(fields) + if first == "" { + return InteractiveCommandResult{}, false + } + if payload := shellDashCPayload(first, fields); payload != "" { + if inner := DetectInteractiveCommand(payload, goos); inner.Interactive { + return inner, true + } + return InteractiveCommandResult{}, false + } + program, ok := interactivePrograms[first] + if !ok || (program.windowsOnly && goos != "windows") { + return InteractiveCommandResult{}, false + } + if hasNonInteractiveFlag(first, fields) { + return InteractiveCommandResult{}, false + } + suggestion := program.suggestion + if goos == "windows" && program.windowsSuggestion != "" { + suggestion = program.windowsSuggestion + } + return InteractiveCommandResult{Interactive: true, Command: first, Reason: program.reason, Suggestion: suggestion}, true +} + // wrapperPrograms are launcher prefixes that precede the real program. After // one of these we keep scanning for the actual executable. var wrapperPrograms = map[string]bool{ diff --git a/internal/sandbox/safe_command_test.go b/internal/sandbox/safe_command_test.go index 3e817b66f..15a2c5d6f 100644 --- a/internal/sandbox/safe_command_test.go +++ b/internal/sandbox/safe_command_test.go @@ -406,3 +406,87 @@ func TestDetectInteractiveMongoEvalAndFullPaths(t *testing.T) { } } } + +// 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 +// second opinion must catch them (issue #473). +func TestDetectInteractiveCommandCatchesParserBypasses(t *testing.T) { + for _, cmd := range []string{ + "echo hi\nvim file.txt", // newline separator collapsed to a space by the regex path + "{ vim file.txt; }", // brace group hides the real program position + } { + got := DetectInteractiveCommand(cmd, "linux") + if !got.Interactive { + t.Errorf("DetectInteractiveCommand(%q) = not interactive, want caught (parser bypass)", cmd) + continue + } + if got.Command != "vim" { + t.Errorf("DetectInteractiveCommand(%q).Command = %q, want vim", cmd, got.Command) + } + } +} + +// The AST second opinion must not flag an interactive program NAME that appears +// only inside a quoted argument (not a real command position) — the parser +// distinguishes program from argument, so this must stay non-interactive. +func TestDetectInteractiveCommandNoFalsePositiveOnQuotedArgument(t *testing.T) { + got := DetectInteractiveCommand(`echo "please run vim later"`, "linux") + if got.Interactive { + t.Fatalf("interactive name inside a quoted argument must not be flagged, got %#v", got) + } +} + +// The AST second opinion must run the FULL pipeline — multi-word interactive +// segments and sh -c payload recursion, not just the per-program lookup — so a +// grouped/nested bypass is caught; and it must not fabricate a program name from +// a command substitution concatenated with a literal (issue #473, review). +func TestDetectInteractiveCommandASTPipelineBoundaries(t *testing.T) { + interactive := []struct{ cmd, wantCmd string }{ + {"{ git rebase -i HEAD~1; }", "git rebase -i"}, // multi-word segment in a brace group + {"{ sh -c 'vim file.txt'; }", "vim"}, // sh -c payload in a brace group + } + for _, tc := range interactive { + got := DetectInteractiveCommand(tc.cmd, "linux") + if !got.Interactive || got.Command != tc.wantCmd { + t.Errorf("DetectInteractiveCommand(%q) = %+v, want interactive Command=%q", tc.cmd, got, tc.wantCmd) + } + } +} + +// The AST pass must not fabricate a program name from a dynamic (non-literal) +// program word: `$(printf '%s' foo)vim` runs as `foovim`, not vim. Tested at the +// extractor so the hand-written pass (which pre-empts DetectInteractiveCommand +// with its own, separate handling of that construct) does not mask the guard. +func TestAstCommandFieldsSkipsDynamicProgram(t *testing.T) { + for _, fields := range astCommandFields("$(printf '%s' foo)vim file.txt") { + if firstProgram(fields) == "vim" { + t.Fatalf("astCommandFields fabricated a vim command from a substitution: %v", fields) + } + } +} + +// The literalness guard covers every word, not just the program name. A dynamic +// ARGUMENT is dropped by wordText the same way a dynamic program word is, so +// `git $(printf foo)rebase -i HEAD~1` (which runs as `git foorebase -i`, a +// non-existent and non-interactive subcommand) would otherwise be reconstructed +// as `git rebase -i` and blocked — a false positive on a command the user never +// wrote. Asserted through DetectInteractiveCommand as well as the extractor, +// because the whole point is what the caller ends up blocking. +func TestAstCommandFieldsSkipsDynamicArgument(t *testing.T) { + const dynamic = "{ git $(printf foo)rebase -i HEAD~1 ; }" + for _, fields := range astCommandFields(dynamic) { + if firstProgram(fields) == "git" { + t.Fatalf("astCommandFields reconstructed a git command from a dynamic argument: %v", fields) + } + } + if got := DetectInteractiveCommand(dynamic, "linux"); got.Interactive { + t.Errorf("DetectInteractiveCommand(%q) = %+v, want not interactive", dynamic, got) + } + // The genuine form must still be detected — the guard skips lossy + // reconstructions, it does not weaken real detection. + const genuine = "{ git rebase -i HEAD~1 ; }" + if got := DetectInteractiveCommand(genuine, "linux"); !got.Interactive || got.Command != "git rebase -i" { + t.Errorf("DetectInteractiveCommand(%q) = %+v, want interactive Command=%q", genuine, got, "git rebase -i") + } +}