[hardwaresofton]

Dear everyone.

Never use pull_request_target.

This is not the first time it’s bitten people. It’s not safe, and honestly GitHub should have better controls around it or remove and rework it — it is a giant footgun.

> One of our engineers figured out this was because it triggered on: pull_request which means external contributions (which come from forks, rather than branches in the repo like internal contributions) would not have the workflow automatically run. The fix for this was changing the trigger to be on: pull_request_target, which runs the workflow as it's defined in the PR target repo/branch, and is therefore considered safe to auto-run.

[amluto]

Dear GitHub Actions: what the heck?

There are so many things about GitHub Actions that make no sense.

Why are actions configured per branch? Let me configure Actions somewhere on the repository that is not modifiable by some yml files that can exist in literally any branch. Let me have actual security policy for configuring Actions that is separate from permission to modify a given branch.

Why do workflows have such strong permissions? Surely each step should have defined inputs (possibly from previous steps), defined outputs, and narrowly defined permissions.

Why can one step corrupt the entire VM for subsequent steps?

Why is security almost impossible to achieve instead of being the default?

Why does the whole architecture feel like someone took something really simple (read a PR or whatever, possibly run some code in a sandbox, and produce an output) of the sort that could easily be done securely in JavaScript or WASM or Lua or even, sigh, Docker and decided to engineer it in the shape of an enormous cannon aimed directly at the user’s feet?

[black3r]

While I agree with the general sentiment that lots of things about GH actions don't make sense, when you actually look at what the vulnerability was, you'll find that for lots of your questions it wasn't GitHub Actions' fault.

This is the vulnerable workflow in question: https://github.com/PostHog/posthog/blob/c60544bc1c07deecf336...

> Why are actions configured per branch?

This workflow uses `pull_request_target` targeting where the actions are configured by the branch you're merging PR into, which should be safe - attacker can't modify the YML actions are running.

> Why do workflows have such strong permissions?

What permissions are workflow run with is irrelevant here, because the workflow runs the JS script with a custom access token instead of the permissions associated with the GH actions runner by default.

> Why is security almost impossible to achieve instead of being the default?

The default for `pull_request_target` is to checkout the branch you're trying to merge into (which again should be safe as it doesn't contain attacker's files), but this workflow explicitly checks out the attacker's branch on line 22.

[amluto]

Sure, that particular workflow is quite busted. But GitHub encourages this garbage design.

For example:

> GITHUB_TOKEN: ${{ secrets.POSTHOG_BOT_GITHUB_TOKEN }}

In no particular order:

- The use of secrets like this should be either entirely invalid or strongly, strongly discouraged. If allowed at all, there should be some kind of explicit approval required for the workflow step that gets to use the secret. And a file in the branch’s tree should not count as approval.

- This particular disaster should at least have been spelled something like ${{ dynamic_secret.assign_reviewer }} where that operation creates a secret that can assign a reviewer and nothing else and that lasts for exactly as long as the workflow step runs.

- In an even better design, there would be no secret at all. One step runs the script and produces output and has no permissions:

            - name: Run reviewer assignment script
              env:
                  PR_NUMBER: ${{ github.event.pull_request.number }}
                  GITHUB_REPOSITORY: ${{ github.repository }}
                  BASE_SHA: ${{ github.event.pull_request.base.sha }}
                  HEAD_SHA: ${{ github.event.pull_request.head.sha }}
              input: worktree ro at cwd
              output: reviewer from /tmp/reviewer
              run: |
                  node .github/scripts/assign-reviewers.js

I made up the syntax - that’s a read only view of “worktree” (a directory produced by a previous step) mounted at cwd. The output is a file at /tmp/reviewers that contains data henceforth known as “reviewer”. Then the next step isn’t a “run” — it’s something else entirely:

            - name: Assign the reviewer
              env:
                  PR_NUMBER: ${{ github.event.pull_request.number }}
                  REVIEWER: ${{ outputs.reviewer }}
              action: assign-reviewer

That last part is critical. This does not execute anything in the runner VM. It consumes the output from the previous step in the VM and then it … drumroll, please, because this is way too straightforward for GitHub … assigns the reviewer. No token, no secrets, no user-modifiable code, no weird side effects, no possibility of compromise due to a rootkit installed by a previous step into the VM, no containers spawned, no nothing. It just assigns a reviewer.

In this world, action workflows have no “write” permission ever. The repository config (actual config, not stuff in .github) gives a menu of allowed “actions”, and the owner checks the box so that this workflow may do “assign-reviewer”, and that’s it. If the box is checked, reviewers may be assigned. If not, they may not. Checking the box does not permit committing things or poisoning caches or submitting review comments or anything else - those are different checkboxes.

Oh, it costs GitHub less money, too, because it does not need to call out to the runner, and that isn’t free.

[nine_k]

A way to determine a workflow per branch, inside the branch, is useful for developing workflows. But it's perilous in other circumstances.

I wish I could, at the repo level, disable the use of actions from ./.github, and instead name another repo as the source of actions.

This could be achieved by defining a pre-merge-commit hook, and reject commits that alter protected parts of the tree. This would also require extra checks on the action runnes side.