From 46ebdcf931dcf54463dde54aee6a7cee31a10723 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Wed, 27 Oct 2021 10:56:07 -0500 Subject: [PATCH 01/10] add dangerous workflow check with untrusted code checkout pattern Signed-off-by: Asra Ali --- checks/dangerous_workflow.go | 262 ++++++++++++++++++ checks/dangerous_workflow_test.go | 98 +++++++ ...low-dangerous-pattern-default-checkout.yml | 38 +++ ...orkflow-dangerous-pattern-safe-trigger.yml | 38 +++ ...low-dangerous-pattern-trusted-checkout.yml | 38 +++ ...w-dangerous-pattern-untrusted-checkout.yml | 38 +++ docs/checks.md | 15 + docs/checks/internal/checks.yaml | 25 ++ e2e/dangerous_workflow_test.go | 60 ++++ 9 files changed, 612 insertions(+) create mode 100644 checks/dangerous_workflow.go create mode 100644 checks/dangerous_workflow_test.go create mode 100644 checks/testdata/github-workflow-dangerous-pattern-default-checkout.yml create mode 100644 checks/testdata/github-workflow-dangerous-pattern-safe-trigger.yml create mode 100644 checks/testdata/github-workflow-dangerous-pattern-trusted-checkout.yml create mode 100644 checks/testdata/github-workflow-dangerous-pattern-untrusted-checkout.yml create mode 100644 e2e/dangerous_workflow_test.go diff --git a/checks/dangerous_workflow.go b/checks/dangerous_workflow.go new file mode 100644 index 00000000000..18889c12cdc --- /dev/null +++ b/checks/dangerous_workflow.go @@ -0,0 +1,262 @@ +// 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. + +package checks + +import ( + "fmt" + "strings" + + "gopkg.in/yaml.v3" + + "github.com/ossf/scorecard/v3/checker" + sce "github.com/ossf/scorecard/v3/errors" +) + +// CheckDangerousWorkflow is the exported name for Dangerous-Workflow check. +const CheckDangerousWorkflow = "Dangerous-Workflow" + +//nolint:gochecknoinits +func init() { + registerCheck(CheckDangerousWorkflow, DangerousWorkflow) +} + +// Holds stateful data to pass thru callbacks. +// Each field correpsonds to a dangerous GitHub workflow pattern, and +// will hold true if the pattern is avoided, false otherwise. +type patternCbData struct { + workflowPattern map[string]bool +} + +// TokenPermissions runs Token-Permissions check. +func DangerousWorkflow(c *checker.CheckRequest) checker.CheckResult { + // data is shared across all GitHub workflows. + data := patternCbData{ + workflowPattern: make(map[string]bool), + } + err := CheckFilesContent(".github/workflows/*", false, + c, validateGitHubActionWorkflowPatterns, &data) + return createResultForDangerousWorkflowPatterns(data, err) +} + +// Check file content. +func validateGitHubActionWorkflowPatterns(path string, content []byte, dl checker.DetailLogger, + data FileCbData) (bool, error) { + // Verify the type of the data. + pdata, ok := data.(*patternCbData) + if !ok { + // This never happens. + panic("invalid type") + } + + if !CheckFileContainsCommands(content, "#") { + return true, nil + } + + var workflow map[interface{}]interface{} + err := yaml.Unmarshal(content, &workflow) + if err != nil { + return false, + sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("yaml.Unmarshal: %v", err)) + } + + // 1. Check for untrusted code checkout with pull_request_target and a ref + if err := validateUntrustedCodeCheckout(workflow, path, dl, pdata); err != nil { + return false, err + } + + // TODO: Check other dangerous patterns. + return true, nil +} + +func validateUntrustedCodeCheckout(config map[interface{}]interface{}, path string, + dl checker.DetailLogger, pdata *patternCbData) error { + isPullRequestTrigger, err := checkPullRequestTrigger(config) + if err != nil { + return err + } + + if isPullRequestTrigger { + return validateUntrustedCodeCheckoutRef(config, path, dl, pdata) + } + + return nil +} + +func validateUntrustedCodeCheckoutRef(config map[interface{}]interface{}, path string, + dl checker.DetailLogger, pdata *patternCbData) error { + var jobs interface{} + + // Now check if this is used with untrusted code checkout ref in jobs + jobs, ok := config["jobs"] + if !ok { + return nil + } + + mjobs, ok := jobs.(map[string]interface{}) + if !ok { + return sce.WithMessage(sce.ErrScorecardInternal, errInvalidGitHubWorkflow.Error()) + } + + for _, value := range mjobs { + job, ok := value.(map[string]interface{}) + if !ok { + return sce.WithMessage(sce.ErrScorecardInternal, errInvalidGitHubWorkflow.Error()) + } + + if err := checkJobForUntrustedCodeCheckout(job, path, dl, pdata); err != nil { + return err + } + } + return nil +} + +func checkPullRequestTrigger(config map[interface{}]interface{}) (bool, error) { + // Check event trigger (required) is pull_request_target + trigger, ok := config["on"] + if !ok { + return false, sce.WithMessage(sce.ErrScorecardInternal, errInvalidGitHubWorkflow.Error()) + } + + isPullRequestTrigger := false + switch val := trigger.(type) { + case string: + if strings.EqualFold(val, "pull_request_target") { + isPullRequestTrigger = true + } + case []string: + for _, onVal := range val { + if strings.EqualFold(onVal, "pull_request_target") { + isPullRequestTrigger = true + } + } + case map[interface{}]interface{}: + for k := range val { + key, ok := k.(string) + if !ok { + return false, sce.WithMessage(sce.ErrScorecardInternal, errInvalidGitHubWorkflow.Error()) + } + if strings.EqualFold(key, "pull_request_target") { + isPullRequestTrigger = true + } + } + default: + return false, sce.WithMessage(sce.ErrScorecardInternal, errInvalidGitHubWorkflow.Error()) + } + return isPullRequestTrigger, nil +} + +func checkJobForUntrustedCodeCheckout(job map[string]interface{}, path string, + dl checker.DetailLogger, pdata *patternCbData) error { + steps, ok := job["steps"] + if !ok { + return nil + } + msteps, ok := steps.([]interface{}) + if !ok { + return sce.WithMessage(sce.ErrScorecardInternal, errInvalidGitHubWorkflow.Error()) + } + // Check each step, which is a map, for checkouts with untrusted ref + for _, step := range msteps { + mstep, ok := step.(map[string]interface{}) + if !ok { + return sce.WithMessage(sce.ErrScorecardInternal, errInvalidGitHubWorkflow.Error()) + } + // Check for a step that uses actions/checkout + uses, ok := mstep["uses"] + if !ok { + continue + } + muses, ok := uses.(string) + if !ok { + return sce.WithMessage(sce.ErrScorecardInternal, errInvalidGitHubWorkflow.Error()) + } + // Uses defaults if not defined. + with, ok := mstep["with"] + if !ok { + continue + } + mwith, ok := with.(map[string]interface{}) + if !ok { + return sce.WithMessage(sce.ErrScorecardInternal, errInvalidGitHubWorkflow.Error()) + } + // Check for reference. If not defined, uses default main branch. + ref, ok := mwith["ref"] + if !ok { + continue + } + mref, ok := ref.(string) + if !ok { + return sce.WithMessage(sce.ErrScorecardInternal, errInvalidGitHubWorkflow.Error()) + } + if strings.Contains(muses, "actions/checkout") && + strings.Contains(mref, "${{ github.event.pull_request.head.sha }}") { + dl.Warn3(&checker.LogMessage{ + Path: path, + Type: checker.FileTypeSource, + // TODO: set line correctly. + Offset: 1, + Text: fmt.Sprintf("untrusted code checkout '%v'", mref), + // TODO: set Snippet. + }) + // Detected untrusted checkout. + pdata.workflowPattern["untrusted_checkout"] = true + } + } + return nil +} + +// Calculate the workflow score. +func calculateWorkflowScore(result patternCbData) int { + // Start with a perfect score. + score := float32(checker.MaxResultScore) + + // pull_request_event indicates untrusted code checkout + if ok := result.workflowPattern["untrusted_checkout"]; ok { + score -= 9 + } + + // We're done, calculate the final score. + if score < checker.MinResultScore { + return checker.MinResultScore + } + + return int(score) +} + +// Create the result. +func createResultForDangerousWorkflowPatterns(result patternCbData, err error) checker.CheckResult { + if err != nil { + return checker.CreateRuntimeErrorResult(CheckDangerousWorkflow, err) + } + + score := calculateWorkflowScore(result) + + if score != checker.MaxResultScore { + return checker.CreateResultWithScore(CheckDangerousWorkflow, + "dangerous workflow patterns detected", score) + } + + return checker.CreateMaxScoreResult(CheckDangerousWorkflow, + "workflow patterns pass dangerous workflow check") +} + +func testValidateGitHubActionDangerousWOrkflow(pathfn string, + content []byte, dl checker.DetailLogger) checker.CheckResult { + data := patternCbData{ + workflowPattern: make(map[string]bool), + } + _, err := validateGitHubActionWorkflowPatterns(pathfn, content, dl, &data) + return createResultForDangerousWorkflowPatterns(data, err) +} diff --git a/checks/dangerous_workflow_test.go b/checks/dangerous_workflow_test.go new file mode 100644 index 00000000000..4b1f2e26fd2 --- /dev/null +++ b/checks/dangerous_workflow_test.go @@ -0,0 +1,98 @@ +// 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. + +package checks + +import ( + "fmt" + "io/ioutil" + "testing" + + "github.com/ossf/scorecard/v3/checker" + scut "github.com/ossf/scorecard/v3/utests" +) + +func TestGithubDangerousWorkflow(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + filename string + expected scut.TestReturn + }{ + { + name: "run untrusted code checkout test", + filename: "./testdata/github-workflow-dangerous-pattern-untrusted-checkout.yml", + expected: scut.TestReturn{ + Error: nil, + Score: checker.MinResultScore, + NumberOfWarn: 0, + NumberOfInfo: 1, + NumberOfDebug: 0, + }, + }, + { + name: "run trusted code checkout test", + filename: "./testdata/github-workflow-dangerous-pattern-trusted-checkout.yml", + expected: scut.TestReturn{ + Error: nil, + Score: checker.MaxResultScore, + NumberOfWarn: 0, + NumberOfInfo: 0, + NumberOfDebug: 0, + }, + }, + { + name: "run default code checkout test", + filename: "./testdata/github-workflow-dangerous-pattern-default-checkout.yml", + expected: scut.TestReturn{ + Error: nil, + Score: checker.MaxResultScore, + NumberOfWarn: 0, + NumberOfInfo: 0, + NumberOfDebug: 0, + }, + }, + { + name: "run safe trigger with code checkout test", + filename: "./testdata/github-workflow-dangerous-pattern-safe-trigger.yml", + expected: scut.TestReturn{ + Error: nil, + Score: checker.MaxResultScore, + NumberOfWarn: 0, + NumberOfInfo: 0, + NumberOfDebug: 0, + }, + }, + } + for _, tt := range tests { + tt := tt // Re-initializing variable so it is not changed while executing the closure below + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + var content []byte + var err error + if tt.filename == "" { + content = make([]byte, 0) + } else { + content, err = ioutil.ReadFile(tt.filename) + if err != nil { + panic(fmt.Errorf("cannot read file: %w", err)) + } + } + dl := scut.TestDetailLogger{} + r := testValidateGitHubActionDangerousWOrkflow(tt.filename, content, &dl) + scut.ValidateTestReturn(t, tt.name, &tt.expected, &r, &dl) + }) + } +} diff --git a/checks/testdata/github-workflow-dangerous-pattern-default-checkout.yml b/checks/testdata/github-workflow-dangerous-pattern-default-checkout.yml new file mode 100644 index 00000000000..12c33a292b4 --- /dev/null +++ b/checks/testdata/github-workflow-dangerous-pattern-default-checkout.yml @@ -0,0 +1,38 @@ +# 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: + pull_request_target + +jobs: + build: + name: Build and test + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + with: + fetch-depth: 0 + + - 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! \ No newline at end of file diff --git a/checks/testdata/github-workflow-dangerous-pattern-safe-trigger.yml b/checks/testdata/github-workflow-dangerous-pattern-safe-trigger.yml new file mode 100644 index 00000000000..6fa2c1c1b70 --- /dev/null +++ b/checks/testdata/github-workflow-dangerous-pattern-safe-trigger.yml @@ -0,0 +1,38 @@ +# 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: + pull_request + +jobs: + build: + name: Build and test + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + with: + ref: my-branch + + - 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! \ No newline at end of file diff --git a/checks/testdata/github-workflow-dangerous-pattern-trusted-checkout.yml b/checks/testdata/github-workflow-dangerous-pattern-trusted-checkout.yml new file mode 100644 index 00000000000..474f218f3c6 --- /dev/null +++ b/checks/testdata/github-workflow-dangerous-pattern-trusted-checkout.yml @@ -0,0 +1,38 @@ +# 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: + pull_request_target + +jobs: + build: + name: Build and test + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + with: + ref: ${{ github.event.pull_request.head.sha }} + + - 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! \ No newline at end of file diff --git a/checks/testdata/github-workflow-dangerous-pattern-untrusted-checkout.yml b/checks/testdata/github-workflow-dangerous-pattern-untrusted-checkout.yml new file mode 100644 index 00000000000..272192d1ec9 --- /dev/null +++ b/checks/testdata/github-workflow-dangerous-pattern-untrusted-checkout.yml @@ -0,0 +1,38 @@ +# 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: + pull_request_target + +jobs: + build: + name: Build and test + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + with: + ref: ${{ github.event.pull_request.head.sha }} + + - 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! \ No newline at end of file diff --git a/docs/checks.md b/docs/checks.md index 068ba1c55bf..a3caa5a5f2b 100644 --- a/docs/checks.md +++ b/docs/checks.md @@ -522,3 +522,18 @@ possible. **Remediation steps** - Fix the vulnerabilities. The details of each vulnerability can be found on . +## Dangerous + +Risk: `High` (vulnerable to repository compromise) + +This check determines whether the project has dangerous code patterns. + +The first check is 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` 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 token in memory. + +This check does not detect whether untrusted code checkouts are used safely, for example, only on pull request that have been assigned a label. + +The highest score is awarded when all workflows avoid the dangerous code patterns. + +**Remediation steps** +- Avoid the dangerous workflow patterns. See this [advisory](https://github.com/justinsteven/advisories/blob/master/2021_github_actions_checkspelling_token_leak_via_advice_symlink.md) for information on avoiding untrusted code checkouts. + diff --git a/docs/checks/internal/checks.yaml b/docs/checks/internal/checks.yaml index 8b881c3d799..dd39b06335a 100644 --- a/docs/checks/internal/checks.yaml +++ b/docs/checks/internal/checks.yaml @@ -614,3 +614,28 @@ checks: - >- Fix the vulnerabilities. The details of each vulnerability can be found on . + Dangerous-Workflow: + risk: High + tags: supply-chain, security, infrastructure + short: Determines if the project's workflows avoid dangerous patterns. + description: | + Risk: `High` (vulnerable to repository compromise) + + This check determines whether the project has dangerous code patterns. + + The first check is 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` 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 token in memory. + + This check does not detect whether untrusted code checkouts are used safely, for + example, only on pull request that have been assigned a label. + + The highest score is awarded when all workflows avoid the dangerous code patterns. + remediation: + - >- + Avoid the dangerous workflow patterns. See this [advisory](https://github.com/justinsteven/advisories/blob/master/2021_github_actions_checkspelling_token_leak_via_advice_symlink.md) + for information on avoiding untrusted code checkouts. + diff --git a/e2e/dangerous_workflow_test.go b/e2e/dangerous_workflow_test.go new file mode 100644 index 00000000000..914983c7b67 --- /dev/null +++ b/e2e/dangerous_workflow_test.go @@ -0,0 +1,60 @@ +// 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. +package e2e + +import ( + "context" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + + "github.com/ossf/scorecard/v3/checker" + "github.com/ossf/scorecard/v3/checks" + "github.com/ossf/scorecard/v3/clients/githubrepo" + scut "github.com/ossf/scorecard/v3/utests" +) + +var _ = Describe("E2E TEST:"+checks.CheckTokenPermissions, func() { + Context("E2E TEST:Validating dangerous workflow check", func() { + It("Should return dangerous workflow works", func() { + dl := scut.TestDetailLogger{} + repo, err := githubrepo.MakeGithubRepo("ossf-tests/scorecard-check-dangerous-workflow-e2e") + Expect(err).Should(BeNil()) + repoClient := githubrepo.CreateGithubRepoClient(context.Background(), logger) + err = repoClient.InitRepo(repo) + Expect(err).Should(BeNil()) + req := checker.CheckRequest{ + Ctx: context.Background(), + RepoClient: repoClient, + Repo: repo, + Dlogger: &dl, + } + expected := scut.TestReturn{ + Error: nil, + Score: checker.MinResultScore, + NumberOfWarn: 1, + NumberOfInfo: 0, + NumberOfDebug: 0, + } + result := checks.DangerousWorkflow(&req) + // UPGRADEv2: to remove. + // Old version. + + Expect(result.Error).Should(BeNil()) + Expect(result.Pass).Should(BeFalse()) + // New version. + Expect(scut.ValidateTestReturn(nil, "dangerous workflow", &expected, &result, &dl)).Should(BeTrue()) + }) + }) +}) From 55c9b673476feebc833012602ba587215b3a95c3 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Thu, 28 Oct 2021 13:26:49 -0500 Subject: [PATCH 02/10] update Signed-off-by: Asra Ali --- checks/dangerous_workflow.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/checks/dangerous_workflow.go b/checks/dangerous_workflow.go index 18889c12cdc..bfa44a94b2f 100644 --- a/checks/dangerous_workflow.go +++ b/checks/dangerous_workflow.go @@ -82,12 +82,12 @@ func validateGitHubActionWorkflowPatterns(path string, content []byte, dl checke func validateUntrustedCodeCheckout(config map[interface{}]interface{}, path string, dl checker.DetailLogger, pdata *patternCbData) error { - isPullRequestTrigger, err := checkPullRequestTrigger(config) + checkPullRequestTrigger, err := checkPullRequestTrigger(config) if err != nil { return err } - if isPullRequestTrigger { + if checkPullRequestTrigger { return validateUntrustedCodeCheckoutRef(config, path, dl, pdata) } @@ -201,7 +201,7 @@ func checkJobForUntrustedCodeCheckout(job map[string]interface{}, path string, return sce.WithMessage(sce.ErrScorecardInternal, errInvalidGitHubWorkflow.Error()) } if strings.Contains(muses, "actions/checkout") && - strings.Contains(mref, "${{ github.event.pull_request.head.sha }}") { + strings.Contains(mref, "github.event.pull_request.head.sha") { dl.Warn3(&checker.LogMessage{ Path: path, Type: checker.FileTypeSource, @@ -224,7 +224,7 @@ func calculateWorkflowScore(result patternCbData) int { // pull_request_event indicates untrusted code checkout if ok := result.workflowPattern["untrusted_checkout"]; ok { - score -= 9 + score -= 10 } // We're done, calculate the final score. @@ -249,7 +249,7 @@ func createResultForDangerousWorkflowPatterns(result patternCbData, err error) c } return checker.CreateMaxScoreResult(CheckDangerousWorkflow, - "workflow patterns pass dangerous workflow check") + "no dangerous workflow patterns detected") } func testValidateGitHubActionDangerousWOrkflow(pathfn string, From 030c8ce95b531562e6003995a1f4292cdbc9a7a6 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Mon, 1 Nov 2021 17:26:38 -0500 Subject: [PATCH 03/10] add env var Signed-off-by: Asra Ali --- cmd/root.go | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/cmd/root.go b/cmd/root.go index 472d36766a2..4731a29cc2b 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -97,6 +97,16 @@ func checksHavePolicies(sp *spol.ScorecardPolicy, enabledChecks checker.CheckNam return true } +func getAllChecks() checker.CheckNameToFnMap { + // Returns the full list of checks, given any environment variable constraints. + possibleChecks := checks.AllChecks + // TODO: Remove this to enable the DANGEROUS_WORKFLOW by default in the next release. + if _, dangerousWorkflowCheck := os.LookupEnv("ENABLE_DANGEROUS_WORKFLOW"); !dangerousWorkflowCheck { + delete(possibleChecks, checks.CheckDangerousWorkflow) + } + return possibleChecks +} + func getEnabledChecks(sp *spol.ScorecardPolicy, argsChecks []string) (checker.CheckNameToFnMap, error) { enabledChecks := checker.CheckNameToFnMap{} @@ -118,7 +128,7 @@ func getEnabledChecks(sp *spol.ScorecardPolicy, argsChecks []string) (checker.Ch } } default: - enabledChecks = checks.AllChecks + enabledChecks = getAllChecks() } // If a policy was passed as argument, ensure all checks @@ -395,7 +405,7 @@ func fetchGitRepositoryFromRubyGems(packageName string) (string, error) { // Enables checks by name. func enableCheck(checkName string, enabledChecks *checker.CheckNameToFnMap) bool { if enabledChecks != nil { - for key, checkFn := range checks.AllChecks { + for key, checkFn := range getAllChecks() { if strings.EqualFold(key, checkName) { (*enabledChecks)[key] = checkFn return true From 5ca892322d27595f7567e31fd5a2d14b344d96e9 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Mon, 1 Nov 2021 17:45:17 -0500 Subject: [PATCH 04/10] fix comment Signed-off-by: Asra Ali --- checks/dangerous_workflow.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/checks/dangerous_workflow.go b/checks/dangerous_workflow.go index bfa44a94b2f..ff2970856f0 100644 --- a/checks/dangerous_workflow.go +++ b/checks/dangerous_workflow.go @@ -39,7 +39,7 @@ type patternCbData struct { workflowPattern map[string]bool } -// TokenPermissions runs Token-Permissions check. +// DangerousWorkflow runs Dangerous-Workflow check. func DangerousWorkflow(c *checker.CheckRequest) checker.CheckResult { // data is shared across all GitHub workflows. data := patternCbData{ From d215700e0da29989289e2b908bdea4be6edeaae2 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Tue, 2 Nov 2021 15:02:12 -0500 Subject: [PATCH 05/10] add repos git checks.yaml Signed-off-by: Asra Ali --- docs/checks/internal/checks.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/checks/internal/checks.yaml b/docs/checks/internal/checks.yaml index dd39b06335a..6aa66f9d1d9 100644 --- a/docs/checks/internal/checks.yaml +++ b/docs/checks/internal/checks.yaml @@ -617,6 +617,7 @@ checks: Dangerous-Workflow: risk: High tags: supply-chain, security, infrastructure + repos: GitHub, local short: Determines if the project's workflows avoid dangerous patterns. description: | Risk: `High` (vulnerable to repository compromise) From e081ba606fd01e33556ebb978c874689c576996d Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Tue, 2 Nov 2021 15:54:03 -0500 Subject: [PATCH 06/10] update checks.md Signed-off-by: Asra Ali --- docs/checks.md | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/docs/checks.md b/docs/checks.md index a3caa5a5f2b..fd251f63901 100644 --- a/docs/checks.md +++ b/docs/checks.md @@ -237,6 +237,28 @@ participants. **Remediation steps** - Ask contributors to [join their respective organizations](https://docs.github.com/en/organizations/managing-membership-in-your-organization/inviting-users-to-join-your-organization), if they have not already. Otherwise, there is no remediation for this check; it simply provides insight into which organizations have contributed so that you can make a trust-based decision based on that information. +## Dangerous-Workflow + +Risk: `High` (vulnerable to repository compromise) + +This check determines whether the project has dangerous code patterns. + +The first check is 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` 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 token in memory. + +This check does not detect whether untrusted code checkouts are used safely, for +example, only on pull request that have been assigned a label. + +The highest score is awarded when all workflows avoid the dangerous code patterns. + + +**Remediation steps** +- Avoid the dangerous workflow patterns. See this [advisory](https://github.com/justinsteven/advisories/blob/master/2021_github_actions_checkspelling_token_leak_via_advice_symlink.md) for information on avoiding untrusted code checkouts. + ## Dependency-Update-Tool Risk: `High` (possibly vulnerable to attacks on known flaws) @@ -522,18 +544,3 @@ possible. **Remediation steps** - Fix the vulnerabilities. The details of each vulnerability can be found on . -## Dangerous - -Risk: `High` (vulnerable to repository compromise) - -This check determines whether the project has dangerous code patterns. - -The first check is 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` 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 token in memory. - -This check does not detect whether untrusted code checkouts are used safely, for example, only on pull request that have been assigned a label. - -The highest score is awarded when all workflows avoid the dangerous code patterns. - -**Remediation steps** -- Avoid the dangerous workflow patterns. See this [advisory](https://github.com/justinsteven/advisories/blob/master/2021_github_actions_checkspelling_token_leak_via_advice_symlink.md) for information on avoiding untrusted code checkouts. - From 8a08c0ddeca09f3580c8b71a3fd11cf4244e0061 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Thu, 4 Nov 2021 12:50:58 -0500 Subject: [PATCH 07/10] address comments Signed-off-by: Asra Ali --- checks/dangerous_workflow.go | 7 ++++++- checks/dangerous_workflow_test.go | 11 +++++++++++ docs/checks.md | 2 +- docs/checks/internal/checks.yaml | 2 +- 4 files changed, 19 insertions(+), 3 deletions(-) diff --git a/checks/dangerous_workflow.go b/checks/dangerous_workflow.go index ff2970856f0..130d9f49593 100644 --- a/checks/dangerous_workflow.go +++ b/checks/dangerous_workflow.go @@ -53,6 +53,10 @@ func DangerousWorkflow(c *checker.CheckRequest) checker.CheckResult { // Check file content. func validateGitHubActionWorkflowPatterns(path string, content []byte, dl checker.DetailLogger, data FileCbData) (bool, error) { + if !isWorkflowFile(path) { + return true, nil + } + // Verify the type of the data. pdata, ok := data.(*patternCbData) if !ok { @@ -191,7 +195,8 @@ func checkJobForUntrustedCodeCheckout(job map[string]interface{}, path string, if !ok { return sce.WithMessage(sce.ErrScorecardInternal, errInvalidGitHubWorkflow.Error()) } - // Check for reference. If not defined, uses default main branch. + // Check for reference. If not defined for a pull_request_target event, this defaults to + // the base branch of the pull request. ref, ok := mwith["ref"] if !ok { continue diff --git a/checks/dangerous_workflow_test.go b/checks/dangerous_workflow_test.go index 4b1f2e26fd2..4f1a7a035b4 100644 --- a/checks/dangerous_workflow_test.go +++ b/checks/dangerous_workflow_test.go @@ -31,6 +31,17 @@ func TestGithubDangerousWorkflow(t *testing.T) { filename string expected scut.TestReturn }{ + { + name: "Non-yaml file", + filename: "./testdata/script.sh", + expected: scut.TestReturn{ + Error: nil, + Score: checker.MaxResultScore, + NumberOfWarn: 0, + NumberOfInfo: 0, + NumberOfDebug: 0, + }, + }, { name: "run untrusted code checkout test", filename: "./testdata/github-workflow-dangerous-pattern-untrusted-checkout.yml", diff --git a/docs/checks.md b/docs/checks.md index fd251f63901..ee63a043353 100644 --- a/docs/checks.md +++ b/docs/checks.md @@ -257,7 +257,7 @@ The highest score is awarded when all workflows avoid the dangerous code pattern **Remediation steps** -- Avoid the dangerous workflow patterns. See this [advisory](https://github.com/justinsteven/advisories/blob/master/2021_github_actions_checkspelling_token_leak_via_advice_symlink.md) for information on avoiding untrusted code checkouts. +- Avoid the dangerous workflow patterns. See this [post](https://securitylab.github.com/research/github-actions-preventing-pwn-requests/) for information on avoiding untrusted code checkouts. ## Dependency-Update-Tool diff --git a/docs/checks/internal/checks.yaml b/docs/checks/internal/checks.yaml index 94ad623b632..bf1c6ee5951 100644 --- a/docs/checks/internal/checks.yaml +++ b/docs/checks/internal/checks.yaml @@ -651,6 +651,6 @@ checks: The highest score is awarded when all workflows avoid the dangerous code patterns. remediation: - >- - Avoid the dangerous workflow patterns. See this [advisory](https://github.com/justinsteven/advisories/blob/master/2021_github_actions_checkspelling_token_leak_via_advice_symlink.md) + Avoid the dangerous workflow patterns. See this [post](https://securitylab.github.com/research/github-actions-preventing-pwn-requests/) for information on avoiding untrusted code checkouts. From 0b0b56e16aeef925e214fe6110869999f0f78851 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Tue, 9 Nov 2021 14:19:43 -0600 Subject: [PATCH 08/10] fix merge Signed-off-by: Asra Ali --- checks/dangerous_workflow.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/checks/dangerous_workflow.go b/checks/dangerous_workflow.go index 130d9f49593..52575d4c2a2 100644 --- a/checks/dangerous_workflow.go +++ b/checks/dangerous_workflow.go @@ -21,6 +21,7 @@ import ( "gopkg.in/yaml.v3" "github.com/ossf/scorecard/v3/checker" + "github.com/ossf/scorecard/v3/checks/fileparser" sce "github.com/ossf/scorecard/v3/errors" ) @@ -53,7 +54,7 @@ func DangerousWorkflow(c *checker.CheckRequest) checker.CheckResult { // Check file content. func validateGitHubActionWorkflowPatterns(path string, content []byte, dl checker.DetailLogger, data FileCbData) (bool, error) { - if !isWorkflowFile(path) { + if !fileparser.IsWorkflowFile(path) { return true, nil } From 08d7255d6ef702c1395dd710ce10f953db4cd4ef Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Tue, 9 Nov 2021 15:19:05 -0600 Subject: [PATCH 09/10] add delete Signed-off-by: Asra Ali --- cron/worker/main.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cron/worker/main.go b/cron/worker/main.go index 73e45c40e53..623f7eae8f1 100644 --- a/cron/worker/main.go +++ b/cron/worker/main.go @@ -195,6 +195,8 @@ func main() { delete(checksToRun, checks.CheckCITests) // TODO: Re-add Contributors check after fixing: #859. delete(checksToRun, checks.CheckContributors) + // TODO: Add this in v4 + delete(checksToRun, checks.CheckDangerousWorkflow) for { req, err := subscriber.SynchronousPull() if err != nil { From bc926314ff7b07c69444b47292ff44badf3e066d Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Tue, 9 Nov 2021 15:38:48 -0600 Subject: [PATCH 10/10] update docs Signed-off-by: Asra Ali --- docs/checks.md | 23 ++++++++++++----------- docs/checks/internal/checks.yaml | 25 +++++++++++++------------ 2 files changed, 25 insertions(+), 23 deletions(-) diff --git a/docs/checks.md b/docs/checks.md index ee63a043353..8e352e1be5a 100644 --- a/docs/checks.md +++ b/docs/checks.md @@ -241,17 +241,18 @@ participants. Risk: `High` (vulnerable to repository compromise) -This check determines whether the project has dangerous code patterns. - -The first check is 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` 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 token in memory. - -This check does not detect whether untrusted code checkouts are used safely, for -example, only on pull request that have been assigned a label. +This check determines whether the project's GitHub Action workflows has dangerous +code patterns. Some examples of these patterns are untrusted code checkouts, +logging github context and secrets, or use of potentially untrusted inputs in scripts. + +The first code pattern checked 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` +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 +token in memory. This check does not detect whether untrusted code checkouts are +used safely, for example, only on pull request that have been assigned a label. The highest score is awarded when all workflows avoid the dangerous code patterns. diff --git a/docs/checks/internal/checks.yaml b/docs/checks/internal/checks.yaml index bf1c6ee5951..afd506be429 100644 --- a/docs/checks/internal/checks.yaml +++ b/docs/checks/internal/checks.yaml @@ -632,21 +632,22 @@ checks: risk: High tags: supply-chain, security, infrastructure repos: GitHub, local - short: Determines if the project's workflows avoid dangerous patterns. + short: Determines if the project's GitHub Action workflows avoid dangerous patterns. description: | Risk: `High` (vulnerable to repository compromise) - This check determines whether the project has dangerous code patterns. - - The first check is 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` 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 token in memory. - - This check does not detect whether untrusted code checkouts are used safely, for - example, only on pull request that have been assigned a label. + This check determines whether the project's GitHub Action workflows has dangerous + code patterns. Some examples of these patterns are untrusted code checkouts, + logging github context and secrets, or use of potentially untrusted inputs in scripts. + + The first code pattern checked 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` + 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 + token in memory. This check does not detect whether untrusted code checkouts are + used safely, for example, only on pull request that have been assigned a label. The highest score is awarded when all workflows avoid the dangerous code patterns. remediation: