Open Stack

Hi James,


> On 25 Jan 2021, at 17:22, Hunt, James O <james.o.hunt at 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:
> 
> 1) 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".
> 
> 2) 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_with_keyslurping_bots/ <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#pull_request
> [3] - https://docs.github.com/en/actions/reference/events-that-trigger-workflows#pull_request_target
> 
> ---
> 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 at redhat.com>
> Sent: Tuesday, 19 January 2021 08:02
> To: kata-dev at 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-requests.
> 
> 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 at 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.
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.katacontainers.io/pipermail/kata-dev/attachments/20210125/a50bd4c6/attachment-0001.html>