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
96 lines
4.1 KiB
JavaScript
96 lines
4.1 KiB
JavaScript
// 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 });
|
||
}
|
||
});
|
||
});
|