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

[pnacht]

What kind of change does this PR introduce?

• PR title follows the guidelines defined in our pull request documentation

What is the current behavior?

Findings have a .Remediation.Patch field which is meant to contain a machine-readable patch fixing that particular finding:

scorecard/finding/probe.go

Lines 50 to 52 in 3155309

	type Remediation struct {
	// Patch for machines.
	Patch *string `json:"patch,omitempty"`

This field currently isn't used by any Scorecard probe.

What is the new behavior (if this is a feature change)?**

This PR adds a machine-readable patch (following the "unified diff" format which users can then apply using patch or git apply) fixing all hasDangerousWorkflowScriptInjection findings.

Each finding is patched by creating a global environment variable that wraps the dangerous GitHub variable and replacing that GitHub variable by the envvar in the relevant run command. That is:

+ env:
+   ISSUE_BODY: ${{ github.event.issue.body}}
+
 jobs:
   foo:
     steps:
-      - run: echo "${{ github.event.issue.body}}"
+      - run: echo "$ISSUE_BODY"

Or, on a real example:

go run main.go \
  --repo pnacht/cronk \
  --commit 6619fad1e79493034363b8865ab5dcbf5442a76c \
  --probes hasDangerousWorkflowScriptInjection \
  --format probe | jq .

{
  "...":,
  "findings": [
    {
      "...": "...",
      "remediation": {
        "patch": "<pretty-printed below>",
        "...": "..."
      },
      "probe": "hasDangerousWorkflowScriptInjection",
    }
  ]
}

Where the patch is:

diff --git a/.github/workflows/awesome-action.yml b/.github/workflows/awesome-action.yml
index 5eeb62ef6d3e94fc63676e9f9557a9389d05a99c..d234fa3415691e8f3c4f40183770fcea755f8d2c 100644
--- a/.github/workflows/awesome-action.yml
+++ b/.github/workflows/awesome-action.yml
@@ -4,6 +4,9 @@   push:
     branches: ["main"]
   pull_request:
 
+env:
+  PR_BODY: ${{ github.event.pull_request.body }}
+
 jobs:
   this_is_safe:
     runs-on: ubuntu-latest
@@ -14,7 +17,7 @@         uses: actions/checkout@v3
 
       - name: "Print PR title"
         if: github.event_name == 'pull_request'
-        run: echo "${{ github.event.pull_request.body }}"
+        run: echo "$PR_BODY"
 
       - name: "Do something awesome"
         uses: super-safe/nothing-to-see-here-action@v1.2.3

The patched workflow is then validated by parsing it with actionlint. As long as the patch added no new parsing errors, it is accepted.

• Tests for the changes have been added (for bug fixes/features)

Which issue(s) this PR fixes

Fixes #3950

Special notes for your reviewer

• This feature requires access to the original workflow files, so we must know the path to the tempdir where the downloaded repository is stored. This is done in pkg/scorecard_result.populateRawResults.
• This feature requires a significant amount of "custom parsing". We can't simply patch the workflow struct created by actionlint.Parse() because it loses style information (i.e. whitespace, order of elements, etc). We must therefore parse the workflow ourselves to ensure we make minimal patches that follow the original file's style as best we can. However, the logic does use the actionlint.Workflow when possible (which is only to read any existing environment variables).
• Regarding the global environment:
  • If the workflow already contains a global environment, it is used.
    • The new envvar adopts the same indentation as the existing envvars.
    • If an existing global envvar already covers the dangerous variable, we use it instead of creating a new envvar with the same value
    • If an existing global envvar has the same name as the one we'd create, but a different value, we append an arbitrary hard-coded string to avoid conflicts. Envvars at lower scopes (job- and step-level) are not considered, which may lead to (likely very rare) conflicts.
  • If a new env must be created:
    • it is created right above the jobs: label
    • In an attempt to keep the workflow's style:
      • the indentation step used for the new envvar will be copied from the indentation step used for the individual job labels.
      • blanks lines will be inserted between the envvar definition and the jobs: label. The number of lines matches the number between the jobs: label and the end of the preceding block in the original workflow.
• In case of errors at any point in the process, the patch is simply left blank, without interrupting Scorecard's ordinary flow. I was unsure how to log these errors (is it just a matter of creating a new logger with sclog.NewLogger(WarnLevel)?).

Known limitations:

• As mentioned above, the feature does not currently detect potential name collisions between the new environment variable (declared globally) and environment variables declared at a lower scope (job- or step-level).
  This would require a full parsing of the entire workflow to understand precisely which step, in which job, the finding is flagging, which environment variables exist at that step, etc. Given that such name collisions seem exceedingly rare, the current "basic" implementation seems sufficient, in my opinion.
• There are situations where the proper use of the envvar isn't $FOO, but env.foo (i.e. in a more complex GitHub variable expansion) or process.env.foo (i.e. when using actions/github-script). The current implementation does not handle these situations properly, and always uses $FOO.

Open questions:

• Should this feature be added to the Scorecard documentation? If so, where? checks.yml/md?
• The logic to generate a patch diff is pretty generic and could easily be used in in other probes' Remediation.patch implementations. However, I'm unsure where the best place for such features would be. Create a new remediation/patch.go?

Does this PR introduce a user-facing change?

When detecting a potential script injection in a GitHub workflow, Scorecard now adds a machine-readable patch to fix the vulnerability. This patch can be applied to your project using `git apply` or `patch -p1` from the repository's root.

Thanks to @joycebrum and @diogoteles08 who helped come up with the tests and the logic to integrate with hasDangerousWorkflowScriptInjection.Run.

[spencerschrock]

Note: this feature is large enough it won't make the v5.0.0 cutoff, but excited to take a look later

[spencerschrock]

Thanks for the PR, I'll try to take a more in-depth look tomorrow but a few questions now based only on your PR description:

Each finding is patched by creating a global environment variable that wraps the dangerous GitHub variable and replacing that GitHub variable by the envvar in the relevant run command

My initial thoughts were around clobbering the environment variables, but it seems like you have a lot of test cases that deal with these scenarios. So I'll have to wait until my deep dive review tomorrow.

The patched workflow is then validated by parsing it with actionlint. As long as the patch added no new parsing errors, it is accepted.

This is a really cool validation step! Nicely done.

Questions for you

Where the patch is:

diff --git a/.github/workflows/awesome-action.yml b/.github/workflows/awesome-action.yml
index 5eeb62ef6d3e94fc63676e9f9557a9389d05a99c..d234fa3415691e8f3c4f40183770fcea755f8d2c 100644

1. Do we know if the patch will still work if the repo HEAD changes? I assume this is a git related question

2. Any idea how expensive this remediation is? Part of my thoughts with regard to remediation is that there should be some flag to control whether or not it gets surfaced/generated.

Open question responses

Should this feature be added to the Scorecard documentation? If so, where? checks.yml/md?

In the hasDangerousWorkflowScriptInjection def.yml file would be a good starting place probably.

The logic to generate a patch diff is pretty generic and could easily be used in in other probes' Remediation.patch implementations.

Until something else wants to use it, I'd say don't worry about where it could live. I'd say a good practice is marking the code as internal until we want others thing to use the code.

So making probes/hasDangerousWorkflowScriptInjection/patch -> probes/hasDangerousWorkflowScriptInjection/internal/patch would be a good move.

[spencerschrock]

some initial thoughts, ran out of review time for today

[spencerschrock]

do we want to do any consistency check on runCmdIndex as it's used it index into lines?

[pnacht]

Done.

[spencerschrock]

runCmdIndex could also be too large and panic

[pnacht]

Ah, true. Done. I now check for index == 0 (can't be negative, since it's a uint) and that it's larger than the number of lines.

[spencerschrock]

any reason to not use a map instead of linearly scanning a slice?

[pnacht]

A map wouldn't help here because we need to perform actual regex matches if p.idRegex.MatchString(unsafeVar), we can't simply use the variable as a key.

The regex is necessary because of the "array variables" (i.e. github.event.commits[3].message).

[spencerschrock]

Ah darn, makes sense though. This may be one of the parts that inspired my questions for you section above (#4218 (comment)):

Any idea how expensive this remediation is? Part of my thoughts with regard to remediation is that there should be some flag to control whether or not it gets surfaced/generated.

[spencerschrock]

just curious, have you tested on a single-line command?

        run: echo "${{ github.event.comment.body }}"

[pnacht]

Yep, that's tested in a few different places, such as https://github.com/ossf/scorecard/pull/4218/files#diff-932a94cc8f222f05147097bbd3271227231c9d12db59b85a2133417a3d6cd8deR42 (realExample1.yaml L42).

In those cases, the isParentLevelIndent(line, runIndent) will return true in the following line (either another step, or some other parameters for the run step, which will both have the same indentation as the run step itself), interrupting the loop, such that only the first line gets "replaced" by the regex below.

[pnacht]

Sorry, I'd missed these questions before.

Where the patch is:
diff --git a/.github/workflows/awesome-action.yml b/.github/workflows/awesome-action.yml
index 5eeb62ef6d3e94fc63676e9f9557a9389d05a99c..d234fa3415691e8f3c4f40183770fcea755f8d2c 100644

1. Do we know if the patch will still work if the repo HEAD changes? I assume this is a git related question

Those hashes aren't relevant; the resulting patch could be applied at any time, at any stage of the repository.

In fact, those hashes aren't even for the actual repository, they're the hashes for the "in-memory" repository used to generate the diff.

2. Any idea how expensive this remediation is? Part of my thoughts with regard to remediation is that there should be some flag to control whether or not it gets surfaced/generated.

I don't really know how expensive this will be in the worst case. But on the vast majority of cases it'll be a no-op, since most projects don't have workflows vulnerable to script injection. Looking at the latest BQ data, out of the 1.2M projects scanned, it only found ~2.5k workflows with script injections, each of which likely only has one or two vulnerabilities.

But if we were to try to fix a "malicious" workflow with hundreds of script injections... yeah, I don't know how long that'd take (I'd still guess not too long, though?).

Should this feature be added to the Scorecard documentation? If so, where? checks.yml/md?

In the hasDangerousWorkflowScriptInjection def.yml file would be a good starting place probably.

Done. PTAL.

I added documentation to def.yml describing that each finding has the patch. I also added the patch to the markdown remediation in def.yml. This works when testing on the CLI, but I'm not 100% how it'll appear in the Security Panel, since I don't know how to test that (I tried using --format sarif with the probe, but the SARIF came out empty, so I don't know if SARIF and probes are integrated yet).

So making probes/hasDangerousWorkflowScriptInjection/patch -> probes/hasDangerousWorkflowScriptInjection/internal/patch would be a good move.

Done.