From 4622952c85aa9ecaed82e49ad6c95ee812a926dd Mon Sep 17 00:00:00 2001 From: laurentsimon <64505099+laurentsimon@users.noreply.github.com> Date: Thu, 21 Apr 2022 15:02:18 -0700 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20Raw=20results=20for=20dangerous=20w?= =?UTF-8?q?orkflow=20(#1849)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * draft * update * update * updates * comments * comments * comments --- checker/raw_result.go | 34 ++ checks/dangerous_workflow.go | 559 +----------------------- checks/dangerous_workflow_test.go | 377 ---------------- checks/evaluation/dangerous_workflow.go | 83 ++++ checks/raw/dangerous_workflow.go | 524 ++++++++++++++++++++++ checks/raw/dangerous_workflow_test.go | 254 +++++++++++ checks/raw/errors.go | 1 + pkg/json_raw_results.go | 103 ++++- 8 files changed, 1008 insertions(+), 927 deletions(-) delete mode 100644 checks/dangerous_workflow_test.go create mode 100644 checks/evaluation/dangerous_workflow.go create mode 100644 checks/raw/dangerous_workflow.go create mode 100644 checks/raw/dangerous_workflow_test.go diff --git a/checker/raw_result.go b/checker/raw_result.go index 3e7cd484719..2c3cbf2215c 100644 --- a/checker/raw_result.go +++ b/checker/raw_result.go @@ -20,6 +20,7 @@ import "time" // is applied. //nolint type RawResults struct { + DangerousWorkflowResults DangerousWorkflowData VulnerabilitiesResults VulnerabilitiesData BinaryArtifactResults BinaryArtifactData SecurityPolicyResults SecurityPolicyData @@ -256,3 +257,36 @@ type ReleaseAsset struct { Name string URL string } + +// DangerousWorkflowData contains raw results +// for dangerous workflow check. +type DangerousWorkflowData struct { + ScriptInjections []ScriptInjection + SecretInPullRequests []EncryptedSecret + UntrustedCheckouts []UntrustedCheckout + // TODO: other +} + +// UntrustedCheckout represents an untrusted checkout. +type UntrustedCheckout struct { + Job *WorkflowJob + File File +} + +// ScriptInjection represents a script injection. +type ScriptInjection struct { + Job *WorkflowJob + File File +} + +// EncryptedSecret represents an encrypted secret. +type EncryptedSecret struct { + Job *WorkflowJob + File File +} + +// WorkflowJob reprresents a workflow job. +type WorkflowJob struct { + Name *string + ID *string +} diff --git a/checks/dangerous_workflow.go b/checks/dangerous_workflow.go index 5512017a835..62a4d96c84e 100644 --- a/checks/dangerous_workflow.go +++ b/checks/dangerous_workflow.go @@ -15,48 +15,15 @@ package checks import ( - "fmt" - "regexp" - "strings" - - "github.com/rhysd/actionlint" - "github.com/ossf/scorecard/v4/checker" - "github.com/ossf/scorecard/v4/checks/fileparser" + "github.com/ossf/scorecard/v4/checks/evaluation" + "github.com/ossf/scorecard/v4/checks/raw" sce "github.com/ossf/scorecard/v4/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() { supportedRequestTypes := []checker.RequestType{ @@ -69,523 +36,19 @@ func init() { } } -type dangerousResults int - -const ( - scriptInjection dangerousResults = iota - untrustedCheckout - secretsViaPullRequests -) - -type triggerName string - -var ( - triggerPullRequestTarget = triggerName("pull_request_target") - triggerWorkflowRun = triggerName("workflow_run") - triggerPullRequest = triggerName("pull_request") - checkoutUntrustedPullRequestRef = "github.event.pull_request" - checkoutUntrustedWorkflowRunRef = "github.event.workflow_run" -) - -// 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[dangerousResults]bool -} - -// DangerousWorkflow runs Dangerous-Workflow check. +// DangerousWorkflow will check the repository contains Dangerous-Workflow. func DangerousWorkflow(c *checker.CheckRequest) checker.CheckResult { - // data is shared across all GitHub workflows. - data := patternCbData{ - workflowPattern: make(map[dangerousResults]bool), - } - err := fileparser.OnMatchingFileContentDo(c.RepoClient, fileparser.PathMatcher{ - Pattern: ".github/workflows/*", - CaseSensitive: false, - }, - validateGitHubActionWorkflowPatterns, c.Dlogger, &data) - return createResultForDangerousWorkflowPatterns(data, err) -} - -// Check file content. -var validateGitHubActionWorkflowPatterns fileparser.DoWhileTrueOnFileContent = func(path string, - content []byte, - args ...interface{}, -) (bool, error) { - if !fileparser.IsWorkflowFile(path) { - return true, nil - } - - if len(args) != 2 { - return false, fmt.Errorf( - "validateGitHubActionWorkflowPatterns requires exactly 2 arguments: %w", errInvalidArgLength) - } - - // Verify the type of the data. - pdata, ok := args[1].(*patternCbData) - if !ok { - return false, fmt.Errorf( - "validateGitHubActionWorkflowPatterns expects arg[0] of type *patternCbData: %w", errInvalidArgType) - } - dl, ok := args[0].(checker.DetailLogger) - if !ok { - return false, fmt.Errorf( - "validateGitHubActionWorkflowPatterns expects arg[1] of type checker.DetailLogger: %w", errInvalidArgType) - } - - if !fileparser.CheckFileContainsCommands(content, "#") { - return true, nil - } - - workflow, errs := actionlint.Parse(content) - if len(errs) > 0 && workflow == nil { - return false, fileparser.FormatActionlintError(errs) - } - - // 1. Check for untrusted code checkout with pull_request_target and a ref - if err := validateUntrustedCodeCheckout(workflow, path, dl, pdata); err != nil { - return false, err - } - - // 2. Check for script injection in workflow inline scripts. - if err := validateScriptInjection(workflow, path, dl, pdata); err != nil { - 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 { - triggers := make(map[triggerName]bool) - - // We need pull request trigger. - usesPullRequest := usesEventTrigger(workflow, triggerPullRequest) - usesPullRequestTarget := usesEventTrigger(workflow, triggerPullRequestTarget) - usesWorkflowRun := usesEventTrigger(workflow, triggerWorkflowRun) - - if !usesPullRequest && !usesPullRequestTarget && !usesWorkflowRun { - return nil - } - - // Record the triggers. - if usesPullRequest { - triggers[triggerPullRequest] = usesPullRequest - } - if usesPullRequestTarget { - triggers[triggerPullRequestTarget] = usesPullRequestTarget - } - if usesWorkflowRun { - triggers[triggerWorkflowRun] = usesWorkflowRun - } - - // Secrets used in env at the top of the wokflow. - if err := checkWorkflowSecretInEnv(workflow, triggers, path, dl, pdata); err != nil { - return err - } - - // Secrets used on jobs. - for _, job := range workflow.Jobs { - if err := checkJobForUsedSecrets(job, triggers, path, dl, pdata); err != nil { - return err - } - } - - return nil -} - -func validateUntrustedCodeCheckout(workflow *actionlint.Workflow, path string, - dl checker.DetailLogger, pdata *patternCbData, -) error { - if !usesEventTrigger(workflow, triggerPullRequestTarget) && !usesEventTrigger(workflow, triggerWorkflowRun) { - 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, triggers map[triggerName]bool, - 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 !jobUsesCodeCheckoutAndNoEnvironment(job, triggers) { - 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 workflowUsesCodeCheckoutAndNoEnvironment(workflow *actionlint.Workflow, - triggers map[triggerName]bool, -) bool { - if workflow == nil { - return false - } - - for _, job := range workflow.Jobs { - if jobUsesCodeCheckoutAndNoEnvironment(job, triggers) { - return true - } - } - return false -} - -func jobUsesCodeCheckoutAndNoEnvironment(job *actionlint.Job, triggers map[triggerName]bool, -) bool { - if job == nil { - return false - } - _, usesPullRequest := triggers[triggerPullRequest] - _, usesPullRequestTarget := triggers[triggerPullRequestTarget] - _, usesWorkflowRun := triggers[triggerWorkflowRun] - - chk, ref := jobUsesCodeCheckout(job) - if !jobUsesEnvironment(job) { - if (chk && usesPullRequest) || - (chk && usesPullRequestTarget && strings.Contains(ref, checkoutUntrustedPullRequestRef)) || - (chk && usesWorkflowRun && strings.Contains(ref, checkoutUntrustedWorkflowRunRef)) { - return true - } - } - - return false -} - -func jobUsesCodeCheckout(job *actionlint.Job) (bool, string) { - if job == nil { - return false, "" - } - - hasCheckout := 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") { - hasCheckout = true - ref, ok := e.Inputs["ref"] - if !ok || ref.Value == nil { - continue - } - return true, ref.Value.Value - } - } - return hasCheckout, "" -} - -func checkJobForUntrustedCodeCheckout(job *actionlint.Job, path string, - dl checker.DetailLogger, pdata *patternCbData, -) error { - if job == nil { - return nil - } - - // Check each step, which is a map, for checkouts with untrusted ref - 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") { - continue - } - // Check for reference. If not defined for a pull_request_target event, this defaults to - // the base branch of the pull request. - ref, ok := e.Inputs["ref"] - if !ok || ref.Value == nil { - continue - } - - if strings.Contains(ref.Value.Value, checkoutUntrustedPullRequestRef) || - strings.Contains(ref.Value.Value, checkoutUntrustedWorkflowRunRef) { - line := fileparser.GetLineNumber(step.Pos) - dl.Warn(&checker.LogMessage{ - Path: path, - Type: checker.FileTypeSource, - Offset: line, - Text: fmt.Sprintf("untrusted code checkout '%v'", ref.Value.Value), - // TODO: set Snippet. - }) - // Detected untrusted checkout. - pdata.workflowPattern[untrustedCheckout] = true - } - } - 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 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, triggers) { - 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()) - } - - // 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.GITHUB_TOKEN") && - strings.Contains(variable, "secrets.") { - line := fileparser.GetLineNumber(pos) - dl.Warn(&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 -} - -func checkVariablesInScript(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()) - } - - // Check if the variable may be untrustworthy. - variable := script[s+3 : s+e] - if containsUntrustedContextPattern(variable) { - line := fileparser.GetLineNumber(pos) - dl.Warn(&checker.LogMessage{ - Path: path, - Type: checker.FileTypeSource, - Offset: line, - Text: fmt.Sprintf("script injection with untrusted input '%v'", variable), - // TODO: set Snippet. - }) - pdata.workflowPattern[scriptInjection] = true - } - script = script[s+e:] - } - return nil -} - -// Calculate the workflow score. -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[untrustedCheckout]; ok { - score -= 10 - } - - // 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 - } - - // We're done, calculate the final score. - if score < checker.MinResultScore { - return checker.MinResultScore - } - - return int(score) -} - -// Create the result. -func createResultForDangerousWorkflowPatterns(result patternCbData, err error) checker.CheckResult { + rawData, err := raw.DangerousWorkflow(c.RepoClient) if err != nil { - return checker.CreateRuntimeErrorResult(CheckDangerousWorkflow, err) + e := sce.WithMessage(sce.ErrScorecardInternal, err.Error()) + return checker.CreateRuntimeErrorResult(CheckDangerousWorkflow, e) } - score := calculateWorkflowScore(result) - - if score != checker.MaxResultScore { - return checker.CreateResultWithScore(CheckDangerousWorkflow, - "dangerous workflow patterns detected", score) + // Return raw results. + if c.RawResults != nil { + c.RawResults.DangerousWorkflowResults = rawData } - return checker.CreateMaxScoreResult(CheckDangerousWorkflow, - "no dangerous workflow patterns detected") -} - -func testValidateGitHubActionDangerousWorkflow(pathfn string, - content []byte, dl checker.DetailLogger, -) checker.CheckResult { - data := patternCbData{ - workflowPattern: make(map[dangerousResults]bool), - } - _, err := validateGitHubActionWorkflowPatterns(pathfn, content, dl, &data) - return createResultForDangerousWorkflowPatterns(data, err) + // Return the score evaluation. + return evaluation.DangerousWorkflow(CheckDangerousWorkflow, c.Dlogger, &rawData) } diff --git a/checks/dangerous_workflow_test.go b/checks/dangerous_workflow_test.go deleted file mode 100644 index 28ecc63afd9..00000000000 --- a/checks/dangerous_workflow_test.go +++ /dev/null @@ -1,377 +0,0 @@ -// 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. - -package checks - -import ( - "fmt" - "io/ioutil" - "strings" - "testing" - - "github.com/ossf/scorecard/v4/checker" - scut "github.com/ossf/scorecard/v4/utests" -) - -func TestGithubDangerousWorkflow(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - filename string - expected scut.TestReturn - }{ - { - name: "Non-yaml file", - filename: "./testdata/script.sh", - expected: scut.TestReturn{ - Error: nil, - Score: checker.MaxResultScore, - NumberOfWarn: 0, - NumberOfInfo: 0, - NumberOfDebug: 0, - }, - }, - { - name: "run untrusted code checkout test - workflow_run", - filename: "./testdata/.github/workflows/github-workflow-dangerous-pattern-untrusted-checkout-workflow_run.yml", - expected: scut.TestReturn{ - Error: nil, - Score: checker.MinResultScore, - NumberOfWarn: 2, - NumberOfInfo: 0, - NumberOfDebug: 0, - }, - }, - { - name: "run untrusted code checkout test", - filename: "./testdata/.github/workflows/github-workflow-dangerous-pattern-untrusted-checkout.yml", - expected: scut.TestReturn{ - Error: nil, - Score: checker.MinResultScore, - NumberOfWarn: 2, - NumberOfInfo: 0, - NumberOfDebug: 0, - }, - }, - { - name: "run trusted code checkout test", - filename: "./testdata/.github/workflows/github-workflow-dangerous-pattern-trusted-checkout.yml", - expected: scut.TestReturn{ - Error: nil, - Score: checker.MaxResultScore, - NumberOfWarn: 0, - NumberOfInfo: 0, - NumberOfDebug: 0, - }, - }, - { - name: "run default code checkout test", - filename: "./testdata/.github/workflows/github-workflow-dangerous-pattern-default-checkout.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", - expected: scut.TestReturn{ - Error: nil, - Score: checker.MinResultScore, - NumberOfWarn: 1, - NumberOfInfo: 0, - NumberOfDebug: 0, - }, - }, - { - name: "run safe script injection", - filename: "./testdata/.github/workflows/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/workflows/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/workflows/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/workflows/github-workflow-dangerous-pattern-untrusted-script-injection-wildcard.yml", - expected: scut.TestReturn{ - Error: nil, - Score: checker.MinResultConfidence, - NumberOfWarn: 1, - NumberOfInfo: 0, - 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, - }, - }, - { - 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: "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", - 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 - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - var content []byte - var err error - if tt.filename == "" { - content = make([]byte, 0) - } else { - content, err = ioutil.ReadFile(tt.filename) - if err != nil { - panic(fmt.Errorf("cannot read file: %w", err)) - } - } - 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() - } - }) - } -} - -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() - } - }) - } -} diff --git a/checks/evaluation/dangerous_workflow.go b/checks/evaluation/dangerous_workflow.go new file mode 100644 index 00000000000..a4ebc963e31 --- /dev/null +++ b/checks/evaluation/dangerous_workflow.go @@ -0,0 +1,83 @@ +// 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. + +package evaluation + +import ( + "fmt" + + "github.com/ossf/scorecard/v4/checker" + sce "github.com/ossf/scorecard/v4/errors" +) + +// DangerousWorkflow applies the score policy for the DangerousWorkflow check. +func DangerousWorkflow(name string, dl checker.DetailLogger, + r *checker.DangerousWorkflowData, +) checker.CheckResult { + if r == nil { + e := sce.WithMessage(sce.ErrScorecardInternal, "empty raw data") + return checker.CreateRuntimeErrorResult(name, e) + } + + // Script injections. + for _, e := range r.ScriptInjections { + dl.Warn(&checker.LogMessage{ + Path: e.File.Path, + Type: e.File.Type, + Offset: e.File.Offset, + Text: fmt.Sprintf("script injection with untrusted input '%v'", e.File.Snippet), + Snippet: e.File.Snippet, + }) + } + + // Untrusted checkouts. + for _, e := range r.UntrustedCheckouts { + dl.Warn(&checker.LogMessage{ + Path: e.File.Path, + Type: e.File.Type, + Offset: e.File.Offset, + Text: fmt.Sprintf("untrusted code checkout '%v'", e.File.Snippet), + Snippet: e.File.Snippet, + }) + } + + // Secrets in pull requests. + for _, e := range r.SecretInPullRequests { + dl.Warn(&checker.LogMessage{ + Path: e.File.Path, + Type: e.File.Type, + Offset: e.File.Offset, + Text: fmt.Sprintf("secret accessible to pull requests '%v'", e.File.Snippet), + Snippet: e.File.Snippet, + }) + } + + if len(r.ScriptInjections) > 0 || + len(r.UntrustedCheckouts) > 0 || + len(r.SecretInPullRequests) > 0 { + return createResult(name, checker.MinResultScore) + } + return createResult(name, checker.MaxResultScore) +} + +// Create the result. +func createResult(name string, score int) checker.CheckResult { + if score != checker.MaxResultScore { + return checker.CreateResultWithScore(name, + "dangerous workflow patterns detected", score) + } + + return checker.CreateMaxScoreResult(name, + "no dangerous workflow patterns detected") +} diff --git a/checks/raw/dangerous_workflow.go b/checks/raw/dangerous_workflow.go new file mode 100644 index 00000000000..15755c9bb8d --- /dev/null +++ b/checks/raw/dangerous_workflow.go @@ -0,0 +1,524 @@ +// 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. + +package raw + +import ( + "fmt" + "regexp" + "strings" + + "github.com/rhysd/actionlint" + + "github.com/ossf/scorecard/v4/checker" + "github.com/ossf/scorecard/v4/checks/fileparser" + "github.com/ossf/scorecard/v4/clients" + sce "github.com/ossf/scorecard/v4/errors" +) + +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) +} + +type triggerName string + +var ( + triggerPullRequestTarget = triggerName("pull_request_target") + triggerWorkflowRun = triggerName("workflow_run") + triggerPullRequest = triggerName("pull_request") + checkoutUntrustedPullRequestRef = "github.event.pull_request" + checkoutUntrustedWorkflowRunRef = "github.event.workflow_run" +) + +// DangerousWorkflow retrieves the raw data for the DangerousWorkflow check. +func DangerousWorkflow(c clients.RepoClient) (checker.DangerousWorkflowData, error) { + // data is shared across all GitHub workflows. + var data checker.DangerousWorkflowData + err := fileparser.OnMatchingFileContentDo(c, fileparser.PathMatcher{ + Pattern: ".github/workflows/*", + CaseSensitive: false, + }, validateGitHubActionWorkflowPatterns, &data) + + return data, err +} + +// Check file content. +var validateGitHubActionWorkflowPatterns fileparser.DoWhileTrueOnFileContent = func(path string, + content []byte, + args ...interface{}, +) (bool, error) { + if !fileparser.IsWorkflowFile(path) { + return true, nil + } + + if len(args) != 1 { + return false, fmt.Errorf( + "validateGitHubActionWorkflowPatterns requires exactly 2 arguments: %w", errInvalidArgLength) + } + + // Verify the type of the data. + pdata, ok := args[0].(*checker.DangerousWorkflowData) + if !ok { + return false, fmt.Errorf( + "validateGitHubActionWorkflowPatterns expects arg[0] of type *patternCbData: %w", errInvalidArgType) + } + + if !fileparser.CheckFileContainsCommands(content, "#") { + return true, nil + } + + workflow, errs := actionlint.Parse(content) + if len(errs) > 0 && workflow == nil { + return false, fileparser.FormatActionlintError(errs) + } + + // 1. Check for untrusted code checkout with pull_request_target and a ref + if err := validateUntrustedCodeCheckout(workflow, path, pdata); err != nil { + return false, err + } + + // 2. Check for script injection in workflow inline scripts. + if err := validateScriptInjection(workflow, path, pdata); err != nil { + return false, err + } + + // 3. Check for secrets used in workflows triggered by pull requests. + if err := validateSecretsInPullRequests(workflow, path, pdata); err != nil { + return false, err + } + + // TODO: Check other dangerous patterns. + return true, nil +} + +func validateSecretsInPullRequests(workflow *actionlint.Workflow, path string, + pdata *checker.DangerousWorkflowData, +) error { + triggers := make(map[triggerName]bool) + + // We need pull request trigger. + usesPullRequest := usesEventTrigger(workflow, triggerPullRequest) + usesPullRequestTarget := usesEventTrigger(workflow, triggerPullRequestTarget) + usesWorkflowRun := usesEventTrigger(workflow, triggerWorkflowRun) + + if !usesPullRequest && !usesPullRequestTarget && !usesWorkflowRun { + return nil + } + + // Record the triggers. + if usesPullRequest { + triggers[triggerPullRequest] = usesPullRequest + } + if usesPullRequestTarget { + triggers[triggerPullRequestTarget] = usesPullRequestTarget + } + if usesWorkflowRun { + triggers[triggerWorkflowRun] = usesWorkflowRun + } + + // Secrets used in env at the top of the wokflow. + if err := checkWorkflowSecretInEnv(workflow, triggers, path, pdata); err != nil { + return err + } + + // Secrets used on jobs. + for _, job := range workflow.Jobs { + if err := checkJobForUsedSecrets(job, triggers, path, pdata); err != nil { + return err + } + } + + return nil +} + +func validateUntrustedCodeCheckout(workflow *actionlint.Workflow, path string, + pdata *checker.DangerousWorkflowData, +) error { + if !usesEventTrigger(workflow, triggerPullRequestTarget) && !usesEventTrigger(workflow, triggerWorkflowRun) { + return nil + } + + for _, job := range workflow.Jobs { + if err := checkJobForUntrustedCodeCheckout(job, path, 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, triggers map[triggerName]bool, + path string, pdata *checker.DangerousWorkflowData, +) 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 !jobUsesCodeCheckoutAndNoEnvironment(job, triggers) { + 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, job, path, pdata); err != nil { + return err + } + + if err := checkSecretInRun(step, job, path, pdata); err != nil { + return err + } + + if err := checkSecretInEnv(step.Env, job, path, pdata); err != nil { + return err + } + } + return nil +} + +func workflowUsesCodeCheckoutAndNoEnvironment(workflow *actionlint.Workflow, + triggers map[triggerName]bool, +) bool { + if workflow == nil { + return false + } + + for _, job := range workflow.Jobs { + if jobUsesCodeCheckoutAndNoEnvironment(job, triggers) { + return true + } + } + return false +} + +func jobUsesCodeCheckoutAndNoEnvironment(job *actionlint.Job, triggers map[triggerName]bool, +) bool { + if job == nil { + return false + } + _, usesPullRequest := triggers[triggerPullRequest] + _, usesPullRequestTarget := triggers[triggerPullRequestTarget] + _, usesWorkflowRun := triggers[triggerWorkflowRun] + + chk, ref := jobUsesCodeCheckout(job) + if !jobUsesEnvironment(job) { + if (chk && usesPullRequest) || + (chk && usesPullRequestTarget && strings.Contains(ref, checkoutUntrustedPullRequestRef)) || + (chk && usesWorkflowRun && strings.Contains(ref, checkoutUntrustedWorkflowRunRef)) { + return true + } + } + + return false +} + +func jobUsesCodeCheckout(job *actionlint.Job) (bool, string) { + if job == nil { + return false, "" + } + + hasCheckout := 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") { + hasCheckout = true + ref, ok := e.Inputs["ref"] + if !ok || ref.Value == nil { + continue + } + return true, ref.Value.Value + } + } + return hasCheckout, "" +} + +func createJob(job *actionlint.Job) *checker.WorkflowJob { + if job == nil { + return nil + } + var r checker.WorkflowJob + if job.Name != nil { + r.Name = &job.Name.Value + } + if job.ID != nil { + r.ID = &job.ID.Value + } + return &r +} + +func checkJobForUntrustedCodeCheckout(job *actionlint.Job, path string, + pdata *checker.DangerousWorkflowData, +) error { + if job == nil { + return nil + } + + // Check each step, which is a map, for checkouts with untrusted ref + 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") { + continue + } + // Check for reference. If not defined for a pull_request_target event, this defaults to + // the base branch of the pull request. + ref, ok := e.Inputs["ref"] + if !ok || ref.Value == nil { + continue + } + + if strings.Contains(ref.Value.Value, checkoutUntrustedPullRequestRef) || + strings.Contains(ref.Value.Value, checkoutUntrustedWorkflowRunRef) { + line := fileparser.GetLineNumber(step.Pos) + pdata.UntrustedCheckouts = append(pdata.UntrustedCheckouts, + checker.UntrustedCheckout{ + File: checker.File{ + Path: path, + Type: checker.FileTypeSource, + Offset: line, + Snippet: ref.Value.Value, + }, + Job: createJob(job), + }, + ) + } + } + return nil +} + +func validateScriptInjection(workflow *actionlint.Workflow, path string, + pdata *checker.DangerousWorkflowData, +) 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, job, path, pdata); err != nil { + return err + } + } + } + return nil +} + +func checkWorkflowSecretInEnv(workflow *actionlint.Workflow, triggers map[triggerName]bool, + path string, pdata *checker.DangerousWorkflowData, +) error { + // We need code checkout and not environment rule protection. + if !workflowUsesCodeCheckoutAndNoEnvironment(workflow, triggers) { + return nil + } + + return checkSecretInEnv(workflow.Env, nil, path, pdata) +} + +func checkSecretInEnv(env *actionlint.Env, job *actionlint.Job, path string, + pdata *checker.DangerousWorkflowData, +) error { + if env == nil { + return nil + } + + for _, v := range env.Vars { + if err := checkSecretInScript(v.Value.Value, v.Value.Pos, job, path, pdata); err != nil { + return err + } + } + return nil +} + +func checkSecretInRun(step *actionlint.Step, job *actionlint.Job, path string, + pdata *checker.DangerousWorkflowData, +) 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, job, path, pdata); err != nil { + return err + } + } + return nil +} + +func checkSecretInActionArgs(step *actionlint.Step, job *actionlint.Job, path string, + pdata *checker.DangerousWorkflowData, +) 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, job, path, pdata); err != nil { + return err + } + } + } + } + return nil +} + +func checkSecretInScript(script string, pos *actionlint.Pos, + job *actionlint.Job, path string, + pdata *checker.DangerousWorkflowData, +) 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()) + } + + // 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.GITHUB_TOKEN") && + strings.Contains(variable, "secrets.") { + line := fileparser.GetLineNumber(pos) + pdata.SecretInPullRequests = append(pdata.SecretInPullRequests, + checker.EncryptedSecret{ + File: checker.File{ + Path: path, + Type: checker.FileTypeSource, + Offset: line, + Snippet: variable, + }, + Job: createJob(job), + }, + ) + } + script = script[s+e:] + } + return nil +} + +func checkVariablesInScript(script string, pos *actionlint.Pos, + job *actionlint.Job, path string, + pdata *checker.DangerousWorkflowData, +) 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()) + } + + // Check if the variable may be untrustworthy. + variable := script[s+3 : s+e] + if containsUntrustedContextPattern(variable) { + line := fileparser.GetLineNumber(pos) + pdata.ScriptInjections = append(pdata.ScriptInjections, + checker.ScriptInjection{ + File: checker.File{ + Path: path, + Type: checker.FileTypeSource, + Offset: line, + Snippet: variable, + }, + Job: createJob(job), + }, + ) + } + script = script[s+e:] + } + return nil +} diff --git a/checks/raw/dangerous_workflow_test.go b/checks/raw/dangerous_workflow_test.go new file mode 100644 index 00000000000..d25e734e9dd --- /dev/null +++ b/checks/raw/dangerous_workflow_test.go @@ -0,0 +1,254 @@ +// 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. + +package raw + +import ( + "errors" + "fmt" + "os" + "testing" + + "github.com/golang/mock/gomock" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + + mockrepo "github.com/ossf/scorecard/v4/clients/mockclients" +) + +func errCmp(e1, e2 error) bool { + return errors.Is(e1, e2) || errors.Is(e2, e1) +} + +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() + } + }) + } +} + +func TestGithubDangerousWorkflow(t *testing.T) { + t.Parallel() + + type ret struct { + err error + nb int + } + tests := []struct { + name string + filename string + expected ret + }{ + { + name: "Non-yaml file", + filename: "script.sh", + expected: ret{nb: 0}, + }, + { + name: "run untrusted code checkout test - workflow_run", + filename: ".github/workflows/github-workflow-dangerous-pattern-untrusted-checkout-workflow_run.yml", + expected: ret{nb: 2}, + }, + { + name: "run untrusted code checkout test", + filename: ".github/workflows/github-workflow-dangerous-pattern-untrusted-checkout.yml", + expected: ret{nb: 2}, + }, + { + name: "run trusted code checkout test", + filename: ".github/workflows/github-workflow-dangerous-pattern-trusted-checkout.yml", + expected: ret{nb: 0}, + }, + { + name: "run default code checkout test", + filename: ".github/workflows/github-workflow-dangerous-pattern-default-checkout.yml", + expected: ret{nb: 0}, + }, + { + name: "run script injection", + filename: ".github/workflows/github-workflow-dangerous-pattern-untrusted-script-injection.yml", + expected: ret{nb: 1}, + }, + { + name: "run safe script injection", + filename: ".github/workflows/github-workflow-dangerous-pattern-trusted-script-injection.yml", + expected: ret{nb: 0}, + }, + { + name: "run multiple script injection", + filename: ".github/workflows/github-workflow-dangerous-pattern-untrusted-multiple-script-injection.yml", + expected: ret{nb: 2}, + }, + { + name: "run inline script injection", + filename: ".github/workflows/github-workflow-dangerous-pattern-untrusted-inline-script-injection.yml", + expected: ret{nb: 1}, + }, + { + name: "run wildcard script injection", + filename: ".github/workflows/github-workflow-dangerous-pattern-untrusted-script-injection-wildcard.yml", + expected: ret{nb: 1}, + }, + { + name: "secret in top env no checkout", + filename: ".github/workflows/github-workflow-dangerous-pattern-secret-env-no-checkout.yml", + expected: ret{nb: 0}, + }, + { + name: "secret in action args", + filename: ".github/workflows/github-workflow-dangerous-pattern-secret-action-args.yml", + expected: ret{nb: 1}, + }, + { + name: "secret in all places", + filename: ".github/workflows/github-workflow-dangerous-pattern-secret-all-checkout.yml", + expected: ret{nb: 7}, + }, + { + name: "secret in env", + filename: ".github/workflows/github-workflow-dangerous-pattern-secret-env.yml", + expected: ret{nb: 2}, + }, + { + name: "secret in env", + filename: ".github/workflows/github-workflow-dangerous-pattern-secret-no-pull-request.yml", + expected: ret{nb: 0}, + }, + { + name: "secret in env", + filename: ".github/workflows/github-workflow-dangerous-pattern-secret-run.yml", + expected: ret{nb: 1}, + }, + { + name: "secret with environment protection", + filename: ".github/workflows/github-workflow-dangerous-pattern-secret-env-environment.yml", + expected: ret{nb: 0}, + }, + { + name: "secret with environment protection pull request target", + filename: ".github/workflows/github-workflow-dangerous-pattern-secret-env-environment-prt.yml", + expected: ret{nb: 1}, + }, + { + name: "secret in env pull request target", + filename: ".github/workflows/github-workflow-dangerous-pattern-secret-run-prt.yml", + expected: ret{nb: 2}, + }, + { + name: "secret in env pull request target", + filename: ".github/workflows/github-workflow-dangerous-pattern-secret-env-prt.yml", + expected: ret{nb: 4}, + }, + { + name: "default secret in pull request", + filename: ".github/workflows/github-workflow-dangerous-pattern-default-secret-pr.yml", + expected: ret{nb: 0}, + }, + { + name: "default secret in pull request target", + filename: ".github/workflows/github-workflow-dangerous-pattern-default-secret-prt.yml", + expected: ret{nb: 1}, + }, + { + name: "secret in top env no checkout pull request target", + filename: ".github/workflows/github-workflow-dangerous-pattern-secret-env-no-checkout-prt.yml", + expected: ret{nb: 0}, + }, + { + name: "secret in top env checkout no ref pull request target", + filename: ".github/workflows/github-workflow-dangerous-pattern-secret-env-checkout-noref-prt.yml", + expected: ret{nb: 0}, + }, + } + 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() + + ctrl := gomock.NewController(t) + mockRepoClient := mockrepo.NewMockRepoClient(ctrl) + mockRepoClient.EXPECT().ListFiles(gomock.Any()).Return([]string{tt.filename}, nil) + mockRepoClient.EXPECT().GetFileContent(gomock.Any()).DoAndReturn(func(file string) ([]byte, error) { + // This will read the file and return the content + content, err := os.ReadFile("../testdata/" + file) + if err != nil { + return content, fmt.Errorf("%w", err) + } + return content, nil + }) + + dw, err := DangerousWorkflow(mockRepoClient) + + if !errCmp(err, tt.expected.err) { + t.Errorf(cmp.Diff(err, tt.expected.err, cmpopts.EquateErrors())) + } + if tt.expected.err != nil { + return + } + + nb := len(dw.ScriptInjections) + len(dw.SecretInPullRequests) + len(dw.UntrustedCheckouts) + if nb != tt.expected.nb { + t.Errorf(cmp.Diff(nb, tt.expected.nb)) + } + }) + } +} diff --git a/checks/raw/errors.go b/checks/raw/errors.go index 6fec85936d0..605e908c20d 100644 --- a/checks/raw/errors.go +++ b/checks/raw/errors.go @@ -23,4 +23,5 @@ var ( errInternalBranchNotFound = errors.New("branch not found") errInvalidArgType = errors.New("invalid arg type") errInvalidArgLength = errors.New("invalid arg length") + errInvalidGitHubWorkflow = errors.New("invalid GitHub workflow") ) diff --git a/pkg/json_raw_results.go b/pkg/json_raw_results.go index 3f150ee4082..c4c2f2d3b35 100644 --- a/pkg/json_raw_results.go +++ b/pkg/json_raw_results.go @@ -35,8 +35,9 @@ type jsonScorecardRawResult struct { // TODO: separate each check extraction into its own file. type jsonFile struct { - Path string `json:"path"` - Offset int `json:"offset,omitempty"` + Snippet *string `json:"snippet,omitempty"` + Path string `json:"path"` + Offset int `json:"offset,omitempty"` } type jsonTool struct { @@ -137,8 +138,36 @@ type jsonLicense struct { // TODO: add fields, like type of license, etc. } +type jsonWorkflows struct { + UntrustedCheckouts []jsonUntrustedCheckout `json:"untrustedCheckouts"` + ScriptInjections []jsonScriptInjection `json:"scriptInjections"` + SecretInPullRequests []jsonEncryptedSecret `json:"secretInPullRequests"` +} + +type jsonUntrustedCheckout struct { + Job *jsonWorkflowJob `json:"job"` + File jsonFile `json:"file"` +} + +type jsonScriptInjection struct { + Job *jsonWorkflowJob `json:"job"` + File jsonFile `json:"file"` +} + +type jsonEncryptedSecret struct { + Job *jsonWorkflowJob `json:"job"` + File jsonFile `json:"file"` +} + +type jsonWorkflowJob struct { + Name *string `json:"name"` + ID *string `json:"id"` +} + //nolint type jsonRawResults struct { + // Workflow results. + Workflows jsonWorkflows `json:"workflows"` // License. Licenses []jsonLicense `json:"licenses"` // List of recent issues. @@ -163,6 +192,71 @@ type jsonRawResults struct { Releases []jsonRelease `json:"releases"` } +//nolint:unparam +func (r *jsonScorecardRawResult) addDangerousWorkflowRawResults(df *checker.DangerousWorkflowData) error { + r.Results.Workflows = jsonWorkflows{} + // Untrusted checkouts. + for _, e := range df.UntrustedCheckouts { + v := jsonUntrustedCheckout{ + File: jsonFile{ + Path: e.File.Path, + Offset: int(e.File.Offset), + }, + } + if e.File.Snippet != "" { + v.File.Snippet = &e.File.Snippet + } + if e.Job != nil { + v.Job = &jsonWorkflowJob{ + Name: e.Job.Name, + ID: e.Job.ID, + } + } + r.Results.Workflows.UntrustedCheckouts = append(r.Results.Workflows.UntrustedCheckouts, v) + } + + // Script injections + for _, e := range df.ScriptInjections { + v := jsonScriptInjection{ + File: jsonFile{ + Path: e.File.Path, + Offset: int(e.File.Offset), + }, + } + if e.File.Snippet != "" { + v.File.Snippet = &e.File.Snippet + } + if e.Job != nil { + v.Job = &jsonWorkflowJob{ + Name: e.Job.Name, + ID: e.Job.ID, + } + } + r.Results.Workflows.ScriptInjections = append(r.Results.Workflows.ScriptInjections, v) + } + + // Secrets in pull requests. + for _, e := range df.SecretInPullRequests { + v := jsonEncryptedSecret{ + File: jsonFile{ + Path: e.File.Path, + Offset: int(e.File.Offset), + }, + } + if e.File.Snippet != "" { + v.File.Snippet = &e.File.Snippet + } + if e.Job != nil { + v.Job = &jsonWorkflowJob{ + Name: e.Job.Name, + ID: e.Job.ID, + } + } + r.Results.Workflows.SecretInPullRequests = append(r.Results.Workflows.SecretInPullRequests, v) + } + return nil +} + //nolint:unparam func (r *jsonScorecardRawResult) addSignedReleasesRawResults(sr *checker.SignedReleasesData) error { r.Results.Releases = []jsonRelease{} @@ -432,6 +526,11 @@ func (r *jsonScorecardRawResult) fillJSONRawResults(raw *checker.RawResults) err return sce.WithMessage(sce.ErrScorecardInternal, err.Error()) } + // Dangerous workflow. + if err := r.addDangerousWorkflowRawResults(&raw.DangerousWorkflowResults); err != nil { + return sce.WithMessage(sce.ErrScorecardInternal, err.Error()) + } + return nil }