New Audit unsound-contains: Checks for problematic contains() usage

[Holzhaus]

See https://github.com/woodruffw/zizmor/pull/577/files?short_path=927c421#diff-927c421dd1ad1dea67595c286c6dd91d883ca0cd0ddb34413383e035ca2d2a55 for details.

[woodruffw]

Hi @Holzhaus, thanks for opening this. Two comments:

1. In the future, could you open up a discussion for new audits before starting PRs for them? I typically have a handful of questions about audit ideas, and it's good to be able to talk through the proposal before code gets put down 🙂
2. In this specific case: do you have real-world examples of this causing a vulnerability? It makes sense to me as a potential risk, but it'd be good to have concrete real-world examples so I can figure out how much of a tradeoff it is (as is, this will be very noisy, since it flags contexts regardless of whether they're attacker-controlled or not).

[woodruffw]

I'm having trouble following this example; is it supposed to be this instead?

Suggested change

	However, this condition will not only evaluate to `true` if either
	`refs/heads/main` or `refs/heads/develop` is passed, but also for substrings of
	those values. For example, if someone pushes to a branch named `main`, then
	`github.ref` would contain the string `refs/heads/mai` and the job would also
	executed.
	However, this condition will not only evaluate to `true` if either
	`refs/heads/main` or `refs/heads/develop` is passed, but also for substrings of
	those values. For example, if someone pushes to a branch named `mai`, then
	`github.ref` would contain the string `refs/heads/mai` and the job would also
	executed.

(NB: This points to a threat model question: GitHub mostly treats branches on the same repo as having equivalent access, so flagging this kind of thing by default might be really noisy for ordinary users. It might be good to instead give these a "pedantic" or "auditor" persona, since we can't uniformly assume that they're always problematic.)

[Holzhaus]

Yes, sorry for the confusion, that was a typo. I actually found another typo, i'll push an update.

This points to a threat model question: GitHub mostly treats branches on the same repo as having equivalent access, so flagging this kind of thing by default might be really noisy for ordinary users.

This is true for many open-source projects. In professional settings, I've mostly seen repos where all developers work on the same repo (rathen than forks) and push to feature branches. Often, the main branch is guarded by branch protection rules (e.g., requires a PR, Approvals, Code Owner Reviews). So this is an actual security issue for such cases.

It might be good to instead give these a "pedantic" or "auditor" persona, since we can't uniformly assume that they're always problematic.

I initially made a list of particularily problematic contexts (e.g., github.ref) and only flagged those. But on second thought, I removed this because I couldn't come up with a single legitimate use case where you want to check if a context var is contained in a string rather than a list of strings. And in the latter case you should be using the fromJSON approach.

[woodruffw]

I removed this because I couldn't come up with a single legitimate use case where you want to check if a context var is contained in a string rather than a list of strings. And in the latter case you should be using the fromJSON approach.

Yeah, good point. Thanks for explaining!

[Holzhaus]

Hi @Holzhaus, thanks for opening this. Two comments:

1. In the future, could you open up a discussion for new audits before starting PRs for them? I typically have a handful of questions about audit ideas, and it's good to be able to talk through the proposal before code gets put down 🙂

Sure.

2. In this specific case: do you have real-world examples of this causing a vulnerability? It makes sense to me as a potential risk, but it'd be good to have concrete real-world examples so I can figure out how much of a tradeoff it is

Yes, I have actually seen this in the real world, but it's not an open-source project so I cannot disclose more details.

(as is, this will be very noisy, since it flags contexts regardless of whether they're attacker-controlled or not)

I'm not sure if this will be actually that noisy. I only matches contains("some string", context.var) and I struggle to find a use case for this. At this point, it will not detect the inverse (i.e. contains(github.event.pull_request.title, "foo") which might be more widely used.

I'll check if there is some convenient way to use the GitHub API for downloading a bunch of random workflow files and see if this audit triggers any false positives.

[Holzhaus]

In this specific case: do you have real-world examples of this causing a vulnerability?

I've found some publicly visible real world examples on GitHub. For obvious reasons I don't want to post them there, but I'll provide them via email to you.

I tried to find some code snippets that would be detected by the audit and that are okay to post here because they are not exploitable:

• https://github.com/anyoptimization/pymoo-doc/blob/ababa0198f8032c0b27e69493d491a7d7f4944de/.github/workflows/doc.yml#L40-L43
  Checks if github.ref is in a string, but in this case not a security issue because they filtered branches via the on.push.branches trigger, so you cannot just push to any branch.
• https://github.com/ZupIT/horusec-platform/blob/46e61fc85de39f8cb3354350f4571e5c9c8eff36/.github/workflows/release-beta.yml#L35
  Using contains("...", github.actor) is in this case probably not a security issue because the workflow is triggered by workflow dispatch only so you can't just trigger the workflow from a newly registered user where the name is a substring.
• https://github.com/pyinstaller/pyinstaller-hooks-contrib/blob/e07f485b609ff95aad91f46e72f8b6aa71a89533/.github/workflows/release.yml#L12
  Not a security issue for the same reason as the as the previous one.

[woodruffw]

Thanks @Holzhaus, this is shaping up really well! I did an initial review pass on it and left some thoughts.

(Apologies for the delay; I'll have more time for reviews in the coming days.)

[woodruffw]

Thanks @Holzhaus, this is looking really good to me. Two small nitpicks and then I'll do a final pass review.

(Edit: also, the lint is failing!)

[Holzhaus]

(Edit: also, the lint is failing!)

Must be a version mismatch, cargo fmt does not make any changes on my local machine. Using rustfmt 1.8.0-stable (4eb161250e 2025-03-15).

[woodruffw]

Must be a version mismatch, cargo fmt does not make any changes on my local machine. Using rustfmt 1.8.0-stable (4eb161250e 2025-03-15).

Could you try rebasing or fast-forwarding this branch onto the latest main? It's probably because main is now using the latest Rust edition, while this branch isn't; you'll probably then need to do rustup update or similar.

[woodruffw]

@Holzhaus let me know if you're planning on resuming this -- if not, I'll probably revive it on a new PR and credit you. Thanks!

[Holzhaus]

Yes, I will have a look later today. Sorry, totally forgot about it.

[woodruffw]

Nitpick: this should work now, since expr::Function has PartialEq<&str> with the right case folding:

Suggested change

	Expr::Call { func, args: exprs } if func.0.eq_ignore_ascii_case("contains") => {
	Expr::Call { func, args: exprs } if func == "contains" => {

[woodruffw]

Suggested change

	| Workflow | [bypassable-contains-conditions.yml] | v1.6.0 | ✅ | ✅ |
	| Workflow | [bypassable-contains-conditions.yml] | v1.7.0 | ✅ | ✅ |

[woodruffw]

NB: This also needs a new Configurable column like the current docs, see e.g.:

https://woodruffw.github.io/zizmor/audits/#secrets-inherit

[woodruffw]

Thanks @Holzhaus, this looks really great to me! I left two small nitpicks, but otherwise I think this is good to go.

One thought: how do you feel about giving this a shorter name, something like bypassable-contains or unsound-contains? I think the current name is descriptive but might be a bit of a mouthful/cause a lot of reflowing on smaller terminals. Curious if either of those sounds good to you or you have other name ideas 🙂

[Holzhaus]

Hi @woodruffw, thanks. I integrated your suggestions and renamed the audit from bypassable-contains-conditions to unsound-contains for brevity.

[woodruffw]

Thanks @Holzhaus! I'll do a final pass tonight and hopefully merge then. I appreciate the time and effort you've put into this and the other PRs you've sent!

[woodruffw]

Merged, thanks again!