Zeroize types/fields containing secrets #1492
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## main #1492 +/- ##
==========================================
- Coverage 96.49% 96.46% -0.03%
==========================================
Files 74 74
Lines 15110 15110
==========================================
- Hits 14580 14576 -4
- Misses 530 534 +4
... and 6 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@ctz The CI results seem promising. Is this something that could be reviewed if it were rebased on main or are there still aspects in flux? |
|
Yes I plan to rebase & prepare this for review. |
ctz
force-pushed
the
jbp-zeroize
branch
3 times, most recently
from
8da4904
to
e05fc67
Compare
|
#1544 should mean I can drop the last couple of commits on this PR. Then I think it will be ready. |
This is not always a secret quantity, but treating it as such covers zeroisation on `OkmBlock`, and hence tls13::key_schedule. It also covers some intermediate values in hkdf computations.
So we can wrap it in `zeroize::Zeroizing`
This avoids having them as 'loose' unzeroized type on the way to being moved to their final home. This is sufficient, because: - tls12: the secret comes from `tls12::ConnectionSecrets::master_secret()` which borrows from its internal storage; `tls12::ConnectionSecrets::drop` zeroes this storage. - tls13: the secret comes from `resumption_master_secret_and_derive_ticket_psk`, of type `hkdf::OkmBlock`, which we borrow from. Only once the borrow finishes will that be zeroized.
djc
approved these changes
Oct 19, 2023
cpu
approved these changes
Oct 19, 2023
FYI @aochagavia this PR might be a good case study for the new method of performance measurement -- instruction counts show a small (and expected) bump, but might cause an outsized impact on real-world performance. |
This was referenced Oct 27, 2023
github-merge-queue bot
pushed a commit
to rustls/rcgen
that referenced
this pull request
Dec 9, 2023
Having types from dependent crates appear in rcgen's public API makes taking semver incompatible updates to those dependencies tricky: we need to treat it as a semver incompatible change to rcgen itself. To make sure whenever this type leak happens that it was done because of a deliberate choice this branch adds [cargo-check-external-types](https://github.com/awslabs/cargo-check-external-types) to CI. Three existing types that leaked into the API are fixed: `ring::error::KeyRejected`, `ring::error::Unspecified`, and `pem::PemError`. In each case it's simple to translate the specific error types from the dependency into our own `Error` variants that don't expose the type. Two types are allow-listed: `time::offset_date_time::OffsetDateTime` and `zeroize::Zeroize`. The first, `OffsetDateTime` is used throughout the public API. It isn't clear to me if we want to keep that as part of the public API and treat `time` updates carefully, or if we should pursue using a more generic representation. I've left this out for now so it can be discussed. The second, `Zeroize`, could probably be avoided by implementing `Drop` and calling `zeroize` on the sensitive fields manually. That's the approach Rustls implemented (rustls/rustls#1492). I've left this out for in case there was a reason to prefer implementing the trait on the public types.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is for #265. After the Microsoft incident publicised a couple of months ago, I think I changed my mind about the relative importance of this. It also goes hand-in-hand with having more types containing secrets, introduced in the recent crypto provider work.
closes #265