fix(llm-security): F-1 — eliminate shell-injection RCE in git-forensics scanner
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 <url>`: 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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V3s6WnubSSrFjAQTLQdVbG
This commit is contained in:
parent
a96e3b947f
commit
e46b5a3256
4 changed files with 129 additions and 36 deletions
0
--json
0
--json
|
|
@ -1 +0,0 @@
|
|||
1775452698205
|
||||
|
|
@ -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',
|
||||
|
|
|
|||
81
tests/scanners/git-injection.test.mjs
Normal file
81
tests/scanners/git-injection.test.mjs
Normal file
|
|
@ -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 <url>` — 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 });
|
||||
}
|
||||
});
|
||||
});
|
||||
Loading…
Add table
Add a link
Reference in a new issue