Add zizmor to pre-commit and fix most findings - #127749
Conversation
Sorry, something went wrong.
|
Nice! As a suggestion: to reduce the risk of the lower-severity findings being ignored indefinitely, the CPython CI could use zizmor's SARIF format here to integrate with GitHub's security scanning feature. Once integrated, zizmor's results get tracked by GitHub itself and can be ignored/suppressed/dismissed within GitHub's own UI. For example, this is what a finding looks like (this one is from Homebrew, and has since been resolved):
The main downside to doing this is that it might need to be shoehorned a bit into the pre-commit approach, or broken out entirely (which might be unacceptable if everything else is driven by pre-commit). As such I leave it up to you to do what's best for this repo; just wanted to make sure people were aware of it 🙂 Edit: another thing security scanning enables is ruleset enforcement, e.g. this is what I have on zizmor's own repo to prevent new audit findings from being introduced:
|
Sorry, something went wrong.
AlexWaygood
left a comment
There was a problem hiding this comment.
Nice, thank you!
Nice! As a suggestion: to reduce the risk of the lower-severity findings being ignored indefinitely, the CPython CI could use zizmor's SARIF format here to integrate with GitHub's security scanning feature [...] The main downside to doing this is that it might need to be shoehorned a bit into the pre-commit approach, or broken out entirely (which might be unacceptable if everything else is driven by pre-commit).
No strong opinion on this! I'm used to pre-commit and I'd quite like to get notified of new security issues when running pre-commit run -a locally. I also think we could track solving the low-severity warnings by opening an issue. But it sounds like this approach also has a lot to recommend it... I'm happy with whatever!
Sorry, something went wrong.
| run: | | ||
| # Fetch enough history to find a common ancestor commit (aka merge-base): | ||
| git fetch origin ${{ env.refspec_pr }} --depth=$(( ${{ github.event.pull_request.commits }} + 1 )) \ | ||
| git fetch origin ${{ env.refspec_pr }} --depth=$(( ${{ env.commits }} + 1 )) \ |
There was a problem hiding this comment.
not an argument against doing this, just a howl of frustration at GitHub: it feels so silly that we have to do this to make our workflow secure ☹️ But the rationale in https://woodruffw.github.io/zizmor/audits/#template-injection is strong, and obviously this is exactly how the recent exploit was accomplished
Sorry, something went wrong.
There was a problem hiding this comment.
Yeah, it's definitely not ideal 🙃.
In this particular case however, this might be a false positive: contextually it looks like github.event.pull_request.commits can only ever be an integer, so there's no risk of code injection. If that sounds right to you all, I can add this context to zizmor's allow-list (which is currently woefully incomplete, since there are a lot of default contexts).
(And as a separate note: ${{ env ... }} is still an injection risk, although probably a lesser one in this context. I think the only reason zizmor isn't warning about it here is because we emit a low-severity warning for it, since it's hard to analyze/generalize over 🙂.)
Sorry, something went wrong.
There was a problem hiding this comment.
Yup, https://docs.github.com/en/webhooks/webhook-events-and-payloads#pull_request documents that pull_request.commits will always be an integer -- so we're safe as long as they don't change that 😄
(And as a separate note: ${{ env ... }} is still an injection risk, although probably a lesser one in this context. I think the only reason zizmor isn't warning about it here is because we emit a low-severity warning for it, since it's hard to analyze/generalize over 🙂.)
I'm very ignorant about all this, but I assumed from your docs at https://woodruffw.github.io/zizmor/audits/#template-injection that using an environment variable here would be safer:
The most common forms of template injection are in run: and similar code-execution blocks. In these cases, an inline template expansion can typically be replaced by an environment variable whose value comes from the expanded template.
This avoids the vulnerability, since variable expansion is subject to normal shell quoting/expansion rules.
Sorry, something went wrong.
There was a problem hiding this comment.
I guess it's saf-er but not truly safe? :)
Sorry, something went wrong.
There was a problem hiding this comment.
Ah, I think that phrasing is confusing! What I was trying to say in those docs is that lines like:
run: foo ${{ env.bar }}should be ideally replaced with:
run: foo "${BAR}"
env:
BAR: ${{ env.bar }}In other words: in the best case ${{ ... }} never ever appears in a run: or similar block, it should always be forwarded via an env: setting.
I can improve those docs to make that more clear 🙂
Sorry, something went wrong.
There was a problem hiding this comment.
(Technically I think run: foo ${bar} might also work, since GHA should set bar in the shell environment if env.bar is set in the context namespace. But there are some weird edge cases around that, so I personally tend to explicitly forward it with an `env: block.)
Sorry, something went wrong.
There was a problem hiding this comment.
Just to make sure I understand correctly, if I have something like:
run: echo "Issue title: ${{ github.event.issue.title }}"and the attacker sets the title to something like "; rm -rf /; echo ", the run command will become:
run: echo "Issue title: "; rm -rf /; echo ""and execute the 3 commands separately (echo+rm+echo).
Whereas if I use
run: echo "Issue title: ${TITLE}"
env:
TITLE: ${{ github.event.issue.title }}when ${TITLE} is used in run it will be escaped and expanded to something like
run: echo "Issue title: \"; rm -rf /; echo \""which will only execute a single echo and print Issue title: "; rm -rf /; echo ", right?
Also what is the difference -- once the env var is set -- between using:
- run: echo "Issue title: $TITLE"
- run: echo "Issue title: ${TITLE}"
- run: echo "Issue title: ${{env.TITLE}}"
Are they all equivalent or do different escaping/expansions rules apply?
Sorry, something went wrong.
There was a problem hiding this comment.
should be ideally replaced with:
run: foo "${BAR}" env: BAR: ${{ env.bar }}
Aha, thanks! I've updated the PR. Although this isn't working in Windows' pwsh.exe shell, what's the right syntax there?
https://github.com/python/cpython/actions/runs/12238891899/job/34138074592?pr=127749
Sorry, something went wrong.
There was a problem hiding this comment.
Although this isn't working in Windows' pwsh.exe shell, what's the right syntax there?
You could try using shell: bash to force a bash shell even on Windows?
Sorry, something went wrong.
There was a problem hiding this comment.
Are they all equivalent or do different escaping/expansions rules apply?
The first two examples are shell interpolations, and have the same expansion rules applied to them. But note: this is only because of the surrounding quotes: if you had instead written echo $TITLE, then $TITLE would be expanded using "splatting" rules and its expansion would be interpreted as flags to echo, rather than a single string input.
The third example is a template interpolation, which bypasses the shell interpolation rules. It's not safe to do in the general case.
TL;DR: you almost always want some variant of "${VAR}" or "$VAR", quoting included 🙂
You could try using shell: bash to force a bash shell even on Windows?
Yep, this is the easiest fix -- the alternative is to use pwsh's $env:VARNAME syntax, but that means having to split your steps.
Sorry, something went wrong.
| echo '${{ github.event_name }}' | ||
| - uses: actions/checkout@v4 | ||
| with: | ||
| persist-credentials: false |
There was a problem hiding this comment.
There's a git fetch in the next step. Wouldn't that still need the credentials in order to work?
Sorry, something went wrong.
There was a problem hiding this comment.
I don't think it needs to be an authenticated git fetch?
It ran okay here: https://github.com/python/cpython/actions/runs/12225521755/job/34099604804
Sorry, something went wrong.
There was a problem hiding this comment.
Yep, persist-credentials is only needed for git ops that need authentication. In the context of public repos and their workflows, in practice that means only ops that mutate the repo, rather than pulling from it 🙂
Sorry, something went wrong.
There was a problem hiding this comment.
That makes sense, but if this is the case, does it also mean that as long as we are just reading from public repos, there are no credentials used so there's nothing to share with the other jobs?
IOW, persist-credentials: true would only be a concern when a token is used, either for mutating operations (e.g. pushes) or for accessing private repos, and in this case there's actually no security risk, right?
(It might still be better to explicitly add persist-credentials: false regardless though -- e.g. in case someone adds a token later.)
Sorry, something went wrong.
There was a problem hiding this comment.
That makes sense, but if this is the case, does it also mean that as long as we are just reading from public repos, there are no credentials used so there's nothing to share with the other jobs?
Nope, unfortunately the default means that a credential is persisted, even though it isn't necessary. So persist-credentials: false actually does something usefully by explicitly removing the credential.
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks for your reply! I'm still not sure I understand if there is an actual issue with reading (i.e. with no writing/mutating operations) public repos though. I tried digging a bit deeper to understand and this is what I found.
The actions/checkout README says that it accepts both a token and an ssh-key (which we are not using):
# Personal access token (PAT) used to fetch the repository. The PAT is configured
# with the local git config, which enables your scripts to run authenticated git
# commands. The post-job step removes the PAT.
#
# We recommend using a service account with the least permissions necessary. Also
# when generating a new PAT, select the least scopes necessary.
#
# [Learn more about creating and using encrypted secrets](https://help.github.com/en/actions/automating-your-workflow-with-github-actions/creating-and-using-encrypted-secrets)
#
# Default: ${{ github.token }}
token: ''
# SSH key used to fetch the repository. The SSH key is configured with the local
# git config, which enables your scripts to run authenticated git commands. The
# post-job step removes the SSH key.
#
# We recommend using a service account with the least permissions necessary.
#
# [Learn more about creating and using encrypted secrets](https://help.github.com/en/actions/automating-your-workflow-with-github-actions/creating-and-using-encrypted-secrets)
ssh-key: ''The comment for persist-credentials says:
# Whether to configure the token or SSH key with the local git config
# Default: true
persist-credentials: ''So my understanding is that if I explicitly set a token/key with token: '...' or ssh-key: '...' and use persist-credentials: true (either explicitly or implicitly since it's the default), the token/key will be "persisted in the local git config" (which I assume is .git/config, i.e. it will be written on a file). This will expose the token/key to the other jobs -- which is a security risk.
If I don't explicitly specify any token/key (i.e. what we are doing in our workflows), actions/checkout will set github.token as default token and persist that. Since github.token is already available to all jobs, the fact that it is persisted shouldn't be an issue -- unless having its value written on a file poses a new threat that I'm not aware of.
Nope, unfortunately the default means that a credential is persisted, even though it isn't necessary.
If no credentials are passed explicitly by using token/ssh-key (which are not needed while reading public repos, like in our workflows) and assuming persist-credentials: true, either:
- there is no security issue (unless either a token or ssh-key are explicitly set);
- persisting github.token is still a security issue (maybe because it's written on a file?); 1
- there's some other credential that I'm missing that causes a security issue;
Footnotes
-
if this is a problem, maybe setting token: '' explicitly will prevent the github.token to be used (and persisted) at all (depending on how the action is implemented) ↩
Sorry, something went wrong.
There was a problem hiding this comment.
No problem! Thanks for your detailed response as well.
Since github.token is already available to all jobs, the fact that it is persisted shouldn't be an issue -- unless having its value written on a file poses a new threat that I'm not aware of.
Yep: the risk with this is that it's easy to inadvertently upload/log/otherwise persist that local filesystem credential. This can't happen with the in-memory token (i.e. github.token) in the ordinary case.
For example, until recently, this would cause a token disclosure via a public artifact:
uses: actions/checkout
# other steps
uses: actions/upload-artifact
with:
path: . # uploads the entire repo, including the persisted tokenThis post has some interesting/elucidative examples of that: https://unit42.paloaltonetworks.com/github-repo-artifacts-leak-tokens/
- if this is a problem, maybe setting token: '' explicitly will prevent the github.token to be used (and persisted) at all (depending on how the action is implemented) ↩
Yeah, I believe this will result in an empty string being saved on the filesystem for the token. I'm not 100% sure but I think this will cause GitHub API errors in some cases.
(I think persist-credentials: false is the intended way to disable github.token.)
Sorry, something went wrong.
There was a problem hiding this comment.
This post has some interesting/elucidative examples of that: https://unit42.paloaltonetworks.com/github-repo-artifacts-leak-tokens/
Thanks for sharing, this is really interesting!
Now that I have all the pieces of the puzzle it finally makes sense:
- GitHub automatically creates a github.token
- If no token: ... is specified in actions/checkout, then github.token is used
- If persist-credentials is true (the default), either the github.token or the specified token will be written in .git/config
- If the file containing the token becomes publicly accessible (e.g. uploaded via actions/upload-artifact) an attacker could access it and use it
- persist-credentials: false avoids writing the tokens on files and prevents token leaks
There are a few more caveats here and there, but this is already more than enough to justify the use of persist-credentials: false.
Sorry, something went wrong.
There was a problem hiding this comment.
No problem! Your summary is great and matches my understanding perfectly 🙂
Sorry, something went wrong.
sethmlarson
left a comment
There was a problem hiding this comment.
LGTM, thanks for this Hugo!
Sorry, something went wrong.
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
| runs-on: ubuntu-latest | ||
| permissions: | ||
| issues: write | ||
| pull-requests: write |
There was a problem hiding this comment.
These labels are only used on PRs, so I'm pretty sure the issues: write can be removed. In addition, I think we might not even need the pull-requests: write since AFAICT we are not writing anything.
I don't have enough energy today to also dig into this to make sure that the permissions are not needed, so feel free to merge this as is and we can open a separate issue to investigate and possibly remove these.
Sorry, something went wrong.
There was a problem hiding this comment.
Yep, we can check with this separately. This PR isn't adding any extra permissions here.
Sorry, something went wrong.
|
Thanks all for the reviews! Let's backport to 3.12 as well. Let's see how the bot goes, I'm expecting merge conflicts. Edit: And we could consider backports to 3.9 as well? |
Sorry, something went wrong.
|
Thanks @hugovk for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13. |
Sorry, something went wrong.
|
Sorry, @hugovk, I could not cleanly backport this to 3.13 due to a conflict. cherry_picker ae31df354d02e12bf656954c5c72380d96c1dc0e 3.13 |
Sorry, something went wrong.
|
Sorry, @hugovk, I could not cleanly backport this to 3.12 due to a conflict. cherry_picker ae31df354d02e12bf656954c5c72380d96c1dc0e 3.12 |
Sorry, something went wrong.
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com> (cherry picked from commit ae31df3)
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com> (cherry picked from commit ae31df3)
|
GH-127786 is a backport of this pull request to the 3.13 branch. |
Sorry, something went wrong.
(cherry picked from commit ae31df3) Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com> Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
|
GH-127788 is a backport of this pull request to the 3.12 branch. |
Sorry, something went wrong.
… (python#127788) Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com> Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com> Co-authored-by: Ezio Melotti <ezio.melotti@gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
https://woodruffw.github.io/zizmor/
This PR adds zizmor to pre-commit, which runs it on the CI.
Here are all the findings currently on main:
134 findings (89 suppressed): 0 unknown, 0 informational, 13 low, 21 medium, 11 highwarning[artipacked]: credential persistence through GitHub Actions artifacts --> .github/workflows/reusable-windows.yml:30:7 | 30 | - uses: actions/checkout@v4 | ------------------------- does not set persist-credentials: false | = note: audit confidence → Low error[template-injection]: code injection via template expansion --> .github/workflows/reusable-windows.yml:34:7 | 34 | - name: Build CPython | ^^^^^^^^^^^^^^^^^^^ this step 35 | run: >- | _______^ 36 | | .\\PCbuild\\build.bat 37 | | -e -d -v 38 | | -p ${{ inputs.arch }} 39 | | ${{ fromJSON(inputs.free-threading) && '--disable-gil' || '' }} | |_______________________________________________________________________^ inputs.arch may expand into attacker-controllable code | = note: audit confidence → Low error[template-injection]: code injection via template expansion --> .github/workflows/reusable-windows.yml:43:7 | 43 | - name: Tests # FIXME(diegorusso): remove the `if` | ^^^^^^^^^^^ this step 44 | if: inputs.arch != 'arm64' 45 | run: >- | _______^ 46 | | .\\PCbuild\\rt.bat 47 | | -p ${{ inputs.arch }} 48 | | -d -q --fast-ci 49 | | ${{ fromJSON(inputs.free-threading) && '--disable-gil' || '' }} | |________________________________________________________________________^ inputs.arch may expand into attacker-controllable code | = note: audit confidence → Low warning[artipacked]: credential persistence through GitHub Actions artifacts --> .github/workflows/reusable-ubuntu.yml:30:7 | 30 | - uses: actions/checkout@v4 | ------------------------- does not set persist-credentials: false | = note: audit confidence → Low help[template-injection]: code injection via template expansion --> .github/workflows/reusable-ubuntu.yml:93:7 | 93 | - name: Check compiler warnings | ----------------------------- help: this step 94 | if: ${{ !inputs.free-threading }} 95 | run: >- | _______- 96 | | python Tools/build/check_warnings.py ... | 101 | | --fail-on-improvement 102 | | --path-prefix="../cpython-ro-srcdir/" | |_____________________________________________- help: env.CPYTHON_BUILDDIR may expand into attacker-controllable code | = note: audit confidence → High error[excessive-permissions]: overly broad workflow or job-level permissions --> .github/workflows/require-pr-label.yml:7:1 | 7 | / permissions: 8 | | issues: write 9 | | pull-requests: write | |______________________^ issues: write is overly broad at the workflow level | = note: audit confidence → High error[excessive-permissions]: overly broad workflow or job-level permissions --> .github/workflows/require-pr-label.yml:7:1 | 7 | / permissions: 8 | | issues: write 9 | | pull-requests: write | |______________________^ pull-requests: write is overly broad at the workflow level | = note: audit confidence → High warning[artipacked]: credential persistence through GitHub Actions artifacts --> .github/workflows/lint.yml:22:9 | 22 | - uses: actions/checkout@v4 | ------------------------- does not set persist-credentials: false | = note: audit confidence → Low warning[artipacked]: credential persistence through GitHub Actions artifacts --> .github/workflows/verify-ensurepip-wheels.yml:28:9 | 28 | - uses: actions/checkout@v4 | ------------------------- does not set persist-credentials: false | = note: audit confidence → Low warning[artipacked]: credential persistence through GitHub Actions artifacts --> .github/workflows/jit.yml:34:9 | 34 | - uses: actions/checkout@v4 | ------------------------- does not set persist-credentials: false | = note: audit confidence → Low warning[artipacked]: credential persistence through GitHub Actions artifacts --> .github/workflows/jit.yml:87:9 | 87 | - uses: actions/checkout@v4 | ------------------------- does not set persist-credentials: false | = note: audit confidence → Low warning[artipacked]: credential persistence through GitHub Actions artifacts --> .github/workflows/jit.yml:140:9 | 140 | - uses: actions/checkout@v4 | ------------------------- does not set persist-credentials: false | = note: audit confidence → Low warning[artipacked]: credential persistence through GitHub Actions artifacts --> .github/workflows/reusable-macos.yml:31:7 | 31 | - uses: actions/checkout@v4 | ------------------------- does not set persist-credentials: false | = note: audit confidence → Low error[excessive-permissions]: overly broad workflow or job-level permissions --> .github/workflows/stale.yml:7:1 | 7 | / permissions: 8 | | pull-requests: write | |______________________^ pull-requests: write is overly broad at the workflow level | = note: audit confidence → High warning[artipacked]: credential persistence through GitHub Actions artifacts --> .github/workflows/reusable-change-detection.yml:63:7 | 63 | - uses: actions/checkout@v4 | ------------------------- does not set persist-credentials: false | = note: audit confidence → Low warning[artipacked]: credential persistence through GitHub Actions artifacts --> .github/workflows/reusable-docs.yml:28:7 | 28 | - name: 'Check out latest PR branch commit' | _______- 29 | | uses: actions/checkout@v4 ... | 36 | | }} 37 | | # Adapted from https://github.com/actions/checkout/issues/520#issuecomment-1167205721 | |_________________________________________________________________________________________- does not set persist-credentials: false | = note: audit confidence → Low warning[artipacked]: credential persistence through GitHub Actions artifacts --> .github/workflows/reusable-docs.yml:83:7 | 83 | - uses: actions/checkout@v4 | ------------------------- does not set persist-credentials: false | = note: audit confidence → Low warning[artipacked]: credential persistence through GitHub Actions artifacts --> .github/workflows/reusable-docs.yml:101:7 | 101 | - uses: actions/checkout@v4 | ------------------------- does not set persist-credentials: false | = note: audit confidence → Low help[template-injection]: code injection via template expansion --> .github/workflows/reusable-docs.yml:38:7 | 38 | - name: 'Fetch commits to get branch diff' | ---------------------------------------- help: this step 39 | if: github.event_name == 'pull_request' 40 | run: | | _______- 41 | | # Fetch enough history to find a common ancestor commit (aka merge-base): ... | 50 | | git fetch origin ${{ env.refspec_base }} --shallow-since="${DATE}" \ 51 | | --no-tags --prune --no-recurse-submodules | |___________________________________________________- help: env.refspec_pr may expand into attacker-controllable code | = note: audit confidence → High error[template-injection]: code injection via template expansion --> .github/workflows/reusable-docs.yml:38:7 | 38 | - name: 'Fetch commits to get branch diff' | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this step 39 | if: github.event_name == 'pull_request' 40 | run: | | _______^ 41 | | # Fetch enough history to find a common ancestor commit (aka merge-base): ... | 50 | | git fetch origin ${{ env.refspec_base }} --shallow-since="${DATE}" \ 51 | | --no-tags --prune --no-recurse-submodules | |___________________________________________________^ github.event.pull_request.commits may expand into attacker-controllable code | = note: audit confidence → High help[template-injection]: code injection via template expansion --> .github/workflows/reusable-docs.yml:38:7 | 38 | - name: 'Fetch commits to get branch diff' | ---------------------------------------- help: this step 39 | if: github.event_name == 'pull_request' 40 | run: | | _______- 41 | | # Fetch enough history to find a common ancestor commit (aka merge-base): ... | 50 | | git fetch origin ${{ env.refspec_base }} --shallow-since="${DATE}" \ 51 | | --no-tags --prune --no-recurse-submodules | |___________________________________________________- help: env.branch_pr may expand into attacker-controllable code | = note: audit confidence → High help[template-injection]: code injection via template expansion --> .github/workflows/reusable-docs.yml:38:7 | 38 | - name: 'Fetch commits to get branch diff' | ---------------------------------------- help: this step 39 | if: github.event_name == 'pull_request' 40 | run: | | _______- 41 | | # Fetch enough history to find a common ancestor commit (aka merge-base): ... | 50 | | git fetch origin ${{ env.refspec_base }} --shallow-since="${DATE}" \ 51 | | --no-tags --prune --no-recurse-submodules | |___________________________________________________- help: env.refspec_base may expand into attacker-controllable code | = note: audit confidence → High help[template-injection]: code injection via template expansion --> .github/workflows/reusable-docs.yml:68:7 | 68 | - name: 'Check warnings' | ---------------------- help: this step 69 | if: github.event_name == 'pull_request' 70 | run: | | _______- 71 | | python Doc/tools/check-warnings.py \ ... | 74 | | --fail-if-improved \ 75 | | --fail-if-new-news-nit | |________________________________- help: env.branch_base may expand into attacker-controllable code | = note: audit confidence → High help[template-injection]: code injection via template expansion --> .github/workflows/reusable-docs.yml:68:7 | 68 | - name: 'Check warnings' | ---------------------- help: this step 69 | if: github.event_name == 'pull_request' 70 | run: | | _______- 71 | | python Doc/tools/check-warnings.py \ ... | 74 | | --fail-if-improved \ 75 | | --fail-if-new-news-nit | |________________________________- help: env.branch_pr may expand into attacker-controllable code | = note: audit confidence → High warning[artipacked]: credential persistence through GitHub Actions artifacts --> .github/workflows/reusable-tsan.yml:27:7 | 27 | - uses: actions/checkout@v4 | ------------------------- does not set persist-credentials: false | = note: audit confidence → Low error[template-injection]: code injection via template expansion --> .github/workflows/reusable-tsan.yml:48:7 | 48 | - name: TSAN Option Setup | ^^^^^^^^^^^^^^^^^^^^^^^ this step 49 | run: | | _______^ 50 | | echo "TSAN_OPTIONS=log_path=${GITHUB_WORKSPACE}/tsan_log suppressions=${GITHUB_WORKSPACE}/${{ inputs.suppressions_path }} ha... 51 | | echo "CC=clang" >> "$GITHUB_ENV" 52 | | echo "CXX=clang++" >> "$GITHUB_ENV" | |___________________________________________^ inputs.suppressions_path may expand into attacker-controllable code | = note: audit confidence → Low error[template-injection]: code injection via template expansion --> .github/workflows/reusable-tsan.yml:61:7 | 61 | - name: Configure CPython | ^^^^^^^^^^^^^^^^^^^^^^^ this step 62 | run: ${{ inputs.options }} | ^^^^^^^^^^^^^^^^^^^^^^^^^^ inputs.options may expand into attacker-controllable code | = note: audit confidence → Low warning[artipacked]: credential persistence through GitHub Actions artifacts --> .github/workflows/mypy.yml:53:9 | 53 | - uses: actions/checkout@v4 | ------------------------- does not set persist-credentials: false | = note: audit confidence → Low warning[artipacked]: credential persistence through GitHub Actions artifacts --> .github/workflows/reusable-windows-msi.yml:22:7 | 22 | - uses: actions/checkout@v4 | ------------------------- does not set persist-credentials: false | = note: audit confidence → Low error[template-injection]: code injection via template expansion --> .github/workflows/reusable-windows-msi.yml:23:7 | 23 | - name: Build CPython installer | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this step 24 | run: .\Tools\msi\build.bat --doc -${{ inputs.arch }} | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ inputs.arch may expand into attacker-controllable code | = note: audit confidence → Low error[excessive-permissions]: overly broad workflow or job-level permissions --> .github/workflows/documentation-links.yml:13:1 | 13 | / permissions: 14 | | pull-requests: write | |______________________^ pull-requests: write is overly broad at the workflow level | = note: audit confidence → High error[dangerous-triggers]: use of fundamentally insecure workflow trigger --> .github/workflows/documentation-links.yml:5:1 | 5 | / on: 6 | | pull_request_target: ... | 10 | | - 'Doc/**' 11 | | - '.github/workflows/doc.yml' | |_________________________________^ pull_request_target is almost always used insecurely | = note: audit confidence → Medium warning[artipacked]: credential persistence through GitHub Actions artifacts --> .github/workflows/reusable-wasi.yml:22:7 | 22 | - uses: actions/checkout@v4 | _______- 23 | | # No problem resolver registered as one doesn't currently exist for Clang. | |______________________________________________________________________________- does not set persist-credentials: false | = note: audit confidence → Low help[template-injection]: code injection via template expansion --> .github/workflows/reusable-wasi.yml:34:7 | 34 | - name: "Install WASI SDK" # Hard-coded to x64. | ------------------------ help: this step 35 | if: steps.cache-wasi-sdk.outputs.cache-hit != 'true' 36 | run: | | _______- 37 | | mkdir ${{ env.WASI_SDK_PATH }} && \ 38 | | curl -s -S --location https://github.com/WebAssembly/wasi-sdk/releases/download/wasi-sdk-${{ env.WASI_SDK_VERSION }}/wasi-sd... 39 | | tar --strip-components 1 --directory ${{ env.WASI_SDK_PATH }} --extract --gunzip | |________________________________________________________________________________________- help: env.WASI_SDK_PATH may expand into attacker-controllable code | = note: audit confidence → High help[template-injection]: code injection via template expansion --> .github/workflows/reusable-wasi.yml:34:7 | 34 | - name: "Install WASI SDK" # Hard-coded to x64. | ------------------------ help: this step 35 | if: steps.cache-wasi-sdk.outputs.cache-hit != 'true' 36 | run: | | _______- 37 | | mkdir ${{ env.WASI_SDK_PATH }} && \ 38 | | curl -s -S --location https://github.com/WebAssembly/wasi-sdk/releases/download/wasi-sdk-${{ env.WASI_SDK_VERSION }}/wasi-sd... 39 | | tar --strip-components 1 --directory ${{ env.WASI_SDK_PATH }} --extract --gunzip | |________________________________________________________________________________________- help: env.WASI_SDK_VERSION may expand into attacker-controllable code | = note: audit confidence → High help[template-injection]: code injection via template expansion --> .github/workflows/reusable-wasi.yml:34:7 | 34 | - name: "Install WASI SDK" # Hard-coded to x64. | ------------------------ help: this step 35 | if: steps.cache-wasi-sdk.outputs.cache-hit != 'true' 36 | run: | | _______- 37 | | mkdir ${{ env.WASI_SDK_PATH }} && \ 38 | | curl -s -S --location https://github.com/WebAssembly/wasi-sdk/releases/download/wasi-sdk-${{ env.WASI_SDK_VERSION }}/wasi-sd... 39 | | tar --strip-components 1 --directory ${{ env.WASI_SDK_PATH }} --extract --gunzip | |________________________________________________________________________________________- help: env.WASI_SDK_VERSION may expand into attacker-controllable code | = note: audit confidence → High help[template-injection]: code injection via template expansion --> .github/workflows/reusable-wasi.yml:34:7 | 34 | - name: "Install WASI SDK" # Hard-coded to x64. | ------------------------ help: this step 35 | if: steps.cache-wasi-sdk.outputs.cache-hit != 'true' 36 | run: | | _______- 37 | | mkdir ${{ env.WASI_SDK_PATH }} && \ 38 | | curl -s -S --location https://github.com/WebAssembly/wasi-sdk/releases/download/wasi-sdk-${{ env.WASI_SDK_VERSION }}/wasi-sd... 39 | | tar --strip-components 1 --directory ${{ env.WASI_SDK_PATH }} --extract --gunzip | |________________________________________________________________________________________- help: env.WASI_SDK_PATH may expand into attacker-controllable code | = note: audit confidence → High help[template-injection]: code injection via template expansion --> .github/workflows/reusable-wasi.yml:74:7 | 74 | - name: "Display build info" | -------------------------- help: this step 75 | run: make --directory ${{ env.CROSS_BUILD_WASI }} pythoninfo | ------------------------------------------------------------ help: env.CROSS_BUILD_WASI may expand into attacker-controllable code | = note: audit confidence → High help[template-injection]: code injection via template expansion --> .github/workflows/reusable-wasi.yml:76:7 | 76 | - name: "Test" | ------------ help: this step 77 | run: make --directory ${{ env.CROSS_BUILD_WASI }} test | ------------------------------------------------------ help: env.CROSS_BUILD_WASI may expand into attacker-controllable code | = note: audit confidence → High warning[artipacked]: credential persistence through GitHub Actions artifacts --> .github/workflows/build.yml:58:9 | 58 | - uses: actions/checkout@v4 | _________- 59 | | with: 60 | | fetch-depth: 1 | |________________________- does not set persist-credentials: false | = note: audit confidence → Low warning[artipacked]: credential persistence through GitHub Actions artifacts --> .github/workflows/build.yml:96:9 | 96 | - uses: actions/checkout@v4 | ------------------------- does not set persist-credentials: false | = note: audit confidence → Low warning[artipacked]: credential persistence through GitHub Actions artifacts --> .github/workflows/build.yml:270:7 | 270 | - uses: actions/checkout@v4 | ------------------------- does not set persist-credentials: false | = note: audit confidence → Low warning[artipacked]: credential persistence through GitHub Actions artifacts --> .github/workflows/build.yml:330:7 | 330 | - uses: actions/checkout@v4 | ------------------------- does not set persist-credentials: false | = note: audit confidence → Low warning[artipacked]: credential persistence through GitHub Actions artifacts --> .github/workflows/build.yml:448:7 | 448 | - uses: actions/checkout@v4 | ------------------------- does not set persist-credentials: false | = note: audit confidence → Low help[template-injection]: code injection via template expansion --> .github/workflows/build.yml:407:7 | 407 | - name: "Run tests" | ----------------- help: this step 408 | working-directory: ${{ env.CPYTHON_BUILDDIR }} 409 | run: | | _______- 410 | | # Most of the excluded tests are slow test suites with no property tests ... | 425 | | -x test_signal \ 426 | | -x test_sysconfig | |___________________________- help: env.VENV_PYTHON may expand into attacker-controllable code | = note: audit confidence → High 134 findings (89 suppressed): 0 unknown, 0 informational, 13 low, 21 medium, 11 highThis PR fixes all errors except this one:
Let's tackle that another time. For now it's added to the ignore list in .github/zizmor.yml.
Also, these high-severity errors are fixed:
error[template-injection]: code injection via template expansion --> .github/workflows/reusable-windows.yml:34:7 | 34 | - name: Build CPython | ^^^^^^^^^^^^^^^^^^^ this step 35 | run: >- | _______^ 36 | | .\\PCbuild\\build.bat 37 | | -e -d -v 38 | | -p ${{ inputs.arch }} 39 | | ${{ fromJSON(inputs.free-threading) && '--disable-gil' || '' }} | |_______________________________________________________________________^ inputs.arch may expand into attacker-controllable code | = note: audit confidence → LowAnd turned into these low-severity findings:
help[template-injection]: code injection via template expansion --> .github/workflows/reusable-windows.yml:38:7 | 38 | - name: Build CPython | ------------------- help: this step 39 | run: >- | _______- 40 | | .\\PCbuild\\build.bat 41 | | -e -d -v 42 | | -p ${{ env.ARCH }} 43 | | ${{ fromJSON(inputs.free-threading) && '--disable-gil' || '' }} | |_______________________________________________________________________- help: env.ARCH may expand into attacker-controllable code | = note: audit confidence → HighThere's also some other low-severity ones like this. So this PR runs zizmor with --min-severity=medium to ignore those low-severity ones for now.
Here's all the findings on this branch, without ignoring anything:
109 findings (89 suppressed): 0 unknown, 0 informational, 19 low, 0 medium, 1 highhelp[template-injection]: code injection via template expansion --> .github/workflows/reusable-windows.yml:38:7 | 38 | - name: Build CPython | ------------------- help: this step 39 | run: >- | _______- 40 | | .\\PCbuild\\build.bat 41 | | -e -d -v 42 | | -p ${{ env.ARCH }} 43 | | ${{ fromJSON(inputs.free-threading) && '--disable-gil' || '' }} | |_______________________________________________________________________- help: env.ARCH may expand into attacker-controllable code | = note: audit confidence → High help[template-injection]: code injection via template expansion --> .github/workflows/reusable-windows.yml:47:7 | 47 | - name: Tests # FIXME(diegorusso): remove the `if` | ----------- help: this step 48 | if: inputs.arch != 'arm64' 49 | run: >- | _______- 50 | | .\\PCbuild\\rt.bat 51 | | -p ${{ env.ARCH }} 52 | | -d -q --fast-ci 53 | | ${{ fromJSON(inputs.free-threading) && '--disable-gil' || '' }} | |________________________________________________________________________- help: env.ARCH may expand into attacker-controllable code | = note: audit confidence → High help[template-injection]: code injection via template expansion --> .github/workflows/reusable-ubuntu.yml:95:7 | 95 | - name: Check compiler warnings | ----------------------------- help: this step 96 | if: ${{ !inputs.free-threading }} 97 | run: >- | _______- 98 | | python Tools/build/check_warnings.py ... | 103 | | --fail-on-improvement 104 | | --path-prefix="../cpython-ro-srcdir/" | |_____________________________________________- help: env.CPYTHON_BUILDDIR may expand into attacker-controllable code | = note: audit confidence → High help[template-injection]: code injection via template expansion --> .github/workflows/reusable-docs.yml:40:7 | 40 | - name: 'Fetch commits to get branch diff' | ---------------------------------------- help: this step 41 | if: github.event_name == 'pull_request' 42 | run: | | _______- 43 | | # Fetch enough history to find a common ancestor commit (aka merge-base): ... | 52 | | git fetch origin ${{ env.refspec_base }} --shallow-since="${DATE}" \ 53 | | --no-tags --prune --no-recurse-submodules | |___________________________________________________- help: env.refspec_pr may expand into attacker-controllable code | = note: audit confidence → High help[template-injection]: code injection via template expansion --> .github/workflows/reusable-docs.yml:40:7 | 40 | - name: 'Fetch commits to get branch diff' | ---------------------------------------- help: this step 41 | if: github.event_name == 'pull_request' 42 | run: | | _______- 43 | | # Fetch enough history to find a common ancestor commit (aka merge-base): ... | 52 | | git fetch origin ${{ env.refspec_base }} --shallow-since="${DATE}" \ 53 | | --no-tags --prune --no-recurse-submodules | |___________________________________________________- help: env.commits may expand into attacker-controllable code | = note: audit confidence → High help[template-injection]: code injection via template expansion --> .github/workflows/reusable-docs.yml:40:7 | 40 | - name: 'Fetch commits to get branch diff' | ---------------------------------------- help: this step 41 | if: github.event_name == 'pull_request' 42 | run: | | _______- 43 | | # Fetch enough history to find a common ancestor commit (aka merge-base): ... | 52 | | git fetch origin ${{ env.refspec_base }} --shallow-since="${DATE}" \ 53 | | --no-tags --prune --no-recurse-submodules | |___________________________________________________- help: env.branch_pr may expand into attacker-controllable code | = note: audit confidence → High help[template-injection]: code injection via template expansion --> .github/workflows/reusable-docs.yml:40:7 | 40 | - name: 'Fetch commits to get branch diff' | ---------------------------------------- help: this step 41 | if: github.event_name == 'pull_request' 42 | run: | | _______- 43 | | # Fetch enough history to find a common ancestor commit (aka merge-base): ... | 52 | | git fetch origin ${{ env.refspec_base }} --shallow-since="${DATE}" \ 53 | | --no-tags --prune --no-recurse-submodules | |___________________________________________________- help: env.refspec_base may expand into attacker-controllable code | = note: audit confidence → High help[template-injection]: code injection via template expansion --> .github/workflows/reusable-docs.yml:70:7 | 70 | - name: 'Check warnings' | ---------------------- help: this step 71 | if: github.event_name == 'pull_request' 72 | run: | | _______- 73 | | python Doc/tools/check-warnings.py \ ... | 76 | | --fail-if-improved \ 77 | | --fail-if-new-news-nit | |________________________________- help: env.branch_base may expand into attacker-controllable code | = note: audit confidence → High help[template-injection]: code injection via template expansion --> .github/workflows/reusable-docs.yml:70:7 | 70 | - name: 'Check warnings' | ---------------------- help: this step 71 | if: github.event_name == 'pull_request' 72 | run: | | _______- 73 | | python Doc/tools/check-warnings.py \ ... | 76 | | --fail-if-improved \ 77 | | --fail-if-new-news-nit | |________________________________- help: env.branch_pr may expand into attacker-controllable code | = note: audit confidence → High help[template-injection]: code injection via template expansion --> .github/workflows/reusable-tsan.yml:53:7 | 53 | - name: TSAN Option Setup | ----------------------- help: this step 54 | run: | | _______- 55 | | echo "TSAN_OPTIONS=log_path=${GITHUB_WORKSPACE}/tsan_log suppressions=${GITHUB_WORKSPACE}/${{ env.SUPPRESSIONS_PATH }} handl... 56 | | echo "CC=clang" >> "$GITHUB_ENV" 57 | | echo "CXX=clang++" >> "$GITHUB_ENV" | |___________________________________________- help: env.SUPPRESSIONS_PATH may expand into attacker-controllable code | = note: audit confidence → High help[template-injection]: code injection via template expansion --> .github/workflows/reusable-tsan.yml:66:7 | 66 | - name: Configure CPython | ----------------------- help: this step 67 | run: ${{ env.OPTIONS }} | ----------------------- help: env.OPTIONS may expand into attacker-controllable code | = note: audit confidence → High help[template-injection]: code injection via template expansion --> .github/workflows/reusable-windows-msi.yml:26:7 | 26 | - name: Build CPython installer | ----------------------------- help: this step 27 | run: .\Tools\msi\build.bat --doc -${{ env.ARCH }} | ------------------------------------------------- help: env.ARCH may expand into attacker-controllable code | = note: audit confidence → High error[dangerous-triggers]: use of fundamentally insecure workflow trigger --> .github/workflows/documentation-links.yml:5:1 | 5 | / on: 6 | | pull_request_target: ... | 10 | | - 'Doc/**' 11 | | - '.github/workflows/doc.yml' | |_________________________________^ pull_request_target is almost always used insecurely | = note: audit confidence → Medium help[template-injection]: code injection via template expansion --> .github/workflows/reusable-wasi.yml:36:7 | 36 | - name: "Install WASI SDK" # Hard-coded to x64. | ------------------------ help: this step 37 | if: steps.cache-wasi-sdk.outputs.cache-hit != 'true' 38 | run: | | _______- 39 | | mkdir ${{ env.WASI_SDK_PATH }} && \ 40 | | curl -s -S --location https://github.com/WebAssembly/wasi-sdk/releases/download/wasi-sdk-${{ env.WASI_SDK_VERSION }}/wasi-sd... 41 | | tar --strip-components 1 --directory ${{ env.WASI_SDK_PATH }} --extract --gunzip | |________________________________________________________________________________________- help: env.WASI_SDK_PATH may expand into attacker-controllable code | = note: audit confidence → High help[template-injection]: code injection via template expansion --> .github/workflows/reusable-wasi.yml:36:7 | 36 | - name: "Install WASI SDK" # Hard-coded to x64. | ------------------------ help: this step 37 | if: steps.cache-wasi-sdk.outputs.cache-hit != 'true' 38 | run: | | _______- 39 | | mkdir ${{ env.WASI_SDK_PATH }} && \ 40 | | curl -s -S --location https://github.com/WebAssembly/wasi-sdk/releases/download/wasi-sdk-${{ env.WASI_SDK_VERSION }}/wasi-sd... 41 | | tar --strip-components 1 --directory ${{ env.WASI_SDK_PATH }} --extract --gunzip | |________________________________________________________________________________________- help: env.WASI_SDK_VERSION may expand into attacker-controllable code | = note: audit confidence → High help[template-injection]: code injection via template expansion --> .github/workflows/reusable-wasi.yml:36:7 | 36 | - name: "Install WASI SDK" # Hard-coded to x64. | ------------------------ help: this step 37 | if: steps.cache-wasi-sdk.outputs.cache-hit != 'true' 38 | run: | | _______- 39 | | mkdir ${{ env.WASI_SDK_PATH }} && \ 40 | | curl -s -S --location https://github.com/WebAssembly/wasi-sdk/releases/download/wasi-sdk-${{ env.WASI_SDK_VERSION }}/wasi-sd... 41 | | tar --strip-components 1 --directory ${{ env.WASI_SDK_PATH }} --extract --gunzip | |________________________________________________________________________________________- help: env.WASI_SDK_VERSION may expand into attacker-controllable code | = note: audit confidence → High help[template-injection]: code injection via template expansion --> .github/workflows/reusable-wasi.yml:36:7 | 36 | - name: "Install WASI SDK" # Hard-coded to x64. | ------------------------ help: this step 37 | if: steps.cache-wasi-sdk.outputs.cache-hit != 'true' 38 | run: | | _______- 39 | | mkdir ${{ env.WASI_SDK_PATH }} && \ 40 | | curl -s -S --location https://github.com/WebAssembly/wasi-sdk/releases/download/wasi-sdk-${{ env.WASI_SDK_VERSION }}/wasi-sd... 41 | | tar --strip-components 1 --directory ${{ env.WASI_SDK_PATH }} --extract --gunzip | |________________________________________________________________________________________- help: env.WASI_SDK_PATH may expand into attacker-controllable code | = note: audit confidence → High help[template-injection]: code injection via template expansion --> .github/workflows/reusable-wasi.yml:76:7 | 76 | - name: "Display build info" | -------------------------- help: this step 77 | run: make --directory ${{ env.CROSS_BUILD_WASI }} pythoninfo | ------------------------------------------------------------ help: env.CROSS_BUILD_WASI may expand into attacker-controllable code | = note: audit confidence → High help[template-injection]: code injection via template expansion --> .github/workflows/reusable-wasi.yml:78:7 | 78 | - name: "Test" | ------------ help: this step 79 | run: make --directory ${{ env.CROSS_BUILD_WASI }} test | ------------------------------------------------------ help: env.CROSS_BUILD_WASI may expand into attacker-controllable code | = note: audit confidence → High help[template-injection]: code injection via template expansion --> .github/workflows/build.yml:414:7 | 414 | - name: "Run tests" | ----------------- help: this step 415 | working-directory: ${{ env.CPYTHON_BUILDDIR }} 416 | run: | | _______- 417 | | # Most of the excluded tests are slow test suites with no property tests ... | 432 | | -x test_signal \ 433 | | -x test_sysconfig | |___________________________- help: env.VENV_PYTHON may expand into attacker-controllable code | = note: audit confidence → High 109 findings (89 suppressed): 0 unknown, 0 informational, 19 low, 0 medium, 1 highSummary:
cc @sethmlarson and zizmor author @woodruffw.