diff --git a/pkg/scanner/rules.go b/pkg/scanner/rules.go index 5e8f4d0..f277b6a 100644 --- a/pkg/scanner/rules.go +++ b/pkg/scanner/rules.go @@ -247,6 +247,19 @@ func CheckPwnRequest(wf *Workflow) []Finding { mitigated = true } + // Environment gate: the executing job is bound to a deployment + // environment, which may carry required-reviewer protection rules — a + // fork PR would then pause for manual approval by a trusted team before + // any step runs. Protection rules aren't visible in the YAML, so this is + // a one-level downgrade, not a suppression (an environment with no rules + // gates nothing). + if job.Environment != "" && !mitigated { + severity = downgradeBy(severity, 1) + mitigation.Details = append(mitigation.Details, fmt.Sprintf( + "job gated on environment '%s' (may require deployment approval)", job.Environment)) + mitigated = true + } + // Path isolation adjusts confidence, not severity if mitigation.PathIsolated && confidence == ConfidenceConfirmed { confidence = ConfidencePatternOnly @@ -2110,6 +2123,19 @@ func CheckOIDCMisconfiguration(wf *Workflow) []Finding { severity = SeverityCritical } + // Environment gate: a job bound to a deployment environment + // (`environment:`) has its secrets AND its execution scoped to that + // environment, which may carry required-reviewer protection rules — + // in which case a fork PR pauses for manual approval by a trusted team + // before any step (or OIDC request) runs. Protection rules aren't + // visible in the workflow YAML, so this is a one-level downgrade, not a + // suppression (an environment with no rules gates nothing). + envNote := "" + if job.Environment != "" { + severity = downgradeBy(severity, 1) + envNote = fmt.Sprintf(" (job gated on environment '%s' — may require deployment approval)", job.Environment) + } + msg := fmt.Sprintf("OIDC Misconfiguration: id-token:write on pull_request_target workflow (job '%s')", jobName) if len(cloudActions) > 0 { msg += fmt.Sprintf(" with cloud auth (%s)", strings.Join(cloudActions, ", ")) @@ -2117,6 +2143,7 @@ func CheckOIDCMisconfiguration(wf *Workflow) []Finding { if checkoutsFork { msg += " — attacker can mint cloud credentials via fork PR" } + msg += envNote findings = append(findings, Finding{ RuleID: "FG-008", @@ -2713,6 +2740,47 @@ func localActionUnderTrustedCheckout(job Job, uses string) bool { return false } +// localActionStagedBeforeForkCheckout reports whether a parent-escaping local +// action (`uses: ./..//...`) resolves to a sibling directory that a run +// step populated BEFORE the untrusted (fork) checkout at forkCheckoutIdx. +// +// This recognizes GitHub's recommended pull_request_target hardening idiom: +// check out the trusted base ref, copy the scripts/actions you intend to +// execute into a directory OUTSIDE the workspace (e.g. `../base-branch/`), then +// check out the fork PR head into the workspace. `actions/checkout` only writes +// within GITHUB_WORKSPACE, so a `..`-escaping path can never be populated by the +// fork checkout; its contents are whatever the trusted pre-checkout run steps +// staged there. +func localActionStagedBeforeForkCheckout(job Job, uses string, forkCheckoutIdx int) bool { + p := strings.TrimPrefix(uses, "./") + // Only credit paths that escape the workspace via `..` — an in-workspace + // `./.github/...` path IS overwritten by the fork checkout and stays untrusted. + if !strings.HasPrefix(p, "../") { + return false + } + seg := "" + for _, s := range strings.Split(p, "/") { + if s == "" || s == "." || s == ".." { + continue + } + seg = s + break + } + if seg == "" || forkCheckoutIdx < 0 { + return false + } + // Require a run step BEFORE the fork checkout that references the sibling + // dir — the trusted staging (`mkdir -p ../base/... && cp ... ../base/...`). + // Requiring it to precede the untrusted checkout guarantees the staged + // content came from the trusted tree, not the fork. + for i := 0; i < forkCheckoutIdx && i < len(job.Steps); i++ { + if run := job.Steps[i].Run; run != "" && strings.Contains(run, seg) { + return true + } + } + return false +} + // CheckLocalActionUntrustedCheckout detects local action usage after fork checkout (FG-016). func CheckLocalActionUntrustedCheckout(wf *Workflow) []Finding { if !wf.On.PullRequestTarget && !wf.On.IssueComment && !wf.On.WorkflowRun { @@ -2759,6 +2827,13 @@ func CheckLocalActionUntrustedCheckout(wf *Workflow) []Finding { if hasAuthorAssociationGuard(job) || containsForkGuard(job.If) { severity = SeverityInfo guardNote = " (mitigated: job restricts execution to trusted authors / non-forks)" + } else if postCheckout := job.Steps[checkoutIdx+1:]; (jobHasNoToken(wf, job) || jobHasReadOnlyToken(wf, job)) && !postCheckoutAccessesSecrets(postCheckout) { + // Read-only (or empty) token + no secrets referenced: the fork-controlled + // action.yml still runs, but the token can't push/comment and there is + // nothing to exfiltrate. This is normal `pull_request` CI (build/lint the + // fork). Mirrors FG-001's ReadOnlyToken/NoCredentialExec mitigation. + severity = SeverityInfo + guardNote = " (mitigated: read-only token, no secrets in scope)" } for _, step := range job.Steps[checkoutIdx+1:] { @@ -2769,6 +2844,16 @@ func CheckLocalActionUntrustedCheckout(wf *Workflow) []Finding { if localActionUnderTrustedCheckout(job, step.Uses) { continue } + // The local action lives in a parent-escaping sibling dir + // (`./..//...`) that a run step staged from trusted code + // BEFORE the untrusted checkout. `actions/checkout` writes only + // inside the workspace, so a `..`-escaping path is never populated + // by the fork checkout — its contents come from the trusted + // pre-checkout staging (GitHub's recommended "copy trusted scripts + // out of the checkout" hardening idiom). + if localActionStagedBeforeForkCheckout(job, step.Uses, checkoutIdx) { + continue + } findings = append(findings, Finding{ RuleID: "FG-016", Severity: severity, diff --git a/pkg/scanner/rules_fixtures_test.go b/pkg/scanner/rules_fixtures_test.go index e675320..b824227 100644 --- a/pkg/scanner/rules_fixtures_test.go +++ b/pkg/scanner/rules_fixtures_test.go @@ -223,6 +223,26 @@ func TestCheckPwnRequest_NoTokenExec(t *testing.T) { } } +// FG-001 must downgrade one level when the executing job is bound to a +// deployment environment (which may require reviewer approval before a fork PR +// runs). The environment fixture has id-token:write (not read-only), so the +// baseline is critical and the env gate drops it to high. Regression for +// cilium/cilium build-images-base.yaml. +func TestCheckPwnRequest_EnvironmentGated(t *testing.T) { + wf := loadFixture(t, "oidc-prt-environment-gated.yaml") + findings := CheckPwnRequest(wf) + if len(findings) != 1 { + t.Fatalf("expected 1 FG-001 finding, got %d: %v", len(findings), findings) + } + f := findings[0] + if f.Severity != SeverityHigh { + t.Errorf("expected high severity (critical downgraded by environment gate), got %s", f.Severity) + } + if !strings.Contains(f.Message, "environment") { + t.Errorf("expected environment mitigation note, got: %s", f.Message) + } +} + // Attacker-controllable expressions (PR title on pull_request_target, used in a // non-echo command) must stay high — the dispatch-input downgrade must not touch them. func TestCheckScriptInjection_PRTitleStaysHigh(t *testing.T) { @@ -687,6 +707,25 @@ func TestCheckOIDC_PushOnly(t *testing.T) { } } +// A PRT job with id-token:write + fork checkout that is bound to a deployment +// environment downgrades one level (critical -> high): the environment may carry +// required-reviewer protection gating the OIDC request behind manual approval. +// Regression for cilium/cilium build-images-base.yaml. +func TestCheckOIDC_EnvironmentGated(t *testing.T) { + wf := loadFixture(t, "oidc-prt-environment-gated.yaml") + findings := CheckOIDCMisconfiguration(wf) + + if len(findings) != 1 { + t.Fatalf("expected 1 finding, got %d", len(findings)) + } + if findings[0].Severity != SeverityHigh { + t.Errorf("expected high (PRT + fork checkout downgraded by environment gate), got %s", findings[0].Severity) + } + if !strings.Contains(findings[0].Message, "environment") { + t.Error("expected message to mention the environment gate") + } +} + // --- FG-009 Self-Hosted Runner tests --- func TestCheckSelfHosted_PR(t *testing.T) { @@ -1284,6 +1323,33 @@ func TestCheckLocalActionUntrustedCheckout_AuthorGuarded(t *testing.T) { } } +// FG-016 must downgrade to info when the executing job has a read-only token +// (permissions: read-all) and references no secrets — fork code runs but the +// token can't write and there is nothing to exfiltrate (normal pull_request CI). +// Regression for cilium/cilium lint-build-commits.yaml. +func TestCheckLocalActionUntrustedCheckout_ReadOnlyToken(t *testing.T) { + wf := loadFixture(t, "local-action-readonly-token.yaml") + findings := CheckLocalActionUntrustedCheckout(wf) + if len(findings) != 1 { + t.Fatalf("expected 1 FG-016 finding, got %d: %v", len(findings), findings) + } + if findings[0].Severity != SeverityInfo { + t.Errorf("expected info severity for read-only-token job, got %s", findings[0].Severity) + } +} + +// FG-016 must not fire when the local action lives in a parent-escaping sibling +// dir (`./../base-branch/...`) that a run step staged from trusted code BEFORE +// the untrusted checkout — actions/checkout can't write to a `..`-escaping path, +// so its contents are trusted. Regression for cilium/cilium build-images-base.yaml. +func TestCheckLocalActionUntrustedCheckout_StagedSiblingDir(t *testing.T) { + wf := loadFixture(t, "local-action-staged-sibling-dir.yaml") + findings := CheckLocalActionUntrustedCheckout(wf) + if len(findings) != 0 { + t.Fatalf("expected 0 findings for trusted staged sibling-dir action, got %d: %v", len(findings), findings) + } +} + // --- FG-027 TOCTOU Label Gate tests --- // Vulnerable pattern: pull_request_target with synchronize, job gated on an diff --git a/test/fixtures/local-action-readonly-token.yaml b/test/fixtures/local-action-readonly-token.yaml new file mode 100644 index 0000000..1c3db3c --- /dev/null +++ b/test/fixtures/local-action-readonly-token.yaml @@ -0,0 +1,22 @@ +name: Build Commits +# Plain pull_request (fork code runs) with a read-only token and no secrets: +# normal CI. The workflow_run trigger is what makes FG-016 consider the workflow, +# but read-all + no secrets means the fork-controlled local action has nothing to +# steal and a token that can't write. +on: + pull_request: {} + workflow_run: + workflows: ["Cache Cleaner"] + branches: [main] + types: [completed] +permissions: read-all +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + ref: ${{ github.event.pull_request.head.sha }} + persist-credentials: false + - uses: ./.github/actions/disk-cleanup + - run: make build diff --git a/test/fixtures/local-action-staged-sibling-dir.yaml b/test/fixtures/local-action-staged-sibling-dir.yaml new file mode 100644 index 0000000..b3bc8f5 --- /dev/null +++ b/test/fixtures/local-action-staged-sibling-dir.yaml @@ -0,0 +1,31 @@ +name: Base Image Release Build +# GitHub's recommended pull_request_target hardening: check out the trusted base +# ref, copy the actions/scripts you intend to run into a directory OUTSIDE the +# workspace, THEN check out the fork PR head. The `./../base-branch/...` action is +# staged from trusted code before the untrusted checkout, so it is not attacker- +# controlled — actions/checkout can't write to a `..`-escaping sibling dir. +on: + pull_request_target: + types: [opened, synchronize, reopened] +permissions: + contents: read +jobs: + build-and-push: + runs-on: ubuntu-latest + steps: + - name: Checkout base branch (trusted) + uses: actions/checkout@v4 + with: + ref: ${{ github.base_ref }} + persist-credentials: false + - name: Copy trusted action out of the workspace + run: | + mkdir -p ../base-branch/.github/actions + cp -r .github/actions/cosign ../base-branch/.github/actions/ + - name: Checkout PR head (NOT TRUSTED) + uses: actions/checkout@v4 + with: + ref: ${{ github.event.pull_request.head.sha }} + persist-credentials: false + - name: Sign with trusted (staged) action + uses: ./../base-branch/.github/actions/cosign diff --git a/test/fixtures/oidc-prt-environment-gated.yaml b/test/fixtures/oidc-prt-environment-gated.yaml new file mode 100644 index 0000000..424e3f5 --- /dev/null +++ b/test/fixtures/oidc-prt-environment-gated.yaml @@ -0,0 +1,23 @@ +name: Base Image Release Build +# pull_request_target with id-token:write AND a fork-head checkout, but the job is +# bound to a deployment environment. If that environment carries required-reviewer +# protection, a fork PR pauses for manual approval before any step (or OIDC token +# request) runs. Protection rules aren't visible in YAML, so FG-008/FG-001 apply a +# one-level downgrade (critical -> high), not a suppression. +on: + pull_request_target: + types: [opened, synchronize, reopened] +permissions: + contents: read + id-token: write +jobs: + build-and-push: + runs-on: ubuntu-latest + environment: + name: release-base-images + steps: + - uses: actions/checkout@v4 + with: + ref: ${{ github.event.pull_request.head.sha }} + persist-credentials: false + - run: make build