-
Notifications
You must be signed in to change notification settings - Fork 480
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ Add machine-readable patch to fix script injections in workflows #4218
base: main
Are you sure you want to change the base?
Conversation
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:
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.
This is a really cool validation step! Nicely done. Questions for you
Open question responses
In the
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 So making |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some initial thoughts, ran out of review time for today
runCmdIndex := f.Offset - 1 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want to do any consistency check on runCmdIndex
as it's used it index into lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
runCmdIndex
could also be too large and panic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
func getUnsafePattern(unsafeVar string) (unsafePattern, error) { | ||
unsafePatterns := []unsafePattern{ | ||
newUnsafePattern("AUTHOR_EMAIL", `github\.event\.commits.*?\.author\.email`), | ||
newUnsafePattern("AUTHOR_EMAIL", `github\.event\.head_commit\.author\.email`), | ||
newUnsafePattern("AUTHOR_NAME", `github\.event\.commits.*?\.author\.name`), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason to not use a map instead of linearly scanning a slice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
currLine := int(runIndex) + i | ||
if i > 0 && isParentLevelIndent(lines[currLine], runIndent) { | ||
// anything at the same indent as the first line of the `- run:` block will mean the end of the run block. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just curious, have you tested on a single-line command?
run: echo "${{ github.event.comment.body }}"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
…-fix create environment for patch on DW script injections Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>
…-with-remediation-output Include the generated patch in the output Signed-off-by: Joyce Brum <joycebrum@google.com>
Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
…erate-patch Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
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 <pnacht@google.com>
- 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 <pnacht@google.com>
- 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 <pnacht@google.com>
- 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 <pnacht@google.com>
- 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 <pnacht@google.com>
Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
- 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 <pnacht@google.com>
- 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 <pnacht@google.com>
Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
Sorry, I'd missed these questions before.
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.
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?).
Done. PTAL. I added documentation to
Done. |
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:scorecard/finding/probe.go
Lines 50 to 52 in 3155309
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:
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.
$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
.