resolved a bunch of todo's

Author: ounsworth

crypto/core-interface/tests/key_material_tests.rs

@@ -439,13 +439,16 @@ mod test_key_material {
     }
 
     #[test]
-    fn from_keymaterial() {
-        let key1 = KeyMaterial256::from_bytes(&DUMMY_KEY[..32]).unwrap();
-
+    fn from_keym() {
+        let key1 = KeyMaterial256::from_bytes_as_type(&DUMMY_KEY[..32], KeyType::MACKey).unwrap();
+        assert_eq!(key1.key_type(), KeyType::MACKey);
+        assert_eq!(key1.security_strength(), SecurityStrength::_256bit);
+        
         // success case: same size using default From impl; only works if the sizes are the same (ie the compiler knows that they are the same type.
         let key2 = KeyMaterial256::from(key1.clone());
         assert_eq!(key1.key_len(), key2.key_len());
         assert_eq!(key1.key_type(), key2.key_type());
+        assert_eq!(key1.security_strength(), key2.security_strength());
         assert_eq!(key1, key2);
 
         // success case: same size

crypto/mldsa/src/aux_functions.rs

@@ -35,12 +35,16 @@ pub(crate) fn coeff_from_three_bytes(b: &[u8; 3]) -> Result<i32, ()> {
 /// Output: An integer between −𝜂 and 𝜂, or ⊥.
 #[inline(always)]
 pub(crate) fn coeff_from_half_byte<const ETA: usize>(b: u8) -> Result<i32, ()> {
-    // todo: there's no way this is constant time:
-    // todo: the if statement might not be so bad because the alternative is rejection,
-    // todo: but that % is a problem.
-    // todo: what does openssl or rust crypto do?
     if ETA == 2 && b < 15 {
-        Ok(2 - (b % 5) as i32) // todo: is constant-time?
+        // Original code is bad because '%' is not constant-time.
+        // Ok(2 - (b % 5) as i32)
+        // I'm still not convinced this is constant-time, but maybe it's closer? And I can't come up with anything better.
+        let b = match b {
+            b if b < 5 => b,
+            b if b < 10 => b - 5,
+            _ => b - 10,
+        };
+        Ok(2 - b as i32)
     } else {
         if ETA == 4 && b < 9 { Ok(4 - b as i32) } else { Err(()) }
     }
@@ -412,7 +416,6 @@ pub(crate) fn sig_encode<
     pos += LAMBDA_over_4;
 
     for i in 0..l {
-        // todo -- remove this copy by having bitpack_gamma1 take an output slice
         output[pos..pos + POLY_Z_PACKED_LEN]
             .copy_from_slice(&bitpack_gamma1::<POLY_Z_PACKED_LEN, GAMMA1>(&z.vec[i]));
         pos += POLY_Z_PACKED_LEN;
@@ -504,7 +507,6 @@ pub(crate) fn sig_decode<
     }
 
     // ▷ read any leftover bytes in the first 𝜔 bytes of 𝑦 for malformed (nonzero) bytes
-
     for j in idx..OMEGA as usize {
         if sig[pos + j] != 0 {
             return Err(());
@@ -790,7 +792,6 @@ fn test_power_2_round() {
 /// Decomposes 𝑟 into (𝑟1, 𝑟0) such that 𝑟 ≡ 𝑟1(2𝛾2) + 𝑟0 mod 𝑞.
 /// Input: 𝑟 ∈ ℤ𝑞.
 /// Output: Integers (𝑟1, 𝑟0).
-
 // the hope here is that the compiler will aggressively inline this function,
 // and optimize away the branching.
 #[inline(always)]
@@ -1029,7 +1030,6 @@ pub(crate) fn ntt(w: &Polynomial) -> Polynomial {
 /// Output: Polynomial 𝑤(𝑋) = ∑255
 /// 𝑗=0 𝑤𝑗𝑋𝑗 ∈ 𝑅𝑞
 pub(crate) fn inv_ntt(w_hat: &Polynomial) -> Polynomial {
-    // todo: optimize to do this in-place? Might actually bench worse.
     let mut w = w_hat.clone();
 
     let mut m: usize = N;

crypto/mldsa/src/hashmldsa.rs

@@ -1,3 +1,26 @@
+use bouncycastle_core_interface::traits::Hash;
+use crate::MLDSA44PublicKey;
 
 /// Note that the PH expected here *is not the same* as the `mu` computed by [compute_mu] ... blah blah explain.
-struct DummyForTheCommentToCompile {}
+struct DummyForTheCommentToCompile {}
+
+/// Note: yes, a HashMLDSAPublicKey is just a re-branded MLDSAPublicKey because they are structurally
+/// the same, but we are giving them different types to indicate that they _should_ not be used interchangeably
+/// in order to prevent certain cross-protocol attacks where a verifier is tricked into accepting an
+/// ML-DSA signature that was actually generated by the HashML-DSA algorithm, and vice-versa.
+/// IE a given key should be used ONLY for ML-DSA or HashML-DSA and should not be used sometimes for one and sometimes for the other.
+/// We won't prevent you from doing so; for example you can convert between them by encoding the key to bytes and
+/// back into the other type, but we want to make you aware that you're opening yourself up to cross-protocol attacks.
+pub type HashMLDSA44PublicKey = MLDSA44PublicKey;
+
+/// An instance of the HashML-DSA algorithm.
+///
+/// We are exposing the HashMLDSA struct this way so that alternative hash functions can be used
+/// without requiring modification of this source code; you can add your own hash function
+/// by specifying the hash function to use (in the verifier), and specifying the bytes of the OID to
+/// to use as its domain separator in constructing the message representative M'.
+// todo: put an example of this in the unit tests and then copy that example into these ducs
+// todo: figure out how to do a const str param
+pub struct HashMLDSA<HASH: Hash + Default, const oid_name: [u8]> {
+    _phantom: std::marker::PhantomData<HASH>,
+}

crypto/mldsa/src/lib.rs

@@ -9,6 +9,7 @@
 #![feature(generic_const_exprs)]
 #![feature(int_roundings)]
 #![feature(inherent_associated_types)]
+#![feature(adt_const_params)]
 // These are because I'm matching variable names exactly against FIPS 204, for example both 'K' and 'k',
 // or 'A' and 'a' are used and have specific meanings.
 // But need to tell the rust linter to not care.
@@ -19,6 +20,9 @@
 // MLDSA implentation, but I don't want accessed from outside, such FIPS-internal functions.
 #![allow(private_bounds)]
 
+// Used in HashMLDSA
+#![feature(unsized_const_params)]
+
 mod mldsa;
 mod hashmldsa;
 mod mldsa_keys;
@@ -56,104 +60,4 @@ pub use mldsa::{MLDSA87_PK_LEN, MLDSA87_SK_LEN, MLDSA87_SIG_LEN};
 
 
 
-/*** Param traits ***/
-
-// todo -- delete
-// /// Private trait on purpose so that only the NIST-approved params can be used.
-// /// Values taken directly from FIPS 204 Table 1 and Table 2
-// #[allow(private_bounds)]
-// trait MLDSAParams {
-//     // from FIPS 204 Table 1
-//     // q, zeta, d defined as global constants since they do not vary by parameter set
-//     const TAU: i32;
-//     const GAMMA1: i32;
-//     const GAMMA2: i32;
-//     const k: usize;
-//     const l: usize;
-//     const ETA: i32;
-//     const BETA: i32; // tau * eta
-//     const OMEGA: i32;
-//
-//     // useful derived values
-//     const C_TILDE: usize;
-//     const POLY_VEC_H_PACKED_LEN: usize;
-//     const POLY_Z_PACKED_LEN: usize;
-//     const POLY_W1_PACKED_LEN: usize;
-//     const POLY_ETA_PACKED_LEN: usize;
-//     const GAMMA1_MASK_LEN: usize;
-//     const LAMBDA_over_4: usize;
-// }
-
-// pub struct MLDSA44Params;
-//
-// impl MLDSAParams for MLDSA44Params {
-//     const TAU: i32 = 39;
-//     const GAMMA1: i32 = 1 << 17;
-//     const GAMMA2: i32 = (q - 1) / 88;
-//     const k: usize = 4;
-//     const l: usize = 4;
-//     const ETA: i32 = 2;
-//     const BETA: i32 = 78;
-//     const OMEGA: i32 = 80;
-//
-//     // const ALG: MldsaAlg = MldsaAlg::MlDsa44;
-//     const C_TILDE: usize = 32;
-//     const POLY_VEC_H_PACKED_LEN: usize = 0; // todo -- compute
-//     const POLY_Z_PACKED_LEN: usize = 576;
-//     const POLY_W1_PACKED_LEN: usize = 192;
-//     const POLY_ETA_PACKED_LEN: usize = 96;
-//
-//     // Alg 32
-//     // 1: 𝑐 ← 1 + bitlen (𝛾1 − 1)
-//     const GAMMA1_MASK_LEN: usize = 576;  // 32*(1 + bitlen (𝛾1 − 1) )
-//     const LAMBDA_over_4: usize = 128/4;
-//     // todo -- bc-java does it as compute: 576usize.div_ceil(symmetric.stream_256_block_bytes) -- which should be 5
-//     // todo -- might need to debug this against bc-java
-//     // todo -- debug this against bc-java; or look in other implementations. I feel like this should be 32*17=544 or 32*19=608
-//     // todo -- I'm not sure why they're adding an extra 32
-//     // todo -- corresponds to aux_functions::expand_mask()
-// }
-
-// pub struct MLDSA65Params;
-//
-// impl MLDSAParams for MLDSA65Params {
-//     const TAU: i32 = 49;
-//     const GAMMA1: i32 = 1 << 19;
-//     const GAMMA2: i32 = (q - 1) / 32;
-//     const k: usize = 6;
-//     const l: usize = 5;
-//     const ETA: i32 = 4;
-//     const BETA: i32 = 196;
-//     const OMEGA: i32 = 55;
-//
-//     const C_TILDE: usize = 48;
-//     const POLY_VEC_H_PACKED_LEN: usize = 0; // todo -- compute
-//     const POLY_Z_PACKED_LEN: usize = 640;
-//     const POLY_W1_PACKED_LEN: usize = 128;
-//     const POLY_ETA_PACKED_LEN: usize = 128;
-//     const GAMMA1_MASK_LEN: usize = 640; // todo -- compute: 640usize.div_ceil(symmetric.stream_256_block_bytes)
-//     const LAMBDA_over_4: usize = 192/4;
-// }
-
-// pub struct MLDSA87Params;
-//
-// impl MLDSAParams for MLDSA87Params {
-//     const TAU: i32 = 60;
-//     const GAMMA1: i32 = 1 << 19;
-//     const GAMMA2: i32 = (q - 1) / 32;
-//     const k: usize = 8;
-//     const l: usize = 7;
-//     const ETA: i32 = 2;
-//     const BETA: i32 = 120;
-//     const OMEGA: i32 = 75;
-//
-//     const C_TILDE: usize = 64;
-//     const POLY_VEC_H_PACKED_LEN: usize = 0; // todo -- compute
-//     const POLY_Z_PACKED_LEN: usize = 640;
-//     const POLY_W1_PACKED_LEN: usize = 128;
-//     const POLY_ETA_PACKED_LEN: usize = 96;
-//     const GAMMA1_MASK_LEN: usize = 640; // todo -- compute: 640usize.div_ceil(symmetric.stream_256_block_bytes)
-//     const LAMBDA_over_4: usize = 256/4;
-// }
-
 // todo -- impl bouncycastle_core_interface::traits::Algorithm with the security strengths from Table 1

crypto/mldsa/src/matrix.rs

@@ -188,7 +188,6 @@ impl<const LEN: usize> Vector<LEN>
         // 4: end for
         for i in 0..LEN {
             w1_tilde[i*POLY_W1_PACKED_LEN .. (i+1)*POLY_W1_PACKED_LEN].copy_from_slice(
-                // todo -- optimize this to take a slice and write directly to it?
                 &self.vec[i].w1_encode::<POLY_W1_PACKED_LEN>()
             )
         }

crypto/mldsa/src/mldsa.rs

@@ -27,6 +27,7 @@ pub(crate) const ROOT_OF_UNITY: i32 = 1753;
 pub const SEED_LEN: usize = 32;
 pub const RND_LEN: usize = 32;
 pub const TR_LEN: usize = 64;
+pub const MU_LEN: usize = 64;
 pub(crate) const POLY_T1PACKED_LEN: usize = 320;
 pub(crate) const POLY_T0PACKED_LEN: usize = 416;
 
@@ -287,7 +288,7 @@ impl<
     /// specifically takes a 32-byte [KeyMaterial256] and checks that it has [KeyType::Seed] and
     /// [SecurityStrength::_256bit].
     /// If you happen to have your seed in a larger KeyMaterial, you'll have to copy it using
-    /// [KeyMaterial::from_key] -- todo: make sure this works and copies key type and security strength correctly.
+    /// [KeyMaterial::from_key]
     fn keygen_internal(
         seed: &KeyMaterial256,
     ) -> Result<
@@ -312,15 +313,11 @@ impl<
         let mut rho_prime: [u8; 64] = [0u8; 64];
         let mut K: [u8; 32] = [0u8; 32];
 
-        // TODO: optimization: re-use variables rather than allocating new ones?
-        // TODO: do with benches because it might not actually be faster. Rust seems to like local vars.
-
         let mut h = H::default();
         h.absorb(seed.ref_to_bytes());
         h.absorb(&(k as u8).to_le_bytes());
         h.absorb(&(l as u8).to_le_bytes());
         let bytes_written = h.squeeze_out(&mut rho);
-        debug_assert_eq!(bytes_written, 32); // todo: remove these asserts once we have unit tests that pass?
         let bytes_written = h.squeeze_out(&mut rho_prime);
         debug_assert_eq!(bytes_written, 64);
         let bytes_written = h.squeeze_out(&mut K);
@@ -421,14 +418,17 @@ impl<
     /// (in which case a keygen_from_seed is run and then the pk's compared).
     ///
     /// Returns either `()` or [SignatureError::ConsistencyCheckFailed].
-    ///
-    /// TODO -- sync with openssl implementation
-    /// TODO -- https://github.com/openssl/openssl/blob/master/crypto/ml_dsa/ml_dsa_key.c#L385
     pub fn keypair_consistency_check(
         pk: &PK,
         sk: &SK,
     ) -> Result<(), SignatureError> {
-        todo!()
+        // This is maybe a computationally heavy way to compare them, but it works
+        let derived_pk = sk.derive_public_key();
+        if derived_pk.compute_tr() == pk.compute_tr() {
+            Ok(())
+        } else {
+            Err(SignatureError::ConsistencyCheckFailed())
+        }
     }
 
         /// This provides the first half of the "External Mu" interface to ML-DSA which is described
@@ -465,8 +465,8 @@ impl<
         pub fn compute_mu_from_tr(
             msg: &[u8],
             ctx: Option<&[u8]>,
-            tr: &[u8; 64],
-        ) -> Result<[u8; 64], SignatureError> {
+            tr: &[u8; TR_LEN],
+        ) -> Result<[u8; TR_LEN], SignatureError> {
             MuBuilder::compute_mu(msg, ctx, tr)
         }
 
@@ -475,7 +475,7 @@ impl<
             msg: &[u8],
             ctx: Option<&[u8]>,
             pk: &PK,
-        ) -> Result<[u8; 64], SignatureError> {
+        ) -> Result<[u8; MU_LEN], SignatureError> {
             MuBuilder::compute_mu(msg, ctx, &pk.compute_tr())
         }
 
@@ -494,7 +494,7 @@ impl<
     /// This mode uses randomized signing (called "hedged mode" in FIPS 204) using an internal RNG.
     fn sign_mu(
         sk: &SK,
-        mu: &[u8; 64],
+        mu: &[u8; MU_LEN],
     ) -> Result<[u8; SIG_LEN], SignatureError> {
         let mut out: [u8; SIG_LEN] = [0u8; SIG_LEN];
         Self::sign_mu_out(sk, mu, &mut out)?;
@@ -509,10 +509,10 @@ impl<
     /// Returns the number of bytes written to the output buffer. Can be called with an oversized buffer.
     fn sign_mu_out(
         sk: &SK,
-        mu: &[u8; 64],
+        mu: &[u8; MU_LEN],
         output: &mut [u8; SIG_LEN],
     ) -> Result<usize, SignatureError> {
-        let mut rnd: [u8; 32] = [0u8; 32];
+        let mut rnd: [u8; RND_LEN] = [0u8; RND_LEN];
         HashDRBG_SHA512::new_from_os().next_bytes_out(&mut rnd)?;
 
         Self::sign_mu_deterministic_out(sk, mu, rnd, output)
@@ -553,8 +553,8 @@ impl<
     /// Returns the number of bytes written to the output buffer. Can be called with an oversized buffer.
     pub(crate) fn sign_mu_deterministic_out(
         sk: &SK,
-        mu: &[u8; 64],
-        rnd: [u8; 32],
+        mu: &[u8; MU_LEN],
+        rnd: [u8; RND_LEN],
         output: &mut [u8; SIG_LEN],
     ) -> Result<usize, SignatureError> {
         // 1: (𝜌, 𝐾, 𝑡𝑟, 𝐬1, 𝐬2, 𝐭0) ← skDecode(𝑠𝑘)
@@ -608,9 +608,6 @@ impl<
             // 11: 𝐲 ∈ 𝑅^ℓ ← ExpandMask(𝜌″, 𝜅)
             let mut y = expand_mask::<l, GAMMA1, GAMMA1_MASK_LEN>(&rho_p_p, kappa);
 
-            // last use of rho_p_p, so zeroizing it
-            rho_p_p.fill(0u8);
-
             // 12: 𝐰 ← NTT−1(𝐀_hat * NTT(𝐲))
             let mut y_hat = y.clone();
             y_hat.ntt();
@@ -696,12 +693,16 @@ impl<
             };
 
             // "In addition, there is an alternative way of implementing the validity checks on 𝐳 and the computation of
-            // 𝐡, which is described in Section 5.1 of. This method may also be used in implementations of ML-DSA."
-            // todo -- check this out
+            // 𝐡, which is described in Section 5.1 of [6] (dilithium-specification-round3-20210208.pdf).
+            // This method may also be used in implementations of ML-DSA."
+            // todo -- I believe this code is already using this optimization, but it could use a deeper look to see if more optimization is possible.
 
             break;
         }
 
+        // zeroize rho_p_p before returning it to the OS
+        rho_p_p.fill(0u8);
+
         // 33: 𝜎 ← sigEncode(𝑐, 𝐳̃ mod±𝑞, 𝐡)
         let bytes_written = sig_encode::<GAMMA1, k, l, LAMBDA_over_4, OMEGA, POLY_Z_PACKED_LEN, SIG_LEN>
             (&sig_val_c_tilde, &sig_val_z, &sig_val_h, output);
@@ -721,7 +722,7 @@ impl<
     /// Input: Signature 𝜎 ∈ 𝔹𝜆/4+ℓ⋅32⋅(1+bitlen (𝛾1−1))+𝜔+𝑘.
     fn verify_mu_internal(
         pk: &PK,
-        mu: &[u8; 64],
+        mu: &[u8; MU_LEN],
         sig: &[u8; SIG_LEN],
     ) -> bool {
         // 1: (𝜌, 𝐭1) ← pkDecode(𝑝𝑘)

crypto/mldsa/src/mldsa_keys.rs

@@ -384,7 +384,7 @@ impl<const k: usize, const l: usize, const eta: usize, const SK_LEN: usize, cons
             seed: seed.clone(),
         }
     }
-    
+
     fn rho(&self) -> &[u8; 32] { &self.rho }
 
     fn K(&self) -> &[u8; 32] { &self.K }