[BUG]: How to address "opt-in for caching here"?

[ashishb]

Pre-submission checks

• I am not filing a feature request. These should be filed via the feature request form instead.
• I have looked through the open issues for a duplicate report.

Expected behavior

When Docker images are built (but not pushed), cached artifacts are harmless.
How do I disable https://woodruffw.github.io/zizmor/audits/#cache-poisoning for those cases?

Actual behavior

error[cache-poisoning]: runtime artifacts potentially vulnerable to a cache poisoning attack
  --> ...
   |
34 | /         with:
35 | |           cache-binary: true
   | |____________________________^ opt-in for caching here
36 |
37 |         - name: Build docker
38 |           uses: docker/build-push-action@v6
   |           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ runtime artifacts usually published here
   |

Reproduction steps

Create a new action

      - name: Set up Docker Buildx
        uses: docker/setup-buildx-action@v3
        with:
          cache-binary: true

      - name: Build docker
        uses: docker/build-push-action@v6
        with:
          cache-from: type=gha
          cache-to: type=gha,mode=max
          push: false

Logs

No response

Additional context

No response

[woodruffw]

Thanks for the report @Ashish!

To make sure I understand: is the false positive happening because push: false disables the publish, but we don't detect that?

If that sounds right to you, I think what we need to do here is better filter the publish actions in a manner similar to the cache-aware actions. CC @ubiratansoares for thoughts 🙂

[ashishb]

To make sure I understand: is the false positive happening because push: false disables the publish, but we don't detect that?

Yes, I am not an expert on caching but I do believe that push: false does kill this attack.

[ubiratansoares]

@ashishb Thanks for reporting!

I agree with @woodruffw, this looks like a false positive, and to proper handle it, we need to enhance what we have for publisher-like Actions (like docker/build-push-action), so we model whether publishing can be disabled or ran in some sort of dry-run mode.

For now, I think the easiest way to disable this check is using zizmor's support to ignoring results.

[danweller18]

I ran into a very similar error- I think is a false positive for the same reason. The underlying problem appears to be not taking context into account for the cache poisoning. In my example I have configured the build-push-action to use the unique github action run ID to benefit from using the cache. However this means it should not be vulnerable to cache poisoning across different runs/branches

error:

error[cache-poisoning]: runtime artifacts potentially vulnerable to a cache poisoning attack
34 |           uses: docker/setup-buildx-action@v3
   |           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cache enabled by default here

Reproduction steps

- name: Set up Docker Buildx
  uses: docker/setup-buildx-action@v3

- name: Build and push to registries
  uses: docker/build-push-action@v6
  with:
    cache-from: type=gha,ref=${{ github.run_id }}
    cache-to: type=gha,ref=${{ github.run_id }},mode=max

[woodruffw]

Ah yeah, that's a challenging case: we likely won't ever have the fidelity needed to fully inspect that kind of cache key configuration. But I'll think some more about that as well 🙂

[ashishb]

And I believe that's ok. The goal isn't to handle all edge cases but to reduce obvious false positives.

[woodruffw]

Yep. One possibility there is that we can encode personas with each selector, so we'll only flag on that more complicated case when the user selects an "auditor" persona.

I'll have time to work on this (first the simple cases, then the more nuanced ones) in the coming days.

[woodruffw]

Thought about this a bit more:

I think the solution here is going to be to generalize CacheAwareAction into a UsesCoordinate or similar, which could then be used whenever we need "this uses: step satisfies a set of conditions." That would allow us to use it for both cache-aware steps and publishing steps.

[woodruffw]

#389 is the final piece here. I'll open up a separate follow-up for #378 (comment), since that requires a yet-higher layer of generality.