✨ Check for secrets in workflows run on pull requests (#1615)

* updates

* missing files

* typo

* linter

* linter

* updates

* updates

checks/dangerous_workflow.go

@@ -69,18 +69,33 @@ func init() {
 	}
 }
 
+type dangerousResults int
+
+const (
+	scriptInjection dangerousResults = iota
+	untrustedCheckout
+	secretsViaPullRequests
+)
+
+type triggerName string
+
+var (
+	triggerPullRequestTarget = triggerName("pull_request_target")
+	triggerPullRequest       = triggerName("pull_request")
+)
+
 // 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
+	workflowPattern map[dangerousResults]bool
 }
 
 // DangerousWorkflow runs Dangerous-Workflow check.
 func DangerousWorkflow(c *checker.CheckRequest) checker.CheckResult {
 	// data is shared across all GitHub workflows.
 	data := patternCbData{
-		workflowPattern: make(map[string]bool),
+		workflowPattern: make(map[dangerousResults]bool),
 	}
 	err := fileparser.CheckFilesContent(".github/workflows/*", false,
 		c, validateGitHubActionWorkflowPatterns, &data)
@@ -120,31 +135,139 @@ func validateGitHubActionWorkflowPatterns(path string, content []byte, dl checke
 		return false, err
 	}
 
+	// 3. Check for secrets used in workflows triggered by pull requests.
+	if err := validateSecretsInPullRequests(workflow, path, dl, pdata); err != nil {
+		return false, err
+	}
+
 	// TODO: Check other dangerous patterns.
 	return true, nil
 }
 
+func validateSecretsInPullRequests(workflow *actionlint.Workflow, path string,
+	dl checker.DetailLogger, pdata *patternCbData) error {
+	// We need pull request trigger.
+	if !usesEventTrigger(workflow, triggerPullRequest) {
+		return nil
+	}
+
+	// Secrets used in env at the top of the wokflow.
+	if err := checkWorkflowSecretInEnv(workflow, 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 {
+			return err
+		}
+	}
+
+	return nil
+}
+
 func validateUntrustedCodeCheckout(workflow *actionlint.Workflow, path string,
 	dl checker.DetailLogger, pdata *patternCbData) error {
-	if checkPullRequestTrigger(workflow) {
-		for _, job := range workflow.Jobs {
-			if err := checkJobForUntrustedCodeCheckout(job, path, dl, pdata); err != nil {
-				return err
-			}
+	if !usesEventTrigger(workflow, triggerPullRequestTarget) {
+		return nil
+	}
+
+	for _, job := range workflow.Jobs {
+		if err := checkJobForUntrustedCodeCheckout(job, path, dl, pdata); err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
+func usesEventTrigger(workflow *actionlint.Workflow, name triggerName) bool {
+	// Check if the webhook event trigger is a pull_request_target
+	for _, event := range workflow.On {
+		if event.EventName() == string(name) {
+			return true
+		}
+	}
+
+	return false
+}
+
+func jobUsesEnvironment(job *actionlint.Job) bool {
+	if job.Environment == nil {
+		return false
+	}
+
+	return job.Environment.Name != nil &&
+		job.Environment.Name.Value != ""
+}
+
+func checkJobForUsedSecrets(job *actionlint.Job, path string,
+	dl checker.DetailLogger, pdata *patternCbData) error {
+	if job == nil {
+		return nil
+	}
+
+	// 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
+	}
+
+	// https://docs.github.com/en/actions/security-guides/encrypted-secrets#naming-your-secrets
+	for _, step := range job.Steps {
+		if step == nil {
+			continue
+		}
+
+		if err := checkSecretInActionArgs(step, path, dl, pdata); err != nil {
+			return err
+		}
+
+		if err := checkSecretInRun(step, path, dl, pdata); err != nil {
+			return err
+		}
+
+		if err := checkSecretInEnv(step.Env, path, dl, pdata); err != nil {
+			return err
 		}
 	}
 	return nil
 }
 
-func checkPullRequestTrigger(workflow *actionlint.Workflow) bool {
-	// Check if the webhook event trigger is a pull_request_target
-	for _, event := range workflow.On {
-		e, ok := event.(*actionlint.WebhookEvent)
-		if ok && e.Hook != nil && e.Hook.Value == "pull_request_target" {
+func workflowUsesCodeCheckoutAndNoEnvironment(workflow *actionlint.Workflow) bool {
+	if workflow == nil {
+		return false
+	}
+
+	for _, job := range workflow.Jobs {
+		if jobUsesCodeCheckout(job) &&
+			!jobUsesEnvironment(job) {
 			return true
 		}
 	}
+	return false
+}
 
+func jobUsesCodeCheckout(job *actionlint.Job) bool {
+	if job == nil {
+		return false
+	}
+	for _, step := range job.Steps {
+		if step == nil || step.Exec == nil {
+			continue
+		}
+		// Check for a step that uses actions/checkout
+		e, ok := step.Exec.(*actionlint.ExecAction)
+		if !ok || e.Uses == nil {
+			continue
+		}
+		if strings.Contains(e.Uses.Value, "actions/checkout") {
+			return true
+		}
+	}
 	return false
 }
 
@@ -162,7 +285,7 @@ func checkJobForUntrustedCodeCheckout(job *actionlint.Job, path string,
 		// Check for a step that uses actions/checkout
 		e, ok := step.Exec.(*actionlint.ExecAction)
 		if !ok || e.Uses == nil {
-			return nil
+			continue
 		}
 		if !strings.Contains(e.Uses.Value, "actions/checkout") {
 			continue
@@ -183,7 +306,7 @@ func checkJobForUntrustedCodeCheckout(job *actionlint.Job, path string,
 				// TODO: set Snippet.
 			})
 			// Detected untrusted checkout.
-			pdata.workflowPattern["untrusted_checkout"] = true
+			pdata.workflowPattern[untrustedCheckout] = true
 		}
 	}
 	return nil
@@ -209,7 +332,96 @@ func validateScriptInjection(workflow *actionlint.Workflow, path string,
 			}
 		}
 	}
+	return nil
+}
 
+func checkWorkflowSecretInEnv(workflow *actionlint.Workflow, path string,
+	dl checker.DetailLogger, pdata *patternCbData) error {
+	// We need code checkout and not environment rule protection.
+	if !workflowUsesCodeCheckoutAndNoEnvironment(workflow) {
+		return nil
+	}
+
+	return checkSecretInEnv(workflow.Env, path, dl, pdata)
+}
+
+func checkSecretInEnv(env *actionlint.Env, path string,
+	dl checker.DetailLogger, pdata *patternCbData) error {
+	if env == nil {
+		return nil
+	}
+
+	for _, v := range env.Vars {
+		if err := checkSecretInScript(v.Value.Value, v.Value.Pos, path, dl, pdata); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+func checkSecretInRun(step *actionlint.Step, path string,
+	dl checker.DetailLogger, pdata *patternCbData) error {
+	if step == nil || step.Exec == nil {
+		return nil
+	}
+
+	run, ok := step.Exec.(*actionlint.ExecRun)
+	if ok && run.Run != nil {
+		if err := checkSecretInScript(run.Run.Value, run.Run.Pos, path, dl, pdata); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+func checkSecretInActionArgs(step *actionlint.Step, path string,
+	dl checker.DetailLogger, pdata *patternCbData) error {
+	if step == nil || step.Exec == nil {
+		return nil
+	}
+
+	e, ok := step.Exec.(*actionlint.ExecAction)
+	if ok && e.Uses != nil {
+		// Check for reference. If not defined for a pull_request_target event, this defaults to
+		// the base branch of the pull request.
+		for _, v := range e.Inputs {
+			if v.Value != nil {
+				if err := checkSecretInScript(v.Value.Value, v.Value.Pos, path, dl, pdata); err != nil {
+					return err
+				}
+			}
+		}
+	}
+	return nil
+}
+
+func checkSecretInScript(script string, pos *actionlint.Pos, path string,
+	dl checker.DetailLogger, pdata *patternCbData) error {
+	for {
+		s := strings.Index(script, "${{")
+		if s == -1 {
+			break
+		}
+
+		e := strings.Index(script[s:], "}}")
+		if e == -1 {
+			return sce.WithMessage(sce.ErrScorecardInternal, errInvalidGitHubWorkflow.Error())
+		}
+
+		variable := strings.Trim(script[s:s+e+2], " ")
+		if strings.Contains(variable, "secrets.") {
+			line := fileparser.GetLineNumber(pos)
+			dl.Warn3(&checker.LogMessage{
+				Path:   path,
+				Type:   checker.FileTypeSource,
+				Offset: line,
+				Text:   fmt.Sprintf("secret accessible to pull requests '%v'", variable),
+				// TODO: set Snippet.
+			})
+			pdata.workflowPattern[secretsViaPullRequests] = true
+		}
+		script = script[s+e:]
+	}
 	return nil
 }
 
@@ -218,7 +430,7 @@ func checkVariablesInScript(script string, pos *actionlint.Pos, path string,
 	for {
 		s := strings.Index(script, "${{")
 		if s == -1 {
-			return nil
+			break
 		}
 
 		e := strings.Index(script[s:], "}}")
@@ -237,10 +449,11 @@ func checkVariablesInScript(script string, pos *actionlint.Pos, path string,
 				Text:   fmt.Sprintf("script injection with untrusted input '%v'", variable),
 				// TODO: set Snippet.
 			})
-			pdata.workflowPattern["script_injection"] = true
+			pdata.workflowPattern[scriptInjection] = true
 		}
 		script = script[s+e:]
 	}
+	return nil
 }
 
 // Calculate the workflow score.
@@ -248,13 +461,18 @@ 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 {
+	// Pull_request_event indicates untrusted code checkout.
+	if ok := result.workflowPattern[untrustedCheckout]; ok {
 		score -= 10
 	}
 
-	// script injection with an untrusted context
-	if ok := result.workflowPattern["script_injection"]; ok {
+	// Script injection with an untrusted context.
+	if ok := result.workflowPattern[scriptInjection]; ok {
+		score -= 10
+	}
+
+	// Secrets available by pull requests.
+	if ok := result.workflowPattern[secretsViaPullRequests]; ok {
 		score -= 10
 	}
 
@@ -286,7 +504,7 @@ func createResultForDangerousWorkflowPatterns(result patternCbData, err error) c
 func testValidateGitHubActionDangerousWorkflow(pathfn string,
 	content []byte, dl checker.DetailLogger) checker.CheckResult {
 	data := patternCbData{
-		workflowPattern: make(map[string]bool),
+		workflowPattern: make(map[dangerousResults]bool),
 	}
 	_, err := validateGitHubActionWorkflowPatterns(pathfn, content, dl, &data)
 	return createResultForDangerousWorkflowPatterns(data, err)

checks/dangerous_workflow_test.go

@@ -76,17 +76,6 @@ func TestGithubDangerousWorkflow(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 		},
-		{
-			name:     "run safe trigger with code checkout test",
-			filename: "./testdata/.github/workflows/github-workflow-dangerous-pattern-safe-trigger.yml",
-			expected: scut.TestReturn{
-				Error:         nil,
-				Score:         checker.MaxResultScore,
-				NumberOfWarn:  0,
-				NumberOfInfo:  0,
-				NumberOfDebug: 0,
-			},
-		},
 		{
 			name:     "run script injection",
 			filename: "./testdata/.github/workflows/github-workflow-dangerous-pattern-untrusted-script-injection.yml",
@@ -142,6 +131,83 @@ func TestGithubDangerousWorkflow(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 		},
+		{
+			name:     "secret in top env no checkout",
+			filename: "./testdata/.github/workflows/github-workflow-dangerous-pattern-secret-env-no-checkout.yml",
+			expected: scut.TestReturn{
+				Error:         nil,
+				Score:         checker.MaxResultConfidence,
+				NumberOfWarn:  0,
+				NumberOfInfo:  0,
+				NumberOfDebug: 0,
+			},
+		},
+		{
+			name:     "secret in action args",
+			filename: "./testdata/.github/workflows/github-workflow-dangerous-pattern-secret-action-args.yml",
+			expected: scut.TestReturn{
+				Error:         nil,
+				Score:         checker.MinResultConfidence,
+				NumberOfWarn:  1,
+				NumberOfInfo:  0,
+				NumberOfDebug: 0,
+			},
+		},
+		{
+			name:     "secret in all places",
+			filename: "./testdata/.github/workflows/github-workflow-dangerous-pattern-secret-all-checkout.yml",
+			expected: scut.TestReturn{
+				Error:         nil,
+				Score:         checker.MinResultConfidence,
+				NumberOfWarn:  7,
+				NumberOfInfo:  0,
+				NumberOfDebug: 0,
+			},
+		},
+		{
+			name:     "secret in env",
+			filename: "./testdata/.github/workflows/github-workflow-dangerous-pattern-secret-env.yml",
+			expected: scut.TestReturn{
+				Error:         nil,
+				Score:         checker.MinResultConfidence,
+				NumberOfWarn:  2,
+				NumberOfInfo:  0,
+				NumberOfDebug: 0,
+			},
+		},
+		{
+			name:     "secret in env",
+			filename: "./testdata/.github/workflows/github-workflow-dangerous-pattern-secret-no-pull-request.yml",
+			expected: scut.TestReturn{
+				Error:         nil,
+				Score:         checker.MaxResultConfidence,
+				NumberOfWarn:  0,
+				NumberOfInfo:  0,
+				NumberOfDebug: 0,
+			},
+		},
+		{
+			name:     "secret in env",
+			filename: "./testdata/.github/workflows/github-workflow-dangerous-pattern-secret-run.yml",
+			expected: scut.TestReturn{
+				Error:         nil,
+				Score:         checker.MinResultConfidence,
+				NumberOfWarn:  1,
+				NumberOfInfo:  0,
+				NumberOfDebug: 0,
+			},
+		},
+		{
+			name:     "secret with environment protection",
+			filename: "./testdata/.github/workflows/github-workflow-dangerous-pattern-secret-env-environment.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

checks/testdata/.github/workflows/github-workflow-dangerous-pattern-secret-action-args.yml

@@ -0,0 +1,17 @@
+name: Close issue on Jira
+
+on:
+  pull_request
+
+jobs:
+  test1:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v1.2.3
+        name: Use in env toJson
+        
+      - name: Use in with toJson
+        uses: some/action@main
+        with:
+          some-args: ${{ toJson(secrets.SE12) }}
+       

checks/testdata/.github/workflows/github-workflow-dangerous-pattern-secret-all-checkout.yml

@@ -0,0 +1,60 @@
+name: Close issue on Jira
+
+on:
+  pull_request
+
+env:
+  BLA: ${{ secrets.SE00 }}
+
+jobs:
+  test1:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v1.2.3
+        name: Use in env toJson
+        env:
+          GITHUB_CONTEXT: ${{ toJson(secrets.SE11) }}
+        #run: echo "$GITHUB_CONTEXT"
+
+      - name: Use in with toJson
+        uses: some/action@main
+        with:
+          some-args: ${{ toJson(secrets.SE12) }}
+        #run: echo "$GITHUB_CONTEXT"
+
+      - 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/action@v1.2.3
+        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/action@v1.2.3
+        with:
+          some-args: ${{ secrets.SE32 }}
+        run: echo "$GITHUB_CONTEXT"
+
+      - name: Use in run toJson
+        run: echo "${{ secrets.SE33 }}"
+      
+      - uses: actions/checkout@v1.2.3

checks/testdata/.github/workflows/github-workflow-dangerous-pattern-secret-env-environment.yml

@@ -0,0 +1,61 @@
+name: Close issue on Jira
+
+on:
+  pull_request
+
+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/checkout@v1.2.3
+  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/action@v1.2.3
+        with:
+          some-args: ${{ secrets.SE22 }}
+        run: echo "$GITHUB_CONTEXT"
+
+      - name: Use in run toJson
+        run: echo "${{ secrets.SE23 }}"
+
+      - uses: actions/checkout@v1.2.3
+  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/action@v1.2.3
+        with:
+          some-args: ${{ secrets.SE32 }}
+        run: echo "$GITHUB_CONTEXT"
+
+      - name: Use in run toJson
+        run: echo "${{ secrets.SE33 }}"
+
+      - uses: actions/checkout@v1.2.3
+      

checks/testdata/.github/workflows/github-workflow-dangerous-pattern-secret-env-no-checkout.yml

@@ -0,0 +1,52 @@
+name: Close issue on Jira
+
+on:
+  pull_request
+
+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/action@v1.2.3
+        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/action@v1.2.3
+        with:
+          some-args: ${{ secrets.SE32 }}
+        run: echo "$GITHUB_CONTEXT"
+
+      - name: Use in run toJson
+        run: echo "${{ secrets.SE33 }}"
+      

checks/testdata/.github/workflows/github-workflow-dangerous-pattern-secret-env.yml

@@ -0,0 +1,22 @@
+name: Close issue on Jira
+
+on:
+  pull_request
+
+jobs:
+  test1:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v1.2.3
+        name: Use in env toJson
+       
+      - name: Use in with toJson
+        env:
+          GITHUB_CONTEXT: ${{ secrets.SE21 }}
+        run: echo "$GITHUB_CONTEXT"
+
+      - name: Use in with toJson
+        uses: some/action@v1.2.3
+        env:
+          GITHUB_CONTEXT: ${{ secrets.SE22 }}
+        run: echo "$GITHUB_CONTEXT"

checks/testdata/.github/workflows/github-workflow-dangerous-pattern-secret-no-pull-request.yml

@@ -0,0 +1,60 @@
+name: Close issue on Jira
+
+on:
+  push
+
+env:
+  BLA: ${{ secrets.SE00 }}
+
+jobs:
+  test1:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v1.2.3
+        name: Use in env toJson
+        env:
+          GITHUB_CONTEXT: ${{ toJson(secrets.SE11) }}
+        #run: echo "$GITHUB_CONTEXT"
+
+      - name: Use in with toJson
+        uses: some/action@main
+        with:
+          some-args: ${{ toJson(secrets.SE12) }}
+        #run: echo "$GITHUB_CONTEXT"
+
+      - 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/action@v1.2.3
+        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/action@v1.2.3
+        with:
+          some-args: ${{ secrets.SE32 }}
+        run: echo "$GITHUB_CONTEXT"
+
+      - name: Use in run toJson
+        run: echo "${{ secrets.SE33 }}"
+      
+      - uses: actions/checkout@v1.2.3

checks/testdata/.github/workflows/github-workflow-dangerous-pattern-secret-run.yml

@@ -0,0 +1,12 @@
+name: Close issue on Jira
+
+on:
+  pull_request
+
+jobs:
+  test1:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v1.2.3
+      - name: Use in run toJson
+        run: echo "${{ toJson(secrets.SE13) }}"