diff --git a/cli/src/helpers.rs b/cli/src/helpers.rs index 62a7741..36fd1fa 100644 --- a/cli/src/helpers.rs +++ b/cli/src/helpers.rs @@ -17,7 +17,7 @@ pub(crate) fn read_from_file(filename: &str) -> Vec { match hex::decode(&buf) { Ok(decoded) => decoded, Err(_) => { - // well, it's not hex, so return it raw + // it's not hex, so return it raw buf } } @@ -47,7 +47,7 @@ pub(crate) fn read_from_file_or_stdin(filename: &Option) -> Vec { match hex::decode(&buf) { Ok(decoded) => decoded, Err(_) => { - // well, it's not hex, so return it raw + // it's not hex, so return it raw buf } } @@ -98,7 +98,7 @@ pub(crate) fn parse_seed(bytes: &[u8]) -> Result::from_bytes_as_type(&seed_bytes, KeyType::Seed).unwrap(); if seed.key_type() == KeyType::Zeroized || seed.security_strength() < SecurityStrength::_256bit diff --git a/cli/src/mldsa_cmd.rs b/cli/src/mldsa_cmd.rs index 8a1a4e3..82c7e8c 100644 --- a/cli/src/mldsa_cmd.rs +++ b/cli/src/mldsa_cmd.rs @@ -1,5 +1,5 @@ -//! Yup, this file is as absolutely atrocious mess of duplicate code that could be much improved -//! by using generics or macros. I just, haven't ... yet. +//! Work in progress. +//! TODO: Use generic macros to eliminate duplicated code. use crate::helpers::{parse_seed, read_from_file, read_from_file_or_stdin, write_bytes_or_hex}; use bouncycastle::core::traits::{Signature, SignaturePrivateKey, SignaturePublicKey}; diff --git a/crypto/base64/src/lib.rs b/crypto/base64/src/lib.rs index b3e57d8..bf47f03 100644 --- a/crypto/base64/src/lib.rs +++ b/crypto/base64/src/lib.rs @@ -1,7 +1,8 @@ //! Good old fashioned base64 encoder and decoder. //! -//! It should just work the way you expect: [encode] takes any bytes-like rust type -//! and returns a String, while [decode] takes a String (which can be in any bytes-like container) +//! It should just work the way it is normally expected: +//! [encode] takes any bytes-like rust type and returns a String, +//! while [decode] takes a String (which can be in any bytes-like container) //! and returns a `Vec`. //! //!``` @@ -31,10 +32,10 @@ //! Unlike Hex, Base64 does not align cleanly to byte boundaries. //! That means that the above one-shot APIs should only be used if you have the entire content to //! process at the same time. -//! In other words, if you arbitrarily break your data into chunks and hand it to the one-shot [encode] and [decode] APIs, -//! you will get incorrect results. -//! If you need to process your data in chunks, you need to use the streaming API that allows -//! repeated calls to `do_update`, producing output as it goes, and correctly holds on to the unprocessed +//! In other words, if data is arbitrarily broken into chunks and handed to the one-shot [encode] and [decode] APIs, +//! the results obtained will be incorrect. +//! Whenever it is necessary to process data in chunks, the streaming API that allows repeated calls to `do_update` +//! must be used. This produces output as it goes, and correctly holds on to the unprocessed //! partial block until either `do_update` or `do_final` is called. //! //! ``` @@ -60,15 +61,15 @@ //! //! > [Util::Lookup: Exploiting key decoding in cryptographic libraries (Sieck, 2021)](https://arxiv.org/pdf/2108.04600.pdf), //! -//! As this is a cryptography library, we are assuming that this base64 implementation will be used to encode +//! As this is a cryptography library, it can be assumed that this base64 implementation will be used to encode //! and decode private keys in PEM and JWK formats and so we are only providing a constant-time implementation //! in order to remove the temptation to shoot yourself in the foot in the name of a small performance gain. //! -//! In our testing, a naïve lookup table-based implementation of base64::decode was 1.7x faster than -//! our constant-time implementation, and we are quite sure that optimized base64 implementations exist that -//! provide still better performance. -//! So if you find yourself in a position of needing to base64 encode gigabytes of non-sensitive data, then -//! we recommend you use one of the good, fast, but non-constant-time base64 implementations available from other projects. +//! During testing, a naïve lookup table-based implementation of base64::decode was 1.7x faster than +//! a constant-time implementation. +//! We are quite sure that optimized base64 implementations exist that provide still better performance. +//! It is necessary to encode gigabytes of non-sensitive data on base64, it is advised to use +//! one of the good, fast, but non-constant-time base64 implementations available from other projects. //! //! //! # Alphabets: @@ -100,7 +101,7 @@ pub enum Base64Error { /// pass the same input to do_final(). Note that do_final() is tolerant of incomplete padding blocks, /// so even if an additional padding character is contained in the next chunk of input, do_final() will still produce /// the correct output -- ie any additional chunks held by the caller can be discarded. - PaddingEnconteredDuringDoUpdate, + PaddingEncounteredDuringDoUpdate, /// Input contained a character that was not in the base64 alphabet. The index of the illegal character is included in the output. InvalidB64Character(usize), @@ -288,7 +289,7 @@ impl Base64Decoder { i += 1; self.vals_in_buf += 1; - // here we get to assume that the buffer contains no padding. + // here, it can be assumed that the buffer contains no padding. if self.vals_in_buf == 4 { // decode block out.push(self.buf[0] << 2 | self.buf[1] >> 4); @@ -302,23 +303,23 @@ impl Base64Decoder { Ok(out) } - /// As you would expect, do_final() consumes the object. + /// As can be expected, do_final() consumes the object. pub fn do_final>(mut self, input: T) -> Result, Base64Error> { // process as much as we can the usual way. let mut out = match self.decode_internal(input, false) { Ok(out) => out, - Err(Base64Error::PaddingEnconteredDuringDoUpdate) => { + Err(Base64Error::PaddingEncounteredDuringDoUpdate) => { panic!( - "rollback_if_padding = false should not produce a Base64Error::PaddingEnconteredDuringDoUpdate" + "rollback_if_padding = false should not produce a Base64Error::PaddingEncounteredDuringDoUpdate" ); } Err(e) => return Err(e), }; - // now we only, maybe, have a single block containing padding to deal with. + // now, a single block containing padding remains to be dealt with. if self.vals_in_buf != 0 { // be tolerant of missing padding - // if we're at the end and it's not a complete block, then imagine the missing padding. + // if it is not a complete block at the end, the infer the byte count from the number of leftover symbols let pad_count: u8 = 3 - (self.vals_in_buf as u8 - 1); out.push(self.buf[0] << 2 | self.buf[1] >> 4); diff --git a/crypto/core/src/key_material.rs b/crypto/core/src/key_material.rs index 48f903e..ebb000f 100644 --- a/crypto/core/src/key_material.rs +++ b/crypto/core/src/key_material.rs @@ -44,7 +44,7 @@ use bouncycastle_utils::{ct, min}; use core::cmp::{Ordering, PartialOrd}; use core::fmt; -/// Sometimes you just need a zero-length dummy key. +/// When it is necessary to get a zero-length dummy key. pub type KeyMaterial0 = KeyMaterial<0>; pub type KeyMaterial128 = KeyMaterial<16>; @@ -389,9 +389,11 @@ impl KeyMaterialTrait for KeyMaterial { self.key_len = key_len; Ok(()) } + fn key_type(&self) -> KeyType { self.key_type.clone() } + fn set_key_type(&mut self, key_type: KeyType) -> Result<(), KeyMaterialError> { if !self.allow_hazardous_operations { return Err(KeyMaterialError::HazardousOperationNotPermitted); @@ -399,6 +401,7 @@ impl KeyMaterialTrait for KeyMaterial { self.key_type = key_type.clone(); Ok(()) } + fn security_strength(&self) -> SecurityStrength { self.security_strength.clone() } @@ -453,6 +456,7 @@ impl KeyMaterialTrait for KeyMaterial { self.drop_hazardous_operations(); Ok(()) } + /// Sets this instance to be able to perform potentially hazardous operations such as /// casting a KeyMaterial of type RawUnknownEntropy or RawLowEntropy into RawFullEntropy or SymmetricCipherKey. /// @@ -463,10 +467,12 @@ impl KeyMaterialTrait for KeyMaterial { fn allow_hazardous_operations(&mut self) { self.allow_hazardous_operations = true; } + /// Resets this instance to not be able to perform potentially hazardous operations. fn drop_hazardous_operations(&mut self) { self.allow_hazardous_operations = false; } + /// Sets the key_type of this KeyMaterial object. /// Does not perform any operations on the actual key material, other than changing the key_type field. /// If allow_hazardous_operations is true, this method will allow conversion to any KeyType, otherwise @@ -529,6 +535,7 @@ impl KeyMaterialTrait for KeyMaterial { self.drop_hazardous_operations(); Ok(()) } + fn is_full_entropy(&self) -> bool { match self.key_type { KeyType::BytesFullEntropy diff --git a/crypto/core/src/traits.rs b/crypto/core/src/traits.rs index 089e282..72f8149 100644 --- a/crypto/core/src/traits.rs +++ b/crypto/core/src/traits.rs @@ -1,4 +1,4 @@ -//! Provides simplified abstracted APIs over classes of cryptigraphic primitives, such as Hash, KDF, etc. +//! Provides simplified abstracted APIs over classes of cryptographic primitives, such as Hash, KDF, etc. use crate::errors::{HashError, KDFError, KEMError, MACError, RNGError, SignatureError}; use crate::key_material::KeyMaterialTrait; @@ -108,7 +108,7 @@ pub trait KDF: Default { /// [KeyType::BytesLowEntropy] -- for example, seeding SHA3-256 with a [KeyMaterial] containing /// only 128 bits of key material. /// - /// An implement can, and in most cases SHOULD, return a [HashError] if provided + /// An implementation can, and in most cases SHOULD, return a [HashError] if provided /// with a [KeyMaterial] of type [KeyType::Zeroized]. /// /// # Additional Input diff --git a/crypto/core/tests/key_material_tests.rs b/crypto/core/tests/key_material_tests.rs index 5e773fd..d022480 100644 --- a/crypto/core/tests/key_material_tests.rs +++ b/crypto/core/tests/key_material_tests.rs @@ -26,7 +26,7 @@ mod test_key_material { _ => panic!("Expected InvalidLength"), } - // But you can slice it down. + // This can be sliced down. match KeyMaterial512::from_bytes(&DUMMY_KEY_TOO_LONG[..64]) { Ok(key) => assert_eq!(key.key_len(), 64), _ => panic!("Expected InvalidLength"), @@ -47,7 +47,7 @@ mod test_key_material { assert_eq!(key.key_type(), KeyType::Zeroized); assert_eq!(key.key_len(), 16); - // ... but we can force it. + // however, it can be forced by allowing hazardous operations. key.allow_hazardous_operations(); key.set_key_type(KeyType::BytesLowEntropy).unwrap(); key.drop_hazardous_operations(); @@ -59,13 +59,12 @@ mod test_key_material { assert_eq!(key.key_type(), KeyType::BytesLowEntropy); assert_eq!(key.security_strength(), SecurityStrength::None); - // but it'll allow it if you allow hazardous operations first. + // it can be enabled by allowing hazardous operations first. let key_bytes = [0u8; 16]; let mut key = KeyMaterial256::new(); key.allow_hazardous_operations(); key.set_bytes_as_type(&key_bytes, KeyType::BytesLowEntropy).unwrap(); assert_eq!(key.key_type(), KeyType::BytesLowEntropy); - // nothing else requires setting hazardous operations. } @@ -89,7 +88,7 @@ mod test_key_material { assert_eq!(key.mut_ref_to_bytes().unwrap()[..16], [1u8; 16]); assert_eq!(key.mut_ref_to_bytes().unwrap()[16..], [0u8; 16]); - // and I can set them + // Then they can be set key.mut_ref_to_bytes().unwrap().copy_from_slice(&[2u8; 32]); key.set_key_len(32).unwrap(); assert_eq!(key.ref_to_bytes(), &[2u8; 32]); @@ -247,7 +246,7 @@ mod test_key_material { assert_eq!(key.key_type(), KeyType::BytesLowEntropy); assert!(!key.is_full_entropy()); - // Note: can't use the usual assert_eq!() here because that requires PartialEq, but we're in a no_std context here. + // Note: the usual assert_eq!() can't be used here because that requires PartialEq, but the current context is no_std. match key.key_type() { KeyType::BytesLowEntropy => { /* good */ } _ => panic!("Expected BytesLowEntropy"), @@ -343,12 +342,13 @@ mod test_key_material { } let zero_key = KeyMaterial256::from_bytes(&[0u8; 19]).unwrap(); - // it should have set the key bytes and length, but also set the key type to Zeroized. + // key bytes and length should be set accordingly, + // as well as the key type should be set to Zeroized. assert_eq!(zero_key.key_type(), KeyType::Zeroized); assert_eq!(zero_key.key_len(), 19); assert_eq!(zero_key.ref_to_bytes(), &[0u8; 19]); - // But it's totally fine if you give it non-zero input data. + // It also admits non-zero input data. let not_zero_key = KeyMaterial256::from_bytes(&[1u8; 19]).unwrap(); assert_eq!(not_zero_key.key_type(), KeyType::BytesLowEntropy); @@ -364,10 +364,10 @@ mod test_key_material { panic!("should have thrown a KeyMaterialError::ActingOnZeroizedKey error.") } } - // but it should still have set the key bytes; it's just giving you a friendly warning + // This should still set the key bytes; only giving a friendly warning that the key is zeroized assert_eq!(zero_key.key_type(), KeyType::Zeroized); - // ... but will allow it if you set .allow_hazardous_operations() first. + // The operation is allowed by setting .allow_hazardous_operations() first. let mut zero_key = KeyMaterial256::new(); zero_key.allow_hazardous_operations(); zero_key.set_bytes_as_type(&[0u8; 19], KeyType::MACKey).unwrap(); @@ -402,7 +402,7 @@ mod test_key_material { _ => panic!("Expected HazardousConversion"), } - /* Should work if you allow hazardous conversions. */ + /* Should work when hazardous conversions are allowed. */ key = KeyMaterial256::from_bytes(&DUMMY_KEY[..32]).unwrap(); key.allow_hazardous_operations(); key.convert_key_type(KeyType::BytesFullEntropy).unwrap(); @@ -445,7 +445,8 @@ mod test_key_material { 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. + // success case: same size using default From impl; + // only works if the sizes are the same (i.e. 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()); @@ -490,7 +491,7 @@ mod test_key_material { _ => panic!("Expected HazardousConversion"), } - // should work if you allow hazardous conversions. + // should work when hazardous conversions are allowed. key.allow_hazardous_operations(); key.convert_key_type(KeyType::SymmetricCipherKey).unwrap(); } @@ -540,7 +541,7 @@ mod test_key_material { assert_eq!(key.key_type(), KeyType::BytesFullEntropy); assert_eq!(key.security_strength(), SecurityStrength::None); - // even if it's long enough, BytesLowEntropy or Zeroized always get ::None + // even if it's long enough, BytesLowEntropy or Zeroized always get ::None as security strength let key = KeyMaterial512::from_bytes_as_type(DUMMY_KEY, KeyType::BytesLowEntropy).unwrap(); assert_eq!(key.key_type(), KeyType::BytesLowEntropy); assert_eq!(key.key_len(), 64); @@ -683,6 +684,7 @@ mod test_key_material { b"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0A\x0B\x0C\x0D\x0E\x0F", ) .unwrap(); + let key2 = KeyMaterial256::from_bytes( b"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0A\x0B\x0C\x0D\x0E\x0F", ) diff --git a/crypto/factory/src/hash_factory.rs b/crypto/factory/src/hash_factory.rs index 9d12117..05edc13 100644 --- a/crypto/factory/src/hash_factory.rs +++ b/crypto/factory/src/hash_factory.rs @@ -14,7 +14,7 @@ //! let h = bouncycastle_factory::hash_factory::HashFactory::new(sha3::SHA3_256_NAME).unwrap(); //! let output: Vec = h.hash(data); //! ``` -//! You can equivalently invoke this by string instead of using the constant: +//! Equivalently, it may be invoked by passing a string instead of using the constant: //! //! ``` //! use bouncycastle_factory::AlgorithmFactory; diff --git a/crypto/factory/src/kdf_factory.rs b/crypto/factory/src/kdf_factory.rs index f3deffb..85e5b3e 100644 --- a/crypto/factory/src/kdf_factory.rs +++ b/crypto/factory/src/kdf_factory.rs @@ -9,7 +9,7 @@ //! use bouncycastle_core::traits::KDF; //! use bouncycastle_factory::AlgorithmFactory; //! -//! // get your key material from a secure place; here we'll use the default RNG, seeded from the OS +//! // Obtain key material from a secure place; here the default RNG is used, seeded from the OS is used //! let seed_key = KeyMaterial256::from_rng(&mut bouncycastle_rng::DefaultRNG::default()).unwrap(); //! let additional_input: &[u8] = b"some additional input"; //! @@ -17,14 +17,14 @@ //! let new_key = h.derive_key(&seed_key, additional_input).unwrap(); //! ``` //! -//! You can equivalently invoke this by string instead of using the constant: +//! Equivalently, it may be invoked by passing a string instead of using the constant: //! //! ``` //! use bouncycastle_core::key_material::{KeyMaterial256, KeyType}; //! use bouncycastle_core::traits::KDF; //! use bouncycastle_factory::AlgorithmFactory; //! -//! // get your key material from a secure place; here we'll use the default RNG, seeded from the OS +//! // Obtain key material from a secure place; here the default RNG is used, seeded from the OS is used //! let seed_key = KeyMaterial256::from_rng(&mut bouncycastle_rng::DefaultRNG::default()).unwrap(); //! let additional_input: &[u8] = b"some additional input"; //! @@ -32,14 +32,14 @@ //! let new_key = h.derive_key(&seed_key, additional_input).unwrap(); //! ``` //! -//! Or if you don't particularly care which algorithm is used, you can use the built-in default: +//! If the algorithm used is not particularly important, the built-in default may be used: //! //! ``` //! use bouncycastle_core::key_material::{KeyMaterial256, KeyType}; //! use bouncycastle_core::traits::KDF; //! use bouncycastle_factory::AlgorithmFactory; //! -//! // get your key material from a secure place; here we'll use the default RNG, seeded from the OS +//! // Obtain key material from a secure place; here the default RNG is used, seeded from the OS is used //! let seed_key = KeyMaterial256::from_rng(&mut bouncycastle_rng::DefaultRNG::default()).unwrap(); //! let additional_input: &[u8] = b"some additional input"; //! diff --git a/crypto/factory/src/mac_factory.rs b/crypto/factory/src/mac_factory.rs index 141c59f..f5efc79 100644 --- a/crypto/factory/src/mac_factory.rs +++ b/crypto/factory/src/mac_factory.rs @@ -34,7 +34,7 @@ //! } //! ``` //! -//! You can equivalently construct an instance of [MACFactory] by string instead of using the constant: +//! Equivalently, an instance of [MACFactory] may be constructed by string instead of using the constant: //! //! ``` //! use bouncycastle_core::key_material::{KeyMaterial256, KeyType}; @@ -52,7 +52,7 @@ //! let hmac = MACFactory::new("HMAC-SHA256", &key).unwrap(); //! ``` //! -//! Or if you don't particularly care which algorithm is used, you can use the built-in default: +//! If the algorithm used is not particularly important, the built-in default may be used: //! //! ``` //! use bouncycastle_core::key_material::{KeyMaterial256, KeyType}; @@ -138,12 +138,12 @@ impl MACFactory { } impl MAC for MACFactory { - /// This is a dummy function, required by the [MAC] trait. Don't call it, it doesn't do anything. + /// This is a dummy function, required by the [MAC] trait. DO NOT call it, it does not do anything. fn new(_key: &impl KeyMaterialTrait) -> Result { unimplemented!() } - /// This is a dummy function, required by the [MAC] trait. Don't call it, it doesn't do anything. + /// This is a dummy function, required by the [MAC] trait. DO NOT call it, it does not do anything. fn new_allow_weak_key(_key: &impl KeyMaterialTrait) -> Result { unimplemented!() } diff --git a/crypto/factory/src/rng_factory.rs b/crypto/factory/src/rng_factory.rs index 9f1b8e0..4925f25 100644 --- a/crypto/factory/src/rng_factory.rs +++ b/crypto/factory/src/rng_factory.rs @@ -29,8 +29,8 @@ //! let h = bouncycastle_factory::hash_factory::HashFactory::new(sha3::SHA3_256_NAME).unwrap(); //! let output: Vec = h.hash(data); //! ``` -//! You can equivalently invoke this by string instead of using the constant: -//! +//! Equivalently, it may be invoked by passing a string instead of using the constant: +//! //! ``` //! use bouncycastle_factory::AlgorithmFactory; //! use bouncycastle_core::traits::Hash; diff --git a/crypto/factory/src/xof_factory.rs b/crypto/factory/src/xof_factory.rs index e35e86e..24d5127 100644 --- a/crypto/factory/src/xof_factory.rs +++ b/crypto/factory/src/xof_factory.rs @@ -16,7 +16,7 @@ //! h.absorb(data); //! let output: Vec = h.squeeze(16); //! ``` -//! You can equivalently invoke this by string instead of using the constant: +//! Equivalently, it may be invoked by passing a string instead of using the constant: //! //! ``` //! use bouncycastle_factory::AlgorithmFactory; @@ -24,8 +24,7 @@ //! //! let mut h = XOFFactory::new("SHAKE128"); //! ``` -//! -//! Or, if you don't particularly care which algorithm you get, you can use the configured default: +//! If the algorithm used is not particularly important, the configured default may be used: //! //! ``` //! use bouncycastle_factory::AlgorithmFactory; diff --git a/crypto/factory/tests/kdf_factory_tests.rs b/crypto/factory/tests/kdf_factory_tests.rs index 16e6949..d34af5f 100644 --- a/crypto/factory/tests/kdf_factory_tests.rs +++ b/crypto/factory/tests/kdf_factory_tests.rs @@ -63,7 +63,7 @@ mod kdf_factory_tests { fn hkdf_tests() { /* HKDF-SHA256 */ // Note: this value is not checked against any external reference implementation, - // I just hard-coded the value to make sure it stays consistent. + // The value is hard-coded to ensure consistency. let key_material = KeyMaterial256::from_bytes_as_type(&DUMMY_SEED_512[..32], KeyType::MACKey).unwrap(); let derived_key = @@ -73,7 +73,7 @@ mod kdf_factory_tests { /* HKDF-SHA512 */ // Note: this value is not checked against any external reference implementation, - // I just hard-coded the value to make sure it stays consistent. + // The value is hard-coded to ensure consistency. let key_material = KeyMaterial512::from_bytes(&DUMMY_SEED_512[..64]).unwrap(); let derived_key = KDFFactory::new("HKDF-SHA512").unwrap().derive_key(&key_material, &[0u8; 0]).unwrap(); diff --git a/crypto/hex/src/lib.rs b/crypto/hex/src/lib.rs index 6fcf787..865c50f 100644 --- a/crypto/hex/src/lib.rs +++ b/crypto/hex/src/lib.rs @@ -3,9 +3,9 @@ //! This one is implemented using constant-time operations in the conversions //! from Strings to byte values, so it is safe to use on cryptographic secret values. //! -//! It should just work the way you expect: encode takes any bytes-like rust type -//! and returns a String, decode takes a String (which can be in any bytes-like container) -//! and returns a `Vec`. +//! It should just work as expected: +//! encode takes any bytes-like rust type and returns a String, +//! decode takes a String (which can be in any bytes-like container) and returns a `Vec`. //! //! ``` //! use bouncycastle_hex as hex; @@ -63,7 +63,8 @@ pub fn encode_out>(input: T, out: &mut [u8]) -> Result::is_within_range(c as i64, 0, 9); let in_af = Condition::::is_within_range(c as i64, 10, 15); - // TODO: redo this once we have ct::u8 implemented ... the i64 is wasteful + // TODO: redo this once we have ct::u8 implemented + // The i64 is wasteful let c_09: i64 = '0' as i64 + (c as i64); let c_az: i64 = 'a' as i64 + (c as i64 - 10); @@ -101,7 +102,8 @@ pub fn decode_out>(input: T, out: &mut [u8]) -> Result { i += 1; @@ -146,7 +148,8 @@ pub fn decode_out>(input: T, out: &mut [u8]) -> Result::is_within_range(b as i64, 65, 70); - // TODO: redo this once we have ct::u8 implemented ... the i64 is wasteful + // TODO: redo this once we have ct::u8 implemented + // The i64 is wasteful let c_09: i64 = b as i64 - ('0' as i64); #[allow(non_snake_case)] diff --git a/crypto/hkdf/src/lib.rs b/crypto/hkdf/src/lib.rs index 76b47f0..2754adc 100644 --- a/crypto/hkdf/src/lib.rs +++ b/crypto/hkdf/src/lib.rs @@ -163,8 +163,8 @@ use bouncycastle_core::traits::XOF; // end doc-only imports /*** Constants ***/ -// Slightly hacky, but set this to accomodate the underlying hash primitive with the largest output size. -// Would be better to somehow pull that at compile time from H, but I'm not sure how to do that. +// Set this to accomodate the underlying hash primitive with the largest output size. +// TODO: pull this value at compile time from H const HMAC_BLOCK_LEN: usize = 64; /*** String constants ***/ @@ -179,7 +179,7 @@ pub type HKDF_SHA256 = HKDF; pub type HKDF_SHA512 = HKDF; pub struct HKDF { - hmac: Option>, // Optional because we can't construct an HMAC until they give us a key + hmac: Option>, // Optional because an HMAC cannot be constructed until a key is provided // to initialize it with. // None should correspond to a state of Uninitialized. entropy: HkdfEntropyTracker, @@ -239,7 +239,7 @@ impl HkdfEntropyTracker { } } -// Since I don't want this struct to be public, the tests have to go here. +// Because this struct is not public, the tests have to go here. #[test] fn test_entropy_tracker() { let mut entropy = HkdfEntropyTracker::::new(); @@ -279,7 +279,7 @@ impl HKDF { self.entropy.get_entropy() } - /// Has the entropy input so far met the threshold for this object to be considered fully seeded? + /// Check whether the entropy input so far met the threshold for this object to be considered fully seeded pub fn is_fully_seeded(&self) -> bool { self.entropy.is_fully_seeded() } @@ -400,7 +400,7 @@ impl HKDF { let out: &mut [u8] = okm.mut_ref_to_bytes()?; // Could potentially speed this up by unrolling T(0) and T(1) - // We're gonna have to kludge the prk key type to MACKey to make HMAC happy, but we'll set it back to the original value afterwards. + // The prk key type must be temporarily changed to MACKey to satisfy HMAC, then restored afterwards. let prk_as_mac_key = KeyMaterial::::from_bytes_as_type(prk.ref_to_bytes(), KeyType::MACKey)?; @@ -409,7 +409,7 @@ impl HKDF { let mut t_len: usize = 0; let mut i = 1u8; while i < N { - // todo: might need this to be new_allow_weak_key() + // TODO: might need this to be new_allow_weak_key() let mut hmac = HMAC::::new(&prk_as_mac_key)?; hmac.do_update(&T[..t_len]); hmac.do_update(info); @@ -422,9 +422,9 @@ impl HKDF { i += 1; } - // On the last iteration, we don't take all of the output. + // Part of the output is not taken on the last iteration let remaining = L - bytes_written; - // todo: might need this to be new_allow_weak_key() + // TODO: might need this to be new_allow_weak_key() let mut hmac = HMAC::::new(&prk_as_mac_key)?; hmac.do_update(&T[..t_len]); hmac.do_update(info); @@ -435,8 +435,8 @@ impl HKDF { out[bytes_written..bytes_written + t_len].copy_from_slice(&T[..t_len]); bytes_written += t_len; - // set the KeyType of the output - // since we've done some computation, the result will not actually be zeroized, even if all input key material was zeroized. + // Set the KeyType of the output + // Since some computation has been performed, the result will not actually be zeroized, even if all input key material was zeroized. if prk.key_type() == KeyType::Zeroized { okm.set_key_type(KeyType::BytesLowEntropy)?; } else { @@ -487,7 +487,7 @@ impl HKDF { }; // Often HMAC is initialized with a zero salt, - // So we're gonna ignore key strength errors here + // Key strength errors are ignored here. // This will all be tabulated correctly via entropy.credit_entropy() self.hmac = Some(HMAC::::new_allow_weak_key(salt)?); diff --git a/crypto/hkdf/tests/hkdf_tests.rs b/crypto/hkdf/tests/hkdf_tests.rs index 1e13d37..e25624e 100644 --- a/crypto/hkdf/tests/hkdf_tests.rs +++ b/crypto/hkdf/tests/hkdf_tests.rs @@ -371,7 +371,7 @@ mod hkdf_tests { assert_eq!(okm.key_type(), KeyType::BytesFullEntropy); // success case -- insufficient entropy due to key types -- KeyType::BytesLowEntropy - // Note: will still return MACKey because that one was first in the inputs. + // Note: this will still return MACKey because that one was first in the inputs. let salt = KeyMaterial128::from_bytes_as_type(&DUMMY_SEED_512[..16], KeyType::MACKey).unwrap(); let ikm = @@ -381,7 +381,7 @@ mod hkdf_tests { _ = HKDF_SHA256::extract_and_expand_out(&salt, &ikm, &[], 32, &mut okm); assert_eq!(okm.key_type(), KeyType::BytesLowEntropy); - // no way to test this on derive_out + // derive_key_out can't reproduce the two-input salt+ikm arrangement let keys = [&salt, &ikm]; _ = HKDF_SHA256::new().derive_key_from_multiple_out(&keys, &[], &mut okm); diff --git a/crypto/hmac/tests/hmac_tests.rs b/crypto/hmac/tests/hmac_tests.rs index a3d8e8c..62de534 100644 --- a/crypto/hmac/tests/hmac_tests.rs +++ b/crypto/hmac/tests/hmac_tests.rs @@ -92,7 +92,7 @@ mod hmac_tests { ) .unwrap(); assert_eq!(short_key.security_strength(), SecurityStrength::_112bit); - // key is too short, so we expect it to fail + // key is too short, so it is expected to fail match HMAC::::new(&short_key) { Err(MACError::KeyMaterialError(KeyMaterialError::SecurityStrength(_))) => { /* good */ } _ => panic!( @@ -100,10 +100,10 @@ mod hmac_tests { ), } - // but this'll work fine + // It works after allowing weak keys HMAC::::new_allow_weak_key(&short_key).unwrap(); - // as will a long enough key + // It works with a long enough key let key = KeyMaterial256::from_bytes_as_type( &hex::decode("0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b").unwrap(), KeyType::MACKey, @@ -143,11 +143,11 @@ mod hmac_tests { #[test] fn security_strength_tests() { - // test: provided key has the correct length, but insufficient tagged security strength + // Test: provided key has the correct length, but insufficient tagged security strength // HMAC should still work, but should return an error // it works with a zero key (as new_allow_weak_key) - // zero-len ey + // zero-len key let mut zero_key = KeyMaterial256::default(); HMAC_SHA256::new_allow_weak_key(&zero_key).unwrap(); @@ -156,14 +156,14 @@ mod hmac_tests { zero_key.set_key_len(32).unwrap(); HMAC_SHA256::new_allow_weak_key(&zero_key).unwrap(); - // but we don't allow zero-len keys that are not Zeroized or MACKey + // Note: zero-len keys that are not Zeroized or MACKey are not allowed // init let mut key = KeyMaterial512::from_bytes_as_type(&DUMMY_SEED_512[..64], KeyType::MACKey).unwrap(); assert_eq!(key.security_strength(), SecurityStrength::_256bit); key.set_security_strength(SecurityStrength::_128bit).unwrap(); - // complains at first + // The call should fail, as the key's security strength is set below the required threshold match HMAC::::new(&key) { Err(MACError::KeyMaterialError(KeyMaterialError::SecurityStrength(_))) => { /* fine */ } _ => { @@ -172,7 +172,7 @@ mod hmac_tests { ) } } - // but fine if you set .allow_weak_keys() + // It passes after setting .allow_weak_keys() let mut hmac = HMAC::::new_allow_weak_key(&key).unwrap(); hmac.do_update(b"Hi There"); hmac.do_final(); @@ -180,7 +180,7 @@ mod hmac_tests { // one-shot APIs still work with a weak key let out = HMAC::::new_allow_weak_key(&key).unwrap().mac(b"Hi There"); assert!(HMAC::::new_allow_weak_key(&key).unwrap().verify(b"Hi There", &out)); - // but fine if you set .allow_weak_keys() + // likewise with pre-allocated buffers let mut out = [0u8; 64]; HMAC::::new_allow_weak_key(&key).unwrap().mac_out(b"Hi There", &mut out).unwrap(); assert!(HMAC::::new_allow_weak_key(&key).unwrap().verify(b"Hi There", &out)); @@ -473,7 +473,7 @@ mod hmac_tests { ) .unwrap(); let mut out = [0u8; 128 / 8]; - // Key is shorter than HMAC security strength, so need to use new_allow_weak_keys() + // Key is shorter than HMAC security strength, so it needs to use new_allow_weak_keys() let hmac = HMAC::::new_allow_weak_key(&key).unwrap(); hmac.mac_out(b"Test With Truncation", &mut out).unwrap(); assert_eq!(&Vec::from(out), &hex::decode("3abf34c3503b2a23a46efc619baef897").unwrap()); diff --git a/crypto/mldsa/src/aux_functions.rs b/crypto/mldsa/src/aux_functions.rs index 3164838..f576594 100644 --- a/crypto/mldsa/src/aux_functions.rs +++ b/crypto/mldsa/src/aux_functions.rs @@ -536,8 +536,8 @@ pub(crate) fn sample_in_ball( let mut j = [0u8]; for i in (N - TAU as usize)..N { // 7: (ctx, 𝑗) ← H.Squeeze(ctx, 1) - // Note: you would think that this would be faster to pre-squeeze a buffer outside the loop, but in testing it - // doesn't make a difference. + // Note: Even though it may appear that pre-squeezing a buffer outside the loop would be faster, + // it doesn't make a noticeable difference after testing it h.squeeze_out(&mut j); // 8: while 𝑗 > 𝑖 do @@ -576,7 +576,7 @@ pub(crate) fn sample_in_ball( /// Algorithm 30 RejNTTPoly(𝜌) /// This is supposed to take a rho: [u8; 34], which is: 𝜌||IntegerToBytes(𝑠, 1)||IntegerToBytes(𝑟, 1) /// but to avoid needing to copy bytes and allocate more memory, -/// we'll split that into a [u8;32] and a [u8;2] +/// that is split into a [u8;32] and a [u8;2] pub(crate) fn rej_ntt_poly(rho: &[u8; 32], nonce: &[u8; 2]) -> Polynomial { let mut w_hat = Polynomial::new(); let mut j: usize = 0; @@ -584,9 +584,9 @@ pub(crate) fn rej_ntt_poly(rho: &[u8; 32], nonce: &[u8; 2]) -> Polynomial { g.absorb(rho); g.absorb(nonce); - // SHAKE is fairly inefficient if you just squeeze 3 bytes at a time, so we'll do a block. - // size doesn't really matter, so long as it's a multiple of 3. - // 288 seemed to be the sweet spot from playing with benchmarks + // SHAKE is fairly inefficient if only 3 bytes are squeezed at a time, so instead this implementation does a block. + // Size is not a limitation, so long as it's a multiple of 3. + // 288 seemed to be the sweet spot after some experimentation and benchmarking // It's probably around the average rejection rate, and 288 is a multiple of both 3 (required for this alg) // and 8 (efficient for SHAKE). let mut s = [0u8; 288]; @@ -621,7 +621,7 @@ pub(crate) fn rej_ntt_poly(rho: &[u8; 32], nonce: &[u8; 2]) -> Polynomial { /// /// This is supposed to take a rho: [u8; 66], which is: 𝜌||IntegerToBytes(𝑠, 1)||IntegerToBytes(𝑟, 1) /// but to avoid needing to copy bytes and allocate more memory, -/// we'll split that into a [u8;64] and a [u8;2] +/// that is split into a [u8;64] and a [u8;2] pub(crate) fn rej_bounded_poly(rho: &[u8; 64], nonce: &[u8; 2]) -> Polynomial { let mut a = Polynomial::new(); let mut j: usize = 0; @@ -820,7 +820,7 @@ pub(crate) fn decompose(r: i32) -> (i32, i32) { r1 = r - r0 * 2 * GAMMA2; // mutants note: the choice of (q - 1) is a bit arbitrary in that after doing the bit-shifting, - // this seems to work out mathematically equivalent if you do q/2, or (q+3)/2, but we'll leave it as (q-1)/2 + // this seems to work out mathematically equivalent if doing q/2, or (q+3)/2, but here it is left as (q-1)/2 // since that's algorithmically correct, and just ignore the mutants results. r1 -= (((q - 1) / 2 - r1) >> 31) & q; @@ -903,7 +903,7 @@ pub(super) fn use_hint(a: i32, hint: i32) -> i32 { match GAMMA2 { MLDSA44_GAMMA2 => { // mutants note: this passes unit tests if it's a1 >= 0 - // we'll leave it like this because it matches the spec + // it is left like this because it matches the spec if a1 > 0 { if a0 == 43 { 0 } else { a0 + 1 } } else { @@ -913,7 +913,7 @@ pub(super) fn use_hint(a: i32, hint: i32) -> i32 { // ML-DSA65 and 87 have the same GAMMA2 MLDSA65_GAMMA2 => { // mutants note: this passes unit tests if it's a1 >= 0 - // we'll leave it like this because it matches the spec + // it is left like this because it matches the spec if a1 > 0 { (a0 + 1) & 15 } else { (a0 - 1) & 15 } } _ => { @@ -949,7 +949,7 @@ pub(crate) fn use_hint_vecs( /// Input: 𝑎, 𝑏 ∈ 𝑇𝑞. /// Output: 𝑐 ∈ 𝑇𝑞. /// Multiply the coefficients in this polynomial by those in another polynomial and perform montgomery reduction. -/// Also called pointwise montgomery multiplication +/// Also called pointwise Montgomery multiplication pub(crate) fn multiply_ntt(a: &Polynomial, b: &Polynomial) -> Polynomial { let mut out = Polynomial::new(); for i in 0..N { diff --git a/crypto/mldsa/src/hash_mldsa.rs b/crypto/mldsa/src/hash_mldsa.rs index 52fbdc8..9c0358b 100644 --- a/crypto/mldsa/src/hash_mldsa.rs +++ b/crypto/mldsa/src/hash_mldsa.rs @@ -1,6 +1,6 @@ //! This implements the HashML-DSA algorithm specified in FIPS 204 which is useful for cases -//! where you need to process the to-be-signed message in chunks, and you cannot use the external mu -//! mode of [MLDSA]; possibly because you have to digest the message before you know which public key +//! where the user needs to process the to-be-signed message in chunks, and they use the external mu +//! mode of [MLDSA]; possibly because the message needs to be digested before knowing which public key //! will sign it. //! //! HashML-DSA is a full signature algorithm implementing the [Signature] trait: @@ -15,7 +15,7 @@ //! let (pk, sk) = HashMLDSA65_with_SHA512::keygen().unwrap(); //! //! let sig = HashMLDSA65_with_SHA512::sign(&sk, msg, None).unwrap(); -//! // This is the signature value that you can save to a file or whatever you need. +//! // This is the signature value that can be saved to a file or whatever it is need. //! //! match HashMLDSA65_with_SHA512::verify(&pk, msg, None, &sig) { //! Ok(()) => println!("Signature is valid!"), @@ -24,7 +24,7 @@ //! } //! ``` //! -//! But you also have access to the pre-hashed function available from [PHSignature]: +//! But the user also has access to the pre-hashed function available from [PHSignature]: //! //! ```rust //! use bouncycastle_core::errors::SignatureError; @@ -34,7 +34,7 @@ //! //! let msg = b"The quick brown fox jumped over the lazy dog"; //! -//! // Here, and in contrast to External Mu mode of ML-DSA, we can pre-hash the message before +//! // Here, and in contrast to External Mu mode of ML-DSA, the message can be pre-hashed before //! // even generating the signing key. //! let ph: [u8; 64] = SHA512::default().hash(msg).as_slice().try_into().unwrap(); //! @@ -42,7 +42,7 @@ //! let (pk, sk) = HashMLDSA65_with_SHA512::keygen().unwrap(); //! //! let sig = HashMLDSA65_with_SHA512::sign_ph(&sk, &ph, None).unwrap(); -//! // This is the signature value that you can save to a file or whatever you need. +//! // This is the signature value that can be saved to a file or whatever it is need. //! //! // This verifies either through the usual one-shot API of the [Signature] trait //! match HashMLDSA65_with_SHA512::verify(&pk, msg, None, &sig) { @@ -60,7 +60,7 @@ //! ``` //! //! Note that the [HashMLDSA] object is just a light wrapper around [MLDSA], and, for example, they share key types, -//! so if you need the fancy keygen functions, just use them from [MLDSA]. +//! so if more sophisticated keygen functions are needed, just use them from [MLDSA]. //! But a simple [HashMLDSA::keygen] is provided in order to have conformance to the [Signature] trait. use crate::mldsa::{H, MLDSA_MU_LEN, MLDSA_RND_LEN, MLDSATrait}; @@ -327,8 +327,8 @@ impl Algorithm for HashMLDSA87_with_SHA512 { /// 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 +/// The code is exposing the HashMLDSA struct this way so that alternative hash functions can be used +/// without requiring modification of this source code; the user can add their 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'. pub struct HashMLDSA< @@ -375,7 +375,7 @@ pub struct HashMLDSA< hash: HASH, /// Since HashML-DSA does message buffering in the external pre-hash, not in mu, - /// we'll need to save this for later + /// this needs to be saved for later ctx: [u8; 255], ctx_len: usize, } @@ -513,9 +513,9 @@ impl< /// /// Security note: /// This mode exposes deterministic signing (called "hedged mode" and allowed by FIPS 204). - /// The ML-DSA algorithm is considered safe to use in deterministic mode, but be aware that - /// the responsibility is on you to ensure that your nonce `rnd` is unique per signature. - /// If not, you may lose some privacy properties; for example it becomes easy to tell if a signer + /// The ML-DSA algorithm is considered safe to use in deterministic mode. However, the user must be aware + /// that is their responsibility to ensure that their nonce `rnd` is unique per signature. + /// If otherwise, some privacy properties may be lost; for example it becomes easy to tell if a signer /// has signed the same message twice or two different messages, or to tell if the same message /// has been signed by the same signer twice or two different signers. /// @@ -634,8 +634,8 @@ impl< } } - /// Alternative initialization of the streaming signer where you have your private key - /// as a seed and you want to delay its expansion as late as possible for memory-usage reasons. + /// Alternative initialization of the streaming signer where the user provides their private key + /// as a seed and they want to delay its expansion as late as possible to optimize memory-usage. pub fn sign_init_from_seed( seed: &KeyMaterial<32>, ctx: Option<&[u8]>, @@ -902,7 +902,8 @@ impl< if self.sk.is_none() && self.seed.is_none() { return Err(SignatureError::GenericError( - "Somehow you managed to construct a streaming signer without a private key, impressive!", + "sign_final_out called on a streaming context with no private key or seed; \ + this is a verify-initialized context. Call verify_final instead", )); } @@ -929,8 +930,8 @@ impl< HashDRBG_SHA512::new_from_os().next_bytes_out(&mut rnd)?; rnd }; - // since at this point we need to fully reconstruct SK in order to compute tr for mu anyway - // there is no savings to using the fancy MLDSA::sign_from_seed + // At this point it is not necessary to fully reconstruct SK in order to compute tr for mu. + // Therefore, there is no advantage in using MLDSA::sign_from_seed let (_pk, sk) = Self::keygen_from_seed(&self.seed.unwrap())?; Self::sign_ph_deterministic_out( &sk, @@ -1046,9 +1047,9 @@ impl< } /// Note that the PH expected here *is not the same* as the `mu` computed by [MuBuilder]. - /// To make use of this function, you need to compute a straight hash of the message using - /// the same hash function as the indicated in the HashML-DSA variant; for example SHA256 for - /// HashMDSA44_with_SHA256, SHA512 for HashMLDSA65_with_SHA512, etc. + /// To make use of this function, the user needs to compute a straight hash of the message using + /// the same hash function as the indicated in the HashML-DSA variant; + /// for example: SHA256 for HashMDSA44_with_SHA256; SHA512 for HashMLDSA65_with_SHA512; etc. fn sign_ph_out( sk: &SK, ph: &[u8; PH_LEN], diff --git a/crypto/mldsa/src/lib.rs b/crypto/mldsa/src/lib.rs index 06e37f9..92e16c3 100644 --- a/crypto/mldsa/src/lib.rs +++ b/crypto/mldsa/src/lib.rs @@ -106,10 +106,11 @@ //! to make this implementation constant-time, which generally means that the core mathematical algorithm //! code that handles secret data uses bitshift-and-xor type constructions instead of if-and-loop //! constructions. That should give this implementation reasonably good resistance to timing and -//! power analysis key extraction attacks, however: A) this is a "best-effort" and not formally verified, -//! and B) the Rust compiler does not guarantee constant-time behaviour no matter how clever your code, -//! so like all Safe Rust code (ie Rust code that does not include inline assembly), we are at the mercy -//! of the Rust compiler's optimizer for whether our bitshift-and-xor code actually remains +//! power analysis key extraction attacks, however: +//! A) this is a "best-effort" and not formally verified, and +//! B) the Rust compiler does not guarantee constant-time behaviour no matter how clever your code, +//! so like all Safe Rust code (ie Rust code that does not include inline assembly), it is up to +//! the Rust compiler's optimizer for whether our bitshift-and-xor code actually remains //! constant-time after compilation. #![no_std] diff --git a/crypto/mldsa/src/matrix.rs b/crypto/mldsa/src/matrix.rs index c0ef5d2..12cb393 100644 --- a/crypto/mldsa/src/matrix.rs +++ b/crypto/mldsa/src/matrix.rs @@ -60,7 +60,7 @@ impl Matrix { // Matrix and Vector do not need to impl Secret because the actual data is in the polynomials, which have their own zeroizing drop. // 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. +// but it is left zeroizing for now, and left also as a potential future optimization. #[derive(Clone)] pub(crate) struct Vector { @@ -190,7 +190,7 @@ impl Vector { /// Optimized from FIPS 204 to feed into the hash one row at a time to reduce overall memory footprint. pub(crate) fn w1_encode_and_hash(&self, h: &mut H) { // 1: 𝐰̃1 ← () - // don't need to allocate anything since we're feeding it into the hash row-wise + // Nothing needs to be allocated since it is being fed into the hash row-wise // 2: for 𝑖 from 0 to 𝑘 − 1 do // 3: 𝐰̃1 ← 𝐰̃1 || SimpleBitPack (𝐰1[𝑖], (𝑞 − 1)/(2𝛾2) − 1) diff --git a/crypto/mldsa/src/mldsa.rs b/crypto/mldsa/src/mldsa.rs index dd8eb0f..baf9eaf 100644 --- a/crypto/mldsa/src/mldsa.rs +++ b/crypto/mldsa/src/mldsa.rs @@ -14,8 +14,8 @@ //! //! let (pk, sk) = MLDSA65::keygen().unwrap(); //! -//! // Let's pretend this message was so long that you couldn't possibly -//! // stream the whole thing over a network, and you need it pre-hashed. +//! // For this example, assume that this message was so long that it is impractical to +//! // stream the whole text over a network, and therefore it needs to be pre-hashed. //! let msg_chunk1 = b"The quick brown fox "; //! let msg_chunk2 = b"jumped over the lazy dog"; //! @@ -23,9 +23,9 @@ //! signer.sign_update(msg_chunk1); //! signer.sign_update(msg_chunk2); //! let sig = signer.sign_final().unwrap(); -//! // This is the signature value that you can save to a file or whatever you need. +//! // This is the signature value that can be saved to a file or whatever is needed. //! -//! // This is compatible with a verifies that takes the whole message as one chunk: +//! // This is compatible with a verifier that takes the whole message as one chunk: //! let msg = b"The quick brown fox jumped over the lazy dog"; //! match MLDSA65::verify(&pk, msg, None, &sig) { //! Ok(()) => println!("Signature is valid!"), @@ -33,7 +33,8 @@ //! Err(e) => panic!("Something else went wrong: {:?}", e), //! } //! -//! // But of course there's also a streaming API for the verifier! +//! // There is also a streaming API for the verifier. +//! //! let mut verifier = MLDSA65::verify_init(&pk, None).unwrap(); //! verifier.verify_update(msg_chunk1); //! verifier.verify_update(msg_chunk2); @@ -56,8 +57,8 @@ //! //! let (pk, sk) = MLDSA65::keygen().unwrap(); //! -//! // Let's pretend this message was so long that you couldn't possibly -//! // stream the whole thing over a network, and you need it pre-hashed. +//! // For this example, assume that this message was so long that it is impractical to +//! // stream the whole text over a network, and therefore it needs to be pre-hashed. //! let msg_chunk1 = b"The quick brown fox "; //! let msg_chunk2 = b"jumped over the lazy dog"; //! @@ -83,14 +84,14 @@ //! produces signatures that are indistinguishable from "direct" ML-DSA mode. //! //! The one potential complication with external mu mode -- that [hash_mldsa] does not have -- -//! is that it requires you to know the public key that you are about to sign the message with. +//! is that it requires the user to know the public key that they are about to sign the message with. //! Or, more specifically, the hash of the public key `tr`. //! `tr` is a public value (derivable from the public key), so there is no harm in, for example, //! sending it down to a client device so that it can pre-hash a large message and only send the //! 64-byte `mu` value up to the server to be signed. //! But in some contexts, the message has to be pre-hashed for performance reasons but //! the public key that will be used for signing cannot be known in advance. -//! For those use cases, your only choice is to use [hash_mldsa]. +//! For those use cases, the only choice is to use [hash_mldsa]. //! //! This library exposes [MuBuilder] which can be used to pre-hash a large to-be-signed message //! along with the public key hash `tr`: @@ -102,17 +103,17 @@ //! //! let (pk, _) = MLDSA65::keygen().unwrap(); //! -//! // Let's pretend this message was so long that you couldn't possibly -//! // stream the whole thing over a network, and you need it pre-hashed. +//! // For this example, assume that this message was so long that it is impractical to +//! // stream the whole text over a network, and therefore it needs to be pre-hashed. //! let msg = b"The quick brown fox jumped over the lazy dog"; //! //! let mu: [u8; 64] = MuBuilder::compute_mu(&pk.compute_tr(), msg, None).unwrap(); //! ``` //! -//! Note: if you are going to bind a `ctx` value (explained below), then you need to do in in [MuBuilder::compute_mu]. +//! Note: in order to bind a `ctx` value (explained below), it is necessary to do in [MuBuilder::compute_mu]. //! -//! If the message really is so huge that you can't hold it all in memory at once, then you might prefer a streaming API for -//! computing mu: +//! If the message really is so huge that it can't be hold it all in memory at once, +//! then it might be preferable to use a streaming API for computing mu: //! //! ```rust //! use bouncycastle_core::errors::SignatureError; @@ -121,8 +122,8 @@ //! //! let (pk, _) = MLDSA65::keygen().unwrap(); //! -//! // Let's pretend this message was so long that you couldn't possibly -//! // stream the whole thing over a network, and you need it pre-hashed. +//! // For this example, assume that this message was so long that it is impractical to +//! // stream the whole text over a network, and therefore it needs to be pre-hashed. //! let msg_chunk1 = b"The quick brown fox "; //! let msg_chunk2 = b"jumped over the lazy dog"; //! @@ -132,7 +133,7 @@ //! let mu = mb.do_final(); //! ``` //! -//! Given a mu value, you can compute a signature that verifies as normal (no mu's required!): +//! Given a mu value, the user can compute a signature that verifies as normal (no mu's required!): //! //! ```rust //! use bouncycastle_core::errors::SignatureError; @@ -143,12 +144,12 @@ //! //! let (pk, sk) = MLDSA65::keygen().unwrap(); //! -//! // Assume this was computed somewhere else and sent to you. -//! // They would have had to know pk! +//! // Assume this was computed somewhere else, then +//! // the party that computed it would have had to know pk //! let mu: [u8; 64] = MuBuilder::compute_mu(&pk.compute_tr(), msg, None).unwrap(); //! //! let sig = MLDSA65::sign_mu(&sk, None, &mu).unwrap(); -//! // This is the signature value that you can save to a file or whatever you need. +//! // This is the signature value that can be saved to a file or whatever it is needed. //! //! match MLDSA65::verify(&pk, msg, None, &sig) { //! Ok(()) => println!("Signature is valid!"), @@ -159,7 +160,7 @@ //! ``` //! //! # Ctx and Rnd params -//! Various functions in this crate let you set the signing context value (`ctx`) and the signing nonce (`rnd`). +//! Various functions in this crate let the user set the signing context value (`ctx`) and the signing nonce (`rnd`). //! Let's talk about them both: //! //! ## ctx @@ -174,8 +175,7 @@ //! attacker to trick a verifier into accepting one in place of the other. //! In a network protocol, `ctx` could be used to bind a transaction ID or protocol nonce in order to strongly //! protect against replay attacks. -//! Generally, `ctx` is one of those things that if you don't know what it does, then you're probably -//! fine to ignore it. +//! Generally, it is safe to ignore any property about a `ctx` object that is not well understood. //! //! Example of signing and verifying with a `ctx` value: //! @@ -190,7 +190,7 @@ //! let (pk, sk) = MLDSA65::keygen().unwrap(); //! //! let sig = MLDSA65::sign(&sk, msg, Some(ctx)).unwrap(); -//! // This is the signature value that you can save to a file or whatever you need. +//! // This is the signature value that can be saved to a file or whatever it is needed. //! //! match MLDSA65::verify(&pk, msg, Some(ctx), &sig) { //! Ok(()) => println!("Signature is valid!"), @@ -201,16 +201,16 @@ //! //! ## rnd //! -//! This is the signature nonce, whose purpose is to ensure that you get different signature values -//! if you sign the same message with the same public key multiple times. +//! This is the signature nonce, whose purpose is to ensure that every time a signature is computed for the same +//! message, it results in a different value //! //! In general, the "deterministic" mode of ML-DSA (which usually uses an all-zero `rnd`) is considered -//! secure and safe to use but you may lose certain privacy properties, because, for example, -//! it becomes obvious that multiple identical signatures means that the same message was signed multiple times +//! secure and safe to use, however, certain privacy properties may be lost. For example, +//! it becomes evident that multiple identical signatures means that the same message was signed multiple times //! by the same private key. //! -//! The default mode of ML-DSA uses a `rnd` generated by the library's OS-backed RNG, but you can set the `rnd` -//! if you need to; for example if you are running on an embedded device that does not have access to an RNG. +//! The default mode of ML-DSA uses a `rnd` generated by the library's OS-backed RNG, the `rnd` can be set by the user +//! if necessary; for example if the function is run on an embedded device that does not have access to an RNG. //! //! Note that in order to avoid combinatorial explosion of API functions, setting the `rnd` value is only //! available in conjunction with external mu or streaming modes. The example of setting `rnd` on the streaming @@ -227,14 +227,14 @@ //! //! let (pk, sk) = MLDSA65::keygen().unwrap(); //! -//! // Assume this was computed somewhere else and sent to you. -//! // They would have had to know pk! +//! // Assume this was computed somewhere else, then +//! // the party that computed it would have had to know pk //! let mu: [u8; 64] = MuBuilder::compute_mu(&pk.compute_tr(), msg, None).unwrap(); //! -//! // Typically, "deterministic" mode of ML-DSA will use an all-zero rnd, -//! // but we've exposed it so you can set any value you need to. +//! // Typically, "deterministic" mode of ML-DSA will use an all-zero `rnd`, +//! // but here it is exposed it so it can be set any value, as needed. //! let sig = MLDSA65::sign_mu_deterministic(&sk, None, &mu, [0u8; 32]).unwrap(); -//! // This is the signature value that you can save to a file or whatever you need. +//! // This is the signature value that can saved to a file or whatever it is needed. //! //! match MLDSA65::verify(&pk, msg, None, &sig) { //! Ok(()) => println!("Signature is valid!"), @@ -248,7 +248,7 @@ //! Within the usual ML-DSA public key representation, the public matrix A is stored as a seed rho, which //! means that both the ML-DSA.sign() and ML-DSA.verify() operations need to expand it into a full matrix //! before performing the matrix multiplication. -//! We offer a version of the public and private key structs that pre-expand the public matrix for repeated use. +//! The code contains a version of the public and private key structs that pre-expand the public matrix for repeated use. //! //! The runtime of ML-DSA.sign() is dominated by the rejection sampling look, making the A-expansion //! a negligible part of the function -- accounting for only about 2% of the computation. @@ -285,19 +285,19 @@ //! //! This mode is intended for users with extreme performance or resource-limitation requirements. //! -//! A very careful analysis of the ML-DSA signing algorithm will show that you don't actually need -//! the entire ML-DSA private key to be in memory at the same time. In fact, it is possible to merge -//! the keygen() and sign() functions +//! A very careful analysis of the ML-DSA signing algorithm will show that +//! the entire ML-DSA private key does not need to be in memory at the same time. +//! In fact, it is possible to merge the keygen() and sign() functions //! -//! We provide [MLDSA::sign_mu_deterministic_from_seed] which implements such an algorithm. +//! The codebase contains [MLDSA::sign_mu_deterministic_from_seed] which implements such an algorithm. //! It has a significantly lower peak-memory-footprint than the regular signing API (although there's //! always room for more optimization), and according to our benchmarks it is only around 25% slower //! than signing with a fully-expanded private key -- which is still faster than performing a full //! keygen followed by a regular sign since there are intermediate values common to keygen and sign //! that the merged function is able to only compute once. //! -//! Since this is intended for hard-core embedded systems people, we have not wrapped this in all -//! the beginner-friendly APIs. If you need this, then we assume you know what you're doing! +//! Since this is intended for embedded systems specialists, the functions are not wrapped in +//! the beginner-friendly APIs. If a user needs this, then it is assumed they know what they are doing //! //! Example usage: //! @@ -315,18 +315,18 @@ //! KeyType::Seed, //! ).unwrap(); //! -//! // At some point, you'll need to compute the public key, both to get `tr`, and so other -//! // people can verify your signature. -//! // There's no possible short-cut to efficiently computing the public key or `tr` from the seed; -//! // you have to run the full keygen to get the full private key, at least momentarily, then -//! // you can discard it in only keep `tr` and `seed`. +//! // The public key is computed so that the signature can be verified by anyone. +//! // It also computes the hash `tr` of the public key to later be used to bind the public key at the time of signing. +//! // There is no short-cut to efficiently computing the public key or `tr` from the seed; +//! // The full keygen need to be run in order to get the full private key, at least momentarily, then +//! // it can be discarded and only keep `tr` and `seed`. //! let (pk, _) = MLDSA44::keygen_from_seed(&seed).unwrap(); //! let tr: [u8; 64] = pk.compute_tr(); //! -//! // Assume this was computed somewhere else and sent to you. -//! // They would have had to know pk! +//! // Assume this was computed somewhere else, then +//! // the party that computed it would have had to know pk //! let mu: [u8; 64] = MuBuilder::compute_mu(&tr, msg, None).unwrap(); -//! let rnd: [u8; 32] = [0u8; 32]; // with this API, you're responsible for your own nonce +//! let rnd: [u8; 32] = [0u8; 32]; // with this API, the user is responsible for their own nonce //! // because in the cases where this level of memory optimization //! // is needed, our RNG probably won't work anyway. //! @@ -637,8 +637,8 @@ impl Algorithm for MLDSA87 { } /// The core internal implementation of the ML-DSA algorithm. -/// This needs to be public for the compiler to be able to find it, but you shouldn't ever -/// need to use this directly. Please use the named public types. +/// This needs to be public for the compiler to be able to find it, however, there is no +/// need to be used directly. Please use the named public types. pub struct MLDSA< const PK_LEN: usize, const SK_LEN: usize, @@ -741,7 +741,7 @@ impl< /// Unlike other interfaces across the library that take an &impl KeyMaterial, this one /// 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 + /// If the seed is in a larger KeyMaterial, it has to be copied using /// [KeyMaterialTrait::from_key] pub(crate) fn keygen_internal(seed: &KeyMaterial256) -> Result<(PK, SK), SignatureError> { if !(seed.key_type() == KeyType::Seed || seed.key_type() == KeyType::BytesFullEntropy) @@ -818,7 +818,7 @@ impl< // Deviation from the FIPS: // Hold on to s1, s2, t0 in ntt form // Note: the result here is not necessarily in reduced form, but since .reduce() is expensive, - // we'll save that for the encode() operation since that's the only place that it matters + // it is saved for the encode() operation since that is the only place where it matters // to have them in normalized form. s2.ntt(); t0.ntt(); @@ -854,7 +854,7 @@ impl< // Already done -- the sk struct is already decoded and in NTT form // 5: 𝐀_hat ← ExpandA(𝜌) - // We're doing an optimization where the user can pre-expand A_hat within the + // It does an optimization where the user can pre-expand A_hat within the // public key object for faster repeated encapsulations against this public key. // 6: 𝜇 ← H(BytesToBits(𝑡𝑟)||𝑀 ′, 64) @@ -890,7 +890,8 @@ impl< loop { // FIPS 204 s. 6.2 allows: // "Implementations may limit the number of iterations in this loop to not exceed a finite maximum value." - // mutants note: there is no test for this because we don't know of a KAT that will exceed this limit. + // mutants note: there is no test for this because, at this point, + // we don't know of a KAT that will exceed this limit. if kappa > 1000 * k as u16 { return Err(SignatureError::GenericError( "Rejection sampling loop exceeded max iterations, try again with a different signing nonce.", @@ -937,7 +938,7 @@ impl< }; // 18: ⟨⟨𝑐𝐬1⟩⟩ ← NTT−1(𝑐_hat * 𝐬1_hat) - // Note: <<.>> in FIPS 204 means that this value will be used again later, so you should hang on to it. + // Note: <<.>> in FIPS 204 means that this value will be used again later, so this should be kept. let mut cs1 = sk.s1_hat().scalar_vector_ntt(&c_hat); cs1.inv_ntt(); @@ -947,8 +948,8 @@ impl< // 23 (first half): if ||𝐳||∞ ≥ 𝛾1 − 𝛽 or ||𝐫0||∞ ≥ 𝛾2 − 𝛽 then (z, h) ← ⊥ // ▷ validity checks - // out-of-order on purpose for performance reasons: - // might as well do the rejection sampling check before any extra heavy computation + // This is done out-of-order on purpose for performance reasons: + // rejection sampling check is done before any extra heavy computation if sig_val_z.check_norm::() { kappa += l as u16; continue; @@ -979,8 +980,8 @@ impl< ct0.inv_ntt(); // 28 (first half): if ||⟨⟨𝑐𝐭0⟩⟩||∞ ≥ 𝛾2 or the number of 1’s in 𝐡 is greater than 𝜔, then (z, h) ← ⊥ - // out-of-order on purpose for performance reasons: - // might as well do the rejection sampling check before any extra heavy computation + // This is done out-of-order on purpose for performance reasons: + // rejection sampling check is done before any extra heavy computation // mutants note: there is currently no unit test that triggers this branch if ct0.check_norm::() { kappa += l as u16; @@ -1220,10 +1221,16 @@ impl< Self::sign_mu_deterministic_from_seed_out(seed, mu, rnd, &mut out)?; Ok(out) } - /// This function is a mash-up of keyGen (Algorithm 6) and sign (Algorithm 7) - /// Although, while this algorithm is a precursor to the lowmemory implementation, I'm not - /// sure that it actually gains you anything over a keygen_from_seed() followed by a sign(), - /// and maybe I should change its implementation to that. + /// External-μ deterministic signing directly from a 32-byte seed (a mash-up of + /// KeyGen Alg 6 and Sign Alg 7). Because μ is supplied by the caller, this never + /// derives the public key: it skips pkEncode and tr = H(pk), never materializes + /// the PK/SK structs, and keeps peak live memory low via scoped temporaries (see below). + /// This is a middle ground between keygen_from_seed()+sign_mu() and + /// the fully streamed low-memory implementation. + // TODO: benchmark peak memory + runtime against + // keygen_from_seed() + sign_mu_deterministic() to confirm the separate path earns being kept. + // Note: this path intentionally avoids the public key entirely + // (no pkEncode / tr = H(pk)) since μ is supplied externally. fn sign_mu_deterministic_from_seed_out( seed: &KeyMaterial<32>, mu: &[u8; 64], @@ -1302,10 +1309,10 @@ impl< // Note on memory optimization: // A_hat consumes a large bit of memory and technically could move inside the loop -- // -- or even more aggressively, could be derived and multiplied by y_hat row-by-row -- - // But in my unit tests, I see the loop typically execute 1 - 3 times, sometimes as many + // But in my unit tests, it can be observed that the loop typically execute 1 - 3 times, sometimes as many // as 20 or even 80 times. So moving expandA() inside the loop would be a pretty drastic speed-for-memory tradeoff - // that I'm not willing to make in general, so I leave that as an optimization that people - // can make on a private fork if you really really need the memory squeeze. + // whose generality falls out of the scope of this implementation. + // It is left as an optimization that can be made by users that require further reduction of memory usage let A_hat = expandA::(&rho); // Alg 7; 8: 𝜅 ← 0 @@ -1381,7 +1388,8 @@ impl< y = { // scope for cs1 // Alg 7; 18: ⟨⟨𝑐𝐬1⟩⟩ ← NTT−1(𝑐_hat * 𝐬1_hat) - // Note: <<.>> in FIPS 204 means that this value will be used again later, so you should hang on to it. + // Note: <<.>> in FIPS 204 means that this value will be used again later, + // so it is better to keep it. let mut cs1 = s1_hat.scalar_vector_ntt(&c_hat); cs1.inv_ntt(); @@ -1400,8 +1408,8 @@ impl< // Alg 7; 23 (first half): if ||𝐳||∞ ≥ 𝛾1 − 𝛽 or ||𝐫0||∞ ≥ 𝛾2 − 𝛽 then (z, h) ← ⊥ // ▷ validity checks - // out-of-order on purpose for performance reasons: - // might as well do the rejection sampling check before any extra heavy computation + // This is done out-of-order on purpose for performance reasons: + // rejection sampling check is done before any extra heavy computation if sig_val_z.check_norm::() { kappa += l as u16; continue; @@ -1421,7 +1429,7 @@ impl< // 21: 𝐫0 ← LowBits(𝐰 − ⟨⟨𝑐𝐬2⟩⟩) let r0 = w.sub_vector(&cs2).low_bits::(); - // while we have s2_hat in scope, derive t0 + // while s2_hat is in scope, derive t0 let mut t = t_hat; t.inv_ntt(); t.add_vector_ntt(&s2); @@ -1439,7 +1447,7 @@ impl< // Alg 7; 23 (second half): if ||𝐳||∞ ≥ 𝛾1 − 𝛽 or ||𝐫0||∞ ≥ 𝛾2 − 𝛽 then (z, h) ← ⊥ // ▷ validity checks if r0.check_norm::() { - // mutants note: mutants thinks you can replace this with -=, but in practice that makes + // mutants note: mutants thinks this can be replaced with -=, but in practice that makes // the rejection sampling loop go forever, so is a false positive. kappa += l as u16; continue; @@ -1491,7 +1499,8 @@ impl< // zeroize rho_p_p before returning it to the OS rho_p_p.fill(0u8); - // sig_encode does not necessarily write to all bytes of the output, so just to be safe: + // sig_encode does not necessarily write to all bytes of the output, + // The following is done for safety output.fill(0u8); // Alg 7; 33: 𝜎 ← sigEncode(𝑐, 𝐳̃ mod±𝑞, 𝐡) @@ -1575,7 +1584,7 @@ impl< // 5: 𝐀 ← ExpandA(𝜌) // ▷ 𝐀 is generated and stored in NTT representation as 𝐀 - // We're doing an optimization where the user can pre-expand A_hat within the + // The code performs an optimization where the user can pre-expand A_hat within the // public key object for faster repeated encapsulations against this public key. // 6: 𝑡𝑟 ← H(𝑝𝑘, 64) @@ -1787,8 +1796,8 @@ pub trait MLDSATrait< /// Security note about deterministic mode: /// This mode exposes deterministic signing (called "hedged mode" and allowed by FIPS 204). /// The ML-DSA algorithm is considered safe to use in deterministic mode, but be aware that - /// the responsibility is on you to ensure that your nonce `rnd` is unique per signature. - /// If not, you may lose some privacy properties; for example it becomes easy to tell if a signer + /// the responsibility is on the user to ensure that the nonce `rnd` is unique for each signature. + /// If not, some privacy properties may be lost; for example it becomes easy to tell if a signer /// has signed the same message twice or two different messagase, or to tell if the same message /// has been signed by the same signer twice or two different signers. /// @@ -1817,8 +1826,8 @@ pub trait MLDSATrait< /// Security note about deterministic mode: /// This mode exposes deterministic signing (called "hedged mode" and allowed by FIPS 204). /// The ML-DSA algorithm is considered safe to use in deterministic mode, but be aware that - /// the responsibility is on you to ensure that your nonce `rnd` is unique per signature. - /// If not, you may lose some privacy properties; for example, it becomes easy to tell if a signer + /// the responsibility is on the user to ensure that the nonce `rnd` is unique for each signature. + /// If not, some privacy properties may be lost; for example, it becomes easy to tell if a signer /// has signed the same message twice or two different messages, or to tell if the same message /// has been signed by the same signer twice or two different signers. /// @@ -1850,8 +1859,8 @@ pub trait MLDSATrait< /// To be used for deterministic signing in conjunction with the [MLDSA44::sign_init], [MLDSA44::sign_update], and [MLDSA44::sign_final] flow. /// Can be set anywhere after [MLDSA44::sign_init] and before [MLDSA44::sign_final]. fn set_signer_rnd(&mut self, rnd: [u8; 32]); - /// Alternative initialization of the streaming signer where you have your private key - /// as a seed and you want to delay its expansion as late as possible for memory-usage reasons. + /// Alternative initialization of the streaming signer where the user has their private key + /// as a seed and they want to delay its expansion as late as possible for memory-usage reasons. fn sign_init_from_seed( seed: &KeyMaterial<32>, ctx: Option<&[u8]>, @@ -1974,7 +1983,8 @@ impl< if self.sk.is_none() && self.seed.is_none() { return Err(SignatureError::GenericError( - "Somehow you managed to construct a streaming signer without a private key, impressive!", + "sign_final_out called on a streaming context with no private key or seed; \ + this is a verify-initialized context. Call verify_final instead", )); } @@ -2061,7 +2071,7 @@ impl< /// /// Note: this struct is only exposed for "pure" ML-DSA and not for HashML-DSA because HashML-DSA /// does not benefit from allowing external construction of the message representative mu. -/// You can get the same behaviour by computing the pre-hash `ph` with the appropriate hash function +/// The same behaviour can be obtained by computing the pre-hash `ph` with the appropriate hash function /// and providing that to HashMLDSA via [PHSignature::sign_ph]. pub struct MuBuilder { h: H, diff --git a/crypto/mldsa/src/mldsa_keys.rs b/crypto/mldsa/src/mldsa_keys.rs index c1643ba..a5cc12c 100644 --- a/crypto/mldsa/src/mldsa_keys.rs +++ b/crypto/mldsa/src/mldsa_keys.rs @@ -589,7 +589,7 @@ impl(&self) -> bool { - // Fine that this is not constant-time (returns true early) because it is used in a rejection loop. - // IE the early quit here leads to rejection and continuing to the top of the rejection loop, or failing the signature validation. + // It is acceptable that this function is not constant-time (returns true early) + // The reason being because it is used in a rejection loop. + // That is, the early quit here leads to rejection and continuing to the top of the rejection loop, + // or failing the signature validation. // So the i32 that we just checked in a non-constant-time manner is about to get thrown away. // Note: this formulation of the check_norm function usually requires this bounds check // if bound > (q - 1) / 8 { // return true; // } - // but since BOUND is a constant here, we'll just do a debug_assert to make sure the value is what we expect. + // but since BOUND is a constant here, a debug_assert is performed to make sure the value is what we expect. debug_assert!(BOUND <= (q - 1) / 8); let mut t: i32; @@ -114,7 +117,7 @@ impl Polynomial { } } - /// Creates the hint vector, and also returns its hamming weight (ie the number of 1's). + /// Creates the hint vector, and also returns its hamming weight (i.e. the number of 1's). pub(crate) fn make_hint(&self, r: &Self) -> (Self, i32) { let mut out = Polynomial::new(); let mut count = 0i32; @@ -156,18 +159,18 @@ impl Polynomial { /// Algorithm 41 NTT(𝑤) /// Computes the NTT. - /// Input: Polynomial 𝑤(𝑋) - /// 𝑗=0 𝑤𝑗𝑋𝑗 ∈ 𝑅𝑞. + /// Input: Polynomial 𝑤(𝑋) = Σ_{j=0}^{255} 𝑤𝑗𝑋𝑗 ∈ 𝑅𝑞. /// Output: 𝑤_hat = (𝑤_hat\[0], ..., 𝑤_hat\[255]) ∈ 𝑇𝑞. /// - /// Note: by convention, variables holding the output of the NTT function should be named "_ntt" + /// Note: by convention, variables holding the output of the NTT function should be named "_hat" /// to indicate that they are in the NTT domain (sometimes called the frequency domain), not the natural domain. - /// I considered using the rust type system to enforce this, but it seemed like overkill, cause that's what - /// NIST test vectors are for. + /// Usage of the rust type system to enforce this is arguably unnecessary, since that's what the NIST + /// test vectors are for. /// - /// Design choice: don't do the NTT in-place, but copy data to a new array. - /// This uses slightly more memory and requires a copy, but makes the code easier to read - /// and less likely to contain a bug. But this optimization could be considered in the future. + /// Lazy reduction: the butterfly omits an explicit reduction modulo `q` + /// This is safe only because ‖input‖∞ ≤ q-1 (i.e. intermediates stay below ~5q + /// (well within i32) and the input of montgomery_reduce input stays below q·2^{31}) and + /// the final result is reduced downstream. pub(crate) fn ntt(&mut self) { let mut m: usize = 0; let mut len: usize = 128; @@ -180,7 +183,9 @@ impl Polynomial { for j in start..start + len { let t = montgomery_reduce(z as i64 * self[j + len] as i64); - self[j + len] = self[j] - t; // '% q' not strictly needed cause it gets reduced at some point later. Removing it gave +5% in benchmarking + // '% q' not strictly needed cause it gets reduced at some point later. + // Removing it gave +5% in benchmarking + self[j + len] = self[j] - t; self[j] = self[j] + t; // '% q' not strictly needed } start = start + 2 * len; @@ -189,11 +194,10 @@ impl Polynomial { } } - /// Algorithm 42 NTT−1(𝑤)̂ + /// Algorithm 42 NTT−1(𝑤_hat) /// Computes the inverse of the NTT. - /// Input: ̂̂ ̂ 𝑤 = (𝑤\[0], … , 𝑤\[255]) ∈ 𝑇𝑞. - /// Output: Polynomial 𝑤(𝑋) = ∑255 - /// 𝑗=0 𝑤𝑗𝑋𝑗 ∈ 𝑅𝑞 + /// Input: 𝑤_hat = (𝑤_hat[0], … , 𝑤_hat[255]) ∈ 𝑇𝑞. + /// Output: Polynomial 𝑤(𝑋) = Σ_{j=0}^{255} 𝑤𝑗𝑋𝑗 ∈ 𝑅𝑞 pub(crate) fn inv_ntt(&mut self) { let mut m: usize = N; let mut len: usize = 1; @@ -219,14 +223,20 @@ impl Polynomial { // 𝑤𝑗+𝑙𝑒𝑛 ← (𝑧 ⋅ 𝑤𝑗+𝑙𝑒𝑛) mod 𝑞 self[j + len] = montgomery_reduce(z as i64 * self[j + len] as i64); } - start = start + 2 * len; // could be optimized to save the multiply-by-two since j finishes as `start + len`. That said 2* is just << 1, which is basically free. + start = start + 2 * len; + // could be optimized to save the multiply-by-two since j finishes as `start + len`. + // That said 2* is just << 1, which is basically free. } len <<= 1; } - // f = 256^-1 mod q - // const f: i64 = 8347681; - // bc-java uses this value rather than the one in FIPS 204 + // Final 1/256 normalization, in Montgomery form to match the montgomery_reduce + // bookkeeping used by every butterfly above (each contributes a 2^-32 factor). + // Note: f != 256^-1 mod q = 8347681. That value is only correct + // applied as a plain multiply (FIPS 204 Alg 42: w_j <- f * w_j mod q). + // Here we apply f via montgomery_reduce(f * w_j), so the constant is the + // Montgomery-domain form: f = mont^2 / 256 mod q = 41978, mont = 2^32 mod q. + // Do NOT substitute 8347681, as it is invalid through montgomery_reduce. const f: i64 = 41978; for j in 0..N { // equiv. to the global constant N diff --git a/crypto/mldsa/tests/mldsa_tests.rs b/crypto/mldsa/tests/mldsa_tests.rs index b5a1b5f..bbc1b9e 100644 --- a/crypto/mldsa/tests/mldsa_tests.rs +++ b/crypto/mldsa/tests/mldsa_tests.rs @@ -50,7 +50,7 @@ mod mldsa_tests { } /// This runs the full bitflipping tests and takes several minutes. - /// I'm leaving this commented out, but feel free to un-comment it and run it. + /// It is left commented out, but the user can un-comment it and run it. // #[test] // fn test_framework_signature_extensive() { // @@ -80,13 +80,13 @@ mod mldsa_tests { let expected_pk_bytes: [u8; MLDSA44_PK_LEN] = hex::decode("d7b2b47254aae0db45e7930d4a98d2c97d8f1397d1789dafa17024b316e9bec94fc9946d42f19b79a7413bbaa33e7149cb42ed5115693ac041facb988adeb5fe0e1d8631184995b592c397d2294e2e14f90aa414ba3826899ac43f4cccacbc26e9a832b95118d5cb433cbef9660b00138e0817f61e762ca274c36ad554eb22aac1162e4ab01acba1e38c4efd8f80b65b333d0f72e55dfe71ce9c1ebb9889e7c56106c0fd73803a2aecfeafded7aa3cb2ceda54d12bd8cd36a78cf975943b47abd25e880ac452e5742ed1e8d1a82afa86e590c758c15ae4d2840d92bca1a5090f40496597fca7d8b9513f1a1bda6e950aaa98de467507d4a4f5a4f0599216582c3572f62eda8905ab3581670c4a02777a33e0ca7295fd8f4ff6d1a0a3a7683d65f5f5f7fc60da023e826c5f92144c02f7d1ba1075987553ea9367fcd76d990b7fa99cd45afdb8836d43e459f5187df058479709a01ea6835935fa70460990cd3dc1ba401ba94bab1dde41ac67ab3319dcaca06048d4c4eef27ee13a9c17d0538f430f2d642dc2415660de78877d8d8abc72523978c042e4285f4319846c44126242976844c10e556ba215b5a719e59d0c6b2a96d39859071fdcc2cde7524a7bedae54e85b318e854e8fe2b2f3edfac9719128270aafd1e5044c3a4fdafd9ff31f90784b8e8e4596144a0daf586511d3d9962b9ea95af197b4e5fc60f2b1ed15de3a5bef5f89bdc79d91051d9b2816e74fa54531efdc1cbe74d448857f476bcd58f21c0b653b3b76a4e076a6559a302718555cc63f74859aabab925f023861ca8cd0f7badb2871f67d55326d7451135ad45f4a1ba69118fbb2c8a30eec9392ef3f977066c9add5c710cc647b1514d217d958c7017c3e90fd20c04e674b90486e9370a31a001d32f473979e4906749e7e477fa0b74508f8a5f2378312b83c25bd388ca0b0fff7478baf42b71667edaac97c46b129643e586e5b055a0c211946d4f36e675bed5860fa042a315d9826164d6a9237c35a5fbf495490a5bd4df248b95c4aae7784b605673166ac4245b5b4b082a09e9323e62f2078c5b76783446defd736ad3a3702d49b089844900a61833397bc4419b30d7a97a0b387c1911474c4d41b53e32a977acb6f0ea75db65bb39e59e701e76957def6f2d44559c31a77122b5204e3b5c219f1688b14ed0bc0b801b3e6e82dcd43e9c0e9f41744cd9815bd1bc8820d8bb123f04facd1b1b685dd5a2b1b8dbbf3ed933670f095a180b4f192d08b10b8fabbdfcc2b24518e32eea0a5e0c904ca844780083f3b0cd2d0b8b6af67bc355b9494025dc7b0a78fa80e3a2dbfeb51328851d6078198e9493651ae787ec0251f922ba30e9f51df62a6d72784cf3dd205393176dfa324a512bd94970a36dd34a514a86791f0eb36f0145b09ab64651b4a0313b299611a2a1c48891627598768a3114060ba4443486df51522a1ce88b30985c216f8e6ed178dd567b304a0d4cafba882a28342f17a9aa26ae58db630083d2c358fdf566c3f5d62a428567bc9ea8ce95caa0f35474b0bfa8f339a250ab4dfcf2083be8eefbc1055e18fe15370eecb260566d83ff06b211aaec43ca29b54ccd00f8815a2465ef0b46515cc7e41f3124f09efff739309ab58b29a1459a00bce5038e938c9678f72eb0e4ee5fdaae66d9f8573fc97fc42b4959f4bf8b61d78433e86b0335d6e9191c4d8bf487b3905c108cfd6ac24b0ceb7dcb7cf51f84d0ed687b95eaeb1c533c06f0d97023d92a70825837b59ba6cb7d4e56b0a87c203862ae8f315ba5925e8edefa679369a2202766151f16a965f9f81ece76cc070b55869e4db9784cf05c830b3242c8312").unwrap() .try_into().unwrap(); - // Decode and re-encode the sk, make sure you get the same thing + // Decode and re-encode the sk, ensure the output matches let expected_sk = MLDSA44PrivateKey::from_bytes(&expected_sk_bytes).unwrap(); let sk_bytes = expected_sk.encode(); assert_eq!(sk_bytes.len(), expected_sk_bytes.len()); assert_eq!(sk_bytes, expected_sk_bytes.as_slice()); - // Decode and re-encode the pk, make sure you get the same thing + // Decode and re-encode the pk, ensure the output matches let decoded_pk = MLDSA44PublicKey::from_bytes(&expected_pk_bytes).unwrap(); let pk_bytes = decoded_pk.encode(); assert_eq!(pk_bytes.len(), expected_pk_bytes.len()); @@ -121,13 +121,13 @@ mod mldsa_tests { let expected_pk_bytes: [u8; MLDSA65_PK_LEN] = hex::decode("48683d91978e31eb3dddb8b0473482d2b88a5f625949fd8f58a561e696bd4c27d05b38dbb2edf01e664efd81be1ea893688ce68aa2d51c5958f8bbc6eb4e89ee67d2c0320954d57212cac7229ff1d6eaf03928bd51511f8d88d847736c7de2730d5978e5410713160978867711bf5539a0bfc4c350c2be572baf0ee2e2fb16ccfea08028d99ac49aebb75937ddce111cdab62fff3cea8ba2233d1e56fbc5c5a1e726de63fadd2af016b119177fa3d971a2d9277173fce55b67745af0b7c21d597dbeb93e6a32f341c49a5a8be9e825088d1f2aa45155d6c8ae15367e4eb003b8fdf7851071949739f9fff09023eaf45104d2a84a45906eed4671a44dc28d27987bb55df69e9e8561f61a80a72699503865fed9b7ee72a8e17a19c408144f4b29afef7031c3a6d8571610b42c9f421245a88f197e16812b031159b65b9687e5b3e934c5225ae98a79ba73d2b399d73510effad19e53b8450f0ba8fce1012fd98d260a74aaaa13fae249a006b1c34f5ba0b882f26378222fb36f2283c243f0ffeb5f1bb414a0a70d55e3d40a56b6cbc88ae1f03b7b2882d98deea28e145c9dedfd8eaf1cef2ed94a8b050f8964f46d1ea0d0c2a43e0dda6182adbf4f6ed175b6742257859bf22f3a417ecf1f9d89317b5e539d587af16b9e1313e04514ffa64ba8b3ff2b8321f8811cb3fb022c8f644e70a4b80a2fbfee604abb7379091ea8e6c5c74dfc0283666b40c0793870028204a136bf5da9568eb798d349038bdb0c11e03445e7847cb5069c75cf28ac601c7799d958210ddbcb226e51afef9f1de47b073873d6d3f97456bede085082e74a298b2cd48f4b3093155f366c8fa601c6af858dfa32c08491b2a29887f90335949a5d6edaa679882a3a95d6bf6d970a221f4b9d3d8cbf384af81aac95e2b3294e04789ac83727a5dc04559f96af41d8a053516feeeebc52746eb6ab2819e09108710d835f011fa63065872ad334d5cdffb2b2310507e92fc993ae317da97f4f309cdaf0f67ed99d90215576083849f953b246d7fedb3fdb67679850a5ad404e64147fb7cf4f6aeddd05afb4b834968d1fe88014960dce5d942236526e12a478d69e5fbe6970310b308c06845018cfc7b2ab430a13a6b1ac7bb02cccbb3d911ac2f11068613fbe029bfdce02cf5cd38950ed72c83944edfbc75615af87f864c051f3c55456c5412863a40c06d1dab562bdff0571b8d3c3917bbd300880bba5e998239b95fa91b7d6416d4f398b3adbcd30983ed3592b4d9ef7d4236fd00f50d98aa53a235ac4172720f77d96172672980cfe8ff7a5a702783edc2ba31b2259015a112fc7f468a9c2f9464039002d30ef678b4cb798bc116216bf7a9a7c18ba03b7b58fd07515d3115049d3614be7a07e744300750df1d2c58753389059eafc3d785ccdd31c07648bedc03a5c3b8ad46d064d59c13d57374729fc4e295362e2a5191204530428bc1522afa28ff5fe1655e304ca5bc8c27ad0e0c6a39dd4df28956c14b38cc93682cefe402bbd5e82d29c464e44eb5d37b48fc568dfe0cc6e8e16baea05e5135590f19294e73e8367b0216dbb815030b9de55913f08039c42351c59e5515dd5af8e089a15e625e8f6dee639386c46497d7a263288774de581a7de9629b41b4424141f978fb8331208efdec3c6e0de39bc57063f3dcd6c470373c08891ea29cbc7cc6d6483b8889083ace86aa7b51b1c2cfe6e2ad18d97ce36fbc56ea42fae97e6a7ac114864478c366df1ebb1e7b11a9098504fd5975bdf1f49dc70002b63c1739a9d263fbad4073f6a9f6c2b8af4b4c332a103a0cffa5deeb2d062ca3c215fd360026be7c5164f4a4424ef74948804d66f46487732c8202c795478647b4ea71d627c086024cca354a41f0877b38f19b3774ad2095c8da53b069e21c76ae2d2007e16719ed40080d334f7da52e9f5a5990439caf083a95b833f02ad10a08c1a6d0f260c007285bd4a2f47703a5aef465287d253b18ac22514316210ff566814b10f87a293d6f199d3c3959990d0c1268b4f50d5f9fcefbbf237bd0c28b80182d6659741f14f10bfbb21bba12ab620aa2396f56c0686b4ea9017990224216b2fe8ad76c4a9148eef9a86a3635a6aa77bc1dcfb6fba59a77dfda9b7530dc0ca8648c8d973738e01bab8f08b4905e84aa4641bd602410cd97520265f2f231f2b35e15eb2fa04d2bd94d5a77abaf1e0e161010a990087f5b46ea988b2bc0512fda0fa923dadd6c45c5301d09483673265b5ab2e10f4ba520f6bbad564a5c3d5e27bdb080f7d20e13296a3181954c39c649c943ebe17df5c1f7aae0a8fe126c477585a5d4d648a0d008b6af5e8cd31be69a9296d4f3fd25ed86f221e4b93f65f5929967533624b9235750c30707550b58536d109a7131c5a5bbe4a5715567c12534aec7660761eebb9fae2891c774589b80e566ad557ddef7367196b7227ea9870ef09ddfec79d6b9319a6879b5205d76bf7aba5acf33afb59d17fc54e68383d6be5a08e9b66da53dcde008bb294b8582bd132cdcc49959fdbc21e52721880c8ad0352c79f03a43bbd84c4cdfdc6c529005e1e7cd9a349a7168a35569ba5dea818968d5a91466bd6e64e20bf62417198afc4e81c28dd77ed4028232398b52fbde86bc84f475b9016710ce2aabc11a06b4dbac901ec16cf365ca3f2d53813948a693a0f93e79c46ca5d5a6dca3d28ca50ad18bd13fca55059dd9b185f79f9c47196a4e81b2104bc460a051e02f2e8444f").unwrap() .try_into().unwrap(); - // Decode and re-encode the sk, make sure you get the same thing + // Decode and re-encode the sk, ensure the output matches let decoded_sk = MLDSA65PrivateKey::from_bytes(&expected_sk_bytes).unwrap(); let sk_bytes = decoded_sk.encode(); assert_eq!(sk_bytes.len(), expected_sk_bytes.len()); assert_eq!(sk_bytes, expected_sk_bytes.as_slice()); - // Decode and re-encode the pk, make sure you get the same thing + // Decode and re-encode the pk, ensure the output matches let expected_pk = MLDSA65PublicKey::from_bytes(&expected_pk_bytes).unwrap(); let pk_bytes = expected_pk.encode(); assert_eq!(pk_bytes.len(), expected_pk_bytes.len()); @@ -162,13 +162,13 @@ mod mldsa_tests { let expected_pk_bytes: [u8; MLDSA87_PK_LEN] = hex::decode("9792bcec2f2430686a82fccf3c2f5ff665e771d7ab41b90258cfa7e90ec97124a73b323b9ba21ab64d767c433f5a521effe18f86e46a188952c4467e048b729e7fc4d115e7e48da1896d5fe119b10dcddef62cb307954074b42336e52836de61da941f8d37ea68ac8106fabe19070679af6008537120f70793b8ea9cc0e6e7b7b4c9a5c7421c60f24451ba1e933db1a2ee16c79559f21b3d1b8305850aa42afbb13f1f4d5b9f4835f9d87dfceb162d0ef4a7fdc4cba1743cd1c87bb4967da16cc8764b6569df8ee5bdcbffe9a4e05748e6fdf225af9e4eeb7773b62e8f85f9b56b548945551844fbd89806a4ac369bed2d256100f688a6ad5e0a709826dc4449e91e23c5506e642361ef5a313712f79bc4b3186861ca85a4bab17e7f943d1b8a333aa3ae7ce16b440d6018f9e04daf5725c7f1a93fad1a5a27b67895bd249aa91685de20af32c8b7e268c7f96877d0c85001135a4f0a8f1b8264fa6ebe5a349d8aecad1a16299ccf2fd9c7b85bace2ced3aa1276ba61ee78ed7e5ca5b67cdd458a9354030e6abbbabf56a0a2316fec9dba83b51d42fd3167f1e0f90855d5c66509b210265dc1e54ec44b43ba7cf9aef118b44d80912ce75166a6651e116cebe49229a7062c09931f71abd2293f76f7efc3215ba97800037e58e470bdbbb43c1b0439eaf79c54d93b44aac9efe9fbe151874cfb2a64cbee28cc4c0fe7775e5d870f1c02e5b2e3c5004c995f24c9b779cb753a277d0e71fd425eb6bc2ca56ce129db51f70740f31e63976b50c7312e9797d78c5b1ac24a5fa347cc916e0a83f5c3b675cd30b81e3fa10b93444e07397571cce98b28da51db9056bc728c5b0b1181e2fbd387b4c79ab1a5fefece37167af772ddad14eb4c3982da5a59d0e9eb173ec6315091170027a3ab5ef6aa129cb8585727b9358a28501d713a72f3f1db31714286f9b6408013af06045d75592fc0b7dd47c73ed9c75b11e9d7c69f7cadfc3280a9062c5273c43be1c34f87448864cea7b5c97d6d32f59bd5f25384653bb5c4faa45bea8b89402843e645b6b9269e2bd988ddacb033328ffb060450f7df080053e6969b251e875ecec32cfc592840d69ab69a75e06b379c535d95266b082f4f09c93162b33b0d9f7307a4eaaa52104437fed66f8ee3eabbd45d67b25a8133f496468b52baffdbfad93eef1a9818b5e42ec722788a3d8d3529fc777d2ba570801dfae01ec88302837c1fb9e0355727645ee1046c3f915f6ae82dad4fb6b0356a46518ffc834155c3b4fe6dafa6cc8a5ccf53c73a0849d8d44f7dcf72754e70e1b7dfb447bb4ef49d1a718f6171bbce200950e0ce926106b151a3e871d5ce49731bd6650a9b0ca972da1c5f136d44820ea6383c08f3b384cf2338e789c513f618cc5694a6f0cee104511e1ed7c5f23a1ebfd8a0db8424553240156dbf622831b0c643d1c551b6f3f7a98d29b85c2de05a65fa615eee16495bd90737672115b53e91c5d90028cf3f1a93953a153de53b44084e9ccff6b736693926daefebb2d77aa5ad689b92f31686669df16d1715cc58f7a2cfb72dd1a51e92f825993a74022be7e9eb6054654457094d14928f20215e7b222ac56b51adbec8d8bdb6983979a7e3a21b44b5d1518ca97d0b5195f51ed6a24350c89747e1edea51b448e3e9147054ce927873c90db394d86888e07dff177593d6f79e152302204aeb03be2386af3e24078bd028b1689f5e147c9f452c8ceb02ec59cc9db63a03576ceeafe98239023897da0236630a53c0de7f435a19869792fab36e7b9e635760f09069e6432e700035ac2a02879fff0a1e1bec522047193d94eb5df1efd53eea1144ca78940852f5ec9727904b366ede4f5e2d331fad5fc282ea2c47e923142771c3dd75a87357487def99e5f18e9d9ed623c175d02888c51f82c07a80d54716b3c3c2bdbe2e9f0a9bbaaebeb4d52936876406f5c00e8e4bbd0a5ec05797e6207c5ab6c88f1a688421bd05a114f4d7de2ac241fa0e8bedff47f762ddcbeaa91004f8d31e85095c81054994ad3826e344ba96040810fc0b2ad1de48cfade002c62e5a49a0731ab38344bc1636df16bf607d56855e56d684003c718e4bad9e5a099979fcddeeb1c4a7776cd37a3417cb0e184e29ef9bc0e87475ba663be09e00ab562eb7c0f7165f969a9b42414198ccf1bff2a2c8d689a414ece7662927665689e94db961ebaec5615cbc1a7895c6851ac961432ff1118d4607d32ef9dc732d51333be4b4d0e30ddea784eca8be47e741be9c19631dc470a52ef4dc13a4f3633fd434d787c170977b417df598e1d0dde506bb71d6f0bc17ec70e3b03cdc1965cb36993f633b0472e50d0923ac6c66fdf1d3e6459cc121f0f5f94d09e9dbcf5d690e23233838a0bacb7c638d1b2650a4308cd171b6855126d1da672a6ed85a8d78c286fb56f4ab3d21497528045c63262c8a42af2f9802c53b7bb8be28e78fe0b5ce45fbb7a1af1a3b28a8d94b7890e3c882e39bc98e9f0ad76025bf0dd2f00298e7141a226b3d7cee414f604d1e0ba54d11d5fe58bccea6ad77ad2e8c1caacf32459014b7b91001b1efa8ad172a523fb8e365b577121bf9fd88a2c60c21e821d7b6acb47a5a995e40caced5c223b8fe6de5e18e9d2e5893aefebb7aae7ff1a146260e2f110e939528213a0025a38ec79aabc861b25ebc509a4674c132aaacb7e0146f14efd11cfcaf4caa4f775a716ce325e0a435a4d349d720bcf137450afc45046fc1a1f83a9d329777a7084e4aadae7122ce97005930528eb3c7f7f1129b372887a371155a3ba201a25cbf1dcb64e7cdee092c3141fb5550fe3d0dd82e870e578b2b46500818113b8f6569773c677385b69a42b77dcba7acffd95fd4452e23aaa1d37e1da2151ea658d40a3596b27ac9f8129dc6cf0643772624b59f4f461230df471ca26087c3942d5c6687df6082835935a3f87cb762b0c3b1d0dda4a6533965bef1b7b8292e254c014d090fed857c44c1839c694c0a64e3fad90a11f534722b6ee1574f2e149d55d744de4887024e08511431c062750e16c74ab9f3242f2db3ffb12a8d6107faa229d6f6373b07f36d3932b3bdb04c19dd64eadd7f93c3c564c358a1c81dcf1c9c31e5b06568f97544c17dc15698c5cb38983a9afc42783faa773a52c9d8260690be9e3156aa5bc1509dea3f69587695cd6ff172ba83e6a6d8a7d6bbebbbcda3672731983f89bc5831dc37c3f3c5c56facc697f3cb20bd5dbadbd702e54844ac2f626901fe159db93dfd4773d8fe73562b846c1fc856d1802762840ebc72d7988bde75cbca70d319d32ce0cc0253bb2ad455723ee0c7f4736ce6e6665c5aca32a481c53839bc259167b013d0423395eeb9aaaee3206149a7d550d67fc5fdfe4a8a5c35d2510b664379ab8f72855a2af47abce2a632048eaf89e5cb4a88debc53a595103acce4f1cff18acff07afe1eb5716aa1e40b63134c3a3ae9579fa87f515be093c2d29db6d6b65c93661e00636b592704d093cc6716c2342eb1853d48c85c63ac8a2854462c7b77e7e3bd1eac5bca28ffaa00b5d349f8a547ad875b96a8c2b2910c9301309a3f9138a5693111f55b3c009ca947c39dfc82d98eb1caa4a9cbe885f786fa86e55be062222f8ba90a974073326b31212aece0a34a60").unwrap() .try_into().unwrap(); - // Decode and re-encode the sk, make sure you get the same thing + // Decode and re-encode the sk, ensure the output matches let decoded_sk = MLDSA87PrivateKey::from_bytes(&expected_sk_bytes).unwrap(); let sk_bytes = decoded_sk.encode(); assert_eq!(sk_bytes.len(), expected_sk_bytes.len()); assert_eq!(sk_bytes, expected_sk_bytes.as_slice()); - // Decode and re-encode the pk, make sure you get the same thing + // Decode and re-encode the pk, ensure the output matches let expected_pk = MLDSA87PublicKey::from_bytes(&expected_pk_bytes).unwrap(); let pk_bytes = expected_pk.encode(); assert_eq!(pk_bytes.len(), expected_pk_bytes.len()); @@ -256,8 +256,8 @@ mod mldsa_tests { #[test] fn deterministic_sign() { - // at least one test each of signing with a deterministic signing nonce - // We support setting the signing nonce (rnd) via two interfaces: external mu, and streaming API. + // At least one test each of signing with a deterministic signing nonce + // Setting the signing nonce is supported (rnd) via two interfaces: external mu, and streaming API. // ML-DSA-44 @@ -371,7 +371,7 @@ mod mldsa_tests { ) .unwrap(); - // test the streaming API on the same value + // Test the streaming API on the same value let mut s = MLDSA87::sign_init(&sk, Some(&hex::decode(MLDSA87_KAT1.ctx).unwrap())).unwrap(); s.set_signer_rnd(rnd); s.sign_update(&hex::decode(MLDSA87_KAT1.message).unwrap()); @@ -486,7 +486,7 @@ mod mldsa_tests { #[test] fn test_sign_mu_deterministic_from_seed() { - // I don't have a KAT, so I'll test dynamically against the regular implementation + // No KAT available at the moment, so it is tested dynamically against the regular implementation // ML-DSA-44 @@ -553,7 +553,7 @@ mod mldsa_tests { ) .unwrap(); - // test invalid seed types + // Test invalid seed types let wrong_len_seed = KeyMaterial256::from_bytes_as_type( &hex::decode("000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d").unwrap(), KeyType::Seed, @@ -579,7 +579,7 @@ mod mldsa_tests { _ => panic!("Expected KeyGenError"), }; - // success case: seed SecurityStrength is exactly right + // Success case: seed SecurityStrength is exactly right let mut low_security_seed = KeyMaterial256::from_bytes_as_type( &hex::decode("000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f") .unwrap(), @@ -618,7 +618,7 @@ mod mldsa_tests { let sig = s.sign_final().unwrap(); assert_eq!(&sig, &expected_sig); - // while we're at it, test the streaming verifier cause I'm not sure where else this is being tested. + // Test also the streaming verifier let mut v = MLDSA44::verify_init(&pk, Some(&hex::decode(MLDSA44_KAT1.ctx).unwrap())).unwrap(); @@ -631,7 +631,7 @@ mod mldsa_tests { let msg = b"The quick brown fox jumped over the lazy dog"; // ctx too long - // this is common to all parameter sets, so I'll just test MLDSA44 + // this is common to all parameter sets, so only MLDSA44 is tested let (_pk, sk) = MLDSA44::keygen().unwrap(); // ctx with len 255 works @@ -745,7 +745,7 @@ mod mldsa_tests { fn sig_val_z_too_big() { // This signature value was manually generated in the debugger to have a z containing several // coefficients of (1<<17) -1, which are just below GAMMA1, and in aggregate should cause check_norm() to fail. - // ... a condition that generally does not show up with well-behaved inputs. + // This condition generally does not manifest with well-behaved inputs. let busted_sig = hex::decode("cb8f8b46d73e7c273500555acbe0e7cf1da54d950675248e11bff5940b45f52f010004001000000682ff854d13b45dd7a148636d330453ecaecc627c0a1b417aa5c52cdbb614aeaa3e73f19a59686151872cab71fe793a217ecad4c7a0504a6bbd2585dcb4fc756ebf43242d933c5d90f940d96c74a2a0817e14b5d5563c1f42cd7ac23d18276a301acd91ce752843982ebea23b1c0a7319adcb6ded96d6db10b80067d20b2cce31ea5ff1dbd2bf0b9d29e2db5f9bd547e9f75d00e7db6f2071b5d4f3cb9137df6924ba5e2e203b000802ee2bd34f933e352a54325804ff0b5c43deae326e7e6af0afcff83c2b4ced702a5e2f2fe57a2aad9223a96aa1b54e422aa2ad23a75ce489bf4232b92cb4ddbae6f9f1c0a7d3472e26b7423caf59c919c916e08fc50981c153ea9daa956afdd0a8d980cbb709082c8fcec7f55cef10f1e2d641b667f4aa54be817c26ed446bf58ebbe4b0a98175e469d6231c73d798e761190018a9340d463c09e525d17ab29b50031cc46f625f20ddffbacee4833cf652f6733c5ab1c3c0554fe916652e3a5b88e634213f7fa34ce2c8cfea0e49eed17ada23844c061846f962410b43d1facd60d5e3a667871f7c10d922ba44b7ed153d9b9337d07e14338e8dcae3371c84483a65b5889591c226aa04f4f22965b0762e0cd98396fe6aa0a5aa902f70f93bf9a816dc686ac6cf055d7acb7a994cb1613fc1a8473fde6d39beabcf302eded80ea213f980bd28df6e5837de4b8afbb685b74a1e9cf4f9fc51639b057e73b68d11d72d15e8c81762e3ebf085cf058e132d1edcf170915509a3a58bc2e3184629b0cfe17b452537421532d6bd7d78237b8cb83811d3f823e150144d102f86418ab8cefb2c4c6a7510f1a34e1d2f9cee9209fb8f62c04975086767bfae2644f0e514e0b973ed6547deee0b8cf0a5d21865b4c41bf7acf05ad8f14103dc599aa9d068d777b047850ff42328de052300588bab7264fa885e981b1afce0b48e756db8589625d6294732c44a51aa4a8d4f5c5572bc3c08a3d18c0d7f9d2291af6ee2f3bb552b22acf3466f75886a77701cc0efa1adbc5258dc7e9463db16983e4e7bf2cf66bd3770ea2ca0a33e3c4515b1865f9bd5057c8a810062a7c61f9c1e73b0c53fb1a29b306ae719ad7602c49dde36909e152e106d248be1df684c4a56355a69277255e646d5c1bc6fea212e19da26caaddae09c63587ca4bc496de9369e22eb270a96706d3d4b3caabb271bea66027739fad15bd5d91108944b7533daa77f5eb3cf87eaaba8a8eb7e1aa234eccb2fda84ba55027bb96b024a8a81afbe46b3334376f5f9f8efbce6530ada96980b2c9938858ec99fce960eae548e84bfccf400209fdca72fdcc8a72cc497ef4f247056f7cd008bf2299a6e3d5bcccc7055abbf7a8585bdde15028815cfee54e7a74a88c09af785d1b8885c090e59ea312f2aabffaceb8f77eb62e122ca5b74cf906ff7bb393f7d5801332aa0edc72a9a264b413405edf3b80c6e2dd410f0ab0ddca75d04cccdef4c76df60482e83e45a4306f7b67028d4ac0a99d75dacd0d78c4056814f1e3624219dc46832ebf66d6520316a7a552d12752e991a0217e119662d21ead999271c74a6cb168af99dd0a63412882be744f409d08c0ace64b60a647326f88262bd6a1c19187ad9ff420e56fd0242c4c8e3dc097e1313047d618b29336c36571b0bf2840da2d8d9d021c271532f475c07b5dfd74754a3b4a2b22035b3575d4a4836c784f24b228be567f30994b71be1c355644edb72dea9458848f91da920c3c45c187121431684e261af26671387a109b1938e14257a646902be53d5bd9a26696c7ef48e194135c6bdf97df5a98e87a77df89150b2906b50d332cd79389710fcd0c57c982ed51d510ca44de02d4acc9ad0fd9e4491fc9727ea26691b2f742c96bf0b0c88c1d844102e2d90d744fa91cd6d01a2123b8e6e0f2f40ab68149d7fc00fcce3ef590152722b4d47bf8f291491e8ebf6efdde2d1992ce9b754aea6ef9ee019afeddf619aed86757c50b5b85bf8c44eaab670f4f018bcde75f7dcead0a1ff234d6e717f057a9a372ba998915c4cac6b8c4d568b414c8c0be19afbb8a40092c686eeb57a899b3f67d1e1d6ae326f1f5fc5e8e3b203e041807462c7f171f49841835c20f32fadca7b3ee944c814ef61796083b88994948967f5422c51df776d0957011d2ed0569d8e7b28ebe02b4e38f52d00330b79850d6fcba7c598e40ab93e12ab3b7c2f46f88b56b5d83f828871d94ff0e2d60b549c7e7b2cfc5f1a960fd7afcc46ce5f058f1e05a872c38495cb3490a365135a26515cf2cc453f9e71a0c3d233fc6e6d0dbe152f7f34add23fa5b101e02fd83c5fdfe1a66754cc7a4748abbfbb96c7762153c33bad113e3720861fe1accea673c334c915036794a8341d47bae11fc9a2a9effdb54c904b9e9fc9a1cef369ad4d04c51b97ef8820c8bc3343c33ab85c6f040a46210b7a72c76b639702620731fa002460fb781b5a663ec200ab82626d4b085e3348ba5d42f83f743fbaf59f009b960d40c3978bedc8b2f6701619a6f79a82b4c27604d28e6b1413742aa35e981e9b6eb3ecac3c013506209bda49a7d5f4667e45d57b311f476617706b415baf0964fe531fd38cd200450007f3aad73141732d6c0ffde482e36cbc47aebec1ec036069020df695fa9a43ad5711ecb9b54b358991640a8090093ecd7f9448ec08c8250d27a45595d2e7a1c012e07c632083af08995fc211eb55d04f9bc787d4d604de5b797b64a4918e9d93f3cd84690f99b93194da46bad979e5cbf5146cedcbb43e931da298b708fde037a4057a7178d66bfa3ad2fedd06c646cfc128496e8d9f3d439421cf0a6199b5d6614a7a06a1369384cccae710f3dcdf8f1d52cf7bd5b7fed962c1646e372dd2b00ee98d286c61f10ffe4d7ec8300202ceae605c64ff085a9adc1031798c5ecd9a747dee38fc64f43883db58572d57fd56c94b27d1a97b15f93daa73a5931a72dcf59d6ba2a5bfceeb8814963c0e953491be65175c7248dd7a3c1486d9713b636cdde8b36cab2dc6773a671d637a739feaed54d8c43742cab579896914770d5f14141eca519adc3556dfc08ca9720045cc0679999e864b19d9cca01ca6fc1f0536ccd7ed3d4a7e2d911346d66d648ea0d2a8d9da8097fa0ac9f239d5a566dd88164ae8c2a18612c47f72dfa73f8166550e516a7656329fcddc63b258de992e4bc918401e91257e6852e9f1c64eddc0ba3e724f4a6c7e0ef80a26eb38501908191a292a314344626f718ca1b4c9dce9f828687197a5b0bdf9112f345f70798dc5e4121d283b6f728fa3adb0c4f9000000000000000000000000000000000000000000000000000000000000000000001119222e").unwrap(); @@ -791,7 +791,7 @@ mod mldsa_tests { #[test] fn keypair_consistency_check() { - // this is common to all parameter sets, so I'll just test MLDSA44 + // this is common to all parameter sets, so only MLDSA44 is tested let (pk, sk) = MLDSA44::keygen().unwrap(); // success case @@ -919,8 +919,10 @@ struct Kat { } // generated by hand against bc-java -// This is almost tgId=1, tcId=1 from https://raw.githubusercontent.com/bcgit/bc-test-data/refs/heads/main/pqc/crypto/mldsa/ML-DSA-sigGen.txt -// except I added an empty ctx instead of testing sign_internal directly and re-ran the signature value against bc-java +// This is almost tgId=1, tcId=1 from +// https://raw.githubusercontent.com/bcgit/bc-test-data/refs/heads/main/pqc/crypto/mldsa/ML-DSA-sigGen.txt +// with the difference being that an empty ctx is added, instead of testing sign_internal directly, +// and the signature value is run against bc-java const MLDSA44_KAT1: Kat = Kat { _parameter_set: "ML-DSA-44", deterministic: true, @@ -930,9 +932,10 @@ const MLDSA44_KAT1: Kat = Kat { signature: "2bddcee4a9ac1b9d19bc1531365c5613e48b95a530339c52f5fc0b671ee01b6587fe08b290c191f82eace640fae216cca90f40fa93bb309e5ff53afc5a042050bffeb69d4e0041c34fd334a7b576c6ebae68042b315fe78f84300dc011cdb144252a06f35c9a2b0a275d00552f890361d1e7b7439572097d1f3d5833b98b751412deb3c0df23ee3a30782919444bcbcc9ac25c645180c8d1a9fe693086cba4779d6e31b1e5fd1a0ccaeb055c471ef065a273676d78c6e7feebfc607c5578107ac27765345363af57f77431e306d9407ce365708d813e44d3107c108f8c5f2848913a4099f6a0ce2ebf80c7414236d4edc07fee79efe1ab6fe70217bec958ea48221fb6b87cef6f5762b41a9c698fbb45f5994969bee21e40f20c95f9a18389e8b49d6ecccdae9f568313448b5162c981bc9ff320753aede977b15f7ddc68bba076a07deb68ac36826e70d80752fac0356d507b3e283b350a17b015f76c71314f7c2086e44601c4d9c707c99a36e55716fd34384a218242a69876c8dea9f6ec28c40189ee2b8608040a96a813f38240dc85a511b3cebab1ea893270e730aef2e29406fcc1f6826101f1361168ceb3f1632e8ee505143e0f031e744928c1e41eab923ebbfce5e9f159fd160db737b013759382274a1d9409554ab06eb72b5ce2a710d8b08f163df991c956abc823d21b0a6d9d7ae484d6cdb2e04de7a282ef317f488e20d40e4e67cbf05d1528c0ce261e3a65163ad518661989484bc964da21122a6c95ef7036b3273f93833d068d9a2c7933e19ad2fde8378c286ad57f7e22c8aa4153718f356b46b34ca4a986e6e9ef3ddb206693acf3b08c0aa5118c8efbb6a99a0fc0674f6228e2f6f53e229129d4b6c0fd6455e28587f0f169270d044398e2c377ab6f985f7f2235d68192d031f39251f9f0270451beda2d29b654511b73bce0f30174a606a1100f4fc984959704ba0e2df2bdf642ba3b0241ca9889009ae575540c71747246835018ba8faeb473b39e4ede5a6fbe0165a4db2e90739cb051f3e5c1c0ce159539758569ecc8b67a759a2e786d5b4e96834db0dd460ab1c5c7ac8f3cf19596979108dfbdc6c8798e7342026b7571ba64fa86d68e1d5179af5768a7e22db76f3a319193d04eeea626d03d4be7dd83ed6c46e2149a1e2428e60f6566833726bcc93d0b342ff4245665b54167b013b02165d29fb64055112c5d800e853bf28bf149da2284e49dc1c5f478f490ce1002f16431564534244cd9c1e0bd12ea9209efe572afb7effc0153e5b2066b1af0dd05a2d9da13308c90a0244ad95a33a64d07a75d7596e46cdcfd154e18f7677ef16b979a1f431eb59279c65a3a31c5429ceebfcc869cf538722a82e6e765bd6a3042dc825684ad4163e054c113b47276b894580db17c92f0859c240b6812716e408a557c1d7ac8cfc3abf9b43f759bb2a13c903bba936206496fed37696171717cb8995b837e869052c9c25348698617c70e98966d30f56bb5bf41369c0131de4178637c5684a64992b50f9ab9a16b51752e14ea521d5bd42bacdfcf100085629796d22bcaa6a7830f9b5859d75b290a0db031bd51b77fa7911eb17d46dcf16d45e3b1ce8fb7dc19ba969a724e43ccce9487c143302c79fb208b2ab0ea1060af3809d5397c11cc0a79ed39ab06c0ef7bceb8e094ec0e3ea52937442ccc176a31b07a52d7de84c0599dbe736cecfacbb41cac206ec3dc20603606a14a1db3ccad8037a540f3213ee9337b529f447ed2c2e287760973b8175f70c78423ace6179757c519c14a0d7b81af00646afde573a35e909ef9660623cd3651b63b79be508698888e1d3bc87344067dcc099416616c273d5ea0e41a8723d11c5ec49e0c6019f6576a2e87d0237c3f64d49afbaf09fc818829d5d0fcb4336e08503d570dc3c96fa71fd7b2d4f51d0d1acc515c2c4dad23896273b2616f7d848816c5cc17f7bb2ed5f952a708c38eb13f0fdd8f14ec71893d3f19ccc8aa15fed848583093f4d8246f36db300c9cdd5d63b507641e4d2ba9ec284d17f4a05696be00da371bd5bab0ba56a13451278dbdeed02b4093e482b96269707b09a10dba9ca1dffb4e4185f9e392ebabcea1f9d26c037d9c0dcd71f2bc13faf559d195ef0d8be7baa0cf8aa105a8069356aa2287ec1003e0759f27b02772d4855bf0b7e0b3e97b7f77bd3119669695cc706090991e6a542eb7a28547978ba8a05d9103a90789108fa15898d14982bd0d5d098019700b37939f9f0d66e78af49c6318b886ba2a9101c072bb1e7bdbdb7319eb38e3320b4fbb1fcb28e8e7fac0bd493faf3c0547214465d55ca212dae99dc53addc3378b7e7b93acbdc1c9787149ed0214cd9846852ed7c18b23ab0ae8b7afbb725d44997a38422ff17b1dbeb22e2387094e0bc59496786114d0bb398f36fdfb06c70ff0ce47e9c3eb8c32c22062ccd5306026b606a9e9c628a377f0efd71087c95b3c1ffbdec8a91f311fbba4793d3d3fbf350e6a4a491d74ab7fb0cb66afa0d66177853df464f0175cdee4f97a4e620366aa18c2d04ebab82ff31ec07722fd53e0b4926973dd41d10422747261d8772b18ab55e0bbdcb89e224a5fc2679c2b729aaa4e1a78f95cf68af3562b98f0586d02134464f87dd15b843451a9160c5f4704c994a32259ca623c937431cfe55ee97d736916ac3e7ef831a1b6978539ac6c3304de2f43b3b208f73d071d17fd5cae631f617929468fc59d529deab1c0080a85ded180f9b7029376059c5ca3ba2eab9ee556a74373eadb5983f5990146b04bd255f2380450864e33c478cfe42072eeaabf8032f2c22fbf111407bf2cc41f6cc55596cca62a69303089c3ec231f42a8358d8bce315debce8cd4cea4aa478fba3476b5252e2d64da6b70d6ea0c1b4a99abebbd194e62e25442e7ecffc788710ac6dd1da815a0a5ef8dcef34ebcc90c6f29372875d664c2a06f3485dad8bd00d94837f417412399f9f085d8666fcaf38d38620897e2d06c7399eb28c5db0ffa42a233fdc6732c3e525bd49771aad03348aad068a5e729565c10d343a10c5cd6e530994c9a400354de3af3b39d20cf2b54fc0c9bde4b6f520688694d9b53c3628f1744b61271383d852219d8afae7a284d2f0b042f2fb70778b53d30876b5a031904a241e5a1741ff2e88bf8faa60472ba66111f0560ef127ee86d24f2c502fcff7575696f7f450109776f0f5e1bbd77a48952697fb2f8657516cc061de2bcc6aff9b861356b97e576f78abab1d3acb70d14a84b7858c22b2a0ee17e1231a2da3738018205091b263c42597ea6d3d7d9dae3fa030a13162243494c4e7f8fa1a2d8e2ff11153a41474a63646770717487919da9bac9d2d3e3070f23455d686a7dbcc9de00000000000000000000000000000000000f1f343f", }; -// This is almost tgId=1, tcId=1 from https://raw.githubusercontent.com/bcgit/bc-test-data/refs/heads/main/pqc/crypto/mldsa/ML-DSA-sigGen.txt -// except I added an empty ctx instead of testing sign_internal directly -// except I added an empty ctx instead of testing sign_internal directly and re-ran the signature value against bc-java +// This is almost tgId=1, tcId=1 from +// https://raw.githubusercontent.com/bcgit/bc-test-data/refs/heads/main/pqc/crypto/mldsa/ML-DSA-sigGen.txt +// with the difference being that an empty ctx is added, instead of testing sign_internal directly, +// and the signature value is run against bc-java const MLDSA65_KAT1: Kat = Kat { _parameter_set: "ML-DSA-65", deterministic: true, @@ -943,8 +946,10 @@ const MLDSA65_KAT1: Kat = Kat { }; // generated by hand against bc-java -// This is almost tgId=1, tcId=1 from https://raw.githubusercontent.com/bcgit/bc-test-data/refs/heads/main/pqc/crypto/mldsa/ML-DSA-sigGen.txt -// except I added an empty ctx instead of testing sign_internal directly and re-ran the signature value against bc-java +// This is almost tgId=1, tcId=1 from +// https://raw.githubusercontent.com/bcgit/bc-test-data/refs/heads/main/pqc/crypto/mldsa/ML-DSA-sigGen.txt +// with the difference being that an empty ctx is added, instead of testing sign_internal directly, +// and the signature value is run against bc-java const MLDSA87_KAT1: Kat = Kat { _parameter_set: "ML-DSA-87", deterministic: true, diff --git a/crypto/mldsa/tests/wycheproof.rs b/crypto/mldsa/tests/wycheproof.rs index bc7fd34..b2a230e 100644 --- a/crypto/mldsa/tests/wycheproof.rs +++ b/crypto/mldsa/tests/wycheproof.rs @@ -325,7 +325,7 @@ impl MLDSASignNoSeedTestCase { // build mu let mu: [u8; 64] = if self.msg.is_none() { - // we can't compute it, so just take the one provided + // it can't be computed, so just takes the one provided hex::decode(&self.mu).unwrap().as_slice().try_into().unwrap() } else { match MuBuilder::compute_mu( @@ -393,7 +393,7 @@ impl MLDSASignNoSeedTestCase { // build mu let mu: [u8; 64] = if self.msg.is_none() { - // we can't compute it, so just take the one provided + // it can't be computed, so just takes the one provided hex::decode(&self.mu).unwrap().as_slice().try_into().unwrap() } else { match MuBuilder::compute_mu( @@ -461,7 +461,7 @@ impl MLDSASignNoSeedTestCase { // build mu let mu: [u8; 64] = if self.msg.is_none() { - // we can't compute it, so just take the one provided + // it can't be computed, so just takes the one provided hex::decode(&self.mu).unwrap().as_slice().try_into().unwrap() } else { match MuBuilder::compute_mu( @@ -621,7 +621,7 @@ impl MLDSASignSeedTestCase { // build mu let mu: [u8; 64] = if self.msg.is_none() { - // we can't compute it, so just take the one provided + // it can't be computed, so just takes the one provided hex::decode(&self.mu).unwrap().as_slice().try_into().unwrap() } else { match MuBuilder::compute_mu( @@ -712,7 +712,7 @@ impl MLDSASignSeedTestCase { // build mu let mu: [u8; 64] = if self.msg.is_none() { - // we can't compute it, so just take the one provided + // it can't be computed, so just takes the one provided hex::decode(&self.mu).unwrap().as_slice().try_into().unwrap() } else { match MuBuilder::compute_mu( @@ -803,7 +803,7 @@ impl MLDSASignSeedTestCase { // build mu let mu: [u8; 64] = if self.msg.is_none() { - // we can't compute it, so just take the one provided + // it can't be computed, so just takes the one provided hex::decode(&self.mu).unwrap().as_slice().try_into().unwrap() } else { match MuBuilder::compute_mu( diff --git a/crypto/mlkem-lowmemory/src/aux_functions.rs b/crypto/mlkem-lowmemory/src/aux_functions.rs index 2246b74..416a8a1 100644 --- a/crypto/mlkem-lowmemory/src/aux_functions.rs +++ b/crypto/mlkem-lowmemory/src/aux_functions.rs @@ -61,10 +61,11 @@ pub(crate) fn byte_decode(B: &[u8; PACK_L // 3: F[i] = SUM_j=0..d-1{ 𝑏[𝑖 ⋅ 𝑑 + 𝑗] ⋅ 2𝑗 } mod m for j in 0..d { // select the next bit, according to bitcount, then shift it up by j - F[i] |= (((B[(i * d + j) / 8] >> (i * d + j) % 8) & 1) as i16) << j; // there's supposed to be a `mod m` here, but that shouldn't matter; we'll check it below anyway. + // there is supposed to be a `mod m` here, but that shouldn't matter as they are being checked below + F[i] |= (((B[(i * d + j) / 8] >> (i * d + j) % 8) & 1) as i16) << j; } // assert the mod m - // We'll relax these because it's being checked above in MLKEMPublicKey::pk_decode() + // These are relaxed because they are being checked above in MLKEMPublicKey::pk_decode() debug_assert!(F[i] >= 0); // debug_assert!(F[i] <= if d<12 {2< Polynomial { // 3: 𝑗 ← 0 let mut j = 0usize; - // SHAKE is fairly inefficient if you just squeeze 3 bytes at a time, so we'll do a block. + // SHAKE is fairly inefficient if only 3 bytes are squeezed at a time, so a block is done instead. // size doesn't really matter, so long as it's a multiple of 3. // 288 seemed to be the sweet spot from playing with benchmarks - // It's probably around the average rejection rate, and 216 is a multiple of both 3 (required for this alg) + // It's likely around the average rejection rate, and 216 is a multiple of both 3 (required for this alg) // and 8 (efficient for SHAKE). let mut C = [0u8; 216]; xof.squeeze_out(&mut C); diff --git a/crypto/mlkem-lowmemory/src/lib.rs b/crypto/mlkem-lowmemory/src/lib.rs index 137afb7..c61a5d5 100644 --- a/crypto/mlkem-lowmemory/src/lib.rs +++ b/crypto/mlkem-lowmemory/src/lib.rs @@ -12,11 +12,11 @@ //! ML-KEM-768 is 3 and 3 x 3 = 9 polynomials, and ML-KEM-1024 is 4, and 4 x 4 = 16 polynomials. //! //! A straightforward implementation of ML-KEM will start by un-compressing all the key material into -//! memory into the format that you need to perform the computation. +//! memory into the format needed to perform the computation. //! A ready-to-use private key consists of a `Vector` //! while the public key is a `Vector` and a `Matrix`. -//! For ML-KEM-768, you expect to use 6 kb of RAM just for holding expanded key material, and then -//! you expect the `.encaps()` and `.decaps()` operations to require several multiples of that as variables for holding +//! For ML-KEM-768, it is expected to use 6 kb of RAM just for holding expanded key material, and then +//! the `.encaps()` and `.decaps()` operations are expected to require several multiples of that as variables for holding //! intermediate values as the computation proceeds. //! A well-written but not memory-optimized ML-KEM-768 can be expected to consume approximately 40 kb of RAM //! at the widest point of the `.decaps()` operation. @@ -24,9 +24,9 @@ //! This crate strives to do better! //! //! The core observation that makes this implementation possible is that by a careful examination of -//! how the matrix multiplication works, you don't ever need the vectors and matrices to be fully +//! how the matrix multiplication works, the vectors and matrices don't ever need to be fully //! expanded at the same time. -//! In fact, you can work one polynomial at a time. +//! In fact, it is possible to work one polynomial at a time. //! This is because the ML-KEM keygen algorithm starts with a single 64-byte seed and expands that //! into intermediate seeds `rho` (32 byte), and `sigma` (32 byte), from which all //! of the vectors and matrices are derived via hash functions. @@ -78,8 +78,8 @@ //! //! The table below shows peak memory usage of the ML-KEM algorithms and the rough performanc (throughput) impact. //! -//! Measuring peak application memory usage can be a bit tricky, and the numbers you get depend heavily on how you designed your -//! measurement harness. Here, we aim to provide a conservative measurement, meaning that we are aiming for an +//! Measuring peak application memory usage can be a bit tricky, and the numbers that are obtained depend heavily on how the +//! measurement harness is designed. Here, we aim to provide a conservative measurement, meaning that we are aiming for an //! over-estimate so that any deployment within an existing application will use incrementally less additional memory //! than the amount stated here. //! @@ -179,7 +179,7 @@ //! ``` //! //! See [MLKEM] and [MLKEM::decaps_from_seed] for an API that uses a merged -//! keygen-and-decaps function to that allows you to store the private key only as a 64-byte seed. +//! keygen-and-decaps function to that allows to store the private key only as a 64-byte seed. //! //! ## Encapsulating and Decapsulating //! @@ -206,21 +206,23 @@ //! # Security //! All functionality exposed by this crate is considered secure to use. //! In other words, this crate does not contain any "hazmat" except for the obvious points about -//! handling your private keys properly: if you post your private key to github, or you generate -//! production keys from a weak seed, I can't help you, that's on you. -//! It is worth mentioning, however, that if using a [MLKEM::keygen_from_seed], then it is your +//! handling private keys properly: if private key are posted to github, or +//! production keys are generated from a weak seed, this library won't be of any help. +//! The responsibility falls on the user +//! It is worth mentioning, however, that if using a [MLKEM::keygen_from_seed], then it is the user's //! responsibility to ensure that the seed is cryptographically random and unpredictable. -//! And also that [MLKEM::encaps_internal] requires you to provide the randomness, so the ciphertext -//! will only be as strong as the randomness that you provide. +//! And also that [MLKEM::encaps_internal] requires the user to provide the randomness, so the ciphertext +//! will only be as strong as the randomness that the user provides. //! //! A note about cryptographic side-channel attacks: considerable effort has been expended to attempt //! to make this implementation constant-time, which generally means that the core mathematical algorithm //! code that handles secret data uses bitshift-and-xor type constructions instead of if-and-loop //! constructions. That should give this implementation reasonably good resistance to timing and -//! power analysis key extraction attacks, however: A) this is a "best-effort" and not formally verified, -//! and B) the Rust compiler does not guarantee constant-time behaviour no matter how clever your code, -//! so like all Safe Rust code (ie Rust code that does not include inline assembly), we are at the mercy -//! of the Rust compiler's optimizer for whether our bitshift-and-xor code actually remains +//! power analysis key extraction attacks, however: +//! A) this is a "best-effort" and not formally verified, and +//! B) the Rust compiler does not guarantee constant-time behaviour no matter how good the design is code, +//! so like all Safe Rust code (ie Rust code that does not include inline assembly), +//! it is up to the Rust compiler's optimizer to decide whether our bitshift-and-xor code actually remains //! constant-time after compilation. #![no_std] @@ -229,13 +231,13 @@ #![allow(incomplete_features)] // needed because currently generic_const_exprs is experimental #![feature(generic_const_exprs)] #![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. +// These are because variable names need to be matched exactly against FIPS 204, +// for example both 'K' and 'k', or 'A' and 'a' are used and have specific meanings. +// linter needs to be instructed to ignore these cases #![allow(non_snake_case)] #![allow(non_upper_case_globals)] -// so I can use private traits to hide internal stuff that needs to be generic within the -// MLKEM implementation, but I don't want accessed from outside, such as FIPS-internal functions. +// This is so that private traits can be used to hide internal components that needs to be generic within the +// MLKEM implementation without providing access from outside, such as FIPS-internal functions. #![allow(private_bounds)] // imports needed just for docs @@ -271,5 +273,5 @@ pub use mlkem::{MLKEM512_CT_LEN, MLKEM512_PK_LEN, MLKEM512_SK_LEN}; pub use mlkem::{MLKEM768_CT_LEN, MLKEM768_PK_LEN, MLKEM768_SK_LEN}; pub use mlkem::{MLKEM1024_CT_LEN, MLKEM1024_PK_LEN, MLKEM1024_SK_LEN}; -// re-export just so it's visible to unit tests +// re-export just so it is visible to unit tests pub use polynomial::Polynomial; diff --git a/crypto/mlkem-lowmemory/src/low_memory_helpers.rs b/crypto/mlkem-lowmemory/src/low_memory_helpers.rs index 3ad71fa..f7773e5 100644 --- a/crypto/mlkem-lowmemory/src/low_memory_helpers.rs +++ b/crypto/mlkem-lowmemory/src/low_memory_helpers.rs @@ -202,7 +202,7 @@ pub(crate) fn unpack_ciphertext_u_row( ) -> Polynomial { let mut u_i = Polynomial::new(); - // make sure we have received a dv + // make sure to received a dv assert!(du == 10 || du == 11); // figure out where in the ct array we're going to write to diff --git a/crypto/mlkem-lowmemory/src/mlkem.rs b/crypto/mlkem-lowmemory/src/mlkem.rs index 3d75cb3..981d918 100644 --- a/crypto/mlkem-lowmemory/src/mlkem.rs +++ b/crypto/mlkem-lowmemory/src/mlkem.rs @@ -178,8 +178,9 @@ impl Algorithm for MLKEM1024 { } /// The core internal implementation of the ML-KEM algorithm. -/// This needs to be public for the compiler to be able to find it, but you shouldn't ever -/// need to use this directly. Please use the named public types. +/// This needs to be public for the compiler to be able to find it, +/// but is shouldn't ever need to be used directly. +/// Please use the named public types. pub struct MLKEM< const PK_LEN: usize, const SK_LEN: usize, @@ -245,7 +246,7 @@ impl< /// Unlike other interfaces across the library that take an &impl KeyMaterial, this one /// specifically takes a 64-byte [KeyMaterial512] and checks that it has [KeyType::Seed] and /// the appropriate [SecurityStrength] for the requested ML-KEM parameter set. - /// If you happen to have your seed in a larger KeyMaterial, you'll have to copy it using + /// If the seed is located in a larger KeyMaterial, it needs to be copied using /// [KeyMaterial::from_key]. pub(crate) fn keygen_internal(seed: &KeyMaterial<64>) -> Result<(PK, SK), KEMError> { let sk = SK::from_keymaterial(seed)?; @@ -269,7 +270,7 @@ impl< let mut ct = [0u8; CT_LEN]; // 1: 𝑁 ← 0 - // since the number of loops here is static; we can hard-code the N values rather than using a counter + // since the number of loops here is static; the N values can be hard-coded rather than using a counter // 2: 𝐭 ← ByteDecode12(ekPKE[0 ∶ 384𝑘]) // 3: 𝜌 ← ekPKE[384𝑘 ∶ 384𝑘 + 32] @@ -278,8 +279,8 @@ impl< // 19: 𝐮 ← NTT−1(𝐀_hat^⊺ ∘ 𝐲_hat) + 𝐞1 // 22: 𝑐1 ← ByteEncode_𝑑𝑢(Compress_𝑑𝑢(𝐮)) - // Note: you need y_hat twice: once here at line 19, and again at line 21. - // We'll just generate it twice to save the memory of holding on to it. + // Note: y_hat is needed twice: once here at line 19, and again at line 21. + // Here it is generated each time it is needed in order to save memory. for i in 0..k { let mut u_i = compute_A_hat_dot_y_hat::(rho, &r, i); @@ -334,17 +335,21 @@ impl< /// Output: ciphertext 𝑐 ∈ 𝔹32(𝑑𝑢𝑘+𝑑𝑣). /// /// Unlike the more public function exposed by [KEM::encaps], this returns the shared secret as raw bytes - /// instead of wrapped in an appropriately-set [KeyMaterialTrait], so you're on your own for handling it properly. + /// instead of wrapped in an appropriately-set [KeyMaterialTrait]. + /// Proper handling is up to the user's own judgement. /// /// Note: this is an internal function that allows the caller to specify the encapsulation /// randomness (which is the message `m` to be encrypted by the underlying PKE scheme). - /// This function should not be used directly unless you really have a - /// good reason. [KEM::encaps] should be used in 99.9% of cases. - /// The reason this is exposed publicly is: A) for unit testing that requires access - /// to the deterministically reproducible function, and B) for operational environments - /// that wish to provide randomness from their own source instead of the built-in RNG in bc-rust. - /// If you think you will be clever and invent some scheme that uses a deterministic KEM, - /// then you will almost certainly end up with security problems. Please don't do this. + /// This function should not be used directly unless there is a good reason to do so. + /// [KEM::encaps] should be used in 99.9% of cases. + /// The reason this is exposed publicly is: + /// A) for unit testing that requires access to the deterministically reproducible function, and + /// B) for operational environments that wish to provide randomness from their own source instead + /// of the built-in RNG in bc-rust. + /// As a reminder, any deterministic KEM (or any encryption mechanism) fails to satisfy any security + /// notion involving indistinguishability (e.g. IND-CPA, IND-CCA2, etc.). + /// Failing to use this properly will result in catastrophic vulnerabilities. + /// Please don't do it. pub fn encaps_internal(ek: &PK, m: [u8; 32]) -> ([u8; 32], [u8; CT_LEN]) { debug_assert_eq!(CT_LEN, 32 * ((du as usize) * k + (dv as usize))); @@ -381,7 +386,7 @@ impl< // 3: 𝐮′ ← Decompress_𝑑𝑢(ByteDecode_𝑑𝑢(𝑐1)) // 5: 𝐬_hat ← ByteDecode12(dkPKE) - // Unnecessary here because we're gonna re-compute them row-by-row + // Unnecessary here because they are re-computed row-by-row // first half of // 6: 𝑤 ← 𝑣′ − NTT−1(𝐬_hat^T ∘ NTT(𝐮′)) @@ -469,7 +474,7 @@ impl< // Compute the rejection sampling key. // Note to future optimizers: this needs to be computed outside of the if at line 9 below // because if its computation is conditional on the Fujisaki-Okamoto check failing, then - // you'll have a timing difference between success and failure. + // there will be a timing difference between success and failure. let K_bar: [u8; MLKEM_SS_LEN]; K_bar = { @@ -496,8 +501,8 @@ impl< K_out } - /// Alternative initialization of the streaming signer where you have your private key - /// as a seed and you want to delay its expansion as late as possible for memory-usage reasons. + /// Alternative initialization of the streaming signer where there is a private key + /// as a seed and its expansion should be delayed as late as possible to reduce memory-usage. pub fn decaps_from_seed( seed: &KeyMaterial<64>, ct: &[u8], diff --git a/crypto/mlkem-lowmemory/src/mlkem_keys.rs b/crypto/mlkem-lowmemory/src/mlkem_keys.rs index b9e58f3..76d8a4b 100644 --- a/crypto/mlkem-lowmemory/src/mlkem_keys.rs +++ b/crypto/mlkem-lowmemory/src/mlkem_keys.rs @@ -283,7 +283,7 @@ impl< let (rho, sigma) = Self::compute_rho_and_sigma(&seed_d); - // Deviation from the FIPS: I am not going to persist the hash of the public key H(ek) in the + // Deviation from the FIPS: The implementation does not persist the hash of the public key H(ek) in the // in-memory representation because it can be re-computed as needed. Ok(Self { rho, sigma, pk_hash: None, z, seed_d }) } diff --git a/crypto/mlkem-lowmemory/src/polynomial.rs b/crypto/mlkem-lowmemory/src/polynomial.rs index bec4ee4..3326a4e 100644 --- a/crypto/mlkem-lowmemory/src/polynomial.rs +++ b/crypto/mlkem-lowmemory/src/polynomial.rs @@ -53,16 +53,26 @@ impl Polynomial { w } - /// Convert a Polynomial back into a message m + /// Decodes a `Polynomial` into its 32-byte message `m`, implementing the message + /// recovery step of K-PKE.Decrypt `ByteEncode_1(Compress_1(self))`, + /// (FIPS 203, Alg. 15). Each coefficient yields one message bit: `Compress_1` + /// (§4.2.1) sets the bit when the coefficient lies nearer `q/2` than `0`, i.e. in + /// the central interval `[833, 2496]` for `q = 3329`. The decision is computed + /// branchlessly and the bits are packed LSB-first. + /// Coefficients are expected to already be canonical in `[0, q]`: the unsigned + /// interval test is not periodic mod `q`, so the caller reduces beforehand (`poly_reduce()` + /// in `pke_decrypt`) and no reduction is repeated here. pub(crate) fn to_msg(self) -> [u8; 32] { const LOWER: i32 = q as i32 >> 2; // 832 const UPPER: i32 = q as i32 - LOWER; // 2497 let mut msg = [0u8; 32]; - // you would expect to use a full reduce() here, but since this is data coming from - // out matrix math and not from an attacker, we can get away with the lighter cond_sub_q() - // Actually; further testing against the bc-test-data set of KATs shows that everything passes even with nothing + // Using full reduce() might be expected here. + // However, this function is only called by pke_decrypt (see mlkem.rs), which performs a + // reduction on every coefficient of the polynomial immediately prior to the call. + // For completeness, testing against the bc-test-data set of KATs shows that everything passes + // without modular reduction. // self.cond_sub_q(); // for (i, item) in msg.iter_mut().enumerate().take(N/8) { @@ -77,8 +87,11 @@ impl Polynomial { msg } - // not currently used, but I'll leave it here because it's useful for debugging if you want to output values - // that are normalized to [0,q] to compare against intermediate results from other libraries. + + + // Not currently used. It is left here as a reference since it's useful for debugging if it's + // necessary to output values that are normalized to [0,q] to compare against intermediate results + // from other libraries. // pub(crate) fn conditional_add_q(&mut self) { // for x in self.0.iter_mut() { // *x = conditional_add_q(*x); @@ -143,17 +156,20 @@ impl Polynomial { /// ByteEncode_𝑑𝑣( Compress_𝑑𝑣(𝑣) ) /// which packs a single polynomial according to the packing coefficient dv pub(crate) fn compress_poly(&self, out: &mut [u8]) { - // make sure we have received a dv + // make sure to received a dv debug_assert!(dv == 4 || dv == 5); - // make sure we were given the right size output buffer + // make sure the right size output buffer is given // each of the N i16's will take dv bits debug_assert_eq!(out.len(), N * (dv as usize) / 8); let mut t = [0u8; 8]; let mut idx = 0; - // bc-java has a cond_sub_q() here, but unit tests show that we don't need it. + // bc-java has a cond_sub_q() here, however, it is not needed + // The reason for this is because a modular reduction is performed immediately + // prior to calling pack_ciphertext in mlkem.rs + // This can be corroborated by running the corresponding unit tests // let mut s = self.clone(); // s.cond_sub_q(); @@ -246,8 +262,9 @@ impl Polynomial { v } - // not currently used, but I'll leave it here because it's useful for debugging if you want to output values - // that are normalized to [0,q] to compare against intermediate results from other libraries. + // Not currently used. It is left here as a reference since it's useful for debugging if it's + // necessary to output values that are normalized to [0,q] to compare against intermediate results + // from other libraries. // pub(crate) fn cond_sub_q(&mut self) { // for i in 0..N { // self[i] = cond_sub_q(self[i]); @@ -338,8 +355,9 @@ impl Display for Polynomial { } } -// Not currently used, but I'll leave it here because it's useful for debugging if you want to output values -// that are normalized to [0,q] to compare against intermediate results from other libraries. +// Not currently used. It is left here as a reference since it's useful for debugging if it's +// necessary to output values that are normalized to [0,q] to compare against intermediate results +// from other libraries. // /// if a is in \[-q..0], then it shifts it up by q to be in \[0..q] // pub(crate) fn conditional_add_q(a: i16) -> i16 { // a + ((a >> 15) & q) diff --git a/crypto/mlkem-lowmemory/tests/mlkem_tests.rs b/crypto/mlkem-lowmemory/tests/mlkem_tests.rs index ff8cc5c..7a062ec 100644 --- a/crypto/mlkem-lowmemory/tests/mlkem_tests.rs +++ b/crypto/mlkem-lowmemory/tests/mlkem_tests.rs @@ -52,7 +52,7 @@ mod mlkem_tests { } /// This runs the full bitflipping tests and takes about 1.5 mins.. - /// I'm leaving this commented out, but feel free to un-comment it and run it. + /// This is left commented out for reasons of efficiency, but a user may run it if they so desire // #[test] // fn test_framework_kem_extensive() { // use bouncycastle_core_test_framework::kem::{TestFrameworkKEM}; @@ -84,13 +84,13 @@ mod mlkem_tests { let expected_pk_bytes: [u8; MLKEM512_PK_LEN] = hex::decode("3995815e597d104355cf29aa5333c93251869d5bcdbe487124f602b8b6a66c16c4761648ad765cf5d8006b515e905a7f0ac076b0c62efa328153e7ca5701699f1305f1e6bc6f90b0e49b693512b6ce992a8b8016ddfc1a662c7e3f9619cbd869dd771af30896ccd5918ac6cb77466c5e779996d67ff9aabc97503f2c7b7e2d000d86450fb1807ca4cabda465825a31c789a1b7a491ab3872765d320d0b71920fa213c94093416b83b8124e69f65e62cb5000dcc37aa9a0fff73970c4772f357d24189ca6f5305568c0e2376a3762a68c605e563c5d209572e0fc7532ca294729535567b5fc413c5e8792d2464536cc808f98add74664f141566f9016a90a541829a98a0464ce41a8bb44c2d4fa3c2c209460728ef14a1a7c4c9b98d12203b4cc3529160a9ab2d7838f7ff6b53ae05aa31a7d646b7afa6c45932526a3c3755619be994c211c2a31c05b3447836cb2150be1829dae6b04c5535cff546e392ba797411720f924f490a5ac5495f21356d550b782a64c1688b6b655bcc7842197a434c2f6563b5b7f09a78bcc488232783561d16f4cbab6755400050781570c66604b817ad1252294736e8b01861a4b5a74519b8b6fe51489a5072392e587626c713776575d33806a1c8e2732af97c2680f51666331c4eb8bbc0431c4f96832daf1b3c45528fba153f6c78b1c198702947ccd337727a46fb53ba11de5cb4191346859516cb6ad72400f3cf209b236aef35a580ac87eb3e30fafd66973ca8a7dd2675af41f7a17b61433cd1af80f7708869f665488497980b1ac10a0cdcb636a00ed8681b35e429124ca80350725b85f83a5eac3a4a3cc1600903e65293560b9b336e5af0d529dac1a048119302cb7a9bcc110b94851bf02117f199dc485a852b7473f09b831a6831d5b54c0b790d225cf6bb92d9462a26cdb33dda5123c7aaf0e26a0b83655eea28bf3a8074725018fd6bae4b601cf61baab71a7a3d35197a343e74b4a272c125d540896426d85b7958d3b38a6ba987ec37225c7b44cdb12dde4539b4ab082363683f04bf7a09cc5c41dfe830a1b162e0b324334362f084a14467723344badd000f8d8c537c48f998f05307cebd1ede0b81c3bc59a065a1b6d63b26c").unwrap() .try_into().unwrap(); - // Decode and re-encode the sk, make sure you get the same thing + // Decode and re-encode the sk, make sure the output is the same let expected_sk = MLKEM512PrivateKey::from_bytes(&sk_seed_bytes).unwrap(); let sk_bytes = expected_sk.encode(); assert_eq!(sk_bytes.len(), sk_seed_bytes.len()); assert_eq!(sk_bytes, sk_seed_bytes.as_slice()); - // Decode and re-encode the pk, make sure you get the same thing + // Decode and re-encode the pk, make sure the output is the same let decoded_pk = MLKEM512PublicKey::from_bytes(&expected_pk_bytes).unwrap(); let pk_bytes = decoded_pk.encode(); assert_eq!(pk_bytes.len(), expected_pk_bytes.len()); @@ -524,7 +524,7 @@ mod mlkem_tests { #[test] fn keypair_consistency_check() { - // this is common to all parameter sets, so I'll just test MLKEM512 + // this is common to all parameter sets, so only MLKEM512 is tested let (pk, sk) = MLKEM512::keygen().unwrap(); // success case @@ -563,7 +563,7 @@ mod mlkem_tests { // // // generated by hand against bc-java // // This is almost tgId=1, tcId=1 from https://raw.githubusercontent.com/bcgit/bc-test-data/refs/heads/main/pqc/crypto/mldsa/ML-DSA-sigGen.txt -// // except I added an empty ctx instead of testing sign_internal directly and re-ran the signature value against bc-java +// // except that an empty ctx is added instead of testing sign_internal directly and re-ran the signature value against bc-java // const MLDSA44_KAT1: Kat = Kat { // _parameter_set: "ML-DSA-44", // deterministic: true, @@ -574,8 +574,7 @@ mod mlkem_tests { // }; // // // This is almost tgId=1, tcId=1 from https://raw.githubusercontent.com/bcgit/bc-test-data/refs/heads/main/pqc/crypto/mldsa/ML-DSA-sigGen.txt -// // except I added an empty ctx instead of testing sign_internal directly -// // except I added an empty ctx instead of testing sign_internal directly and re-ran the signature value against bc-java +// // except that an empty ctx is added instead of testing sign_internal directly and re-ran the signature value against bc-java // const MLDSA65_KAT1: Kat = Kat { // _parameter_set: "ML-DSA-65", // deterministic: true, diff --git a/crypto/mlkem-lowmemory/tests/wycheproof.rs b/crypto/mlkem-lowmemory/tests/wycheproof.rs index b331a3f..25295cf 100644 --- a/crypto/mlkem-lowmemory/tests/wycheproof.rs +++ b/crypto/mlkem-lowmemory/tests/wycheproof.rs @@ -310,9 +310,9 @@ impl MLKEMEncapsTestCase { /* Perform the deterministic encaps and compare results */ - // there's some weird stuff in the wycheproof tests that give a 64 byte m, even though - // it's only allowed to be 32 bytes. - // I guess we treat that as an error that we're meant to catch? + // For wycheproof tests that give a 64 byte `m` + // Parse `m` into the [u8; 32] the API demands; a length mismatch is a bad/unsupported + // test vector, not something encaps_internal validates. let m: [u8; 32] = match hex::decode(&self.m).unwrap().try_into() { Ok(m) => m, Err(e) => { diff --git a/crypto/mlkem/benches/mlkem_benches.rs b/crypto/mlkem/benches/mlkem_benches.rs index 5330f73..7d0e3dd 100644 --- a/crypto/mlkem/benches/mlkem_benches.rs +++ b/crypto/mlkem/benches/mlkem_benches.rs @@ -12,8 +12,8 @@ use std::hint::black_box; fn bench_mlkem_keygen(c: &mut Criterion) { let mut group = c.benchmark_group("KeyGen"); - // set up the seeds outside of the timing loop - // Doing different seeds so that the CPU doesn't cache them or do too much branch prediction + // Set up the seeds outside of the timing loop + // Doing different seeds so that the CPU does not cache them or do too much branch prediction let mut seeds = Vec::::new(); for dummy_seed in DUMMY_SEED_1024.chunks(64) { seeds.extend(KeyMaterial512::from_bytes_as_type(dummy_seed, KeyType::Seed)); @@ -51,8 +51,8 @@ fn bench_mlkem_keygen(c: &mut Criterion) { fn bench_mlkem_keygen_and_expand(c: &mut Criterion) { let mut group = c.benchmark_group("KeyGen_and_expand"); - // set up the seeds outside of the timing loop - // Doing different seeds so that the CPU doesn't cache them or do too much branch prediction + // Set up the seeds outside of the timing loop + // Doing different seeds so that the CPU does not cache them or do too much branch prediction let mut seeds = Vec::::new(); for dummy_seed in DUMMY_SEED_1024.chunks(64) { seeds.extend(KeyMaterial512::from_bytes_as_type(dummy_seed, KeyType::Seed)); @@ -93,8 +93,8 @@ fn bench_mlkem_keygen_and_expand(c: &mut Criterion) { fn bench_mlkem_encaps(c: &mut Criterion) { let mut group = c.benchmark_group("Encaps"); - // set up the seeds outside of the timing loop - // Doing different seeds so that the CPU doesn't cache them or do too much branch prediction + // Set up the seeds outside of the timing loop + // Doing different seeds so that the CPU does not cache them or do too much branch prediction let seed = KeyMaterial512::from_bytes_as_type( &hex::decode( "000102030405060708090a0b0c0d0e0f @@ -107,7 +107,7 @@ fn bench_mlkem_encaps(c: &mut Criterion) { ) .unwrap(); - // create a vector of signing nonces so that we're not measuring the time of the RNG + // Create a vector of signing nonces so that we're not measuring the time of the RNG const NUM_ELEMS: usize = 256; let mut nonces = [[0u8; 32]; NUM_ELEMS]; for i in 0..256 { @@ -159,8 +159,8 @@ fn bench_mlkem_encaps(c: &mut Criterion) { fn bench_mlkem_encaps_for_expanded(c: &mut Criterion) { let mut group = c.benchmark_group("Encaps_for_expanded_key"); - // set up the seeds outside of the timing loop - // Doing different seeds so that the CPU doesn't cache them or do too much branch prediction + // Set up the seeds outside of the timing loop + // Doing different seeds so that the CPU does not cache them or do too much branch prediction let seed = KeyMaterial512::from_bytes_as_type( &hex::decode( "000102030405060708090a0b0c0d0e0f @@ -173,7 +173,7 @@ fn bench_mlkem_encaps_for_expanded(c: &mut Criterion) { ) .unwrap(); - // create a vector of signing nonces so that we're not measuring the time of the RNG + // Create a vector of signing nonces so that we're not measuring the time of the RNG const NUM_ELEMS: usize = 256; let mut nonces = [[0u8; 32]; NUM_ELEMS]; for i in 0..256 { @@ -228,8 +228,8 @@ fn bench_mlkem_encaps_for_expanded(c: &mut Criterion) { fn bench_mlkem_decaps(c: &mut Criterion) { let mut group = c.benchmark_group("Decaps"); - // set up the seeds outside of the timing loop - // Doing different seeds so that the CPU doesn't cache them or do too much branch prediction + // Set up the seeds outside of the timing loop + // Doing different seeds so that the CPU does not cache them or do too much branch prediction let seed = KeyMaterial512::from_bytes_as_type( &hex::decode( "000102030405060708090a0b0c0d0e0f @@ -247,10 +247,10 @@ fn bench_mlkem_decaps(c: &mut Criterion) { /*** ML-KEM-512 ***/ let (pk, sk) = MLKEM512::keygen_from_seed(&seed).unwrap(); - // create a bunch of ciphertexts to decaps + // Create a bunch of ciphertexts to decaps let mut cts = [[0u8; MLKEM512_CT_LEN]; NUM_ELEMS]; for i in 0..NUM_ELEMS { - // create each ct with a unique nonce + // Create each ct with a unique nonce // encaps_internal() returns (ss, ct) ... we only want ct, hence the ".1" cts[i].copy_from_slice(&MLKEM512::encaps_internal(&pk, None, [i as u8; MLKEM_RND_LEN]).1); } @@ -268,10 +268,10 @@ fn bench_mlkem_decaps(c: &mut Criterion) { /*** ML-KEM-768 ***/ let (pk, sk) = MLKEM768::keygen_from_seed(&seed).unwrap(); - // create a bunch of ciphertexts to decaps + // Create a bunch of ciphertexts to decaps let mut cts = [[0u8; MLKEM768_CT_LEN]; NUM_ELEMS]; for i in 0..NUM_ELEMS { - // create each ct with a unique nonce + // Create each ct with a unique nonce // encaps_internal() returns (ss, ct) ... we only want ct, hence the ".1" cts[i].copy_from_slice(&MLKEM768::encaps_internal(&pk, None, [i as u8; MLKEM_RND_LEN]).1); } @@ -289,10 +289,10 @@ fn bench_mlkem_decaps(c: &mut Criterion) { /*** ML-KEM-1024 ***/ let (pk, sk) = MLKEM1024::keygen_from_seed(&seed).unwrap(); - // create a bunch of ciphertexts to decaps + // Create a bunch of ciphertexts to decaps let mut cts = [[0u8; MLKEM1024_CT_LEN]; NUM_ELEMS]; for i in 0..NUM_ELEMS { - // create each ct with a unique nonce + // Create each ct with a unique nonce // encaps_internal() returns (ss, ct) ... we only want ct, hence the ".1" cts[i].copy_from_slice(&MLKEM1024::encaps_internal(&pk, None, [i as u8; MLKEM_RND_LEN]).1); } @@ -313,8 +313,8 @@ fn bench_mlkem_decaps(c: &mut Criterion) { fn bench_mlkem_decaps_with_expanded_key(c: &mut Criterion) { let mut group = c.benchmark_group("Decaps_with_expanded_key"); - // set up the seeds outside of the timing loop - // Doing different seeds so that the CPU doesn't cache them or do too much branch prediction + // Set up the seeds outside of the timing loop + // Doing different seeds so that the CPU does not cache them or do too much branch prediction let seed = KeyMaterial512::from_bytes_as_type( &hex::decode( "000102030405060708090a0b0c0d0e0f @@ -333,10 +333,10 @@ fn bench_mlkem_decaps_with_expanded_key(c: &mut Criterion) { let (pk, sk) = MLKEM512::keygen_from_seed(&seed).unwrap(); let sk_expanded = MLKEM512PrivateKeyExpanded::from(&sk); - // create a bunch of ciphertexts to decaps + // Create a bunch of ciphertexts to decaps let mut cts = [[0u8; MLKEM512_CT_LEN]; NUM_ELEMS]; for i in 0..NUM_ELEMS { - // create each ct with a unique nonce + // Create each ct with a unique nonce // encaps_internal() returns (ss, ct) ... we only want ct, hence the ".1" cts[i].copy_from_slice(&MLKEM512::encaps_internal(&pk, None, [i as u8; MLKEM_RND_LEN]).1); } @@ -355,10 +355,10 @@ fn bench_mlkem_decaps_with_expanded_key(c: &mut Criterion) { let (pk, sk) = MLKEM768::keygen_from_seed(&seed).unwrap(); let sk_expanded = MLKEM768PrivateKeyExpanded::from(&sk); - // create a bunch of ciphertexts to decaps + // Create a bunch of ciphertexts to decaps let mut cts = [[0u8; MLKEM768_CT_LEN]; NUM_ELEMS]; for i in 0..NUM_ELEMS { - // create each ct with a unique nonce + // Create each ct with a unique nonce // encaps_internal() returns (ss, ct) ... we only want ct, hence the ".1" cts[i].copy_from_slice(&MLKEM768::encaps_internal(&pk, None, [i as u8; MLKEM_RND_LEN]).1); } @@ -377,10 +377,10 @@ fn bench_mlkem_decaps_with_expanded_key(c: &mut Criterion) { let (pk, sk) = MLKEM1024::keygen_from_seed(&seed).unwrap(); let sk_expanded = MLKEM1024PrivateKeyExpanded::from(&sk); - // create a bunch of ciphertexts to decaps + // Create a bunch of ciphertexts to decaps let mut cts = [[0u8; MLKEM1024_CT_LEN]; NUM_ELEMS]; for i in 0..NUM_ELEMS { - // create each ct with a unique nonce + // Create each ct with a unique nonce // encaps_internal() returns (ss, ct) ... we only want ct, hence the ".1" cts[i].copy_from_slice(&MLKEM1024::encaps_internal(&pk, None, [i as u8; MLKEM_RND_LEN]).1); } @@ -401,8 +401,8 @@ fn bench_mlkem_decaps_with_expanded_key(c: &mut Criterion) { fn bench_mlkem_decaps_from_seed(c: &mut Criterion) { let mut group = c.benchmark_group("decaps_from_seed"); - // set up the seeds outside of the timing loop - // Doing different seeds so that the CPU doesn't cache them or do too much branch prediction + // Set up the seeds outside of the timing loop + // Doing different seeds so that the CPU does not cache them or do too much branch prediction let seed = KeyMaterial512::from_bytes_as_type( &hex::decode( "000102030405060708090a0b0c0d0e0f @@ -420,10 +420,10 @@ fn bench_mlkem_decaps_from_seed(c: &mut Criterion) { /*** ML-KEM-512 ***/ let (pk, _sk) = MLKEM512::keygen_from_seed(&seed).unwrap(); - // create a bunch of ciphertexts to decaps + // Create a bunch of ciphertexts to decaps let mut cts = [[0u8; MLKEM512_CT_LEN]; NUM_ELEMS]; for i in 0..NUM_ELEMS { - // create each ct with a unique nonce + // Create each ct with a unique nonce // encaps_internal() returns (ss, ct) ... we only want ct, hence the ".1" cts[i].copy_from_slice(&MLKEM512::encaps_internal(&pk, None, [i as u8; MLKEM_RND_LEN]).1); } @@ -441,10 +441,10 @@ fn bench_mlkem_decaps_from_seed(c: &mut Criterion) { /*** ML-KEM-768 ***/ let (pk, _sk) = MLKEM768::keygen_from_seed(&seed).unwrap(); - // create a bunch of ciphertexts to decaps + // Create a bunch of ciphertexts to decaps let mut cts = [[0u8; MLKEM768_CT_LEN]; NUM_ELEMS]; for i in 0..NUM_ELEMS { - // create each ct with a unique nonce + // Create each ct with a unique nonce // encaps_internal() returns (ss, ct) ... we only want ct, hence the ".1" cts[i].copy_from_slice(&MLKEM768::encaps_internal(&pk, None, [i as u8; MLKEM_RND_LEN]).1); } @@ -462,10 +462,10 @@ fn bench_mlkem_decaps_from_seed(c: &mut Criterion) { /*** ML-KEM-1024 ***/ let (pk, _sk) = MLKEM1024::keygen_from_seed(&seed).unwrap(); - // create a bunch of ciphertexts to decaps + // Create a bunch of ciphertexts to decaps let mut cts = [[0u8; MLKEM1024_CT_LEN]; NUM_ELEMS]; for i in 0..NUM_ELEMS { - // create each ct with a unique nonce + // Create each ct with a unique nonce // encaps_internal() returns (ss, ct) ... we only want ct, hence the ".1" cts[i].copy_from_slice(&MLKEM1024::encaps_internal(&pk, None, [i as u8; MLKEM_RND_LEN]).1); } diff --git a/crypto/mlkem/src/aux_functions.rs b/crypto/mlkem/src/aux_functions.rs index 0d331ef..afd7044 100644 --- a/crypto/mlkem/src/aux_functions.rs +++ b/crypto/mlkem/src/aux_functions.rs @@ -97,9 +97,9 @@ pub(crate) fn sample_ntt(rho: &[u8; 32], nonce: &[u8; 2]) -> Polynomial { // 3: 𝑗 ← 0 let mut j = 0usize; - // SHAKE is fairly inefficient if you just squeeze 3 bytes at a time, so we'll do a block. - // size doesn't really matter, so long as it's a multiple of 3. - // 288 seemed to be the sweet spot from playing with benchmarks + // SHAKE is fairly inefficient if you just squeeze 3 bytes at a time, therefore a block is squeezed here. + // Size is not an important factor, so long as it's a multiple of 3. + // 288 seemed to be the sweet spot according to the benchmarks // It's probably around the average rejection rate, and 216 is a multiple of both 3 (required for this alg) // and 8 (efficient for SHAKE). let mut C = [0u8; 216]; @@ -297,8 +297,10 @@ pub(crate) fn barrett_reduce(a: i16) -> i16 { a - (((t as i32) * q as i32) as i16) } -// not currently used, but I'll leave it here because it's useful for debugging if you want to output values -// that are normalized to [0,q] to compare against intermediate results from other libraries. + +// Not currently used. It is left here as a reference since it's useful for debugging if it's +// necessary to output values that are normalized to [0,q] to compare against intermediate results +// from other libraries. // pub(super) fn cond_sub_q(a: i16) -> i16 { // let tmp = a - q; // tmp + ((tmp >> 15) & q) diff --git a/crypto/mlkem/src/lib.rs b/crypto/mlkem/src/lib.rs index 4de7d42..aa84425 100644 --- a/crypto/mlkem/src/lib.rs +++ b/crypto/mlkem/src/lib.rs @@ -115,13 +115,13 @@ //! | ML-KEM-1024_expanded | 1568 | 10272 | 3168 | 12418 | //! //! All values are in bytes. The "in memory" sizes are measured by rust's `std::mem::size_of`. -//! Values in parentheses are the usual sizes in our un-optimized implementation in the \[bouncycastle_mldsa] crate. +//! Values in parentheses are the usual sizes in the un-optimized implementation in the \[bouncycastle_mldsa] crate. //! //! # Security //! All functionality exposed by this crate is considered secure to use. //! In other words, this crate does not contain any "hazmat" except for the obvious points about //! handling your private keys properly: if you post your private key to github, or you generate -//! production keys from a weak seed, I can't help you, that's on you. +//! production keys from a weak seed, that use is unsupported //! It is worth mentioning, however, that if using a [MLKEM::keygen_from_seed], then it is your //! responsibility to ensure that the seed is cryptographically random and unpredictable. //! And also that [MLKEM::encaps_internal] requires you to provide the randomness, so the ciphertext @@ -133,8 +133,8 @@ //! constructions. That should give this implementation reasonably good resistance to timing and //! power analysis key extraction attacks, however: A) this is a "best-effort" and not formally verified, //! and B) the Rust compiler does not guarantee constant-time behaviour no matter how clever your code, -//! so like all Safe Rust code (ie Rust code that does not include inline assembly), we are at the mercy -//! of the Rust compiler's optimizer for whether our bitshift-and-xor code actually remains +//! so like all Safe Rust code (ie Rust code that does not include inline assembly), the Rust compiler's optimizer +//! determines whether the bitshift-and-xor code actually remains //! constant-time after compilation. #![no_std] @@ -143,13 +143,13 @@ #![allow(incomplete_features)] // needed because currently generic_const_exprs is experimental #![feature(generic_const_exprs)] #![feature(adt_const_params)] -// These are because I'm matching variable names exactly against FIPS 204, for example both 'K' and 'k', +// These are because variable names are matched 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. #![allow(non_snake_case)] #![allow(non_upper_case_globals)] -// so I can use private traits to hide internal stuff that needs to be generic within the -// MLKEM implementation, but I don't want accessed from outside, such as FIPS-internal functions. +// so private traits can hide internal items that need to be generic within the +// MLKEM implementation, but should not be accessed from outside, such as FIPS-internal functions. #![allow(private_bounds)] // imports needed just for docs diff --git a/crypto/mlkem/src/matrix.rs b/crypto/mlkem/src/matrix.rs index 100d6e0..06ffa0a 100644 --- a/crypto/mlkem/src/matrix.rs +++ b/crypto/mlkem/src/matrix.rs @@ -1,4 +1,4 @@ -//! These are somewhat unnecessary wrappers around simple arrays, but they are helpful to me in clearly +//! These are somewhat unnecessary wrappers around simple arrays, but they are helpful for clearly //! keeping the types and sizes obvious. use core::ops::{Index, IndexMut}; @@ -79,7 +79,8 @@ impl Matrix { // Matrix and Vector do not need to impl Secret because the actual data is in the polynomials, which have their own zeroizing drop. // 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. +// but this is left zeroizing for now. +// TODO: Investigate potential optimization related to the above. #[derive(Clone)] pub(crate) struct Vector { @@ -113,7 +114,7 @@ impl Vector { /// Add another vector to this vector pub(crate) fn add_vector_ntt(&mut self, s: &Self) { for i in 0..k { - // perform montgomery addition of each polynomial in the vector + // perform Montgomery addition of each polynomial in the vector self[i].add(&s[i]); } } @@ -126,9 +127,8 @@ impl Vector { let w1 = polynomial::base_mult_montgomery(&self[i], &v[i]); w.add(&w1); } - // in theory, we need this here, but all unit tests pass without it since - // it actually doesn't matter if you go outside the [0, q] range as long as you - // reduce down before encoding out. + // Note: This function DOES NOT perform modular reduction, as the current + // construction of ML-KEM only reduces modulo q when it's necessary. // w.poly_reduce(); w @@ -169,7 +169,12 @@ impl Vector { // each of the N i16's will take dv bits debug_assert_eq!(out.len(), k * (N * (du as usize) / 8)); - // bc-java has a conditional_sub_q() here, but I pass all unit tests without it, so I'm taking it out for performance. + // No conditional_sub_q needed (as done in bc-java): callers must reduce() first, + // so coefficients are in [0, q) (barrett_reduce, floor variant). The Compress mask `& (2^du - 1)` folds + // mod q, so values in [q, 2q) would also be correct. WARNING: the `as u32` cast + // below REQUIRES non-negative coefficients. That is to say DO NOT switch barrett_reduce to a + // signed/centered variant (e.g. pq-crystals' rounded form) without restoring a + // reduction here, or this will silently produce garbage. // let mut s = self.clone(); // s.conditional_sub_q(); diff --git a/crypto/mlkem/src/mlkem.rs b/crypto/mlkem/src/mlkem.rs index bd31e43..2f305a2 100644 --- a/crypto/mlkem/src/mlkem.rs +++ b/crypto/mlkem/src/mlkem.rs @@ -71,7 +71,7 @@ //! KeyType::Seed, //! ).unwrap(); //! -//! // for this demo, we do need to run keygen only to get the public key +//! // for this demo, it is necessary to run keygen only to get the public key //! let (pk, _sk) = MLKEM768::keygen_from_seed(&seed).unwrap(); //! //! // Create the shared secret and ciphertext using the public key @@ -93,14 +93,17 @@ //! Contact us if you need such a thing implemented. //! ## Deterministic encapsulation //! -//! This section pertains to [MLKEM::encaps_internal] which allows you to pass in the encapsulation randomness +//! This section pertains to [MLKEM::encaps_internal] which allows to pass in the encapsulation randomness //! and thus obtain a deterministic encapsulation. //! -//! The only good reasons for doing this are: A) testing if you need reproducible results, or -//! B) if you want to use your own source of randomness, such as a hardware RNG, instead of the library's -//! default RNG. -//! If you think you will invent same clever cryptographic scheme by making clever use of this parameter: -//! don't; you will almost certainly end up with something completely insecure. +//! The only good reasons for doing this are: +//! A) testing, if reproducible results are needed; or +//! B) if the user wants to use their own source of randomness, such as a hardware RNG, instead of the library's +//! default RNG. +//! As a reminder, any deterministic KEM (or any encryption mechanism) fails to satisfy any security +//! notion involving indistinguishability (e.g. IND-CPA, IND-CCA2, etc.). +//! Any custom randomness construction will have serious consequences. +//! Failing to use this properly, as indicated, will result in catastrophic vulnerabilities. //! //! ```rust //! use bouncycastle_mlkem::{MLKEM768, MLKEMTrait}; @@ -388,7 +391,8 @@ impl< // 2: 𝑁 ← 0 // Note: in the definition of PRF_eta on page 18, it's said to be one byte. - // since the number of loops here is static; we can hard-code the N values rather than using a counter + // since the number of loops here is static, it is possible to hard-code the N values + // rather than using a counter // 8: for (𝑖 ← 0; 𝑖 < 𝑘; 𝑖++) // ▷ generate 𝐬 ∈ (ℤ256)^k @@ -436,7 +440,7 @@ impl< // 19: ekPKE ← ByteEncode12(𝐭)‖𝜌 ▷ run ByteEncode12 𝑘 times, then append 𝐀-seed // 20: dkPKE ← ByteEncode12(𝐬)̂ ▷ run ByteEncode12 𝑘 times - // Note: I'm skipping the encoding at this stage and leaving it expanded for future efficiency when it's used. + // Note: The encoding is skipped at this stage and left expanded for future efficiency when it's used. // 21: return (ekPKE, dkPKE) (PK::new(t_hat, rho), s_hat) } @@ -449,7 +453,8 @@ impl< /// Output: ciphertext 𝑐 ∈ 𝔹32(𝑑𝑢𝑘+𝑑𝑣). fn pke_encrypt(ek: &PK, A_hat: &Matrix, m: [u8; 32], r: &[u8; 32]) -> [u8; CT_LEN] { // 1: 𝑁 ← 0 - // since the number of loops here is static; we can hard-code the N values rather than using a counter + // since the number of loops here is static, it is possible to hard-code the N values + // rather than using a counter // 2: 𝐭 ← ByteDecode12(ekPKE[0 ∶ 384𝑘]) // 3: 𝜌 ← ekPKE[384𝑘 ∶ 384𝑘 + 32] @@ -457,7 +462,7 @@ impl< // 4: for (𝑖 ← 0; 𝑖 < 𝑘; 𝑖++) // ▷ re-generate matrix 𝐀 ∈ (ℤ256_𝑞 )𝑘×𝑘 sampled in Alg. 13 - // We're doing an optimization where the user can pre-expand A_hat within the + // An optimization is done where the user can pre-expand A_hat within the // public key object for faster repeated encapsulations against this public key. // 9: for (𝑖 ← 0; 𝑖 < 𝑘; 𝑖++) @@ -518,27 +523,31 @@ impl< /// Output: shared secret key 𝐾 ∈ 𝔹32 . /// Output: ciphertext 𝑐 ∈ 𝔹32(𝑑𝑢𝑘+𝑑𝑣). /// - /// This function also takes an Option for the public matrix A. - /// If you don't know what it is, just provide None. + /// This function also takes an Option for the public matrix `A`. + /// If `A` is not known, `None` may be provided. /// This is to enable performance /// optimizations when the same public key is used for multiple encapsulations and the intermediate /// value called the public matrix A_hat can be re-used for multiple encapsulations. /// A_hat can be obtained from [MLKEMPublicKeyTrait::A_hat]. - /// Alternatively, you can use a [MLKEMPublicKeyExpanded] with [MLKEM::encaps_for_expanded_key]. - /// If you specify None, the function will compute A_hat internally and everything will work fine. + /// Alternatively, a [MLKEMPublicKeyExpanded] with [MLKEM::encaps_for_expanded_key] can be used. + /// If `None` is specified, the function will compute A_hat internally and everything will work fine. /// /// Unlike the more public function exposed by [KEM::encaps], this returns the shared secret as raw bytes - /// instead of wrapped in an appropriately-set [KeyMaterialTrait], so you're on your own for handling it properly. + /// instead of wrapped in an appropriately-set [KeyMaterialTrait]. + /// Proper handling is up to the user's own judgement. /// /// Note: this is an internal function that allows the caller to specify the encapsulation /// randomness (which is the message `m` to be encrypted by the underlying PKE scheme). - /// This function should not be used directly unless you really have a - /// good reason. [KEM::encaps] should be used in 99.9% of cases. - /// The reason this is exposed publicly is: A) for unit testing that requires access - /// to the deterministically reproducible function, and B) for operational environments - /// that wish to provide randomness from their own source instead of the built-in RNG in bc-rust. - /// If you think you will be clever and invent some scheme that uses a deterministic KEM, - /// then you will almost certainly end up with security problems. Please don't do this. + /// This function should not be used directly unless there is a good reason to do so. + /// [KEM::encaps] should be used in 99.9% of cases. + /// The reason this is exposed publicly is: + /// A) for unit testing that requires access to the deterministically reproducible function, and + /// B) for operational environments that wish to provide randomness from their own source instead + /// of the built-in RNG in bc-rust. + /// As a reminder, any deterministic KEM (or any encryption mechanism) fails to satisfy any security + /// notion involving indistinguishability (e.g. IND-CPA, IND-CCA2, etc.). + /// Failing to use this properly will result in catastrophic vulnerabilities. + /// Please don't do it. pub fn encaps_internal( ek: &PK, A_hat: Option<&Matrix>, @@ -564,8 +573,8 @@ impl< // 2: 𝑐 ← K-PKE.Encrypt(ek, 𝑚, 𝑟) // ▷ encrypt 𝑚 using K-PKE with randomness 𝑟 // deviation from FIPS: - // To allow for pre-computing A_hat for multiple encapsulations, we will either take - // A_hat passed in, or compute it fresh. + // To allow for pre-computing A_hat for multiple encapsulations, the code either takes + // A_hat passed in, or computes it fresh. let ct = match A_hat { Some(A_hat) => Self::pke_encrypt(ek, A_hat, m, &r), None => Self::pke_encrypt(ek, &ek.A_hat(), m, &r), @@ -621,9 +630,9 @@ impl< A_hat: Option<&Matrix>, c: [u8; CT_LEN], ) -> [u8; MLKEM_SS_LEN] { - // I have tried to keep this as clean as possible for correspondence with the FIPS, - // but I have moved things around so that I can use unnamed scopes to limit how many - // stack variables are alive at the same time. + + // Structured to mirror the FIPS as closely as possible, with unnamed scopes + // used to limit the number of live stack variables at any given time. // 1: dkPKE ← dk[0 ∶ 384𝑘] ▷ extract (from KEM decaps key) the PKE decryption key // 2: ekPKE ← dk[384𝑘 ∶ 768𝑘 + 32] ▷ extract PKE encryption key @@ -651,9 +660,9 @@ impl< // 7: 𝐾_bar ← J(𝑧‖𝑐) // Compute the rejection sampling key. - // Note to future optimizers: this needs to be computed outside of the if at line 9 below - // because if its computation is conditional on the Fujisaki-Okamoto check failing, then - // you'll have a timing difference between success and failure. + // Note to future optimizers: this needs to be computed outside of the conditional at line 9 below. + // This is because if the computation is conditional on the Fujisaki-Okamoto check failing, then + // it will result in a timing difference between success and failure. let K_bar: [u8; MLKEM_SS_LEN]; K_bar = { @@ -670,7 +679,7 @@ impl< // 8: 𝑐′ ← K-PKE.Encrypt(ekPKE, 𝑚′, 𝑟′) // ▷ re-encrypt using the derived randomness 𝑟′ // deviation from FIPS: - // To allow for pre-computing A_hat for multiple encapsulations, we will either take + // To allow for pre-computing A_hat for multiple encapsulations, we will either take // A_hat passed in, or compute it fresh. let c_prime = match A_hat { Some(A_hat) => Self::pke_encrypt(dk.pk(), A_hat, m_prime, &r_prime), @@ -688,7 +697,7 @@ impl< /// Alternative initialization of the streaming signer where you have your private key /// as a seed and you want to delay its expansion as late as possible for memory-usage reasons. - // todo -- should we build a fully-stitched-together decaps-from-seed ... or not? + // TODO: Check whether a fully-stitched-together decaps-from-seed implementation is beneficial pub fn decaps_from_seed( seed: &KeyMaterial<64>, ct: &[u8], diff --git a/crypto/mlkem/src/mlkem_keys.rs b/crypto/mlkem/src/mlkem_keys.rs index 327878c..2acca0b 100644 --- a/crypto/mlkem/src/mlkem_keys.rs +++ b/crypto/mlkem/src/mlkem_keys.rs @@ -119,10 +119,10 @@ impl MLKEMPublicKeyTrait // FIPS 203 says: // "Specifically, ByteDecode12 converts each 12-bit - // segment of its input into an integer modulo 212 = 4096 and then reduces the result + // segment of its input into an integer modulo 2^{12} = 4096 and then reduces the result // modulo 𝑞. This is no longer a one-to-one operation. Indeed, some 12-bit segments could // correspond to an integer greater than 𝑞 − 1 = 3328 but less than 4096." - // Since we are here in the d=12 case, we can and should check that all coeffs are less than q-1 + // Since this concerns to the case d=12, it should be checked that all coeffs are less than q-1 for coeff in t_i.coeffs.iter() { if *coeff < 0 || *coeff >= q { return Err(KEMError::DecodingError("Invalid or corrupted key")); @@ -505,10 +505,10 @@ impl< // FIPS 203 says: // "Specifically, ByteDecode12 converts each 12-bit - // segment of its input into an integer modulo 212 = 4096 and then reduces the result + // segment of its input into an integer modulo 2^{12} = 4096 and then reduces the result // modulo 𝑞. This is no longer a one-to-one operation. Indeed, some 12-bit segments could // correspond to an integer greater than 𝑞 − 1 = 3328 but less than 4096." - // Since we are here in the d=12 case, we can and should check that all coeffs are less than q + // Since this concerns to the case d=12, it should be checked that all coeffs are less than q-1 for coeff in s_hat[i].coeffs.iter() { if *coeff < 0 || *coeff >= q { return Err(KEMError::DecodingError("Invalid or corrupted key")); @@ -526,8 +526,8 @@ impl< pos += 32; // This satisfies the "Decapsulation input check #3) in FIPS 203 section 7.3. - // We're doing it here on key load rather than as part of the decapsulation for performance - // because if you're doing multiple decapsulations, you only need to perform this check once. + // It is done here on key load rather than as part of the decapsulation for performance + // because if multiple decapsulations are being performed, this check needs to be done only once. if h_pk != ek.compute_hash() { return Err(KEMError::ConsistencyCheckFailed( "Corrupted private key: computed hash of ek != h_ek stored in private key", diff --git a/crypto/mlkem/src/polynomial.rs b/crypto/mlkem/src/polynomial.rs index 951cc13..a6babd1 100644 --- a/crypto/mlkem/src/polynomial.rs +++ b/crypto/mlkem/src/polynomial.rs @@ -1,4 +1,4 @@ -//! Represents a polynomial over the ML-DSA ring. +//! Represents a polynomial over the ML-KEM ring. use core::fmt; use core::fmt::{Debug, Display, Formatter}; @@ -11,9 +11,10 @@ use crate::mlkem::{N, q}; use bouncycastle_core::traits::Secret; /// A polynomial over the ML-KEM 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. +/// Dev note: The following structure does not necessarily need to be declared as public. +/// There is no real scenario where this function is called directly. However, in order to test the Debug and Display traits, +/// it is necessary to use STD, so those can't be tested from inline tests in this file and the real unit tests are in a different crate. +/// That's the reason why pub is used. #[derive(Clone)] pub struct Polynomial { pub(crate) coeffs: [i16; N], @@ -40,7 +41,14 @@ impl Polynomial { Self { coeffs: [0i16; N] } } - /// Create a Polynomial from the message m + /// Encodes a 32-byte message `m` into a `Polynomial`, implementing the message + /// encoding step of K-PKE.Encrypt `Decompress_1(ByteDecode_1(m))`, + /// (FIPS 203, Alg. 14). Each message bit becomes one coefficient: `Decompress_1` + /// (§4.2.1) maps bit `1` to `⌈q/2⌉ = (q + 1) / 2 = 1665` (for `q = 3329`) and bit + /// `0` to `0`, placing a set bit at the point farthest from `0` to maximize the + /// decryption noise margin. The mapping is computed branchlessly (constant-time) + /// via a bit-derived all-ones / all-zeros mask, and bits are read LSB-first. This + /// is the exact inverse of [`to_msg`]. pub(crate) fn from_msg(m: [u8; 32]) -> Self { let mut w = Polynomial::new(); @@ -54,16 +62,26 @@ impl Polynomial { w } - /// Convert a Polynomial back into a message m + /// Decodes a `Polynomial` into its 32-byte message `m`, implementing the message + /// recovery step of K-PKE.Decrypt `ByteEncode_1(Compress_1(self))`, + /// (FIPS 203, Alg. 15). Each coefficient yields one message bit: `Compress_1` + /// (§4.2.1) sets the bit when the coefficient lies nearer `q/2` than `0`, i.e. in + /// the central interval `[833, 2496]` for `q = 3329`. The decision is computed + /// branchlessly and the bits are packed LSB-first. + /// Coefficients are expected to already be canonical in `[0, q]`: the unsigned + /// interval test is not periodic mod `q`, so the caller reduces beforehand (`poly_reduce()` + /// in `pke_decrypt`) and no reduction is repeated here. pub(crate) fn to_msg(self) -> [u8; 32] { - const LOWER: i32 = q as i32 >> 2; // 832 - const UPPER: i32 = q as i32 - LOWER; // 2497 + const LOWER: i32 = q as i32 >> 2; // ⌊q/4⌋ = 832 + const UPPER: i32 = q as i32 - LOWER; // q - ⌊q/2⌋ = 2497 let mut msg = [0u8; 32]; - // you would expect to use a full reduce() here, but since this is data coming from - // out matrix math and not from an attacker, we can get away with the lighter cond_sub_q(). - // Actually; further testing against the bc-test-data set of KATs shows that everything passes even with nothing + // Using full reduce() might be expected here. + // However, this function is only called by pke_decrypt (see mlkem.rs), which performs a + // reduction on every coefficient of the polynomial immediately prior to the call. + // For completeness, testing against the bc-test-data set of KATs shows that everything passes + // without modular reduction. // self.cond_sub_q(); // for (i, item) in msg.iter_mut().enumerate().take(N/8) { @@ -78,8 +96,9 @@ impl Polynomial { msg } - // not currently used, but I'll leave it here because it's useful for debugging if you want to output values - // that are normalized to [0,q] to compare against intermediate results from other libraries. + // Not currently used. It is left here as a reference since it's useful for debugging if it's + // necessary to output values that are normalized to [0,q] to compare against intermediate results + // from other libraries. // pub(crate) fn conditional_add_q(&mut self) { // for x in self.0.iter_mut() { // *x = conditional_add_q(*x); @@ -123,14 +142,17 @@ impl Polynomial { // make sure we have received a dv debug_assert!(dv == 4 || dv == 5); - // make sure we were given the right size output buffer + // make sure the right size output buffer is given // each of the N i16's will take dv bits debug_assert_eq!(out.len(), N * (dv as usize) / 8); let mut t = [0u8; 8]; let mut idx = 0; - // bc-java has a cond_sub_q() here, but unit tests show that we don't need it. + // bc-java has a cond_sub_q() here, however, it is not needed + // The reason for this is because a modular reduction is performed immediately + // prior to calling pack_ciphertext in mlkem.rs + // This can be corroborated by running the corresponding unit tests // let mut s = self.clone(); // s.cond_sub_q(); @@ -173,13 +195,13 @@ impl Polynomial { } /// This is an optimized version of - /// Decompress_𝑑𝑣( ByteDecode_𝑑𝑣(𝑐2) ) + /// Decompress_𝑑𝑣( ByteDecode_𝑑𝑣(𝑐2) ) /// which unpacks a single polynomial according to the packing coefficient dv pub(crate) fn decompress_poly(compressed_v: &[u8]) -> Polynomial { - // make sure we have received a dv + // make sure to received a dv debug_assert!(dv == 4 || dv == 5); - // make sure we were given the right size output buffer + // make sure the right size output buffer is given // each of the N i16's will take dv bits debug_assert_eq!(compressed_v.len(), N * (dv as usize) / 8); @@ -223,8 +245,9 @@ impl Polynomial { v } - // not currently used, but I'll leave it here because it's useful for debugging if you want to output values - // that are normalized to [0,q] to compare against intermediate results from other libraries. + // Not currently used. It is left here as a reference since it's useful for debugging if it's + // necessary to output values that are normalized to [0,q] to compare against intermediate results + // from other libraries. // pub(crate) fn cond_sub_q(&mut self) { // for i in 0..N { // self[i] = cond_sub_q(self[i]); @@ -259,11 +282,11 @@ impl Polynomial { /// Algorithm 10 NTT (𝑓_hat) /// Computes the polynomial 𝑓 ∈ 𝑅𝑞 that corresponds to the given NTT representation 𝑓 ∈ 𝑇𝑞. - /// Input: array 𝑓 ∈ ℤ256 ▷ the coefficients of input NTT representation - /// Output: array 𝑓 ∈ ℤ256 ▷ the coefficients of the inverse NTT of the input + /// Input: array 𝑓 ∈ ℤ_{256} ▷ the coefficients of input NTT representation + /// Output: array 𝑓 ∈ ℤ_{256} ▷ the coefficients of the inverse NTT of the input pub(crate) fn inv_ntt(&mut self) { // FIPS 203 ALg 10 wants you to copy f_hat into f, and then act of f - // but we're going to do this in-place for memory-saving reasons. + // but here it is performed in-place in order to optimize memory usage. let mut len = 2; let mut k = 0; @@ -346,8 +369,9 @@ impl Display for Polynomial { } } -// Not currently used, but I'll leave it here because it's useful for debugging if you want to output values -// that are normalized to [0,q] to compare against intermediate results from other libraries. +// Not currently used. It is left here as a reference since it's useful for debugging if it's +// necessary to output values that are normalized to [0,q] to compare against intermediate results +// from other libraries. // /// if a is in \[-q..0], then it shifts it up by q to be in \[0..q] // pub(crate) fn conditional_add_q(a: i16) -> i16 { // a + ((a >> 15) & q) diff --git a/crypto/mlkem/tests/mlkem_key_tests.rs b/crypto/mlkem/tests/mlkem_key_tests.rs index 7317d50..66b54a8 100644 --- a/crypto/mlkem/tests/mlkem_key_tests.rs +++ b/crypto/mlkem/tests/mlkem_key_tests.rs @@ -35,13 +35,13 @@ mod mlkem_key_tests { let expected_pk_bytes: [u8; MLKEM512_PK_LEN] = hex::decode("3995815e597d104355cf29aa5333c93251869d5bcdbe487124f602b8b6a66c16c4761648ad765cf5d8006b515e905a7f0ac076b0c62efa328153e7ca5701699f1305f1e6bc6f90b0e49b693512b6ce992a8b8016ddfc1a662c7e3f9619cbd869dd771af30896ccd5918ac6cb77466c5e779996d67ff9aabc97503f2c7b7e2d000d86450fb1807ca4cabda465825a31c789a1b7a491ab3872765d320d0b71920fa213c94093416b83b8124e69f65e62cb5000dcc37aa9a0fff73970c4772f357d24189ca6f5305568c0e2376a3762a68c605e563c5d209572e0fc7532ca294729535567b5fc413c5e8792d2464536cc808f98add74664f141566f9016a90a541829a98a0464ce41a8bb44c2d4fa3c2c209460728ef14a1a7c4c9b98d12203b4cc3529160a9ab2d7838f7ff6b53ae05aa31a7d646b7afa6c45932526a3c3755619be994c211c2a31c05b3447836cb2150be1829dae6b04c5535cff546e392ba797411720f924f490a5ac5495f21356d550b782a64c1688b6b655bcc7842197a434c2f6563b5b7f09a78bcc488232783561d16f4cbab6755400050781570c66604b817ad1252294736e8b01861a4b5a74519b8b6fe51489a5072392e587626c713776575d33806a1c8e2732af97c2680f51666331c4eb8bbc0431c4f96832daf1b3c45528fba153f6c78b1c198702947ccd337727a46fb53ba11de5cb4191346859516cb6ad72400f3cf209b236aef35a580ac87eb3e30fafd66973ca8a7dd2675af41f7a17b61433cd1af80f7708869f665488497980b1ac10a0cdcb636a00ed8681b35e429124ca80350725b85f83a5eac3a4a3cc1600903e65293560b9b336e5af0d529dac1a048119302cb7a9bcc110b94851bf02117f199dc485a852b7473f09b831a6831d5b54c0b790d225cf6bb92d9462a26cdb33dda5123c7aaf0e26a0b83655eea28bf3a8074725018fd6bae4b601cf61baab71a7a3d35197a343e74b4a272c125d540896426d85b7958d3b38a6ba987ec37225c7b44cdb12dde4539b4ab082363683f04bf7a09cc5c41dfe830a1b162e0b324334362f084a14467723344badd000f8d8c537c48f998f05307cebd1ede0b81c3bc59a065a1b6d63b26c").unwrap() .try_into().unwrap(); - // Decode and re-encode the sk, make sure you get the same thing + // Decode and re-encode the sk, ensure the output is the same let sk = MLKEM512PrivateKey::from_bytes(&expected_sk_bytes).unwrap(); let sk_bytes = sk.encode(); assert_eq!(sk_bytes.len(), expected_sk_bytes.len()); assert_eq!(sk_bytes, expected_sk_bytes.as_slice()); - // Decode and re-encode the pk, make sure you get the same thing + // Decode and re-encode the pk, ensure the output is the same let decoded_pk = MLKEM512PublicKey::from_bytes(&expected_pk_bytes).unwrap(); let pk_bytes = decoded_pk.encode(); assert_eq!(pk_bytes.len(), expected_pk_bytes.len()); @@ -54,9 +54,10 @@ mod mlkem_key_tests { #[test] fn test_ek_hash() { // three separate tests here: - // 1) does it calculate H(ek) properly from a public key? - // 2) does it calculate H(ek) properly from a private key? - // 3) does it reject a private key if the H(ek) is wrong? + // 1) whether it calculates H(ek) properly from a public key + // 2) whether it calculates H(ek) properly from a private key + // 3) whether it rejects a private key if the H(ek) is wrong + let seed = KeyMaterial512::from_bytes_as_type( &hex::decode( @@ -80,10 +81,12 @@ mod mlkem_key_tests { .try_into() .unwrap(); + // 1) test whether it calculates H(ek) properly from a public key assert_eq!(pk.compute_hash(), expected_h_ek); + // 2) test whether it calculates H(ek) properly from a private key assert_eq!(sk.pk_hash(), &expected_h_ek); - // 3) does it reject a private key if the H(ek) is wrong? + // 3) test whether it rejects a private key if the H(ek) is wrong let mut sk_bytes: [u8; MLKEM512_SK_LEN] = sk.encode(); // h is: // dk[768𝑘 + 32 ∶ 768𝑘 + 64] @@ -204,7 +207,7 @@ mod mlkem_key_tests { // FIPS 203 says: // " Indeed, some 12-bit segments could // correspond to an integer greater than 𝑞 − 1 = 3328 but less than 4096." - // so let's test these conditions in both private key s_hat and public key t_hat + // Test these conditions in both private key s_hat and public key t_hat match MLKEM512PrivateKey::from_bytes(&[255u8; MLKEM512_SK_LEN]) { Err(KEMError::DecodingError(_)) => { /* good */ } diff --git a/crypto/mlkem/tests/mlkem_tests.rs b/crypto/mlkem/tests/mlkem_tests.rs index c24abd2..8d20b67 100644 --- a/crypto/mlkem/tests/mlkem_tests.rs +++ b/crypto/mlkem/tests/mlkem_tests.rs @@ -47,7 +47,7 @@ mod mlkem_tests { } /// This runs the full bitflipping tests and takes about 30s.. - /// I'm leaving this commented out, but feel free to un-comment it and run it. + /// This is left out of the testing suite, but it can be included if so desired // #[test] // fn test_framework_kem_extensive() { // use bouncycastle_core_test_framework::kem::{TestFrameworkKEM}; @@ -262,7 +262,7 @@ mod mlkem_tests { let expected_pk_bytes: [u8; MLKEM1024_PK_LEN] = hex::decode("A04184D4BC7B532A0F70A54D7757CDE6175A6843B861CB2BC4830C0012554CFC5D2C8A2027AA3CD967130E9B96241B11C4320C7649CC23A71BAFE691AFC08E680BCEF42907000718E4EACE8DA28214197BE1C269DA9CB541E1A3CE97CFADF9C6058780FE6793DBFA8218A2760B802B8DA2AA271A38772523A76736A7A31B9D3037AD21CEBB11A472B8792EB17558B940E70883F264592C689B240BB43D5408BF446432F412F4B9A5F6865CC252A43CF40A320391555591D67561FDD05353AB6B019B3A08A73353D51B6113AB2FA51D975648EE254AF89A230504A236A4658257740BDCBBE1708AB022C3C588A410DB3B9C308A06275BDF5B4859D3A2617A295E1A22F90198BAD0166F4A943417C5B831736CB2C8580ABFDE5714B586ABEEC0A175A08BC710C7A2895DE93AC438061BF7765D0D21CD418167CAF89D1EFC3448BCBB96D69B3E010C82D15CAB6CACC6799D3639669A5B21A633C865F8593B5B7BC800262BB837A924A6C5440E4FC73B41B23092C3912F4C6BEBB4C7B4C62908B03775666C22220DF9C88823E344C7308332345C8B795D34E8C051F21F5A21C214B69841358709B1C305B32CC2C3806AE9CCD3819FFF4507FE520FBFC27199BC23BE6B9B2D2AC1717579AC769279E2A7AAC68A371A47BA3A7DBE016F14E1A727333663C4A5CD1A0F8836CF7B5C49AC51485CA60345C990E06888720003731322C5B8CD5E6907FDA1157F468FD3FC20FA8175EEC95C291A262BA8C5BE990872418930852339D88A19B37FEFA3CFE82175C224407CA414BAEB37923B4D2D83134AE154E490A9B45A0563B06C953C3301450A2176A07C614A74E3478E48509F9A60AE945A8EBC7815121D90A3B0E07091A096CF02C57B25BCA58126AD0C629CE166A7EDB4B33221A0D3F72B85D562EC698B7D0A913D73806F1C5C87B38EC003CB303A3DC51B4B35356A67826D6EDAA8FEB93B98493B2D1C11B676A6AD9506A1AAAE13A824C7C08D1C6C2C4DBA9642C76EA7F6C8264B64A23CCCA9A74635FCBF03E00F1B5722B214376790793B2C4F0A13B5C40760B4218E1D2594DCB30A70D9C1782A5DD30576FA4144BFC8416EDA8118FC6472F56A979586F33BB070FB0F1B0B10BC4897EBE01BCA3893D4E16ADB25093A7417D0708C83A26322E22E6330091E30152BF823597C04CCF4CFC7331578F43A2726CCB428289A90C863259DD180C5FF142BEF41C7717094BE07856DA2B140FA67710967356AA47DFBC8D255B4722AB86D439B7E0A6090251D2D4C1ED5F20BBE6807BF65A90B7CB2EC0102AF02809DC9AC7D0A3ABC69C18365BCFF59185F33996887746185906C0191AED4407E139446459BE29C6822717644353D24AB6339156A9C424909F0A9025BB74720779BE43F16D81C8CC666E99710D8C68BB5CC4E12F314E925A551F09CC59003A1F88103C254BB978D75F394D3540E31E771CDA36E39EC54A62B5832664D821A72F1E6AFBBA27F84295B2694C498498E812BC8E9378FE541CEC5891B25062901CB7212E3CDC46179EC5BCEC10BC0B9311DE05074290687FD6A5392671654284CD9C8CC3EBA80EB3B662EB53EB75116704A1FEB5C2D056338532868DDF24EB8992AB8565D9E490CADF14804360DAA90718EAB616BAB0765D33987B47EFB6599C5563235E61E4BE670E97955AB292D9732CB8930948AC82DF230AC72297A23679D6B94C17F1359483254FEDC2F05819F0D069A443B78E3FC6C3EF4714B05A3FCA81CBBA60242A7060CD885D8F39981BB18092B23DAA59FD9578388688A09BBA079BC809A54843A60385E2310BBCBCC0213CE3DFAAB33B47F9D6305BC95C6107813C585C4B657BF30542833B14949F573C0612AD524BAAE69590C1277B86C286571BF66B3CFF46A3858C09906A794DF4A06E9D4B0A2E43F10F72A6C6C47E5646E2C799B71C33ED2F01EEB45938EB7A4E2E2908C53558A540D350369FA189C616943F7981D7618CF02A5B0A2BCC422E857D1A47871253D08293C1C179BCDC0437069107418205FDB9856623B8CA6B694C96C084B17F13BB6DF12B2CFBBC2B0E0C34B00D0FCD0AECFB27924F6984E747BE2A09D83A8664590A8077331491A4F7D720843F23E652C6FA840308DB4020337AAD37967034A9FB523B67CA70330F02D9EA20C1E84CB8E5757C9E1896B60581441ED618AA5B26DA56C0A5A73C4DCFD755E610B4FC81FF84E21").unwrap() .try_into().unwrap(); - // we're just going to keep the pk + // pk is kept here let (pk, _sk) = MLKEM1024::keygen_from_seed(&seed).unwrap(); assert_eq!(pk.encode(), expected_pk_bytes.as_slice()); @@ -371,13 +371,14 @@ mod mlkem_tests { // 2. (Decapsulation key type check) If dk is not a byte array of length 768𝑘 + 96 for the value of // 𝑘 specified by the relevant parameter set, then input checking has failed. - // This does not need to be tested because of the static-sizing of the SK array. + // This does not need to be tested because of the static-sizing of the dk array. // 3. (Hash check) Perform the computation // test ← H(dk[384𝑘 ∶ 768𝑘 + 32])) (7.2) // If test ≠ dk[768𝑘 + 32 ∶ 768𝑘 + 64], then input checking has failed. - // Sure, let's test a busted ek component of the sk - // This is actually caught on loading the sk, not on decaps, which is better for performance + + // Test the procedure with portions of dk corresponding to ek that have been corrupted + // This is actually caught on loading the dk, not on decaps, which is better for performance // since you might do many decaps's on one key, and that should be fine for FIPS? let (pk1, sk) = MLKEM512::keygen().unwrap(); diff --git a/crypto/rng/src/hash_drbg80090a.rs b/crypto/rng/src/hash_drbg80090a.rs index 431d036..278bd0e 100644 --- a/crypto/rng/src/hash_drbg80090a.rs +++ b/crypto/rng/src/hash_drbg80090a.rs @@ -61,13 +61,14 @@ impl HashDRBG80090AParams for HashDRBG80090AParams_SHA512 { const RESEED_INTERVAL: u64 = 1u64 << 48; // 2^48 requests } -// TODO: is there a rustacious way to extract this from HASH? +// TODO: is there a rust way to extract this from HASH? const LARGEST_HASHER_OUTPUT_LEN: usize = 64; #[allow(private_bounds)] /// Implementation of the Hash_DRBG algorithm as specified in NIST SP 800-90Ar1. pub struct HashDRBG80090A { - // Rust is stupid. What's the point of having a generic parameter if we can't use constants inside it? + // Ideally `state: WorkingState`, but stable Rust can't use an associated const + // of a generic type parameter as a const-generic argument // state: WorkingState, state: WorkingState, admin_info: AdministrativeInfo, @@ -194,7 +195,7 @@ impl Sp80090ADrbg for HashDRBG80090A { )); } - // todo: take this out once supported + // TODO: take this out once supported if prediction_resistance { todo!("Prediction resistance is not yet supported by Hash_DRBG80090A.") } @@ -219,7 +220,8 @@ impl Sp80090ADrbg for HashDRBG80090A { "Provided seed exceeds the maximum seed length.", ))?; } - // On purpose not checking the SecurityStrength field of the seed, because we assume it's pure entropy and hasn't been touched by any actual algoritms yet. + // On purpose not checking the SecurityStrength field of the seed, + // because we assume it's pure entropy and hasn't been touched by any actual algoritms yet. if security_strength > H::MAX_SECURITY_STRENGTH { return Err(KeyMaterialError::SecurityStrength( "Requested security strength exceeds the maximum strength that this DRBG instance can provide.", @@ -517,7 +519,8 @@ impl RNG for HashDRBG80090A { /// the hash_df function as defined in SP 800-90Ar1 section 10.3.1. /// no_of_bits_to_return is the length of the provided output buffer. -/// Because array concatenation is not available in a no_std / no_alloc build, this takes many input parameters. To leave a parameter unused, simply provide an empty array &[0u8;0] +/// Because array concatenation is not available in a no_std / no_alloc build, this takes many input parameters. +// To leave a parameter unused, simply provide an empty array &[0u8;0] fn hash_df( in1: &[u8], in2: &[u8], @@ -527,7 +530,7 @@ fn hash_df( ) { // Note: all lengths here are in bytes, whereas the spec uses bits. - // // I'm gonna panic! here because this is private and shouldn't get into weird inputs. + // The implementation panic! here because this is private and shouldn't get into weird inputs. if out.len() > 255 * H::OUTPUT_LEN { panic!("hash_df can't produce that much output!") } @@ -539,8 +542,8 @@ fn hash_df( let len = u32::div_ceil(out.len() as u32, H::OUTPUT_LEN as u32); let mut counter: u8 = 0x01; - // note: this could probably be performance optimized a tiny bit by pulling no_of_bits_to_return.to_le_bytes() out of the loop - // and by merging i and counter into the same variable. + // note: this could probably be performance optimized a tiny bit by pulling no_of_bits_to_return.to_le_bytes() + // out of the loop and by merging i and counter into the same variable. for i in 1..len { let mut h = H::default(); h.do_update(&counter.to_le_bytes()); @@ -555,7 +558,8 @@ fn hash_df( } // Handle the last block separately since not all of it will fit in the output buffer. - // First, do we even need to do a last block, or was the requested number of bits already a multiple of the output length? + // TODO: Check whether it is necessary to do a last block, + // or was the requested number of bits already a multiple of the output length let bytes_written = (len - 1) as usize * H::OUTPUT_LEN; let remainder = out.len() - bytes_written; if remainder != 0 { @@ -567,7 +571,6 @@ fn hash_df( h.do_update(in3); h.do_update(in4); - // I don't understand rust // let mut temp = [0u8; H::OUTPUT_LEN]; let mut temp = [0u8; 64]; h.do_final_out(&mut temp); @@ -662,11 +665,11 @@ fn hashgen(v: &[u8], out: &mut [u8]) { } // Handle the last block separately since not all of it will fit in the output buffer. - // First, do we even need to do a last block, or was the requested number of bits already a multiple of the output length? + // TODO: Check whether it is necessary to do a last block, + // or was the requested number of bits already a multiple of the output length let bytes_written = (m - 1) as usize * H::OUTPUT_LEN; let remainder = out.len() - bytes_written; if remainder != 0 { - // I don't understand rust // let mut temp = [0u8; H::OUTPUT_LEN]; let mut temp = [0u8; 64]; H::default().hash_out(&data, &mut temp); diff --git a/crypto/rng/src/lib.rs b/crypto/rng/src/lib.rs index dbe62fb..a4dccab 100644 --- a/crypto/rng/src/lib.rs +++ b/crypto/rng/src/lib.rs @@ -3,7 +3,7 @@ //! This crate provides the implementations of the deterministic random bit generator (DRBG) algorithms //! which, together with a strong entropy source, form the basis of cryptographic random number generation. //! -//! Here's the basic way to get some random bytes: +//! Here is the basic way to get some random bytes: //! //! ``` //! use bouncycastle_core::traits::RNG; @@ -13,7 +13,7 @@ //! ``` //! This is secure because `::default()` seeds the RNG from the OS, configured for general use. //! -//! **WARNING: most people should stop reading here and should not be mucking around with the internals of RNGs. +//! **WARNING: most people should stop reading here and should not attempt to modify the internals of RNGs. //! This crate contains dragons and other horrible things. 🐉🐍🐜** //! //! # 🚨🚨🚨Security Warning 🚨🚨🚨 @@ -60,8 +60,14 @@ pub type DefaultRNG = HashDRBG_SHA512; pub type Default128BitRNG = HashDRBG_SHA256; pub type Default256BitRNG = HashDRBG_SHA512; -/// Implements the five functions specified in SP 800-90A section 7.4 are instantate, generate, reseed, uninstantiate, and health_test. -/// Note: this function implements Rust's Drop on the sensitive working state in place of the explicit Uninstantiate function listed in SP 800-90Ar1. +/// Implements the five functions specified in SP 800-90A section 7.4 are +/// - instantate, +/// - generate, +/// - reseed, +/// - uninstantiate, and +/// - health_test. +/// Note: this function implements Rust's Drop on the sensitive working state in place of the explicit +/// Uninstantiate function listed in SP 800-90Ar1. pub trait Sp80090ADrbg { /// The input KeyMaterial must be of type [KeyType::Seed]. /// @@ -79,7 +85,8 @@ pub trait Sp80090ADrbg { /// required. /// """ /// - /// This function takes ownership of the seed KeyMaterial object, to reduce the likelihood of its reuse in a second function call. + /// This function takes ownership of the seed KeyMaterial object, + /// to reduce the likelihood of its reuse in a second function call. /// /// There is no entropy requirement on the nonce, but it is expected as a KeyMaterial so that it /// benefits from the secure erasure and logging protections in the KeyMaterial object. @@ -93,7 +100,8 @@ pub trait Sp80090ADrbg { ) -> Result<(), RNGError>; /// Reseeds the DRBG with the provided seed. - /// TODO: this needs to be thought out to take some sort of EntropySource object that'll work well with DRBGs that require frequent reseeding. + /// TODO: this needs to be redesigned to take some sort of EntropySource object that will work well + // with DRBGs that require frequent reseeding. fn reseed( &mut self, seed: &impl KeyMaterialTrait, diff --git a/crypto/rng/tests/hash_drbg80090a_tests.rs b/crypto/rng/tests/hash_drbg80090a_tests.rs index e4fd534..c08a81e 100644 --- a/crypto/rng/tests/hash_drbg80090a_tests.rs +++ b/crypto/rng/tests/hash_drbg80090a_tests.rs @@ -85,9 +85,12 @@ mod tests { _ => panic!("Expected KeyMaterialError error"), } - // Skipping tests for max lengths of seeds and personalization strings, because they're in the gigabyte range and that'll blow up the test suite. + // Skipping tests for max lengths of seeds and personalization strings + // because they are on the order of a gigabyte in size. + // Testing would blow up the test suite. - // Error case: security strength requested at init is higher than the underlying hash function's max security strength + // Error case: security strength requested at init is higher than the underlying + // hash function's max security strength let mut rng = HashDRBG_SHA256::new_unititialized(); let seed = KeyMaterial256::from_bytes_as_type(&DUMMY_SEED_512[..32], KeyType::Seed).unwrap(); @@ -96,7 +99,8 @@ mod tests { _ => panic!("Expected KeyMaterialError error"), } - // Success case: security strength requested at init is lower than the underlying hash function's max security strength + // Success case: security strength requested at init is lower than the underlying + // hash function's max security strength // ... 112 bit let mut rng = HashDRBG_SHA256::new_unititialized(); let seed = @@ -162,7 +166,10 @@ mod tests { _ => panic!("Expected KeyMaterialError error"), } - // Skipping tests for max lengths of seeds and personalization strings, because they're in the gigabyte range and that'll blow up the test suite. + // Skipping tests for max lengths of seeds and personalization strings + // because they are on the order of a gigabyte in size. + // Testing would blow up the test suite. + } #[test] @@ -197,9 +204,11 @@ mod tests { _ => panic!("Expected Uninitialized error"), } - // Skipping tests for max lengths of seeds and personalization strings, because they're in the gigabyte range and that'll blow up the test suite. + // Skipping tests for max lengths of seeds and personalization strings + // because they are on the order of a gigabyte in size. + // Testing would blow up the test suite. - // TODO: tests for ReseedRequired. How do I trigger this? The limits are in the exobyte range. + // TODO: Tests for ReseedRequired. Investigate how this gets triggered. The limits are in the exobyte range. } #[test] @@ -241,9 +250,11 @@ mod tests { _ => panic!("Expected Uninitialized error"), } - // Skipping tests for max lengths of seeds and personalization strings, because they're in the gigabyte range and that'll blow up the test suite. + // Skipping tests for max lengths of seeds and personalization strings + // because they are on the order of a gigabyte in size. + // Testing would blow up the test suite. - // TODO: tests for ReseedRequired. How do I trigger this? The limits are in the exobyte range. + // TODO: tests for ReseedRequired. Investigate how this gets triggered. The limits are in the exobyte range. } #[test] @@ -289,9 +300,11 @@ mod tests { _ => panic!("Expected Uninitialized error"), } - // Skipping tests for max lengths of seeds and personalization strings, because they're in the gigabyte range and that'll blow up the test suite. + // Skipping tests for max lengths of seeds and personalization strings + // because they are on the order of a gigabyte in size. + // Testing would blow up the test suite. - // TODO: tests for ReseedRequired. How do I trigger this? The limits are in the exobyte range. + // TODO: tests for ReseedRequired. Investigate how this gets triggered. The limits are in the exobyte range. } #[test] diff --git a/crypto/sha2/src/sha256.rs b/crypto/sha2/src/sha256.rs index 7be7f47..617cb54 100644 --- a/crypto/sha2/src/sha256.rs +++ b/crypto/sha2/src/sha256.rs @@ -154,7 +154,8 @@ pub struct SHA256Internal { byte_count: u64, x_buf: [u8; 64], x_buf_off: usize, - // TODO: should we add a maximum message size according to FIPS 180-4? (2^64 for SHA256 and 2^128 for SHA512) + // TODO: Investigate whether maximum message size (according to FIPS 180-4) should be added + // (2^64 for SHA256 and 2^128 for SHA512) } impl Drop for SHA256Internal { @@ -279,7 +280,7 @@ impl Hash for SHA256Internal { /// TODO: This is defined in FIPS 180-4 s. 5.1.2 /// TODO: - /// TODO: Could implement if there is demand. + /// TODO: It can be implemented if required #[allow(unused)] fn do_final_partial_bits( self, @@ -291,7 +292,7 @@ impl Hash for SHA256Internal { /// TODO: This is defined in FIPS 180-4 s. 5.1.2 /// TODO: - /// TODO: Could implement if there is demand. + /// TODO: It can be implemented if required #[allow(unused)] fn do_final_partial_bits_out( self, diff --git a/crypto/sha2/src/sha512.rs b/crypto/sha2/src/sha512.rs index b4ae990..541c1a9 100644 --- a/crypto/sha2/src/sha512.rs +++ b/crypto/sha2/src/sha512.rs @@ -163,7 +163,8 @@ impl Sha512State { pub struct Sha512Internal { _params: std::marker::PhantomData, state: Sha512State, - byte_count: u64, // NOTE We only support 2^67 bits, not the full 2^128 + // NOTE The code currently only supports 2^67 bits, not the full 2^128 + byte_count: u64, x_buf: [u8; 128], x_buf_off: usize, } @@ -290,7 +291,7 @@ impl Hash for Sha512Internal { /// TODO: This is defined in FIPS 180-4 s. 5.1.2 /// TODO: - /// TODO: Could implement if there is demand. + /// TODO: It can be implemented if required #[allow(unused)] fn do_final_partial_bits( self, @@ -302,7 +303,7 @@ impl Hash for Sha512Internal { /// TODO: This is defined in FIPS 180-4 s. 5.1.2 /// TODO: - /// TODO: Could implement if there is demand. + /// TODO: It can be implemented if required #[allow(unused)] fn do_final_partial_bits_out( self, diff --git a/crypto/sha3/src/keccak.rs b/crypto/sha3/src/keccak.rs index 5e6f6a8..c446b72 100644 --- a/crypto/sha3/src/keccak.rs +++ b/crypto/sha3/src/keccak.rs @@ -179,7 +179,7 @@ impl KeccakState { } } -// Mutants note: this fails because you can't write unit tests for drop() +// Mutants note: this fails because unit tests cannot be written for drop() impl Drop for KeccakState { fn drop(&mut self) { // Zeroize the contents before returning the memory to the OS. @@ -210,7 +210,9 @@ impl KeccakDigest { pub(super) fn new(size: KeccakSize) -> Self { let rate = 1600 - ((size as usize) << 1); - // todo I think this check is not needed since the fixed set of allowed sizes can't yield an invalid rate, but I'll leave this here for now. + // TODO: Verify whether this check is needed. + // The code currently skips the check since the fixed set of allowed sizes can't yield an + // invalid rate. It is, for now, left as a comment for reference. // if rate == 0 || rate >= 1600 || (rate & 63) != 0 { // return Err(HashError::InvalidLength("invalid rate value")); // } @@ -229,8 +231,8 @@ impl KeccakDigest { panic!("attempt to absorb with odd length queue"); } if self.squeezing { - // Maybe this should be an error rather than a panic? - // But, like, don't write your code to absorb after squeezing. + // TODO: check whether this should be an error rather than a panic + // It should warn the user to not absorb after squeezing. panic!("attempt to absorb while squeezing"); } diff --git a/crypto/sha3/src/lib.rs b/crypto/sha3/src/lib.rs index 42ecac4..8f944fc 100644 --- a/crypto/sha3/src/lib.rs +++ b/crypto/sha3/src/lib.rs @@ -146,7 +146,7 @@ trait SHA3Params: HashAlgParams { const SIZE: KeccakSize; } -// TODO: more elegant to macro this? +// TODO: Consider whether is better to macro this impl Algorithm for SHA3_224 { const ALG_NAME: &'static str = SHA3_224_NAME; const MAX_SECURITY_STRENGTH: SecurityStrength = SecurityStrength::_112bit; diff --git a/crypto/sha3/src/sha3.rs b/crypto/sha3/src/sha3.rs index cb22998..8ec39eb 100644 --- a/crypto/sha3/src/sha3.rs +++ b/crypto/sha3/src/sha3.rs @@ -14,7 +14,7 @@ pub struct SHA3 { kdf_entropy: usize, } -// Note: don't need a zeroizing Drop here because all the sensitive info is in KeccakDigest, which has one. +// Note: zeroizing Drop is not necessary here because all the sensitive info is in KeccakDigest, which has one. impl SHA3 { pub fn new() -> Self { @@ -71,7 +71,7 @@ impl SHA3 { ) -> Result { // For the KDF to be considered "fully-seeded" and be capable of outputting full-entropy KeyMaterials, // it requires full-entropy input that is at least block length. - // TODO: citation needed, which NIST spec did I get this from? + // TODO: citation needed (NIST) if self.kdf_entropy < PARAMS::OUTPUT_LEN { self.kdf_key_type = min(&self.kdf_key_type, &KeyType::BytesLowEntropy).clone(); self.kdf_security_strength = SecurityStrength::None; // BytesLowEntropy can't have a securtiy level. @@ -85,7 +85,7 @@ impl SHA3 { let bytes_written = self.do_final_out(output_key.mut_ref_to_bytes()?); output_key.set_key_len(bytes_written)?; - // since we've done some computation, the result will not actually be zeroized, even if all input key material was zeroized. + // since computation has been performed, the result will not actually be zeroized, even if all input key material was zeroized. if key_type == KeyType::Zeroized { key_type = KeyType::BytesLowEntropy; } @@ -141,12 +141,14 @@ impl Hash for SHA3 { output } - // todo -- why doesn't this take a &mut [u8; HASH_LEN] ? - // That's probably more user-friendly than this auto-truncating that I have here. + // TODO: investigate why this doesn't take a &mut [u8; HASH_LEN] + // Being able to do so would improve ergonomics fn do_final_out(mut self, output: &mut [u8]) -> usize { output.fill(0); - self.keccak.absorb_bits(0x02, 2).expect("do_final_out: keccak.absorb_bits failed."); // this shouldn't fail because by construction you can only enter this function once, and this is the only way to absorb partial bits. + // this shouldn't fail because, by construction, the function is only called once, + // and this is the only way to absorb partial bits. + self.keccak.absorb_bits(0x02, 2).expect("do_final_out: keccak.absorb_bits failed."); let bytes_written = if output.len() <= self.output_len() { self.keccak.squeeze(output) @@ -180,7 +182,8 @@ impl Hash for SHA3 { ) -> Result { output.fill(0); - // Mutants note: yep, this is just bit-setting into empty space, so it doesn't matter whether it's OR or XOR. + // Mutants note: This is just bit-setting into empty space. + // It works the same regardless of whether it's OR or XOR. let mut final_input: u16 = ((partial_byte as u16) & ((1 << num_partial_bits) - 1)) | (0x02 << num_partial_bits); let mut final_bits = num_partial_bits + 2; diff --git a/crypto/sha3/src/shake.rs b/crypto/sha3/src/shake.rs index 59f3bac..ae64a3a 100644 --- a/crypto/sha3/src/shake.rs +++ b/crypto/sha3/src/shake.rs @@ -28,7 +28,7 @@ pub struct SHAKE { kdf_entropy: usize, } -// Note: don't need a zeroizing Drop here because all the sensitive info is in KeccakDigest, which has one. +// Note: zeroizing Drop here is not needed because all the sensitive info is in KeccakDigest, which has one. impl Algorithm for SHAKE { const ALG_NAME: &'static str = PARAMS::ALG_NAME; @@ -82,8 +82,7 @@ impl SHAKE { mut self, additional_input: &[u8], ) -> Result, KDFError> { - // It's unfortunate to return an oversized KeyMaterial most of the time, but I've had enough - // of fighting with Rust traits for now ... + // At the moment, oversized KeyMaterial is returned for most cases. let mut output_key = KeyMaterial::<64>::new(); self.derive_key_out_final_internal(additional_input, &mut output_key)?; @@ -99,9 +98,9 @@ impl SHAKE { ) -> Result { // For the KDF to be considered "fully-seeded" and be capable of outputting full-entropy KeyMaterials, // it requires full-entropy input that is at least 2x the bit size (ie 256 bits for SHAKE128, and 512 bits for SHAKE256). - // TODO: citation needed, which NIST spec did I get this from? - // TODO: intuitivitely this makes sense since SHAKE256 and SHA3-256 are both KECCAK[512], and SHAKE128 is KECCAK[256], - // TODO: but I would rather find an actual reference for this "fully-seeded" threshold. + // TODO: citation needed (NIST) + // TODO: The intuition behind this is that SHAKE256 and SHA3-256 are both KECCAK[512], and SHAKE128 is KECCAK[256], + // TODO: However, it is necessary to find an actual reference for this "fully-seeded" threshold. if self.kdf_entropy < 2 * (PARAMS::SIZE as usize) / 8 { self.kdf_key_type = min(&self.kdf_key_type, &KeyType::BytesLowEntropy).clone(); self.kdf_security_strength = SecurityStrength::None; // BytesLowEntropy can't have a securtiy level. @@ -118,7 +117,7 @@ impl SHAKE { ); output_key.set_key_len(bytes_written)?; - // since we've done some computation, the result will not actually be zeroized, even if all input key material was zeroized. + // since computation has been performed, the result will not actually be zeroized, even if all input key material was zeroized. if self.kdf_key_type == KeyType::Zeroized { self.kdf_key_type = KeyType::BytesLowEntropy; } @@ -225,7 +224,8 @@ impl XOF for SHAKE { if !(1..=7).contains(&num_partial_bits) { return Err(HashError::InvalidLength("must be in the range [0,7]")); } - // Mutants note: yep, this is just bit-setting into empty space, so it doesn't matter whether it's OR or XOR. + // Mutants note: This is just bit-setting into empty space. + // It works the same regardless of whether it's OR or XOR. let mut final_input: u16 = ((partial_byte as u16) & ((1 << num_partial_bits) - 1)) | (0x0F << num_partial_bits); let mut final_bits = num_partial_bits + 4; diff --git a/crypto/sha3/tests/sha3_tests.rs b/crypto/sha3/tests/sha3_tests.rs index 5fb5cae..4b834e4 100644 --- a/crypto/sha3/tests/sha3_tests.rs +++ b/crypto/sha3/tests/sha3_tests.rs @@ -353,7 +353,7 @@ mod sha3_tests { Err(_) => { /* good */ } } - // This works because we allow hazardous conversions before doing the conversion. + // This works because hazardous conversions are allowed before doing the conversion. let input_seed = KeyMaterial256::from_bytes(&DUMMY_SEED_512[..32]).expect("Error happened"); let mut output_seed = SHA3_256::new() .derive_key(&input_seed, b"some addtional input to the KDF") @@ -364,7 +364,7 @@ mod sha3_tests { // will automatically drop allow_hazardous_conversions() after performing one. assert_eq!(output_seed.key_type(), KeyType::MACKey); - // This works because we explicitly tag the input data as BytesFullEntropy. + // This works because the input data is tagged explicitly as BytesFullEntropy. // This is the preferred and better way to do it. let input_seed = KeyMaterial256::from_bytes_as_type(&DUMMY_SEED_512[..32], KeyType::BytesFullEntropy) diff --git a/crypto/sha3/tests/shake_tests.rs b/crypto/sha3/tests/shake_tests.rs index e87ea0d..280d1e0 100644 --- a/crypto/sha3/tests/shake_tests.rs +++ b/crypto/sha3/tests/shake_tests.rs @@ -13,8 +13,8 @@ mod shake_tests { #[test] fn test_xof_partial_bit_output() { - // we know that the 4th ([3]) byte of the output of SHA128(\x00\x01\x02\x03\x04) is 0xFF - // So we'll play with that to test partial byte output. + // The 4th ([3]) byte of the output of SHA128(\x00\x01\x02\x03\x04) is known to be 0xFF + // That fact is used to test partial byte output. let output = SHAKE128::new().hash_xof(&[0u8, 1u8, 2u8, 3u8, 4u8], 4); assert_eq!(output[3], 0xFF); diff --git a/crypto/utils/src/ct.rs b/crypto/utils/src/ct.rs index 96b1950..a3f93e2 100644 --- a/crypto/utils/src/ct.rs +++ b/crypto/utils/src/ct.rs @@ -75,7 +75,7 @@ impl Condition { Self::is_negative(x - y) } - // Note: haven't found a clever way to make this const, since it either needs a (non-const) not (!) or a boolean OR is_zero. + // Note: this cannot currently be marked as const, since it either needs a (non-const) not (!) or a boolean OR is_zero. pub fn is_lte(x: i64, y: i64) -> Self { !Self::is_gt(x, y) } @@ -84,7 +84,7 @@ impl Condition { Self::is_lt(y, x) } - // Note: haven't found a clever way to make this const, since it either needs a (non-const) not (!) or a boolean OR is_zero. + // Note: this cannot currently be marked as const, since it either needs a (non-const) not (!) or a boolean OR is_zero. pub fn is_gte(x: i64, y: i64) -> Self { !Self::is_lt(x, y) } @@ -262,7 +262,7 @@ where } /// Rust doesn't guarantee that anything can truly be constant-time under all compilation targets -/// and optimization levels, but we'll try. +/// and optimization levels. The following presents the standard constant-time shape. pub fn ct_eq_bytes(a: &[u8], b: &[u8]) -> bool { if a.len() != b.len() { return false; @@ -275,7 +275,7 @@ pub fn ct_eq_bytes(a: &[u8], b: &[u8]) -> bool { } /// Rust doesn't guarantee that anything can truly be constant-time under all compilation targets -/// and optimization levels, but we'll try. +/// and optimization levels. The following presents the standard constant-time shape. pub fn ct_eq_zero_bytes(a: &[u8]) -> bool { let mut result = 0u8; for i in 0..a.len() { diff --git a/crypto/utils/src/lib.rs b/crypto/utils/src/lib.rs index 67644be..3f6a385 100644 --- a/crypto/utils/src/lib.rs +++ b/crypto/utils/src/lib.rs @@ -2,12 +2,12 @@ pub mod ct; -/// Basic max function. If they are equal, you get back the first one. +/// Basic max function. If they are equal, it returns the first one. pub fn max<'a, T: PartialOrd>(x: &'a T, y: &'a T) -> &'a T { if x >= y { x } else { y } } -/// Basic min function. If they are equal, you get back the first one. +/// Basic min function. If they are equal, it returns the first one. pub fn min<'a, T: PartialOrd>(x: &'a T, y: &'a T) -> &'a T { if x <= y { x } else { y } } diff --git a/src/bench_mldsa_mem_usage.rs b/src/bench_mldsa_mem_usage.rs index 55bdc24..2717da4 100644 --- a/src/bench_mldsa_mem_usage.rs +++ b/src/bench_mldsa_mem_usage.rs @@ -5,15 +5,16 @@ //! //! > ms_print massif.out.835000 //! -//! or, shoved all into one line: +//! alternatively, as a one line command: //! //! > clear; clear; valgrind --tool=massif --heap=no --stacks=yes -- target/release/bench_mldsa_mem_usage > /dev/null; ms_print massif.out.*; rm massif.out.* //! //! Make sure you build in release mode! //! -//! Note: I'm using print!() to force the compiler not to optimize away the actual code. -//! I'm printing the important stuff for benchmarking to stderr so that I can pipe the junk to /dev/null -//! (I'm not doing it the other way because /usr/bin/time prints its useful stuff to stderr as well) +//! Note: +//! The code is using print!() to force the compiler not to optimize away the actual code. +//! It is printing important outputs for benchmarking to stderr so that the rest can be mapped to /dev/null +//! (this is because /usr/bin/time prints useful outputs to stderr as well) //! //! Main is at the bottom, controls which this was actually run. @@ -25,7 +26,7 @@ use bouncycastle_core_interface::traits::{Signature, SignaturePublicKey}; use bouncycastle_hex as hex; use bouncycastle_mldsa::MLDSA44PublicKey; -/// This exists so I can use /usr/bin/time to measure the base memory footprint of the cargo bench harness +/// This exists so that /usr/bin/time can be used to measure the base memory footprint of the cargo bench harness fn bench_do_nothing() { eprintln!("DoNothing");