harden(local-agents): close cmd.exe prompt-injection + POSIX detect fallback
Addresses review feedback on the Windows detection PR.
1. Command injection (the important one). spawnAgent ran .cmd/.bat shims via a
hand-rolled `spawn('cmd.exe', ['/d','/s','/c', shim, ...args])`. cmd.exe
re-parses its tail with its own rules and ignores `\"`-escaping, so an
adversarial prompt (claude `-p <prompt>`, codex `exec <prompt>`) containing
`"` + `& | > %VAR%` broke out and executed — the BatBadBut / CVE-2024-1874
class. Verified with a `x" & echo PWNED` payload: it ran.
Fix: use Node's own safe `.cmd` handling — `spawn(shim, args, {shell:true})`.
On Node >=18.20.2 / >=20.12.2 / >=21 (CVE-2024-27980) Node escapes each arg
for cmd, so the whole prompt lands as ONE literal argument and cannot break
out (verified on Node 24: the same payload arrived intact in %1). .exe/POSIX
still spawn directly with shell:false.
2. POSIX detection regression. Detection gated on resolveBin() returning a
value; on a bash-less/busybox image where both `command -v` and `which` are
absent, an installed agent flipped to "not installed". Fix: on POSIX fall
back to the bare name (`resolveBin(bin) || (isWindows ? undefined : bin)`) —
the OS resolves PATH at exec time and the ENOENT branch still catches
genuinely-absent binaries. Windows keeps resolveBin authoritative.
Typecheck clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
1 parent
a836e61adb
commit
bf50be9971
1 file changed
+22
-13
+22
-13
@@ -223,26 +223,31 @@ function resolveBin(bin: string): string | undefined {
|
|||||||
/**
|
/**
|
||||||
* Windows-safe launcher for a resolved agent binary.
|
* Windows-safe launcher for a resolved agent binary.
|
||||||
*
|
*
|
||||||
* A `.cmd`/`.bat` shim can't be spawned directly (shell:false throws EINVAL/ENOEXEC). The common
|
* A `.cmd`/`.bat` shim can't be spawned with shell:false (Node throws EINVAL on Windows) — it must
|
||||||
* "fix" — `spawn(bin, args, { shell: true })` — is UNSAFE here: runLocalAgent puts the (untrusted)
|
* run through cmd.exe. But hand-rolling `spawn('cmd.exe', ['/d','/s','/c', shim, ...args])` is
|
||||||
* prompt straight into argv (claude `-p <prompt>`, codex `exec … <prompt>`), and shell:true would
|
* UNSAFE: cmd.exe re-parses its command tail with its OWN rules and does not honor `\"`-escaping, so
|
||||||
* route that prompt through cmd.exe where `&`, `|`, `>`, `%VAR%` become metacharacters → command
|
* the (untrusted, adversarial) prompt reaching argv — claude `-p <prompt>`, codex `exec <prompt>` —
|
||||||
* injection in a security tool. Instead we invoke cmd.exe explicitly with an ARGV ARRAY
|
* can contain `"` + `& | > %VAR%` and break out to execute (the BatBadBut / CVE-2024-1874 class;
|
||||||
* (`cmd.exe /d /s /c <shim> <arg1> <arg2> …`) and shell:false. Node hands each element to the child
|
* verified: a `x" & echo PWNED` payload ran).
|
||||||
* as a distinct argv entry with no shell re-parsing, so cmd.exe resolves the `.cmd` association while
|
*
|
||||||
* the prompt is never interpreted. Real `.exe`/POSIX binaries spawn directly (no wrapper).
|
* The safe route is Node's OWN `.cmd` handling: `spawn(shim, args, { shell: true })`. On Node
|
||||||
|
* >=18.20.2 / >=20.12.2 / >=21 (CVE-2024-27980 fix) Node escapes each arg for cmd, so the whole
|
||||||
|
* prompt lands as a SINGLE literal argument and cannot break out (verified on Node 24: the same
|
||||||
|
* payload arrived intact in %1). Real `.exe`/POSIX binaries spawn directly with shell:false.
|
||||||
*/
|
*/
|
||||||
function spawnAgent(resolvedBin: string, args: string[], options: import('child_process').SpawnOptions): import('child_process').ChildProcess {
|
function spawnAgent(resolvedBin: string, args: string[], options: import('child_process').SpawnOptions): import('child_process').ChildProcess {
|
||||||
if (needsShell(resolvedBin)) {
|
if (needsShell(resolvedBin)) {
|
||||||
return spawn('cmd.exe', ['/d', '/s', '/c', resolvedBin, ...args], { ...options, shell: false });
|
// shell:true lets Node run the .cmd via cmd.exe WITH its CVE-2024-27980 arg-escaping (safe);
|
||||||
|
// the manual cmd.exe wrapper bypassed that escaping and was injectable.
|
||||||
|
return spawn(resolvedBin, args, { ...options, shell: true });
|
||||||
}
|
}
|
||||||
return spawn(resolvedBin, args, { ...options, shell: false });
|
return spawn(resolvedBin, args, { ...options, shell: false });
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* True when the resolved binary is a Windows `.cmd`/`.bat` shim (npm global installs land as these).
|
* True when the resolved binary is a Windows `.cmd`/`.bat` shim (npm global installs land as these).
|
||||||
* Such a shim can't be spawned directly — it must go through cmd.exe (see spawnAgent, which does so
|
* Such a shim can't be spawned with shell:false — it must go through cmd.exe (see spawnAgent, which
|
||||||
* SAFELY via an argv array rather than shell:true). `.exe` and POSIX binaries return false.
|
* does so SAFELY via Node's own shell:true `.cmd` arg-escaping). `.exe` and POSIX binaries return false.
|
||||||
*/
|
*/
|
||||||
function needsShell(resolvedBin: string): boolean {
|
function needsShell(resolvedBin: string): boolean {
|
||||||
return isWindows && /\.(cmd|bat)$/i.test(resolvedBin);
|
return isWindows && /\.(cmd|bat)$/i.test(resolvedBin);
|
||||||
@@ -266,9 +271,13 @@ function detectOne(spec: AgentSpec): Promise<AgentDetection> {
|
|||||||
id: spec.id, label: spec.label, vendor: spec.vendor, bin: spec.bin,
|
id: spec.id, label: spec.label, vendor: spec.vendor, bin: spec.bin,
|
||||||
blurb: spec.blurb, invokeHint: spec.invokeHint,
|
blurb: spec.blurb, invokeHint: spec.invokeHint,
|
||||||
};
|
};
|
||||||
const resolved = resolveBin(spec.bin);
|
// POSIX fallback: if neither `command -v` nor `which` resolved it (e.g. a bash-less/busybox image
|
||||||
|
// where both are absent) still try the bare name — the OS resolves it against PATH at exec time,
|
||||||
|
// and the ENOENT branch below correctly flips a genuinely-absent binary to not-installed. Windows
|
||||||
|
// keeps `resolved` authoritative: a bare npm `.cmd` shim name has no safe direct spawn.
|
||||||
|
const resolved = resolveBin(spec.bin) || (isWindows ? undefined : spec.bin);
|
||||||
return new Promise((resolve) => {
|
return new Promise((resolve) => {
|
||||||
// Not on PATH at all → genuinely not installed.
|
// Not resolvable at all → genuinely not installed.
|
||||||
if (!resolved) {
|
if (!resolved) {
|
||||||
resolve({ ...base, installed: false, authed: false, ready: false });
|
resolve({ ...base, installed: false, authed: false, ready: false });
|
||||||
return;
|
return;
|
||||||
|
|||||||
Reference in new issue
Block a user