From 9a62180d89a38d0ba8dcae5ea09ac0eeefb1a744 Mon Sep 17 00:00:00 2001 From: Sebastion Date: Wed, 25 Mar 2026 05:12:48 +0000 Subject: [PATCH] fix: replace vulnerable double-quote shell escaping with POSIX single-quote method MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The hand-rolled regex-based quoting in defaultExecutor wraps arguments in double quotes but only escapes " and \. Inside double quotes, $(), backticks, and other shell metacharacters remain live, enabling command injection when useShell=true is used with attacker-influenced arguments (e.g. scheme, projectPath flowing through detectPlatformFromScheme). Replace with shellEscapeArg() using the standard POSIX single-quote escaping technique (same as Python shlex.quote). Single-quoted strings have zero interpretation by the shell — no variable expansion, no command substitution, no metacharacter processing. CWE-78 --- src/utils/__tests__/shell-escape.test.ts | 70 ++++++++++++++++++++++++ src/utils/command.ts | 13 +---- src/utils/shell-escape.ts | 15 +++++ 3 files changed, 88 insertions(+), 10 deletions(-) create mode 100644 src/utils/__tests__/shell-escape.test.ts create mode 100644 src/utils/shell-escape.ts diff --git a/src/utils/__tests__/shell-escape.test.ts b/src/utils/__tests__/shell-escape.test.ts new file mode 100644 index 00000000..9b72ab82 --- /dev/null +++ b/src/utils/__tests__/shell-escape.test.ts @@ -0,0 +1,70 @@ +import { describe, it, expect } from 'vitest'; +import { shellEscapeArg } from '../shell-escape.ts'; + +describe('shellEscapeArg', () => { + it('wraps a simple string in single quotes', () => { + expect(shellEscapeArg('hello')).toBe("'hello'"); + }); + + it('returns empty single-quoted string for empty input', () => { + expect(shellEscapeArg('')).toBe("''"); + }); + + it('escapes embedded single quotes using the POSIX technique', () => { + expect(shellEscapeArg("it's")).toBe("'it'\\''s'"); + }); + + it('handles multiple single quotes', () => { + expect(shellEscapeArg("a'b'c")).toBe("'a'\\''b'\\''c'"); + }); + + it('passes through double quotes safely inside single quotes', () => { + expect(shellEscapeArg('say "hi"')).toBe('\'say "hi"\''); + }); + + it('neutralises dollar-sign variable expansion', () => { + const escaped = shellEscapeArg('$HOME'); + expect(escaped).toBe("'$HOME'"); + }); + + it('neutralises backtick command substitution', () => { + const escaped = shellEscapeArg('`id`'); + expect(escaped).toBe("'`id`'"); + }); + + it('neutralises $() command substitution', () => { + const escaped = shellEscapeArg('$(whoami)'); + expect(escaped).toBe("'$(whoami)'"); + }); + + it('neutralises semicolon command chaining', () => { + const escaped = shellEscapeArg('foo; rm -rf /'); + expect(escaped).toBe("'foo; rm -rf /'"); + }); + + it('handles newlines (cannot break out of single quotes)', () => { + const escaped = shellEscapeArg('line1\nline2'); + expect(escaped).toBe("'line1\nline2'"); + }); + + it('handles backslashes', () => { + const escaped = shellEscapeArg('path\\to\\file'); + expect(escaped).toBe("'path\\to\\file'"); + }); + + it('handles pipe and redirection metacharacters', () => { + const escaped = shellEscapeArg('a | b > c < d'); + expect(escaped).toBe("'a | b > c < d'"); + }); + + it('handles a realistic malicious scheme name (CWE-78 PoC)', () => { + // An attacker might supply this as a scheme parameter via MCP tool call + const malicious = '$(touch /tmp/pwned)'; + const escaped = shellEscapeArg(malicious); + // The result should be a valid single-quoted string that cannot execute $(touch) + expect(escaped).toBe("'$(touch /tmp/pwned)'"); + // Verify no unquoted regions exist + expect(escaped.startsWith("'")).toBe(true); + expect(escaped.endsWith("'")).toBe(true); + }); +}); diff --git a/src/utils/command.ts b/src/utils/command.ts index 9d85fb3d..7bfc1032 100644 --- a/src/utils/command.ts +++ b/src/utils/command.ts @@ -13,6 +13,7 @@ import { spawn } from 'child_process'; import { createWriteStream, existsSync } from 'fs'; import { tmpdir as osTmpdir } from 'os'; import { log } from './logger.ts'; +import { shellEscapeArg } from './shell-escape.ts'; import type { FileSystemExecutor } from './FileSystemExecutor.ts'; import type { CommandExecutor, CommandResponse, CommandExecOptions } from './CommandExecutor.ts'; @@ -41,16 +42,8 @@ async function defaultExecutor( let escapedCommand = command; if (useShell) { // For shell execution, we need to format as ['/bin/sh', '-c', 'full command string'] - const commandString = command - .map((arg) => { - // Shell metacharacters that require quoting: space, quotes, equals, dollar, backticks, semicolons, pipes, etc. - if (/[\s,"'=$`;&|<>(){}[\]\\*?~]/.test(arg) && !/^".*"$/.test(arg)) { - // Escape all quotes and backslashes, then wrap in double quotes - return `"${arg.replace(/(["\\])/g, '\\$1')}"`; - } - return arg; - }) - .join(' '); + // Use POSIX single-quote escaping for each argument to prevent injection + const commandString = command.map((arg) => shellEscapeArg(arg)).join(' '); escapedCommand = ['/bin/sh', '-c', commandString]; } diff --git a/src/utils/shell-escape.ts b/src/utils/shell-escape.ts new file mode 100644 index 00000000..bb61a0e1 --- /dev/null +++ b/src/utils/shell-escape.ts @@ -0,0 +1,15 @@ +/** + * POSIX-safe shell argument escaping. + * + * Wraps a string in single quotes and escapes any embedded single quotes + * using the standard `'\''` technique. This is the safest way to pass + * arbitrary strings as arguments to `/bin/sh -c` commands. + * + * @param arg The argument to escape for safe shell interpolation + * @returns A single-quoted, safely escaped string + */ +export function shellEscapeArg(arg: string): string { + // Replace each single quote with: end current quote, escaped single quote, start new quote + // Then wrap the whole thing in single quotes + return "'" + arg.replace(/'/g, "'\\''") + "'"; +}