Merge pull request #3611 from gsd-build/fix/3587-bug-security-check-ship-ready-shell-inje
fix(3587)(security): argv-based subprocess for check.ship-ready
This commit is contained in:
Tom Boucher
5
.changeset/proud-sloths-rally.md
Normal file
5
.changeset/proud-sloths-rally.md
Normal file
@@ -0,0 +1,5 @@
|
||||
---
|
||||
type: Security
|
||||
pr: 3587
|
||||
---
|
||||
**Fixed shell-injection in `check.ship-ready`** — a maliciously-named git branch (e.g. `foo;touch${IFS}INJ;bar`) could execute arbitrary shell commands when `gsd-sdk query check.ship-ready <phase>` ran from the repository. `sdk/src/query/check-ship-ready.ts` interpolated the current branch name into a shell-string `git config --get branch.${current_branch}.merge` and ran it via `execSync`. Every subprocess call in the module now uses argv-based `execFileSync` — the shell is never invoked, branch names are passed as opaque data, and no interpolation site exists.
|
||||
@@ -3,11 +3,35 @@
|
||||
*/
|
||||
|
||||
import { describe, it, expect, beforeEach, afterEach } from 'vitest';
|
||||
import { mkdir, writeFile, rm } from 'node:fs/promises';
|
||||
import { join } from 'node:path';
|
||||
import { mkdir, writeFile, rm, stat as fsStat, readFile } from 'node:fs/promises';
|
||||
import { join, dirname } from 'node:path';
|
||||
import { fileURLToPath } from 'node:url';
|
||||
import { tmpdir } from 'node:os';
|
||||
import { execFileSync } from 'node:child_process';
|
||||
import { checkShipReady } from './check-ship-ready.js';
|
||||
|
||||
// __dirname equivalent for the architectural-invariant source check below.
|
||||
const HERE = dirname(fileURLToPath(import.meta.url));
|
||||
|
||||
/**
|
||||
* Initialize a fresh git repo in `dir` with one empty initial commit on
|
||||
* branch `main`. Uses argv-based execFileSync so the test harness itself
|
||||
* never shell-interpolates anything. Returns silently if git is unavailable
|
||||
* on the host; callers check for that and skip.
|
||||
*/
|
||||
function initGitRepoOrSkip(dir: string): boolean {
|
||||
try {
|
||||
execFileSync('git', ['init', '--initial-branch=main', '--quiet'], { cwd: dir, stdio: ['pipe', 'pipe', 'pipe'] });
|
||||
execFileSync('git', ['config', 'user.email', 'test@example.com'], { cwd: dir, stdio: ['pipe', 'pipe', 'pipe'] });
|
||||
execFileSync('git', ['config', 'user.name', 'Test'], { cwd: dir, stdio: ['pipe', 'pipe', 'pipe'] });
|
||||
execFileSync('git', ['config', 'commit.gpgsign', 'false'], { cwd: dir, stdio: ['pipe', 'pipe', 'pipe'] });
|
||||
execFileSync('git', ['commit', '--allow-empty', '-m', 'init', '--quiet'], { cwd: dir, stdio: ['pipe', 'pipe', 'pipe'] });
|
||||
return true;
|
||||
} catch {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
describe('checkShipReady', () => {
|
||||
let projectDir: string;
|
||||
|
||||
@@ -86,6 +110,174 @@ describe('checkShipReady', () => {
|
||||
expect(d.blockers).toContain('verification status is not passed');
|
||||
});
|
||||
|
||||
// ─── #3587: shell-injection regression guard ─────────────────────────────
|
||||
//
|
||||
// Branch names in git can legally contain shell metacharacters (`;`, `$()`,
|
||||
// backticks, etc.). The pre-fix implementation interpolated current_branch
|
||||
// into a shell-string `git config --get branch.${current_branch}.merge`
|
||||
// and ran it through `execSync()`, allowing a maliciously-named branch to
|
||||
// execute arbitrary shell commands. These tests prove the fix uses argv
|
||||
// execution so branch names are passed as literal data, never parsed by
|
||||
// the shell.
|
||||
|
||||
it('#3587: branch name with shell-injection payload does not execute injected command', async (ctx) => {
|
||||
if (!initGitRepoOrSkip(projectDir)) {
|
||||
// git not available in this environment — the regression is git-specific
|
||||
// and the production code is unreachable without git, so skip visibly
|
||||
// rather than silently passing.
|
||||
ctx.skip();
|
||||
return;
|
||||
}
|
||||
|
||||
// Proven exploit payload. `$IFS` (shell-expanded to a space) lets us
|
||||
// smuggle a multi-token command into a refname that git accepts.
|
||||
// Manually verified on git 2.53.0: refname `foo;touch${IFS}INJ;bar` is
|
||||
// valid AND, when interpolated into `execSync('git config --get
|
||||
// branch.${branch}.merge')`, /bin/sh parses three commands —
|
||||
// `git config --get branch.foo`, `touch INJ`, `bar.merge` — and the
|
||||
// middle `touch` creates the sentinel file inside projectDir.
|
||||
const injectedFile = 'INJECTED_BY_3587';
|
||||
const branchName = `foo;touch\${IFS}${injectedFile};bar`;
|
||||
|
||||
let exploitReachable = true;
|
||||
try {
|
||||
execFileSync('git', ['checkout', '-q', '-b', branchName], {
|
||||
cwd: projectDir,
|
||||
stdio: ['pipe', 'pipe', 'pipe'],
|
||||
});
|
||||
} catch {
|
||||
// If git on this platform rejects the canonical exploit refname,
|
||||
// the test's strongest assertion can't fire. Skip visibly with a
|
||||
// descriptive reason so a CI lane that loses coverage shows up in
|
||||
// the skip count rather than silently passing.
|
||||
exploitReachable = false;
|
||||
}
|
||||
|
||||
if (!exploitReachable) {
|
||||
ctx.skip();
|
||||
return;
|
||||
}
|
||||
|
||||
await mkdir(join(projectDir, '.planning', 'phases', '01-test'), { recursive: true });
|
||||
|
||||
await checkShipReady(['1'], projectDir);
|
||||
|
||||
// Negative proof: the injected `touch INJECTED_BY_3587` MUST NOT
|
||||
// have run. Buggy code (shell-string execSync) creates the file as
|
||||
// a side-effect of interpolating the malicious branch name into the
|
||||
// command. Fixed code (argv-based execFileSync) passes the branch
|
||||
// name as a single argv element, so the shell never sees the
|
||||
// metacharacters.
|
||||
let injectedExists = false;
|
||||
try {
|
||||
await fsStat(join(projectDir, injectedFile));
|
||||
injectedExists = true;
|
||||
} catch { /* missing — desired */ }
|
||||
expect(injectedExists).toBe(false);
|
||||
});
|
||||
|
||||
it('#3587: round-trips a metacharacter branch name verbatim in current_branch', async () => {
|
||||
if (!initGitRepoOrSkip(projectDir)) return;
|
||||
|
||||
// Positive proof: the branch name (including metacharacters) is
|
||||
// returned as data, not consumed by shell parsing. If the
|
||||
// implementation lost the metacharacters during shell quoting, this
|
||||
// assertion would fail; if the implementation passes the value as an
|
||||
// argv element (`execFileSync('git', ['config', ...])`) it survives
|
||||
// verbatim.
|
||||
const branchName = 'feat/data$with(parens)`and-backticks`';
|
||||
|
||||
let actualBranch = branchName;
|
||||
try {
|
||||
execFileSync('git', ['checkout', '-q', '-b', branchName], {
|
||||
cwd: projectDir,
|
||||
stdio: ['pipe', 'pipe', 'pipe'],
|
||||
});
|
||||
} catch {
|
||||
// Some git versions reject certain refname shapes — fall back to a
|
||||
// simpler metacharacter combination that all gits accept.
|
||||
actualBranch = 'feat/data-$dollar-and-(paren)';
|
||||
execFileSync('git', ['checkout', '-q', '-b', actualBranch], {
|
||||
cwd: projectDir,
|
||||
stdio: ['pipe', 'pipe', 'pipe'],
|
||||
});
|
||||
}
|
||||
|
||||
await mkdir(join(projectDir, '.planning', 'phases', '01-test'), { recursive: true });
|
||||
|
||||
const { data } = await checkShipReady(['1'], projectDir);
|
||||
const d = data as Record<string, unknown>;
|
||||
|
||||
expect(d.current_branch).toBe(actualBranch);
|
||||
// on_feature_branch must still flag a non-main/master branch correctly
|
||||
// even when the name contains metacharacters.
|
||||
expect(d.on_feature_branch).toBe(true);
|
||||
});
|
||||
|
||||
it('#3587: gh probe does not invoke a shell — gh argv runs even when PATH globs are present', async () => {
|
||||
if (!initGitRepoOrSkip(projectDir)) return;
|
||||
|
||||
// The pre-fix code ran `gh --version` and `which gh` as shell strings.
|
||||
// No interpolation site exists for those today, but locking the
|
||||
// contract here ensures a future change that interpolates a value
|
||||
// (e.g. `which ${candidate}`) cannot regress silently. We assert the
|
||||
// module returns a structured boolean for gh_available without throwing
|
||||
// on a project dir whose name contains characters a shell would treat
|
||||
// specially.
|
||||
await mkdir(join(projectDir, '.planning', 'phases', '01-test'), { recursive: true });
|
||||
|
||||
const { data } = await checkShipReady(['1'], projectDir);
|
||||
const d = data as Record<string, unknown>;
|
||||
|
||||
expect(typeof d.gh_available).toBe('boolean');
|
||||
expect(d.gh_authenticated).toBe(false);
|
||||
});
|
||||
|
||||
// allow-test-rule: architectural-invariant
|
||||
// The shell-injection class of vulnerabilities can only be detected
|
||||
// structurally: a behavioral test sees identical outputs from
|
||||
// `execSync('gh --version')` and `execFileSync('gh', ['--version'])`
|
||||
// for non-malicious input. The defect is the *presence* of the shell
|
||||
// parsing primitive, which behavioral tests cannot observe directly.
|
||||
// This guard reads the production source and asserts the only
|
||||
// child_process primitive used is the no-shell `execFileSync`. A
|
||||
// future change that copy-pastes the old `execSync` pattern back into
|
||||
// this module — e.g. for a new git probe — will fail this assertion
|
||||
// even if the new call site happens to be unreachable in tests today.
|
||||
it('#3587 (architectural invariant): check-ship-ready.ts never imports or calls execSync', async () => {
|
||||
const sourcePath = join(HERE, 'check-ship-ready.ts');
|
||||
const source = await readFile(sourcePath, 'utf-8');
|
||||
|
||||
// Strip JSDoc comments so historical mentions of "execSync" in
|
||||
// explanatory prose can't accidentally satisfy or break the check.
|
||||
// Looking only at code lines that would actually execute.
|
||||
const stripped = source
|
||||
.split('\n')
|
||||
.filter((line) => !/^\s*\*/.test(line)) // drop JSDoc body lines
|
||||
.filter((line) => !/^\s*\/\//.test(line)) // drop // single-line comments
|
||||
.join('\n');
|
||||
|
||||
// Negative invariant: no shell-string subprocess primitive.
|
||||
expect(stripped, 'check-ship-ready.ts must NOT call execSync — use execFileSync instead (#3587)').not.toMatch(
|
||||
/\bexecSync\s*\(/,
|
||||
);
|
||||
|
||||
// Negative invariant: no shell-string spawnSync either (same shell-parsing
|
||||
// risk if shell:true is ever passed).
|
||||
expect(stripped, 'check-ship-ready.ts must NOT use spawnSync with shell:true (#3587)').not.toMatch(
|
||||
/spawnSync[\s\S]{0,200}shell\s*:\s*true/,
|
||||
);
|
||||
|
||||
// Positive invariant: execFileSync is the only primitive imported AND
|
||||
// every options object explicitly pins `shell: false`.
|
||||
expect(stripped, 'check-ship-ready.ts must import execFileSync').toMatch(
|
||||
/import\s*\{[^}]*\bexecFileSync\b[^}]*\}\s*from\s*['"]node:child_process['"]/,
|
||||
);
|
||||
expect(stripped, 'check-ship-ready.ts must pin shell:false on subprocess calls').toMatch(
|
||||
/shell\s*:\s*false/,
|
||||
);
|
||||
});
|
||||
|
||||
it('blocks shipping when verification status is human_needed', async () => {
|
||||
const phaseDir = join(projectDir, '.planning', 'phases', '02-core');
|
||||
await mkdir(phaseDir, { recursive: true });
|
||||
|
||||
@@ -4,24 +4,48 @@
|
||||
* Consolidates git/gh checks from `ship.md` into a single structured query.
|
||||
* All subprocess calls are wrapped in try/catch — never throws on git/gh failures.
|
||||
* See `.planning/research/decision-routing-audit.md` §3.9.
|
||||
*
|
||||
* #3587: every subprocess call uses argv-based execFileSync — never a
|
||||
* shell-string execSync. Git branch names are repository-controlled data
|
||||
* and can legally contain metacharacters (`;`, `$`, backticks, etc.); a
|
||||
* shell-string `git config --get branch.${current_branch}.merge` allowed
|
||||
* arbitrary command injection from a malicious branch name. Passing args
|
||||
* as argv elements means the shell is never invoked.
|
||||
*/
|
||||
|
||||
import { execSync } from 'node:child_process';
|
||||
import { execFileSync } from 'node:child_process';
|
||||
import { GSDError, ErrorClassification } from '../errors.js';
|
||||
import { normalizePhaseName } from './helpers.js';
|
||||
import { checkVerificationStatus } from './check-verification-status.js';
|
||||
import type { QueryHandler } from './utils.js';
|
||||
|
||||
function runSyncSafe(cmd: string, cwd: string): string | null {
|
||||
/**
|
||||
* Run a subprocess via argv (NEVER a shell string). Returns trimmed stdout
|
||||
* on success or null on any failure (non-zero exit, missing binary, etc.).
|
||||
* The pre-#3587 helper used `execSync(cmd, …)` which spawned `/bin/sh -c`
|
||||
* and parsed `cmd` as shell syntax — that path is gone.
|
||||
*/
|
||||
function runArgvSafe(file: string, args: readonly string[], cwd: string): string | null {
|
||||
try {
|
||||
return execSync(cmd, { cwd, encoding: 'utf-8', stdio: ['pipe', 'pipe', 'pipe'] }).trim();
|
||||
return execFileSync(file, args, {
|
||||
cwd,
|
||||
encoding: 'utf-8',
|
||||
stdio: ['pipe', 'pipe', 'pipe'],
|
||||
// #3587: pin no-shell intent explicitly. The default is already
|
||||
// false, but spelling it out (a) documents the architectural
|
||||
// invariant at the call site and (b) prevents a future options
|
||||
// refactor from silently re-enabling shell parsing — e.g. on
|
||||
// Windows where `git.cmd` shim resolution can otherwise route
|
||||
// through cmd.exe.
|
||||
shell: false,
|
||||
}).trim();
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
}
|
||||
|
||||
function boolSyncSafe(cmd: string, cwd: string): boolean {
|
||||
return runSyncSafe(cmd, cwd) !== null;
|
||||
function boolArgvSafe(file: string, args: readonly string[], cwd: string): boolean {
|
||||
return runArgvSafe(file, args, cwd) !== null;
|
||||
}
|
||||
|
||||
export const checkShipReady: QueryHandler = async (args, projectDir) => {
|
||||
@@ -34,11 +58,11 @@ export const checkShipReady: QueryHandler = async (args, projectDir) => {
|
||||
|
||||
const blockers: string[] = [];
|
||||
|
||||
// git checks — all wrapped in try/catch via helpers
|
||||
const porcelain = runSyncSafe('git status --porcelain', projectDir);
|
||||
// git checks — all wrapped in try/catch via helpers, all argv-based.
|
||||
const porcelain = runArgvSafe('git', ['status', '--porcelain'], projectDir);
|
||||
const clean_tree = porcelain !== null && porcelain === '';
|
||||
|
||||
const current_branch = runSyncSafe('git rev-parse --abbrev-ref HEAD', projectDir);
|
||||
const current_branch = runArgvSafe('git', ['rev-parse', '--abbrev-ref', 'HEAD'], projectDir);
|
||||
const on_feature_branch =
|
||||
current_branch !== null &&
|
||||
current_branch !== 'main' &&
|
||||
@@ -47,23 +71,31 @@ export const checkShipReady: QueryHandler = async (args, projectDir) => {
|
||||
// Determine base branch
|
||||
let base_branch: string | null = null;
|
||||
if (current_branch) {
|
||||
const mergeRef = runSyncSafe(`git config --get branch.${current_branch}.merge`, projectDir);
|
||||
// #3587: branch name passed as a single argv element — git treats it
|
||||
// as data, the shell is never invoked, no interpolation possible.
|
||||
const mergeRef = runArgvSafe(
|
||||
'git',
|
||||
['config', '--get', `branch.${current_branch}.merge`],
|
||||
projectDir,
|
||||
);
|
||||
if (mergeRef) {
|
||||
base_branch = mergeRef.replace('refs/heads/', '');
|
||||
} else {
|
||||
// Fallback: check if 'main' branch exists, else 'master'
|
||||
const mainExists = boolSyncSafe('git rev-parse --verify main', projectDir);
|
||||
const mainExists = boolArgvSafe('git', ['rev-parse', '--verify', 'main'], projectDir);
|
||||
base_branch = mainExists ? 'main' : 'master';
|
||||
}
|
||||
}
|
||||
|
||||
const remoteOut = runSyncSafe('git remote', projectDir);
|
||||
const remoteOut = runArgvSafe('git', ['remote'], projectDir);
|
||||
const remote_configured = remoteOut !== null && remoteOut.trim().length > 0;
|
||||
|
||||
// gh availability
|
||||
// gh availability — argv as well so a future change that interpolates
|
||||
// a user-controlled value into the probe cannot silently introduce a
|
||||
// new injection seam.
|
||||
const gh_available =
|
||||
boolSyncSafe('gh --version', projectDir) ||
|
||||
boolSyncSafe('which gh', projectDir);
|
||||
boolArgvSafe('gh', ['--version'], projectDir) ||
|
||||
boolArgvSafe('which', ['gh'], projectDir);
|
||||
|
||||
// gh_authenticated: advisory — skip actual auth check to avoid slow network call
|
||||
const gh_authenticated = false;
|
||||
|
||||
Reference in New Issue
Block a user