mvdnes / spin-rs

Spin-based synchronization primitives
MIT License
485 stars 92 forks source link

CI: Set minimal permissions on GitHub Workflow #140

Closed diogoteles08 closed 1 year ago

diogoteles08 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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