Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: address security risks in GitHub Actions workflows #1651

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

AdnaneKhan
Copy link

The recent pull request mentions I don't understand what dangers there might be with cache: yarn but I guess we'll find out!.

It turns out, this adds a lot of danger. An attacker can compromise the NPM token and the Chrome extension release secrets fairly easily by leveraging a technique known as GitHub Actions cache poisoning.

This happens because the style-check.yml workflow runs on pull_request_target. I've updated the workflows to do a few things:

  • Remove cache consumption in release job
  • Tighten GITHUB_TOKEN permissions in other workflows that consume the cache

Ideally, you should move away from running user-controlled code from forks on pull_request_target, but that requires a more substantial overhaul to the repository's CI. The changes here will make it impossible to directly compromise the release secrets.

Some background on cache poisoning:

Copy link

changeset-bot bot commented Feb 6, 2025

⚠️ No Changeset found

Latest commit: 569d028

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@AdnaneKhan AdnaneKhan changed the title Address security risks in GitHub Actions workflows fix: address security risks in GitHub Actions workflows Feb 6, 2025
@Juice10
Copy link
Contributor

Juice10 commented Feb 7, 2025

Thanks for sharing this @AdnaneKhan and putting in the time to find this exploit, and find the recent PR. We have been using cache: yarn for a while now on some of the other GitHub actions in the repo, my recent change was to add cache in the spots we were missing. I'm guessing those caches also need removing/having their permissions reduced. Would you agree?

@AdnaneKhan
Copy link
Author

Thanks for sharing this @AdnaneKhan and putting in the time to find this exploit, and find the recent PR. We have been using cache: yarn for a while now on some of the other GitHub actions in the repo, my recent change was to add cache in the spots we were missing. I'm guessing those caches also need removing/having their permissions reduced. Would you agree?

That’s your decision - so the root cause (that allows an exploit) here is the workflow that runs code on pull_request_target from fork PRs combined with the release workflow consuming the cache. Ideally, you should look at overhauling the CI so that you use the pull_request trigger (not pull_request_target) to run the untrusted code, then use a second workflow on workflow_run to upload the results/format code, etc. This pattern is called out in https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/. Thats a more involved change, though.

The changes in this PR protect your release secrets from immediate risk. Even if someone did try a cache poisoning attack they would not be able to cause real harm (such as stealing the NPM secret - which we can agree would lead to a big miss if rrweb was backdoored).

I can work on contributing a follow-up PR to design a more robust release process for the project once this one is implemented. Better to remove the immediate risk first, though.

Copy link
Contributor

@Juice10 Juice10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all these suggested changes, I went through them to try to understand and double check everything. Looks really good, I thought it would make sense to go as strict as we can since we're looking at it right now so I suggested some stricter permissions

.github/workflows/ci-cd.yml Outdated Show resolved Hide resolved
.github/workflows/style-check.yml Outdated Show resolved Hide resolved
.github/workflows/style-check.yml Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants