Open pnacht opened 2 months ago
Note: this feature is large enough it won't make the v5.0.0 cutoff, but excited to take a look later
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.
Where the patch is:
diff --git a/.github/workflows/awesome-action.yml b/.github/workflows/awesome-action.yml index 5eeb62ef6d3e94fc63676e9f9557a9389d05a99c..d234fa3415691e8f3c4f40183770fcea755f8d2c 100644
Do we know if the patch will still work if the repo HEAD changes? I assume this is a git
related question
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.
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.
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
- 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.
- 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.
also DCO and make generate-docs
/scdiff generate Dangerous-Workflow
What kind of change does this PR introduce?
What is the current behavior?
Findings have a
.Remediation.Patch
field which is meant to contain a machine-readable patch fixing that particular finding:https://github.com/ossf/scorecard/blob/3155309aa81adf3395f4d62ee133b524ff316da1/finding/probe.go#L50-L52
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
orgit apply
) fixing allhasDangerousWorkflowScriptInjection
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:Or, on a real example:
Where the patch is:
The patched workflow is then validated by parsing it with
actionlint
. As long as the patch added no new parsing errors, it is accepted.Which issue(s) this PR fixes
Fixes #3950
Special notes for your reviewer
pkg/scorecard_result.populateRawResults
.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 theactionlint.Workflow
when possible (which is only to read any existing environment variables).env
must be created:jobs:
labeljobs:
label. The number of lines matches the number between thejobs:
label and the end of the preceding block in the original workflow.sclog.NewLogger(WarnLevel)
?).Known limitations:
$FOO
, butenv.foo
(i.e. in a more complex GitHub variable expansion) orprocess.env.foo
(i.e. when usingactions/github-script
). The current implementation does not handle these situations properly, and always uses$FOO
.Open questions:
checks.yml/md
?remediation/patch.go
?Does this PR introduce a user-facing change?
Thanks to @joycebrum and @diogoteles08 who helped come up with the tests and the logic to integrate with
hasDangerousWorkflowScriptInjection.Run
.