Replace Secret's Drop supertrait with ZeroizeOnDrop

Cargo.toml

@@ -27,6 +27,7 @@ bouncycastle-utils = { path = "./crypto/utils", version = "0.1.1" }
 
 # *** External Dependencies ***
 criterion = "0.8.2" # only for benchmarking, not used in lib or cli build
+zeroize = { version = "1.9", features = ["zeroize_derive"] }
 
 [profile.release]
 lto = "thin"

crypto/core/Cargo.toml

@@ -5,6 +5,7 @@ edition.workspace = true
 
 [dependencies]
 bouncycastle-utils.workspace = true
+zeroize.workspace = true
 
 [dev-dependencies]
 bouncycastle-rng.workspace = true

crypto/core/src/key_material.rs

@@ -44,6 +44,8 @@ use bouncycastle_utils::{ct, min};
 use core::cmp::{Ordering, PartialOrd};
 use core::fmt;
 
+use zeroize::{DefaultIsZeroes, Zeroize, ZeroizeOnDrop};
+
 /// Sometimes you just need a zero-length dummy key.
 pub type KeyMaterial0 = KeyMaterial<0>;
 
@@ -173,8 +175,6 @@ pub trait KeyMaterialTrait {
 
     fn is_full_entropy(&self) -> bool;
 
-    fn zeroize(&mut self);
-
     /// Is simply an alias to [KeyMaterialTrait::set_key_len], however, this does not require [KeyMaterialTrait::allow_hazardous_operations]
     /// since truncation is a safe operation.
     /// If truncating below the current security strength, the security strength will be lowered accordingly.
@@ -199,7 +199,7 @@ pub trait KeyMaterialTrait {
 /// A wrapper for holding bytes-like key material (symmetric keys or seeds) which aims to apply a
 /// strict typing system to prevent many kinds of mis-use mistakes.
 /// The capacity of the internal buffer can be set at compile-time via the <KEY_LEN> param.
-#[derive(Clone)]
+#[derive(Clone, Zeroize, ZeroizeOnDrop)]
 pub struct KeyMaterial<const KEY_LEN: usize> {
     buf: [u8; KEY_LEN],
     key_len: usize,
@@ -210,8 +210,9 @@ pub struct KeyMaterial<const KEY_LEN: usize> {
 
 impl<const KEY_LEN: usize> Secret for KeyMaterial<KEY_LEN> {}
 
-#[derive(Clone, Copy, Debug, Eq, PartialEq)]
+#[derive(Clone, Copy, Debug, Default, Eq, PartialEq)]
 pub enum KeyType {
+    #[default]
     /// The KeyMaterial is zeroized and MUST NOT be used for any cryptographic operation in this state.
     Zeroized,
 
@@ -231,6 +232,8 @@ pub enum KeyType {
     SymmetricCipherKey,
 }
 
+impl DefaultIsZeroes for KeyType {}
+
 impl<const KEY_LEN: usize> Default for KeyMaterial<KEY_LEN> {
     /// Create a new empty (zeroized) instance.
     fn default() -> Self {
@@ -515,12 +518,6 @@ impl<const KEY_LEN: usize> KeyMaterialTrait for KeyMaterial<KEY_LEN> {
         }
     }
 
-    fn zeroize(&mut self) {
-        self.buf.fill(0u8);
-        self.key_len = 0;
-        self.key_type = KeyType::Zeroized;
-    }
-
     fn truncate(&mut self, new_len: usize) -> Result<(), KeyMaterialError> {
         if new_len > self.key_len {
             return Err(KeyMaterialError::InvalidLength);
@@ -623,10 +620,3 @@ impl<const KEY_LEN: usize> fmt::Debug for KeyMaterial<KEY_LEN> {
         )
     }
 }
-
-/// Zeroize the key material on drop.
-impl<const KEY_LEN: usize> Drop for KeyMaterial<KEY_LEN> {
-    fn drop(&mut self) {
-        self.zeroize()
-    }
-}

crypto/core/src/traits.rs

@@ -4,6 +4,7 @@ use crate::errors::{HashError, KDFError, KEMError, MACError, RNGError, Signature
 use crate::key_material::KeyMaterialTrait;
 use core::fmt::{Debug, Display};
 use core::marker::Sized;
+use zeroize::{DefaultIsZeroes, ZeroizeOnDrop};
 
 // Imports needed for docs
 #[allow(unused_imports)]
@@ -379,8 +380,9 @@ pub trait MAC: Sized {
     fn max_security_strength(&self) -> SecurityStrength;
 }
 
-#[derive(Eq, PartialEq, PartialOrd, Clone, Debug)]
+#[derive(Eq, PartialEq, PartialOrd, Clone, Copy, Debug, Default)]
 pub enum SecurityStrength {
+    #[default]
     None,
     _112bit,
     _128bit,
@@ -420,6 +422,8 @@ impl SecurityStrength {
     }
 }
 
+impl DefaultIsZeroes for SecurityStrength {}
+
 /// An interface for random number generation.
 /// This interface is meant to be simpler and more ergonomic than the interfaces provided by the
 /// `rng` crate, but that one should
@@ -451,11 +455,7 @@ pub trait RNG: Default {
 
 /// A trait that forces an object to implement a zeroizing Drop() as well as Debug and Display that
 /// will not log the sensitive contents, even in error or crash-dump scenarios.
-// Since rust auto-implements Drop, there's a lint that explicitly bounding on Drop is useless.
-// 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.
-#[allow(drop_bounds)]
-pub trait Secret: Drop + Debug + Display {}
+pub trait Secret: ZeroizeOnDrop + Debug + Display {}
 
 /// Pre-Hashed Signer is an extension to [Signer] that adds functionality specific to signature
 /// primatives that can operate on a pre-hashed message instead of the full message.

crypto/core/tests/key_material_tests.rs

@@ -6,6 +6,7 @@ mod test_key_material {
         KeyMaterialTrait, KeyType,
     };
     use bouncycastle_core::traits::SecurityStrength;
+    use zeroize::Zeroize;
 
     const DUMMY_KEY: &[u8; 64] = b"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0A\x0B\x0C\x0D\x0E\x0F\
                                    \x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1A\x1B\x1C\x1D\x1E\x1F\
@@ -173,29 +174,37 @@ mod test_key_material {
 
     #[test]
     fn zeroize() {
-        let mut key = KeyMaterial256::from_bytes(&DUMMY_KEY[..32]).unwrap();
-        let capacity = key.capacity();
+        let assert_construction = |key: &KeyMaterial<32>| {
+            assert_eq!(key.key_len(), 32);
+            assert_eq!(key.key_type(), KeyType::BytesLowEntropy);
+            assert_eq!(key.security_strength(), SecurityStrength::None);
+        };
+
+        let assert_zeroization = |key: &mut KeyMaterial<32>| {
+            let key_len = key.key_len();
+            assert_eq!(key_len, 0);
+            assert_eq!(key.key_type(), KeyType::Zeroized);
+
+            key.allow_hazardous_operations();
+            assert!(!key.mut_ref_to_bytes().unwrap().iter().any(|&b| b != 0));
+            key.drop_hazardous_operations();
+        };
+
+        // starting at i=1 to skip the initial \x00 byte
+        let mut key = KeyMaterial256::from_bytes(&DUMMY_KEY[1..33]).unwrap();
 
         // Sanity check: the backing buffer actually holds non-zero key material before it is wiped.
         // Without this, the post-zeroize assertion below could pass vacuously.
-        key.allow_hazardous_operations();
-        assert!(key.mut_ref_to_bytes().unwrap().iter().any(|&b| b != 0));
-        key.drop_hazardous_operations();
+        assert!(!key.ref_to_bytes().iter().any(|&b| b == 0));
 
+        assert_construction(&key);
         key.zeroize();
-        let key_len = key.key_len();
-        assert_eq!(key_len, 0);
-        assert_eq!(key.key_type(), KeyType::Zeroized);
+        assert_zeroization(&mut key);
 
-        // zeroize() must wipe the entire backing buffer.
-        // Full capacity must be inspected to confirm the previously-set bytes were
-        // actually overwritten with zeros.
-        // Note: key_len is now 0, so ref_to_bytes() returns an empty slice.
-        key.allow_hazardous_operations();
-        let full_buf = key.mut_ref_to_bytes().unwrap();
-        assert_eq!(full_buf.len(), capacity);
-        assert!(full_buf.iter().all(|&b| b == 0));
-        key.drop_hazardous_operations();
+        let mut key = KeyMaterial256::from_bytes(&DUMMY_KEY[..32]).unwrap();
+        assert_construction(&key);
+        unsafe { core::ptr::drop_in_place(&mut key) };
+        assert_zeroization(&mut key);
     }
 
     #[test]

crypto/mldsa-lowmemory/Cargo.toml

@@ -9,6 +9,7 @@ bouncycastle-sha2.workspace = true
 bouncycastle-sha3.workspace = true
 bouncycastle-rng.workspace = true
 bouncycastle-utils.workspace = true
+zeroize.workspace = true
 
 [dev-dependencies]
 bouncycastle-core-test-framework.workspace = true

crypto/mldsa-lowmemory/src/mldsa_keys.rs

@@ -28,6 +28,8 @@ use bouncycastle_core::traits::{
 use core::fmt;
 use core::fmt::{Debug, Display, Formatter};
 
+use zeroize::ZeroizeOnDrop;
+
 // imports just for docs
 #[allow(unused_imports)]
 use crate::mldsa::MLDSATrait;
@@ -294,7 +296,7 @@ pub trait MLDSAPrivateKeyTrait<
 }
 
 /// Internal structure for holding a seed-based private key for ML-DSA.
-#[derive(Clone, PartialEq, Eq)]
+#[derive(Clone, PartialEq, Eq, ZeroizeOnDrop)]
 pub struct MLDSASeedPrivateKey<
     const LAMBDA: i32,
     const GAMMA2: i32,
@@ -314,41 +316,6 @@ pub struct MLDSASeedPrivateKey<
     K: [u8; 32],
 }
 
-impl<
-    const LAMBDA: i32,
-    const GAMMA2: i32,
-    const k: usize,
-    const l: usize,
-    const eta: usize,
-    const S1_PACKED_LEN: usize,
-    const S2_PACKED_LEN: usize,
-    const T1_PACKED_LEN: usize,
-    const SK_LEN: usize,
-    const PK_LEN: usize,
-    const FULL_SK_LEN: usize,
-> Drop
-    for MLDSASeedPrivateKey<
-        LAMBDA,
-        GAMMA2,
-        k,
-        l,
-        eta,
-        S1_PACKED_LEN,
-        S2_PACKED_LEN,
-        T1_PACKED_LEN,
-        PK_LEN,
-        SK_LEN,
-        FULL_SK_LEN,
-    >
-{
-    fn drop(&mut self) {
-        // seed is a KeyMaterialSized which will zeroize itself
-        self.rho.fill(0u8);
-        self.rho_prime.fill(0u8);
-        self.K.fill(0u8);
-    }
-}
-
 impl<
     const LAMBDA: i32,
     const GAMMA2: i32,

crypto/mldsa-lowmemory/src/polynomial.rs

@@ -7,11 +7,13 @@ use core::fmt;
 use core::fmt::{Debug, Display, Formatter};
 use core::ops::{Index, IndexMut};
 
+use zeroize::ZeroizeOnDrop;
+
 /// A polynomial over the ML-DSA ring.
 /// Dev note: this doesn't strictly need to be pub ... ie there's no good reason for a caller to use this class directly,
 /// but in order to test the Debug and Display traits, you need STD, so those can't be tested from inline tests in this file
 /// and the real unit tests are in a different crate, so here we are.
-#[derive(Clone)]
+#[derive(Clone, ZeroizeOnDrop)]
 pub struct Polynomial {
     pub(crate) coeffs: [i32; N],
 }
@@ -248,12 +250,6 @@ impl Polynomial {
 
 impl Secret for Polynomial {}
 
-impl Drop for Polynomial {
-    fn drop(&mut self) {
-        self.coeffs.fill(0i32);
-    }
-}
-
 impl Debug for Polynomial {
     fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
         write!(f, "Polynomial (data masked)")

crypto/mldsa/Cargo.toml

@@ -9,6 +9,7 @@ bouncycastle-sha2.workspace = true
 bouncycastle-sha3.workspace = true
 bouncycastle-rng.workspace = true
 bouncycastle-utils.workspace = true
+zeroize.workspace = true
 
 [dev-dependencies]
 bouncycastle-core-test-framework.workspace = true

crypto/mldsa/src/matrix.rs

@@ -6,9 +6,10 @@ use crate::mldsa::H;
 use crate::polynomial::Polynomial;
 use bouncycastle_core::traits::XOF;
 use core::ops::{Index, IndexMut};
+use zeroize::ZeroizeOnDrop;
 
 /// A matrix over the ML-DSA ring.
-#[derive(Clone)]
+#[derive(Clone, ZeroizeOnDrop)]
 pub struct Matrix<const k: usize, const l: usize>(/*pub(crate)*/ [[Polynomial; l]; k]);
 
 /// Convenience function to avoid ".0" all over the place.
@@ -62,7 +63,7 @@ impl<const k: usize, const l: usize> Matrix<k, l> {
 // Technically all matrices and some vectors are only part of the public key and might not need to be zeroized,
 // but I'll leave it zeroizing for now and leave this as a potential future optimization.
 
-#[derive(Clone)]
+#[derive(Clone, ZeroizeOnDrop)]
 pub(crate) struct Vector<const k: usize> {
     pub(crate) vec: [Polynomial; k],
 }

crypto/mldsa/src/mldsa_keys.rs

@@ -14,6 +14,7 @@ use bouncycastle_core::key_material::KeyMaterial;
 use bouncycastle_core::traits::{Secret, SignaturePrivateKey, SignaturePublicKey, XOF};
 use core::fmt;
 use core::fmt::{Debug, Display, Formatter};
+use zeroize::ZeroizeOnDrop;
 
 // imports just for docs
 #[allow(unused_imports)]
@@ -403,7 +404,7 @@ impl<
 }
 
 /// An ML-DSA private key.
-#[derive(Clone)]
+#[derive(Clone, ZeroizeOnDrop)]
 pub struct MLDSAPrivateKey<
     const k: usize,
     const l: usize,
@@ -805,20 +806,11 @@ impl<const k: usize, const l: usize, const eta: usize, const SK_LEN: usize, cons
     }
 }
 
-/// Zeroizing drop
-impl<const k: usize, const l: usize, const eta: usize, const SK_LEN: usize, const PK_LEN: usize>
-    Drop for MLDSAPrivateKey<k, l, eta, SK_LEN, PK_LEN>
-{
-    fn drop(&mut self) {
-        self.K.fill(0u8);
-        // s1, s2, t0, seed have their own zeroizing drop
-    }
-}
 
 /// A fully expanded ML-DSA private key that includes the intermediate values needed for performing
 /// multiple sign operations with the same private key, which causes the private ey struct to take up
 /// more memory, but results in more efficient repeated sign() operations.
-#[derive(Clone)]
+#[derive(Clone, ZeroizeOnDrop)]
 pub struct MLDSAPrivateKeyExpanded<
     const k: usize,
     const l: usize,
@@ -876,22 +868,6 @@ impl<
 {
 }
 
-impl<
-    const k: usize,
-    const l: usize,
-    const eta: usize,
-    PK: MLDSAPublicKeyInternalTrait<k, PK_LEN>,
-    SK: MLDSAPrivateKeyTrait<k, l, eta, SK_LEN, PK_LEN>
-        + MLDSAPrivateKeyInternalTrait<k, l, eta, SK_LEN, PK_LEN>,
-    const SK_LEN: usize,
-    const PK_LEN: usize,
-> Drop for MLDSAPrivateKeyExpanded<k, l, eta, PK, SK, SK_LEN, PK_LEN>
-{
-    fn drop(&mut self) {
-        // Nothing to do since self.sk already impls zeroizing Drop
-    }
-}
-
 impl<
     const k: usize,
     const l: usize,