fix(llm-security): F-2/F-3 — contain path traversal + kill npm-view shell injection
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
This commit is contained in:
parent
a546b6a1aa
commit
3f64aa5bab
4 changed files with 180 additions and 2 deletions
54
tests/hooks/supply-chain-injection.test.mjs
Normal file
54
tests/hooks/supply-chain-injection.test.mjs
Normal file
|
|
@ -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 });
|
||||
}
|
||||
});
|
||||
});
|
||||
Loading…
Add table
Add a link
Reference in a new issue