Hi James,
On 25 Jan 2021, at 17:22, Hunt, James O james.o.hunt@intel.com wrote:
Hi Christophe,
I've checked and we do actually have a couple of GitHub actions using the "pull_request_target" event:
Thanks for taking the time to follow up!
- Action to enforce the PR porting labels [1]
- Action to move all issues to "in progress" when a PR references them [2]
Note that both these actions are in *all* our repos.
# What is the "pull_request_target" event?
If you want a GitHub acction to trigger when a pull request is opened, the obvious choice seems to be the "pull_request" event. However, GitHub documents in [2] that:
Pull request events for forked repositories Note: Workflows do not run on private base repositories when you open a pull request from a forked repository.
That's a problem because all the PRs the community raises are on "forked repositories".
Enter the "pull_request_target" event. The event *does* work with "forked repositories" and allows actions to run in the context of the PR rather than the main repo. However, quoting from [3],
Warning: The pull_request_target event is granted a read/write repository token and can access secrets, even when it is triggered from a fork.
This seems somewhat odd given the event appears to have been created to make actions safer and you have to wonder if GitHub might introduce a new event that sits somewhere between "pull_request" and "pull_request_target" at some point.
Taking each of our actions in turn...
# Porting labels action
## What is it for?
The action checks to ensure that the PR is correctly labelled with respect to porting labels [3]. Note that the action is ultimately only checking to ensure the PR has a number of labels set.
## Can we survive without it?
I'd say yes. The action is only reminding the community of the need to perform the porting - it cannot enforce it due to the complexities of bi-directional porting and timing [3].
## Can the action be run without "pull_request_target"?
I think it could. I'm going to test this and see if we can change the event to "pull_request" instead...
# Move issue to in progress label
## What is it for?
The action is triggered when a PR is opened and if that PR has an associated (linked) issue number (aka a "fixes #XXX" comment), that issue is resolved and moved into the issue backlog GitHub board [1]. GitHub allows us to move newly opened issue into the project automatically, but the GitHub API does not support "linked (issues|prs)" well, hence the need for the action.
## Can we survive without it?
Possibly. This was added specifically for the project board [1]. But since creating it, we have been hampered somewhat by GitHub's very restricted set of queries (filters) that can be run in a project. In fact, the weekly issue backlog "scrub" doesn't use that project - it uses a simple GitHub query (which is not restriected in the filters that can be applied).
Ariel may wish to comment on this further.
## Can the action be run without "pull_request_target"?
afaik, no it cannot. That event is required to allow access to the PR. However, I'll re-test to see if that is still the case...
# TL;DR
The all important summary ;) fwics, a "rogue PR" that attempts to install a bad action that does naughty things won't actually get run until it is itself merged in the main branch.
The bad code would only run on the *next* PR raised after the bad PR was merged. This is good and bad. However, looking on the positive side, it seems that if we want to keep these two actions there are two relatively easy changes we can make:
- Adopt GitHub's suggestion to make our actions only trigger when labelled with something like `safe-to-test`
This effectively makes the actions "manually triggered".
- Rotate the GitHub secrets (effectively the "uid" the actions run as) and check they have the absolute minimum set of privileges.
To me, there are two categories of rogue PR:
1/ The ones you describe, which try to install bad stuff in the infrastructure, typically a github action. I believe you have covered that relatively exhaustively in your report. I agree with your analysis. I would add that we might be able to flag any commit that targets github actions with a specific label. Make sure we have eyes on them.
2/ PRs that take advantage of the testing infrastructure to execute malicious code disguised as unit tests. In a chat with Ariel, I described that as a "unit test" doing some bitcoin mining. This was the case I was more concerned with in the context of a discussion about merging test and code repos. Frankly, I'm surprised we don't get more spammers trying to do this kind of things, given that our testing infrastructure runs containers as well as unit tests that could do practically anything.
I suspect that what makes this hard today is the relatively low traffic in terms of number of PRs. We would notice bad PRs rapidly during review. It's probably much more beneficial to attempt to get access to AWS instances (see https://www.theregister.com/2015/01/06/dev_blunder_shows_github_crawling_wit... https://www.theregister.com/2015/01/06/dev_blunder_shows_github_crawling_with_keyslurping_bots/ for an example). But I can't completely shake the idea now that I had it.
Thanks Christophe
Cheers,
James
[1] - https://github.com/orgs/kata-containers/projects/23 [2] - https://docs.github.com/en/actions/reference/events-that-trigger-workflows#p... [3] - https://docs.github.com/en/actions/reference/events-that-trigger-workflows#p...
https://katacontainers.io/ | https://github.com/kata-containers Open Source Technology Center Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ.
From: Christophe de Dinechin dinechin@redhat.com Sent: Tuesday, 19 January 2021 08:02 To: kata-dev@lists.katacontainers.io Subject: [kata-dev] GitHub actions security recommendations
Hello,
I don't know if you are aware of the security recommendations in this paper: https://securitylab.github.com/research/github-actions-preventing-pwn-reques....
I thought this was interesting notably in the context of the discussion regarding unifying testing/CI and code repos. While it is true that a single repo makes it possible to commit a test along with the change, it also means a malicious PR now has a much larger attack surface, something that I personally was not aware of.
Food for thought.
Thanks, Christophe
kata-dev mailing list kata-dev@lists.katacontainers.io http://lists.katacontainers.io/cgi-bin/mailman/listinfo/kata-dev
Intel Corporation (UK) Limited Registered No. 1134945 (England) Registered Office: Pipers Way, Swindon SN3 1RJ VAT No: 860 2173 47
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.