Hacker News Reader

Comment by woodruffw | Hacker News Reader

This is a great example of why `pull_request_target` is fundamentally insecure, and why GitHub should (IMO) probably just remove it outright: conventional wisdom dictates that `pull_request_target` is "safe" as long as branch-controlled code is never executed in the context of the job, but these kinds of argument injections/local file inclusion vectors demonstrate that the vulnerability surface is significantly larger.

At the moment, the only legitimate uses of `pull_request_target` are for things like labeling and auto-commenting on third-party PRs. But there's no reason for these actions to have default write access to the repository; GitHub can and should be able to grant fine-grained or (even better) single-use tokens that enable those exact operations.

(This is why zizmor blanket-flags all use of `pull_request_target` and other dangerous triggers[1]).

[1]: https://docs.zizmor.sh/audits/#dangerous-triggers

View on HN

I don't disagree... but, there is a use case for orgs that don't allow forks. Some tools do their merging outside of github and thus allow for PRs that cannot be clean from a merge perspective. This won't trigger workflows that are pull_request. Because pull_request requires a clean merge. In those cases pull_request_target is literally the only option.

The best move would be for github to have a setting for allowing the automation to run on PRs that don't have clean merges, off by default and intended for use with linters only really. Until that happens though pull_request_target is the only game in town to get around that limitation. Much to my and other SecDevOps engineers sadness.

NOTE: with these external tools you absolutely cannot do the merge manually in github unless you want to break the entire thing. It's a whole heap of not fun.

That's a fantastic use case that should be supported discretely!

Why github didn't is beyond me. Even if something isn't merge clean doesn't mean linters shouldn't be run. I get not running deployments etc. but not even having the option is pain.

Inside private repos we use pull_request_target because 1. it runs the workflow as it exists on main and therefore provides a surface where untampered with test suites can run, and 2. provides a deterministic job_workflow_ref in the sub claim in the jwt that can be used for highly fine grained access control in OIDC enabled systems from the workflow

Private repos aren't as much of a concern, for obvious reasons.

However, it's worth noting that you don't (necessarily) need `pull_request_target` for the OIDC credential in a private repo: all first-party PRs will get it with the `pull_request` event. You can configure the subject for that credential with whatever components you want to make it deterministic.

You’re right! I edited my comment to clarify I was talking about good ole job_workflow_ref.

This is what GitHub says about it:

> This event runs in the context of the base of the pull request, rather than in the context of the merge commit, as the pull_request event does. This prevents execution of unsafe code from the head of the pull request that could alter your repository or steal any secrets you use in your workflow.

Which is comical given how easily secrets were exilfiltrated.

Yeah, I think that documentation is irresponsibly misleading: it implies that (1) attacker code execution requires the attacker to be able to run code directly (it doesn't, per this post), and (2) that checking out at the base branch somehow stymies the attacker, when all it does is incentivizes people to check out the attacker-controlled branch explicitly.

GitHub has written a series of blog posts[1] over the years about "pwn requests," which do a great job of explaining the problem. But the misleading documentation persists, and has led to a lot of user confusion where maintainers mistakenly believe that any use of `pull_request_target` is somehow more secure than `pull_request`, when the exact opposite is true.

[1]: https://securitylab.github.com/resources/github-actions-prev...

This attack surface is essentially unfixed for almost a year now.

Remember the python packages that got pwned with a malicious branch name that contained shellshock like code? Yeah, that incident.

I blogged about all vulnerable variables at the time and how the attack works from a pentesting perspective [1].

[1] https://cookie.engineer/weblog/articles/malware-insights-git...

Reply on news.ycombinator.com