Fix command injection via newline and quoting/wildcard blocklist bypass #429
+29
−5
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,8 @@ class CommandManager { | |
| commandString = commandString.trim(); | ||
|
|
||
| // Define command separators - these are the operators that can chain commands | ||
| // Include newline variants to prevent newline-based blocklist bypass (#422) | ||
| const separators = ['\r\n', '\n', '\r', ';', '&&', '||', '|', '&']; | ||
|
|
||
| // This will store our extracted commands | ||
| const commands: string[] = []; | ||
|
|
@@ -137,7 +138,8 @@ class CommandManager { | |
| // We found a separator - extract the command before it | ||
| if (currentCmd.trim()) { | ||
| const baseCommand = this.extractBaseCommand(currentCmd.trim()); | ||
| // null means glob/invalid — push sentinel so validateCommand fails closed | ||
| commands.push(baseCommand !== null ? baseCommand : '__INVALID_COMMAND__'); | ||
| } | ||
|
Comment on lines
139
to
143
Contributor
There was a problem hiding this comment. Fail-closed sentinel is pushed for all
Previously these were silently dropped by the Disambiguate: only emit the sentinel when the segment actually contains glob/brace characters, and keep the silent-drop behavior for the other null causes. 🛡️ Proposed fix — gate sentinel on glob presence in the segment if (commandString.startsWith(separator, i)) {
// We found a separator - extract the command before it
if (currentCmd.trim()) {
- const baseCommand = this.extractBaseCommand(currentCmd.trim());
- // null means glob/invalid — push sentinel so validateCommand fails closed
- commands.push(baseCommand !== null ? baseCommand : '__INVALID_COMMAND__');
+ const trimmed = currentCmd.trim();
+ const baseCommand = this.extractBaseCommand(trimmed);
+ if (baseCommand !== null) {
+ commands.push(baseCommand);
+ } else if (/[*?\[\]{}]/.test(trimmed)) {
+ // Segment contained a glob/brace token we could not
+ // safely resolve — fail closed.
+ commands.push('__INVALID_COMMAND__');
+ }
+ // else: env-vars-only / variable-only / empty — drop silently.
}Apply the same treatment to the final-segment block at lines 159–163. Also applies to: 159-163 🤖 Prompt for AI Agents |
||
|
|
||
| // Move past the separator | ||
|
|
@@ -156,7 +158,8 @@ class CommandManager { | |
| // Don't forget to add the last command | ||
| if (currentCmd.trim()) { | ||
| const baseCommand = this.extractBaseCommand(currentCmd.trim()); | ||
| // null means glob/invalid — push sentinel so validateCommand fails closed | ||
| commands.push(baseCommand !== null ? baseCommand : '__INVALID_COMMAND__'); | ||
| } | ||
|
|
||
| // Remove duplicates and return | ||
|
|
@@ -217,6 +220,16 @@ class CommandManager { | |
| return null; | ||
| } | ||
|
|
||
| // Strip all quote characters so that concatenated fragments like | ||
| // "r"m, r"m", 'r''m' are normalized to the actual command name (#421) | ||
| firstToken = firstToken.replace(/["']/g, ''); | ||
|
Contributor
There was a problem hiding this comment. Suggestion: Stripping only quote characters is not enough to normalize shell command tokens: backslash escapes are still left intact. A blocked command can still be bypassed with input like Severity Level: Critical 🚨- ❌ Blocked commands executable via backslash-escaped names.
- ❌ start_process tool can't reliably enforce command blocklist.
- ⚠️ Potential privilege escalation via sudo, su commands.Steps of Reproduction ✅1. Start the MCP server defined in `src/server.ts` so the `start_process` tool is
registered (terminal tools section around lines 797–816 describes `start_process` as the
primary terminal tool).
2. Note that the default config in `src/config-manager.ts:109-155` includes `"sudo"` in
`blockedCommands`, so `sudo` should always be rejected by validation.
3. From a MCP client, call the `start_process` tool (handled by `handleStartProcess` in
`src/handlers/terminal-handlers.ts:13-16`, which calls `startProcess` in
`src/tools/improved-process-tools.ts:97-127`) with a command like `s\udo id`.
4. Inside `startProcess`, the call `commandManager.validateCommand(parsed.data.command)`
at `src/tools/improved-process-tools.ts:119-125` invokes `validateCommand` in
`src/command-manager.ts:242-276`, which calls `extractCommands(command)` at line 248;
`extractCommands` builds the token `s\udo` into `currentCmd` (lines 29-45) and passes it
to `extractBaseCommand` at `src/command-manager.ts:177-235`, where `firstToken` remains
`s\udo` except for quote-stripping via `firstToken = firstToken.replace(/["']/g, '');` at
line 225; since there is no backslash normalization, `validateCommand` compares `"s\\udo"`
against `blockedCommands` (lines 268-273), does not match `"sudo"`, returns `true`, and
`startProcess` executes the original `s\udo id` via `terminalManager.executeCommand` at
`src/tools/improved-process-tools.ts:170-175`, allowing the blocked `sudo` command to run
because the shell interprets `s\udo` as `sudo`.Fix in Cursor | Fix in VSCode Claude (Use Cmd/Ctrl + Click for best experience) Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** src/command-manager.ts
**Line:** 225:225
**Comment:**
*Security: Stripping only quote characters is not enough to normalize shell command tokens: backslash escapes are still left intact. A blocked command can still be bypassed with input like `r\m -rf /`, because validation compares `r\m` against the blocklist while the shell resolves it to `rm` at execution time. Normalize shell escapes (or reject escaped command-name tokens) before blocklist comparison.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+223
to
+225
Contributor
There was a problem hiding this comment. Suggestion: Quote normalization is incomplete because it only removes quote characters and does not normalize shell escape characters. On POSIX shells, an attacker can still obfuscate a blocked command with backslash escaping (for example Severity Level: Critical 🚨- ❌ Blocked commands bypassed via backslash-escaped names.
- ❌ Malicious shell commands can run on host system.
- ⚠️ BlockedCommands security guarantees undermined for escaped inputs.Steps of Reproduction ✅1. Invoke the MCP `start_process` tool via the server entrypoint `handleStartProcess` in
`src/handlers/terminal-handlers.ts:5-7`, passing args matching `StartProcessArgsSchema`
(`src/tools/schemas.ts:11-16`) with `command: "r\\m -rf /tmp/testdir"` and `timeout_ms`
set to any valid number.
2. `handleStartProcess` forwards the parsed args to `startProcess` in
`src/tools/improved-process-tools.ts:18-27`, which records telemetry and then calls
`commandManager.validateCommand(parsed.data.command)` at
`src/tools/improved-process-tools.ts:40`.
3. Inside `validateCommand` (`src/command-manager.ts:242-260`), `extractCommands`
(`src/command-manager.ts:11-21`) processes the string `r\\m -rf /tmp/testdir`. It never
hits a separator, so `currentCmd` becomes `r\\m -rf /tmp/testdir`. At the end of the loop
(`src/command-manager.ts:158-163`), `extractBaseCommand(currentCmd.trim())` is called,
which returns the first token `r\\m` (after env-var stripping and quote stripping at
`src/command-manager.ts:180-225`) and does not treat the backslash as special.
4. `extractCommands` returns `["r\\m"]`, so `validateCommand` sees `allCommands =
["r\\m"]` (`src/command-manager.ts:248-260`), does not trigger the `__INVALID_COMMAND__`
sentinel check (`src/command-manager.ts:262-265`), and compares `"r\\m"` against
`blockedCommands` from config (e.g. a user-configured `"rm"` entry in `blockedCommands`
loaded by `configManager.getConfig()` at `src/config-manager.ts:191-193`). Since `"r\\m"
!== "rm"`, validation passes and `terminalManager.executeCommand`
(`src/terminal-manager.ts:10-69`) runs the command via `/bin/sh -c "r\\m -rf
/tmp/testdir"` or similar; in POSIX shells (see `getShellSpawnArgs` at
`src/terminal-manager.ts:13-21,43-47`), `r\\m` is interpreted as `rm`, so the blocked `rm`
executes despite being on the blocklist.Fix in Cursor | Fix in VSCode Claude (Use Cmd/Ctrl + Click for best experience) Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** src/command-manager.ts
**Line:** 223:225
**Comment:**
*Security: Quote normalization is incomplete because it only removes quote characters and does not normalize shell escape characters. On POSIX shells, an attacker can still obfuscate a blocked command with backslash escaping (for example `r\m`), which the shell resolves to `rm` at execution time while the validator compares the unnormalized token and lets it pass. Normalize shell escapes before blocklist comparison (or parse using a shell-aware tokenizer).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix |
||
|
|
||
| // Reject commands containing glob/wildcard or brace-expansion characters | ||
| // to prevent blocklist bypass, e.g. /usr/bin/su*o or /usr/bin/s{udo,x} (#421) | ||
| if (/[*?\[\]{}]/.test(firstToken)) { | ||
| return null; | ||
| } | ||
|
|
||
| // strip path prefix so /usr/bin/sudo gets caught as "sudo" | ||
| const baseName = path.basename(firstToken); | ||
| return baseName.toLowerCase(); | ||
|
|
@@ -231,16 +244,27 @@ class CommandManager { | |
| // Get blocked commands from config | ||
| const config = await configManager.getConfig(); | ||
| const blockedCommands = config.blockedCommands || []; | ||
|
|
||
| // Extract all commands from the command string | ||
|
Contributor
There was a problem hiding this comment. 🔴 Architect Review — CRITICAL Blocked-command validation can still be bypassed via shell variable expansion of the executable name. For a command like Suggestion: In extractBaseCommand/extractCommands, treat dollar-prefixed tokens used in command position as unsafe rather than skipping them: either reject commands whose effective executable is variable-based (e.g. Fix in Cursor | Fix in VSCode Claude (Use Cmd/Ctrl + Click for best experience) Prompt for AI Agent 🤖This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** src/command-manager.ts
**Line:** 248:271
**Comment:**
*CRITICAL: Blocked-command validation can still be bypassed via shell variable expansion of the executable name. For a command like `export X=sudo; $X whoami`, extractCommands/extractBaseCommand skip the `$X` token and instead record `export` and `whoami`, so `sudo` is never compared against `blockedCommands` even though the shell executes `sudo`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix |
||
| const allCommands = this.extractCommands(command); | ||
|
|
||
| // If there are no commands extracted, fall back to base command | ||
| if (allCommands.length === 0) { | ||
| const baseCommand = this.getBaseCommand(command); | ||
| // Reject if the base command contains glob/wildcard/brace characters | ||
| // to prevent bypass via e.g. /usr/bin/su*o or s{udo,x} (#421) | ||
| if (/[*?\[\]{}]/.test(baseCommand)) { | ||
| return false; | ||
| } | ||
| return !blockedCommands.includes(baseCommand); | ||
| } | ||
|
|
||
| // Fail closed: if any segment failed extraction (e.g. contained a glob), | ||
| // reject the entire command rather than silently ignoring the bad segment. | ||
| if (allCommands.includes('__INVALID_COMMAND__')) { | ||
| return false; | ||
|
Comment on lines
+264
to
+265
Contributor
There was a problem hiding this comment. Suggestion: The new fail-closed sentinel check rejects commands whenever Severity Level: Major
|
||
| } | ||
|
|
||
| // Check if any of the extracted commands are in the blocked list | ||
| for (const cmd of allCommands) { | ||
| if (blockedCommands.includes(cmd)) { | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Treating every
nullfrom command extraction as__INVALID_COMMAND__causes legitimate command chains to be rejected, becauseextractBaseCommandreturnsnullfor non-malicious segments like env-only assignments (for exampleFOO=bar && echo ok). Only map to an invalid sentinel when the token is explicitly unsafe (glob/brace pattern), and skip segments that simply do not represent an executable command. [logic error]Severity Level: Major⚠️
Steps of Reproduction ✅
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖