-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Run zizmor in CI, and fix most warnings #14844
Conversation
# `release.yml` is autogenerated by `dist`; security issues need to be fixed there | ||
# (https://opensource.axo.dev/cargo-dist/) | ||
exclude: .github/workflows/release.yml | ||
# We could consider enabling the low-severity warnings, but they're noisy | ||
args: [--min-severity=medium] |
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.
I know we prefer to keep configuration in separate files rather than putting it in pre-commit, but it doesn't seem possible to specify these in zizmor's configuration file right now (https://woodruffw.github.io/zizmor/configuration/#settings)
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.
I noticed this too. Tip: open an issue for zizmor and I bet William will implement it :)
- repo: https://github.com/python-jsonschema/check-jsonschema | ||
rev: 0.29.4 | ||
hooks: | ||
- id: check-github-workflows |
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.
This is just some more extra linting for our workflows. It's unrelated to the other changes in the PR. It doesn't have any quarrels with any of our workflows currently, but I figured it might be a good idea to add it as well.
|
## Summary This PR changes our zizmor configuration to also flag low-severity security issues in our GitHub Actions workflows. It's a followup to #14844. The issues being fixed here were all flagged by [zizmor's `template-injection` rule](https://woodruffw.github.io/zizmor/audits/#template-injection): > Detects potential sources of code injection via template expansion. > > GitHub Actions allows workflows to define template expansions, which occur within special `${{ ... }}` delimiters. These expansions happen before workflow and job execution, meaning the expansion of a given expression appears verbatim in whatever context it was performed in. > > Template expansions aren't syntax-aware, meaning that they can result in unintended shell injection vectors. This is especially true when they're used with attacker-controllable expression contexts, such as `github.event.issue.title` (which the attacker can fully control by supplying a new issue title). [...] > To fully remediate the vulnerability, you should not use `${{ env.VARNAME }}`, since that is still a template expansion. Instead, you should use `${VARNAME}` to ensure that the shell itself performs the variable expansion. ## Test Plan I tested that this passes all zizmore warnings by running `pre-commit run -a zizmor` locally. The other test is obviously to check that the workflows all still run correctly in CI 😄
Summary
A recent exploit brought attention to how easy it can be for attackers to use template expansion in GitHub Actions workflows to inject arbitrary code into a repository. That vulnerability would have been caught by the zizmor linter, which looks for potential security vulnerabilities in GitHub Actions workflows. This PR adds zizmor as a pre-commit hook and fixes the high- and medium-severity warnings flagged by the tool.
All the warnings fixed in this PR are related to this zizmor check: https://woodruffw.github.io/zizmor/audits/#artipacked. The summary of the check is that
actions/checkout
will by default persist git configuration for the duration of the workflow, which can be insecure. It's unnecessary unless you actually need to do things withgit
later on in the workflow. None of our workflows do except forpublish-docs.yml
andsync-typeshed.yml
, so I setpersist-credentials: true
for those two butpersist-credentials: false
for all other uses ofactions/checkout
.Unfortunately there are several warnings in
release.yml
, including four high-severity warnings. However, this is a generated workflow file, so I have deliberately excluded this file from the check. These are the findings inrelease.yml
:release.yml findings
Test Plan
uvx pre-commit run -a