From f422f692fe8cef93460c45d6b2b75b3a5c0f2022 Mon Sep 17 00:00:00 2001 From: AdamKorcz <44787359+AdamKorcz@users.noreply.github.com> Date: Mon, 6 Nov 2023 21:43:03 +0000 Subject: [PATCH] :seedling: Convert Dangerous Workflow check to probes (#3521) * :seedling: Convert Dangerous Workflow check to probes Signed-off-by: AdamKorcz * remove hasAnyWorkflows probe Signed-off-by: AdamKorcz * combine two conditionals into one Signed-off-by: AdamKorcz * preserve logging from original evaluation Signed-off-by: AdamKorcz * rebase Signed-off-by: AdamKorcz --------- Signed-off-by: AdamKorcz --- checks/dangerous_workflow.go | 19 +- checks/evaluation/dangerous_workflow.go | 98 ++++-- checks/evaluation/dangerous_workflow_test.go | 298 +++++++++++------- checks/raw/dangerous_workflow.go | 5 +- checks/raw/dangerous_workflow_test.go | 9 +- probes/entries.go | 6 + .../def.yml | 29 ++ .../impl.go | 84 +++++ .../impl_test.go | 97 ++++++ .../def.yml | 29 ++ .../impl.go | 92 ++++++ .../impl_test.go | 97 ++++++ 12 files changed, 712 insertions(+), 151 deletions(-) create mode 100644 probes/hasDangerousWorkflowScriptInjection/def.yml create mode 100644 probes/hasDangerousWorkflowScriptInjection/impl.go create mode 100644 probes/hasDangerousWorkflowScriptInjection/impl_test.go create mode 100644 probes/hasDangerousWorkflowUntrustedCheckout/def.yml create mode 100644 probes/hasDangerousWorkflowUntrustedCheckout/impl.go create mode 100644 probes/hasDangerousWorkflowUntrustedCheckout/impl_test.go diff --git a/checks/dangerous_workflow.go b/checks/dangerous_workflow.go index 5b12f488df4..71384bfe158 100644 --- a/checks/dangerous_workflow.go +++ b/checks/dangerous_workflow.go @@ -19,6 +19,8 @@ import ( "github.com/ossf/scorecard/v4/checks/evaluation" "github.com/ossf/scorecard/v4/checks/raw" sce "github.com/ossf/scorecard/v4/errors" + "github.com/ossf/scorecard/v4/probes" + "github.com/ossf/scorecard/v4/probes/zrunner" ) // CheckDangerousWorkflow is the exported name for Dangerous-Workflow check. @@ -38,17 +40,22 @@ func init() { // DangerousWorkflow will check the repository contains Dangerous-Workflow. func DangerousWorkflow(c *checker.CheckRequest) checker.CheckResult { - rawData, err := raw.DangerousWorkflow(c.RepoClient) + rawData, err := raw.DangerousWorkflow(c) if err != nil { e := sce.WithMessage(sce.ErrScorecardInternal, err.Error()) return checker.CreateRuntimeErrorResult(CheckDangerousWorkflow, e) } - // Return raw results. - if c.RawResults != nil { - c.RawResults.DangerousWorkflowResults = rawData + // Set the raw results. + pRawResults := getRawResults(c) + pRawResults.DangerousWorkflowResults = rawData + + // Evaluate the probes. + findings, err := zrunner.Run(pRawResults, probes.DangerousWorkflows) + if err != nil { + e := sce.WithMessage(sce.ErrScorecardInternal, err.Error()) + return checker.CreateRuntimeErrorResult(CheckDangerousWorkflow, e) } - // Return the score evaluation. - return evaluation.DangerousWorkflow(CheckDangerousWorkflow, c.Dlogger, &rawData) + return evaluation.DangerousWorkflow(CheckDangerousWorkflow, findings, c.Dlogger) } diff --git a/checks/evaluation/dangerous_workflow.go b/checks/evaluation/dangerous_workflow.go index 9aa76fe751c..9abdeebdda2 100644 --- a/checks/evaluation/dangerous_workflow.go +++ b/checks/evaluation/dangerous_workflow.go @@ -15,59 +15,89 @@ package evaluation import ( - "fmt" - "github.com/ossf/scorecard/v4/checker" sce "github.com/ossf/scorecard/v4/errors" + "github.com/ossf/scorecard/v4/finding" + "github.com/ossf/scorecard/v4/probes/hasDangerousWorkflowScriptInjection" + "github.com/ossf/scorecard/v4/probes/hasDangerousWorkflowUntrustedCheckout" ) // DangerousWorkflow applies the score policy for the DangerousWorkflow check. -func DangerousWorkflow(name string, dl checker.DetailLogger, - r *checker.DangerousWorkflowData, +func DangerousWorkflow(name string, + findings []finding.Finding, dl checker.DetailLogger, ) checker.CheckResult { - if r == nil { - e := sce.WithMessage(sce.ErrScorecardInternal, "empty raw data") + expectedProbes := []string{ + hasDangerousWorkflowScriptInjection.Probe, + hasDangerousWorkflowUntrustedCheckout.Probe, + } + + if !finding.UniqueProbesEqual(findings, expectedProbes) { + e := sce.WithMessage(sce.ErrScorecardInternal, "invalid probe results") return checker.CreateRuntimeErrorResult(name, e) } - if r.NumWorkflows == 0 { + if !hasWorkflows(findings) { return checker.CreateInconclusiveResult(name, "no workflows found") } - for _, e := range r.Workflows { - var text string - switch e.Type { - case checker.DangerousWorkflowUntrustedCheckout: - text = fmt.Sprintf("untrusted code checkout '%v'", e.File.Snippet) - case checker.DangerousWorkflowScriptInjection: - text = fmt.Sprintf("script injection with untrusted input '%v'", e.File.Snippet) - default: - err := sce.WithMessage(sce.ErrScorecardInternal, "invalid type") - return checker.CreateRuntimeErrorResult(name, err) + // Log all detected dangerous workflows + for i := range findings { + f := &findings[i] + if f.Outcome == finding.OutcomeNegative { + if f.Location == nil { + e := sce.WithMessage(sce.ErrScorecardInternal, "invalid probe results") + return checker.CreateRuntimeErrorResult(name, e) + } + dl.Warn(&checker.LogMessage{ + Path: f.Location.Path, + Type: f.Location.Type, + Offset: *f.Location.LineStart, + Text: f.Message, + Snippet: *f.Location.Snippet, + }) } + } - dl.Warn(&checker.LogMessage{ - Path: e.File.Path, - Type: e.File.Type, - Offset: e.File.Offset, - Text: text, - Snippet: e.File.Snippet, - }) + if hasDWWithUntrustedCheckout(findings) || hasDWWithScriptInjection(findings) { + return checker.CreateMinScoreResult(name, + "dangerous workflow patterns detected") } - if len(r.Workflows) > 0 { - return createResult(name, checker.MinResultScore) + return checker.CreateMaxScoreResult(name, + "no dangerous workflow patterns detected") +} + +// Both probes return OutcomeNotApplicable, if there project has no workflows. +func hasWorkflows(findings []finding.Finding) bool { + for i := range findings { + f := &findings[i] + if f.Outcome == finding.OutcomeNotApplicable { + return false + } } - return createResult(name, checker.MaxResultScore) + return true } -// Create the result. -func createResult(name string, score int) checker.CheckResult { - if score != checker.MaxResultScore { - return checker.CreateResultWithScore(name, - "dangerous workflow patterns detected", score) +func hasDWWithUntrustedCheckout(findings []finding.Finding) bool { + for i := range findings { + f := &findings[i] + if f.Probe == hasDangerousWorkflowUntrustedCheckout.Probe { + if f.Outcome == finding.OutcomeNegative { + return true + } + } } + return false +} - return checker.CreateMaxScoreResult(name, - "no dangerous workflow patterns detected") +func hasDWWithScriptInjection(findings []finding.Finding) bool { + for i := range findings { + f := &findings[i] + if f.Probe == hasDangerousWorkflowScriptInjection.Probe { + if f.Outcome == finding.OutcomeNegative { + return true + } + } + } + return false } diff --git a/checks/evaluation/dangerous_workflow_test.go b/checks/evaluation/dangerous_workflow_test.go index ae3eef25991..7a12fd7bbfc 100644 --- a/checks/evaluation/dangerous_workflow_test.go +++ b/checks/evaluation/dangerous_workflow_test.go @@ -16,151 +16,233 @@ package evaluation import ( "testing" - "github.com/google/go-cmp/cmp" - "github.com/google/go-cmp/cmp/cmpopts" - "github.com/ossf/scorecard/v4/checker" + "github.com/ossf/scorecard/v4/finding" scut "github.com/ossf/scorecard/v4/utests" ) +var ( + testSnippet = "other/checkout@a81bbbf8298c0fa03ea29cdc473d45769f953675" + testLineStart = uint(123) +) + func TestDangerousWorkflow(t *testing.T) { t.Parallel() - type args struct { //nolint:govet - name string - dl checker.DetailLogger - r *checker.DangerousWorkflowData - } tests := []struct { - name string - args args - want checker.CheckResult + name string + findings []finding.Finding + result scut.TestReturn }{ { - name: "DangerousWorkflow - empty", - args: args{ - name: "DangerousWorkflow", - dl: &scut.TestDetailLogger{}, - r: &checker.DangerousWorkflowData{}, + name: "Has untrusted checkout workflow", + findings: []finding.Finding{ + { + Probe: "hasDangerousWorkflowScriptInjection", + Outcome: finding.OutcomePositive, + }, { + Probe: "hasDangerousWorkflowUntrustedCheckout", + Outcome: finding.OutcomeNegative, + Location: &finding.Location{ + Type: finding.FileTypeText, + Path: "./github/workflows/dangerous-workflow.yml", + LineStart: &testLineStart, + Snippet: &testSnippet, + }, + }, + }, + result: scut.TestReturn{ + Score: 0, + NumberOfWarn: 1, + }, + }, + { + name: "DangerousWorkflow - no worklflows", + findings: []finding.Finding{ + { + Probe: "hasDangerousWorkflowScriptInjection", + Outcome: finding.OutcomeNotApplicable, + }, { + Probe: "hasDangerousWorkflowUntrustedCheckout", + Outcome: finding.OutcomeNotApplicable, + }, }, - want: checker.CheckResult{ - Score: checker.InconclusiveResultScore, - Reason: "no workflows found", - Version: 2, - Name: "DangerousWorkflow", + result: scut.TestReturn{ + Score: checker.InconclusiveResultScore, }, }, { name: "DangerousWorkflow - found workflows, none dangerous", - args: args{ - name: "DangerousWorkflow", - dl: &scut.TestDetailLogger{}, - r: &checker.DangerousWorkflowData{ - NumWorkflows: 5, + findings: []finding.Finding{ + { + Probe: "hasDangerousWorkflowScriptInjection", + Outcome: finding.OutcomePositive, + }, { + Probe: "hasDangerousWorkflowUntrustedCheckout", + Outcome: finding.OutcomePositive, }, }, - want: checker.CheckResult{ - Score: checker.MaxResultScore, - Reason: "no dangerous workflow patterns detected", - Version: 2, - Name: "DangerousWorkflow", + result: scut.TestReturn{ + Score: 10, }, }, { name: "DangerousWorkflow - Dangerous workflow detected", - args: args{ - name: "DangerousWorkflow", - dl: &scut.TestDetailLogger{}, - r: &checker.DangerousWorkflowData{ - NumWorkflows: 1, - Workflows: []checker.DangerousWorkflow{ - { - Type: checker.DangerousWorkflowUntrustedCheckout, - File: checker.File{ - Path: "a", - Snippet: "a", - Offset: 0, - EndOffset: 0, - Type: 0, - }, - }, + findings: []finding.Finding{ + { + Probe: "hasDangerousWorkflowScriptInjection", + Outcome: finding.OutcomePositive, + }, { + Probe: "hasDangerousWorkflowUntrustedCheckout", + Outcome: finding.OutcomeNegative, + Location: &finding.Location{ + Type: finding.FileTypeText, + Path: "./github/workflows/dangerous-workflow.yml", + LineStart: &testLineStart, + Snippet: &testSnippet, }, }, }, - want: checker.CheckResult{ - Score: 0, - Reason: "dangerous workflow patterns detected", - Version: 2, - Name: "DangerousWorkflow", + result: scut.TestReturn{ + Score: 0, + NumberOfWarn: 1, }, }, { name: "DangerousWorkflow - Script injection detected", - args: args{ - name: "DangerousWorkflow", - dl: &scut.TestDetailLogger{}, - r: &checker.DangerousWorkflowData{ - NumWorkflows: 1, - Workflows: []checker.DangerousWorkflow{ - { - Type: checker.DangerousWorkflowScriptInjection, - File: checker.File{ - Path: "a", - Snippet: "a", - Offset: 0, - EndOffset: 0, - Type: 0, - }, - }, + findings: []finding.Finding{ + { + Probe: "hasDangerousWorkflowScriptInjection", + Outcome: finding.OutcomeNegative, + Location: &finding.Location{ + Type: finding.FileTypeText, + Path: "./github/workflows/dangerous-workflow.yml", + LineStart: &testLineStart, + Snippet: &testSnippet, }, + }, { + Probe: "hasDangerousWorkflowUntrustedCheckout", + Outcome: finding.OutcomePositive, }, }, - want: checker.CheckResult{ - Score: 0, - Reason: "dangerous workflow patterns detected", - Version: 2, - Name: "DangerousWorkflow", + result: scut.TestReturn{ + Score: 0, + NumberOfWarn: 1, }, }, { - name: "DangerousWorkflow - unknown type", - args: args{ - name: "DangerousWorkflow", - dl: &scut.TestDetailLogger{}, - r: &checker.DangerousWorkflowData{ - NumWorkflows: 1, - Workflows: []checker.DangerousWorkflow{ - { - Type: "foobar", - File: checker.File{ - Path: "a", - Snippet: "a", - Offset: 0, - EndOffset: 0, - Type: 0, - }, - }, + name: "DangerousWorkflow - 3 script injection workflows detected", + findings: []finding.Finding{ + { + Probe: "hasDangerousWorkflowScriptInjection", + Outcome: finding.OutcomeNegative, + Location: &finding.Location{ + Type: finding.FileTypeText, + Path: "./github/workflows/dangerous-workflow.yml", + LineStart: &testLineStart, + Snippet: &testSnippet, }, + }, { + Probe: "hasDangerousWorkflowScriptInjection", + Outcome: finding.OutcomeNegative, + Location: &finding.Location{ + Type: finding.FileTypeText, + Path: "./github/workflows/dangerous-workflow2.yml", + LineStart: &testLineStart, + Snippet: &testSnippet, + }, + }, { + Probe: "hasDangerousWorkflowUntrustedCheckout", + Outcome: finding.OutcomePositive, }, }, - want: checker.CheckResult{ - Score: -1, - Reason: "internal error: invalid type", - Version: 2, - Name: "DangerousWorkflow", + result: scut.TestReturn{ + Score: 0, + NumberOfWarn: 2, }, }, { - name: "DangerousWorkflow - nil data", - args: args{ - name: "DangerousWorkflow", - dl: &scut.TestDetailLogger{}, - r: nil, + name: "DangerousWorkflow - 8 script injection workflows detected", + findings: []finding.Finding{ + { + Probe: "hasDangerousWorkflowScriptInjection", + Outcome: finding.OutcomeNegative, + Location: &finding.Location{ + Type: finding.FileTypeText, + Path: "./github/workflows/dangerous-workflow.yml", + LineStart: &testLineStart, + Snippet: &testSnippet, + }, + }, { + Probe: "hasDangerousWorkflowScriptInjection", + Outcome: finding.OutcomeNegative, + Location: &finding.Location{ + Type: finding.FileTypeText, + Path: "./github/workflows/dangerous-workflow2.yml", + LineStart: &testLineStart, + Snippet: &testSnippet, + }, + }, { + Probe: "hasDangerousWorkflowScriptInjection", + Outcome: finding.OutcomeNegative, + Location: &finding.Location{ + Type: finding.FileTypeText, + Path: "./github/workflows/dangerous-workflow3.yml", + LineStart: &testLineStart, + Snippet: &testSnippet, + }, + }, { + Probe: "hasDangerousWorkflowScriptInjection", + Outcome: finding.OutcomeNegative, + Location: &finding.Location{ + Type: finding.FileTypeText, + Path: "./github/workflows/dangerous-workflow4.yml", + LineStart: &testLineStart, + Snippet: &testSnippet, + }, + }, { + Probe: "hasDangerousWorkflowScriptInjection", + Outcome: finding.OutcomeNegative, + Location: &finding.Location{ + Type: finding.FileTypeText, + Path: "./github/workflows/dangerous-workflow5.yml", + LineStart: &testLineStart, + Snippet: &testSnippet, + }, + }, { + Probe: "hasDangerousWorkflowScriptInjection", + Outcome: finding.OutcomeNegative, + Location: &finding.Location{ + Type: finding.FileTypeText, + Path: "./github/workflows/dangerous-workflow6.yml", + LineStart: &testLineStart, + Snippet: &testSnippet, + }, + }, { + Probe: "hasDangerousWorkflowScriptInjection", + Outcome: finding.OutcomeNegative, + Location: &finding.Location{ + Type: finding.FileTypeText, + Path: "./github/workflows/dangerous-workflow7.yml", + LineStart: &testLineStart, + Snippet: &testSnippet, + }, + }, { + Probe: "hasDangerousWorkflowScriptInjection", + Outcome: finding.OutcomeNegative, + Location: &finding.Location{ + Type: finding.FileTypeText, + Path: "./github/workflows/dangerous-workflow8.yml", + LineStart: &testLineStart, + Snippet: &testSnippet, + }, + }, { + Probe: "hasDangerousWorkflowUntrustedCheckout", + Outcome: finding.OutcomePositive, + }, }, - want: checker.CheckResult{ - Score: -1, - Reason: "internal error: empty raw data", - Name: "DangerousWorkflow", - Version: 2, + result: scut.TestReturn{ + Score: 0, + NumberOfWarn: 8, }, }, } @@ -168,8 +250,10 @@ func TestDangerousWorkflow(t *testing.T) { tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() - if got := DangerousWorkflow(tt.args.name, tt.args.dl, tt.args.r); !cmp.Equal(got, tt.want, cmpopts.IgnoreFields(checker.CheckResult{}, "Error")) { //nolint:lll - t.Errorf("DangerousWorkflow() = %v, want %v", got, cmp.Diff(got, tt.want, cmpopts.IgnoreFields(checker.CheckResult{}, "Error"))) //nolint:lll + dl := scut.TestDetailLogger{} + got := DangerousWorkflow(tt.name, tt.findings, &dl) + if !scut.ValidateTestReturn(t, tt.name, &tt.result, &got, &dl) { + t.Errorf("got %v, expected %v", got, tt.result) } }) } diff --git a/checks/raw/dangerous_workflow.go b/checks/raw/dangerous_workflow.go index 1da80f0e2eb..4ab79c64d47 100644 --- a/checks/raw/dangerous_workflow.go +++ b/checks/raw/dangerous_workflow.go @@ -23,7 +23,6 @@ import ( "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" "github.com/ossf/scorecard/v4/finding" ) @@ -66,10 +65,10 @@ var ( ) // DangerousWorkflow retrieves the raw data for the DangerousWorkflow check. -func DangerousWorkflow(c clients.RepoClient) (checker.DangerousWorkflowData, error) { +func DangerousWorkflow(c *checker.CheckRequest) (checker.DangerousWorkflowData, error) { // data is shared across all GitHub workflows. var data checker.DangerousWorkflowData - err := fileparser.OnMatchingFileContentDo(c, fileparser.PathMatcher{ + err := fileparser.OnMatchingFileContentDo(c.RepoClient, fileparser.PathMatcher{ Pattern: ".github/workflows/*", CaseSensitive: false, }, validateGitHubActionWorkflowPatterns, &data) diff --git a/checks/raw/dangerous_workflow_test.go b/checks/raw/dangerous_workflow_test.go index 3e0e43a65ce..787f37f3311 100644 --- a/checks/raw/dangerous_workflow_test.go +++ b/checks/raw/dangerous_workflow_test.go @@ -15,6 +15,7 @@ package raw import ( + "context" "errors" "fmt" "os" @@ -24,6 +25,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" + "github.com/ossf/scorecard/v4/checker" mockrepo "github.com/ossf/scorecard/v4/clients/mockclients" ) @@ -166,7 +168,12 @@ func TestGithubDangerousWorkflow(t *testing.T) { return content, nil }) - dw, err := DangerousWorkflow(mockRepoClient) + req := &checker.CheckRequest{ + Ctx: context.Background(), + RepoClient: mockRepoClient, + } + + dw, err := DangerousWorkflow(req) if !errCmp(err, tt.expected.err) { t.Errorf(cmp.Diff(err, tt.expected.err, cmpopts.EquateErrors())) diff --git a/probes/entries.go b/probes/entries.go index 59bed16ebbd..671877d018e 100644 --- a/probes/entries.go +++ b/probes/entries.go @@ -31,6 +31,8 @@ import ( "github.com/ossf/scorecard/v4/probes/fuzzedWithPythonAtheris" "github.com/ossf/scorecard/v4/probes/fuzzedWithRustCargofuzz" "github.com/ossf/scorecard/v4/probes/fuzzedWithSwiftLibFuzzer" + "github.com/ossf/scorecard/v4/probes/hasDangerousWorkflowScriptInjection" + "github.com/ossf/scorecard/v4/probes/hasDangerousWorkflowUntrustedCheckout" "github.com/ossf/scorecard/v4/probes/hasFSFOrOSIApprovedLicense" "github.com/ossf/scorecard/v4/probes/hasLicenseFile" "github.com/ossf/scorecard/v4/probes/hasLicenseFileAtTopDir" @@ -95,6 +97,10 @@ var ( Vulnerabilities = []ProbeImpl{ hasOSVVulnerabilities.Run, } + DangerousWorkflows = []ProbeImpl{ + hasDangerousWorkflowScriptInjection.Run, + hasDangerousWorkflowUntrustedCheckout.Run, + } ) //nolint:gochecknoinits diff --git a/probes/hasDangerousWorkflowScriptInjection/def.yml b/probes/hasDangerousWorkflowScriptInjection/def.yml new file mode 100644 index 00000000000..f2c30529506 --- /dev/null +++ b/probes/hasDangerousWorkflowScriptInjection/def.yml @@ -0,0 +1,29 @@ +# Copyright 2023 OpenSSF 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. + +id: hasDangerousWorkflowScriptInjection +short: Check whether the project has Github Actions workflows that enable script injection. +motivation: > + Script Injection with Untrusted Context Variables: This pattern detects whether a workflow's inline script may execute untrusted input from attackers. This occurs when an attacker adds malicious commands and scripts to a context. When a workflow runs, these strings may be interpreted as code that is executed on the runner. Attackers can add their own content to certain github context variables that are considered untrusted, for example, github.event.issue.title. These values should not flow directly into executable code. +implementation: > + The probe iterates through the workflows from the raw results and checks the workflow type. If it finds a workflow of the type `DangerousWorkflowScriptInjection`, it returns. +outcome: + - If the project has at least one workflow with possibility of script injection, the probe returns one finding with OutcomeNegative (0). + - If the project does not have a single workflow with possibility of script injection, the probe returns one finding with OutcomePositive (1). +remediation: + effort: Low + text: + - Avoid the dangerous workflow patterns. + markdown: + - Avoid the dangerous workflow patterns. See [this post](https://securitylab.github.com/research/github-actions-preventing-pwn-requests/) for information on avoiding untrusted code checkouts. See [this document](https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#understanding-the-risk-of-script-injections) for information on avoiding and mitigating the risk of script injections. \ No newline at end of file diff --git a/probes/hasDangerousWorkflowScriptInjection/impl.go b/probes/hasDangerousWorkflowScriptInjection/impl.go new file mode 100644 index 00000000000..e3b91f95988 --- /dev/null +++ b/probes/hasDangerousWorkflowScriptInjection/impl.go @@ -0,0 +1,84 @@ +// Copyright 2023 OpenSSF 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. + +// nolint:stylecheck +package hasDangerousWorkflowScriptInjection + +import ( + "embed" + "fmt" + + "github.com/ossf/scorecard/v4/checker" + "github.com/ossf/scorecard/v4/finding" + "github.com/ossf/scorecard/v4/probes/internal/utils/uerror" +) + +//go:embed *.yml +var fs embed.FS + +const Probe = "hasDangerousWorkflowScriptInjection" + +func Run(raw *checker.RawResults) ([]finding.Finding, string, error) { + if raw == nil { + return nil, "", fmt.Errorf("%w: raw", uerror.ErrNil) + } + + r := raw.DangerousWorkflowResults + + if r.NumWorkflows == 0 { + f, err := finding.NewWith(fs, Probe, + "Project does not have any workflows.", nil, + finding.OutcomeNotApplicable) + if err != nil { + return nil, Probe, fmt.Errorf("create finding: %w", err) + } + return []finding.Finding{*f}, Probe, nil + } + + var findings []finding.Finding + for _, e := range r.Workflows { + e := e + if e.Type == checker.DangerousWorkflowScriptInjection { + f, err := finding.NewWith(fs, Probe, + fmt.Sprintf("script injection with untrusted input '%v'", e.File.Snippet), + nil, finding.OutcomeNegative) + if err != nil { + return nil, Probe, fmt.Errorf("create finding: %w", err) + } + f = f.WithLocation(&finding.Location{ + Path: e.File.Path, + Type: e.File.Type, + LineStart: &e.File.Offset, + Snippet: &e.File.Snippet, + }) + findings = append(findings, *f) + } + } + + if len(findings) == 0 { + return positiveOutcome() + } + + return findings, Probe, nil +} + +func positiveOutcome() ([]finding.Finding, string, error) { + f, err := finding.NewWith(fs, Probe, + "Project does not have dangerous workflow(s) with possibility of script injection.", nil, + finding.OutcomePositive) + if err != nil { + return nil, Probe, fmt.Errorf("create finding: %w", err) + } + return []finding.Finding{*f}, Probe, nil +} diff --git a/probes/hasDangerousWorkflowScriptInjection/impl_test.go b/probes/hasDangerousWorkflowScriptInjection/impl_test.go new file mode 100644 index 00000000000..9b35a4b7708 --- /dev/null +++ b/probes/hasDangerousWorkflowScriptInjection/impl_test.go @@ -0,0 +1,97 @@ +// Copyright 2023 OpenSSF 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. + +// nolint:stylecheck +package hasDangerousWorkflowScriptInjection + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + + "github.com/ossf/scorecard/v4/checker" + "github.com/ossf/scorecard/v4/finding" +) + +func Test_Run(t *testing.T) { + t.Parallel() + // nolint:govet + tests := []struct { + name string + raw *checker.RawResults + outcomes []finding.Outcome + err error + }{ + { + name: "Three workflows one of which has possibility of script injection.", + raw: &checker.RawResults{ + DangerousWorkflowResults: checker.DangerousWorkflowData{ + NumWorkflows: 3, + Workflows: []checker.DangerousWorkflow{ + { + Type: checker.DangerousWorkflowScriptInjection, + }, + }, + }, + }, + outcomes: []finding.Outcome{ + finding.OutcomeNegative, + }, + }, + { + name: "Three workflows none of which have possibility of script injection.", + raw: &checker.RawResults{ + DangerousWorkflowResults: checker.DangerousWorkflowData{ + NumWorkflows: 3, + Workflows: []checker.DangerousWorkflow{ + { + Type: checker.DangerousWorkflowUntrustedCheckout, + }, + }, + }, + }, + outcomes: []finding.Outcome{ + finding.OutcomePositive, + }, + }, + } + 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() + + findings, s, err := Run(tt.raw) + if !cmp.Equal(tt.err, err, cmpopts.EquateErrors()) { + t.Errorf("mismatch (-want +got):\n%s", cmp.Diff(tt.err, err, cmpopts.EquateErrors())) + } + if err != nil { + return + } + if diff := cmp.Diff(Probe, s); diff != "" { + t.Errorf("mismatch (-want +got):\n%s", diff) + } + if diff := cmp.Diff(len(tt.outcomes), len(findings)); diff != "" { + t.Errorf("mismatch (-want +got):\n%s", diff) + } + for i := range tt.outcomes { + outcome := &tt.outcomes[i] + f := &findings[i] + if diff := cmp.Diff(*outcome, f.Outcome); diff != "" { + t.Errorf("mismatch (-want +got):\n%s", diff) + } + } + }) + } +} diff --git a/probes/hasDangerousWorkflowUntrustedCheckout/def.yml b/probes/hasDangerousWorkflowUntrustedCheckout/def.yml new file mode 100644 index 00000000000..0a6e9c0cddf --- /dev/null +++ b/probes/hasDangerousWorkflowUntrustedCheckout/def.yml @@ -0,0 +1,29 @@ +# Copyright 2023 OpenSSF 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. + +id: hasDangerousWorkflowUntrustedCheckout +short: Check whether the project has Github Actions workflows that does untrusted checkouts. +motivation: > + Untrusted Code Checkout: This is the misuse of potentially dangerous triggers. This checks if a pull_request_target or workflow_run workflow trigger was used in conjunction with an explicit pull request checkout. Workflows triggered with pull_request_target / workflow_run have write permission to the target repository and access to target repository secrets. With the PR checkout, PR authors may compromise the repository, for example, by using build scripts controlled by the author of the PR or reading token in memory. This check does not detect whether untrusted code checkouts are used safely, for example, only on pull request that have been assigned a label. +implementation: > + The probe iterates through the workflows from the raw results and checks the workflow type. If it finds a workflow of the type `DangerousWorkflowUntrustedCheckout`, it returns. +outcome: + - If the project has at least one workflow with possibility of untrusted code checkout, the probe returns one finding with OutcomeNegative (0). + - If the project does not have a single workflow with possibility of untrusted code checkout, the probe returns one finding with OutcomePositive (1). +remediation: + effort: Low + text: + - Avoid the dangerous workflow patterns. + markdown: + - Avoid the dangerous workflow patterns. See [this post](https://securitylab.github.com/research/github-actions-preventing-pwn-requests/) for information on avoiding untrusted code checkouts. See [this document](https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#understanding-the-risk-of-script-injections) for information on avoiding and mitigating the risk of script injections. \ No newline at end of file diff --git a/probes/hasDangerousWorkflowUntrustedCheckout/impl.go b/probes/hasDangerousWorkflowUntrustedCheckout/impl.go new file mode 100644 index 00000000000..d3bd59f5269 --- /dev/null +++ b/probes/hasDangerousWorkflowUntrustedCheckout/impl.go @@ -0,0 +1,92 @@ +// Copyright 2023 OpenSSF 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. + +// nolint:stylecheck +package hasDangerousWorkflowUntrustedCheckout + +import ( + "embed" + "fmt" + + "github.com/ossf/scorecard/v4/checker" + "github.com/ossf/scorecard/v4/finding" + "github.com/ossf/scorecard/v4/probes/internal/utils/uerror" +) + +//go:embed *.yml +var fs embed.FS + +const Probe = "hasDangerousWorkflowUntrustedCheckout" + +func Run(raw *checker.RawResults) ([]finding.Finding, string, error) { + if raw == nil { + return nil, "", fmt.Errorf("%w: raw", uerror.ErrNil) + } + + r := raw.DangerousWorkflowResults + + if r.NumWorkflows == 0 { + f, err := finding.NewWith(fs, Probe, + "Project does not have any workflows.", nil, + finding.OutcomeNotApplicable) + if err != nil { + return nil, Probe, fmt.Errorf("create finding: %w", err) + } + return []finding.Finding{*f}, Probe, nil + } + + var findings []finding.Finding + for _, e := range r.Workflows { + e := e + if e.Type == checker.DangerousWorkflowUntrustedCheckout { + f, err := finding.NewWith(fs, Probe, + fmt.Sprintf("untrusted code checkout '%v'", e.File.Snippet), + nil, finding.OutcomeNegative) + if err != nil { + return nil, Probe, fmt.Errorf("create finding: %w", err) + } + f = f.WithLocation(&finding.Location{ + Path: e.File.Path, + Type: e.File.Type, + LineStart: &e.File.Offset, + Snippet: &e.File.Snippet, + }) + findings = append(findings, *f) + } + } + if len(findings) == 0 { + return positiveOutcome() + } + return findings, Probe, nil +} + +func positiveOutcome() ([]finding.Finding, string, error) { + f, err := finding.NewWith(fs, Probe, + "Project does not have workflow(s) with untrusted checkout.", nil, + finding.OutcomePositive) + if err != nil { + return nil, Probe, fmt.Errorf("create finding: %w", err) + } + return []finding.Finding{*f}, Probe, nil +} + +/*func negativeOutcome() ([]finding.Finding, string, error) { + f, err := finding.NewWith(fs, Probe, + "Project has workflow(s) with untrusted checkout.", nil, + finding.OutcomeNegative) + if err != nil { + return nil, Probe, fmt.Errorf("create finding: %w", err) + } + return []finding.Finding{*f}, Probe, nil +}*/ diff --git a/probes/hasDangerousWorkflowUntrustedCheckout/impl_test.go b/probes/hasDangerousWorkflowUntrustedCheckout/impl_test.go new file mode 100644 index 00000000000..b3aed7b7162 --- /dev/null +++ b/probes/hasDangerousWorkflowUntrustedCheckout/impl_test.go @@ -0,0 +1,97 @@ +// Copyright 2023 OpenSSF 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. + +// nolint:stylecheck +package hasDangerousWorkflowUntrustedCheckout + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + + "github.com/ossf/scorecard/v4/checker" + "github.com/ossf/scorecard/v4/finding" +) + +func Test_Run(t *testing.T) { + t.Parallel() + // nolint:govet + tests := []struct { + name string + raw *checker.RawResults + outcomes []finding.Outcome + err error + }{ + { + name: "Three workflows none of which do untrusted checkout.", + raw: &checker.RawResults{ + DangerousWorkflowResults: checker.DangerousWorkflowData{ + NumWorkflows: 3, + Workflows: []checker.DangerousWorkflow{ + { + Type: checker.DangerousWorkflowScriptInjection, + }, + }, + }, + }, + outcomes: []finding.Outcome{ + finding.OutcomePositive, + }, + }, + { + name: "Three workflows one of which has possibility of untrusted checkout.", + raw: &checker.RawResults{ + DangerousWorkflowResults: checker.DangerousWorkflowData{ + NumWorkflows: 3, + Workflows: []checker.DangerousWorkflow{ + { + Type: checker.DangerousWorkflowUntrustedCheckout, + }, + }, + }, + }, + outcomes: []finding.Outcome{ + finding.OutcomeNegative, + }, + }, + } + 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() + + findings, s, err := Run(tt.raw) + if !cmp.Equal(tt.err, err, cmpopts.EquateErrors()) { + t.Errorf("mismatch (-want +got):\n%s", cmp.Diff(tt.err, err, cmpopts.EquateErrors())) + } + if err != nil { + return + } + if diff := cmp.Diff(Probe, s); diff != "" { + t.Errorf("mismatch (-want +got):\n%s", diff) + } + if diff := cmp.Diff(len(tt.outcomes), len(findings)); diff != "" { + t.Errorf("mismatch (-want +got):\n%s", diff) + } + for i := range tt.outcomes { + outcome := &tt.outcomes[i] + f := &findings[i] + if diff := cmp.Diff(*outcome, f.Outcome); diff != "" { + t.Errorf("mismatch (-want +got):\n%s", diff) + } + } + }) + } +}