diff --git a/pkg/scanner/rules.go b/pkg/scanner/rules.go index 9b0d995..c951a09 100644 --- a/pkg/scanner/rules.go +++ b/pkg/scanner/rules.go @@ -1656,6 +1656,13 @@ func isTrustedRef(ref string) bool { if strings.Contains(lower, "pull_request.base.") { return true } + // github.workflow_sha = the commit of the workflow file (the base/trusted ref + // on pull_request_target), and github.sha = the base commit on + // pull_request_target. Both check out trusted code, unlike ...head.sha (fork). + // "github.sha" does not appear as a substring of "...head.sha", so this is safe. + if strings.Contains(lower, "workflow_sha") || strings.Contains(lower, "github.sha") { + return true + } return false } @@ -2610,6 +2617,30 @@ func CheckCurlPipeBash(wf *Workflow) []Finding { return findings } +// localActionUnderTrustedCheckout reports whether a `uses: .//...` local +// action resolves to a directory that a checkout of a trusted ref populated +// (e.g. `actions/checkout` at `github.workflow_sha` into `path: `), so the +// action definition comes from trusted code, not the fork. +func localActionUnderTrustedCheckout(job Job, uses string) bool { + p := strings.TrimPrefix(uses, "./") + seg := p + if i := strings.Index(p, "/"); i != -1 { + seg = p[:i] + } + if seg == "" || seg == "." { + return false + } + for _, step := range job.Steps { + if !isCheckoutAction(step.Uses) { + continue + } + if normalizePathspec(step.With["path"]) == seg && isTrustedRef(step.With["ref"]) { + 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 { @@ -2660,6 +2691,12 @@ func CheckLocalActionUntrustedCheckout(wf *Workflow) []Finding { for _, step := range job.Steps[checkoutIdx+1:] { if strings.HasPrefix(step.Uses, "./") { + // The local action lives under a subdir populated by a checkout of + // a trusted ref (e.g. actions checked out at github.workflow_sha + // into path/), so its definition is not attacker-controlled. + if localActionUnderTrustedCheckout(job, step.Uses) { + 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 a060b8d..9530fdb 100644 --- a/pkg/scanner/rules_fixtures_test.go +++ b/pkg/scanner/rules_fixtures_test.go @@ -1201,6 +1201,17 @@ func TestCheckLocalActionUntrustedCheckout_TrustedRefIsolated(t *testing.T) { } } +// A local action loaded from a subdir populated by a checkout of the trusted +// github.workflow_sha (not the fork) is not attacker-controlled. Regression for +// leanprover-community/mathlib4's PR_summary.yml. +func TestCheckLocalActionUntrustedCheckout_WorkflowShaDir(t *testing.T) { + wf := loadFixture(t, "local-action-workflow-sha.yaml") + findings := CheckLocalActionUntrustedCheckout(wf) + if len(findings) != 0 { + t.Fatalf("expected 0 findings for local action under trusted workflow_sha checkout, got %d: %v", len(findings), findings) + } +} + // A run block that fetches the PR head to a named ref but never checks it out // leaves fork code off the working tree; a local action loaded afterward comes // from the base clone, not the fork. No FG-016 finding. diff --git a/pkg/scanner/rules_test.go b/pkg/scanner/rules_test.go index f8d537a..114606d 100644 --- a/pkg/scanner/rules_test.go +++ b/pkg/scanner/rules_test.go @@ -949,6 +949,29 @@ func TestCheckLifecycleInstallBeforeCredentialedOperation_SelfUpdatePlusDeps(t * } } +func TestIsTrustedRef(t *testing.T) { + trusted := []string{ + "main", "master", "${{ github.base_ref }}", + "${{ github.event.pull_request.base.sha }}", + "${{ github.workflow_sha }}", "${{ github.sha }}", + } + for _, r := range trusted { + if !isTrustedRef(r) { + t.Errorf("expected %q to be trusted", r) + } + } + untrusted := []string{ + "${{ github.event.pull_request.head.sha }}", + "${{ github.event.pull_request.head.ref }}", + "${{ github.head_ref }}", + } + for _, r := range untrusted { + if isTrustedRef(r) { + t.Errorf("expected %q to NOT be trusted (fork head)", r) + } + } +} + // --- AllRules completeness --- func TestAllRules_IncludesNewRules(t *testing.T) { diff --git a/test/fixtures/local-action-workflow-sha.yaml b/test/fixtures/local-action-workflow-sha.yaml new file mode 100644 index 0000000..c909dfd --- /dev/null +++ b/test/fixtures/local-action-workflow-sha.yaml @@ -0,0 +1,20 @@ +name: PR Summary +on: pull_request_target +permissions: + contents: read + pull-requests: write +jobs: + summary: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + ref: ${{ github.event.pull_request.head.sha }} + path: pr-branch + persist-credentials: false + - uses: actions/checkout@v4 + with: + ref: ${{ github.workflow_sha }} + sparse-checkout: .github/actions + path: workflow-actions + - uses: ./workflow-actions/.github/actions/get-ci