From f083cdad2e37dca15872bd951e3436b949fb2deb Mon Sep 17 00:00:00 2001 From: Kjell Tore Guttormsen Date: Sat, 18 Jul 2026 08:48:09 +0200 Subject: [PATCH] =?UTF-8?q?fix(llm-security):=20CRITICAL=20=E2=80=94=20com?= =?UTF-8?q?mand=20injection=20in=20auto-cleaner=20(v7.8.1)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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";;".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) Claude-Session: https://claude.ai/code/session_01TcQyMTQfyrsAapaCMPxTtQ --- scanners/auto-cleaner.mjs | 37 +++++++-- tests/scanners/auto-cleaner-rce.test.mjs | 101 +++++++++++++++++++++++ 2 files changed, 133 insertions(+), 5 deletions(-) create mode 100644 tests/scanners/auto-cleaner-rce.test.mjs diff --git a/scanners/auto-cleaner.mjs b/scanners/auto-cleaner.mjs index 3bd39e9..c69dbf5 100644 --- a/scanners/auto-cleaner.mjs +++ b/scanners/auto-cleaner.mjs @@ -12,7 +12,7 @@ import { readFile, writeFile, rename, unlink, stat } from 'node:fs/promises'; import { writeFileSync, unlinkSync } from 'node:fs'; import { resolve, extname, join, dirname, sep } from 'node:path'; import { fileURLToPath } from 'node:url'; -import { execSync } from 'node:child_process'; +import { spawnSync } from 'node:child_process'; import { fixResult, cleanEnvelope } from './lib/output.mjs'; // --------------------------------------------------------------------------- @@ -728,8 +728,15 @@ function validateContent(absPath, content) { const tmpPath = absPath.replace(/(\.\w+)$/, '.clean-check$1'); try { writeFileSync(tmpPath, content); - execSync(`node --check "${tmpPath}"`, { stdio: 'pipe', timeout: 5000 }); + // No shell: `tmpPath` derives from an untrusted scanned-repo FILENAME, and a + // name like `x";cmd;".mjs` would break out of an interpolated command string + // (CRITICAL, fixed v7.8.1). spawnSync with an argv array never parses metachars. + const check = spawnSync('node', ['--check', tmpPath], { stdio: 'pipe', timeout: 5000 }); unlinkSync(tmpPath); + if (check.error) throw check.error; + if (check.status !== 0) { + throw new Error(String(check.stderr || '').trim() || `node --check exited ${check.status}`); + } return { valid: true }; } catch (e) { try { unlinkSync(tmpPath); } catch { /* ignore */ } @@ -773,6 +780,21 @@ async function applyFixes(targetPath, findings, dryRun) { continue; } + // Belt (v7.8.1): refuse untrusted filenames carrying shell metacharacters or + // control chars before they reach ANY sink. The command-injection fix above + // removed the shell from validateContent, so this is defense-in-depth — it keeps + // a future sink that does interpolate a path from becoming exploitable again. + if (/["'`$;|&<>\n\r\\]|[\x00-\x1f]/.test(f.file)) { + fixes.push(fixResult({ + finding_id: f.id, + file: f.file, + operation: 'skip', + status: 'skipped', + description: 'Filename contains shell/control metacharacters — refused', + })); + continue; + } + const absPath = resolve(targetPath, f.file); // F-2: path-traversal containment. `f.file` is untrusted (scanned-repo @@ -984,12 +1006,17 @@ async function main() { console.error('[auto-cleaner] No --findings provided. Running scan-orchestrator...'); try { const orchestratorPath = join(dirname(fileURLToPath(import.meta.url)), 'scan-orchestrator.mjs'); - const result = execSync(`node "${resolve(orchestratorPath)}" "${targetPath}"`, { + // No shell — `targetPath` is operator-supplied but still never shell-parsed. + const run = spawnSync('node', [resolve(orchestratorPath), targetPath], { encoding: 'utf-8', timeout: 60000, stdio: ['pipe', 'pipe', 'pipe'], }); - const envelope = JSON.parse(result); + if (run.error) throw run.error; + if (run.status !== 0) { + throw new Error(String(run.stderr || '').trim() || `scan-orchestrator exited ${run.status}`); + } + const envelope = JSON.parse(run.stdout); findings = []; for (const scanner of Object.values(envelope.scanners || {})) { if (Array.isArray(scanner.findings)) { @@ -1052,4 +1079,4 @@ if (isMain) { } // Export for testing -export { classifyFinding, FIX_OPS, opsForFinding, applyFixes }; +export { classifyFinding, FIX_OPS, opsForFinding, applyFixes, validateContent }; diff --git a/tests/scanners/auto-cleaner-rce.test.mjs b/tests/scanners/auto-cleaner-rce.test.mjs new file mode 100644 index 0000000..b3fa994 --- /dev/null +++ b/tests/scanners/auto-cleaner-rce.test.mjs @@ -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";;".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 }); + } + }); +});