diff --git a/webview-ui/src/utils/__tests__/command-validation.spec.ts b/webview-ui/src/utils/__tests__/command-validation.spec.ts index 29370c471f..a29f10181f 100644 --- a/webview-ui/src/utils/__tests__/command-validation.spec.ts +++ b/webview-ui/src/utils/__tests__/command-validation.spec.ts @@ -11,7 +11,7 @@ import { getSingleCommandDecision, CommandValidator, createCommandValidator, - containsSubshell, + containsDangerousSubstitution, } from "../command-validation" describe("Command Validation", () => { @@ -43,66 +43,6 @@ describe("Command Validation", () => { expect(parseCommand("diff <(sort f1) <(sort f2)")).toEqual(["diff", "sort f1", "sort f2"]) }) - it("detects additional subshell patterns", () => { - // Test $[] arithmetic expansion detection - expect(parseCommand("echo $[1 + 2]")).toEqual(["echo $[1 + 2]"]) - - // Verify containsSubshell detects all subshell patterns - expect(containsSubshell("echo $[1 + 2]")).toBe(true) // $[] arithmetic expansion - expect(containsSubshell("echo $((1 + 2))")).toBe(true) // $(()) arithmetic expansion - expect(containsSubshell("echo $(date)")).toBe(true) // $() command substitution - expect(containsSubshell("echo `date`")).toBe(true) // backtick substitution - expect(containsSubshell("diff <(sort f1) <(sort f2)")).toBe(true) // process substitution - expect(containsSubshell("echo hello")).toBe(false) // no subshells - }) - - it("detects subshell grouping patterns", () => { - // Basic subshell grouping with shell operators - expect(containsSubshell("(ls; rm file)")).toBe(true) - expect(containsSubshell("(cd /tmp && rm -rf *)")).toBe(true) - expect(containsSubshell("(command1 || command2)")).toBe(true) - expect(containsSubshell("(ls | grep test)")).toBe(true) - expect(containsSubshell("(sleep 10 & echo done)")).toBe(true) - - // Nested subshells - expect(containsSubshell("(cd /tmp && (rm -rf * || echo failed))")).toBe(true) - - // Multiple operators in subshell - expect(containsSubshell("(cmd1; cmd2 && cmd3 | cmd4)")).toBe(true) - - // Subshell with spaces - expect(containsSubshell("( ls ; rm file )")).toBe(true) - }) - - it("does NOT detect legitimate parentheses usage", () => { - // Function calls should not be flagged as subshells - expect(containsSubshell("myfunction(arg1, arg2)")).toBe(false) - expect(containsSubshell("func( arg1, arg2 )")).toBe(false) - - // Simple parentheses without operators - expect(containsSubshell("(simple text)")).toBe(false) - - // Parentheses in strings - expect(containsSubshell('echo "this (has) parentheses"')).toBe(false) - - // Empty parentheses - expect(containsSubshell("()")).toBe(false) - }) - - it("handles mixed subshell patterns", () => { - // Mixed subshell types - expect(containsSubshell("(echo $(date); rm file)")).toBe(true) - - // Subshell with command substitution - expect(containsSubshell("(ls `pwd`; echo done)")).toBe(true) - - // No subshells - expect(containsSubshell("echo hello world")).toBe(false) - - // Empty string - expect(containsSubshell("")).toBe(false) - }) - it("handles empty and whitespace input", () => { expect(parseCommand("")).toEqual([]) expect(parseCommand(" ")).toEqual([]) @@ -261,6 +201,120 @@ ls -la || echo "Failed"` expect(isAutoApprovedSingleCommand("npm test", [])).toBe(false) }) }) + + describe("containsDangerousSubstitution", () => { + it("detects parameter expansion with @P operator (prompt string expansion)", () => { + // This is the specific vulnerability from the report - @P can execute commands + expect(containsDangerousSubstitution('echo "${var1=aa\\140whoami\\140c}${var1@P}"')).toBe(true) + expect(containsDangerousSubstitution("echo ${var@P}")).toBe(true) + expect(containsDangerousSubstitution("result=${input@P}")).toBe(true) + expect(containsDangerousSubstitution("${somevar@P}")).toBe(true) + }) + + it("detects other dangerous parameter expansion operators", () => { + // @Q - Quote removal + expect(containsDangerousSubstitution("echo ${var@Q}")).toBe(true) + // @E - Escape sequence expansion + expect(containsDangerousSubstitution("echo ${var@E}")).toBe(true) + // @A - Assignment statement + expect(containsDangerousSubstitution("echo ${var@A}")).toBe(true) + // @a - Attribute flags + expect(containsDangerousSubstitution("echo ${var@a}")).toBe(true) + }) + + it("detects parameter assignments with octal escape sequences", () => { + // Octal \140 is backtick, which can execute commands + expect(containsDangerousSubstitution('echo "${var=\\140whoami\\140}"')).toBe(true) + expect(containsDangerousSubstitution('echo "${var:=\\140ls\\140}"')).toBe(true) + expect(containsDangerousSubstitution('echo "${var+\\140pwd\\140}"')).toBe(true) + expect(containsDangerousSubstitution('echo "${var:-\\140date\\140}"')).toBe(true) + expect(containsDangerousSubstitution('echo "${var:+\\140echo test\\140}"')).toBe(true) + expect(containsDangerousSubstitution('echo "${var:?\\140rm file\\140}"')).toBe(true) + // Test various octal patterns + expect(containsDangerousSubstitution('echo "${var=\\001\\140\\141}"')).toBe(true) + expect(containsDangerousSubstitution('echo "${var=\\777}"')).toBe(true) + }) + + it("detects parameter assignments with hex escape sequences", () => { + // Hex \x60 is backtick + expect(containsDangerousSubstitution('echo "${var=\\x60whoami\\x60}"')).toBe(true) + expect(containsDangerousSubstitution('echo "${var:=\\x60ls\\x60}"')).toBe(true) + expect(containsDangerousSubstitution('echo "${var+\\x60pwd\\x60}"')).toBe(true) + expect(containsDangerousSubstitution('echo "${var:-\\x60date\\x60}"')).toBe(true) + // Test various hex patterns + expect(containsDangerousSubstitution('echo "${var=\\x00\\x60\\x61}"')).toBe(true) + expect(containsDangerousSubstitution('echo "${var=\\xFF}"')).toBe(true) + }) + + it("detects parameter assignments with unicode escape sequences", () => { + // Unicode \u0060 is backtick + expect(containsDangerousSubstitution('echo "${var=\\u0060whoami\\u0060}"')).toBe(true) + expect(containsDangerousSubstitution('echo "${var:=\\u0060ls\\u0060}"')).toBe(true) + expect(containsDangerousSubstitution('echo "${var+\\u0060pwd\\u0060}"')).toBe(true) + expect(containsDangerousSubstitution('echo "${var:-\\u0060date\\u0060}"')).toBe(true) + // Test various unicode patterns + expect(containsDangerousSubstitution('echo "${var=\\u0000\\u0060\\u0061}"')).toBe(true) + expect(containsDangerousSubstitution('echo "${var=\\uFFFF}"')).toBe(true) + }) + + it("detects indirect variable references", () => { + // ${!var} performs indirect expansion which can be dangerous + expect(containsDangerousSubstitution("echo ${!var}")).toBe(true) + expect(containsDangerousSubstitution("result=${!indirect}")).toBe(true) + expect(containsDangerousSubstitution("${!prefix*}")).toBe(true) + expect(containsDangerousSubstitution("${!prefix@}")).toBe(true) + }) + + it("detects here-strings with command substitution", () => { + expect(containsDangerousSubstitution("cat <<<$(whoami)")).toBe(true) + expect(containsDangerousSubstitution("read <<<`date`")).toBe(true) + expect(containsDangerousSubstitution("grep pattern <<< $(ls)")).toBe(true) + expect(containsDangerousSubstitution("sort <<< `pwd`")).toBe(true) + }) + + it("does NOT flag safe parameter expansions", () => { + // Regular parameter expansions without dangerous operators + expect(containsDangerousSubstitution("echo ${var}")).toBe(false) + expect(containsDangerousSubstitution("echo ${var:-default}")).toBe(false) + expect(containsDangerousSubstitution("echo ${var:+alternative}")).toBe(false) + expect(containsDangerousSubstitution("echo ${#var}")).toBe(false) + expect(containsDangerousSubstitution("echo ${var%pattern}")).toBe(false) + expect(containsDangerousSubstitution("echo ${var#pattern}")).toBe(false) + expect(containsDangerousSubstitution("echo ${var/old/new}")).toBe(false) + expect(containsDangerousSubstitution("echo ${var^^}")).toBe(false) + expect(containsDangerousSubstitution("echo ${var,,}")).toBe(false) + expect(containsDangerousSubstitution("echo ${var:0:5}")).toBe(false) + + // Parameter assignments without escape sequences + expect(containsDangerousSubstitution('echo "${var=normal text}"')).toBe(false) + expect(containsDangerousSubstitution('echo "${var:-default value}"')).toBe(false) + expect(containsDangerousSubstitution('echo "${var:+alternative}"')).toBe(false) + + // Here-strings without command substitution + expect(containsDangerousSubstitution("cat << { + // Multiple dangerous patterns in one command + expect(containsDangerousSubstitution('echo "${var1=\\140ls\\140}${var1@P}" && ${!indirect}')).toBe(true) + // Nested patterns + expect(containsDangerousSubstitution('echo "${outer=${inner@P}}"')).toBe(true) + // Mixed with safe patterns + expect(containsDangerousSubstitution("echo ${safe:-default} ${dangerous@P}")).toBe(true) + }) + + it("detects the exact exploit from the security report", () => { + // The exact pattern reported in the vulnerability + const exploit = 'echo "${var1=aa\\140whoami\\140c}${var1@P}"' + expect(containsDangerousSubstitution(exploit)).toBe(true) + + // Variations of the exploit + expect(containsDangerousSubstitution('echo "${x=\\140id\\140}${x@P}"')).toBe(true) + expect(containsDangerousSubstitution('result="${cmd=\\x60pwd\\x60}${cmd@P}"')).toBe(true) + }) + }) }) /** @@ -771,6 +825,97 @@ describe("Unified Command Decision Functions", () => { expect(getCommandDecision("npm install && echo done", allowedCommands, deniedCommands)).toBe("auto_approve") }) + describe("dangerous substitution handling", () => { + it("prevents auto-approve for commands with dangerous parameter expansion", () => { + // Commands that would normally be auto-approved are blocked by dangerous patterns + expect(getCommandDecision("echo ${var@P}", allowedCommands, deniedCommands)).toBe("ask_user") + expect(getCommandDecision("echo hello", allowedCommands, deniedCommands)).toBe("auto_approve") // Safe version + + // Even with allowed prefix, dangerous patterns prevent auto-approval + expect(getCommandDecision("npm install ${var@P}", allowedCommands, deniedCommands)).toBe("ask_user") + expect( + getCommandDecision('echo "${var1=\\140whoami\\140c}${var1@P}"', allowedCommands, deniedCommands), + ).toBe("ask_user") + }) + + it("does NOT override auto_deny decisions with dangerous patterns", () => { + // If a command would be denied, dangerous patterns don't change that + expect(getCommandDecision("npm test ${var@P}", allowedCommands, deniedCommands)).toBe("auto_deny") + expect(getCommandDecision('npm test "${var=\\140ls\\140}"', allowedCommands, deniedCommands)).toBe( + "auto_deny", + ) + + // Regular denied commands without dangerous patterns + expect(getCommandDecision("npm test --coverage", allowedCommands, deniedCommands)).toBe("auto_deny") + }) + + it("prevents auto-approval for various dangerous substitution types", () => { + // Octal escape sequences + expect(getCommandDecision('echo "${var=\\140ls\\140}"', allowedCommands, deniedCommands)).toBe( + "ask_user", + ) + expect(getCommandDecision('npm run "${var:=\\140pwd\\140}"', allowedCommands, deniedCommands)).toBe( + "ask_user", + ) + + // Hex escape sequences + expect(getCommandDecision('echo "${var=\\x60whoami\\x60}"', allowedCommands, deniedCommands)).toBe( + "ask_user", + ) + + // Indirect variable references + expect(getCommandDecision("echo ${!var}", allowedCommands, deniedCommands)).toBe("ask_user") + + // Here-strings with command substitution + expect(getCommandDecision("cat <<<$(whoami)", allowedCommands, deniedCommands)).toBe("ask_user") + expect(getCommandDecision("read <<<`date`", allowedCommands, deniedCommands)).toBe("ask_user") + }) + + it("allows safe parameter expansions to follow normal rules", () => { + // Safe parameter expansions should follow normal allowlist/denylist rules + expect(getCommandDecision("echo ${var}", allowedCommands, deniedCommands)).toBe("auto_approve") + expect(getCommandDecision("echo ${var:-default}", allowedCommands, deniedCommands)).toBe("auto_approve") + expect(getCommandDecision("npm install ${package_name}", allowedCommands, deniedCommands)).toBe( + "auto_approve", + ) + + // Here-strings without command substitution are safe + expect(getCommandDecision("echo test <<<$var", allowedCommands, deniedCommands)).toBe("auto_approve") + }) + + it("handles command chains correctly with dangerous patterns", () => { + // If any part of a chain has dangerous substitution, prevent auto-approval + expect(getCommandDecision("npm install && echo ${var@P}", allowedCommands, deniedCommands)).toBe( + "ask_user", + ) + expect( + getCommandDecision('echo safe && echo "${var=\\140ls\\140}"', allowedCommands, deniedCommands), + ).toBe("ask_user") + + // But if chain would be denied, keep the deny decision + expect(getCommandDecision("npm test ${var@P} && echo safe", allowedCommands, deniedCommands)).toBe( + "auto_deny", + ) + expect(getCommandDecision("npm install && npm test ${var@P}", allowedCommands, deniedCommands)).toBe( + "auto_deny", + ) + + // Safe chains should still be auto-approved + expect(getCommandDecision("npm install && echo done", allowedCommands, deniedCommands)).toBe( + "auto_approve", + ) + }) + + it("handles the exact exploit from the security report", () => { + const exploit = 'echo "${var1=aa\\140whoami\\140c}${var1@P}"' + // Even though 'echo' is in the allowlist, the dangerous pattern prevents auto-approval + expect(getCommandDecision(exploit, allowedCommands, deniedCommands)).toBe("ask_user") + + // But if it were a denied command, it would still be denied + expect(getCommandDecision(`npm test ${exploit}`, allowedCommands, deniedCommands)).toBe("auto_deny") + }) + }) + it("returns auto_deny for commands with any sub-command auto-denied", () => { expect(getCommandDecision("npm test", allowedCommands, deniedCommands)).toBe("auto_deny") expect(getCommandDecision("npm install && npm test", allowedCommands, deniedCommands)).toBe("auto_deny") @@ -899,7 +1044,6 @@ describe("Unified Command Decision Functions", () => { expect(details.decision).toBe("auto_approve") expect(details.subCommands).toEqual(["npm install", "echo done"]) - expect(details.hasSubshells).toBe(false) expect(details.allowedMatches).toHaveLength(2) expect(details.deniedMatches).toHaveLength(2) @@ -912,12 +1056,10 @@ describe("Unified Command Decision Functions", () => { it("detects subshells correctly", () => { const details = validator.getValidationDetails("npm install $(echo test)") - expect(details.hasSubshells).toBe(true) expect(details.decision).toBe("auto_approve") // all commands are allowed // Test with denied prefix in subshell const detailsWithDenied = validator.getValidationDetails("npm install $(npm test)") - expect(detailsWithDenied.hasSubshells).toBe(true) expect(detailsWithDenied.decision).toBe("auto_deny") // npm test is denied }) diff --git a/webview-ui/src/utils/command-validation.ts b/webview-ui/src/utils/command-validation.ts index 700aed554b..16f73a317a 100644 --- a/webview-ui/src/utils/command-validation.ts +++ b/webview-ui/src/utils/command-validation.ts @@ -36,7 +36,7 @@ type ShellToken = string | { op: string } | { command: string } * * ## Command Processing Pipeline: * - * 1. **Subshell Detection**: Commands containing dangerous patterns like $(), ``, or (cmd1; cmd2) are flagged as security risks + * 1. **Dangerous Substitution Detection**: Commands containing dangerous patterns like ${var@P} are never auto-approved * 2. **Command Parsing**: Split chained commands (&&, ||, ;, |, &) into individual commands for separate validation * 3. **Pattern Matching**: For each individual command, find the longest matching prefix in both allowlist and denylist * 4. **Decision Logic**: Apply longest prefix match rule - more specific (longer) matches take precedence @@ -44,7 +44,7 @@ type ShellToken = string | { op: string } | { command: string } * * ## Security Considerations: * - * - **Subshell Protection**: Detects and blocks command injection attempts via command substitution, process substitution, and subshell grouping + * - **Dangerous Substitution Protection**: Detects dangerous parameter expansions and escape sequences that could execute commands * - **Chain Analysis**: Each command in a chain (cmd1 && cmd2) is validated separately to prevent bypassing via chaining * - **Case Insensitive**: All pattern matching is case-insensitive for consistent behavior across different input styles * - **Whitespace Handling**: Commands are trimmed and normalized before matching to prevent whitespace-based bypasses @@ -59,62 +59,51 @@ type ShellToken = string | { op: string } | { command: string } */ /** - * Detect subshell usage and command substitution patterns that could be security risks. - * - * Subshells allow executing commands in isolated environments and can be used to bypass - * command validation by hiding dangerous commands inside substitution patterns. + * Detect dangerous parameter substitutions that could lead to command execution. + * These patterns are never auto-approved and always require explicit user approval. * * Detected patterns: - * - $() - command substitution: executes command and substitutes output - * - `` - backticks (legacy command substitution): same as $() but older syntax - * - <() - process substitution (input): creates temporary file descriptor for command output - * - >() - process substitution (output): creates temporary file descriptor for command input - * - $(()) - arithmetic expansion: evaluates mathematical expressions (can contain commands) - * - $[] - arithmetic expansion (alternative syntax): same as $(()) but older syntax - * - (cmd1; cmd2) - subshell grouping: executes multiple commands in isolated subshell - * - * @param source - The command string to analyze for subshell patterns - * @returns true if any subshell patterns are detected, false otherwise - * - * @example - * ```typescript - * // Command substitution - executes 'date' and substitutes its output - * containsSubshell("echo $(date)") // true - * - * // Backtick substitution - legacy syntax for command substitution - * containsSubshell("echo `date`") // true - * - * // Process substitution - creates file descriptor for command output - * containsSubshell("diff <(sort f1)") // true - * - * // Arithmetic expansion - can contain command execution - * containsSubshell("echo $((1+2))") // true - * containsSubshell("echo $[1+2]") // true - * - * // Subshell grouping - executes commands in isolated environment - * containsSubshell("(ls; rm file)") // true - * containsSubshell("(cd /tmp && rm -rf *)") // true - * - * // Safe patterns that should NOT be flagged - * containsSubshell("func(arg1, arg2)") // false - function call, not subshell - * containsSubshell("echo hello") // false - no subshell patterns - * containsSubshell("(simple text)") // false - no shell operators in parentheses - * ``` + * - ${var@P} - Prompt string expansion (interprets escape sequences and executes embedded commands) + * - ${var@Q} - Quote removal + * - ${var@E} - Escape sequence expansion + * - ${var@A} - Assignment statement + * - ${var@a} - Attribute flags + * - ${var=value} with escape sequences - Can embed commands via \140 (backtick), \x60, or \u0060 + * - ${!var} - Indirect variable references + * - <<<$(...) or <<<`...` - Here-strings with command substitution + * + * @param source - The command string to analyze + * @returns true if dangerous substitution patterns are detected, false otherwise */ -export function containsSubshell(source: string): boolean { - // Check for command substitution, process substitution, and arithmetic expansion patterns - // These patterns allow executing commands and substituting their output, which can bypass validation - const commandSubstitutionPatterns = /(\$\()|`|(<\(|>\()|(\$\(\()|(\$\[)/.test(source) - - // Check for subshell grouping: parentheses containing shell command operators - // Pattern explanation: \( = literal opening paren, [^)]* = any chars except closing paren, - // [;&|]+ = one or more shell operators (semicolon, ampersand, pipe), [^)]* = any chars except closing paren, \) = literal closing paren - // This detects dangerous patterns like: (cmd1; cmd2), (cmd1 && cmd2), (cmd1 || cmd2), (cmd1 | cmd2), (cmd1 & cmd2) - // But avoids false positives like function calls: func(arg1, arg2) - no shell operators inside - const subshellGroupingPattern = /\([^)]*[;&|]+[^)]*\)/.test(source) - - // Return true if any subshell pattern is detected - return commandSubstitutionPatterns || subshellGroupingPattern +export function containsDangerousSubstitution(source: string): boolean { + // Check for dangerous parameter expansion operators that can execute commands + // ${var@P} - Prompt string expansion (interprets escape sequences and executes embedded commands) + // ${var@Q} - Quote removal + // ${var@E} - Escape sequence expansion + // ${var@A} - Assignment statement + // ${var@a} - Attribute flags + const dangerousParameterExpansion = /\$\{[^}]*@[PQEAa][^}]*\}/.test(source) + + // Check for parameter expansions with assignments that could contain escape sequences + // ${var=value} or ${var:=value} can embed commands via escape sequences like \140 (backtick) + // Also check for ${var+value}, ${var:-value}, ${var:+value}, ${var:?value} + const parameterAssignmentWithEscapes = + /\$\{[^}]*[=+\-?][^}]*\\[0-7]{3}[^}]*\}/.test(source) || // octal escapes + /\$\{[^}]*[=+\-?][^}]*\\x[0-9a-fA-F]{2}[^}]*\}/.test(source) || // hex escapes + /\$\{[^}]*[=+\-?][^}]*\\u[0-9a-fA-F]{4}[^}]*\}/.test(source) // unicode escapes + + // Check for indirect variable references that could execute commands + // ${!var} performs indirect expansion which can be dangerous with crafted variable names + const indirectExpansion = /\$\{![^}]+\}/.test(source) + + // Check for here-strings with command substitution + // <<<$(...) or <<<`...` can execute commands + const hereStringWithSubstitution = /<<<\s*(\$\(|`)/.test(source) + + // Return true if any dangerous pattern is detected + return ( + dangerousParameterExpansion || parameterAssignmentWithEscapes || indirectExpansion || hereStringWithSubstitution + ) } /** @@ -471,15 +460,15 @@ export type CommandDecision = "auto_approve" | "auto_deny" | "ask_user" * to resolve conflicts between allowlist and denylist patterns. * * **Decision Logic:** - * 1. **Subshell Protection**: If subshells ($() or ``) are present and denylist exists → auto-deny + * 1. **Dangerous Substitution Protection**: Commands with dangerous parameter expansions are never auto-approved * 2. **Command Parsing**: Split command chains (&&, ||, ;, |, &) into individual commands * 3. **Individual Validation**: For each sub-command, apply longest prefix match rule * 4. **Aggregation**: Combine decisions using "any denial blocks all" principle * * **Return Values:** - * - `"auto_approve"`: All sub-commands are explicitly allowed + * - `"auto_approve"`: All sub-commands are explicitly allowed and no dangerous patterns detected * - `"auto_deny"`: At least one sub-command is explicitly denied - * - `"ask_user"`: Mixed or no matches found, requires user decision + * - `"ask_user"`: Mixed or no matches found, requires user decision, or contains dangerous patterns * * **Examples:** * ```typescript @@ -487,6 +476,10 @@ export type CommandDecision = "auto_approve" | "auto_deny" | "ask_user" * getCommandDecision("git status", ["git"], []) * // Returns "auto_approve" * + * // Dangerous pattern - never auto-approved + * getCommandDecision('echo "${var@P}"', ["echo"], []) + * // Returns "ask_user" + * * // Longest prefix match - denial wins * getCommandDecision("git push origin", ["git"], ["git push"]) * // Returns "auto_deny" @@ -528,6 +521,11 @@ export function getCommandDecision( return "auto_deny" } + // Require explicit user approval for dangerous patterns + if (containsDangerousSubstitution(command)) { + return "ask_user" + } + // If all sub-commands are approved, approve the whole command if (decisions.every((decision) => decision === "auto_approve")) { return "auto_approve" @@ -681,10 +679,10 @@ export class CommandValidator { subCommands: string[] allowedMatches: Array<{ command: string; match: string | null }> deniedMatches: Array<{ command: string; match: string | null }> - hasSubshells: boolean + hasDangerousSubstitution: boolean } { const subCommands = parseCommand(command) - const hasSubshells = containsSubshell(command) + const hasDangerousSubstitution = containsDangerousSubstitution(command) const allowedMatches = subCommands.map((cmd) => ({ command: cmd, @@ -701,7 +699,7 @@ export class CommandValidator { subCommands, allowedMatches, deniedMatches, - hasSubshells, + hasDangerousSubstitution, } }