Repository navigation
bugfix: cache-poisoning: fix false positive for docker/setup-buildx-action - #644
Conversation
crazy-max
left a comment
There was a problem hiding this comment.
Nice work thanks!
Left a comment about runtime artifacts
| error[cache-poisoning]: runtime artifacts potentially vulnerable to a cache poisoning attack | ||
| --> @@INPUT@@:20:9 | ||
| | | ||
| 15 | / with: | ||
| 16 | | cache-binary: true | ||
| 17 | | version: latest | ||
| | |_________________________^ opt-in for caching here | ||
| 18 | | ||
| 19 | - name: Build docker | ||
| 20 | uses: docker/build-push-action@48aba3b46d1b1fec4febb7c5d0c644b249a11355 | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ runtime artifacts usually published here | ||
| | |
There was a problem hiding this comment.
Indeed we are publishing the build record as GitHub artifact in post step of the build-push-action: https://github.com/docker/build-push-action/blob/84ad562665bb303b549fec655d1b64f9945f3f91/src/main.ts#L208-L214
But there could be false positives. If env vars DOCKER_BUILD_SUMMARY=false or DOCKER_BUILD_RECORD_UPLOAD=false are set, then no artifact is uploaded: https://github.com/docker/build-push-action/tree/master?tab=readme-ov-file#environment-variables
There are also other tricky cases where build summary would be disabled and therefore no artifact will be uploaded: https://github.com/docker/build-push-action/blob/84ad562665bb303b549fec655d1b64f9945f3f91/src/main.ts#L170-L180 but I think these are edge cases.
Env var could also be tricky to detect in some cases like internal script using core.exportVariable or append to GITHUB_ENV.
There was a problem hiding this comment.
Thanks for calling this out! Yeah, I don't have a fantastic way to model implicit controls like environment variables quite yet -- it should be doable in the same basic way, but I need to think more through the complexity implications 🙂
In the mean time, this is one of the reasons this finding is marked as "low confidence" -- we only issue high-confidence cache poisoning findings when the workflow trigger itself is unambiguously a publish trigger, i.e. tag or release.
There was a problem hiding this comment.
In the mean time, this is one of the reasons this finding is marked as "low confidence"
Ah ok got it, didn't know that
…ction (zizmorcore#644) * bugfix: cache-poisoning: fix false positive for docker/setup-buildx-action Fixes zizmorcore#642. * cleanup * update jdx/mise-action coordinate * release-notes: record changes
This is an initial stab at exposing greater generality when evaluating whether a set of controls are "satisfied" for the purpose of considering an action enabled.
Doing this while also preserving the concept of "default enablement" results in some very wonky 4-valued logic, since there are 4 different states a control can evaluate to:
This is a little hard to follow, so I need to think about this more before actually merging it. However, the tests do pass and I've also added a successful test for the original bug case that spurred this.
Fixes #642.