Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How can 'github.ref_name' be attacker controlled? #85

Closed
ecnepsnai opened this issue Oct 31, 2024 · 6 comments
Closed

How can 'github.ref_name' be attacker controlled? #85

ecnepsnai opened this issue Oct 31, 2024 · 6 comments
Labels
question Further information is requested

Comments

@ecnepsnai
Copy link

Given the following example workflow:

name: "Example"

on:
  push:
    tags: [ "*" ]

permissions:
  actions: read
  contents: read

jobs:
  build:
    name: "Example"
    runs-on: ubuntu-latest
    steps:
      - name: Test
        run: |
          echo "${{ github.ref_name }}"

The use of github.ref_name will be flagged as being possibly attacker controlled, but I'm genuinely curious how?

My understanding is that this value will only refer to either a commit SHA, or a tag name. People without commit rights can't commit tags, so wouldn't that only leave a commit SHA?

@ecnepsnai
Copy link
Author

(Great tool, by the way. Workflow security is such an interesting topic to me)

@woodruffw
Copy link
Owner

Hi @ecnepsnai, thanks for the report and the kind words!

We currently flag github.ref_name because it can be either the tag or branch name when it isn't a SHA ref. In typical workflows the tag won't be attacker controlled, but the branch name could potentially be.

(More generally however, it's also difficult to prove that a tag ref can't be attacker controlled -- a user could theoretically allow external tag creation through a workflow or other automation, and subsequent expansions in a template context would then be attacker controlled.)

TL;DR: we consider it potentially attacker controllable since it can in theory expand to something besides a SHA ref, even if in typical scenarios a dangerous expansion is unlikely.

With that being said, I'm curious if you think this is too sensitive -- one possibility would be lowering the severity for this particular context access, or potentially gating it behind --pedantic (although the latter would require some more thought IMO).

@woodruffw woodruffw added the question Further information is requested label Oct 31, 2024
@ecnepsnai
Copy link
Author

Ahh I see, I forgot that the ref name could be a branch, and yeah I see how that is attacker controlled.

As for being too sensitive, in my specific example I would say it is, but my example isn't a good representation, I wonder if perhaps zizmor could look at the triggers and determine if its likely to be a risk, and use that to determine if it should or shouldn't flag it?

@woodruffw
Copy link
Owner

As for being too sensitive, in my specific example I would say it is, but my example isn't a good representation, I wonder if perhaps zizmor could look at the triggers and determine if its likely to be a risk, and use that to determine if it should or shouldn't flag it?

I think this is a great idea, but I don't have a good sense yet for how to make the divisions here 😅 -- one option would be to distinguish between "safe" and "unsafe" workflows (e.g. github.ref_name is probably fine to expand in a push or release event), but these can also be pretty context-sensitive or even indeterminate (e.g. workflow_call is generally unsafe, but there might be a hard-to-analyze condition that limits calls to only safe callers).

I'll leave this open to see if other folks have thoughts on addressing this more generally -- at full generality this looks a lot like the big picture problem of zizmor doing the equivalent of whole program dataflow/control flow, but for GitHub Actions 🙂

@funnelfiasco
Copy link
Contributor

I didn't see this issue when I started discussion #102, but I think the best (from a "make zizmor broadly useful" perspective) approach is to use the WIP ignore functionality in #116 to exclude the cases that you believe to be reasonably safe.

@woodruffw
Copy link
Owner

Closing in favor of ongoing discussion in #102.

Also for viz: #127 will reduce template-injection's sensitivity by making it aware of safe vs. unsafe expressions. This won't directly impact the github.ref_name case, but it's the first step towards actual flow analysis here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants