✨ Check for secrets in pull_request_target (#1634)

* checks/dangerous_workflow.go: add pull_request_target support for secrets * missing files * linter
ossf · Feb 15, 2022 · e7fd58d · e7fd58d
1 parent e3637c9
commit e7fd58d
Show file tree

Hide file tree

Showing 8 changed files with 316 additions and 22 deletions.
diff --git a/checks/dangerous_workflow.go b/checks/dangerous_workflow.go
@@ -82,6 +82,7 @@ type triggerName string
 var (
 	triggerPullRequestTarget = triggerName("pull_request_target")
 	triggerPullRequest       = triggerName("pull_request")
+	checkoutUntrustedRef     = "github.event.pull_request"
 )
 
 // Holds stateful data to pass thru callbacks.
@@ -146,22 +147,31 @@ func validateGitHubActionWorkflowPatterns(path string, content []byte, dl checke
 
 func validateSecretsInPullRequests(workflow *actionlint.Workflow, path string,
 	dl checker.DetailLogger, pdata *patternCbData) error {
+	triggers := make(map[triggerName]bool)
+
 	// We need pull request trigger.
-	if !usesEventTrigger(workflow, triggerPullRequest) {
+	usesPullRequest := usesEventTrigger(workflow, triggerPullRequest)
+	usesPullRequestTarget := usesEventTrigger(workflow, triggerPullRequestTarget)
+	if !usesPullRequest && !usesPullRequestTarget {
 		return nil
 	}
 
+	// Record the triggers.
+	if usesPullRequest {
+		triggers[triggerPullRequest] = usesPullRequest
+	}
+	if usesPullRequestTarget {
+		triggers[triggerPullRequestTarget] = usesPullRequestTarget
+	}
+
 	// Secrets used in env at the top of the wokflow.
-	if err := checkWorkflowSecretInEnv(workflow, path, dl, pdata); err != nil {
+	if err := checkWorkflowSecretInEnv(workflow, triggers, path, dl, pdata); err != nil {
 		return err
 	}
 
 	// Secrets used on jobs.
 	for _, job := range workflow.Jobs {
-		if !jobUsesCodeCheckout(job) {
-			continue
-		}
-		if err := checkJobForUsedSecrets(job, path, dl, pdata); err != nil {
+		if err := checkJobForUsedSecrets(job, triggers, path, dl, pdata); err != nil {
 			return err
 		}
 	}
@@ -204,8 +214,8 @@ func jobUsesEnvironment(job *actionlint.Job) bool {
 		job.Environment.Name.Value != ""
 }
 
-func checkJobForUsedSecrets(job *actionlint.Job, path string,
-	dl checker.DetailLogger, pdata *patternCbData) error {
+func checkJobForUsedSecrets(job *actionlint.Job, triggers map[triggerName]bool,
+	path string, dl checker.DetailLogger, pdata *patternCbData) error {
 	if job == nil {
 		return nil
 	}
@@ -216,6 +226,15 @@ func checkJobForUsedSecrets(job *actionlint.Job, path string,
 		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))) {
+		return nil
+	}
+
 	// https://docs.github.com/en/actions/security-guides/encrypted-secrets#naming-your-secrets
 	for _, step := range job.Steps {
 		if step == nil {
@@ -237,24 +256,32 @@ func checkJobForUsedSecrets(job *actionlint.Job, path string,
 	return nil
 }
 
-func workflowUsesCodeCheckoutAndNoEnvironment(workflow *actionlint.Workflow) bool {
+func workflowUsesCodeCheckoutAndNoEnvironment(workflow *actionlint.Workflow,
+	triggers map[triggerName]bool) bool {
 	if workflow == nil {
 		return false
 	}
 
+	_, usesPullRequest := triggers[triggerPullRequest]
+	_, usesPullRequestTarget := triggers[triggerPullRequestTarget]
+
 	for _, job := range workflow.Jobs {
-		if jobUsesCodeCheckout(job) &&
+		chk, ref := jobUsesCodeCheckout(job)
+		if ((chk && usesPullRequest) ||
+			(chk && usesPullRequestTarget && strings.Contains(ref, checkoutUntrustedRef))) &&
 			!jobUsesEnvironment(job) {
 			return true
 		}
 	}
 	return false
 }
 
-func jobUsesCodeCheckout(job *actionlint.Job) bool {
+func jobUsesCodeCheckout(job *actionlint.Job) (bool, string) {
 	if job == nil {
-		return false
+		return false, ""
 	}
+
+	hasCheckout := false
 	for _, step := range job.Steps {
 		if step == nil || step.Exec == nil {
 			continue
@@ -265,10 +292,15 @@ func jobUsesCodeCheckout(job *actionlint.Job) bool {
 			continue
 		}
 		if strings.Contains(e.Uses.Value, "actions/checkout") {
-			return true
+			hasCheckout = true
+			ref, ok := e.Inputs["ref"]
+			if !ok || ref.Value == nil {
+				continue
+			}
+			return true, ref.Value.Value
 		}
 	}
-	return false
+	return hasCheckout, ""
 }
 
 func checkJobForUntrustedCodeCheckout(job *actionlint.Job, path string,
@@ -296,7 +328,7 @@ func checkJobForUntrustedCodeCheckout(job *actionlint.Job, path string,
 		if !ok || ref.Value == nil {
 			continue
 		}
-		if strings.Contains(ref.Value.Value, "github.event.pull_request") {
+		if strings.Contains(ref.Value.Value, checkoutUntrustedRef) {
 			line := fileparser.GetLineNumber(step.Pos)
 			dl.Warn(&checker.LogMessage{
 				Path:    path,
@@ -336,10 +368,10 @@ func validateScriptInjection(workflow *actionlint.Workflow, path string,
 	return nil
 }
 
-func checkWorkflowSecretInEnv(workflow *actionlint.Workflow, path string,
-	dl checker.DetailLogger, pdata *patternCbData) error {
+func checkWorkflowSecretInEnv(workflow *actionlint.Workflow, triggers map[triggerName]bool,
+	path string, dl checker.DetailLogger, pdata *patternCbData) error {
 	// We need code checkout and not environment rule protection.
-	if !workflowUsesCodeCheckoutAndNoEnvironment(workflow) {
+	if !workflowUsesCodeCheckoutAndNoEnvironment(workflow, triggers) {
 		return nil
 	}
 

diff --git a/checks/dangerous_workflow_test.go b/checks/dangerous_workflow_test.go
@@ -49,7 +49,7 @@ func TestGithubDangerousWorkflow(t *testing.T) {
 			expected: scut.TestReturn{
 				Error:         nil,
 				Score:         checker.MinResultScore,
-				NumberOfWarn:  1,
+				NumberOfWarn:  2,
 				NumberOfInfo:  0,
 				NumberOfDebug: 0,
 			},
@@ -208,6 +208,61 @@ func TestGithubDangerousWorkflow(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 		},
+		{
+			name:     "secret with environment protection pull request target",
+			filename: "./testdata/.github/workflows/github-workflow-dangerous-pattern-secret-env-environment-prt.yml",
+			expected: scut.TestReturn{
+				Error:         nil,
+				Score:         checker.MinResultConfidence,
+				NumberOfWarn:  1,
+				NumberOfInfo:  0,
+				NumberOfDebug: 0,
+			},
+		},
+		{
+			name:     "secret in env pull request target",
+			filename: "./testdata/.github/workflows/github-workflow-dangerous-pattern-secret-run-prt.yml",
+			expected: scut.TestReturn{
+				Error:         nil,
+				Score:         checker.MinResultConfidence,
+				NumberOfWarn:  2,
+				NumberOfInfo:  0,
+				NumberOfDebug: 0,
+			},
+		},
+		{
+			name:     "secret in env pull request target",
+			filename: "./testdata/.github/workflows/github-workflow-dangerous-pattern-secret-env-prt.yml",
+			expected: scut.TestReturn{
+				Error:         nil,
+				Score:         checker.MinResultConfidence,
+				NumberOfWarn:  4,
+				NumberOfInfo:  0,
+				NumberOfDebug: 0,
+			},
+		},
+		{
+			name:     "secret in top env no checkout pull request target",
+			filename: "./testdata/.github/workflows/github-workflow-dangerous-pattern-secret-env-no-checkout-prt.yml",
+			expected: scut.TestReturn{
+				Error:         nil,
+				Score:         checker.MaxResultConfidence,
+				NumberOfWarn:  0,
+				NumberOfInfo:  0,
+				NumberOfDebug: 0,
+			},
+		},
+		{
+			name:     "secret in top env checkout no ref pull request target",
+			filename: "./testdata/.github/workflows/github-workflow-dangerous-pattern-secret-env-checkout-noref-prt.yml",
+			expected: scut.TestReturn{
+				Error:         nil,
+				Score:         checker.MaxResultConfidence,
+				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
@@ -225,7 +280,6 @@ func TestGithubDangerousWorkflow(t *testing.T) {
 			}
 			dl := scut.TestDetailLogger{}
 			p := strings.Replace(tt.filename, "./testdata/", "", 1)
-
 			r := testValidateGitHubActionDangerousWorkflow(p, content, &dl)
 			if !scut.ValidateTestReturn(t, tt.name, &tt.expected, &r, &dl) {
 				t.Fail()

diff --git a/...ata/.github/workflows/github-workflow-dangerous-pattern-secret-env-checkout-noref-prt.yml b/...ata/.github/workflows/github-workflow-dangerous-pattern-secret-env-checkout-noref-prt.yml
@@ -0,0 +1,52 @@
+name: Close issue on Jira
+
+on:
+  pull_request_target
+
+env:
+  BLA: ${{ secrets.SE00 }}
+
+jobs:
+  test1:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Use in with toJson
+        uses: some/action@main
+        with:
+          some-args: ${{ toJson(secrets.SE12) }}
+
+      - name: Use in run toJson
+        run: echo "${{ toJson(secrets.SE13) }}"
+  test2:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Use in env toJson
+        env:
+          GITHUB_CONTEXT: ${{ secrets.SE21 }}
+        run: echo "$GITHUB_CONTEXT"
+
+      - name: Use in with toJson
+        uses: some/[email protected]
+        with:
+          some-args: ${{ secrets.SE22 }}
+        run: echo "$GITHUB_CONTEXT"
+
+      - name: Use in run toJson
+        run: echo "${{ secrets.SE23 }}"
+  test3:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Use in env toJson
+        env:
+          GITHUB_CONTEXT: ${{ secrets.SE31 }}
+        run: echo "$GITHUB_CONTEXT"
+
+      - name: Use in with toJson
+        uses: some/[email protected]
+        with:
+          some-args: ${{ secrets.SE32 }}
+        run: echo "$GITHUB_CONTEXT"
+
+      - name: Use in run toJson
+        run: echo "${{ secrets.SE33 }}"
+
diff --git a/...stdata/.github/workflows/github-workflow-dangerous-pattern-secret-env-environment-prt.yml b/...stdata/.github/workflows/github-workflow-dangerous-pattern-secret-env-environment-prt.yml
@@ -0,0 +1,63 @@
+name: Close issue on Jira
+
+on:
+  pull_request_target
+
+env:
+  BLA: ${{ secrets.SE00 }}
+
+jobs:
+  test1:
+    runs-on: ubuntu-latest
+    environment: protected
+    steps:
+      - name: Use in with toJson
+        uses: some/action@main
+        with:
+          some-args: ${{ toJson(secrets.SE12) }}
+
+      - name: Use in run toJson
+        run: echo "${{ toJson(secrets.SE13) }}"
+
+      - uses: actions/[email protected]
+        with:
+          ref: ${{ github.event.pull_request.head.sha }}
+  test2:
+    runs-on: ubuntu-latest
+    environment: protected
+    steps:
+      - name: Use in env toJson
+        env:
+          GITHUB_CONTEXT: ${{ secrets.SE21 }}
+        run: echo "$GITHUB_CONTEXT"
+
+      - name: Use in with toJson
+        uses: some/[email protected]
+        with:
+          some-args: ${{ secrets.SE22 }}
+        run: echo "$GITHUB_CONTEXT"
+
+      - name: Use in run toJson
+        run: echo "${{ secrets.SE23 }}"
+
+      - uses: actions/[email protected]
+  test3:
+    runs-on: ubuntu-latest
+    environment: protected
+    steps:
+      - name: Use in env toJson
+        env:
+          GITHUB_CONTEXT: ${{ secrets.SE31 }}
+        run: echo "$GITHUB_CONTEXT"
+
+      - name: Use in with toJson
+        uses: some/[email protected]
+        with:
+          some-args: ${{ secrets.SE32 }}
+        run: echo "$GITHUB_CONTEXT"
+
+      - name: Use in run toJson
+        run: echo "${{ secrets.SE33 }}"
+
+      - uses: actions/[email protected]
+