Comment by ssanderson11235 | Hacker News Reader

Comment by ssanderson11235 | Hacker News Reader

Parent Full thread

ssanderson11235

The fundamental mistake here seems to have been not fully understanding the threat model of the pull_request_target action trigger.

pull_request_target jobs run in response to various events related to a pull request opened against your repo from a fork (e.g, someone opens a new PR or updates an existing one). Unlike pull_request jobs, which are read-only by default, pull_request_target jobs have read/write permissions.

The broader permissions of pull_request_target are supposed to be mitigated by the fact that pull_request_target jobs run in a checkout of your current default branch rather than on a checkout of the opened PR. For example, if someone opens a PR from some branch, pull_request_target runs on `main`, not on the new branch. The compromised action, however, checked out the source code of the PR to run a benchmark task, which resulted in running malicious attacker-controlled code in a context that had sensitive credentials.

The GHA docs warn about this risk specifically:

> Running untrusted code on the pull_request_target trigger may lead to security vulnerabilities. These vulnerabilities include cache poisoning and granting unintended access to write privileges or secrets.

They also further link to a post from 2021 about this specific problem: https://securitylab.github.com/resources/github-actions-prev.... That post opens with:

> TL;DR: Combining pull_request_target workflow trigger with an explicit checkout of an untrusted PR is a dangerous practice that may lead to repository compromise.

The workflow authors presumably thought this was safe because they had a block setting permissions.contents: read, but that block only affects the permissions for GITHUB_TOKEN, which is not the token used to interact with the cache. This seems like the biggest oversight in the existing GHA documentation/api (beyond the general unsafety of having pull_request_target at all). Someone could (and presumably did!) see that block and think "this job runs with read-only permissions", which wasn't actually true here.

What I don't get is how the GitHub Action cache is shared between unprotected and protected refs. Is that really the case?

Why even have protected branch rules when anyone with write access to an unprotected branch can poison the Action cache and compromise the CI on the next protected branch run?

In GitLab CI caches are not shared between unprotected and protected runs.

Static analyzers like https://github.com/zizmorcore/zizmor can help find such misconfiguration. It is however unfortunate, that such footguns aren't harder to fire.

Many thanks for sharing this. I wasn't aware it existed.

From a GitHub product owner POV, if the architecture is not to be changed, what is the solution?

A big ugly warning in the UI?

Or, push back on the architecture?

Or, is threatening a big ugly warning in the UI actually pushing back on the architecture?

Many projects kind of take a different approach where for pull requests CI is not run until approvals from maintainers are given even for very simple jobs to avoid untrusted code running in ci.

> A big ugly warning in the UI?

There's already a warning in the docs. There's no UI to put the warning in that isn't going to be visible until it's too late. And even that warning isn't scary enough - this documentation is buried behind a "warning" in the docs and then two more links to get to the meat.

> Workflows triggered via pull_request_target have write permission to the target repository. They also have access to target repository secrets. The same is true for workflows triggered on pull_request from a branch in the same repository, but not from external forks. The reasoning behind the latter is that it is safe to share the repository secrets if the user creating the PR has write permission to the target repository already.

> pull_request_target runs in the context of the target repository of the PR, rather than in the merge commit. This means the standard checkout action uses the target repository to prevent accidental usage of the user supplied code.

So what this means is if you use `pull_request_target`, the jobs have read and write access to privileged data in the repo (including secrets) and the code the job runs is controlled by the target.

> Or, push back on the architecture?

Personally, I would advocate to remove this feature for public repositories. It has ~zero legitimate use cases. If it needs to come back, it should be an error to run jobs on this trigger if the user that initiated it doesn't have write permissions for the repo.

If this breaks CI pipelines that is a good thing. Those pipelines are just waiting to be pwned.

There's a PR open to mitigate this on actions/cache but I don't believe it's actually solving the root cause, which is in the design of actions itself.

Reply on news.ycombinator.com