From 58608966196896da7ff7c015dbc427a76088afd0 Mon Sep 17 00:00:00 2001 From: noamd Date: Tue, 5 Apr 2022 15:26:20 +0300 Subject: [PATCH] detect workflow_run as a dangerous trigger --- checks/dangerous_workflow.go | 57 ++++++++++++------- checks/dangerous_workflow_test.go | 11 ++++ ...attern-untrusted-checkout-workflow_run.yml | 39 +++++++++++++ docs/checks.md | 6 +- docs/checks/internal/checks.yaml | 4 +- 5 files changed, 92 insertions(+), 25 deletions(-) create mode 100644 checks/testdata/.github/workflows/github-workflow-dangerous-pattern-untrusted-checkout-workflow_run.yml diff --git a/checks/dangerous_workflow.go b/checks/dangerous_workflow.go index f9a0e342137..5512017a835 100644 --- a/checks/dangerous_workflow.go +++ b/checks/dangerous_workflow.go @@ -80,9 +80,11 @@ const ( type triggerName string var ( - triggerPullRequestTarget = triggerName("pull_request_target") - triggerPullRequest = triggerName("pull_request") - checkoutUntrustedRef = "github.event.pull_request" + triggerPullRequestTarget = triggerName("pull_request_target") + triggerWorkflowRun = triggerName("workflow_run") + triggerPullRequest = triggerName("pull_request") + checkoutUntrustedPullRequestRef = "github.event.pull_request" + checkoutUntrustedWorkflowRunRef = "github.event.workflow_run" ) // Holds stateful data to pass thru callbacks. @@ -168,7 +170,9 @@ func validateSecretsInPullRequests(workflow *actionlint.Workflow, path string, // We need pull request trigger. usesPullRequest := usesEventTrigger(workflow, triggerPullRequest) usesPullRequestTarget := usesEventTrigger(workflow, triggerPullRequestTarget) - if !usesPullRequest && !usesPullRequestTarget { + usesWorkflowRun := usesEventTrigger(workflow, triggerWorkflowRun) + + if !usesPullRequest && !usesPullRequestTarget && !usesWorkflowRun { return nil } @@ -179,6 +183,9 @@ func validateSecretsInPullRequests(workflow *actionlint.Workflow, path string, if usesPullRequestTarget { triggers[triggerPullRequestTarget] = usesPullRequestTarget } + if usesWorkflowRun { + triggers[triggerWorkflowRun] = usesWorkflowRun + } // Secrets used in env at the top of the wokflow. if err := checkWorkflowSecretInEnv(workflow, triggers, path, dl, pdata); err != nil { @@ -198,7 +205,7 @@ func validateSecretsInPullRequests(workflow *actionlint.Workflow, path string, func validateUntrustedCodeCheckout(workflow *actionlint.Workflow, path string, dl checker.DetailLogger, pdata *patternCbData, ) error { - if !usesEventTrigger(workflow, triggerPullRequestTarget) { + if !usesEventTrigger(workflow, triggerPullRequestTarget) && !usesEventTrigger(workflow, triggerWorkflowRun) { return nil } @@ -240,16 +247,7 @@ func checkJobForUsedSecrets(job *actionlint.Job, triggers map[triggerName]bool, // If the job has an environment, assume it's an env secret gated by // some approval and don't alert. - if jobUsesEnvironment(job) { - return nil - } - - // For pull request target, we need a ref to the pull request. - _, usesPullRequest := triggers[triggerPullRequest] - _, usesPullRequestTarget := triggers[triggerPullRequestTarget] - chk, ref := jobUsesCodeCheckout(job) - if !((chk && usesPullRequest) || - (chk && usesPullRequestTarget && strings.Contains(ref, checkoutUntrustedRef))) { + if !jobUsesCodeCheckoutAndNoEnvironment(job, triggers) { return nil } @@ -281,17 +279,32 @@ func workflowUsesCodeCheckoutAndNoEnvironment(workflow *actionlint.Workflow, return false } + for _, job := range workflow.Jobs { + if jobUsesCodeCheckoutAndNoEnvironment(job, triggers) { + return true + } + } + return false +} + +func jobUsesCodeCheckoutAndNoEnvironment(job *actionlint.Job, triggers map[triggerName]bool, +) bool { + if job == nil { + return false + } _, usesPullRequest := triggers[triggerPullRequest] _, usesPullRequestTarget := triggers[triggerPullRequestTarget] + _, usesWorkflowRun := triggers[triggerWorkflowRun] - for _, job := range workflow.Jobs { - chk, ref := jobUsesCodeCheckout(job) - if ((chk && usesPullRequest) || - (chk && usesPullRequestTarget && strings.Contains(ref, checkoutUntrustedRef))) && - !jobUsesEnvironment(job) { + chk, ref := jobUsesCodeCheckout(job) + if !jobUsesEnvironment(job) { + if (chk && usesPullRequest) || + (chk && usesPullRequestTarget && strings.Contains(ref, checkoutUntrustedPullRequestRef)) || + (chk && usesWorkflowRun && strings.Contains(ref, checkoutUntrustedWorkflowRunRef)) { return true } } + return false } @@ -348,7 +361,9 @@ func checkJobForUntrustedCodeCheckout(job *actionlint.Job, path string, if !ok || ref.Value == nil { continue } - if strings.Contains(ref.Value.Value, checkoutUntrustedRef) { + + if strings.Contains(ref.Value.Value, checkoutUntrustedPullRequestRef) || + strings.Contains(ref.Value.Value, checkoutUntrustedWorkflowRunRef) { line := fileparser.GetLineNumber(step.Pos) dl.Warn(&checker.LogMessage{ Path: path, diff --git a/checks/dangerous_workflow_test.go b/checks/dangerous_workflow_test.go index fc2e018faaf..28ecc63afd9 100644 --- a/checks/dangerous_workflow_test.go +++ b/checks/dangerous_workflow_test.go @@ -43,6 +43,17 @@ func TestGithubDangerousWorkflow(t *testing.T) { NumberOfDebug: 0, }, }, + { + name: "run untrusted code checkout test - workflow_run", + filename: "./testdata/.github/workflows/github-workflow-dangerous-pattern-untrusted-checkout-workflow_run.yml", + expected: scut.TestReturn{ + Error: nil, + Score: checker.MinResultScore, + NumberOfWarn: 2, + NumberOfInfo: 0, + NumberOfDebug: 0, + }, + }, { name: "run untrusted code checkout test", filename: "./testdata/.github/workflows/github-workflow-dangerous-pattern-untrusted-checkout.yml", diff --git a/checks/testdata/.github/workflows/github-workflow-dangerous-pattern-untrusted-checkout-workflow_run.yml b/checks/testdata/.github/workflows/github-workflow-dangerous-pattern-untrusted-checkout-workflow_run.yml new file mode 100644 index 00000000000..63ce0451e7c --- /dev/null +++ b/checks/testdata/.github/workflows/github-workflow-dangerous-pattern-untrusted-checkout-workflow_run.yml @@ -0,0 +1,39 @@ +# Copyright 2021 Security Scorecard Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +on: + workflow_run: + workflows: ['dummy'] + +jobs: + build: + name: Build and test + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + with: + ref: ${{ github.event.workflow_run }} + + - uses: actions/setup-node@v1 + - run: | + npm install + npm build + + - uses: completely/fakeaction@v2 + with: + arg1: ${{ secrets.supersecret }} + + - uses: fakerepo/comment-on-pr@v1 + with: + message: | + Thank you! diff --git a/docs/checks.md b/docs/checks.md index 997c0e61427..718b03836be 100644 --- a/docs/checks.md +++ b/docs/checks.md @@ -264,8 +264,8 @@ logging github context and secrets, or use of potentially untrusted inputs in sc The following patterns are checked: Untrusted Code Checkout: This is the misuse of potentially dangerous triggers. -This checks if a `pull_request_target` workflow trigger was used in conjunction -with an explicit pull request checkout. Workflows triggered with `pull_request_target` +This checks if a `pull_request_target` or `workflow_run` workflow trigger was used in conjunction +with an explicit pull request checkout. Workflows triggered with `pull_request_target` / `workflow_run` have write permission to the target repository and access to target repository secrets. With the PR checkout, PR authors may compromise the repository, for example, by using build scripts controlled by the author of the PR or reading @@ -606,8 +606,10 @@ possible. Risk: `Critical` (service possibly accessible to third parties) This check determines whether the webhook defined in the repository has a token configured to authenticate the origins of requests. + **Remediation steps** - Check whether your service supports token authentication. - If there is support for token authentication, set the secret in the webhook configuration. See [Setting up a webhook](https://docs.github.com/en/developers/webhooks-and-events/webhooks/creating-webhooks#setting-up-a-webhook) - If there is no support for token authentication, consider implementing it by following [these directions](https://docs.github.com/en/developers/webhooks-and-events/webhooks/securing-your-webhooks). + diff --git a/docs/checks/internal/checks.yaml b/docs/checks/internal/checks.yaml index 37310f4daf0..b21f6e631e9 100644 --- a/docs/checks/internal/checks.yaml +++ b/docs/checks/internal/checks.yaml @@ -674,8 +674,8 @@ checks: The following patterns are checked: Untrusted Code Checkout: This is the misuse of potentially dangerous triggers. - This checks if a `pull_request_target` workflow trigger was used in conjunction - with an explicit pull request checkout. Workflows triggered with `pull_request_target` + This checks if a `pull_request_target` or `workflow_run` workflow trigger was used in conjunction + with an explicit pull request checkout. Workflows triggered with `pull_request_target` / `workflow_run` have write permission to the target repository and access to target repository secrets. With the PR checkout, PR authors may compromise the repository, for example, by using build scripts controlled by the author of the PR or reading