✨ Add machine-readable patch to fix script injections in workflows (#…

…4218) * Merge pull request #1 from joycebrum/feature/setup-environment-for-dw-fix create environment for patch on DW script injections Signed-off-by: Diogo Teles Sant'Anna <[email protected]> Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * Merge pull request #3 from joycebrum/feat/connect-patch-generator-with-remediation-output Include the generated patch in the output Signed-off-by: Joyce Brum <[email protected]> Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * Merge pull request #2 from joycebrum/test/initial-tests-for-dw-fix Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * Merge pull request #4 from joycebrum/feat/get-input-needed-to-generate-patch Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * impl.go: slight refactor to loop Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * Add envvars to existing or new env, still not replaced in `run` Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * Replace unsafe variables in run commands, generate git diff Git diff created using hexops/gotextdiff, WHICH IS ARCHIVED. It is unfortunately the only package I found which could do it. To be discussed with Scorecard maintainers whether it's worth it. Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * Rewrite test file - Test patchWorkflow instead of GeneratePatch. This avoids the complication of comparing diff files; we can instead simply compare the output workflow to an expected "fixed" workflow. - Examples with multiple findings must have separate "fixed" workflows for each finding, not a single file which covers all findings - Instead of hard-coding the finding details (snippet, line position), run raw.DangerousWorkflow() to get that data automatically. This does make these tests a bit more "integration-test-like", but makes them substantially easier to maintain. Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * Rewrite patch/impl.go - misc refactors - use go-git to generate diff - Most functions now return errors instead of bools. This can be later used for simpler logging - Existing environment variables are now detected by parsing the files as GH workflows. This is WIP to handle existing envvars in our patches. - Remove instances of C-style for-loops, unnecessarily dangerous! - Fixed proper detection of existing env, handling blank lines and comments. Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * Update test workflows - Fix inconsistencies between original and "fixed" versions - Store multiple "fixed" workflows for tests with multiple findings. Each "fixed" workflow fixes a single finding. The files are numbered according to the order in which the findings are found by moving down the file. - allKindsOfUserInput removed. Would require too many "fixed" workflows to test. The behavior can be tested more directly. Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * Use existing envvars, validate patched workflow - If an envvar with our name and value already existed but simply wasn't used, the patch no longer duplicates it. - After the patched workflow is created, we validate that it is valid. Or, at least did not introduce any syntax errors that were not present in the original workflow. Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * Test for same injection in same step, leading to duplicate findings Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * Use existing envvars with different name but same meaning Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * Avoid conflicts with irrelevant but existing envvars Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * Use first job's indent to define envvar indent Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * Refactor patch/impl_test - Create helper function `readWorkflow` - Improved error handling in case of failed workflow validation - Allow the declaration of duplicate findings (cases where 2+ findings have the same patch) Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * patch/impl: Simplify unsafePatterns, use errors, docs, lint - Simplify use of unsafePatterns - Replaced boolean returns with errors, for easier log/debugging - Improved documentation - Changes to satisfy linter, adoption of 120-char line limit Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * Fix panic in hasScriptInjection test due to missing file Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * Avoid duplicate envvars dealing with array variables Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * Adopt existing inter-block spacing for new env Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * chore: Tidy up function order, remove unused files Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * Define localPath in runScorecard Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * Assert valid offset, use TrimSpace, drop unused struct member Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * Just use []bytes instead of string Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * Use []byte, not string Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * go mod tidy updates Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * Ensure valid offset Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * Move /patch to /internal/patch Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * Document patch behavior and add patch to remediation in def.yml Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * Updates from review Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> * Add patch to finding before adding to list of findings Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> --------- Signed-off-by: Diogo Teles Sant'Anna <[email protected]> Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]> Signed-off-by: Joyce Brum <[email protected]> Co-authored-by: Diogo Teles Sant'Anna <[email protected]> Co-authored-by: Joyce Brum <[email protected]>
ossf · Nov 10, 2024 · cf30f20 · cf30f20
1 parent 965d15b
commit cf30f20
Show file tree

Hide file tree

Showing 49 changed files with 2,474 additions and 19 deletions.
diff --git a/cmd/internal/scdiff/app/runner/runner_test.go b/cmd/internal/scdiff/app/runner/runner_test.go
@@ -46,6 +46,7 @@ func TestRunner_Run(t *testing.T) {
 	mockRepo.EXPECT().GetDefaultBranchName().Return("main", nil)
 	mockRepo.EXPECT().Close().Return(nil)
 	mockRepo.EXPECT().GetFileReader(gomock.Any()).Return(nil, errors.New("reading files unsupported for this test")).AnyTimes()
+	mockRepo.EXPECT().LocalPath().Return(".", nil)
 	r := Runner{
 		ctx: context.Background(),
 		// use a check which works locally, but we declare no files above so no-op

diff --git a/docs/probes.md b/docs/probes.md
@@ -194,7 +194,7 @@ If the probe finds no binary files, it returns a single OutcomeFalse.
 
 **Implementation**: The probe analyzes the repository's workflows for known dangerous patterns.
 
-**Outcomes**: The probe returns one finding with OutcomeTrue for each dangerous script injection pattern detected.
+**Outcomes**: The probe returns one finding with OutcomeTrue for each dangerous script injection pattern detected. Each finding may include a suggested patch to fix the respective script injection.
 If no dangerous patterns are found, the probe returns one finding with OutcomeFalse.
 
 

diff --git a/go.mod b/go.mod
@@ -150,7 +150,7 @@ require (
 	github.com/emirpasic/gods v1.18.1 // indirect
 	github.com/fatih/color v1.17.0 // indirect
 	github.com/go-git/gcfg v1.5.1-0.20230307220236-3a3c6141e376 // indirect
-	github.com/go-git/go-billy/v5 v5.5.0 // indirect
+	github.com/go-git/go-billy/v5 v5.5.0
 	github.com/gogo/protobuf v1.3.2 // indirect
 	github.com/golang-jwt/jwt/v4 v4.5.1 // indirect
 	github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect

diff --git a/pkg/scorecard/scorecard.go b/pkg/scorecard/scorecard.go
@@ -138,6 +138,11 @@ func runScorecard(ctx context.Context,
 
 	resultsCh := make(chan checker.CheckResult)
 
+	localPath, err := repoClient.LocalPath()
+	if err != nil {
+		return Result{}, fmt.Errorf("RepoClient.LocalPath: %w", err)
+	}
+
 	// Set metadata for all checks to use. This is necessary
 	// to create remediations from the probe yaml files.
 	ret.RawResults.Metadata.Metadata = map[string]string{
@@ -146,6 +151,7 @@ func runScorecard(ctx context.Context,
 		"repository.uri":           repo.URI(),
 		"repository.sha1":          commitSHA,
 		"repository.defaultBranch": defaultBranch,
+		"localPath":                localPath,
 	}
 
 	request := &checker.CheckRequest{

diff --git a/pkg/scorecard/scorecard_test.go b/pkg/scorecard/scorecard_test.go
@@ -242,6 +242,7 @@ func TestRun_WithProbes(t *testing.T) {
 							"repository.name":          "ossf/scorecard",
 							"repository.sha1":          "1a17bb812fb2ac23e9d09e86e122f8b67563aed7",
 							"repository.uri":           "github.com/ossf/scorecard",
+							"localPath":                "test_path",
 						},
 					},
 				},
@@ -279,6 +280,9 @@ func TestRun_WithProbes(t *testing.T) {
 			t.Parallel()
 			ctrl := gomock.NewController(t)
 			mockRepoClient := mockrepo.NewMockRepoClient(ctrl)
+			mockRepoClient.EXPECT().LocalPath().DoAndReturn(func() (string, error) {
+				return "test_path", nil
+			}).AnyTimes()
 			repo := mockrepo.NewMockRepo(ctrl)
 
 			repo.EXPECT().URI().Return(tt.args.uri).AnyTimes()

diff --git a/probes/hasDangerousWorkflowScriptInjection/def.yml b/probes/hasDangerousWorkflowScriptInjection/def.yml
@@ -20,7 +20,7 @@ motivation: >
 implementation: >
   The probe analyzes the repository's workflows for known dangerous patterns.
 outcome:
-  - The probe returns one finding with OutcomeTrue for each dangerous script injection pattern detected.
+  - The probe returns one finding with OutcomeTrue for each dangerous script injection pattern detected. Each finding may include a suggested patch to fix the respective script injection.
   - If no dangerous patterns are found, the probe returns one finding with OutcomeFalse.
 remediation:
   onOutcome: True
@@ -30,6 +30,11 @@ remediation:
   markdown:
     - Avoid the dangerous workflow patterns.
     - 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.
+    - |
+      Here is a proposed patch to eliminate this risk:
+      ```yml
+      ${{ metadata.patch }}
+      ```
 ecosystem:
   languages:
     - all

diff --git a/probes/hasDangerousWorkflowScriptInjection/impl.go b/probes/hasDangerousWorkflowScriptInjection/impl.go
@@ -18,11 +18,16 @@ package hasDangerousWorkflowScriptInjection
 import (
 	"embed"
 	"fmt"
+	"os"
+	"path"
+
+	"github.com/rhysd/actionlint"
 
 	"github.com/ossf/scorecard/v5/checker"
 	"github.com/ossf/scorecard/v5/finding"
 	"github.com/ossf/scorecard/v5/internal/checknames"
 	"github.com/ossf/scorecard/v5/internal/probes"
+	"github.com/ossf/scorecard/v5/probes/hasDangerousWorkflowScriptInjection/internal/patch"
 	"github.com/ossf/scorecard/v5/probes/internal/utils/uerror"
 )
 
@@ -53,23 +58,37 @@ func Run(raw *checker.RawResults) ([]finding.Finding, string, error) {
 	}
 
 	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.OutcomeTrue)
-			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)
+	var currWorkflow string
+	var workflow *actionlint.Workflow
+	var content []byte
+	var errs []*actionlint.Error
+	localPath := raw.Metadata.Metadata["localPath"]
+	for _, w := range r.Workflows {
+		w := w
+		if w.Type != checker.DangerousWorkflowScriptInjection {
+			continue
+		}
+
+		f, err := finding.NewWith(fs, Probe,
+			fmt.Sprintf("script injection with untrusted input '%v'", w.File.Snippet),
+			nil, finding.OutcomeTrue)
+		if err != nil {
+			return nil, Probe, fmt.Errorf("create finding: %w", err)
 		}
+
+		f = f.WithLocation(&finding.Location{
+			Path:      w.File.Path,
+			Type:      w.File.Type,
+			LineStart: &w.File.Offset,
+			Snippet:   &w.File.Snippet,
+		})
+
+		err = parseWorkflow(localPath, &w, &currWorkflow, &content, &workflow, &errs)
+		if err == nil {
+			generatePatch(&w, content, workflow, errs, f)
+		}
+
+		findings = append(findings, *f)
 	}
 
 	if len(findings) == 0 {
@@ -79,6 +98,50 @@ func Run(raw *checker.RawResults) ([]finding.Finding, string, error) {
 	return findings, Probe, nil
 }
 
+func parseWorkflow(
+	localPath string,
+	e *checker.DangerousWorkflow,
+	currWorkflow *string,
+	content *[]byte,
+	workflow **actionlint.Workflow,
+	errs *[]*actionlint.Error,
+) error {
+	var err error
+	wp := path.Join(localPath, e.File.Path)
+	if *currWorkflow != wp {
+		// update current open file if injection in different file
+		*currWorkflow = wp
+		*content, err = os.ReadFile(wp)
+		if err != nil {
+			return err //nolint:wrapcheck // we only care about the error's existence
+		}
+
+		*workflow, *errs = actionlint.Parse(*content)
+		if len(*errs) > 0 && *workflow == nil {
+			// the workflow contains unrecoverable parsing errors, skip.
+			return err //nolint:wrapcheck // we only care about the error's existence
+		}
+	}
+	return nil
+}
+
+func generatePatch(
+	e *checker.DangerousWorkflow,
+	content []byte,
+	workflow *actionlint.Workflow,
+	errs []*actionlint.Error,
+	f *finding.Finding,
+) {
+	findingPatch, err := patch.GeneratePatch(e.File, content, workflow, errs)
+	if err != nil {
+		return
+	}
+	f.WithPatch(&findingPatch)
+	f.WithRemediationMetadata(map[string]string{
+		"patch": findingPatch,
+	})
+}
+
 func falseOutcome() ([]finding.Finding, string, error) {
 	f, err := finding.NewWith(fs, Probe,
 		"Project does not have dangerous workflow(s) with possibility of script injection.", nil,

diff --git a/probes/hasDangerousWorkflowScriptInjection/impl_test.go b/probes/hasDangerousWorkflowScriptInjection/impl_test.go
@@ -43,6 +43,9 @@ func Test_Run(t *testing.T) {
 					Workflows: []checker.DangerousWorkflow{
 						{
 							Type: checker.DangerousWorkflowScriptInjection,
+							File: checker.File{
+								Path: "patch/testdata/userInputAssignedToVariable.yaml",
+							},
 						},
 					},
 				},