search

apache / beam

Apache Beam is a unified programming model for Batch and Streaming data processing.
https://beam.apache.org/
Apache License 2.0
7.85k stars 4.25k forks source link

[Task]: CI: Review dangerous use of pull_request_target #27470

Closed diogoteles08 closed 1 year ago

diogoteles08 commented 1 year ago

What needs to happen?

Motivation

I'm creating this issue to bring your attention some dangerous workflow patterns currently present on your project involving the usage of pull_request_target along with a actions/checkout, which inevitably open doors to run untrusted code. There are 14 occurrences that seem to follow the very same algorithm, as the beam_PreCommit_Go.yml or beam_PreCommit_Website.yml , and another file with different purpose: build_runner_image.yml . All of them were identified using the Scorecard tool.

As a general rule, GitHub itself recommends that a pull_request_target trigger shouldn't be used along with a checkout, as you can see on this warning displayed on the link to the trigger:

image

Although this is a very strong recommendation by itself, I'll try to give some concretes examples on how this pattern could be dangerous specifically to your code.

  1. For file beam_PreCommit_Go.yml and similars, I saw that you take the caution to set most of the permissions as read or None, but you still have the actions: write permission. This may seem safe, but as you can see on its documentation, it can be used to approve the run of some other malicious action through the endpoint POST /repos/{owner}/{repo}/actions/runs/{run_id}/approve. Also, at the line 46 you are running a local script after your checkout, which could be totally changed by a malicious PR and abuse any permission or secrets you may spoil.
  2. At the file build_runner_image.yml you are not directly calling any script that could be changed, but you are using some critical secrets and pushing a docker image based on a image stored on the repository. I'm not sure exactly how the pushed image is being used, but an attacker could probably change the docker image to whatever they want. And given the image is being built at the same environment that the secrets were used, probably we can't have warranty that the image couldn't extract them.

Possible remediations

There simply isn't a single nor a right solution to this, so we may want to evaluate the motivations behind the two jobs to say what's the best path to follow, but it follows some of the general ideas on how to proceed:

Divide the workflow in two, so that the pull_request_target is done separately from the checkout

We usually can think of ways to split the workflow to avoid this dangerous pattern. Looking at the beam_PreCommit_Go.yml code, does the rerun-job-action really need to happen at the same workflow as the following steps of the workflow (e.g "Install Java", "Setup Gradle")? If not, the rerun-job-action can be called in a workflow with write permissions, and then use workflow_run to trigger another workflow to run the other steps without write permissions.

Thinking even further on the pre_commit_go case, maybe you could have a single workflow with the job of calling the rerun-job-action and then use information about the trigger of this job to router a run of another workflow to use checkout and run the correspondent preCommit script.

Use label verification

We can use a type: [labeled] and a condition of if: ${{ github.event.label.name == 'is ok to test' }} to check for a label "is ok to test" for example, that you would manually add once you saw that nothing potentially dangerous would be running.

This still isn't 100% save, because one can incautiously label a unsafe workflow as "ok to test", but it's safer.

Finally, I'm available to help find a good solution for this, and even contribute with the PR if wanted.

Thanks for the attention!

Additional context

I'm Diogo and I work on Google's Open Source Security Team(GOSST) in cooperation with the Open Source Security Foundation (OpenSSF). My core job is to suggest and implement security changes on widely used open source projects 😊

Issue Priority

Priority: 2 (default / most normal work should be filed as P2)

Issue Components

diogoteles08 commented 1 year ago

Hey! This issue/PR has been idle for quite some time. I'm following up here because this issue is still valid and it's considerably dangerous. Trying to summarize my (extensive) original issue, the main concerns are: 1. the beam_PreCommit_Go.yml file can possibly be abused to leak the secrets.GE_CACHE_PASSWORD secret and the others listed on this code section. The workflow triggered by pull_request_target is run on the code of the PRs, and the script run on this line can be altered to print the secrets, that are inserted as ENV variables.

  1. The build_runner_image.yml can possibly be abused to push a malicious docker image to the registry apache-beam-testing/beam-github-actions/beam-arc-runner . When triggered by pull_request_target, the workflow would run on the code of the malicious PR who triggered it, which could alter the code of the docker file that is built and pushed around these lines of code.

In general, both cases fit into the patterns named as Dangerous Workflow by Scorecard, which exposes this as a Critical risk, the most critical between the Scorecard checks.

EDIT: After some discussions, I've concluded that actually the beam_PreCommit_Go.yml file and similars are not vulnerable workflows. At the text right above the "Conclusion" of this blogpost, we can understand that the actions/checkout only actually checkouts to the PR code if you use the ref: ${{ github.event.pull_request.head.sha }} input, which was removed from the code on this commit. That said, the beam_PreCommit_Go.yml is not vulnerable anymore, but the build_runner_image.yml is still vulnerable.

damccorm commented 1 year ago

I'm moving that workflow to pull_request

kennknowles commented 1 year ago

Awesome that's great. And thanks very much for the report and analysis @diogoteles08. It is tricky to have a usable CI that interacts with cloud resources and we appreciate the attention you gave to ours.