Open Stack

Hi Christophe,

I guess we're going to all have to be as diligent as possible when it comes to reviewing PRs, to minimise the risks as far as possible.

# Manually triggering the problematic actions

As suggested in the GitHub article [1], I think manually triggering the two problematic actions is the best route. I've re-tested them and can confirm that the do both require the "pull_request_target" event; if you use the "pull_request" event, they both fail. The reasons for the failures are shown on the first of a set of PRs I'll be raising to add in the requirement that we trigger these actions manually (by adding the "safe-to-test" label) [2].

As tradition dictates, I've raised the first PR on the 1.x proxy repo; once that PR lands, I'll create a test PR in that repo so we can ensure the manual triggering works (I've already tested this in a separate testing GitHub org, but it doens't hurt to be careful ;) Once we're convinced, the test PR can be closed and I'll open PRs on all the other Kata repos to apply the same behaviour.

Note that assuming we agree with this approach and [2] lands, when a *new* PR is raised in that repo, the two GitHub actions shown will be marked with a "⊘" symbol [3]. That's GitHub's way of saying, "This action is pending". Once the "safe-to-test" label has been applied to the PR, it will auto-run the two actions in question.

# Refreshing secrets

We're also in the process of changing the GitHub "secret" used to run those actions. Since it's a security token, this is good practise anyway. But it will also have the pleasing side effect of not showing that my GH user is the one that's moving issues - it'll show that the Kata bot [4] is doing this instead ;)

Cheers,

James

[1] - https://securitylab.github.com/research/github-actions-preventing-pwn-requests
[2] - https://github.com/kata-containers/proxy/pull/300
[3] - Like this: https://github.com/jamesodhunt-org/test-gh-actions-2/pull/22
[4] - https://github.com/katacontainersbot

---
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: Monday, 25 January 2021 17:33
To: Hunt, James O
Cc: kata-dev at lists.katacontainers.io
Subject: Re: [kata-dev] GitHub actions security recommendations

Hi James,


On 25 Jan 2021, at 17:22, Hunt, James O <james.o.hunt at intel.com<mailto: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/ 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<mailto:dinechin at redhat.com>>
Sent: Tuesday, 19 January 2021 08:02
To: kata-dev at lists.katacontainers.io<mailto: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<mailto: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.


---------------------------------------------------------------------
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.