✨ Add Script Injection to Dangerous-Workflow (#1368)

* add dangerous workflow pattern script injection

Signed-off-by: Asra Ali <asraa@google.com>

* add more tests

Signed-off-by: Asra Ali <asraa@google.com>

* update laurent comments

Signed-off-by: Asra Ali <asraa@google.com>

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

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()
+			}
+		})
+	}
+}

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 }}"

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

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 }}"

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

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

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 
 

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