Kjell Tore Guttormsen
dfb663fa61
Session B of the security-fix track. Both sinks trusted untrusted strings at a
filesystem/subprocess boundary; same fix family as F-1.
F-2 (HIGH, arbitrary file write) — scanners/auto-cleaner.mjs: applyFixes() did
resolve(targetPath, f.file) with no containment, then wrote the cleaned content
back. f.file is untrusted (scanned-repo filenames, or a fully attacker-chosen
--findings file), so file: "../../.claude/settings.json" let the cleaner modify
files OUTSIDE the scanned tree. Add a prefix-containment check before grouping:
absPath must equal targetPath or start with targetPath + sep, else the finding
is refused and reported as skipped. (Documented residual gap: prefix containment
does not stop a symlink inside the tree pointing out — noted inline.)
F-3 (HIGH, command injection, pre-confirmation) — hooks/scripts/
pre-install-supply-chain.mjs: inspectNpmPackage ran execSafe(`npm view ${spec}
--json`), a shell string. spec derives from package tokens parsed out of the
scanned Bash command, so a metachar-bearing token reached the shell on
PreToolUse(Bash) — BEFORE the install, so it ran even if the user then denied
the command. Switch to spawnSync('npm', ['view', spec, '--json']) (no shell);
spec is passed as one argv element. The static `npm audit --json` execSafe call
is left as-is (no interpolation).
TDD (repro -> red -> green):
- tests/scanners/auto-cleaner-traversal.test.mjs: a "../secret.txt" finding must
not rewrite the outside file; contained files still get cleaned.
- tests/hooks/supply-chain-injection.test.mjs: `npm install $(>/abs/PWNED)`
(redirect-only $(...) survives normalizeBashExpansion + the whitespace split)
must not create the sentinel.
Closing gates: full node --test suite 1863/0 (was 1860; +3); gitleaks clean;
F-1 regression re-run GREEN (sink still closed).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V3s6WnubSSrFjAQTLQdVbG
54 lines
2.7 KiB
JavaScript
54 lines
2.7 KiB
JavaScript
// 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 });
|
|
}
|
|
});
|
|
});
|