feat: initial cache-poisoning audit #294

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 🙂

src/audit/cache_poisoning.rs

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

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

src/audit/cache_poisoning.rs

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

src/audit/cache_poisoning.rs

+
+        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 🙂

src/audit/cache_poisoning.rs

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 😄

docs/audits.md

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?

src/audit/cache_poisoning.rs

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

src/models.rs

+    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

src/audit/cache_poisoning.rs Show resolved Hide resolved

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

src/audit/cache_poisoning.rs Show resolved Hide resolved

woodruffw reviewed Dec 13, 2024

View reviewed changes

src/audit/cache_poisoning.rs Outdated Show resolved Hide resolved

woodruffw reviewed Dec 13, 2024

View reviewed changes

src/audit/cache_poisoning.rs Outdated Show resolved Hide resolved

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

src/audit/cache_poisoning.rs Show resolved Hide resolved

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

docs/audits.md Outdated Show resolved Hide resolved

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

6866b18

woodruffw reviewed Dec 17, 2024

View reviewed changes

src/audit/cache_poisoning.rs Outdated Show resolved Hide resolved

woodruffw reviewed Dec 17, 2024

View reviewed changes

src/audit/cache_poisoning.rs Outdated Show resolved Hide resolved

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 woodruffw:main Dec 19, 2024
5 checks passed

This was referenced Dec 19, 2024

New audit: cache poisoning #261

Closed

cache-poisoning: follow-up improvements #334

Open

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: initial cache-poisoning audit #294

feat: initial cache-poisoning audit #294

ubiratansoares commented Dec 13, 2024 •

edited

Loading

woodruffw commented Dec 13, 2024

woodruffw Dec 13, 2024

ubiratansoares commented Dec 16, 2024

woodruffw commented Dec 16, 2024

ubiratansoares commented Dec 17, 2024 •

edited

Loading

ubiratansoares commented Dec 17, 2024 •

edited

Loading

ubiratansoares Dec 17, 2024 •

edited

Loading

woodruffw Dec 19, 2024

woodruffw commented Dec 17, 2024

woodruffw commented Dec 19, 2024 •

edited

Loading

woodruffw left a comment


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

		if !self.trigger_used_when_publishing_artifacts(trigger) {

feat: initial cache-poisoning audit #294

feat: initial cache-poisoning audit #294

Conversation

ubiratansoares commented Dec 13, 2024 • edited Loading

woodruffw commented Dec 13, 2024

woodruffw Dec 13, 2024

Choose a reason for hiding this comment

ubiratansoares commented Dec 16, 2024

woodruffw commented Dec 16, 2024

ubiratansoares commented Dec 17, 2024 • edited Loading

ubiratansoares commented Dec 17, 2024 • edited Loading

ubiratansoares Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

woodruffw Dec 19, 2024

Choose a reason for hiding this comment

woodruffw commented Dec 17, 2024

woodruffw commented Dec 19, 2024 • edited Loading

woodruffw left a comment

Choose a reason for hiding this comment

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