docs: security fix brief for next session — F-1 RCE + F-2/F-3 sinks
Action brief from the marketplace-wide review (see docs/review-2026-06-20.md). Three shell/path
injection sinks: F-1 CRITICAL zero-interaction RCE in git-forensics.mjs (execSync shell string +
attacker-controlled filenames; reproduced), F-2 HIGH path-containment write in auto-cleaner.mjs,
F-3 HIGH command injection in the supply-chain hook. Common fix: execSync(string) -> spawnSync(array)
/ input containment. DO AFTER the current active session finishes.
Disclosure hold: this brief + review-2026-06-20.md (ff9bd13) are committed locally but NOT pushed;
push both only together with the F-1 fix (the brief carries a working RCE repro payload).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ter3E2JSi1Khgmuf2kady8
This commit is contained in:
parent
616e7ff18c
commit
92bd99f2a8
1 changed files with 80 additions and 0 deletions
80
docs/security-fix-brief-2026-06-20.md
Normal file
80
docs/security-fix-brief-2026-06-20.md
Normal file
|
|
@ -0,0 +1,80 @@
|
|||
# Security fix brief — 3 shell/path injection sinks (2026-06-20)
|
||||
|
||||
> **Action brief for the NEXT session.** Found in the marketplace-wide review (full context:
|
||||
> `docs/review-2026-06-20.md`). Do this **after the current active session finishes** — do not
|
||||
> interleave with in-flight work.
|
||||
>
|
||||
> **Disclosure hold:** the review report (`review-2026-06-20.md`, commit `738770e`) and this brief
|
||||
> are committed locally but **NOT pushed**, because F-1 is a working RCE with a reproduction
|
||||
> payload. **Push both only together with the F-1 fix** — never publish the RCE detail to a public
|
||||
> remote while it is still exploitable.
|
||||
|
||||
## Priority order
|
||||
|
||||
| # | Severity | Fix first? |
|
||||
|---|----------|-----------|
|
||||
| F-1 | **CRITICAL** (zero-interaction RCE) | **YES — before any `/security scan` of an untrusted repo URL** |
|
||||
| F-2 | HIGH (arbitrary file write) | yes |
|
||||
| F-3 | HIGH (command injection, pre-confirmation) | yes |
|
||||
| F-4 | LOW | optional |
|
||||
| F-5/F-6 | LOW (hygiene) | optional |
|
||||
|
||||
## Common root cause
|
||||
|
||||
F-1/F-2/F-3 all trust untrusted strings at a subprocess/filesystem sink. The fix family is the same:
|
||||
**`execSync(shell-string)` → `spawnSync('cmd', [...argArray])`** (no shell), or input containment.
|
||||
Every affected call site already has discrete tokens, so the change is mechanical and localized.
|
||||
|
||||
## F-1 — CRITICAL — `scanners/git-forensics.mjs`
|
||||
|
||||
- **Sink:** `git()` at `:59-66` runs `execSync(\`git ${cmd}\`)` (shell string). Attacker-controlled
|
||||
filenames from the scanned repo (`git ls-files` :202, `git log --name-only` :549) are interpolated
|
||||
at **:211, :223, :231, :564**. `"${relFile}"` quoting at :211 does NOT stop `$(...)`/backticks;
|
||||
:223/:231 are unquoted.
|
||||
- **Why critical:** `gitScan` is in the **default** SCANNERS array (`scan-orchestrator.mjs:119`), and
|
||||
`commands/scan.md` clones a user-supplied GitHub URL then scans it. A hostile repo containing a
|
||||
file named `commands/$(touch INJECTED).md` runs arbitrary code on the analyst's machine on
|
||||
`/security scan <url>` — no install, no confirmation. Forensics runs **outside** the git-clone
|
||||
OS-sandbox (that wraps only the clone), so it runs unsandboxed on all platforms. Reproduced
|
||||
end-to-end.
|
||||
- **Fix:** convert `git()` to `spawnSync('git', [...argArray], {cwd})` and pass each call site's
|
||||
tokens as an array (they are already discrete). No shell, no interpolation.
|
||||
- **Verify:** add a test fixture repo with a file named `$(touch /tmp/pwned).md`; assert the scan
|
||||
completes and `/tmp/pwned` is NOT created. (TDD: write the failing repro first.)
|
||||
|
||||
## F-2 — HIGH — `scanners/auto-cleaner.mjs`
|
||||
|
||||
- **Sink:** `:776` `resolve(targetPath, f.file)` with no containment check; written at `:878-881`.
|
||||
`f.file` comes from findings JSON (untrusted repo filenames, or a fully attacker-chosen
|
||||
`--findings` file), so `file: "../../../.claude/settings.json"` writes outside the scanned tree.
|
||||
- **Fix:** before writing, assert `absPath === targetPath || absPath.startsWith(targetPath + sep)`;
|
||||
otherwise skip + report.
|
||||
- **Verify:** a finding with `file: "../escape.txt"` must be refused, not written.
|
||||
|
||||
## F-3 — HIGH — `hooks/scripts/pre-install-supply-chain.mjs` → `supply-chain-data.mjs`
|
||||
|
||||
- **Sink:** `pre-install-supply-chain.mjs:254` → `inspectNpmPackage` calls
|
||||
`execSafe(\`npm view ${spec} --json\`)`; `execSafe` (`supply-chain-data.mjs:221-227`) is
|
||||
`execSync` (shell). A spec like `foo;touch /tmp/X` survives `extractNpmPackages`+`parseSpec`
|
||||
(name=`foo;touch`) and reaches the shell. It fires on **PreToolUse(Bash) before** the user's npm
|
||||
install — so it executes pre-confirmation even if the user then denies the Bash call.
|
||||
- **Fix:** `spawnSync('npm', ['view', spec, '--json'])`, or validate `spec` against
|
||||
`^[@/A-Za-z0-9._-]+$` before use. (pip/go/OSV paths already use fetch/array args — only the npm
|
||||
`npm view` path shells out.)
|
||||
- **Verify:** a package spec `foo;touch /tmp/X` must not execute the `touch`.
|
||||
|
||||
## F-4 / F-5 / F-6 — LOW
|
||||
|
||||
- **F-4** `mcp-live-inspect.mjs:296` spawns the scanned target's declared MCP command (array-arg, no
|
||||
metachar injection). By-design for `/security mcp-inspect`, but scanning an untrusted target
|
||||
launches attacker-declared processes — add a confirm before spawning servers from a non-home target.
|
||||
- **F-5/F-6** two stray committed root artifacts: `--json` (0-byte redirect husk) and `.orphaned_at`.
|
||||
Inert; `git rm` them.
|
||||
|
||||
## Closing gates (when fixing)
|
||||
|
||||
- TDD per fix (repro → red → green). Full `node --test` suite green.
|
||||
- gitleaks clean. Re-run the F-1 repro to confirm the sink is closed.
|
||||
- **Then** push: the F-1 fix commit + `review-2026-06-20.md` + this brief, together. After that the
|
||||
disclosure hold is lifted.
|
||||
- Fixing F-1/F-2/F-3 lifts the plugin from B− to a solid A−.
|
||||
Loading…
Add table
Add a link
Reference in a new issue