enhance(#3617): one canonical Windows binary resolver in the platform seam (epic #3411 Phase 1) (#3621)
* feat(#3411): one canonical Windows binary resolver in the platform seam CONTEXT.md declares src/shell-command-projection.cts the single OS-facing seam, but Windows binary resolution had grown four divergent implementations outside it. #3445 folded two of them together — inside gsd-core/bin/gsd-tools.cjs, not the seam — so the declaration stayed untrue and execTool still had no handling at all. Lift the resolver into the seam as resolveExecutableBinary, and export the half that actually executes as projectSpawnInvocation: CreateProcess cannot run a .cmd/.bat, so the cmd.exe mediation is inseparable from the lookup and splitting them is how the copies accumulated. cmd.exe is invoked with an explicit argv array, never shell:true — CVE-2024-27980's vector and Node 26's DEP0190. execTool now resolves on win32. POSIX is a strict no-op by construction, which matters: execTool rates CRITICAL blast radius (167 symbols, 53 files). gsd-tools.cjs deletes its private scan and its private mediation and delegates. Two semantics grown beyond #3445's resolver, both additive: a name already carrying a PATHEXT-listed extension is tried as-is before the append loop, and a suffix outside PATHEXT is not treated as an extension. Refs #3411 * fix(#3411): keep mediating a declared .cmd that PATH resolution misses Standards review caught a narrowing against the code this replaces. gsd-tools.cjs computed `target = resolveSpawnBinary(binary) || binary` and keyed the shim test on `target`, so a declared .cmd mediated whether or not PATH resolution found it. That is load-bearing: resolveExecutableBinary scans PATH only, while `cmd.exe /c` also finds a batch file in the current directory. Mediation now keys on the target — resolved path, else declared name. The ENOENT contract still holds for BARE unresolved names, which is the case it was written for. P9/P10 pin both halves. Spec review found E1/E2/E3/E5 promised by 50-test-matrix.md but never written; added. E3 is the integration proof that the CVE-relevant mediation fires through execTool, not only through projectSpawnInvocation in isolation. Also adds the CONTEXT.md glossary entry for the seam's new resolution ownership (a PR gate) and the changeset fragment. Refs #3411 * fix(#3617): pass mediated cmd.exe arguments verbatim so metacharacters cannot inject The isolated security pass found the mediation shape carried an argument-injection surface. libuv's quote_cmd_arg force-quotes an argv element only when it contains a space, tab, or quote — never for a cmd metacharacter — and cmd.exe re-parses everything after /c. So an arg of a&calc arrived unquoted and cmd ran calc. Node's own CVE-2024-27980 escaping cannot help: it fires only when the spawned FILE is the .bat/.cmd, and here the file is cmd.exe. Caret-escaping is not a fix. It is correct only when libuv does not quote, and libuv quotes whenever the arg also contains a space — no per-arg transform is right in both cases. So build the command line and pass it through verbatim, the shape Rust's std adopted for the sibling CVE-2024-24576: one outer quote pair that cmd /c strips, every token inside force-quoted, embedded quotes doubled. An argument containing CR or LF is refused rather than mediated — a newline cannot be represented in a Windows command line, so mediating would silently truncate. Failing visibly is correct. Known limit, documented at the seam: %VAR% still expands inside a /c string and has no escape outside a batch file. That is information disclosure, not arbitrary execution, and is the same limit Rust's std documents. This was byte-for-byte the shape #3445 shipped, so the fix closes it for the reviewer-lane spawn path too, not only for execTool's newly reachable route. Refs #3411 * docs(#3617): document the subprocess-execution security posture Adds Layer 4 to the security model: why GSD never uses shell:true for binary invocation (CVE-2024-27980, Node 26 DEP0190), why resolution is explicit and never tries the bare name on Windows (the npm extensionless-shim trap behind #3275), and why .cmd/.bat mediation builds a verbatim force-quoted command line rather than relying on default escaping — Node's own CVE protection cannot fire once the started program is cmd.exe. The residual %VAR% expansion limit is stated plainly under Trade-offs rather than left implicit: it is information disclosure, not arbitrary execution, and callers passing untrusted text to a Windows .cmd should not assume the value arrives byte-identical. Docs-only; no code change. Refs #3411 * chore(#3617): backfill changeset pr number 3621 * fix(#3617): read PATH, PATHEXT and ComSpec case-insensitively The Windows CI lane on #3621 failed E5, and the root cause was a defect in the implementation, not the assertion. Windows names the variable Path, not PATH. process.env is a case-insensitive proxy, so process.env.PATH works — but execTool builds { ...process.env, ...opts.env } whenever a caller supplies opts.env, and spreading discards the proxy while keeping the OS's actual casing. The exact-case env['PATH'] lookup then returned undefined, the PATH scan saw zero segments, resolution returned null, and the change degraded to precisely the spawn ENOENT it exists to fix. ComSpec and PATHEXT had the same exposure. #3445's tests never caught it because they pass uppercase keys explicitly, and neither did the Linux remote runner — this is a defect only the Windows lane could see. _envGet resolves a variable by exact match first (so a canonical caller pays no scan) and falls back to a case-insensitive sweep. R23 and P16 pin it and were proven RED by execution: with the fix stashed and build:lib re-run, R23 returned null and P16 returned the cmd.exe default. R24 was rewritten because the first version was vacuous — it staged foo.CMD, so the default PATHEXT already contained .CMD and it passed against the broken code for the wrong reason. It now stages foo.XYZ, an extension absent from the default, and carries a negative control asserting that dropping the Pathext key yields null. Re-proven RED the same way. E5's assertion was corrected alongside the fix: 'PATH' in options.env expressed the wrong contract. It now checks case-insensitively for the key. Refs #3411 * fix(#3617): execTool spawns the declared name unless mediation is required The Windows full-test lane on #3621 failed tests/graphify.test.cjs — the python3 identity check asserted 'python3' and got the absolute resolved path C:\hostedtoolcache\windows\Python\3.12.10\x64\python3.EXE instead. Those tests are correct and the change was wrong. They pin a long-standing contract — execTool spawns the program name it was given — by spying on spawnSync's first argument, and routing every win32 call through the projected invocation broke it. Resolving a .exe buys nothing. libuv's CreateProcess path already performs PATH + PATHEXT search, which is why spawning a bare 'node' has always worked on Windows. The only case the OS genuinely cannot spawn is a .cmd/.bat. So execTool now adopts the projection only when mediation actually happened — windowsVerbatimArguments is exactly that flag — and otherwise passes the declared program and args through untouched. 40-design.md already rejected gratuitous change for this reason: symmetry is not worth a behavior change to 53 files that fixes nothing. That reasoning was applied to POSIX and missed the win32 non-batch case. Rows 5 and 20 now record it, and the CONTEXT.md glossary states the caller-choice rule. deps.spawn deliberately still adopts the resolved path: its hasBinary probe answers from the same resolver, so probe and spawn must agree on the exact file (#3445). The asymmetry is now documented at both call sites rather than latent. E7 pins the restored contract and was verified by executing execTool against a monkeypatched spawnSync: python3 in, python3 spawned. Refs #3411 --------- Co-authored-by: sim <sim@local>
This commit is contained in:
@@ -255,6 +255,51 @@ hide malicious content in diffs.
|
||||
|
||||
---
|
||||
|
||||
## Layer 4 — Subprocess execution
|
||||
|
||||
GSD starts external programs constantly: git, npm, reviewer CLIs declared by
|
||||
capabilities, and whatever a gate predicate names. Every one of those is a
|
||||
place where an argument could become a command. One module owns the whole
|
||||
question — `src/shell-command-projection.cts`, the single platform seam.
|
||||
|
||||
**No `shell: true` for binary invocation.** Passing `shell: true` on Windows is
|
||||
the mechanism behind CVE-2024-27980: the shell re-parses the argument list, so
|
||||
a value containing `&` or `|` stops being data and becomes a second command.
|
||||
Node 26 additionally deprecates `shell: true` alongside an argument array
|
||||
(DEP0190), because arguments are concatenated rather than escaped. GSD resolves
|
||||
binaries explicitly instead.
|
||||
|
||||
**Explicit resolution, not shell lookup.** `resolveExecutableBinary` scans
|
||||
`PATH` and, on Windows, the `PATHEXT` extensions, and returns the resolved
|
||||
path. It never tries the bare name on Windows: npm global installs drop an
|
||||
extensionless POSIX `sh` shim beside `foo.CMD`, and resolving to that shim is
|
||||
how the reviewer lanes failed with `spawn ENOENT` (#3275). On macOS and Linux
|
||||
the bare name goes to `spawnSync` unchanged, so the operating system's own
|
||||
lookup keeps doing the work.
|
||||
|
||||
**Mediating `.cmd` and `.bat` safely.** Windows `CreateProcess` cannot execute a
|
||||
batch file at all, so one must be run through `cmd.exe`. That is where the
|
||||
injection risk actually lives, and it is not solved by resolution alone.
|
||||
`projectSpawnInvocation` builds the command line itself and passes it through
|
||||
verbatim: one outer quote pair that `cmd /c` strips, every token inside
|
||||
force-quoted, embedded quotes doubled. Force-quoting is the point — an unquoted
|
||||
`a&calc` is split by `cmd` into two commands, while a quoted `"a&calc"` is one
|
||||
literal argument. This is the shape Rust's standard library adopted for the
|
||||
sibling CVE-2024-24576.
|
||||
|
||||
Relying on the default argument escaping would not be enough. Node's own
|
||||
CVE-2024-27980 protection fires only when the program being started is itself
|
||||
the `.bat` or `.cmd`; once the program is `cmd.exe`, that check no longer
|
||||
applies, and the underlying quoting only quotes arguments containing spaces,
|
||||
tabs, or quotes — never one containing a bare `&`.
|
||||
|
||||
An argument containing a carriage return or newline is refused rather than
|
||||
mediated. A newline cannot be represented in a Windows command line, so
|
||||
mediating it would silently truncate the argument; failing visibly is the
|
||||
safer outcome.
|
||||
|
||||
---
|
||||
|
||||
## Trade-offs and limits
|
||||
|
||||
The security model described here meaningfully reduces the attack surface for
|
||||
@@ -292,6 +337,16 @@ research agents — but novel jailbreaks and low-signal injections may still pas
|
||||
undetected. Defence in depth means each layer makes the attack harder, not that
|
||||
any single layer makes it impossible.
|
||||
|
||||
**What subprocess execution does not eliminate:** `cmd.exe` expands `%VAR%`
|
||||
inside a `/c` string, and there is no escape for `%` outside a batch file. An
|
||||
argument containing `%FOO%` is therefore substituted with the environment
|
||||
value before the target program sees it. That is information disclosure, not
|
||||
arbitrary execution — the force-quoting still prevents an argument from
|
||||
becoming a second command — and it is the same residual limit Rust's standard
|
||||
library documents for its own batch-file handling. Callers that pass untrusted
|
||||
text as an argument to a Windows `.cmd` or `.bat` should not assume the value
|
||||
arrives byte-identical.
|
||||
|
||||
**Reporting vulnerabilities.** Report via private GitHub security advisory at
|
||||
`https://github.com/open-gsd/gsd-core/security/advisories/new`. Do not open
|
||||
public issues. See [SECURITY.md](../../SECURITY.md) for the response timeline
|
||||
|
||||
Reference in New Issue
Block a user