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

CI: Set minimal permissions on GitHub Workflow #140

Closed
diogoteles08 opened this issue Mar 6, 2023 · 3 comments · Fixed by #141
Closed

CI: Set minimal permissions on GitHub Workflow #140

diogoteles08 opened this issue Mar 6, 2023 · 3 comments · Fixed by #141

Comments

@diogoteles08
Copy link
Contributor

Hello!

I'd like to suggest the definition of minimal permissions on your workflow, as it would harden your security agains supply-chain attacks. I see that you have only one workflow, the rust.yml, but it does not specify the permissions for its jobs, letting their privileges determined by GitHub's defaults. By defining minimal permissions you would be secured against erroneous or malicious actions from external jobs you call from your workflow. It's specially important for the case they get compromised, for example.

Setting minimum permissions for workflows is recommended by GitHub itself and also by other security tools, such as Scorecards and StepSecurity.

Let me know what you think about this. I'd be happy to raise a PR with the changes if you agree.

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 😊

@zesterer
Copy link
Collaborator

zesterer commented Mar 6, 2023

I'd be interested in seeing a PR, although this crate's CI doesn't meaningfully interact with the outside world and I personally check every PR before approving a CI run, so I think the opportunities for exploitation are limited.

@diogoteles08
Copy link
Contributor Author

Hey Joshua, thanks for the reply. I'll be creating the PR shortly.

this crate's CI doesn't meaningfully interact with the outside world and I personally check every PR before approving a CI run, so I think the opportunities for exploitation are limited.

Your reviews on every PRs are really great, indeed help on the security on the repo and you should definitely continue doing that! But on the specific case of workflows external jobs it might not be enough, because the exploitation could actually come from changes on the code of the job you called.

To exemplify, note that on the rust.yml file you have uses: taiki-e/install-action@cargo-hack. This part is running code from a version of the action defined on the https://github.com/taiki-e/install-action repo. This could be dangerous for your repo because:

  1. AFAIU by reading the taiki-e repo, calling uses: taiki-e/install-action@cargo-hack does not pin the version of the code, so you would be always running the latest version. This means that you would be exposed in case this repo releases a compromised or erroneous code.
  2. If you don't declare the permissions, you might be running the external code with write permissions. So the code could run git commit and git push towards your repo, for example.
  3. Depending on your Branch protection settings, the push commands could succeed and push malicious code into you main branch.

The change that I initially purposed (updating workflow permissions) would already prevent this flow by limiting the permissions on the workflows. Additionally, my example went through some minor security risks that I would also be available and happy to help solving. Those would be:

  1. Specify the version of every external job you run.
  2. Determine branch protection rules for you main branch. E.g., there is a rule that requires PRs to merge code into the branch, preventing malicious direct pushes

Let me know your thoughts

@zesterer
Copy link
Collaborator

zesterer commented Mar 7, 2023

As mentioned, I'm happy to review a PR that changes workflow permissions. Thanks for your time.

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 a pull request may close this issue.

2 participants