Skip to content
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

Merged
merged 1 commit into from
Dec 9, 2024
Merged

Run zizmor in CI, and fix most warnings #14844

merged 1 commit into from
Dec 9, 2024

Conversation

AlexWaygood
Copy link
Member

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 with git later on in the workflow. None of our workflows do except for publish-docs.yml and sync-typeshed.yml, so I set persist-credentials: true for those two but persist-credentials: false for all other uses of actions/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 in release.yml:

release.yml findings
warning[artipacked]: credential persistence through GitHub Actions artifacts
  --> /Users/alexw/dev/ruff/.github/workflows/release.yml:62:9
   |
62 |         - uses: actions/checkout@v4
   |  _________-
63 | |         with:
64 | |           submodules: recursive
   | |_______________________________- does not set persist-credentials: false
   |
   = note: audit confidence → Low

warning[artipacked]: credential persistence through GitHub Actions artifacts
   --> /Users/alexw/dev/ruff/.github/workflows/release.yml:124:9
    |
124 |         - uses: actions/checkout@v4
    |  _________-
125 | |         with:
126 | |           submodules: recursive
    | |_______________________________- does not set persist-credentials: false
    |
    = note: audit confidence → Low

warning[artipacked]: credential persistence through GitHub Actions artifacts
   --> /Users/alexw/dev/ruff/.github/workflows/release.yml:174:9
    |
174 |         - uses: actions/checkout@v4
    |  _________-
175 | |         with:
176 | |           submodules: recursive
    | |_______________________________- does not set persist-credentials: false
    |
    = note: audit confidence → Low

warning[artipacked]: credential persistence through GitHub Actions artifacts
   --> /Users/alexw/dev/ruff/.github/workflows/release.yml:249:9
    |
249 |         - uses: actions/checkout@v4
    |  _________-
250 | |         with:
251 | |           submodules: recursive
252 | |       # Create a GitHub Release while uploading all files to it
    | |_______________________________________________________________- does not set persist-credentials: false
    |
    = note: audit confidence → Low

error[excessive-permissions]: overly broad workflow or job-level permissions
  --> /Users/alexw/dev/ruff/.github/workflows/release.yml:17:1
   |
17 | / permissions:
18 | |   "contents": "write"
...  |
39 | | # If there's a prerelease-style suffix to the version, then the release(s)
40 | | # will be marked as a prerelease.
   | |_________________________________^ contents: write is overly broad at the workflow level
   |
   = note: audit confidence → High

error[template-injection]: code injection via template expansion
  --> /Users/alexw/dev/ruff/.github/workflows/release.yml:80:9
   |
80 |          - id: plan
   |   _________^
81 |  |         run: |
   |  |_________^
82 | ||           dist ${{ (inputs.tag && inputs.tag != 'dry-run' && format('host --steps=create --tag={0}', inputs.tag)) || 'plan' }} --out...
83 | ||           echo "dist ran successfully"
84 | ||           cat plan-dist-manifest.json
85 | ||           echo "manifest=$(jq -c "." plan-dist-manifest.json)" >> "$GITHUB_OUTPUT"
   | ||__________________________________________________________________________________^ this step
   | ||__________________________________________________________________________________^ inputs.tag may expand into attacker-controllable code
   |
   = note: audit confidence → Low

error[template-injection]: code injection via template expansion
  --> /Users/alexw/dev/ruff/.github/workflows/release.yml:80:9
   |
80 |          - id: plan
   |   _________^
81 |  |         run: |
   |  |_________^
82 | ||           dist ${{ (inputs.tag && inputs.tag != 'dry-run' && format('host --steps=create --tag={0}', inputs.tag)) || 'plan' }} --out...
83 | ||           echo "dist ran successfully"
84 | ||           cat plan-dist-manifest.json
85 | ||           echo "manifest=$(jq -c "." plan-dist-manifest.json)" >> "$GITHUB_OUTPUT"
   | ||__________________________________________________________________________________^ this step
   | ||__________________________________________________________________________________^ inputs.tag may expand into attacker-controllable code
   |
   = note: audit confidence → Low

error[template-injection]: code injection via template expansion
  --> /Users/alexw/dev/ruff/.github/workflows/release.yml:80:9
   |
80 |          - id: plan
   |   _________^
81 |  |         run: |
   |  |_________^
82 | ||           dist ${{ (inputs.tag && inputs.tag != 'dry-run' && format('host --steps=create --tag={0}', inputs.tag)) || 'plan' }} --out...
83 | ||           echo "dist ran successfully"
84 | ||           cat plan-dist-manifest.json
85 | ||           echo "manifest=$(jq -c "." plan-dist-manifest.json)" >> "$GITHUB_OUTPUT"
   | ||__________________________________________________________________________________^ this step
   | ||__________________________________________________________________________________^ inputs.tag may expand into attacker-controllable code
   |
   = note: audit confidence → Low

Test Plan

uvx pre-commit run -a

@AlexWaygood AlexWaygood added ci Related to internal CI tooling security Related to security vulnerabilities labels Dec 8, 2024
Comment on lines +95 to +99
# `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]
Copy link
Member Author

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)

Copy link
Contributor

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 :)

Comment on lines +101 to +104
- repo: https://github.com/python-jsonschema/check-jsonschema
rev: 0.29.4
hooks:
- id: check-github-workflows
Copy link
Member Author

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.

Copy link
Contributor

github-actions bot commented Dec 8, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@AlexWaygood AlexWaygood merged commit 58e7db8 into main Dec 9, 2024
46 checks passed
@AlexWaygood AlexWaygood deleted the alex/zizmor branch December 9, 2024 00:42
AlexWaygood added a commit that referenced this pull request Dec 12, 2024
## 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 😄
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Related to internal CI tooling security Related to security vulnerabilities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants