diff --git a/packages/core/src/ci-environment/git.test.ts b/packages/core/src/ci-environment/git.test.ts index 7e44fd98..387659c6 100644 --- a/packages/core/src/ci-environment/git.test.ts +++ b/packages/core/src/ci-environment/git.test.ts @@ -35,34 +35,59 @@ describe.skip("#getRepositoryURL", () => { }); }); -describe("#getMergeBaseCommitSha (command injection)", () => { +describe("injection through the branch or commit of the CI environment", () => { let cwd: string; + let root: string; let repoDir: string; let markerFile: string; + /** SHA of the tip of `main`, pushed to origin. */ + let mainSha: string; + let warn: ReturnType; + + const git = (...args: string[]) => + execFileSync("git", ["-C", repoDir, ...args]) + .toString() + .trim(); beforeEach(() => { cwd = process.cwd(); - const root = mkdtempSync(join(tmpdir(), "argos-git-test-")); + root = mkdtempSync(join(tmpdir(), "argos-git-injection-test-")); repoDir = join(root, "repo"); markerFile = join(root, "pwned"); // A bare repo acts as the "origin" remote so that git fetch has a - // reachable target. + // reachable target. A local path goes through git's file transport, which + // runs `--upload-pack` through a shell like the ssh transport does. const bareDir = join(root, "origin.git"); execFileSync("git", ["init", "--bare", bareDir]); execFileSync("git", ["init", repoDir]); - const git = (...args: string[]) => - execFileSync("git", ["-C", repoDir, ...args]); git("remote", "add", "origin", bareDir); + git("config", "user.email", "test@argos-ci.com"); + git("config", "user.name", "Argos Test"); + git("commit", "--allow-empty", "-m", "initial"); + git("branch", "-M", "main"); + git("push", "origin", "main"); + mainSha = git("rev-parse", "HEAD"); + warn = vi.spyOn(console, "warn").mockImplementation(() => {}); process.chdir(repoDir); }); afterEach(() => { process.chdir(cwd); - rmSync(join(repoDir, ".."), { recursive: true, force: true }); + warn.mockRestore(); + rmSync(root, { recursive: true, force: true }); }); + /** + * Mirrors the GHSA-v58q-fvq4-9vh4 PoC: a ref name starting with a dash is + * accepted by `git check-ref-format` and can be pushed, but once in a + * positional argument git parses it as an option. `--upload-pack` makes git + * run the given program through a shell on ssh:// and file:// origins. + */ + const getOptionInjection = () => + `--upload-pack=touch\${IFS}${markerFile};true`; + it("does not execute shell metacharacters in the branch name", async () => { // Mirrors the GHSA-4x45-gxvp-6283 PoC: a ref containing $() command // substitution must be passed to git as a literal argument, never @@ -79,6 +104,52 @@ describe("#getMergeBaseCommitSha (command injection)", () => { expect(existsSync(markerFile)).toBe(false); }); + + it("does not let a branch name starting with a dash inject a git option", async () => { + const malicious = getOptionInjection(); + + const sha = await getMergeBaseCommitSha({ base: "main", head: malicious }); + + expect(existsSync(markerFile)).toBe(false); + // The ref exists neither on origin nor locally. + expect(sha).toBe(null); + }); + + it("does not let a reference commit starting with a dash inject a git option", async () => { + const malicious = getOptionInjection(); + + const ancestors = await listAncestorCommits({ sha: malicious, limit: 10 }); + + expect(existsSync(markerFile)).toBe(false); + expect(ancestors).toEqual([]); + }); + + it("does not let a commit starting with a dash inject a git option", async () => { + const malicious = getOptionInjection(); + + const parents = await getCommitParents(malicious); + + expect(existsSync(markerFile)).toBe(false); + expect(parents).toBe(null); + }); + + it("fetches a branch whose name starts with a dash as a ref", async () => { + // `git branch` refuses such a name but `git push` accepts it as a refspec + // destination, so it can reach the CI environment. It is only on origin: + // the local history has no ref to fall back to. + git("checkout", "-q", "-b", "tmp"); + git("commit", "--allow-empty", "-m", "dashed commit"); + const dashedSha = git("rev-parse", "HEAD"); + git("push", "origin", "tmp:refs/heads/-dashed"); + git("checkout", "-q", "main"); + git("branch", "-D", "tmp"); + + const sha = await getMergeBaseCommitSha({ base: "main", head: "-dashed" }); + + expect(sha).toBe(mainSha); + expect(git("rev-parse", "--verify", "argos/-dashed")).toBe(dashedSha); + expect(warn).not.toHaveBeenCalled(); + }); }); describe("#getMergeBaseCommitSha (refs missing on origin)", () => { diff --git a/packages/core/src/ci-environment/git.ts b/packages/core/src/ci-environment/git.ts index 9cbb9948..f9259c6e 100644 --- a/packages/core/src/ci-environment/git.ts +++ b/packages/core/src/ci-environment/git.ts @@ -5,6 +5,16 @@ import { debug, isDebugEnabled } from "../debug"; const execFileAsync = promisify(execFile); +// The branches and commits passed to git come from the CI environment, where +// anyone opening a pull request names a branch. Two rules keep them from being +// interpreted as anything but a ref: +// - git is spawned without a shell (`execFile`), so shell metacharacters are +// never evaluated (GHSA-4x45-gxvp-6283); +// - they are always passed after `--end-of-options` (git 2.24+), as git parses +// any argument starting with a dash as an option: a branch named +// `--upload-pack=` would otherwise make git run `` on +// ssh:// and file:// origins (GHSA-v58q-fvq4-9vh4). + /** * Check if the current directory is a git repository. */ @@ -68,7 +78,12 @@ export function getRepositoryURL() { */ function gitMergeBase(input: { base: string; head: string }) { try { - return execFileSync("git", ["merge-base", input.head, input.base]) + return execFileSync("git", [ + "merge-base", + "--end-of-options", + input.head, + input.base, + ]) .toString() .trim(); } catch (error) { @@ -111,39 +126,45 @@ function checkIsGitLockError(error: unknown): boolean { } /** - * Run `git fetch` with the given arguments. + * Run `git fetch` from origin with the given options and refs (or refspecs). * * Retries on lock contention (`.git/shallow.lock` "File exists") with an * exponential backoff, since this is usually a transient conflict with another * git process and resolves once that process releases the lock. */ -function runGitFetch(args: string[]) { - return pRetry(() => execFileAsync("git", ["fetch", ...args]), { - retries: 3, - minTimeout: 500, - shouldRetry: ({ error }) => checkIsGitLockError(error), - onFailedAttempt: ({ error, retriesLeft, retryDelay }) => { - if (checkIsGitLockError(error) && retriesLeft > 0) { - debug( - `git fetch failed on lock contention, retrying in ${retryDelay}ms (${retriesLeft} left)`, - ); - } +function runGitFetch(input: { options: string[]; refs: string[] }) { + return pRetry( + () => + execFileAsync("git", [ + "fetch", + ...input.options, + "--end-of-options", + "origin", + ...input.refs, + ]), + { + retries: 3, + minTimeout: 500, + shouldRetry: ({ error }) => checkIsGitLockError(error), + onFailedAttempt: ({ error, retriesLeft, retryDelay }) => { + if (checkIsGitLockError(error) && retriesLeft > 0) { + debug( + `git fetch failed on lock contention, retrying in ${retryDelay}ms (${retriesLeft} left)`, + ); + } + }, }, - }); + ); } /** * Run git fetch with a specific ref and depth. */ async function gitFetch(input: { ref: string; depth: number; target: string }) { - await runGitFetch([ - "--force", - "--update-head-ok", - "--depth", - String(input.depth), - "origin", - `${input.ref}:${input.target}`, - ]); + await runGitFetch({ + options: ["--force", "--update-head-ok", "--depth", String(input.depth)], + refs: [`${input.ref}:${input.target}`], + }); } /** @@ -180,6 +201,7 @@ function checkLocalRefExists(ref: string): boolean { "rev-parse", "--verify", "--quiet", + "--end-of-options", `${ref}^{commit}`, ]); return true; @@ -316,7 +338,7 @@ function listShas(path: string, maxCount?: number): string[] { if (maxCount) { args.push(`--max-count=${maxCount}`); } - args.push(path); + args.push("--end-of-options", path); const raw = execFileSync("git", args); const shas = raw.toString().trim().split("\n"); return shas; @@ -338,7 +360,7 @@ export async function listAncestorCommits(input: { // Fetch one extra commit since the commit itself is excluded from the result. const depth = input.limit + 1; try { - await runGitFetch([`--depth=${depth}`, "origin", input.sha]); + await runGitFetch({ options: [`--depth=${depth}`], refs: [input.sha] }); } catch (error) { debug( `Failed to deepen history for ${input.sha}, using local history`, @@ -371,7 +393,7 @@ export async function getCommitParents(sha: string): Promise { // Fetch the commit and its parents, so the parents stop being hidden by the // boundary of a shallow history. try { - await runGitFetch(["--depth=2", "origin", sha]); + await runGitFetch({ options: ["--depth=2"], refs: [sha] }); } catch (error) { debug( `Failed to deepen history for ${sha}, using local history`, @@ -393,7 +415,7 @@ function readCommitParents(sha: string): string[] | null { try { const raw = execFileSync( "git", - ["rev-list", "--parents", "-n", "1", sha, "--"], + ["rev-list", "--parents", "-n", "1", "--end-of-options", sha, "--"], { stdio: ["ignore", "pipe", "pipe"] }, ); const [, ...parents] = raw.toString().trim().split(" ");