diff --git a/hooks/scripts/pre-install-supply-chain.mjs b/hooks/scripts/pre-install-supply-chain.mjs index 0c8e381..4a1e736 100644 --- a/hooks/scripts/pre-install-supply-chain.mjs +++ b/hooks/scripts/pre-install-supply-chain.mjs @@ -20,6 +20,7 @@ // - Allow (exit 0): everything else import { readFileSync, existsSync } from 'node:fs'; +import { spawnSync } from 'node:child_process'; import { AGE_THRESHOLD_HOURS, NPM_COMPROMISED, PIP_COMPROMISED, CARGO_COMPROMISED, GEM_COMPROMISED, @@ -251,7 +252,15 @@ function checkNpmProvenance(meta) { function inspectNpmPackage(name, version) { const spec = version ? `${name}@${version}` : name; - const raw = execSafe(`npm view ${spec} --json`); + // F-3: `spec` derives from attacker-controlled package tokens parsed out of the + // scanned Bash command. Pass it as a discrete argv element via spawnSync (no + // shell), so metacharacters like ';', '$(...)', backticks are never interpreted. + // npm still receives the full spec and reports the metadata (or an error JSON on + // stdout for an invalid name), matching the previous execSafe behaviour. + const res = spawnSync('npm', ['view', spec, '--json'], { + timeout: 10000, encoding: 'utf-8', stdio: ['pipe', 'pipe', 'pipe'], + }); + const raw = res.stdout || null; if (!raw) return null; try { return JSON.parse(raw); } catch { return null; } } diff --git a/scanners/auto-cleaner.mjs b/scanners/auto-cleaner.mjs index fe824ef..3bd39e9 100644 --- a/scanners/auto-cleaner.mjs +++ b/scanners/auto-cleaner.mjs @@ -10,7 +10,7 @@ import { readFile, writeFile, rename, unlink, stat } from 'node:fs/promises'; import { writeFileSync, unlinkSync } from 'node:fs'; -import { resolve, extname, join, dirname } from 'node:path'; +import { resolve, extname, join, dirname, sep } from 'node:path'; import { fileURLToPath } from 'node:url'; import { execSync } from 'node:child_process'; import { fixResult, cleanEnvelope } from './lib/output.mjs'; @@ -774,6 +774,25 @@ async function applyFixes(targetPath, findings, dryRun) { } const absPath = resolve(targetPath, f.file); + + // F-2: path-traversal containment. `f.file` is untrusted (scanned-repo + // filenames, or a fully attacker-chosen --findings file). A value like + // "../../.claude/settings.json" resolves OUTSIDE the scanned tree. Refuse + // anything that is not the target itself or contained within it, so the + // cleaner never writes outside the directory it was pointed at. + // NOTE: prefix containment does NOT defend against a symlink inside the + // tree pointing out of it — a known residual gap (see security-fix brief). + if (absPath !== targetPath && !absPath.startsWith(targetPath + sep)) { + fixes.push(fixResult({ + finding_id: f.id, + file: f.file, + operation: 'skip', + status: 'skipped', + description: 'Path escapes target directory — refused (path traversal)', + })); + continue; + } + if (!fileGroups.has(f.file)) { fileGroups.set(f.file, { findings: [], absPath }); } diff --git a/tests/hooks/supply-chain-injection.test.mjs b/tests/hooks/supply-chain-injection.test.mjs new file mode 100644 index 0000000..dca4339 --- /dev/null +++ b/tests/hooks/supply-chain-injection.test.mjs @@ -0,0 +1,54 @@ +// supply-chain-injection.test.mjs — Security regression for F-3 (HIGH, command injection). +// +// The pre-install-supply-chain hook inspects unknown npm packages via +// inspectNpmPackage(), which historically ran `execSafe(`npm view ${spec} --json`)` +// — a shell string. The `spec` derives from package tokens parsed out of the scanned +// Bash command (extractNpmPackages -> parseSpec), so a token carrying shell +// metacharacters reached the shell. +// +// Critically this fires on PreToolUse(Bash) BEFORE the user's install runs — so it +// executes pre-confirmation, even if the user then DENIES the npm command. +// +// Payload note: `normalizeBashExpansion` rewrites ${IFS}, <(...), `...`, ${...} etc., +// but leaves `$(...)` command substitution intact. A redirect-only substitution +// `$(>PATH)` contains no whitespace, so it survives extractNpmPackages' whitespace +// split as a single "package" token, and `>PATH` creates PATH when the shell evaluates +// the substitution. The test asserts that sentinel file is NEVER created — it must FAIL +// against the execSync sink and PASS once inspectNpmPackage uses spawnSync (no shell). + +import { describe, it } from 'node:test'; +import assert from 'node:assert/strict'; +import { mkdtempSync, existsSync, rmSync } from 'node:fs'; +import { join, resolve } from 'node:path'; +import { tmpdir } from 'node:os'; +import { runHook } from './hook-helper.mjs'; + +const SCRIPT = resolve(import.meta.dirname, '../../hooks/scripts/pre-install-supply-chain.mjs'); + +function bashPayload(command) { + return { tool_name: 'Bash', tool_input: { command } }; +} + +describe('pre-install-supply-chain command-injection regression (F-3)', () => { + it('does not execute shell metacharacters embedded in an npm package spec', async () => { + const dir = mkdtempSync(join(tmpdir(), 'supply-chain-injection-')); + const sentinel = join(dir, 'PWNED'); + try { + assert.ok(!existsSync(sentinel), 'precondition: sentinel must not exist before the hook runs'); + + // `$(>/abs/PWNED)` — redirect-only command substitution, no whitespace. + // Vulnerable: the shell creates PWNED while evaluating `npm view $(>...) --json`. + // Fixed: the literal string is passed as one argv element to `npm view`, no shell. + const result = await runHook(SCRIPT, bashPayload(`npm install $(>${sentinel})`)); + + assert.ok( + !existsSync(sentinel), + 'COMMAND INJECTION: a shell-metachar npm spec executed during pre-install inspection', + ); + // The hook must still terminate (block/warn/allow), not crash. + assert.ok([0, 2].includes(result.code), `hook should exit cleanly, got code ${result.code}`); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); +}); diff --git a/tests/scanners/auto-cleaner-traversal.test.mjs b/tests/scanners/auto-cleaner-traversal.test.mjs new file mode 100644 index 0000000..3bae392 --- /dev/null +++ b/tests/scanners/auto-cleaner-traversal.test.mjs @@ -0,0 +1,96 @@ +// auto-cleaner-traversal.test.mjs — Security regression for F-2 (HIGH, arbitrary file write). +// +// auto-cleaner's applyFixes() historically did `resolve(targetPath, f.file)` with no +// containment check, then wrote the modified content back to that path. `f.file` comes +// from findings JSON — untrusted scanned-repo filenames, or a fully attacker-chosen +// `--findings` file. A finding with `file: "../secret.txt"` therefore let the cleaner +// modify (overwrite) a file OUTSIDE the scanned tree. +// +// This test places an auto-fixable file outside the target dir and points an auto-tier +// finding at it via "../secret.txt". It must FAIL while the sink is uncontained (the +// outside file gets rewritten) and PASS once applyFixes refuses paths that escape the +// target (prefix containment), reporting them as skipped instead of writing. + +import { describe, it } from 'node:test'; +import assert from 'node:assert/strict'; +import { mkdtempSync, mkdirSync, writeFileSync, readFileSync, rmSync } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; +import { resetCounter } from '../../scanners/lib/output.mjs'; +import { applyFixes } from '../../scanners/auto-cleaner.mjs'; + +const ZW = '​'; // zero-width space — strip_zero_width will remove it + +describe('auto-cleaner path-traversal regression (F-2)', () => { + it('refuses to write a finding whose file path escapes the target directory', async () => { + // root/ <- mkdtemp parent + // secret.txt <- the escape target, OUTSIDE the scanned tree + // scan-target/ <- the directory actually passed to the cleaner + const root = mkdtempSync(join(tmpdir(), 'auto-cleaner-traversal-')); + const target = join(root, 'scan-target'); + const escape = join(root, 'secret.txt'); + try { + mkdirSync(target, { recursive: true }); + + // The escape file carries a zero-width char so the (auto-tier) strip_zero_width + // op WOULD change it — proving a write actually reaches outside the tree if + // containment is missing. + const original = `secret${ZW}value\n`; + writeFileSync(escape, original, 'utf-8'); + + // Untrusted finding: classified 'auto' (UNI zero-width), file traverses up and out. + const findings = [{ + id: 'TRAVERSAL-1', + scanner: 'UNI', + title: 'Zero-width characters detected', + severity: 'high', + file: '../secret.txt', + }]; + + resetCounter(); + const { fixes } = await applyFixes(target, findings, /* dryRun */ false); + + // The file outside the target must be byte-for-byte untouched. + assert.equal( + readFileSync(escape, 'utf-8'), + original, + 'PATH TRAVERSAL: auto-cleaner rewrote a file outside the scanned target directory', + ); + + // And the escaping finding must be surfaced as refused/skipped, never applied. + const escaped = fixes.find(f => f.file === '../secret.txt'); + assert.ok(escaped, 'the escaping finding should be reported'); + assert.notEqual(escaped.status, 'applied', 'escaping finding must not be applied'); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }); + + it('still applies fixes to files contained within the target directory', async () => { + const target = mkdtempSync(join(tmpdir(), 'auto-cleaner-contained-')); + try { + const inside = join(target, 'inside.md'); + writeFileSync(inside, `---\ntitle: x${ZW}y\n---\nbody\n`, 'utf-8'); + + const findings = [{ + id: 'CONTAINED-1', + scanner: 'UNI', + title: 'Zero-width characters detected', + severity: 'high', + file: 'inside.md', + }]; + + resetCounter(); + const { fixes } = await applyFixes(target, findings, /* dryRun */ false); + + assert.ok( + !readFileSync(inside, 'utf-8').includes(ZW), + 'contained file should have been cleaned', + ); + const applied = fixes.find(f => f.file === 'inside.md' && f.status === 'applied'); + assert.ok(applied, 'a contained auto-fix should be applied'); + } finally { + rmSync(target, { recursive: true, force: true }); + } + }); +});