Replace Secret's Drop supertrait with ZeroizeOnDrop#36
Conversation
…ment DefaultIsZeroes for SecurityStrength
| fn is_full_entropy(&self) -> bool; | ||
|
|
||
| fn zeroize(&mut self); | ||
|
|
There was a problem hiding this comment.
I still think that KeyMaterial should still expose an API to force it to zeroize itself mid-usage, for example if the user want to make sure that it was fully zeroized before writing a smaller key (or a key of unknown size) into it and make sure that none of the original key material is still in the unused part of the buffer.
| fn default() -> Self { | ||
| Self::None | ||
| } | ||
| } |
There was a problem hiding this comment.
Can you explain this change?
What is the purpose of adding a ::default() to SecurityStrenght?
(I'm not necessarily saying that it's wrong, I just don't see why this would be used)
| // I disagree because I want to force things that are secrets to manually implement Drop that zeroizes the data. | ||
| // So I'm turning off this lint. | ||
| pub trait Secret: Drop + Debug + Display {} | ||
| pub trait Secret : ZeroizeOnDrop + Debug + Display {} |
| @@ -173,7 +172,12 @@ mod test_key_material { | |||
| #[test] | |||
| fn zeroize() { | |||
| let mut key = KeyMaterial256::from_bytes(&DUMMY_KEY[..32]).unwrap(); | |||
There was a problem hiding this comment.
I think we should put this API back, which means this test will need to be run twice; once for the .zeroize() and once for the drop_in_place()
|
|
||
| /// A matrix over the ML-DSA ring. | ||
| #[derive(Clone)] | ||
| #[derive(Clone, ZeroizeOnDrop)] |
There was a problem hiding this comment.
This shouldn't need to be Zeroized because Polynomial is where the secret data is, and it is where we should impl ZeroizeOnDrop.
| #[derive(Clone)] | ||
| use zeroize::ZeroizeOnDrop; | ||
|
|
||
| #[derive(Clone, ZeroizeOnDrop)] |
There was a problem hiding this comment.
This shouldn't need to be Zeroized because Polynomial is where the secret data is, and it is where we should impl ZeroizeOnDrop.
| pub(crate) vec: [Polynomial; k], | ||
| } | ||
|
|
||
| #[derive(Clone, ZeroizeOnDrop)] |
There was a problem hiding this comment.
This shouldn't need to be Zeroized because Polynomial is where the secret data is, and it is where we should impl ZeroizeOnDrop.
| } | ||
|
|
||
| impl<const k: usize, PK: MLKEMPublicKeyInternalTrait<k, PK_LEN>, const SK_LEN: usize, const PK_LEN: usize> | ||
| ZeroizeOnDrop for MLKEMPrivateKey<k, PK, SK_LEN, PK_LEN> {} |
There was a problem hiding this comment.
Could you explain for my understanding why here you provide an empty impl, but in other places you use the #derive macro?
| bouncycastle-sha3.workspace = true | ||
| bouncycastle-rng.workspace = true | ||
| bouncycastle-utils.workspace = true | ||
| zeroize = { version = "1.9.0", features = ["zeroize_derive"] } |
There was a problem hiding this comment.
Can you please add this to the workspace dependencies in the top-level cargo.toml, so that when we eventually want to bump the version, we only need to do it in one place and not all over?
I'm fairly new to cargo, but I think you can do version = "1.9", and that gives it permission to take 1.9.1, 1.9.2, etc when they become available?
| bouncycastle-sha3.workspace = true | ||
| bouncycastle-rng.workspace = true | ||
| bouncycastle-utils.workspace = true | ||
| zeroize = { version = "1.9.0", features = ["zeroize_derive"] } |
There was a problem hiding this comment.
Can you please add this to the workspace dependencies in the top-level cargo.toml, so that when we eventually want to bump the version, we only need to do it in one place and not all over?
I'm fairly new to cargo, but I think you can do version = "1.9", and that gives it permission to take 1.9.1, 1.9.2, etc when they become available?
There was a problem hiding this comment.
If it's not useful, it should be removed in a separate PR.
| self.buf[self.key_len..new_key_len].copy_from_slice(other.ref_to_bytes()); | ||
| self.key_len += other.key_len(); | ||
| self.key_type = min(&self.key_type, &other.key_type()).clone(); | ||
| self.security_strength = min(&self.security_strength, &other.security_strength()).clone(); |
There was a problem hiding this comment.
Why did you suddenly change min/max? If you want to have a prompt review, I suggest sticking with one topic, which is at this point Zeroize.
It's also a good idea to lay commits in reasonable chunks, for example
crypto/core
crypto/mlsda
crypto/mlkem
...
| KeyType::Zeroized | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
-#[derive(Clone, Copy, Debug, Eq, PartialEq)]
+#[derive(Clone, Copy, Debug, Eq, PartialEq, Default)]
pub enum KeyType {
+ #[default]
/// The KeyMaterial is zeroized and MUST NOT be used for any cryptographic operation in this state.
Zeroized,
@@ -206,12 +206,6 @@ pub enum KeyType {
impl DefaultIsZeroes for KeyType {}
-impl Default for KeyType {
- fn default() -> Self {
- KeyType::Zeroized
- }
-}| fn is_full_entropy(&self) -> bool; | ||
|
|
||
| fn zeroize(&mut self); | ||
|
|
| /// Input: integer array 𝐹 ∈ ℤ_M^256, where 𝑚 = 2^𝑑 if 𝑑 < 12, and 𝑚 = 𝑞 if 𝑑 = 12. | ||
| /// Output: byte array 𝐵 ∈ 𝔹32𝑑. | ||
| /// Note: this is exposed publicly only for testing purposes and there is no good reason to use it in production code. | ||
| pub fn byte_encode<const d: usize, const PACK_LEN: usize>(F: &Polynomial) -> [u8; PACK_LEN] { |
There was a problem hiding this comment.
Is there any driver for making the function public?
| /// Output: integer array 𝐹 ∈ ℤ256 , where 𝑚 = 2𝑑 if 𝑑 < 12 and 𝑚 = 𝑞 if 𝑑 = 12. | ||
| pub(crate) fn byte_decode<const d: usize, const PACK_LEN: usize>(B: &[u8; PACK_LEN]) -> Polynomial { | ||
| /// Note: this is exposed publicly only for testing purposes and there is no good reason to use it in production code. | ||
| pub fn byte_decode<const d: usize, const PACK_LEN: usize>(B: &[u8; PACK_LEN]) -> Polynomial { |
| /// Output: array 𝑎_hat ∈ ℤ256 ▷ the coefficients of the NTT of a polynomial | ||
| pub(crate) fn sample_ntt(rho: &[u8; 32], nonce: &[u8; 2]) -> Polynomial { | ||
| /// Note: this is exposed publicly only for testing purposes and there is no good reason to use it in production code. | ||
| pub fn sample_ntt(rho: &[u8; 32], nonce: &[u8; 2]) -> Polynomial { |
| } | ||
| } | ||
| 3 => { | ||
| for i in 0..N / 4 { |
There was a problem hiding this comment.
All formatting issues were fixed upstream/release/0.1.2alpha. Try rebase please.
|
|
||
| mod aux_functions; | ||
| // Exposed only for testing purposes | ||
| pub mod aux_functions; |
4 participants
Description
First pass integrating the zeroize crate.
Currently only the structs that are already implementing Secret are refactored. The intention is that it allows the PR to be relatively small and also settle on the pattern to be followed before full propagation.
Issue
Relates to: #32