diff --git a/checks/dangerous_workflow.go b/checks/dangerous_workflow.go index 5b12f488..71384bfe 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 9aa76fe7..9abdeebd 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 len(r.Workflows) > 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) + if hasDWWithUntrustedCheckout(findings) || hasDWWithScriptInjection(findings) { + return checker.CreateMinScoreResult(name, + "dangerous workflow patterns detected") } 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 true +} + +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 +} + +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 ae3eef25..7a12fd7b 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, + }, + }, }, - want: checker.CheckResult{ - Score: checker.InconclusiveResultScore, - Reason: "no workflows found", - Version: 2, - Name: "DangerousWorkflow", + result: scut.TestReturn{ + Score: 0, + NumberOfWarn: 1, + }, + }, + { + name: "DangerousWorkflow - no worklflows", + findings: []finding.Finding{ + { + Probe: "hasDangerousWorkflowScriptInjection", + Outcome: finding.OutcomeNotApplicable, + }, { + Probe: "hasDangerousWorkflowUntrustedCheckout", + Outcome: finding.OutcomeNotApplicable, + }, + }, + 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 1da80f0e..4ab79c64 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 3e0e43a6..787f37f3 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 59bed16e..671877d0 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 00000000..f2c30529 --- /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 00000000..e3b91f95 --- /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 00000000..9b35a4b7 --- /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 00000000..0a6e9c0c --- /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 00000000..d3bd59f5 --- /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 00000000..b3aed7b7 --- /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) + } + } + }) + } +}