fix(llm-security): CRITICAL — command injection in auto-cleaner (v7.8.1)
validateContent() syntax-checked .mjs/.js/.cjs candidates via
execSync(`node --check "${tmpPath}"`), where tmpPath derives from the
untrusted scanned-repo FILENAME. The F-2 guard checks path containment
but never quotes or strips shell metacharacters, so a file named
`x";<cmd>;".mjs` in a scanned repo turned `/security clean` (live is the
documented default) into arbitrary command execution. Verified with a
live PoC before the fix.
- validateContent: spawnSync('node', ['--check', tmpPath]) — no shell.
- CLI orchestrator fallback: same treatment (argv array, explicit
status/error handling instead of execSync's throw-on-nonzero).
- Belt (defense-in-depth): applyFixes now refuses findings whose `file`
carries shell or control metacharacters, reported as skipped.
- Regression tests cover both layers separately, so the belt cannot mask
a re-introduced shell in the sink.
node --test: 1865/1865 green (1863 + 2 new).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TcQyMTQfyrsAapaCMPxTtQ
This commit is contained in:
parent
0dd46da2f7
commit
f083cdad2e
2 changed files with 133 additions and 5 deletions
101
tests/scanners/auto-cleaner-rce.test.mjs
Normal file
101
tests/scanners/auto-cleaner-rce.test.mjs
Normal file
|
|
@ -0,0 +1,101 @@
|
|||
// auto-cleaner-rce.test.mjs — Security regression for the v7.8.0 CRITICAL (RCE).
|
||||
//
|
||||
// auto-cleaner's validateContent() syntax-checked .mjs/.js/.cjs candidates by shelling
|
||||
// out: execSync(`node --check "${tmpPath}"`). `tmpPath` derives from the finding's
|
||||
// `file` field — an untrusted scanned-repo FILENAME. The F-2 guard checks path
|
||||
// containment (the path must stay inside the target tree) but never strips or quotes
|
||||
// shell metacharacters, and a filename containing `"` closes the interpolated quote.
|
||||
//
|
||||
// A file named `x";<cmd>;".mjs` inside an untrusted repo therefore turned
|
||||
// `/security clean` (live mode is the documented default) into arbitrary command
|
||||
// execution on the operator's machine.
|
||||
//
|
||||
// This test drops such a file in the scanned tree and asserts the injected command
|
||||
// never ran. It FAILS while the sink goes through a shell, and PASSES once the call
|
||||
// is a no-shell spawnSync array.
|
||||
|
||||
import { describe, it } from 'node:test';
|
||||
import assert from 'node:assert/strict';
|
||||
import { mkdtempSync, mkdirSync, writeFileSync, existsSync, rmSync } from 'node:fs';
|
||||
import { join } from 'node:path';
|
||||
import { tmpdir } from 'node:os';
|
||||
import { resetCounter } from '../../scanners/lib/output.mjs';
|
||||
import { applyFixes, validateContent } from '../../scanners/auto-cleaner.mjs';
|
||||
|
||||
const ZW = ''; // zero-width space — strip_zero_width will remove it
|
||||
|
||||
// The payload target goes through $CLEANER_RCE_CANARY because a literal path would
|
||||
// need `/`, which no filename may contain — the env-var expansion also proves a
|
||||
// *shell* interpreted the string rather than exec'ing an argv array.
|
||||
const HOSTILE_NAME = 'x";touch $CLEANER_RCE_CANARY;".mjs';
|
||||
|
||||
describe('auto-cleaner command-injection regression (CRITICAL, v7.8.1)', () => {
|
||||
// Layer 1 — the sink itself. Exercised directly, because the applyFixes-level
|
||||
// metachar guard (layer 2) would otherwise stop the input before it ever reaches
|
||||
// validateContent, and the test would pass without proving the shell is gone.
|
||||
it('validateContent does not shell-interpret a hostile filename', () => {
|
||||
const root = mkdtempSync(join(tmpdir(), 'auto-cleaner-rce-sink-'));
|
||||
const canary = join(root, 'pwned');
|
||||
try {
|
||||
process.env.CLEANER_RCE_CANARY = canary;
|
||||
const result = validateContent(join(root, HOSTILE_NAME), 'export const a = 1;\n');
|
||||
|
||||
assert.equal(
|
||||
existsSync(canary),
|
||||
false,
|
||||
'COMMAND INJECTION: validateContent shell-executed a command embedded in the filename',
|
||||
);
|
||||
assert.equal(result.valid, true, 'syntactically valid content must still validate');
|
||||
} finally {
|
||||
delete process.env.CLEANER_RCE_CANARY;
|
||||
rmSync(root, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
// Layer 2 — the belt: such a finding never reaches any sink in the first place.
|
||||
it('applyFixes refuses a finding whose filename carries shell metacharacters', async () => {
|
||||
const root = mkdtempSync(join(tmpdir(), 'auto-cleaner-rce-'));
|
||||
const target = join(root, 'scan-target');
|
||||
const canary = join(root, 'pwned');
|
||||
|
||||
try {
|
||||
mkdirSync(target, { recursive: true });
|
||||
|
||||
process.env.CLEANER_RCE_CANARY = canary;
|
||||
|
||||
// Zero-width char makes the UNI finding auto-tier AND makes the content actually
|
||||
// change, so without the guard this file would flow on into the fix pipeline.
|
||||
writeFileSync(
|
||||
join(target, HOSTILE_NAME),
|
||||
`export const a = 1;${ZW}\n`,
|
||||
'utf-8',
|
||||
);
|
||||
|
||||
const findings = [{
|
||||
id: 'RCE-1',
|
||||
scanner: 'UNI',
|
||||
title: 'Zero-width characters detected',
|
||||
severity: 'high',
|
||||
file: HOSTILE_NAME,
|
||||
}];
|
||||
|
||||
resetCounter();
|
||||
const { fixes } = await applyFixes(target, findings, /* dryRun */ false);
|
||||
|
||||
assert.equal(
|
||||
existsSync(canary),
|
||||
false,
|
||||
'COMMAND INJECTION: a shell command embedded in a scanned filename was executed by auto-cleaner',
|
||||
);
|
||||
|
||||
// ...and it must be surfaced as refused, never silently dropped.
|
||||
const skipped = fixes.find((x) => x.finding_id === 'RCE-1');
|
||||
assert.equal(skipped?.status, 'skipped');
|
||||
assert.match(skipped.description, /metacharacters/);
|
||||
} finally {
|
||||
delete process.env.CLEANER_RCE_CANARY;
|
||||
rmSync(root, { recursive: true, force: true });
|
||||
rmSync(canary, { force: true });
|
||||
}
|
||||
});
|
||||
});
|
||||
Loading…
Add table
Add a link
Reference in a new issue