Skip to content
This repository has been archived by the owner on Jan 3, 2024. It is now read-only.

security: validate hash of fetched binary #169

Merged
merged 3 commits into from
Apr 22, 2021
Merged

security: validate hash of fetched binary #169

merged 3 commits into from
Apr 22, 2021

Conversation

jesec
Copy link
Contributor

@jesec jesec commented Apr 18, 2021

Recently there are several high-profile supply chain attacks [1][2]. This project, starred by over 15k developers with over 5k dependents, could be an ideal target of such attack.

Additionally, we ship prebuilt patched Node binaries, and users usually use them instead of compiling their own. This makes the problem worse, as it is more difficult to spot malicious binary.

We are making progress by building binaries with a public CI service (Github Actions). The build processes are now transparent and auditable. Plus, even repo admins / org owners can not tamper the artifacts and logs of a Github Actions run.

However, we maintainers do have the privileges to "Edit Release", and unfortunately, at the moment Github does not allow viewing the change history of release assets or locking down release assets. As a result, if one of us got compromised, the hacker can potentially seed malicious binaries to our users and end-users.

Note that this is not a problem that is unique to us. Other projects such as nexe host binaries on Github, as well, and they have not deployed any defense.

Currently, I post the GPG-signed hashes to the release page, so our users can validate binaries, and we can easily spot a compromise. However, most users probably wouldn't do that. And, this makes me the "trust anchor". While I take every precaution, I am not confident enough to say that I can defend my private key against state-sponsored hackers.

This change hardcodes the expected hashes of binaries and makes pkg-fetch validate hash of fetched binary. This mechanism makes the changes of binaries transparent and defeats compromise of control of release assets.

With this change, the release flow:

  1. Run binary workflows
  2. Downloads binaries from Github Actions, gets hashes and commits the hashes
  3. Bump the version, tag the revision, download binaries, upload binaries to Github Releases, release to npm

[1]: https://about.codecov.io/security-update
[2]: https://www.phoronix.com/scan.php?page=news_item&px=PHP-Git-Compromised

@jesec jesec requested review from leerob and robertsLando April 18, 2021 13:39
@leerob
Copy link
Member

leerob commented Apr 18, 2021

I love this write-up. Great explanation and brilliant idea.

'5206878079f160e75a02ad33b7559b4a869e8181ee03d51d7211b52995f9ca7b',
'node-v8.17.0-macos-x64':
'dffa71e39100f4daa57de73fda7b4debecd09f552b15cf11854c8475380d3817',
'node-v8.17.0-win-x64':
Copy link
Member

Choose a reason for hiding this comment

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

Should we auto-generate these so it doesn't require a PR for every release? Or not doable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The whole point is to hardcode the hashes in the source code. So we do have to make a commit.

You can get the hashes simply by cat *.sha256sum after downloading from Github Actions. Then, it is simple RegEx (to convert to object).

To ease the process, I can make the CI print out the hashes in the log.

Copy link
Contributor

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

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

Well done @jesec ! LGTM

jesec added a commit that referenced this pull request Apr 22, 2021
This reverts commit e9c2efa.

We don't want to waste resources. Plus, with #169, it is mandatory
to bump minor if there is any change of binary.
jesec added a commit that referenced this pull request Apr 22, 2021
This reverts commit e9c2efa.

We don't want to waste resources. Plus, with #169, it is mandatory
to bump minor if there is any change of binary.
@jesec jesec merged commit 629cc71 into master Apr 22, 2021
@jesec jesec deleted the pr/validate-hash branch April 22, 2021 14:54
jesec added a commit that referenced this pull request Apr 22, 2021
This reverts commit e9c2efa.

We don't want to waste resources. Plus, with #169, it is mandatory
to bump minor if there is any change of binary.
jesec added a commit that referenced this pull request Apr 23, 2021
This reverts commit e9c2efa.

We don't want to waste resources. Plus, with #169, it is mandatory
to bump minor if there is any change of binary.
jesec added a commit that referenced this pull request Apr 23, 2021
This reverts commit e9c2efa.

We don't want to waste resources. Plus, with #169, it is mandatory
to bump minor if there is any change of binary.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants