diff --git a/checks/dangerous_workflow.go b/checks/dangerous_workflow.go index c0f0a5a8..970f8923 100644 --- a/checks/dangerous_workflow.go +++ b/checks/dangerous_workflow.go @@ -16,17 +16,47 @@ package checks import ( "fmt" + "regexp" "strings" "github.com/rhysd/actionlint" "github.com/ossf/scorecard/v3/checker" "github.com/ossf/scorecard/v3/checks/fileparser" + sce "github.com/ossf/scorecard/v3/errors" ) // CheckDangerousWorkflow is the exported name for Dangerous-Workflow check. const CheckDangerousWorkflow = "Dangerous-Workflow" +func containsUntrustedContextPattern(variable string) bool { + // GitHub event context details that may be attacker controlled. + // See https://securitylab.github.com/research/github-actions-untrusted-input/ + untrustedContextPattern := regexp.MustCompile( + `.*(issue\.title|` + + `issue\.body|` + + `pull_request\.title|` + + `pull_request\.body|` + + `comment\.body|` + + `review\.body|` + + `review_comment\.body|` + + `pages.*\.page_name|` + + `commits.*\.message|` + + `head_commit\.message|` + + `head_commit\.author\.email|` + + `head_commit\.author\.name|` + + `commits.*\.author\.email|` + + `commits.*\.author\.name|` + + `pull_request\.head\.ref|` + + `pull_request\.head\.label|` + + `pull_request\.head\.repo\.default_branch).*`) + + if strings.Contains(variable, "github.head_ref") { + return true + } + return strings.Contains(variable, "github.event.") && untrustedContextPattern.MatchString(variable) +} + //nolint:gochecknoinits func init() { registerCheck(CheckDangerousWorkflow, DangerousWorkflow) @@ -78,6 +108,11 @@ func validateGitHubActionWorkflowPatterns(path string, content []byte, dl checke return false, err } + // 2. Check for script injection in workflow inline scripts. + if err := validateScriptInjection(workflow, path, dl, pdata); err != nil { + return false, err + } + // TODO: Check other dangerous patterns. return true, nil } @@ -132,10 +167,14 @@ func checkJobForUntrustedCodeCheckout(job *actionlint.Job, path string, continue } if strings.Contains(ref.Value.Value, "github.event.pull_request") { + line := 1 + if step.Pos != nil { + line = step.Pos.Line + } dl.Warn3(&checker.LogMessage{ Path: path, Type: checker.FileTypeSource, - Offset: step.Pos.Line, + Offset: line, Text: fmt.Sprintf("untrusted code checkout '%v'", ref.Value.Value), // TODO: set Snippet. }) @@ -146,6 +185,63 @@ func checkJobForUntrustedCodeCheckout(job *actionlint.Job, path string, return nil } +func validateScriptInjection(workflow *actionlint.Workflow, path string, + dl checker.DetailLogger, pdata *patternCbData) error { + for _, job := range workflow.Jobs { + if job == nil { + continue + } + for _, step := range job.Steps { + if step == nil { + continue + } + run, ok := step.Exec.(*actionlint.ExecRun) + if !ok || run.Run == nil { + continue + } + // Check Run *String for user-controllable (untrustworthy) properties. + if err := checkVariablesInScript(run.Run.Value, run.Run.Pos, path, dl, pdata); err != nil { + return err + } + } + } + + return nil +} + +func checkVariablesInScript(script string, pos *actionlint.Pos, path string, + dl checker.DetailLogger, pdata *patternCbData) error { + for { + s := strings.Index(script, "${{") + if s == -1 { + return nil + } + + e := strings.Index(script[s:], "}}") + if e == -1 { + return sce.WithMessage(sce.ErrScorecardInternal, errInvalidGitHubWorkflow.Error()) + } + + // Check if the variable may be untrustworthy. + variable := script[s+3 : s+e] + if containsUntrustedContextPattern(variable) { + line := 1 + if pos != nil { + line = pos.Line + } + dl.Warn3(&checker.LogMessage{ + Path: path, + Type: checker.FileTypeSource, + Offset: line, + Text: fmt.Sprintf("script injection with untrusted input '%v'", variable), + // TODO: set Snippet. + }) + pdata.workflowPattern["script_injection"] = true + } + script = script[s+e:] + } +} + // Calculate the workflow score. func calculateWorkflowScore(result patternCbData) int { // Start with a perfect score. @@ -156,6 +252,11 @@ func calculateWorkflowScore(result patternCbData) int { score -= 10 } + // script injection with an untrusted context + if ok := result.workflowPattern["script_injection"]; ok { + score -= 10 + } + // We're done, calculate the final score. if score < checker.MinResultScore { return checker.MinResultScore diff --git a/checks/dangerous_workflow_test.go b/checks/dangerous_workflow_test.go index be3ad2ca..e7aa6f1c 100644 --- a/checks/dangerous_workflow_test.go +++ b/checks/dangerous_workflow_test.go @@ -86,6 +86,61 @@ func TestGithubDangerousWorkflow(t *testing.T) { NumberOfDebug: 0, }, }, + { + name: "run script injection", + filename: "./testdata/github-workflow-dangerous-pattern-untrusted-script-injection.yml", + expected: scut.TestReturn{ + Error: nil, + Score: checker.MinResultScore, + NumberOfWarn: 1, + NumberOfInfo: 0, + NumberOfDebug: 0, + }, + }, + { + name: "run safe script injection", + filename: "./testdata/github-workflow-dangerous-pattern-trusted-script-injection.yml", + expected: scut.TestReturn{ + Error: nil, + Score: checker.MaxResultScore, + NumberOfWarn: 0, + NumberOfInfo: 0, + NumberOfDebug: 0, + }, + }, + { + name: "run multiple script injection", + filename: "./testdata/github-workflow-dangerous-pattern-untrusted-multiple-script-injection.yml", + expected: scut.TestReturn{ + Error: nil, + Score: checker.MinResultConfidence, + NumberOfWarn: 2, + NumberOfInfo: 0, + NumberOfDebug: 0, + }, + }, + { + name: "run inline script injection", + filename: "./testdata/github-workflow-dangerous-pattern-untrusted-inline-script-injection.yml", + expected: scut.TestReturn{ + Error: nil, + Score: checker.MinResultConfidence, + NumberOfWarn: 1, + NumberOfInfo: 0, + NumberOfDebug: 0, + }, + }, + { + name: "run wildcard script injection", + filename: "./testdata/github-workflow-dangerous-pattern-untrusted-script-injection-wildcard.yml", + expected: scut.TestReturn{ + Error: nil, + Score: checker.MinResultConfidence, + NumberOfWarn: 1, + NumberOfInfo: 0, + NumberOfDebug: 0, + }, + }, } for _, tt := range tests { tt := tt // Re-initializing variable so it is not changed while executing the closure below @@ -109,3 +164,58 @@ func TestGithubDangerousWorkflow(t *testing.T) { }) } } + +func TestUntrustedContextVariables(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + variable string + expected bool + }{ + { + name: "trusted", + variable: "github.action", + expected: false, + }, + { + name: "untrusted", + variable: "github.head_ref", + expected: true, + }, + { + name: "untrusted event", + variable: "github.event.issue.title", + expected: true, + }, + { + name: "untrusted pull request", + variable: "github.event.pull_request.body", + expected: true, + }, + { + name: "trusted pull request", + variable: "github.event.pull_request.number", + expected: false, + }, + { + name: "untrusted wildcard", + variable: "github.event.commits[0].message", + expected: true, + }, + { + name: "trusted wildcard", + variable: "github.event.commits[0].id", + expected: false, + }, + } + 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() + if r := containsUntrustedContextPattern(tt.variable); !r == tt.expected { + t.Fail() + } + }) + } +} diff --git a/checks/testdata/github-workflow-dangerous-pattern-trusted-script-injection.yml b/checks/testdata/github-workflow-dangerous-pattern-trusted-script-injection.yml new file mode 100644 index 00000000..1c224d06 --- /dev/null +++ b/checks/testdata/github-workflow-dangerous-pattern-trusted-script-injection.yml @@ -0,0 +1,24 @@ +# 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: issue_comment + +jobs: + issue_commented: + # This job only runs for issue comments + name: Issue comment + if: ${{ !github.event.issue.pull_request }} + runs-on: ubuntu-latest + steps: + - run: | + echo "Comment on issue #${{ github.event.issue.number }}" \ No newline at end of file diff --git a/checks/testdata/github-workflow-dangerous-pattern-untrusted-inline-script-injection.yml b/checks/testdata/github-workflow-dangerous-pattern-untrusted-inline-script-injection.yml new file mode 100644 index 00000000..414f2af1 --- /dev/null +++ b/checks/testdata/github-workflow-dangerous-pattern-untrusted-inline-script-injection.yml @@ -0,0 +1,31 @@ +# 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: ${{ github.event.pull_request.head.sha }} + + - name: Check title + run: | + echo "Some first line" + if [[ ! ${{ github.event.issue.title }} =~ ^.*:\ .*$ ]]; then + echo "Bad issue title" + exit 1 + fi diff --git a/checks/testdata/github-workflow-dangerous-pattern-untrusted-multiple-script-injection.yml b/checks/testdata/github-workflow-dangerous-pattern-untrusted-multiple-script-injection.yml new file mode 100644 index 00000000..e61693db --- /dev/null +++ b/checks/testdata/github-workflow-dangerous-pattern-untrusted-multiple-script-injection.yml @@ -0,0 +1,32 @@ +# 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: ${{ github.event.pull_request.head.sha }} + + - name: Check title + run: | + title="${{ github.event.issue.title }}" + if [[ ! $title =~ ^.*:\ .*$ ]]; then + echo "Bad issue title" + exit 1 + fi + another_test="${{ github.head_ref }}" diff --git a/checks/testdata/github-workflow-dangerous-pattern-untrusted-script-injection-wildcard.yml b/checks/testdata/github-workflow-dangerous-pattern-untrusted-script-injection-wildcard.yml new file mode 100644 index 00000000..5931807e --- /dev/null +++ b/checks/testdata/github-workflow-dangerous-pattern-untrusted-script-injection-wildcard.yml @@ -0,0 +1,31 @@ +# 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: ${{ github.event.pull_request.head.sha }} + + - name: Check msg + run: | + msg="${{ github.event.commits[0].message }}" + if [[ ! $msg =~ ^.*:\ .*$ ]]; then + echo "Bad message " + exit 1 + fi diff --git a/checks/testdata/github-workflow-dangerous-pattern-untrusted-script-injection.yml b/checks/testdata/github-workflow-dangerous-pattern-untrusted-script-injection.yml new file mode 100644 index 00000000..a5e99e6a --- /dev/null +++ b/checks/testdata/github-workflow-dangerous-pattern-untrusted-script-injection.yml @@ -0,0 +1,31 @@ +# 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: ${{ github.event.pull_request.head.sha }} + + - name: Check title + run: | + title="${{ github.event.issue.title }}" + if [[ ! $title =~ ^.*:\ .*$ ]]; then + echo "Bad issue title" + exit 1 + fi diff --git a/docs/checks.md b/docs/checks.md index f4302775..1462c9dd 100644 --- a/docs/checks.md +++ b/docs/checks.md @@ -266,8 +266,9 @@ Risk: `Critical` (vulnerable to repository compromise) 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 following patterns are checked: -The first code pattern checked is the misuse of potentially dangerous triggers. +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` have write permission to the target repository and access to target repository @@ -276,11 +277,19 @@ 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. +Script Injection with Untrusted Context Variables: This pattern detects whether a +workflow's inline script may execute untrusted input from attackers. This occurs when +an attacker adds malicious commands and scripts to a context. When a workflow runs, +these strings may be interpreted as code that is executed on the runner. Attackers +can add their own content to certain github context variables that are considered +untrusted, for example, `github.event.issue.title`. These values should not flow +directly into executable code. + The highest score is awarded when all workflows avoid the dangerous code patterns. **Remediation steps** -- 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. +- 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. See this [document](https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#understanding-the-risk-of-script-injections) for information on avoiding and mitigating the risk of script injections. ## Dependency-Update-Tool diff --git a/docs/checks/internal/checks.yaml b/docs/checks/internal/checks.yaml index 0cb3585d..d60c2de9 100644 --- a/docs/checks/internal/checks.yaml +++ b/docs/checks/internal/checks.yaml @@ -670,8 +670,9 @@ checks: 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 following patterns are checked: - The first code pattern checked is the misuse of potentially dangerous triggers. + 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` have write permission to the target repository and access to target repository @@ -680,11 +681,22 @@ checks: 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. + Script Injection with Untrusted Context Variables: This pattern detects whether a + workflow's inline script may execute untrusted input from attackers. This occurs when + an attacker adds malicious commands and scripts to a context. When a workflow runs, + these strings may be interpreted as code that is executed on the runner. Attackers + can add their own content to certain github context variables that are considered + untrusted, for example, `github.event.issue.title`. These values should not flow + directly into executable code. + The highest score is awarded when all workflows avoid the dangerous code patterns. remediation: - >- - Avoid the dangerous workflow patterns. See this [post](https://securitylab.github.com/research/github-actions-preventing-pwn-requests/) + 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. + See this [document](https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#understanding-the-risk-of-script-injections) + for information on avoiding and mitigating the risk of script injections. License: risk: Low