From 7d359c6cd0ee4c380b7423e09b10b12b9ccc0cfa Mon Sep 17 00:00:00 2001 From: laurentsimon Date: Tue, 22 Mar 2022 23:15:51 +0000 Subject: [PATCH 1/2] Discard GitHub token in dangerous workflow check --- ...ow-dangerous-pattern-default-secret-pr.yml | 36 +++++++++++++++++++ ...w-dangerous-pattern-default-secret-prt.yml | 36 +++++++++++++++++++ 2 files changed, 72 insertions(+) create mode 100644 checks/testdata/.github/workflows/github-workflow-dangerous-pattern-default-secret-pr.yml create mode 100644 checks/testdata/.github/workflows/github-workflow-dangerous-pattern-default-secret-prt.yml diff --git a/checks/testdata/.github/workflows/github-workflow-dangerous-pattern-default-secret-pr.yml b/checks/testdata/.github/workflows/github-workflow-dangerous-pattern-default-secret-pr.yml new file mode 100644 index 00000000000..9e927e8a00c --- /dev/null +++ b/checks/testdata/.github/workflows/github-workflow-dangerous-pattern-default-secret-pr.yml @@ -0,0 +1,36 @@ +name: Close issue on Jira + +on: + pull_request + +env: + BLA: ${{ secrets.GITHUB_TOKEN }} + +jobs: + test1: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v1.2.3 + with: + ref: ${{ github.event.pull_request.head.sha }} + name: Use in env toJson + + - uses: some/action@v1.2.3 + with: + option: ${{ secrets.GITHUB_TOKEN }} + name: Use secret in args + + - name: Use in with toJson + env: + GITHUB_CONTEXT: ${{ secrets.GITHUB_TOKEN }} + run: | + echo "$GITHUB_CONTEXT" + echo "${{ secrets.GITHUB_TOKEN }}" + + - name: Use in with toJson + uses: some/action@v1.2.3 + env: + GITHUB_CONTEXT: ${{ secrets.GITHUB_TOKEN }} + run: | + echo "$GITHUB_CONTEXT" + echo "${{ secrets.GITHUB_TOKEN }}" diff --git a/checks/testdata/.github/workflows/github-workflow-dangerous-pattern-default-secret-prt.yml b/checks/testdata/.github/workflows/github-workflow-dangerous-pattern-default-secret-prt.yml new file mode 100644 index 00000000000..28e185b57cb --- /dev/null +++ b/checks/testdata/.github/workflows/github-workflow-dangerous-pattern-default-secret-prt.yml @@ -0,0 +1,36 @@ +name: Close issue on Jira + +on: + pull_request_target + +env: + BLA: ${{ secrets.GITHUB_TOKEN }} + +jobs: + test1: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v1.2.3 + with: + ref: ${{ github.event.pull_request.head.sha }} + name: Use in env toJson + + - uses: some/action@v1.2.3 + with: + option: ${{ secrets.GITHUB_TOKEN }} + name: Use secret in args + + - name: Use in with toJson + env: + GITHUB_CONTEXT: ${{ secrets.GITHUB_TOKEN }} + run: | + echo "$GITHUB_CONTEXT" + echo "${{ secrets.GITHUB_TOKEN }}" + + - name: Use in with toJson + uses: some/action@v1.2.3 + env: + GITHUB_CONTEXT: ${{ secrets.GITHUB_TOKEN }} + run: | + echo "$GITHUB_CONTEXT" + echo "${{ secrets.GITHUB_TOKEN }}" From 79c49239089df5ff535f229d97714a156d495cd8 Mon Sep 17 00:00:00 2001 From: laurentsimon Date: Tue, 22 Mar 2022 23:24:39 +0000 Subject: [PATCH 2/2] missing files --- checks/dangerous_workflow.go | 49 +++++++++++++++++++++---------- checks/dangerous_workflow_test.go | 22 ++++++++++++++ 2 files changed, 56 insertions(+), 15 deletions(-) diff --git a/checks/dangerous_workflow.go b/checks/dangerous_workflow.go index bfe83e65a51..f9a0e342137 100644 --- a/checks/dangerous_workflow.go +++ b/checks/dangerous_workflow.go @@ -109,7 +109,8 @@ func DangerousWorkflow(c *checker.CheckRequest) checker.CheckResult { // Check file content. var validateGitHubActionWorkflowPatterns fileparser.DoWhileTrueOnFileContent = func(path string, content []byte, - args ...interface{}) (bool, error) { + args ...interface{}, +) (bool, error) { if !fileparser.IsWorkflowFile(path) { return true, nil } @@ -160,7 +161,8 @@ var validateGitHubActionWorkflowPatterns fileparser.DoWhileTrueOnFileContent = f } func validateSecretsInPullRequests(workflow *actionlint.Workflow, path string, - dl checker.DetailLogger, pdata *patternCbData) error { + dl checker.DetailLogger, pdata *patternCbData, +) error { triggers := make(map[triggerName]bool) // We need pull request trigger. @@ -194,7 +196,8 @@ func validateSecretsInPullRequests(workflow *actionlint.Workflow, path string, } func validateUntrustedCodeCheckout(workflow *actionlint.Workflow, path string, - dl checker.DetailLogger, pdata *patternCbData) error { + dl checker.DetailLogger, pdata *patternCbData, +) error { if !usesEventTrigger(workflow, triggerPullRequestTarget) { return nil } @@ -229,7 +232,8 @@ func jobUsesEnvironment(job *actionlint.Job) bool { } func checkJobForUsedSecrets(job *actionlint.Job, triggers map[triggerName]bool, - path string, dl checker.DetailLogger, pdata *patternCbData) error { + path string, dl checker.DetailLogger, pdata *patternCbData, +) error { if job == nil { return nil } @@ -271,7 +275,8 @@ func checkJobForUsedSecrets(job *actionlint.Job, triggers map[triggerName]bool, } func workflowUsesCodeCheckoutAndNoEnvironment(workflow *actionlint.Workflow, - triggers map[triggerName]bool) bool { + triggers map[triggerName]bool, +) bool { if workflow == nil { return false } @@ -318,7 +323,8 @@ func jobUsesCodeCheckout(job *actionlint.Job) (bool, string) { } func checkJobForUntrustedCodeCheckout(job *actionlint.Job, path string, - dl checker.DetailLogger, pdata *patternCbData) error { + dl checker.DetailLogger, pdata *patternCbData, +) error { if job == nil { return nil } @@ -359,7 +365,8 @@ func checkJobForUntrustedCodeCheckout(job *actionlint.Job, path string, } func validateScriptInjection(workflow *actionlint.Workflow, path string, - dl checker.DetailLogger, pdata *patternCbData) error { + dl checker.DetailLogger, pdata *patternCbData, +) error { for _, job := range workflow.Jobs { if job == nil { continue @@ -382,7 +389,8 @@ func validateScriptInjection(workflow *actionlint.Workflow, path string, } func checkWorkflowSecretInEnv(workflow *actionlint.Workflow, triggers map[triggerName]bool, - path string, dl checker.DetailLogger, pdata *patternCbData) error { + path string, dl checker.DetailLogger, pdata *patternCbData, +) error { // We need code checkout and not environment rule protection. if !workflowUsesCodeCheckoutAndNoEnvironment(workflow, triggers) { return nil @@ -392,7 +400,8 @@ func checkWorkflowSecretInEnv(workflow *actionlint.Workflow, triggers map[trigge } func checkSecretInEnv(env *actionlint.Env, path string, - dl checker.DetailLogger, pdata *patternCbData) error { + dl checker.DetailLogger, pdata *patternCbData, +) error { if env == nil { return nil } @@ -406,7 +415,8 @@ func checkSecretInEnv(env *actionlint.Env, path string, } func checkSecretInRun(step *actionlint.Step, path string, - dl checker.DetailLogger, pdata *patternCbData) error { + dl checker.DetailLogger, pdata *patternCbData, +) error { if step == nil || step.Exec == nil { return nil } @@ -421,7 +431,8 @@ func checkSecretInRun(step *actionlint.Step, path string, } func checkSecretInActionArgs(step *actionlint.Step, path string, - dl checker.DetailLogger, pdata *patternCbData) error { + dl checker.DetailLogger, pdata *patternCbData, +) error { if step == nil || step.Exec == nil { return nil } @@ -442,7 +453,8 @@ func checkSecretInActionArgs(step *actionlint.Step, path string, } func checkSecretInScript(script string, pos *actionlint.Pos, path string, - dl checker.DetailLogger, pdata *patternCbData) error { + dl checker.DetailLogger, pdata *patternCbData, +) error { for { s := strings.Index(script, "${{") if s == -1 { @@ -454,8 +466,13 @@ func checkSecretInScript(script string, pos *actionlint.Pos, path string, return sce.WithMessage(sce.ErrScorecardInternal, errInvalidGitHubWorkflow.Error()) } + // Note: The default GitHub token is allowed, as it has + // only read permission for `pull_request`. + // For `pull_request_event`, we use other signals such as + // whether checkout action is used. variable := strings.Trim(script[s:s+e+2], " ") - if strings.Contains(variable, "secrets.") { + if !strings.Contains(variable, "secrets.GITHUB_TOKEN") && + strings.Contains(variable, "secrets.") { line := fileparser.GetLineNumber(pos) dl.Warn(&checker.LogMessage{ Path: path, @@ -472,7 +489,8 @@ func checkSecretInScript(script string, pos *actionlint.Pos, path string, } func checkVariablesInScript(script string, pos *actionlint.Pos, path string, - dl checker.DetailLogger, pdata *patternCbData) error { + dl checker.DetailLogger, pdata *patternCbData, +) error { for { s := strings.Index(script, "${{") if s == -1 { @@ -548,7 +566,8 @@ func createResultForDangerousWorkflowPatterns(result patternCbData, err error) c } func testValidateGitHubActionDangerousWorkflow(pathfn string, - content []byte, dl checker.DetailLogger) checker.CheckResult { + content []byte, dl checker.DetailLogger, +) checker.CheckResult { data := patternCbData{ workflowPattern: make(map[dangerousResults]bool), } diff --git a/checks/dangerous_workflow_test.go b/checks/dangerous_workflow_test.go index 065dae30ebc..fc2e018faaf 100644 --- a/checks/dangerous_workflow_test.go +++ b/checks/dangerous_workflow_test.go @@ -241,6 +241,28 @@ func TestGithubDangerousWorkflow(t *testing.T) { NumberOfDebug: 0, }, }, + { + name: "default secret in pull request", + filename: "./testdata/.github/workflows/github-workflow-dangerous-pattern-default-secret-pr.yml", + expected: scut.TestReturn{ + Error: nil, + Score: checker.MaxResultConfidence, + NumberOfWarn: 0, + NumberOfInfo: 0, + NumberOfDebug: 0, + }, + }, + { + name: "default secret in pull request target", + filename: "./testdata/.github/workflows/github-workflow-dangerous-pattern-default-secret-prt.yml", + expected: scut.TestReturn{ + Error: nil, + Score: checker.MinResultConfidence, + NumberOfWarn: 1, + 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",