feat: initial cache-poisoning audit by ubiratansoares · Pull Request #294 · zizmorcore/zizmor

ubiratansoares · 2024-12-13T22:20:17Z

A first iteration on #261

There is a lot to unpack in this PR already, so I added several inline comments, and I'll go for docs as soon as we are good with the overal design 🙂

First consideration : for now, I'm matching as triggers for this audit only release and push (with tags filters).
I'm aware that more scenarios exist : I myself use another pattern (on top of workflow_dispatch), happy to get input and iterate, or eventually tackle them as a follow-up. I miss a check for the default branch of the repo, but since it requires querying the Github API, I've skipped it for now.

Second consideration: because of the multitude of caching-aware Actions that exist (and how they behave), and because we should aim to consume a static API at some point in the future, I've modelled a CacheAwareAction struct, which is the very core of this PR.

In general, I tried to capture with this data holder:

that some actions will run caching behavior by default, both for saving and restoring (eg actions/setup-go)
that some actions have an input that allows either with opt-in or opt-out from restoring cache (eg actions/cache)
that some actions define opt-in not by specifying a boolean input, but actually a set of values (eg actions/setup-node)
eventually the user opts-in for caching on top of an expression: there is not much to do, but I keep track of that

There is a static list of CacheAwareAction, covering all Actions provided by Github and a few others. Happy to add as many as we want to have 🔥

Third consideration: I defined a strategy for this audit on top of a chat I had with Adnan, focusing on the unzip part of cache-poisoning attacks. This means I'm not considering actions/cache/save in the Actions we inspect for.

Most of the Actions I've listed bring both saving/restoring out-of-the-box, but some of them define a lookup-only input (as per actions/cache and actions/cache/restore do), that only queries the Github API for cache entries but do not download any caches. Maybe this is incomplete/insufficient, and I'd love to get inputs here!

Last but not least : I added several examples of vulnerable Workflows and snapshot test all of them.

Most likely I missed other things, but hopefully this PR sesa a place to get started 🙂

woodruffw · 2024-12-13T22:59:46Z

Thanks @ubiratansoares! I'll try and do an initial pass on this today or tomorrow.

woodruffw · 2024-12-13T23:50:52Z

+
+        let trigger = &step.workflow().on;
+
+        if !self.trigger_used_when_publishing_artifacts(trigger) {


This looks good for now, thanks!

Thinking out loud: another future proxy we could use for "is this a publishing workflow" is whether or not the workflow contains any well-known publishing actions, e.g. gh-action-pypi-publish.

Not something to do with this iteration, but worth thinking about 🙂

ubiratansoares · 2024-12-16T23:25:25Z

@woodruffw Addressed your suggestions, let me know whether I missed something! Docs to follow 🙂

woodruffw · 2024-12-16T23:45:46Z

Thanks! I think that's everything -- just undraft and give me a ping when you're ready for a final review!

ubiratansoares · 2024-12-17T09:33:43Z

@woodruffw Added some docs here, let me know what you think :)

Some follow-ups from this PR, so we can keep its size manageable for the review:

Evaluate the presence of well-know publish-like Actions (e.g. gh-action-pypi-publish) in addition to the trigger-based evaluation
Improve evaluation for push trigger with well-know branch paths (eg, release-* branches) ?
Add more Actions in the list of well-know cache-aware Actions 😄

ubiratansoares · 2024-12-17T14:31:04Z

Added this additional commit to highlight another (potential) follow-up : should we evaluate step.if to avoid some false positives, since the user can avoid cache restoration by disabling the a cache-aware step?

ubiratansoares · 2024-12-17T21:59:02Z

+    pub(crate) fn as_template(&self) -> String {
+        format!("{}/{}", self.owner, self.repo)
+    }


Created this new method as a helper, since I need to get a template from what we have in well-know list, essentially due the asymmetrical nature of RepositoryUses::matches

I don't love that we have to re-build the slug here, but that's my fault for building a bad abstraction here 😅 -- I'll try and clean this up as part of the final review.

woodruffw · 2024-12-17T22:05:45Z

Thanks! I'll give this a final pass tomorrow, but this is looking great to me!

Let's track the follow-ups in #294 (comment) with a new issue 🙂

Signed-off-by: William Woodruff <william@yossarian.net>

Allow `RepositoryUses::matches` to take `impl TryInto<RepositoryUses>` instead, which has an auto-derived infallible impl for `RepositoryUses` itself. Signed-off-by: William Woodruff <william@yossarian.net>

GHA doesn't know the difference between `true` and `'true'`, so neither should we. Signed-off-by: William Woodruff <william@yossarian.net>

Signed-off-by: William Woodruff <william@yossarian.net>

woodruffw · 2024-12-19T01:31:39Z

ee7d227 (#294) tweaks the audit slightly so that it catches our own problematic pypi.yml workflow. I need to think about how best to fix that, plus how best to ensure this audit sees our fix.

Signed-off-by: William Woodruff <william@yossarian.net>

woodruffw

LGTM, great work @ubiratansoares!

ubiratansoares commented Dec 13, 2024

View reviewed changes

Comment thread src/audit/cache_poisoning.rs

ubiratansoares force-pushed the ufs/patch-12 branch from 259018b to a67519e Compare December 13, 2024 22:32

woodruffw reviewed Dec 13, 2024

View reviewed changes

Comment thread src/audit/cache_poisoning.rs

woodruffw reviewed Dec 13, 2024

View reviewed changes

Comment thread src/audit/cache_poisoning.rs Outdated

woodruffw reviewed Dec 13, 2024

View reviewed changes

Comment thread src/audit/cache_poisoning.rs Outdated

woodruffw reviewed Dec 13, 2024

View reviewed changes

woodruffw added enhancement New feature or request new-audit New audits labels Dec 13, 2024

woodruffw reviewed Dec 15, 2024

View reviewed changes

Comment thread src/audit/cache_poisoning.rs

ubiratansoares added 3 commits December 16, 2024 21:41

feat: initial iteration on cache-poisoning audit

881de9f

Improves overall design of CacheAwareAction

85475ab

Adds PyO3/maturin-action to well-know cache-aware Actions

accbdbd

ubiratansoares force-pushed the ufs/patch-12 branch from a67519e to accbdbd Compare December 16, 2024 22:46

Adds docs

f707162

ubiratansoares marked this pull request as ready for review December 17, 2024 09:33

ubiratansoares requested a review from woodruffw December 17, 2024 09:33

ubiratansoares commented Dec 17, 2024

View reviewed changes

Comment thread docs/audits.md Outdated

Marks if clauses at cache-aware steps as a follow-up

6866b18

woodruffw reviewed Dec 17, 2024

View reviewed changes

Comment thread src/audit/cache_poisoning.rs Outdated

woodruffw reviewed Dec 17, 2024

View reviewed changes

Comment thread src/audit/cache_poisoning.rs Outdated

Parsing templates for uses steps with proper Uses APIs

c935683

ubiratansoares commented Dec 17, 2024

View reviewed changes

woodruffw added 2 commits December 19, 2024 00:55

Merge branch 'main' into ufs/patch-12

3518b33

cache_poisoning: mark primary loc

6868a90

Signed-off-by: William Woodruff <william@yossarian.net>

woodruffw added 3 commits December 19, 2024 01:13

remove RepositoryUses::as_template

1b92605

Allow `RepositoryUses::matches` to take `impl TryInto<RepositoryUses>` instead, which has an auto-derived infallible impl for `RepositoryUses` itself. Signed-off-by: William Woodruff <william@yossarian.net>

cache_poisoning: tweak bool handling

ee7d227

GHA doesn't know the difference between `true` and `'true'`, so neither should we. Signed-off-by: William Woodruff <william@yossarian.net>

clippy

f2baa18

Signed-off-by: William Woodruff <william@yossarian.net>

woodruffw added 2 commits December 19, 2024 02:44

tests: add a boolish test

a1c4ed5

Signed-off-by: William Woodruff <william@yossarian.net>

docs: tweak

f379432

Signed-off-by: William Woodruff <william@yossarian.net>

woodruffw changed the title ~~feat: initial iteration on cache-poisoning audit~~ feat: initial cache-poisoning audit Dec 19, 2024

woodruffw enabled auto-merge (squash) December 19, 2024 03:40

woodruffw approved these changes Dec 19, 2024

View reviewed changes

woodruffw merged commit 10e9fe3 into zizmorcore:main Dec 19, 2024

This was referenced Dec 19, 2024

New audit: cache poisoning #261

Closed

cache-poisoning: follow-up improvements #334

Closed

BrewTestBot mentioned this pull request Dec 19, 2024

zizmor 0.10.0 Homebrew/homebrew-core#201803

Merged

ubiratansoares mentioned this pull request Dec 20, 2024

feat: detects well-known publisher actions in cache-audit #338

Merged


		let trigger = &step.workflow().on;

		if !self.trigger_used_when_publishing_artifacts(trigger) {

Uh oh!

Uh oh!

Conversation

ubiratansoares commented Dec 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

woodruffw commented Dec 13, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

woodruffw Dec 13, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ubiratansoares commented Dec 16, 2024

Uh oh!

woodruffw commented Dec 16, 2024

Uh oh!

ubiratansoares commented Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ubiratansoares commented Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ubiratansoares Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

woodruffw Dec 19, 2024

Choose a reason for hiding this comment

Uh oh!

woodruffw commented Dec 17, 2024

Uh oh!

woodruffw commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

woodruffw left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ubiratansoares commented Dec 13, 2024 •

edited

Loading

ubiratansoares commented Dec 17, 2024 •

edited

Loading

ubiratansoares commented Dec 17, 2024 •

edited

Loading

ubiratansoares Dec 17, 2024 •

edited

Loading

woodruffw commented Dec 19, 2024 •

edited

Loading