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

Pin bootstrap checksums and add a tool to update it automatically #88362

Merged
merged 2 commits into from
Sep 6, 2021

Conversation

pietroalbini
Copy link
Member

⚠️ ⚠️ This is just a proactive hardening we're performing on the build system, and it's not prompted by any known compromise. If you're aware of security issues being exploited please check out our responsible disclosure page. ⚠️ ⚠️


This PR aims to improve Rust's supply chain security by pinning the checksums of the bootstrap compiler downloaded by x.py, preventing a compromised static.rust-lang.org from affecting building the compiler. The checksums are stored in src/stage0.json, which replaces src/stage0.txt. This PR also adds a tool to automatically update the bootstrap compiler.

The changes in this PR were originally discussed in Zulip.

Potential attack

Before this PR, an attacker who wanted to compromise the bootstrap compiler would "just" need to:

  1. Gain write access to static.rust-lang.org, either by compromising DNS or the underlying storage.
  2. Upload compromised binaries and corresponding .sha256 files to static.rust-lang.org.

There is no signature verification in x.py as we don't want the build system to depend on GPG. Also, since the checksums were not pinned inside the repository, they were downloaded from static.rust-lang.org too: this only protected from accidental changes in static.rust-lang.org that didn't change the *.sha256 files. The attack would allow the attacker to compromise past and future invocations of x.py.

Mitigations introduced in this PR

This PR adds pinned checksums for all the bootstrap components in src/stage0.json instead of downloading the checksums from static.rust-lang.org. This changes the attack scenario to:

  1. Gain write access to static.rust-lang.org, either by compromising DNS or the underlying storage.
  2. Upload compromised binaries to static.rust-lang.org.
  3. Land a (reviewed) change in the rust-lang/rust repository changing the pinned hashes.

Even with a successful attack, existing clones of the Rust repository won't be affected, and once the attack is detected reverting the pinned hashes changes should be enough to be protected from the attack. This also enables further mitigations to be implemented in following PRs, such as verifying signatures when pinning new checksums (removing the trust on first use aspect of this PR) and adding a check in CI making sure a PR updating the checksum has not been tampered with (see the future improvements section).

Additional changes

There are additional changes implemented in this PR to enable the mitigation:

  • The src/stage0.txt file has been replaced with src/stage0.json. The reasoning for the change is that there is existing tooling to read and manipulate JSON files compared to the custom format we were using before, and the slight challenge of manually editing JSON files (no comments, no trailing commas) are not a problem thanks to the new bump-stage0.

  • A new tool has been added to the repository, bump-stage0. When invoked, the tool automatically calculates which release should be used as the bootstrap compiler given the current version and channel, gathers all the relevant checksums and updates src/stage0.json. The tool can be invoked by running:

    ./x.py run src/tools/bump-stage0
    
  • Support for downloading releases from https://dev-static.rust-lang.org has been removed, as it's not possible to verify checksums there (it's customary to replace existing artifacts there if a rebuild is warranted). This will require a change to the release process to avoid bumping the bootstrap compiler on beta before the stable release.

Future improvements

  • Add signature verification as part of bump-stage0, which would require the attacker to also obtain the release signing keys in order to successfully compromise the bootstrap compiler. This would be fine to add now, as the burden of installing the tool to verify signatures would only be placed on whoever updates the bootstrap compiler, instead of everyone compiling Rust.

  • Add a check on CI that ensures the checksums in src/stage0.json are the expected ones. If a PR changes the stage0 file CI should also run the bump-stage0 tool and fail if the output in CI doesn't match the committed file. This prevents the PR author from tweaking the output of the tool manually, which would otherwise be close to impossible for a human to detect.

  • Automate creating the PRs bumping the bootstrap compiler, by setting up a scheduled job in GitHub Actions that runs the tool and opens a PR.

  • Investigate whether a similar mitigation can be done for "download from CI" components like the prebuilt LLVM.

r? @Mark-Simulacrum

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 26, 2021
@Mark-Simulacrum
Copy link
Member

Ok, I've looked over this PR and it seems good to me.

I'm going to say r=me but hold off until we don't need this for the upcoming bump (in a couple days) for this release, since I don't want to accidentally interfere with that. Looking forward to setting up a GHA job (or similar) to auto-file PRs bumping bootstrap.

Can you also file a PR against forge with the updated release process? I believe in particular we'll need to reshuffle https://forge.rust-lang.org/release/process.html#promote-master-to-beta-t-2-days-tuesday since it needs to go after the full stable release now, right? I admit to feeling the pushing back there is a little unfortunate, but hopefully as we automate these steps more it'll be less of a hassle. Alternatively, we can consider returning the dev-static support -- without hashes stored in-tree.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 4, 2021
@pietroalbini
Copy link
Member Author

Opened rust-lang/rust-forge#566 to update the release process.

@Mark-Simulacrum
Copy link
Member

I've thought a little more about this and let's actually get this merged. If it causes any problems, it will be easy for me (or you, whatever) to revert when pushing changes anyway, so there's not much risk. beta/stable are already (virtually) branched, too.

@bors r+

@bors
Copy link
Contributor

bors commented Sep 6, 2021

📌 Commit ea8b1ff has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 6, 2021
@Mark-Simulacrum
Copy link
Member

@bors rollup=never

@bors
Copy link
Contributor

bors commented Sep 6, 2021

⌛ Testing commit ea8b1ff with merge 8ceea01...

@bors
Copy link
Contributor

bors commented Sep 6, 2021

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 8ceea01 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 6, 2021
@bors bors merged commit 8ceea01 into rust-lang:master Sep 6, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 6, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (8ceea01): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@pietroalbini pietroalbini deleted the bump-stage0 branch September 6, 2021 21:41
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Oct 13, 2021
fix the stage0 tools config file path in `config.toml.example`

in  rust-lang#88362  , the `stage0.txt ` have been switched to `stage0.json`  , but in `config.toml.example` the guide didn't change ,  this PR fix  this issue
@est31
Copy link
Member

est31 commented Oct 15, 2021

I've just seen this change now, and let me bump the dead thread to say that I really like it, for multiple reasons. The S3 bucket could be compromised, or there could be a TLS based MITM attack. There are hundreds of CAs in the root list and a single one of them has to make one mistake and issue a certificate to the wrong party that maybe even did some social engineering or such (or the CA is malicious outright, but this is less likely).

Regarding the gpg suggestion, I don't think it's needed to evolve it in that direction. Signatures are less secure than using hashes directly, as you could have signature keys stolen, hacked, sold, or just extorted. It's basically impossible to verify that this hasn't happened with a signature key. Plus there is the danger of gpg key expiry causing builds of old compilers to fail in the future. Last, the commonly used signature algorithms are not quantum secure, although there are quantum computer proof signature algorithms. Note that this is separate from considerations of using signatures in rustup. Here, the hashes of the binaries are not known in advance, so one has to resort to signatures.

Fun fact: back in the old days there existed a bootstrap key in stage0 but it got removed in #36548 to make upgrading the bootstrap compiler easier.

As to the format of using a json file, I'm wondering a bit, because rust usually uses toml for everything, including Cargo.lock. Right now there is a _comment field at the top which could be represented by a proper toml comment, if toml format is used (the toml crate doesn't support emitting comments but you can hack this via simple string concat). It would also save an additional dependency in the bump-stage0 tool on the serde_json crate (toml is already used). Would a PR that changes the json format to toml be welcomed?

@pietroalbini
Copy link
Member Author

As to the format of using a json file, I'm wondering a bit, because rust usually uses toml for everything, including Cargo.lock. Right now there is a _comment field at the top which could be represented by a proper toml comment, if toml format is used (the toml crate doesn't support emitting comments but you can hack this via simple string concat). It would also save an additional dependency in the bump-stage0 tool on the serde_json crate (toml is already used). Would a PR that changes the json format to toml be welcomed?

Given the purpose of the file and who's expected to access it, I think it's better to use JSON rather than TOML for src/stage0. The expected consumers of the file are:

  • x.py, which only has a hacky TOML parser that only accepts a small subset of TOML, while it has access to a proper JSON parser in Python's stdlib.
  • CI scripts and adjacent tooling: at least in Rust's infra those are usually written in Bash (jq is widely available) or Python (which has a parser in its stdlib), and support for TOML in those environments is minimal.

Also, humans are not expected to edit that file manually, so a lot of the limitations of JSON don't apply here.

@est31
Copy link
Member

est31 commented Oct 15, 2021

@pietroalbini I see, those are both great arguments. You convinced me that it's not a good idea :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants