diff --git a/Cargo.lock b/Cargo.lock index f5d0c6a1a4..81a6cd1531 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -892,6 +892,12 @@ dependencies = [ "cc", ] +[[package]] +name = "cmov" +version = "0.4.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c11ed919bd3bae4af5ab56372b627dfc32622aba6cec36906e8ab46746037c9d" + [[package]] name = "colorchoice" version = "1.0.4" @@ -1177,6 +1183,7 @@ dependencies = [ "commonware-parallel", "commonware-utils", "criterion", + "ctutils", "ecdsa", "ed25519-consensus", "getrandom 0.2.16", @@ -1899,6 +1906,15 @@ dependencies = [ "typenum", ] +[[package]] +name = "ctutils" +version = "0.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7c67c81499f542d1dd38c6a2a2fe825f4dd4bca5162965dd2eea0c8119873d3c" +dependencies = [ + "cmov", +] + [[package]] name = "curve25519-dalek" version = "4.1.3" diff --git a/Cargo.toml b/Cargo.toml index a88248440e..b2d7d9eb6e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -101,6 +101,7 @@ console-subscriber = "0.5.0" crc32fast = "1.5.0" criterion = "0.7.0" crossterm = "0.29.0" +ctutils = "0.3.1" ecdsa = { version = "0.16.9", default-features = false } ed25519-consensus = { version = "2.1.0", default-features = false } ed25519-zebra = "4.1.0" diff --git a/cryptography/Cargo.toml b/cryptography/Cargo.toml index 515929b01c..62896efd38 100644 --- a/cryptography/Cargo.toml +++ b/cryptography/Cargo.toml @@ -25,6 +25,7 @@ commonware-codec.workspace = true commonware-math.workspace = true commonware-parallel.workspace = true commonware-utils.workspace = true +ctutils.workspace = true ecdsa.workspace = true ed25519-consensus = { workspace = true, default-features = false } p256 = { workspace = true, features = ["ecdsa"] } diff --git a/cryptography/fuzz/fuzz_targets/bls12381_primitive_operations.rs b/cryptography/fuzz/fuzz_targets/bls12381_primitive_operations.rs index aaa653cb5f..42f48de2a3 100644 --- a/cryptography/fuzz/fuzz_targets/bls12381_primitive_operations.rs +++ b/cryptography/fuzz/fuzz_targets/bls12381_primitive_operations.rs @@ -348,10 +348,7 @@ fn arbitrary_g2(u: &mut Unstructured) -> Result { } fn arbitrary_share(u: &mut Unstructured) -> Result { - Ok(Share { - index: u.int_in_range(1..=100)?, - private: arbitrary_scalar(u)?, - }) + Ok(Share::new(u.int_in_range(1..=100)?, arbitrary_scalar(u)?)) } fn arbitrary_poly_scalar(u: &mut Unstructured) -> Result, arbitrary::Error> { diff --git a/cryptography/fuzz/fuzz_targets/common/mod.rs b/cryptography/fuzz/fuzz_targets/common/mod.rs index 0f0b89360e..d958ff406d 100644 --- a/cryptography/fuzz/fuzz_targets/common/mod.rs +++ b/cryptography/fuzz/fuzz_targets/common/mod.rs @@ -101,10 +101,7 @@ pub fn arbitrary_scalar(u: &mut Unstructured) -> Result Result { - Ok(Share { - index: u.int_in_range(1..=100)?, - private: arbitrary_scalar(u)?, - }) + Ok(Share::new(u.int_in_range(1..=100)?, arbitrary_scalar(u)?)) } #[allow(unused)] diff --git a/cryptography/fuzz/fuzz_targets/secp256r1_recoverable_decode.rs b/cryptography/fuzz/fuzz_targets/secp256r1_recoverable_decode.rs index c652cbad0d..7d7e654cb6 100644 --- a/cryptography/fuzz/fuzz_targets/secp256r1_recoverable_decode.rs +++ b/cryptography/fuzz/fuzz_targets/secp256r1_recoverable_decode.rs @@ -53,7 +53,7 @@ fn test_private_key(data: &[u8]) { if let (Ok(ref_key), Ok(our_key)) = (ref_result, our_result) { assert_eq!( ref_key.to_bytes().as_slice(), - our_key.as_ref(), + our_key.encode().as_ref(), "32-byte input: keys don't match" ); } diff --git a/cryptography/fuzz/fuzz_targets/secp256r1_standard_decode.rs b/cryptography/fuzz/fuzz_targets/secp256r1_standard_decode.rs index 8e91a70014..cdc7f83bcd 100644 --- a/cryptography/fuzz/fuzz_targets/secp256r1_standard_decode.rs +++ b/cryptography/fuzz/fuzz_targets/secp256r1_standard_decode.rs @@ -53,7 +53,7 @@ fn test_private_key(data: &[u8]) { if let (Ok(ref_key), Ok(our_key)) = (ref_result, our_result) { assert_eq!( ref_key.to_bytes().as_slice(), - our_key.as_ref(), + our_key.encode().as_ref(), "32-byte input: keys don't match" ); } diff --git a/cryptography/src/bls12381/dkg.rs b/cryptography/src/bls12381/dkg.rs index 4829ac0e5b..5d61e811be 100644 --- a/cryptography/src/bls12381/dkg.rs +++ b/cryptography/src/bls12381/dkg.rs @@ -290,7 +290,7 @@ use crate::{ variant::Variant, }, transcript::{Summary, Transcript}, - PublicKey, Signer, + PublicKey, Secret, Signer, }; use commonware_codec::{Encode, EncodeSize, RangeCfg, Read, ReadExt, Write}; use commonware_math::{ @@ -566,7 +566,9 @@ impl Info { return false; }; let expected = pub_msg.commitment.eval_msm(&scalar, &Sequential); - expected == V::Public::generator() * &priv_msg.share + priv_msg + .share + .expose(|share| expected == V::Public::generator() * share) } } @@ -680,26 +682,29 @@ where } } -#[derive(Clone, PartialEq, Eq)] +#[derive(Clone, Debug, PartialEq, Eq)] pub struct DealerPrivMsg { - share: Scalar, + share: Secret, } -impl std::fmt::Debug for DealerPrivMsg { - fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { - write!(f, "DealerPrivMsg(REDACTED)") +impl DealerPrivMsg { + /// Creates a new `DealerPrivMsg` with the given share. + pub const fn new(share: Scalar) -> Self { + Self { + share: Secret::new(share), + } } } impl EncodeSize for DealerPrivMsg { fn encode_size(&self) -> usize { - self.share.encode_size() + self.share.expose(|share| share.encode_size()) } } impl Write for DealerPrivMsg { fn write(&self, buf: &mut impl bytes::BufMut) { - self.share.write(buf); + self.share.expose(|share| share.write(buf)); } } @@ -710,17 +715,14 @@ impl Read for DealerPrivMsg { buf: &mut impl bytes::Buf, _cfg: &Self::Cfg, ) -> Result { - Ok(Self { - share: ReadExt::read(buf)?, - }) + Ok(Self::new(ReadExt::read(buf)?)) } } #[cfg(feature = "arbitrary")] impl arbitrary::Arbitrary<'_> for DealerPrivMsg { fn arbitrary(u: &mut arbitrary::Unstructured<'_>) -> arbitrary::Result { - let share = u.arbitrary()?; - Ok(Self { share }) + Ok(Self::new(u.arbitrary()?)) } } @@ -1173,7 +1175,14 @@ impl Dealer { ) -> Result<(Self, DealerPubMsg, Vec<(S::PublicKey, DealerPrivMsg)>), Error> { // Check that this dealer is defined in the round. info.dealer_index(&me.public_key())?; - let share = info.unwrap_or_random_share(&mut rng, share.map(|x| x.private))?; + let share = info.unwrap_or_random_share( + &mut rng, + // We are extracting the private scalar from `Secret` protection because + // `Poly::new_with_constant` requires an owned value. The extracted scalar is + // scoped to this function and will be zeroized on drop (i.e. the secret is + // only exposed for the duration of this function). + share.map(|x| x.private.expose_unwrap()), + )?; let my_poly = Poly::new_with_constant(&mut rng, info.degree(), share); let priv_msgs = info .players @@ -1181,12 +1190,10 @@ impl Dealer { .map(|pk| { ( pk.clone(), - DealerPrivMsg { - share: my_poly.eval_msm( - &info.player_scalar(pk).expect("player should exist"), - &Sequential, - ), - }, + DealerPrivMsg::new(my_poly.eval_msm( + &info.player_scalar(pk).expect("player should exist"), + &Sequential, + )), ) }) .collect::>(); @@ -1474,13 +1481,18 @@ impl Player { strategy: &Y, ) -> Result<(Output, Share), Error> { let selected = select(&self.info, logs)?; + // We are extracting the private scalars from `Secret` protection + // because interpolation/summation needs owned scalars for polynomial + // arithmetic. The extracted scalars are scoped to this function and + // will be zeroized on drop (i.e. the secrets are only exposed for the + // duration of this function). let dealings = selected .iter_pairs() .map(|(dealer, log)| { let share = self .view .get(dealer) - .map(|(_, priv_msg)| priv_msg.share.clone()) + .map(|(_, priv_msg)| priv_msg.share.clone().expose_unwrap()) .unwrap_or_else(|| { log.get_reveal(&self.me_pub).map_or_else( || { @@ -1488,7 +1500,7 @@ impl Player { "select didn't check dealer reveal, or we're not a player?" ) }, - |priv_msg| priv_msg.share.clone(), + |priv_msg| priv_msg.share.clone().expose_unwrap(), ) }); (dealer.clone(), share) @@ -1511,10 +1523,7 @@ impl Player { .expect("select ensures that we can recover") }, ); - let share = Share { - index: self.index, - private, - }; + let share = Share::new(self.index, private); Ok((output, share)) } } @@ -1543,10 +1552,7 @@ pub fn deal( &mode.scalar(n, i).expect("player index should be valid"), &Sequential, ); - let share = Share { - index: i, - private: eval, - }; + let share = Share::new(i, eval); (p.clone(), share) }) .try_collect() @@ -1929,69 +1935,66 @@ mod test_plan { let share = match (shares.get(&pk), round.replace_shares.contains(&i_dealer)) { (None, _) => None, (Some(s), false) => Some(s.clone()), - (Some(_), true) => Some(Share { - index: i_dealer, - private: Scalar::random(&mut rng), - }), + (Some(_), true) => Some(Share::new(i_dealer, Scalar::random(&mut rng))), }; // Start dealer (with potential modifications) - let (mut dealer, pub_msg, mut priv_msgs) = if let Some(shift) = - round.shift_degrees.get(&i_dealer) - { - // Create dealer with shifted degree - let degree = - u32::try_from(info.degree() as i32 + shift.get()).unwrap_or_default(); - - // Manually create the dealer with adjusted polynomial - let share = info - .unwrap_or_random_share(&mut rng, share.map(|s| s.private)) - .expect("Failed to generate dealer share"); - - let my_poly = Poly::new_with_constant(&mut rng, degree, share); - let priv_msgs = info - .players - .iter() - .map(|pk| { - ( - pk.clone(), - DealerPrivMsg { - share: my_poly.eval_msm( + let (mut dealer, pub_msg, mut priv_msgs) = + if let Some(shift) = round.shift_degrees.get(&i_dealer) { + // Create dealer with shifted degree + let degree = u32::try_from(info.degree() as i32 + shift.get()) + .unwrap_or_default(); + + // Manually create the dealer with adjusted polynomial + let share = info + .unwrap_or_random_share( + &mut rng, + share.map(|s| s.private.expose_unwrap()), + ) + .expect("Failed to generate dealer share"); + + let my_poly = Poly::new_with_constant(&mut rng, degree, share); + let priv_msgs = info + .players + .iter() + .map(|pk| { + ( + pk.clone(), + DealerPrivMsg::new(my_poly.eval_msm( &info.player_scalar(pk).expect("player should exist"), &Sequential, - ), - }, - ) - }) - .collect::>(); - let results: Map<_, _> = priv_msgs - .iter() - .map(|(pk, pm)| (pk.clone(), AckOrReveal::Reveal(pm.clone()))) - .try_collect() - .unwrap(); - let commitment = Poly::commit(my_poly); - let pub_msg = DealerPubMsg { commitment }; - let transcript = { - let t = transcript_for_round(&info); - transcript_for_ack(&t, &pk, &pub_msg) - }; - let dealer = Dealer { - me: sk.clone(), - info: info.clone(), - pub_msg: pub_msg.clone(), - results, - transcript, + )), + ) + }) + .collect::>(); + let results: Map<_, _> = priv_msgs + .iter() + .map(|(pk, pm)| (pk.clone(), AckOrReveal::Reveal(pm.clone()))) + .try_collect() + .unwrap(); + let commitment = Poly::commit(my_poly); + let pub_msg = DealerPubMsg { commitment }; + let transcript = { + let t = transcript_for_round(&info); + transcript_for_ack(&t, &pk, &pub_msg) + }; + let dealer = Dealer { + me: sk.clone(), + info: info.clone(), + pub_msg: pub_msg.clone(), + results, + transcript, + }; + (dealer, pub_msg, priv_msgs) + } else { + Dealer::start(&mut rng, info.clone(), sk.clone(), share)? }; - (dealer, pub_msg, priv_msgs) - } else { - Dealer::start(&mut rng, info.clone(), sk.clone(), share)? - }; // Apply BadShare perturbations for (player, priv_msg) in &mut priv_msgs { let player_key_idx = pk_to_key_idx[player]; if round.bad_shares.contains(&(i_dealer, player_key_idx)) { - priv_msg.share = Scalar::random(&mut rng); + *priv_msg = DealerPrivMsg::new(Scalar::random(&mut rng)); } } assert_eq!(priv_msgs.len(), players.len()); @@ -2068,9 +2071,9 @@ mod test_plan { *results .get_value_mut(&player_pk) .ok_or_else(|| anyhow!("unknown player: {:?}", &player_pk))? = - AckOrReveal::Reveal(DealerPrivMsg { - share: Scalar::random(&mut rng), - }); + AckOrReveal::Reveal(DealerPrivMsg::new(Scalar::random( + &mut rng, + ))); } } } @@ -2398,6 +2401,8 @@ mod test { use super::{test_plan::*, *}; use crate::{bls12381::primitives::variant::MinPk, ed25519}; use anyhow::anyhow; + use commonware_math::algebra::Random; + use commonware_utils::test_rng; use core::num::NonZeroI32; use rand::SeedableRng; use rand_chacha::ChaCha8Rng; @@ -2573,6 +2578,14 @@ mod test { Ok(()) } + #[test] + fn test_dealer_priv_msg_redacted() { + let mut rng = test_rng(); + let msg = DealerPrivMsg::new(Scalar::random(&mut rng)); + let debug = format!("{:?}", msg); + assert!(debug.contains("REDACTED")); + } + #[cfg(feature = "arbitrary")] mod conformance { use super::*; diff --git a/cryptography/src/bls12381/primitives/group.rs b/cryptography/src/bls12381/primitives/group.rs index cfa6805a4b..c9dfbaeea5 100644 --- a/cryptography/src/bls12381/primitives/group.rs +++ b/cryptography/src/bls12381/primitives/group.rs @@ -11,6 +11,7 @@ //! is already taken care of for you if you use the provided `deserialize` function. use super::variant::Variant; +use crate::Secret; #[cfg(not(feature = "std"))] use alloc::{vec, vec::Vec}; use blst::{ @@ -45,6 +46,7 @@ use core::{ ops::{Add, AddAssign, Mul, MulAssign, Neg, Sub, SubAssign}, ptr, }; +use ctutils::CtEq; use rand_core::CryptoRngCore; use zeroize::{Zeroize, ZeroizeOnDrop}; @@ -214,23 +216,6 @@ pub type Private = Scalar; pub const PRIVATE_KEY_LENGTH: usize = SCALAR_LENGTH; impl Scalar { - /// Generate a non-zero scalar from the randomly populated buffer. - fn from_bytes(mut ikm: [u8; 64]) -> Self { - let mut sc = blst_scalar::default(); - let mut ret = blst_fr::default(); - // SAFETY: ikm is a valid 64-byte buffer; blst_keygen handles null key_info. - unsafe { - // blst_keygen loops until a non-zero value is produced (in accordance with IETF BLS KeyGen 4+). - blst_keygen(&mut sc, ikm.as_ptr(), ikm.len(), ptr::null(), 0); - blst_fr_from_scalar(&mut ret, &sc); - } - - // Zeroize the ikm buffer - ikm.zeroize(); - - Self(ret) - } - /// Maps arbitrary bytes to a scalar using RFC9380 hash-to-field. pub fn map(dst: DST, msg: &[u8]) -> Self { // The BLS12-381 scalar field has a modulus of approximately 255 bits. @@ -285,7 +270,7 @@ impl Scalar { Self(ret) } - /// Encodes the scalar into a slice. + /// Encodes the scalar into a byte array. fn as_slice(&self) -> [u8; Self::SIZE] { let mut slice = [0u8; Self::SIZE]; // SAFETY: All pointers valid; blst_bendian_from_scalar writes exactly 32 bytes. @@ -349,6 +334,12 @@ impl Hash for Scalar { } } +impl CtEq for Scalar { + fn ct_eq(&self, other: &Self) -> ctutils::Choice { + self.0.l.ct_eq(&other.0.l) + } +} + impl PartialOrd for Scalar { fn partial_cmp(&self, other: &Self) -> Option { Some(self.cmp(other)) @@ -488,39 +479,54 @@ impl Random for Scalar { fn random(mut rng: impl CryptoRngCore) -> Self { let mut ikm = [0u8; 64]; rng.fill_bytes(&mut ikm); - Self::from_bytes(ikm) + + let mut sc = blst_scalar::default(); + let mut ret = blst_fr::default(); + // SAFETY: ikm is a valid 64-byte buffer; blst_keygen handles null key_info. + unsafe { + // blst_keygen loops until a non-zero value is produced (in accordance with IETF BLS KeyGen 4+). + blst_keygen(&mut sc, ikm.as_ptr(), ikm.len(), ptr::null(), 0); + blst_fr_from_scalar(&mut ret, &sc); + } + + // Zeroize the ikm buffer + ikm.zeroize(); + + Self(ret) } } /// A share of a threshold signing key. -#[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] -#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))] +#[derive(Clone, Debug, PartialEq, Eq)] pub struct Share { /// The share's index in the polynomial. pub index: u32, /// The scalar corresponding to the share's secret. - pub private: Private, + pub private: Secret, } -impl AsRef for Share { - fn as_ref(&self) -> &Private { - &self.private +impl Share { + /// Creates a new `Share` with the given index and private key. + pub const fn new(index: u32, private: Private) -> Self { + Self { + index, + private: Secret::new(private), + } } -} -impl Share { /// Returns the public key corresponding to the share. /// /// This can be verified against the public polynomial. pub fn public(&self) -> V::Public { - V::Public::generator() * &self.private + self.private + .expose(|private| V::Public::generator() * private) } } impl Write for Share { fn write(&self, buf: &mut impl BufMut) { UInt(self.index).write(buf); - self.private.write(buf); + self.private.expose(|private| private.write(buf)); } } @@ -530,25 +536,34 @@ impl Read for Share { fn read_cfg(buf: &mut impl Buf, _: &()) -> Result { let index = UInt::read(buf)?.into(); let private = Private::read(buf)?; - Ok(Self { index, private }) + Ok(Self { + index, + private: Secret::new(private), + }) } } impl EncodeSize for Share { fn encode_size(&self) -> usize { - UInt(self.index).encode_size() + self.private.encode_size() + UInt(self.index).encode_size() + self.private.expose(|private| private.encode_size()) } } impl Display for Share { fn fmt(&self, f: &mut Formatter<'_>) -> core::fmt::Result { - write!(f, "Share(index={}, private={})", self.index, self.private) + write!(f, "{:?}", self) } } -impl Debug for Share { - fn fmt(&self, f: &mut Formatter<'_>) -> core::fmt::Result { - write!(f, "Share(index={}, private={})", self.index, self.private) +#[cfg(feature = "arbitrary")] +impl arbitrary::Arbitrary<'_> for Share { + fn arbitrary(u: &mut arbitrary::Unstructured<'_>) -> arbitrary::Result { + let index = u.arbitrary()?; + let private = Private::arbitrary(u)?; + Ok(Self { + index, + private: Secret::new(private), + }) } } @@ -1126,11 +1141,13 @@ impl HashToGroup for G2 { #[cfg(test)] mod tests { use super::*; + use crate::bls12381::primitives::group::Scalar; use commonware_codec::{DecodeExt, Encode}; - use commonware_math::algebra::test_suites; + use commonware_math::algebra::{test_suites, Random}; use commonware_parallel::Sequential; use commonware_utils::test_rng; use proptest::{prelude::*, strategy::Strategy}; + use rand::{rngs::StdRng, SeedableRng}; use std::collections::{BTreeSet, HashMap}; impl Arbitrary for Scalar { @@ -1138,7 +1155,9 @@ mod tests { type Strategy = BoxedStrategy; fn arbitrary_with(_args: Self::Parameters) -> Self::Strategy { - any::<[u8; 64]>().prop_map(Self::from_bytes).boxed() + any::<[u8; 32]>() + .prop_map(|seed| Self::random(&mut StdRng::from_seed(seed))) + .boxed() } } @@ -1445,27 +1464,20 @@ mod tests { let mut scalar_set = BTreeSet::new(); let mut g1_set = BTreeSet::new(); let mut g2_set = BTreeSet::new(); - let mut share_set = BTreeSet::new(); while scalar_set.len() < NUM_ITEMS { let scalar = Scalar::random(&mut rng); let g1 = G1::generator() * &scalar; let g2 = G2::generator() * &scalar; - let share = Share { - index: scalar_set.len() as u32, - private: scalar.clone(), - }; scalar_set.insert(scalar); g1_set.insert(g1); g2_set.insert(g2); - share_set.insert(share); } // Verify that the sets contain the expected number of unique items. assert_eq!(scalar_set.len(), NUM_ITEMS); assert_eq!(g1_set.len(), NUM_ITEMS); assert_eq!(g2_set.len(), NUM_ITEMS); - assert_eq!(share_set.len(), NUM_ITEMS); // Verify that `BTreeSet` iteration is sorted, which relies on `Ord`. let scalars: Vec<_> = scalar_set.iter().collect(); @@ -1474,20 +1486,16 @@ mod tests { assert!(g1s.windows(2).all(|w| w[0] <= w[1])); let g2s: Vec<_> = g2_set.iter().collect(); assert!(g2s.windows(2).all(|w| w[0] <= w[1])); - let shares: Vec<_> = share_set.iter().collect(); - assert!(shares.windows(2).all(|w| w[0] <= w[1])); // Test that we can use these types as keys in hash maps, which relies on `Hash` and `Eq`. let scalar_map: HashMap<_, _> = scalar_set.iter().cloned().zip(0..).collect(); let g1_map: HashMap<_, _> = g1_set.iter().cloned().zip(0..).collect(); let g2_map: HashMap<_, _> = g2_set.iter().cloned().zip(0..).collect(); - let share_map: HashMap<_, _> = share_set.iter().cloned().zip(0..).collect(); // Verify that the maps contain the expected number of unique items. assert_eq!(scalar_map.len(), NUM_ITEMS); assert_eq!(g1_map.len(), NUM_ITEMS); assert_eq!(g2_map.len(), NUM_ITEMS); - assert_eq!(share_map.len(), NUM_ITEMS); } #[test] @@ -1541,6 +1549,33 @@ mod tests { ); } + #[test] + fn test_secret_scalar_equality() { + let mut rng = test_rng(); + let scalar1 = Scalar::random(&mut rng); + let scalar2 = scalar1.clone(); + let scalar3 = Scalar::random(&mut rng); + + let s1 = Secret::new(scalar1); + let s2 = Secret::new(scalar2); + let s3 = Secret::new(scalar3); + + // Same scalar should be equal + assert_eq!(s1, s2); + // Different scalars should (very likely) be different + assert_ne!(s1, s3); + } + + #[test] + fn test_share_redacted() { + let mut rng = test_rng(); + let share = Share::new(1, Scalar::random(&mut rng)); + let debug = format!("{:?}", share); + let display = format!("{}", share); + assert!(debug.contains("REDACTED")); + assert!(display.contains("REDACTED")); + } + #[cfg(feature = "arbitrary")] mod conformance { use super::*; diff --git a/cryptography/src/bls12381/primitives/ops/threshold.rs b/cryptography/src/bls12381/primitives/ops/threshold.rs index 49cb3e9b51..a6193c951f 100644 --- a/cryptography/src/bls12381/primitives/ops/threshold.rs +++ b/cryptography/src/bls12381/primitives/ops/threshold.rs @@ -44,7 +44,10 @@ pub fn sign_message( namespace: &[u8], message: &[u8], ) -> PartialSignature { - let sig = super::sign_message::(&private.private, namespace, message); + let sig = private + .private + .expose(|private| super::sign_message::(private, namespace, message)); + PartialSignature { value: sig, index: private.index, @@ -61,11 +64,14 @@ pub fn sign_proof_of_possession( private: &Share, namespace: &[u8], ) -> PartialSignature { - let sig = super::sign::( - &private.private, - V::PROOF_OF_POSSESSION, - &union_unique(namespace, &sharing.public().encode()), - ); + let sig = private.private.expose(|private| { + super::sign::( + private, + V::PROOF_OF_POSSESSION, + &union_unique(namespace, &sharing.public().encode()), + ) + }); + PartialSignature { value: sig, index: private.index, @@ -331,13 +337,16 @@ where #[cfg(test)] mod tests { use super::*; - use crate::bls12381::{ - dkg, - primitives::{ - group::{Private, Scalar, G1_MESSAGE, G2_MESSAGE}, - ops::{self, hash_with_namespace}, - variant::{MinPk, MinSig}, + use crate::{ + bls12381::{ + dkg, + primitives::{ + group::{Private, Scalar, G1_MESSAGE, G2_MESSAGE}, + ops::{self, hash_with_namespace}, + variant::{MinPk, MinSig}, + }, }, + Secret, }; use blst::BLST_ERROR; use commonware_codec::Encode; @@ -720,7 +729,7 @@ mod tests { dkg::deal_anonymous::(&mut rng, Default::default(), NZU32!(n)); let share = shares.get_mut(3).unwrap(); - share.private = Private::random(&mut rng); + share.private = Secret::new(Private::random(&mut rng)); let namespace = b"test"; let msg = b"hello"; @@ -773,7 +782,7 @@ mod tests { let msg = b"hello"; let corrupted_index = 1; - shares[corrupted_index].private = Private::random(&mut rng); + shares[corrupted_index].private = Secret::new(Private::random(&mut rng)); let partials: Vec<_> = shares .iter() @@ -811,7 +820,7 @@ mod tests { let corrupted_indices = vec![1, 3]; for &idx in &corrupted_indices { - shares[idx].private = Private::random(&mut rng); + shares[idx].private = Secret::new(Private::random(&mut rng)); } let partials: Vec<_> = shares @@ -903,7 +912,7 @@ mod tests { let namespace = b"test"; let msg = b"hello"; - shares[0].private = Private::random(&mut rng); + shares[0].private = Secret::new(Private::random(&mut rng)); let partials: Vec<_> = shares .iter() @@ -932,7 +941,7 @@ mod tests { let msg = b"hello"; let corrupted_index = n - 1; - shares[corrupted_index as usize].private = Private::random(&mut rng); + shares[corrupted_index as usize].private = Secret::new(Private::random(&mut rng)); let partials: Vec<_> = shares .iter() diff --git a/cryptography/src/bls12381/scheme.rs b/cryptography/src/bls12381/scheme.rs index a772b53517..20b75e8c18 100644 --- a/cryptography/src/bls12381/scheme.rs +++ b/cryptography/src/bls12381/scheme.rs @@ -30,7 +30,7 @@ use super::primitives::{ ops, variant::{MinPk, Variant}, }; -use crate::{Array, BatchVerifier, Signer as _}; +use crate::{BatchVerifier, Secret, Signer as _}; #[cfg(not(feature = "std"))] use alloc::vec::Vec; use bytes::{Buf, BufMut}; @@ -39,27 +39,34 @@ use commonware_codec::{ }; use commonware_math::algebra::Random; use commonware_parallel::Sequential; -use commonware_utils::{hex, Span}; +use commonware_utils::{hex, Array, Span}; use core::{ fmt::{Debug, Display, Formatter}, hash::{Hash, Hasher}, ops::Deref, }; use rand_core::CryptoRngCore; -use zeroize::{Zeroize, ZeroizeOnDrop}; const CURVE_NAME: &str = "bls12381"; /// BLS12-381 private key. -#[derive(Clone, Eq, PartialEq, Zeroize, ZeroizeOnDrop)] +#[derive(Clone, Debug)] pub struct PrivateKey { - raw: [u8; group::PRIVATE_KEY_LENGTH], - key: group::Private, + raw: Secret<[u8; group::PRIVATE_KEY_LENGTH]>, + key: Secret, } +impl PartialEq for PrivateKey { + fn eq(&self, other: &Self) -> bool { + self.raw == other.raw + } +} + +impl Eq for PrivateKey {} + impl Write for PrivateKey { fn write(&self, buf: &mut impl BufMut) { - self.raw.write(buf); + self.raw.expose(|raw| raw.write(buf)); } } @@ -70,7 +77,10 @@ impl Read for PrivateKey { let raw = <[u8; Self::SIZE]>::read(buf)?; let key = group::Private::decode(raw.as_ref()) .map_err(|e| CodecError::Wrapped(CURVE_NAME, e.into()))?; - Ok(Self { raw, key }) + Ok(Self { + raw: Secret::new(raw), + key: Secret::new(key), + }) } } @@ -78,57 +88,19 @@ impl FixedSize for PrivateKey { const SIZE: usize = group::PRIVATE_KEY_LENGTH; } -impl Span for PrivateKey {} - -impl Array for PrivateKey {} - -impl Hash for PrivateKey { - fn hash(&self, state: &mut H) { - self.raw.hash(state); - } -} - -impl Ord for PrivateKey { - fn cmp(&self, other: &Self) -> core::cmp::Ordering { - self.raw.cmp(&other.raw) - } -} - -impl PartialOrd for PrivateKey { - fn partial_cmp(&self, other: &Self) -> Option { - Some(self.cmp(other)) - } -} - -impl AsRef<[u8]> for PrivateKey { - fn as_ref(&self) -> &[u8] { - &self.raw - } -} - -impl Deref for PrivateKey { - type Target = [u8]; - fn deref(&self) -> &[u8] { - &self.raw - } -} - impl From for PrivateKey { fn from(key: Scalar) -> Self { let raw = key.encode_fixed(); - Self { raw, key } - } -} - -impl Debug for PrivateKey { - fn fmt(&self, f: &mut Formatter<'_>) -> core::fmt::Result { - write!(f, "{}", hex(&self.raw)) + Self { + raw: Secret::new(raw), + key: Secret::new(key), + } } } impl Display for PrivateKey { fn fmt(&self, f: &mut Formatter<'_>) -> core::fmt::Result { - write!(f, "{}", hex(&self.raw)) + write!(f, "{:?}", self) } } @@ -139,11 +111,13 @@ impl crate::Signer for PrivateKey { type PublicKey = PublicKey; fn public_key(&self) -> Self::PublicKey { - PublicKey::from(ops::compute_public::(&self.key)) + self.key + .expose(|key| PublicKey::from(ops::compute_public::(key))) } fn sign(&self, namespace: &[u8], msg: &[u8]) -> Self::Signature { - ops::sign_message::(&self.key, namespace, msg).into() + self.key + .expose(|key| ops::sign_message::(key, namespace, msg).into()) } } @@ -151,7 +125,10 @@ impl Random for PrivateKey { fn random(mut rng: impl CryptoRngCore) -> Self { let (private, _) = ops::keypair::<_, MinPk>(&mut rng); let raw = private.encode_fixed(); - Self { raw, key: private } + Self { + raw: Secret::new(raw), + key: Secret::new(private), + } } } @@ -427,8 +404,10 @@ impl BatchVerifier for Batch { #[cfg(test)] mod tests { use super::*; - use crate::bls12381; + use crate::{bls12381, Verifier as _}; use commonware_codec::{DecodeExt, Encode}; + use commonware_math::algebra::Random; + use commonware_utils::test_rng; #[test] fn test_codec_private_key() { @@ -487,6 +466,27 @@ mod tests { ) } + #[test] + fn test_from_scalar() { + let mut rng = test_rng(); + let scalar = Scalar::random(&mut rng); + let private_key = PrivateKey::from(scalar); + // Verify the key works by signing and verifying + let msg = b"test message"; + let sig = private_key.sign(b"ns", msg); + assert!(private_key.public_key().verify(b"ns", msg, &sig)); + } + + #[test] + fn test_private_key_redacted() { + let mut rng = test_rng(); + let private_key = PrivateKey::random(&mut rng); + let debug = format!("{:?}", private_key); + let display = format!("{}", private_key); + assert!(debug.contains("REDACTED")); + assert!(display.contains("REDACTED")); + } + #[cfg(feature = "arbitrary")] mod conformance { use super::*; diff --git a/cryptography/src/ed25519/certificate/mocks.rs b/cryptography/src/ed25519/certificate/mocks.rs index 1a8bbef102..ce504ecbfd 100644 --- a/cryptography/src/ed25519/certificate/mocks.rs +++ b/cryptography/src/ed25519/certificate/mocks.rs @@ -7,13 +7,13 @@ use crate::{ }; use commonware_math::algebra::Random; use commonware_utils::{ - ordered::{BiMap, Set}, + ordered::{Map, Set}, TryCollect as _, }; use rand::{CryptoRng, RngCore}; /// Generates ed25519 identity participants. -pub fn participants(rng: &mut R, n: u32) -> BiMap +pub fn participants(rng: &mut R, n: u32) -> Map where R: RngCore + CryptoRng, { diff --git a/cryptography/src/ed25519/scheme.rs b/cryptography/src/ed25519/scheme.rs index d8f5e8f13b..422c806211 100644 --- a/cryptography/src/ed25519/scheme.rs +++ b/cryptography/src/ed25519/scheme.rs @@ -1,4 +1,4 @@ -use crate::{Array, BatchVerifier}; +use crate::{BatchVerifier, Secret}; #[cfg(not(feature = "std"))] use alloc::{ borrow::{Cow, ToOwned}, @@ -7,7 +7,7 @@ use alloc::{ use bytes::{Buf, BufMut}; use commonware_codec::{Error as CodecError, FixedSize, Read, ReadExt, Write}; use commonware_math::algebra::Random; -use commonware_utils::{hex, union_unique, Span}; +use commonware_utils::{hex, union_unique, Array, Span}; use core::{ fmt::{Debug, Display}, hash::{Hash, Hasher}, @@ -17,7 +17,6 @@ use ed25519_consensus::{self, VerificationKey}; use rand_core::CryptoRngCore; #[cfg(feature = "std")] use std::borrow::{Cow, ToOwned}; -use zeroize::{Zeroize, ZeroizeOnDrop}; const CURVE_NAME: &str = "ed25519"; const PRIVATE_KEY_LENGTH: usize = 32; @@ -25,10 +24,10 @@ const PUBLIC_KEY_LENGTH: usize = 32; const SIGNATURE_LENGTH: usize = 64; /// Ed25519 Private Key. -#[derive(Clone, Zeroize, ZeroizeOnDrop)] +#[derive(Clone, Debug)] pub struct PrivateKey { - raw: [u8; PRIVATE_KEY_LENGTH], - key: ed25519_consensus::SigningKey, + raw: Secret<[u8; PRIVATE_KEY_LENGTH]>, + key: Secret, } impl crate::PrivateKey for PrivateKey {} @@ -42,11 +41,13 @@ impl crate::Signer for PrivateKey { } fn public_key(&self) -> Self::PublicKey { - let raw = self.key.verification_key().to_bytes(); - Self::PublicKey { - raw, - key: self.key.verification_key().to_owned(), - } + self.key.expose(|key| { + let raw = key.verification_key().to_bytes(); + Self::PublicKey { + raw, + key: key.verification_key().to_owned(), + } + }) } } @@ -56,8 +57,7 @@ impl PrivateKey { let payload = namespace .map(|namespace| Cow::Owned(union_unique(namespace, msg))) .unwrap_or_else(|| Cow::Borrowed(msg)); - let sig = self.key.sign(&payload); - Signature::from(sig) + self.key.expose(|key| Signature::from(key.sign(&payload))) } } @@ -65,13 +65,16 @@ impl Random for PrivateKey { fn random(rng: impl CryptoRngCore) -> Self { let key = ed25519_consensus::SigningKey::new(rng); let raw = key.to_bytes(); - Self { raw, key } + Self { + raw: Secret::new(raw), + key: Secret::new(key), + } } } impl Write for PrivateKey { fn write(&self, buf: &mut impl BufMut) { - self.raw.write(buf); + self.raw.expose(|raw| raw.write(buf)); } } @@ -81,7 +84,10 @@ impl Read for PrivateKey { fn read_cfg(buf: &mut impl Buf, _: &()) -> Result { let raw = <[u8; Self::SIZE]>::read(buf)?; let key = ed25519_consensus::SigningKey::from(raw); - Ok(Self { raw, key }) + Ok(Self { + raw: Secret::new(raw), + key: Secret::new(key), + }) } } @@ -89,65 +95,27 @@ impl FixedSize for PrivateKey { const SIZE: usize = PRIVATE_KEY_LENGTH; } -impl Span for PrivateKey {} - -impl Array for PrivateKey {} - impl Eq for PrivateKey {} -impl Hash for PrivateKey { - fn hash(&self, state: &mut H) { - self.raw.hash(state); - } -} - impl PartialEq for PrivateKey { fn eq(&self, other: &Self) -> bool { self.raw == other.raw } } -impl Ord for PrivateKey { - fn cmp(&self, other: &Self) -> core::cmp::Ordering { - self.raw.cmp(&other.raw) - } -} - -impl PartialOrd for PrivateKey { - fn partial_cmp(&self, other: &Self) -> Option { - Some(self.cmp(other)) - } -} - -impl AsRef<[u8]> for PrivateKey { - fn as_ref(&self) -> &[u8] { - &self.raw - } -} - -impl Deref for PrivateKey { - type Target = [u8]; - fn deref(&self) -> &[u8] { - &self.raw - } -} - impl From for PrivateKey { fn from(key: ed25519_consensus::SigningKey) -> Self { let raw = key.to_bytes(); - Self { raw, key } - } -} - -impl Debug for PrivateKey { - fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { - write!(f, "{}", hex(&self.raw)) + Self { + raw: Secret::new(raw), + key: Secret::new(key), + } } } impl Display for PrivateKey { fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { - write!(f, "{}", hex(&self.raw)) + write!(f, "{:?}", self) } } @@ -170,11 +138,13 @@ pub struct PublicKey { impl From for PublicKey { fn from(value: PrivateKey) -> Self { - let raw = value.key.verification_key().to_bytes(); - Self { - raw, - key: value.key.verification_key(), - } + value.key.expose(|key| { + let raw = key.verification_key().to_bytes(); + Self { + raw, + key: key.verification_key(), + } + }) } } @@ -434,7 +404,7 @@ impl Batch { #[cfg(test)] mod tests { use super::*; - use crate::ed25519; + use crate::{ed25519, Signer as _}; use commonware_codec::{DecodeExt, Encode}; use commonware_math::algebra::Random; use commonware_utils::test_rng; @@ -830,6 +800,29 @@ mod tests { assert!(!public_key.verify_inner(None, &message, &bad_signature)); } + #[test] + fn test_from_signing_key() { + let signing_key = ed25519_consensus::SigningKey::new(OsRng); + let expected_public = signing_key.verification_key(); + let private_key = PrivateKey::from(signing_key); + assert_eq!(private_key.public_key().key, expected_public); + } + + #[test] + fn test_private_key_redacted() { + let private_key = PrivateKey::random(&mut OsRng); + let debug = format!("{:?}", private_key); + let display = format!("{}", private_key); + assert!(debug.contains("REDACTED")); + assert!(display.contains("REDACTED")); + } + + #[test] + fn test_from_private_key_to_public_key() { + let private_key = PrivateKey::random(&mut OsRng); + assert_eq!(private_key.public_key(), PublicKey::from(private_key)); + } + #[cfg(feature = "arbitrary")] mod conformance { use super::*; diff --git a/cryptography/src/handshake.rs b/cryptography/src/handshake.rs index fc4505672e..9e9bd26831 100644 --- a/cryptography/src/handshake.rs +++ b/cryptography/src/handshake.rs @@ -297,10 +297,12 @@ pub fn dial_end( { return Err(Error::HandshakeFailed); } - let Some(secret) = esk.exchange(&msg.epk) else { + let Some(shared) = esk.exchange(&msg.epk) else { return Err(Error::HandshakeFailed); }; - transcript.commit(secret.as_ref()); + shared + .secret + .expose(|secret| transcript.commit(secret.as_ref())); let recv = RecvCipher::new(transcript.noise(LABEL_CIPHER_L2D)); let send = SendCipher::new(transcript.noise(LABEL_CIPHER_D2L)); let confirmation_l2d = transcript.fork(LABEL_CONFIRMATION_L2D).summarize(); @@ -350,10 +352,12 @@ pub fn listen_start( .commit(current_time.encode()) .commit(epk.encode()) .sign(&my_identity); - let Some(secret) = esk.exchange(&msg.epk) else { + let Some(shared) = esk.exchange(&msg.epk) else { return Err(Error::HandshakeFailed); }; - transcript.commit(secret.as_ref()); + shared + .secret + .expose(|secret| transcript.commit(secret.as_ref())); let send = SendCipher::new(transcript.noise(LABEL_CIPHER_L2D)); let recv = RecvCipher::new(transcript.noise(LABEL_CIPHER_D2L)); let confirmation_l2d = transcript.fork(LABEL_CONFIRMATION_L2D).summarize(); diff --git a/cryptography/src/handshake/cipher.rs b/cryptography/src/handshake/cipher.rs index 5bd87db1e0..889cecb2a8 100644 --- a/cryptography/src/handshake/cipher.rs +++ b/cryptography/src/handshake/cipher.rs @@ -1,12 +1,12 @@ // Intentionally avoid depending directly on super, to depend on the sibling. use super::error::Error; +use crate::Secret; use chacha20poly1305::{ aead::{generic_array::typenum::Unsigned, Aead}, AeadCore, ChaCha20Poly1305, KeyInit as _, }; use rand_core::CryptoRngCore; use std::vec::Vec; -use zeroize::ZeroizeOnDrop; /// The amount of overhead in a ciphertext, compared to the plain message. pub const CIPHERTEXT_OVERHEAD: usize = ::TagSize::USIZE; @@ -18,9 +18,6 @@ struct CounterNonce { inner: u128, } -// We don't need to zeroize nonces. -impl ZeroizeOnDrop for CounterNonce {} - impl CounterNonce { /// Creates a new counter nonce starting at zero. pub const fn new() -> Self { @@ -39,55 +36,52 @@ impl CounterNonce { } } -#[derive(ZeroizeOnDrop)] pub struct SendCipher { nonce: CounterNonce, - inner: ChaCha20Poly1305, + inner: Secret, } impl SendCipher { /// Creates a new sending cipher with a random key. pub fn new(mut rng: impl CryptoRngCore) -> Self { - let mut key = [0u8; 32]; - rng.fill_bytes(&mut key[..]); Self { nonce: CounterNonce::new(), - inner: ChaCha20Poly1305::new(&key.into()), + inner: Secret::new(ChaCha20Poly1305::new(&ChaCha20Poly1305::generate_key( + &mut rng, + ))), } } /// Encrypts data and returns the ciphertext. pub fn send(&mut self, data: &[u8]) -> Result, Error> { + let nonce = self.nonce.inc()?; self.inner - .encrypt((&self.nonce.inc()?[..NONCE_SIZE_BYTES]).into(), data) + .expose(|cipher| cipher.encrypt((&nonce[..NONCE_SIZE_BYTES]).into(), data)) .map_err(|_| Error::EncryptionFailed) } } -#[derive(ZeroizeOnDrop)] pub struct RecvCipher { nonce: CounterNonce, - inner: ChaCha20Poly1305, + inner: Secret, } impl RecvCipher { /// Creates a new receiving cipher with a random key. pub fn new(mut rng: impl CryptoRngCore) -> Self { - let mut key = [0u8; 32]; - rng.fill_bytes(&mut key[..]); Self { nonce: CounterNonce::new(), - inner: ChaCha20Poly1305::new(&key.into()), + inner: Secret::new(ChaCha20Poly1305::new(&ChaCha20Poly1305::generate_key( + &mut rng, + ))), } } /// Decrypts ciphertext and returns the original data. pub fn recv(&mut self, encrypted_data: &[u8]) -> Result, Error> { + let nonce = self.nonce.inc()?; self.inner - .decrypt( - (&self.nonce.inc()?[..NONCE_SIZE_BYTES]).into(), - encrypted_data, - ) + .expose(|cipher| cipher.decrypt((&nonce[..NONCE_SIZE_BYTES]).into(), encrypted_data)) .map_err(|_| Error::DecryptionFailed) } } diff --git a/cryptography/src/handshake/key_exchange.rs b/cryptography/src/handshake/key_exchange.rs index bbc753bca4..12e8804090 100644 --- a/cryptography/src/handshake/key_exchange.rs +++ b/cryptography/src/handshake/key_exchange.rs @@ -1,16 +1,18 @@ +use crate::Secret; use commonware_codec::{FixedSize, Read, ReadExt, Write}; use rand_core::CryptoRngCore; -use zeroize::ZeroizeOnDrop; /// A shared secret derived from X25519 key exchange. -#[derive(ZeroizeOnDrop)] pub struct SharedSecret { - inner: x25519_dalek::SharedSecret, + pub(crate) secret: Secret, } -impl AsRef<[u8]> for SharedSecret { - fn as_ref(&self) -> &[u8] { - self.inner.as_bytes().as_slice() +impl SharedSecret { + /// Creates a new SharedSecret wrapping the given x25519 shared secret. + const fn new(secret: x25519_dalek::SharedSecret) -> Self { + Self { + secret: Secret::new(secret), + } } } @@ -55,37 +57,35 @@ impl<'a> arbitrary::Arbitrary<'a> for EphemeralPublicKey { } } -// I would implement `ZeroizeOnDrop`, but this seemingly fails because of a line -// below, where the secret must be consumed. /// An ephemeral X25519 secret key used during handshake. pub struct SecretKey { - inner: x25519_dalek::EphemeralSecret, + inner: Secret, } impl SecretKey { /// Generates a new random ephemeral secret key. pub fn new(rng: impl CryptoRngCore) -> Self { Self { - inner: x25519_dalek::EphemeralSecret::random_from_rng(rng), + inner: Secret::new(x25519_dalek::EphemeralSecret::random_from_rng(rng)), } } /// Derives the corresponding public key. pub fn public(&self) -> EphemeralPublicKey { - EphemeralPublicKey { - inner: (&self.inner).into(), - } + self.inner.expose(|secret| EphemeralPublicKey { + inner: x25519_dalek::PublicKey::from(secret), + }) } /// Performs X25519 key exchange with another public key. /// Returns None if the exchange is non-contributory. pub fn exchange(self, other: &EphemeralPublicKey) -> Option { - // This is the line mentioned above preventing `ZeroizeOnDrop` for this struct. - let out = self.inner.diffie_hellman(&other.inner); + let secret = self.inner.expose_unwrap(); + let out = secret.diffie_hellman(&other.inner); if !out.was_contributory() { return None; } - Some(SharedSecret { inner: out }) + Some(SharedSecret::new(out)) } } diff --git a/cryptography/src/lib.rs b/cryptography/src/lib.rs index 87e0d7964e..332876d0b8 100644 --- a/cryptography/src/lib.rs +++ b/cryptography/src/lib.rs @@ -35,6 +35,8 @@ pub mod handshake; pub mod lthash; pub use lthash::LtHash; pub mod secp256r1; +pub mod secret; +pub use secret::Secret; pub mod transcript; /// Produces [Signature]s over messages that can be verified with a corresponding [PublicKey]. @@ -72,7 +74,7 @@ pub trait Signer: Random + Send + Sync + Clone + 'static { } /// A [Signer] that can be serialized/deserialized. -pub trait PrivateKey: Signer + Sized + ReadExt + Encode + PartialEq + Array {} +pub trait PrivateKey: Signer + Sized + ReadExt + Encode + PartialEq {} /// Verifies [Signature]s over messages. pub trait Verifier { diff --git a/cryptography/src/secp256r1/common.rs b/cryptography/src/secp256r1/common.rs index b13233f34f..532cc3940f 100644 --- a/cryptography/src/secp256r1/common.rs +++ b/cryptography/src/secp256r1/common.rs @@ -1,10 +1,4 @@ -cfg_if::cfg_if! { - if #[cfg(feature = "std")] { - use std::borrow::ToOwned; - } else { - use alloc::borrow::ToOwned; - } -} +use crate::Secret; use bytes::{Buf, BufMut}; use commonware_codec::{Error as CodecError, FixedSize, Read, ReadExt, Write}; use commonware_math::algebra::Random; @@ -16,34 +10,38 @@ use core::{ }; use p256::ecdsa::{SigningKey, VerifyingKey}; use rand_core::CryptoRngCore; -use zeroize::{Zeroize, ZeroizeOnDrop}; pub const CURVE_NAME: &str = "secp256r1"; pub const PRIVATE_KEY_LENGTH: usize = 32; pub const PUBLIC_KEY_LENGTH: usize = 33; // Y-Parity || X /// Internal Secp256r1 Private Key storage. -#[derive(Clone, Eq, PartialEq, ZeroizeOnDrop)] +#[derive(Clone, Debug)] pub struct PrivateKeyInner { - raw: [u8; PRIVATE_KEY_LENGTH], - pub key: SigningKey, + raw: Secret<[u8; PRIVATE_KEY_LENGTH]>, + pub(crate) key: Secret, } -impl Zeroize for PrivateKeyInner { - fn zeroize(&mut self) { - self.raw.zeroize(); - - // skip zeroizing `key` here, `ZeroizeOnDrop` is implemented for `SigningKey` and - // can't be called directly. - // - // Reference: +impl PartialEq for PrivateKeyInner { + fn eq(&self, other: &Self) -> bool { + self.raw == other.raw } } +impl Eq for PrivateKeyInner {} + impl PrivateKeyInner { pub fn new(key: SigningKey) -> Self { - let raw = key.to_bytes().into(); - Self { raw, key } + let raw: [u8; PRIVATE_KEY_LENGTH] = key.to_bytes().into(); + Self { + raw: Secret::new(raw), + key: Secret::new(key), + } + } + + /// Returns the `VerifyingKey` corresponding to this private key. + pub fn verifying_key(&self) -> VerifyingKey { + self.key.expose(|key| *key.verifying_key()) } } @@ -55,7 +53,7 @@ impl Random for PrivateKeyInner { impl Write for PrivateKeyInner { fn write(&self, buf: &mut impl BufMut) { - self.raw.write(buf); + self.raw.expose(|raw| raw.write(buf)); } } @@ -64,13 +62,13 @@ impl Read for PrivateKeyInner { fn read_cfg(buf: &mut impl Buf, _: &()) -> Result { let raw = <[u8; PRIVATE_KEY_LENGTH]>::read(buf)?; - let result = SigningKey::from_slice(&raw); + let key = SigningKey::from_slice(&raw); #[cfg(feature = "std")] - let key = result.map_err(|e| CodecError::Wrapped(CURVE_NAME, e.into()))?; + let key = key.map_err(|e| CodecError::Wrapped(CURVE_NAME, e.into()))?; #[cfg(not(feature = "std"))] - let key = result - .map_err(|e| CodecError::Wrapped(CURVE_NAME, alloc::format!("{:?}", e).into()))?; - Ok(Self { raw, key }) + let key = + key.map_err(|e| CodecError::Wrapped(CURVE_NAME, alloc::format!("{:?}", e).into()))?; + Ok(Self::new(key)) } } @@ -78,56 +76,15 @@ impl FixedSize for PrivateKeyInner { const SIZE: usize = PRIVATE_KEY_LENGTH; } -impl Span for PrivateKeyInner {} - -impl Array for PrivateKeyInner {} - -impl Hash for PrivateKeyInner { - fn hash(&self, state: &mut H) { - self.raw.hash(state); - } -} - -impl Ord for PrivateKeyInner { - fn cmp(&self, other: &Self) -> core::cmp::Ordering { - self.raw.cmp(&other.raw) - } -} - -impl PartialOrd for PrivateKeyInner { - fn partial_cmp(&self, other: &Self) -> Option { - Some(self.cmp(other)) - } -} - -impl AsRef<[u8]> for PrivateKeyInner { - fn as_ref(&self) -> &[u8] { - &self.raw - } -} - -impl Deref for PrivateKeyInner { - type Target = [u8]; - fn deref(&self) -> &[u8] { - &self.raw - } -} - impl From for PrivateKeyInner { fn from(signer: SigningKey) -> Self { Self::new(signer) } } -impl Debug for PrivateKeyInner { - fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { - write!(f, "{}", hex(&self.raw)) - } -} - impl Display for PrivateKeyInner { fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { - write!(f, "{}", hex(&self.raw)) + write!(f, "{:?}", self) } } @@ -157,7 +114,7 @@ impl PublicKeyInner { } pub fn from_private_key(private_key: &PrivateKeyInner) -> Self { - Self::new(private_key.key.verifying_key().to_owned()) + Self::new(private_key.verifying_key()) } /// aws-lc-rs can decompress keys, but since we already have the key parsed @@ -275,41 +232,6 @@ macro_rules! impl_private_key_wrapper { const SIZE: usize = PRIVATE_KEY_LENGTH; } - impl commonware_utils::Span for $name {} - - impl commonware_utils::Array for $name {} - - impl core::hash::Hash for $name { - fn hash(&self, state: &mut H) { - self.0.hash(state); - } - } - - impl Ord for $name { - fn cmp(&self, other: &Self) -> core::cmp::Ordering { - self.0.cmp(&other.0) - } - } - - impl PartialOrd for $name { - fn partial_cmp(&self, other: &Self) -> Option { - Some(self.cmp(other)) - } - } - - impl AsRef<[u8]> for $name { - fn as_ref(&self) -> &[u8] { - self.0.as_ref() - } - } - - impl core::ops::Deref for $name { - type Target = [u8]; - fn deref(&self) -> &[u8] { - &self.0 - } - } - impl From for $name { fn from(signer: p256::ecdsa::SigningKey) -> Self { Self(PrivateKeyInner::from(signer)) @@ -877,6 +799,15 @@ pub(crate) mod tests { (public_key, sig, message, true) } + #[test] + fn test_private_key_redacted() { + let private_key = create_private_key(); + let debug = format!("{:?}", private_key); + let display = format!("{}", private_key); + assert!(debug.contains("REDACTED")); + assert!(display.contains("REDACTED")); + } + #[cfg(feature = "arbitrary")] mod conformance { use super::*; diff --git a/cryptography/src/secp256r1/recoverable.rs b/cryptography/src/secp256r1/recoverable.rs index c333529a12..b97793c274 100644 --- a/cryptography/src/secp256r1/recoverable.rs +++ b/cryptography/src/secp256r1/recoverable.rs @@ -51,7 +51,7 @@ impl PrivateKey { let (mut signature, mut recovery_id) = self .0 .key - .sign_recoverable(&payload) + .expose(|key| key.sign_recoverable(&payload)) .expect("signing must succeed"); // The signing algorithm generates k, then calculates r <- x(k * G). Normalizing s by negating it is equivalent diff --git a/cryptography/src/secp256r1/standard.rs b/cryptography/src/secp256r1/standard.rs index 8b312dede8..27f4028e44 100644 --- a/cryptography/src/secp256r1/standard.rs +++ b/cryptography/src/secp256r1/standard.rs @@ -50,7 +50,7 @@ impl PrivateKey { let payload = namespace.map_or(Cow::Borrowed(msg), |namespace| { Cow::Owned(union_unique(namespace, msg)) }); - let signature: p256::ecdsa::Signature = self.0.key.sign(&payload); + let signature: p256::ecdsa::Signature = self.0.key.expose(|key| key.sign(&payload)); let signature = signature.normalize_s().unwrap_or(signature); Signature::from(signature) } diff --git a/cryptography/src/secret.rs b/cryptography/src/secret.rs new file mode 100644 index 0000000000..1bc0d97b4a --- /dev/null +++ b/cryptography/src/secret.rs @@ -0,0 +1,204 @@ +//! A wrapper type for secret values that prevents accidental leakage. +//! +//! `Secret` provides the following guarantees: +//! - Debug and Display always show `[REDACTED]` instead of the actual value +//! - The inner value is zeroized on drop +//! - Access to the inner value requires an explicit `expose()` call +//! - Comparisons use constant-time operations to prevent timing attacks +//! +//! # Type Constraints +//! +//! **Important**: `Secret` is designed for flat data types without pointers +//! (e.g. `[u8; N]`). It does NOT provide full protection for types with +//! indirection. Types like `Vec`, `String`, or `Box` will only have their +//! metadata (pointer, length, capacity) zeroized, the referenced data remains +//! intact. Do not use `Secret` with types that contain pointers. + +use core::{ + fmt::{Debug, Display, Formatter}, + mem::ManuallyDrop, +}; +use ctutils::CtEq; +use zeroize::{Zeroize, ZeroizeOnDrop}; + +/// Zeroize memory at the given pointer using volatile writes. +/// +/// # Safety +/// +/// `ptr` must point to allocated, writable memory of at least `size_of::()` bytes. +#[inline] +unsafe fn zeroize_ptr(ptr: *mut T) { + let slice = core::slice::from_raw_parts_mut(ptr as *mut u8, core::mem::size_of::()); + slice.zeroize(); +} + +/// A wrapper for secret values that prevents accidental leakage. +/// +/// - Debug and Display show `[REDACTED]` +/// - Zeroized on drop +/// - Access requires explicit `expose()` call +/// +/// # Type Constraints +/// +/// Only use with flat data types that have no pointers (e.g. `[u8; N]`). +/// See [module-level documentation](self) for details. +pub struct Secret(ManuallyDrop); + +impl Secret { + /// Creates a new `Secret` wrapping the given value. + #[inline] + pub const fn new(value: T) -> Self { + Self(ManuallyDrop::new(value)) + } + + /// Exposes the secret value for read-only access within a closure. + /// + /// # Note + /// + /// The closure uses a higher-ranked trait bound (`for<'a>`) to prevent + /// the returned value from containing references to the secret data. + /// This ensures the reference cannot escape the closure scope. However, + /// this does not prevent copying or cloning the secret value within + /// the closure (e.g., `secret.expose(|s| s.clone())`). Callers should + /// avoid leaking secrets through such patterns. + /// + /// Additionally, any temporaries derived from the secret (e.g. + /// `s.as_slice()`) may leave secret data on the stack that will not be + /// automatically zeroized. Callers should wrap such temporaries in + /// [`zeroize::Zeroizing`] if they contain sensitive data. + #[inline] + pub fn expose(&self, f: impl for<'a> FnOnce(&'a T) -> R) -> R { + f(&self.0) + } + + /// Consumes the [Secret] and returns the inner value, zeroizing the original + /// memory location. + /// + /// Use this when you need to transfer ownership of the secret value (e.g., + /// for APIs that consume the value). + #[inline] + pub fn expose_unwrap(mut self) -> T { + let ptr = &raw mut *self.0; + // SAFETY: + // Pointer obtained while self.0 is still initialized, + // self.0 is initialized and we have exclusive access + let value = unsafe { ManuallyDrop::take(&mut self.0) }; + + // Prevent Secret::drop from running (would double-zeroize or double-free on panic) + core::mem::forget(self); + + // SAFETY: uses raw pointer (not reference) to zero memory after drop + unsafe { zeroize_ptr(ptr) }; + + value + } +} + +impl Drop for Secret { + fn drop(&mut self) { + let ptr = &raw mut *self.0; + // SAFETY: + // - Pointer obtained while self.0 is still initialized + // - ManuallyDrop::drop: self.0 is initialized and we have exclusive access + // - zeroize_ptr: uses raw pointer (not reference) to zero memory after drop + unsafe { + ManuallyDrop::drop(&mut self.0); + zeroize_ptr(ptr); + } + } +} + +impl Debug for Secret { + fn fmt(&self, f: &mut Formatter<'_>) -> core::fmt::Result { + f.write_str("Secret([REDACTED])") + } +} + +impl Display for Secret { + fn fmt(&self, f: &mut Formatter<'_>) -> core::fmt::Result { + f.write_str("[REDACTED]") + } +} + +impl ZeroizeOnDrop for Secret {} + +impl Clone for Secret { + fn clone(&self) -> Self { + self.expose(|v| Self::new(v.clone())) + } +} + +impl PartialEq for Secret { + fn eq(&self, other: &Self) -> bool { + self.expose(|a| other.expose(|b| a.ct_eq(b).into())) + } +} + +impl Eq for Secret {} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_debug_redacted() { + let secret = Secret::new([1u8, 2, 3, 4]); + assert_eq!(format!("{:?}", secret), "Secret([REDACTED])"); + } + + #[test] + fn test_display_redacted() { + let secret = Secret::new([1u8, 2, 3, 4]); + assert_eq!(format!("{}", secret), "[REDACTED]"); + } + + #[test] + fn test_expose() { + let secret = Secret::new([1u8, 2, 3, 4]); + secret.expose(|v| { + assert_eq!(v, &[1u8, 2, 3, 4]); + }); + } + + #[test] + fn test_expose_unwrap() { + let secret = Secret::new([1u8, 2, 3, 4]); + let value = secret.expose_unwrap(); + assert_eq!(value, [1u8, 2, 3, 4]); + } + + #[test] + fn test_clone() { + let secret = Secret::new([1u8, 2, 3, 4]); + let cloned = secret.clone(); + secret.expose(|a| { + cloned.expose(|b| { + assert_eq!(a, b); + }); + }); + } + + #[test] + fn test_equality() { + let s1 = Secret::new([1u8, 2, 3, 4]); + let s2 = Secret::new([1u8, 2, 3, 4]); + let s3 = Secret::new([5u8, 6, 7, 8]); + assert_eq!(s1, s2); + assert_ne!(s1, s3); + } + + #[test] + fn test_multiple_expose() { + let secret = Secret::new([42u8; 32]); + + // First expose + secret.expose(|v| { + assert_eq!(v[0], 42); + }); + + // Second expose + secret.expose(|v| { + assert_eq!(v[31], 42); + }); + } +}