From e46b5a32564712153e170717a0c7b31526183ad9 Mon Sep 17 00:00:00 2001 From: Kjell Tore Guttormsen Date: Sat, 20 Jun 2026 10:18:41 +0200 Subject: [PATCH] =?UTF-8?q?fix(llm-security):=20F-1=20=E2=80=94=20eliminat?= =?UTF-8?q?e=20shell-injection=20RCE=20in=20git-forensics=20scanner?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit git-forensics ran execSync(`git ${cmd}`), interpolating attacker-controlled filenames from the SCANNED repo (git ls-files / git log --name-only) into a shell string. A hostile repo containing a file named `commands/$(touch X).md` achieved zero-interaction RCE on `/security scan `: gitScan is in the default scanner array and runs OUTSIDE the git-clone OS sandbox (which wraps only the clone), so it executed unsandboxed on all platforms. Convert the git() helper to spawnSync('git', [...args]) with no shell; every call site now passes discrete tokens (shell quoting removed — git does its own pathspec globbing). The helper throws on non-zero exit, preserving existing per-category/per-file try/catch semantics. TDD: adds a failing-first regression (tests/scanners/git-injection.test.mjs) that builds a hostile-filename fixture repo and asserts the injected command never runs. RED against execSync, GREEN after the fix. Also removes two stray committed root artifacts (F-5/F-6): `--json` (0-byte redirect husk) and .orphaned_at. Closing gates: full node --test suite 1860/0; gitleaks clean. F-2/F-3 follow in Session B. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01V3s6WnubSSrFjAQTLQdVbG --- --json | 0 .orphaned_at | 1 - scanners/git-forensics.mjs | 83 ++++++++++++++++----------- tests/scanners/git-injection.test.mjs | 81 ++++++++++++++++++++++++++ 4 files changed, 129 insertions(+), 36 deletions(-) delete mode 100644 --json delete mode 100644 .orphaned_at create mode 100644 tests/scanners/git-injection.test.mjs diff --git a/--json b/--json deleted file mode 100644 index e69de29..0000000 diff --git a/.orphaned_at b/.orphaned_at deleted file mode 100644 index 57fe109..0000000 --- a/.orphaned_at +++ /dev/null @@ -1 +0,0 @@ -1775452698205 \ No newline at end of file diff --git a/scanners/git-forensics.mjs b/scanners/git-forensics.mjs index 9b795db..2c49967 100644 --- a/scanners/git-forensics.mjs +++ b/scanners/git-forensics.mjs @@ -9,7 +9,7 @@ import { finding, scannerResult } from './lib/output.mjs'; import { SEVERITY } from './lib/severity.mjs'; import { levenshtein } from './lib/string-utils.mjs'; -import { execSync } from 'node:child_process'; +import { spawnSync } from 'node:child_process'; import { existsSync } from 'node:fs'; import { join } from 'node:path'; @@ -50,19 +50,32 @@ const NETWORK_PATTERNS = /\b(fetch|http|https|curl|wget|dns\.lookup|net\.connect // --------------------------------------------------------------------------- /** - * Run a git command in the target directory. - * @param {string} cmd - Git command (without 'git' prefix) or full command - * @param {string} cwd - Working directory - * @returns {string} - stdout string, trimmed - * @throws - On non-zero exit or timeout + * Run a git command in the target directory WITHOUT a shell. + * + * Each argument is passed as a discrete token to spawnSync, so attacker-controlled + * filenames from the scanned repo (`git ls-files`, `git log --name-only`) can never + * reach a shell for command substitution (`$(...)`, backticks) or metacharacter + * injection. This is the F-1 (CRITICAL RCE) fix — see tests/scanners/git-injection.test.mjs. + * + * @param {string[]} args - Git arguments as discrete tokens (no 'git' prefix, no shell quoting) + * @param {string} cwd - Working directory + * @returns {string} - stdout string, trimmed + * @throws - On spawn failure, non-zero exit, or timeout */ -function git(cmd, cwd) { - return execSync(`git ${cmd}`, { +function git(args, cwd) { + const result = spawnSync('git', args, { cwd, timeout: GIT_TIMEOUT_MS, encoding: 'utf-8', - stdio: ['pipe', 'pipe', 'pipe'], - }).trim(); + stdio: ['ignore', 'pipe', 'pipe'], + maxBuffer: 64 * 1024 * 1024, + }); + if (result.error) throw result.error; + if (result.status !== 0) { + const stderr = (result.stderr || '').toString().trim(); + throw new Error(`git ${args.join(' ')} failed (exit ${result.status}): ${stderr}`); + } + return (result.stdout || '').trim(); } // --------------------------------------------------------------------------- @@ -78,7 +91,7 @@ function git(cmd, cwd) { function isGitRepo(targetPath) { if (existsSync(join(targetPath, '.git'))) return true; try { - git('rev-parse --git-dir', targetPath); + git(['rev-parse', '--git-dir'], targetPath); return true; } catch { return false; @@ -100,7 +113,7 @@ function detectForcePushes(targetPath) { // Check reflog for reset entries (local force push evidence) try { - const reflog = git("reflog --format='%H %gD %gs' -n 500", targetPath); + const reflog = git(['reflog', '--format=%H %gD %gs', '-n', '500'], targetPath); const lines = reflog.split('\n').filter(Boolean); const resetLines = lines.filter(l => l.includes('reset:') || l.includes('reset')); @@ -128,7 +141,7 @@ function detectForcePushes(targetPath) { // Check walk-reflogs for forced-update try { - const walkLog = git('log --walk-reflogs --format="%H %gD %gs" -n 200', targetPath); + const walkLog = git(['log', '--walk-reflogs', '--format=%H %gD %gs', '-n', '200'], targetPath); const forcedLines = walkLog.split('\n').filter(l => l.includes('forced-update')); if (forcedLines.length > 0) { @@ -199,7 +212,7 @@ function detectDescriptionDrift(targetPath) { // List tracked files matching commands/*.md or agents/*.md let trackedFiles; try { - const raw = git('ls-files -- "commands/*.md" "agents/*.md"', targetPath); + const raw = git(['ls-files', '--', 'commands/*.md', 'agents/*.md'], targetPath); trackedFiles = raw.split('\n').filter(Boolean).slice(0, MAX_DRIFT_FILES); } catch { return results; @@ -208,7 +221,7 @@ function detectDescriptionDrift(targetPath) { for (const relFile of trackedFiles) { try { // Find the commit that first added this file - const addHash = git(`log --diff-filter=A --format='%H' -- "${relFile}"`, targetPath) + const addHash = git(['log', '--diff-filter=A', '--format=%H', '--', relFile], targetPath) .split('\n') .filter(Boolean) .pop(); // oldest = last in log output (reverse chrono) @@ -220,7 +233,7 @@ function detectDescriptionDrift(targetPath) { // Get initial content at that commit let initialContent; try { - initialContent = git(`show ${addHash}:${relFile}`, targetPath); + initialContent = git(['show', `${addHash}:${relFile}`], targetPath); } catch { continue; } @@ -228,7 +241,7 @@ function detectDescriptionDrift(targetPath) { // Get current content let currentContent; try { - currentContent = git(`show HEAD:${relFile}`, targetPath); + currentContent = git(['show', `HEAD:${relFile}`], targetPath); } catch { continue; } @@ -286,7 +299,7 @@ function detectHookModifications(targetPath) { let hookFiles; try { - const raw = git('ls-files -- "hooks/scripts/*"', targetPath); + const raw = git(['ls-files', '--', 'hooks/scripts/*'], targetPath); hookFiles = raw.split('\n').filter(Boolean); } catch { return results; @@ -295,7 +308,7 @@ function detectHookModifications(targetPath) { for (const relFile of hookFiles) { try { // Count total commits touching this file - const logLines = git(`log --oneline -- "${relFile}"`, targetPath) + const logLines = git(['log', '--oneline', '--', relFile], targetPath) .split('\n') .filter(Boolean); const modCount = logLines.length; @@ -305,7 +318,7 @@ function detectHookModifications(targetPath) { // Check if latest diff adds network calls let latestDiff = ''; try { - latestDiff = git(`diff HEAD~1 HEAD -- "${relFile}"`, targetPath); + latestDiff = git(['diff', 'HEAD~1', 'HEAD', '--', relFile], targetPath); } catch { // HEAD~1 may not exist (single commit repo after first mod) } @@ -390,7 +403,7 @@ function detectNewOutboundUrls(targetPath) { // Get initial commit hash let initialHash; try { - initialHash = git('rev-list --max-parents=0 HEAD', targetPath).split('\n')[0].trim(); + initialHash = git(['rev-list', '--max-parents=0', 'HEAD'], targetPath).split('\n')[0].trim(); } catch { return results; } @@ -398,14 +411,14 @@ function detectNewOutboundUrls(targetPath) { // Get all URLs present in initial commit (full tree) let initialUrls = new Set(); try { - const initialContent = git(`show ${initialHash}:`, targetPath); + const initialContent = git(['show', `${initialHash}:`], targetPath); // This lists files — we need content. Use git grep on the initial tree. - const initialGrep = git(`grep -r "https\\?://" ${initialHash}`, targetPath); + const initialGrep = git(['grep', '-r', 'https\\?://', initialHash], targetPath); initialUrls = extractHostnames(initialGrep); } catch { // Fallback: grep the initial commit diff itself try { - const initDiff = git(`show ${initialHash}`, targetPath); + const initDiff = git(['show', initialHash], targetPath); initialUrls = extractHostnames(initDiff); } catch { // Cannot determine initial URLs — skip @@ -416,7 +429,7 @@ function detectNewOutboundUrls(targetPath) { // Get diff of last 50 commits (added lines only) let recentDiff = ''; try { - recentDiff = git(`log -50 --format='' -p`, targetPath); + recentDiff = git(['log', '-50', '--format=', '-p'], targetPath); } catch { return results; } @@ -473,7 +486,7 @@ function detectAuthorChanges(targetPath) { let emailList; try { - emailList = git('log --format="%ae"', targetPath).split('\n').filter(Boolean); + emailList = git(['log', '--format=%ae'], targetPath).split('\n').filter(Boolean); } catch { return results; } @@ -503,7 +516,7 @@ function detectAuthorChanges(targetPath) { // Flag: mid-history author change (compare first commit author to later commits) try { - const allAuthors = git('log --reverse --format="%ae"', targetPath); + const allAuthors = git(['log', '--reverse', '--format=%ae'], targetPath); const firstAuthor = allAuthors.split('\n')[0].trim(); const laterAuthors = emailList.slice(0, -1); // all except the oldest (last in desc order) const newAuthors = laterAuthors.filter(e => e !== firstAuthor); @@ -546,7 +559,7 @@ function detectBinaryAdditions(targetPath) { let addedFiles; try { - const raw = git('log --diff-filter=A --name-only --format="" -50', targetPath); + const raw = git(['log', '--diff-filter=A', '--name-only', '--format=', '-50'], targetPath); addedFiles = raw.split('\n').filter(Boolean); } catch { return results; @@ -561,7 +574,7 @@ function detectBinaryAdditions(targetPath) { // Find which commit added it let addCommit = 'unknown'; try { - addCommit = git(`log --diff-filter=A --format="%H %ae %ai" -- "${binFile}"`, targetPath) + addCommit = git(['log', '--diff-filter=A', '--format=%H %ae %ai', '--', binFile], targetPath) .split('\n')[0] || 'unknown'; } catch { // non-fatal @@ -605,7 +618,7 @@ function detectSuspiciousCommitPatterns(targetPath) { let commitHashes; try { - const raw = git(`log --format="%H" -${MAX_COMMITS}`, targetPath); + const raw = git(['log', '--format=%H', `-${MAX_COMMITS}`], targetPath); commitHashes = raw.split('\n').filter(Boolean).slice(0, 50); // check last 50 } catch { return results; @@ -614,13 +627,13 @@ function detectSuspiciousCommitPatterns(targetPath) { for (const hash of commitHashes) { try { // Get commit subject and diff stat - const subject = git(`log -1 --format="%s" ${hash}`, targetPath).toLowerCase(); + const subject = git(['log', '-1', '--format=%s', hash], targetPath).toLowerCase(); const isCosmeticMsg = /^(update|fix|cleanup|refactor|minor|bump|chore)/.test(subject); if (!isCosmeticMsg) continue; // Check if this "cosmetic" commit actually touches hooks - const changedFiles = git(`diff-tree --no-commit-id -r --name-only ${hash}`, targetPath) + const changedFiles = git(['diff-tree', '--no-commit-id', '-r', '--name-only', hash], targetPath) .split('\n') .filter(Boolean); const touchesHooks = changedFiles.some(f => f.includes('hooks/') || f.includes('hook')); @@ -630,7 +643,7 @@ function detectSuspiciousCommitPatterns(targetPath) { // Check if the diff adds network patterns let commitDiff; try { - commitDiff = git(`show ${hash} --format=""`, targetPath); + commitDiff = git(['show', hash, '--format='], targetPath); } catch { continue; } @@ -643,8 +656,8 @@ function detectSuspiciousCommitPatterns(targetPath) { if (!NETWORK_PATTERNS.test(addedInCommit)) continue; const shortHash = hash.slice(0, 8); - const author = git(`log -1 --format="%ae" ${hash}`, targetPath); - const date = git(`log -1 --format="%ai" ${hash}`, targetPath); + const author = git(['log', '-1', '--format=%ae', hash], targetPath); + const date = git(['log', '-1', '--format=%ai', hash], targetPath); results.push(finding({ scanner: 'GIT', diff --git a/tests/scanners/git-injection.test.mjs b/tests/scanners/git-injection.test.mjs new file mode 100644 index 0000000..c6987cd --- /dev/null +++ b/tests/scanners/git-injection.test.mjs @@ -0,0 +1,81 @@ +// git-injection.test.mjs — Security regression for F-1 (CRITICAL, zero-interaction RCE). +// +// The git-forensics scanner historically ran `execSync(`git ${cmd}`)`, interpolating +// attacker-controlled filenames (from `git ls-files` / `git log --name-only` of the +// SCANNED repo) into a shell string. A repo containing a file named +// `commands/$(touch INJECTED).md` therefore executed arbitrary code on the analyst's +// machine during `/security scan ` — before any confirmation, outside the +// git-clone OS sandbox. +// +// This test builds such a hostile fixture repo and asserts the injected `touch` +// never runs. It must FAIL against the vulnerable execSync() helper and PASS once +// `git()` uses spawnSync('git', [...args]) with no shell. + +import { describe, it } from 'node:test'; +import assert from 'node:assert/strict'; +import { spawnSync } from 'node:child_process'; +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 { scan } from '../../scanners/git-forensics.mjs'; + +const gitAvailable = spawnSync('git', ['--version'], { encoding: 'utf-8' }).status === 0; + +/** Initialise a hermetic git repo (no global hooks/signing) at `dir`. */ +function gitInit(dir) { + const run = (...args) => + spawnSync('git', args, { cwd: dir, encoding: 'utf-8', env: { ...process.env, GIT_CONFIG_NOSYSTEM: '1' } }); + run('init', '-q'); + run('config', 'user.email', 'test@example.com'); + run('config', 'user.name', 'Test'); + run('config', 'commit.gpgsign', 'false'); + run('config', 'core.hooksPath', '/dev/null'); + return run; +} + +describe('git-forensics shell-injection regression (F-1)', () => { + it('does not execute injected commands embedded in scanned filenames', { skip: !gitAvailable }, async () => { + const repo = mkdtempSync(join(tmpdir(), 'git-forensics-injection-')); + // The injected `touch INJECTED` runs with cwd = repo, so the sentinel lands here. + const sentinel = join(repo, 'INJECTED'); + try { + const run = gitInit(repo); + + // A benign first commit so the repo has history the scanner will walk. + writeFileSync(join(repo, 'README.md'), '# fixture\n'); + run('add', '-A'); + run('commit', '-q', '-m', 'initial'); + + // The payload: a file under commands/ whose NAME is a shell command + // substitution. It matches the scanner's `commands/*.md` pathspec, so the + // name is fed back into git show/log for description-drift analysis. + mkdirSync(join(repo, 'commands'), { recursive: true }); + const evilName = 'commands/$(touch INJECTED).md'; + writeFileSync( + join(repo, evilName), + '---\ndescription: original description text for drift baseline\n---\nbody\n', + ); + run('add', '-A'); + run('commit', '-q', '-m', 'add command'); + + assert.ok(!existsSync(sentinel), 'precondition: sentinel must not exist before scan'); + + resetCounter(); + const result = await scan(repo, {}); + + // The scanner must complete gracefully on a hostile repo... + assert.ok( + ['ok', 'skipped', 'error'].includes(result.status), + `scan should complete, got status '${result.status}'`, + ); + // ...and crucially must NOT have executed the injected command. + assert.ok( + !existsSync(sentinel), + 'SHELL INJECTION: scanning a repo with a $(...) filename executed the injected command', + ); + } finally { + rmSync(repo, { recursive: true, force: true }); + } + }); +});