From e7fd58d9a33e1a3851f9d0db71c206fea4ad59d8 Mon Sep 17 00:00:00 2001 From: laurentsimon <64505099+laurentsimon@users.noreply.github.com> Date: Tue, 15 Feb 2022 08:04:57 -0800 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20Check=20for=20secrets=20in=20pull?= =?UTF-8?q?=5Frequest=5Ftarget=20(#1634)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * checks/dangerous_workflow.go: add pull_request_target support for secrets * missing files * linter --- checks/dangerous_workflow.go | 68 ++++++++++++++----- checks/dangerous_workflow_test.go | 58 +++++++++++++++- ...-pattern-secret-env-checkout-noref-prt.yml | 52 ++++++++++++++ ...ous-pattern-secret-env-environment-prt.yml | 63 +++++++++++++++++ ...ous-pattern-secret-env-no-checkout-prt.yml | 52 ++++++++++++++ ...kflow-dangerous-pattern-secret-env-prt.yml | 27 ++++++++ ...kflow-dangerous-pattern-secret-run-prt.yml | 14 ++++ e2e/dangerous_workflow_test.go | 4 +- 8 files changed, 316 insertions(+), 22 deletions(-) create mode 100644 checks/testdata/.github/workflows/github-workflow-dangerous-pattern-secret-env-checkout-noref-prt.yml create mode 100644 checks/testdata/.github/workflows/github-workflow-dangerous-pattern-secret-env-environment-prt.yml create mode 100644 checks/testdata/.github/workflows/github-workflow-dangerous-pattern-secret-env-no-checkout-prt.yml create mode 100644 checks/testdata/.github/workflows/github-workflow-dangerous-pattern-secret-env-prt.yml create mode 100644 checks/testdata/.github/workflows/github-workflow-dangerous-pattern-secret-run-prt.yml diff --git a/checks/dangerous_workflow.go b/checks/dangerous_workflow.go index 281a3e07..5e6923d9 100644 --- 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,13 +256,19 @@ 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 } @@ -251,10 +276,12 @@ func workflowUsesCodeCheckoutAndNoEnvironment(workflow *actionlint.Workflow) boo 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 index 4d59d316..065dae30 100644 --- 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/checks/testdata/.github/workflows/github-workflow-dangerous-pattern-secret-env-checkout-noref-prt.yml b/checks/testdata/.github/workflows/github-workflow-dangerous-pattern-secret-env-checkout-noref-prt.yml new file mode 100644 index 00000000..6cdfa84f --- /dev/null +++ b/checks/testdata/.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/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 }}" + \ No newline at end of file diff --git a/checks/testdata/.github/workflows/github-workflow-dangerous-pattern-secret-env-environment-prt.yml b/checks/testdata/.github/workflows/github-workflow-dangerous-pattern-secret-env-environment-prt.yml new file mode 100644 index 00000000..6e7cbdff --- /dev/null +++ b/checks/testdata/.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/checkout@v1.2.3 + 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/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 + \ No newline at end of file diff --git a/checks/testdata/.github/workflows/github-workflow-dangerous-pattern-secret-env-no-checkout-prt.yml b/checks/testdata/.github/workflows/github-workflow-dangerous-pattern-secret-env-no-checkout-prt.yml new file mode 100644 index 00000000..354742a2 --- /dev/null +++ b/checks/testdata/.github/workflows/github-workflow-dangerous-pattern-secret-env-no-checkout-prt.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 }}" + \ No newline at end of file diff --git a/checks/testdata/.github/workflows/github-workflow-dangerous-pattern-secret-env-prt.yml b/checks/testdata/.github/workflows/github-workflow-dangerous-pattern-secret-env-prt.yml new file mode 100644 index 00000000..8e39566b --- /dev/null +++ b/checks/testdata/.github/workflows/github-workflow-dangerous-pattern-secret-env-prt.yml @@ -0,0 +1,27 @@ +name: Close issue on Jira + +on: + pull_request_target + +env: + BLA: ${{ secrets.SE00 }} + +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 + + - 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" diff --git a/checks/testdata/.github/workflows/github-workflow-dangerous-pattern-secret-run-prt.yml b/checks/testdata/.github/workflows/github-workflow-dangerous-pattern-secret-run-prt.yml new file mode 100644 index 00000000..fdaaea25 --- /dev/null +++ b/checks/testdata/.github/workflows/github-workflow-dangerous-pattern-secret-run-prt.yml @@ -0,0 +1,14 @@ +name: Close issue on Jira + +on: + pull_request_target + +jobs: + test1: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v1.2.3 + with: + ref: ${{ github.event.pull_request.head.sha }} + - name: Use in run toJson + run: echo "${{ toJson(secrets.SE13) }}" \ No newline at end of file diff --git a/e2e/dangerous_workflow_test.go b/e2e/dangerous_workflow_test.go index 1efcc64b..337f23a9 100644 --- a/e2e/dangerous_workflow_test.go +++ b/e2e/dangerous_workflow_test.go @@ -44,7 +44,7 @@ var _ = Describe("E2E TEST:"+checks.CheckTokenPermissions, func() { expected := scut.TestReturn{ Error: nil, Score: checker.MinResultScore, - NumberOfWarn: 1, + NumberOfWarn: 2, NumberOfInfo: 0, NumberOfDebug: 0, } @@ -73,7 +73,7 @@ var _ = Describe("E2E TEST:"+checks.CheckTokenPermissions, func() { expected := scut.TestReturn{ Error: nil, Score: checker.MinResultScore, - NumberOfWarn: 1, + NumberOfWarn: 2, NumberOfInfo: 0, NumberOfDebug: 0, }