From 806b393419e9c37475b78d444a682d2cb1440d22 Mon Sep 17 00:00:00 2001 From: David Palm Date: Fri, 6 Dec 2024 17:08:39 +0100 Subject: [PATCH 1/9] Don't use slice indexing (part 1 of n) --- synedrion/src/cggmp21/entities.rs | 15 +- synedrion/src/cggmp21/interactive_signing.rs | 137 ++++++++++++++----- synedrion/src/lib.rs | 3 +- synedrion/src/www02/entities.rs | 95 ++++++++++--- synedrion/src/www02/key_resharing.rs | 70 ++++++---- 5 files changed, 233 insertions(+), 87 deletions(-) diff --git a/synedrion/src/cggmp21/entities.rs b/synedrion/src/cggmp21/entities.rs index 87aeef57..3db76a96 100644 --- a/synedrion/src/cggmp21/entities.rs +++ b/synedrion/src/cggmp21/entities.rs @@ -133,10 +133,23 @@ impl KeyShare { let secret_share = SecretBox::new(Box::new( self.secret_share.expose_secret() + change.secret_share_change.expose_secret(), )); + // TODO(dp): return error instead? + assert_eq!( + self.public_shares.len(), + change.public_share_changes.len(), + "Inconsistent number of public key shares in updated share set (expected {}, was {})", + self.public_shares.len(), + change.public_share_changes.len() + ); let public_shares = self .public_shares .iter() - .map(|(id, public_share)| (id.clone(), public_share + &change.public_share_changes[id])) + .zip(change.public_share_changes) + // TODO(dp): this should fail, I'm pretty sure, but doesn't (no test) + // let hh = change.public_share_changes.first_key_value().unwrap().0.clone(); + // .map(|(pub_share, changed_pub_share)| (hh.clone(), pub_share.1 + &changed_pub_share.1)) + // TODO(dp): is this correct? No test! + .map(|(pub_share, changed_pub_share)| (changed_pub_share.0, pub_share.1 + &changed_pub_share.1)) .collect(); Self { diff --git a/synedrion/src/cggmp21/interactive_signing.rs b/synedrion/src/cggmp21/interactive_signing.rs index ecffde4e..0cd360d9 100644 --- a/synedrion/src/cggmp21/interactive_signing.rs +++ b/synedrion/src/cggmp21/interactive_signing.rs @@ -20,7 +20,7 @@ use secrecy::{ExposeSecret, SecretBox}; use serde::{Deserialize, Serialize}; use super::{ - entities::{AuxInfo, AuxInfoPrecomputed, KeyShare, PresigningData, PresigningValues}, + entities::{AuxInfo, AuxInfoPrecomputed, KeyShare, PresigningData, PresigningValues, PublicAuxInfoPrecomputed}, params::SchemeParams, sigma::{AffGProof, DecProof, EncProof, LogStarProof, MulProof, MulStarProof}, }; @@ -206,6 +206,16 @@ struct Round1 { cap_g: Ciphertext, } +impl Round1 { + fn public_aux(&self, i: &I) -> Result<&PublicAuxInfoPrecomputed

, LocalError> { + self.context + .aux_info + .public_aux + .get(i) + .ok_or_else(|| LocalError::new(format!("Missing public_aux for party Id {i:?}"))) + } +} + #[derive(Clone, Serialize, Deserialize)] #[serde(bound(serialize = "CiphertextWire: Serialize"))] #[serde(bound(deserialize = "CiphertextWire: for<'x> Deserialize<'x>"))] @@ -272,7 +282,7 @@ impl Round for Round1 { &self.context.rho, self.context.aux_info.secret_aux.paillier_sk.public_key(), &self.cap_k, - &self.context.aux_info.public_aux[destination].rp_params, + &self.public_aux(destination)?.rp_params, &aux, ); @@ -295,9 +305,9 @@ impl Round for Round1 { let aux = (&self.context.ssid_hash, &self.context.my_id); - let public_aux = &self.context.aux_info.public_aux[&self.context.my_id]; + let public_aux = self.public_aux(&self.context.my_id)?; - let from_pk = &self.context.aux_info.public_aux[from].paillier_pk; + let from_pk = &self.public_aux(from)?.paillier_pk; if !direct_message.psi0.verify( from_pk, @@ -334,19 +344,21 @@ impl Round for Round1 { let mut all_cap_k = others_cap_k .into_iter() .map(|(id, ciphertext)| { - let ciphertext_mod = ciphertext.to_precomputed(&self.context.aux_info.public_aux[&id].paillier_pk); - (id, ciphertext_mod) + let paux = self.public_aux(&id)?; + Ok((id, ciphertext.to_precomputed(&paux.paillier_pk))) }) - .collect::>(); - all_cap_k.insert(my_id.clone(), self.cap_k); + .collect::, _>>()?; let mut all_cap_g = others_cap_g .into_iter() .map(|(id, ciphertext)| { - let ciphertext_mod = ciphertext.to_precomputed(&self.context.aux_info.public_aux[&id].paillier_pk); - (id, ciphertext_mod) + let paux = self.public_aux(&id)?; + let ciphertext_mod = ciphertext.to_precomputed(&paux.paillier_pk); + Ok((id, ciphertext_mod)) }) - .collect::>(); + .collect::, _>>()?; + + all_cap_k.insert(my_id.clone(), self.cap_k); all_cap_g.insert(my_id, self.cap_g); Ok(FinalizeOutcome::AnotherRound(BoxedRound::new_dynamic(Round2 { @@ -364,6 +376,17 @@ struct Round2 { all_cap_g: BTreeMap>, } +// TODO(dp): convenient enough to include in `Round`? +impl Round2 { + fn public_aux(&self, i: &I) -> Result<&PublicAuxInfoPrecomputed

, LocalError> { + self.context + .aux_info + .public_aux + .get(i) + .ok_or_else(|| LocalError::new(format!("Missing public_aux for party Id {i:?}"))) + } +} + #[derive(Clone, Serialize, Deserialize)] #[serde(bound(serialize = " CiphertextWire: Serialize, @@ -438,7 +461,7 @@ impl Round for Round2 { let cap_gamma = self.context.gamma.mul_by_generator(); let pk = self.context.aux_info.secret_aux.paillier_sk.public_key(); - let target_pk = &self.context.aux_info.public_aux[destination].paillier_pk; + let target_pk = &self.public_aux(destination)?.paillier_pk; let beta = SecretBox::new(Box::new(Signed::random_bounded_bits(rng, P::LP_BOUND))); let hat_beta = SecretBox::new(Box::new(Signed::random_bounded_bits(rng, P::LP_BOUND))); @@ -448,16 +471,22 @@ impl Round for Round2 { let hat_s = Randomizer::random(rng, target_pk); let cap_f = Ciphertext::new_with_randomizer_signed(pk, beta.expose_secret(), &r.to_wire()); - let cap_d = &self.all_cap_k[destination] * P::signed_from_scalar(&self.context.gamma) + let cap_d = self + .all_cap_k + .get(destination) + .ok_or(LocalError::new("Missing destination={destination:?} in all_cap_k"))? + * P::signed_from_scalar(&self.context.gamma) + Ciphertext::new_with_randomizer_signed(target_pk, &-beta.expose_secret(), &s.to_wire()); let hat_cap_f = Ciphertext::new_with_randomizer_signed(pk, hat_beta.expose_secret(), &hat_r.to_wire()); - let hat_cap_d = &self.all_cap_k[destination] + let hat_cap_d = self + .all_cap_k + .get(destination) + .ok_or(LocalError::new("Missing destination={destination:?} in all_cap_k"))? * P::signed_from_scalar(self.context.key_share.secret_share.expose_secret()) + Ciphertext::new_with_randomizer_signed(target_pk, &-hat_beta.expose_secret(), &hat_s.to_wire()); - let public_aux = &self.context.aux_info.public_aux[destination]; - let rp = &public_aux.rp_params; + let rp = &self.public_aux(destination)?.rp_params; let psi = AffGProof::new( rng, @@ -467,7 +496,9 @@ impl Round for Round2 { r.clone(), target_pk, pk, - &self.all_cap_k[destination], + self.all_cap_k + .get(destination) + .ok_or(LocalError::new("destination={destination:?} is missing in all_cap_k"))?, &cap_d, &cap_f, &cap_gamma, @@ -483,10 +514,16 @@ impl Round for Round2 { hat_r.clone(), target_pk, pk, - &self.all_cap_k[destination], + self.all_cap_k + .get(destination) + .ok_or(LocalError::new("destination={destination:?} is missing in all_cap_k"))?, &hat_cap_d, &hat_cap_f, - &self.context.key_share.public_shares[&self.context.my_id], + self.context + .key_share + .public_shares + .get(&self.context.my_id) + .ok_or(LocalError::new("my_id={my_id:?} missing in public_shares"))?, rp, &aux, ); @@ -496,7 +533,10 @@ impl Round for Round2 { &P::signed_from_scalar(&self.context.gamma), &self.context.nu, pk, - &self.all_cap_g[&self.context.my_id], + self.all_cap_g.get(&self.context.my_id).ok_or(LocalError::new(format!( + "my_id={:?} is missing in all_cap_g", + &self.context.my_id + )))?, &Point::GENERATOR, &cap_gamma, rp, @@ -546,14 +586,18 @@ impl Round for Round2 { normal_broadcast.assert_is_none()?; let direct_message = direct_message.deserialize::>(deserializer)?; - let aux = (&self.context.ssid_hash, &from); - let pk = &self.context.aux_info.secret_aux.paillier_sk.public_key(); - let from_pk = &self.context.aux_info.public_aux[from].paillier_pk; + let aux = (&self.context.ssid_hash, from); + let pk = self.context.aux_info.secret_aux.paillier_sk.public_key(); + let from_pk = &self.public_aux(from)?.paillier_pk; - let cap_x = self.context.key_share.public_shares[from]; + let cap_x = self + .context + .key_share + .public_shares + .get(from) + .ok_or(LocalError::new("from={from:?} is missing in public_shares"))?; - let public_aux = &self.context.aux_info.public_aux[&self.context.my_id]; - let rp = &public_aux.rp_params; + let rp = &self.public_aux(&self.context.my_id)?.rp_params; let cap_d = direct_message.cap_d.to_precomputed(pk); let hat_cap_d = direct_message.hat_cap_d.to_precomputed(pk); @@ -561,7 +605,9 @@ impl Round for Round2 { if !direct_message.psi.verify( pk, from_pk, - &self.all_cap_k[&self.context.my_id], + self.all_cap_k + .get(&self.context.my_id) + .ok_or(LocalError::new("my_id={my_id:?} is missing in all_cap_k"))?, &cap_d, &direct_message.cap_f.to_precomputed(from_pk), &direct_message.cap_gamma, @@ -576,10 +622,12 @@ impl Round for Round2 { if !direct_message.hat_psi.verify( pk, from_pk, - &self.all_cap_k[&self.context.my_id], + self.all_cap_k + .get(&self.context.my_id) + .ok_or(LocalError::new("my_id={my_id:?} is missing in all_cap_k"))?, &hat_cap_d, &direct_message.hat_cap_f.to_precomputed(from_pk), - &cap_x, + cap_x, rp, &aux, ) { @@ -590,7 +638,9 @@ impl Round for Round2 { if !direct_message.hat_psi_prime.verify( from_pk, - &self.all_cap_g[from], + self.all_cap_g + .get(from) + .ok_or(LocalError::new("from={from:?} is missing in all_cap_g"))?, &Point::GENERATOR, &direct_message.cap_gamma, rp, @@ -686,6 +736,16 @@ struct Round3 { round2_artifacts: BTreeMap>, } +impl Round3 { + fn public_aux(&self, i: &I) -> Result<&PublicAuxInfoPrecomputed

, LocalError> { + self.context + .aux_info + .public_aux + .get(i) + .ok_or_else(|| LocalError::new(format!("Missing public_aux for party Id {i:?}"))) + } +} + #[derive(Clone, Serialize, Deserialize)] #[serde(bound(serialize = "LogStarProof

: Serialize"))] #[serde(bound(deserialize = "LogStarProof

: for<'x> Deserialize<'x>"))] @@ -728,15 +788,17 @@ impl Round for Round3 { let aux = (&self.context.ssid_hash, &self.context.my_id); let pk = &self.context.aux_info.secret_aux.paillier_sk.public_key(); - let public_aux = &self.context.aux_info.public_aux[destination]; - let rp = &public_aux.rp_params; + let rp = &self.public_aux(destination)?.rp_params; let psi_pprime = LogStarProof::new( rng, &P::signed_from_scalar(&self.context.k), &self.context.rho, pk, - &self.all_cap_k[&self.context.my_id], + self.all_cap_k.get(&self.context.my_id).ok_or(LocalError::new(format!( + "my_id={:?} is missing in all_cap_k", + &self.context.my_id + )))?, &self.cap_gamma, &self.cap_delta, rp, @@ -769,14 +831,15 @@ impl Round for Round3 { let direct_message = direct_message.deserialize::>(deserializer)?; let aux = (&self.context.ssid_hash, &from); - let from_pk = &self.context.aux_info.public_aux[from].paillier_pk; + let from_pk = &self.public_aux(from)?.paillier_pk; - let public_aux = &self.context.aux_info.public_aux[&self.context.my_id]; - let rp = &public_aux.rp_params; + let rp = &self.public_aux(&self.context.my_id)?.rp_params; if !direct_message.psi_pprime.verify( from_pk, - &self.all_cap_k[from], + self.all_cap_k + .get(from) + .ok_or(LocalError::new("from={from:?} is missing in all_cap_k"))?, &self.cap_gamma, &direct_message.cap_delta, rp, diff --git a/synedrion/src/lib.rs b/synedrion/src/lib.rs index e45c245b..7d3dff46 100644 --- a/synedrion/src/lib.rs +++ b/synedrion/src/lib.rs @@ -9,7 +9,8 @@ rust_2018_idioms, trivial_casts, trivial_numeric_casts, - unused_qualifications + unused_qualifications, + clippy::indexing_slicing )] #![cfg_attr(not(test), warn(clippy::unwrap_used))] diff --git a/synedrion/src/www02/entities.rs b/synedrion/src/www02/entities.rs index b6997ce6..4419d7d8 100644 --- a/synedrion/src/www02/entities.rs +++ b/synedrion/src/www02/entities.rs @@ -1,9 +1,11 @@ use alloc::{ boxed::Box, collections::{BTreeMap, BTreeSet}, + format, vec::Vec, }; use core::{fmt::Debug, marker::PhantomData}; +use manul::session::LocalError; use bip32::{DerivationPath, PrivateKey, PrivateKeyBytes, PublicKey}; use k256::ecdsa::{SigningKey, VerifyingKey}; @@ -37,8 +39,11 @@ pub struct ThresholdKeyShare { impl ThresholdKeyShare { /// Threshold share ID. - pub fn share_id(&self) -> ShareId { - self.share_ids[&self.owner] + pub fn share_id(&self) -> Result<&ShareId, LocalError> { + self.share_ids.get(&self.owner).ok_or(LocalError::new(format!( + "owner={:?} is missing in the share_ids", + self.owner + ))) } /// The threshold. @@ -66,24 +71,37 @@ impl ThresholdKeyShare>(); + .map(|(id, share_id)| { + let secret_share = secret_shares + .get(share_id) + .ok_or(LocalError::new("share_id={share_id:?} is missing in the secret shares"))?; + Ok((id.clone(), secret_share.mul_by_generator())) + }) + .collect::, LocalError>>() + .expect("TODO(dp): Return error"); ids.iter() .map(|id| { - ( + let share_id = share_ids + .get(id) + .ok_or(LocalError::new("id={id:?} is missing in the share_ids"))?; + let secret_share = secret_shares + .get(share_id) + .ok_or(LocalError::new("share_id={share_id:?} is missing in the secret shares"))?; + Ok(( id.clone(), Self { owner: id.clone(), threshold: threshold as u32, - secret_share: SecretBox::new(Box::new(secret_shares[&share_ids[id]])), + secret_share: SecretBox::new(Box::new(*secret_share)), share_ids: share_ids.clone(), public_shares: public_shares.clone(), phantom: PhantomData, }, - ) + )) }) - .collect() + .collect::>() + .expect("TODO(dp): Return LocalErr") } pub(crate) fn verifying_key_as_point(&self) -> Point { @@ -91,9 +109,15 @@ impl ThresholdKeyShare>() + .expect("TODO(dp): return LocalError"), ) } @@ -111,25 +135,46 @@ impl ThresholdKeyShare>(); + .map(|id| { + let share_id = self + .share_ids + .get(id) + .ok_or(LocalError::new("id={id:?} is missing in the share_ids"))?; + Ok((id.clone(), *share_id)) + }) + .collect::, LocalError>>() + .expect("TODO(dp): make this method return LocalErr"); let share_ids_set = share_ids.values().cloned().collect(); let secret_share = SecretBox::new(Box::new( - self.secret_share.expose_secret() * &interpolation_coeff(&share_ids_set, &share_id), + self.secret_share.expose_secret() * &interpolation_coeff(&share_ids_set, owner_share_id), )); let public_shares = ids .iter() .map(|id| { - ( + let public_share = self + .public_shares + .get(id) + .ok_or(LocalError::new("id={id:?} is missing in the public shares"))?; + let this_share_id = self + .share_ids + .get(id) + .ok_or(LocalError::new("id={id:?} is missing in the share_ids"))?; + Ok(( id.clone(), - self.public_shares[id] * interpolation_coeff(&share_ids_set, &self.share_ids[id]), - ) + public_share * &interpolation_coeff(&share_ids_set, this_share_id), + )) }) - .collect(); + .collect::>() + .expect("TODO(dp): make this method return LocalErr"); KeyShare { owner: self.owner.clone(), @@ -150,18 +195,24 @@ impl ThresholdKeyShare>(); let share_ids_set = share_ids.values().cloned().collect(); + let owner_share_id = share_ids + .get(key_share.owner()) + .expect("Just created a ShareId for all parties"); let secret_share = SecretBox::new(Box::new( key_share.secret_share.expose_secret() - * &interpolation_coeff(&share_ids_set, &share_ids[key_share.owner()]) + * &interpolation_coeff(&share_ids_set, owner_share_id) .invert() .expect("the interpolation coefficient is a non-zero scalar"), )); let public_shares = ids .iter() .map(|id| { - let share_id = share_ids[id]; - let public_share = key_share.public_shares[id] - * interpolation_coeff(&share_ids_set, &share_id) + let share_id = share_ids.get(id).expect("share_ids and ids have identical lengths"); + let public_share = key_share + .public_shares + .get(id) + .expect("There is one public share (Point) for each party") + * &interpolation_coeff(&share_ids_set, share_id) .invert() .expect("the interpolation coefficient is a non-zero scalar"); (id.clone(), public_share) diff --git a/synedrion/src/www02/key_resharing.rs b/synedrion/src/www02/key_resharing.rs index 948e4e6b..09ceec9d 100644 --- a/synedrion/src/www02/key_resharing.rs +++ b/synedrion/src/www02/key_resharing.rs @@ -8,6 +8,7 @@ use alloc::{ boxed::Box, collections::{BTreeMap, BTreeSet}, + format, string::String, vec::Vec, }; @@ -177,20 +178,23 @@ impl EntryPoint for KeyResharing { EchoRoundParticipation::Default }; - let old_holder = self.old_holder.map(|old_holder| { - let polynomial = Polynomial::random( - rng, - old_holder.key_share.secret_share.expose_secret(), - self.new_threshold, - ); - let public_polynomial = polynomial.public(); - - OldHolderData { - polynomial, - share_id: old_holder.key_share.share_id(), - public_polynomial, - } - }); + let old_holder = self + .old_holder + .map(|old_holder| { + let polynomial = Polynomial::random( + rng, + old_holder.key_share.secret_share.expose_secret(), + self.new_threshold, + ); + let public_polynomial = polynomial.public(); + + Ok(OldHolderData { + polynomial, + share_id: *old_holder.key_share.share_id()?, + public_polynomial, + }) + }) + .transpose()?; let new_holder = self.new_holder.map(|new_holder| NewHolderData { inputs: new_holder }); @@ -298,7 +302,12 @@ impl Round for Round1 { destination: &I, ) -> Result<(DirectMessage, Option), LocalError> { if let Some(old_holder) = self.old_holder.as_ref() { - let subshare = old_holder.polynomial.evaluate(&self.new_share_ids[destination]); + let their_share_id = self.new_share_ids.get(destination).ok_or(LocalError::new(format!( + "destination={:?} is missing from the new_share_ids", + destination + )))?; + + let subshare = old_holder.polynomial.evaluate(their_share_id); let dm = DirectMessage::new(serializer, Round1DirectMessage { subshare })?; Ok((dm, None)) } else { @@ -321,9 +330,11 @@ impl Round for Round1 { if let Some(new_holder) = self.new_holder.as_ref() { if new_holder.inputs.old_holders.contains(from) { - let public_subshare_from_poly = echo_broadcast - .public_polynomial - .evaluate(&self.new_share_ids[&self.my_id]); + let my_share_id = self.new_share_ids.get(&self.my_id).ok_or(LocalError::new(format!( + "my_id={:?} is missing from the new_share_ids", + &self.my_id + )))?; + let public_subshare_from_poly = echo_broadcast.public_polynomial.evaluate(my_share_id); let public_subshare_from_private = direct_message.subshare.mul_by_generator(); // Check that the public polynomial sent in the broadcast corresponds to the secret share @@ -356,13 +367,16 @@ impl Round for Round1 { let mut payloads = payloads.downcast_all::()?; - let share_id = self.new_share_ids[&self.my_id]; + let share_id = self + .new_share_ids + .get(&self.my_id) + .ok_or_else(|| LocalError::new(format!("my_id={:?} is missing from new_share_ids", &self.my_id)))?; // If this node is both an old and a new holder, // add a simulated payload to the mapping, as if it sent a message to itself. if let Some(old_holder) = self.old_holder.as_ref() { if self.new_holder.as_ref().is_some() { - let subshare = old_holder.polynomial.evaluate(&share_id); + let subshare = old_holder.polynomial.evaluate(share_id); let my_payload = Round1Payload { subshare, public_polynomial: old_holder.public_polynomial.clone(), @@ -396,19 +410,23 @@ impl Round for Round1 { .inputs .old_holders .iter() - .map(|id| (payloads[id].old_share_id, payloads[id].subshare)) - .collect::>(); + .map(|id| { + let payload = payloads + .get(id) + .ok_or(LocalError::new("id={id:?} is missing from the payloads"))?; + Ok((payload.old_share_id, payload.subshare)) + }) + .collect::, _>>()?; let secret_share = SecretBox::new(Box::new(shamir_join_scalars(&subshares))); // Generate the public shares of all the new holders. let public_shares = self .new_share_ids - .keys() - .map(|id| { - let share_id = self.new_share_ids[id]; + .iter() + .map(|(id, share_id)| { let public_subshares = payloads .values() - .map(|p| (p.old_share_id, p.public_polynomial.evaluate(&share_id))) + .map(|p| (p.old_share_id, p.public_polynomial.evaluate(share_id))) .collect::>(); let public_share = shamir_join_points(&public_subshares); (id.clone(), public_share) From 20938a7aabfb78ee7f601e35f05eaecf968d7aac Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 9 Dec 2024 13:00:02 +0100 Subject: [PATCH 2/9] Avoid index slicing --- synedrion/src/cggmp21/aux_gen.rs | 1 + synedrion/src/cggmp21/interactive_signing.rs | 19 +++++++++--- synedrion/src/cggmp21/key_init.rs | 1 + synedrion/src/cggmp21/key_refresh.rs | 1 + synedrion/src/cggmp21/params.rs | 31 +++++++++++++++----- synedrion/src/cggmp21/sigma/mod_.rs | 11 +++---- synedrion/src/cggmp21/sigma/prm.rs | 10 +++---- synedrion/src/cggmp21/signing_malicious.rs | 1 + synedrion/src/curve/arithmetic.rs | 1 + synedrion/src/paillier/rsa.rs | 12 ++++++-- synedrion/src/tools/bitvec.rs | 4 +-- synedrion/src/tools/hashing.rs | 27 ++++++++++------- synedrion/src/tools/sss.rs | 20 ++++++++----- synedrion/src/uint/bounded.rs | 10 +++++-- synedrion/src/uint/traits.rs | 12 ++++---- synedrion/src/www02/entities.rs | 1 + synedrion/src/www02/key_resharing.rs | 8 +++-- 17 files changed, 116 insertions(+), 54 deletions(-) diff --git a/synedrion/src/cggmp21/aux_gen.rs b/synedrion/src/cggmp21/aux_gen.rs index 10722d64..fb3f128c 100644 --- a/synedrion/src/cggmp21/aux_gen.rs +++ b/synedrion/src/cggmp21/aux_gen.rs @@ -626,6 +626,7 @@ impl Round for Round3 { } } +#[allow(clippy::indexing_slicing)] #[cfg(test)] mod tests { use alloc::collections::BTreeSet; diff --git a/synedrion/src/cggmp21/interactive_signing.rs b/synedrion/src/cggmp21/interactive_signing.rs index 0cd360d9..8d40ed32 100644 --- a/synedrion/src/cggmp21/interactive_signing.rs +++ b/synedrion/src/cggmp21/interactive_signing.rs @@ -887,18 +887,28 @@ impl Round for Round3 { .round2_artifacts .into_iter() .map(|(id, artifact)| { + let cap_k = self + .all_cap_k + .get(&id) + .ok_or_else(|| LocalError::new("id={id:?} is missing in all_cap_k"))? + .clone(); + let hat_cap_d_received = self + .hat_cap_ds + .get(&id) + .ok_or_else(|| LocalError::new("id={id:?} is missing in hat_cap_ds"))? + .clone(); let values = PresigningValues { hat_beta: artifact.hat_beta, hat_r: artifact.hat_r, hat_s: artifact.hat_s, - cap_k: self.all_cap_k[&id].clone(), - hat_cap_d_received: self.hat_cap_ds[&id].clone(), + cap_k, + hat_cap_d_received, hat_cap_d: artifact.hat_cap_d, hat_cap_f: artifact.hat_cap_f, }; - (id, values) + Ok((id, values)) }) - .collect(); + .collect::>()?; let presigning_data = PresigningData { nonce, @@ -1350,6 +1360,7 @@ impl Round for SigningErrorRound { } } +#[allow(clippy::indexing_slicing)] #[cfg(test)] mod tests { use alloc::collections::BTreeSet; diff --git a/synedrion/src/cggmp21/key_init.rs b/synedrion/src/cggmp21/key_init.rs index fbf8bb82..e4f8750c 100644 --- a/synedrion/src/cggmp21/key_init.rs +++ b/synedrion/src/cggmp21/key_init.rs @@ -458,6 +458,7 @@ impl Round for Round3 { } } +#[allow(clippy::indexing_slicing)] #[cfg(test)] mod tests { use alloc::collections::{BTreeMap, BTreeSet}; diff --git a/synedrion/src/cggmp21/key_refresh.rs b/synedrion/src/cggmp21/key_refresh.rs index 9ade4fb3..5d7fa820 100644 --- a/synedrion/src/cggmp21/key_refresh.rs +++ b/synedrion/src/cggmp21/key_refresh.rs @@ -774,6 +774,7 @@ impl Round for Round3 { } } +#[allow(clippy::indexing_slicing)] #[cfg(test)] mod tests { diff --git a/synedrion/src/cggmp21/params.rs b/synedrion/src/cggmp21/params.rs index d437a0bf..5c5a9e44 100644 --- a/synedrion/src/cggmp21/params.rs +++ b/synedrion/src/cggmp21/params.rs @@ -20,12 +20,14 @@ use crate::{ #[derive(Debug, Copy, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct PaillierTest; +#[allow(clippy::indexing_slicing)] const fn upcast_uint(value: K256Uint) -> K256Uint { assert!(N2 >= N1, "Upcast target must be bigger than the upcast candidate"); let mut result_words = [0; N2]; let mut i = 0; + let words = value.as_words(); while i < N1 { - result_words[i] = value.as_words()[i]; + result_words[i] = words[i]; i += 1; } K256Uint::from_words(result_words) @@ -132,8 +134,14 @@ pub trait SchemeParams: Debug + Clone + Send + PartialEq + Eq + Send + Sync + 's let uint_len = repr.as_ref().len(); let scalar_len = scalar_bytes.len(); - debug_assert!(uint_len >= scalar_len); - repr.as_mut()[uint_len - scalar_len..].copy_from_slice(&scalar_bytes); + debug_assert!( + uint_len >= scalar_len, + "PaillierParams::Uint is expected to be bigger than a Scalar" + ); + repr.as_mut() + .get_mut(uint_len - scalar_len..) + .expect("PaillierParams::Uint is expected to be bigger than a Scalar") + .copy_from_slice(&scalar_bytes); ::Uint::from_be_bytes(repr) } @@ -162,8 +170,12 @@ pub trait SchemeParams: Debug + Clone + Send + PartialEq + Eq + Send + Sync + 's let scalar_len = Scalar::repr_len(); // Can unwrap here since the value is within the Scalar range - Scalar::try_from_bytes(&repr.as_ref()[uint_len - scalar_len..]) - .expect("the value was reduced modulo `CURVE_ORDER`, so it's a valid curve scalar") + Scalar::try_from_bytes( + repr.as_ref() + .get(uint_len - scalar_len..) + .expect("Uint is assumed to be bigger than Scalar"), + ) + .expect("the value was reduced modulo `CURVE_ORDER`, so it's a valid curve scalar") } /// Converts a `Signed`-wrapped integer to the associated curve scalar type. @@ -181,8 +193,13 @@ pub trait SchemeParams: Debug + Clone + Send + PartialEq + Eq + Send + Sync + 's let scalar_len = Scalar::repr_len(); // Can unwrap here since the value is within the Scalar range - Scalar::try_from_bytes(&repr.as_ref()[uint_len - scalar_len..]) - .expect("the value was reduced modulo `CURVE_ORDER`, so it's a valid curve scalar") + Scalar::try_from_bytes( + repr.as_ref() + .get(uint_len - scalar_len..) + // TODO(dp): Do I need a better proof that this is true? + .expect("WideUint is assumed to be bigger than Scalar"), + ) + .expect("the value was reduced modulo `CURVE_ORDER`, so it's a valid curve scalar") } /// Converts a `Signed`-wrapped wide integer to the associated curve scalar type. diff --git a/synedrion/src/cggmp21/sigma/mod_.rs b/synedrion/src/cggmp21/sigma/mod_.rs index d687ca6c..059a63b3 100644 --- a/synedrion/src/cggmp21/sigma/mod_.rs +++ b/synedrion/src/cggmp21/sigma/mod_.rs @@ -80,14 +80,15 @@ impl ModProof

{ let (omega_mod_p, omega_mod_q) = sk.rns_split(&commitment.0); - let proof = (0..challenge.0.len()) - .map(|i| { + let proof = challenge + .0 + .iter() + .map(|y| { let mut y_sqrt = None; let mut found_a = false; let mut found_b = false; for (a, b) in [(false, false), (false, true), (true, false), (true, true)].iter() { - let y = challenge.0[i]; - let (mut y_mod_p, mut y_mod_q) = sk.rns_split(&y); + let (mut y_mod_p, mut y_mod_q) = sk.rns_split(y); if *a { y_mod_p = -y_mod_p; y_mod_q = -y_mod_q; @@ -114,7 +115,7 @@ impl ModProof

{ let y_4th = sk.rns_join(&y_4th_parts); - let y = challenge.0[i].to_montgomery(pk.monty_params_mod_n()); + let y = y.to_montgomery(pk.monty_params_mod_n()); let sk_inv_modulus = sk.inv_modulus(); let z = y.pow_bounded(sk_inv_modulus.expose_secret()); diff --git a/synedrion/src/cggmp21/sigma/prm.rs b/synedrion/src/cggmp21/sigma/prm.rs index bf799dea..82740255 100644 --- a/synedrion/src/cggmp21/sigma/prm.rs +++ b/synedrion/src/cggmp21/sigma/prm.rs @@ -122,12 +122,10 @@ impl PrmProof

{ return false; } - for i in 0..challenge.0.len() { - let z = self.proof[i]; - let e = challenge.0[i]; - let a = self.commitment.0[i].to_montgomery(monty_params); - let pwr = setup.base().pow_bounded(&z); - let test = if e { pwr == a * setup.power() } else { pwr == a }; + for ((e, z), a) in challenge.0.iter().zip(self.proof.iter()).zip(self.commitment.0.iter()) { + let a = a.to_montgomery(monty_params); + let pwr = setup.base().pow_bounded(z); + let test = if *e { pwr == a * setup.power() } else { pwr == a }; if !test { return false; } diff --git a/synedrion/src/cggmp21/signing_malicious.rs b/synedrion/src/cggmp21/signing_malicious.rs index 533b2192..e2bc67b8 100644 --- a/synedrion/src/cggmp21/signing_malicious.rs +++ b/synedrion/src/cggmp21/signing_malicious.rs @@ -57,6 +57,7 @@ impl Misbehaving for MaliciousSignin type MaliciousSigning = MisbehavingEntryPoint>; +#[allow(clippy::indexing_slicing)] #[test] fn execute_signing() { let signers = (0..3).map(TestSigner::new).collect::>(); diff --git a/synedrion/src/curve/arithmetic.rs b/synedrion/src/curve/arithmetic.rs index 5bd84ba8..63c93aac 100644 --- a/synedrion/src/curve/arithmetic.rs +++ b/synedrion/src/curve/arithmetic.rs @@ -116,6 +116,7 @@ impl Scalar { Self(*sk.as_nonzero_scalar().as_ref()) } + // TODO(dp): Does this assume big endian order? pub(crate) fn try_from_bytes(bytes: &[u8]) -> Result { let arr = GenericArray::>::from_exact_iter(bytes.iter().cloned()) .ok_or("Invalid length of a curve scalar")?; diff --git a/synedrion/src/paillier/rsa.rs b/synedrion/src/paillier/rsa.rs index 16ea7277..8cfbb45e 100644 --- a/synedrion/src/paillier/rsa.rs +++ b/synedrion/src/paillier/rsa.rs @@ -14,7 +14,7 @@ use crate::{ fn random_paillier_blum_prime(rng: &mut impl CryptoRngCore) -> P::HalfUint { loop { let prime = P::HalfUint::generate_prime_with_rng(rng, P::PRIME_BITS); - if prime.as_ref()[0].0 & 3 == 3 { + if prime.as_ref().first().expect("First Limb exists").0 & 3 == 3 { return prime; } } @@ -34,8 +34,14 @@ pub(crate) struct SecretPrimesWire { impl SecretPrimesWire

{ /// A single constructor to check the invariants fn new(p: Secret, q: Secret) -> Self { - debug_assert!(p.expose_secret().as_ref()[0].0 & 3 == 3, "p must be 3 mod 4"); - debug_assert!(q.expose_secret().as_ref()[0].0 & 3 == 3, "q must be 3 mod 4"); + debug_assert!( + p.expose_secret().as_ref().first().expect("First Limb exists").0 & 3 == 3, + "p must be 3 mod 4" + ); + debug_assert!( + q.expose_secret().as_ref().first().expect("First Limb exists").0 & 3 == 3, + "q must be 3 mod 4" + ); Self { p, q } } diff --git a/synedrion/src/tools/bitvec.rs b/synedrion/src/tools/bitvec.rs index cb5b02df..f5717bea 100644 --- a/synedrion/src/tools/bitvec.rs +++ b/synedrion/src/tools/bitvec.rs @@ -20,8 +20,8 @@ impl BitVec { impl BitXorAssign<&BitVec> for BitVec { fn bitxor_assign(&mut self, rhs: &BitVec) { assert!(self.0.len() == rhs.0.len()); - for i in 0..self.0.len() { - self.0[i] ^= rhs.0[i]; + for (lhs, rhs) in self.0.iter_mut().zip(rhs.0.iter()) { + *lhs ^= rhs } } } diff --git a/synedrion/src/tools/hashing.rs b/synedrion/src/tools/hashing.rs index 3cc674a6..11ae3383 100644 --- a/synedrion/src/tools/hashing.rs +++ b/synedrion/src/tools/hashing.rs @@ -155,25 +155,32 @@ where let backend_modulus = modulus.as_ref(); let n_bits = backend_modulus.bits_vartime(); - let n_bytes = (n_bits + 7) / 8; // ceiling division by 8 + let n_bytes = n_bits.div_ceil(8) as usize; - // If the number of bits is not a multiple of 8, - // use a mask to zeroize the high bits in the gererated random bytestring, - // so that we don't have to reject too much. + // If the number of bits is not a multiple of 8, use a mask to zeroize the high bits in the + // gererated random bytestring, so that we don't have to reject too much. let mask = if n_bits & 7 != 0 { (1 << (n_bits & 7)) - 1 } else { u8::MAX }; + // TODO(dp): this uses `to_le_bytes` (little-endian) but then the original code masks out bits + // from the *last* byte, so it'd mask out the *low* bits yeah? Changing to mask the first byte + // but this could use a test. let mut bytes = T::zero().to_le_bytes(); loop { - reader.read(&mut (bytes.as_mut()[0..n_bytes as usize])); - bytes.as_mut()[n_bytes as usize - 1] &= mask; - let n = T::from_le_bytes(bytes); - - if n.ct_lt(backend_modulus).into() { - return n; + if let Some(buf) = bytes.as_mut().get_mut(0..n_bytes) { + reader.read(buf); + bytes.as_mut().first_mut().map(|byte| { + *byte &= mask; + Some(byte) + }); + let n = T::from_le_bytes(bytes); + + if n.ct_lt(backend_modulus).into() { + return n; + } } } } diff --git a/synedrion/src/tools/sss.rs b/synedrion/src/tools/sss.rs index 23277ad1..e0b507c4 100644 --- a/synedrion/src/tools/sss.rs +++ b/synedrion/src/tools/sss.rs @@ -3,6 +3,7 @@ use alloc::{ vec::Vec, }; use core::ops::{Add, Mul}; +use manul::session::LocalError; use rand_core::CryptoRngCore; use serde::{Deserialize, Serialize}; @@ -34,13 +35,15 @@ fn evaluate_polynomial(coeffs: &[T], x: &Scalar) -> T where T: Copy + Add + for<'a> Mul<&'a Scalar, Output = T>, { + assert!(coeffs.len() > 1, "Expected coefficients to be non-empty"); // Evaluate in reverse to save on multiplications. // Basically: a0 + a1 x + a2 x^2 + a3 x^3 == (((a3 x) + a2) x + a1) x + a0 - let mut res = coeffs[coeffs.len() - 1]; - for i in (0..(coeffs.len() - 1)).rev() { - res = res * x + coeffs[i]; - } - res + + let (acc, coeffs) = coeffs.split_last().expect("Coefficients is not empty"); + coeffs.iter().rev().fold(*acc, |mut acc, coeff| { + acc = acc * x + *coeff; + acc + }) } #[derive(Debug, ZeroizeOnDrop)] @@ -73,8 +76,10 @@ impl PublicPolynomial { evaluate_polynomial(&self.0, &x.0) } - pub fn coeff0(&self) -> Point { - self.0[0] + pub fn coeff0(&self) -> Result<&Point, LocalError> { + self.0 + .first() + .ok_or_else(|| LocalError::new("Invalid PublicPolynomial")) } } @@ -116,6 +121,7 @@ pub(crate) fn shamir_join_points(pairs: &BTreeMap) -> Point { .sum() } +#[allow(clippy::indexing_slicing)] #[cfg(test)] mod tests { use rand_core::OsRng; diff --git a/synedrion/src/uint/bounded.rs b/synedrion/src/uint/bounded.rs index adeedaa1..a6ba5a27 100644 --- a/synedrion/src/uint/bounded.rs +++ b/synedrion/src/uint/bounded.rs @@ -27,7 +27,10 @@ where fn from(val: Bounded) -> Self { let repr = val.as_ref().to_be_bytes(); let bound_bytes = (val.bound() + 7) / 8; - let slice = &repr.as_ref()[(repr.as_ref().len() - bound_bytes as usize)..]; + let slice = repr + .as_ref() + .get((repr.as_ref().len() - bound_bytes as usize)..) + .expect("val has a valid bound that was checked when it was created"); Self { bound: val.bound(), bytes: slice.into(), @@ -52,7 +55,10 @@ where )); } - repr.as_mut()[(repr_len - bytes_len)..].copy_from_slice(&val.bytes); + repr.as_mut() + .get_mut((repr_len - bytes_len)..) + .expect("Just checked that val's data all fit in a T") + .copy_from_slice(&val.bytes); let abs_value = T::from_be_bytes(repr); Self::new(abs_value, val.bound).ok_or_else(|| "Invalid values for the signed integer".into()) diff --git a/synedrion/src/uint/traits.rs b/synedrion/src/uint/traits.rs index 1369ca47..e9745306 100644 --- a/synedrion/src/uint/traits.rs +++ b/synedrion/src/uint/traits.rs @@ -1,8 +1,7 @@ use crypto_bigint::{ modular::MontyForm, - nlimbs, subtle::{ConditionallySelectable, CtOption}, - Encoding, Integer, Invert, PowBoundedExp, RandomMod, Square, Zero, U1024, U2048, U4096, U512, U8192, + Encoding, Integer, Invert, Limb, PowBoundedExp, RandomMod, Square, Zero, U1024, U2048, U4096, U512, U8192, }; use crate::uint::{Bounded, Signed}; @@ -220,10 +219,11 @@ impl HasWide for U4096 { } } -pub type U512Mod = MontyForm<{ nlimbs!(512) }>; -pub type U1024Mod = MontyForm<{ nlimbs!(1024) }>; -pub type U2048Mod = MontyForm<{ nlimbs!(2048) }>; -pub type U4096Mod = MontyForm<{ nlimbs!(4096) }>; +// TODO(dp): Suggest crypto-bigint update nlimbs! macro. +pub type U512Mod = MontyForm<{ 512u32.div_ceil(Limb::BITS) as usize }>; +pub type U1024Mod = MontyForm<{ 1024u32.div_ceil(Limb::BITS) as usize }>; +pub type U2048Mod = MontyForm<{ 2048u32.div_ceil(Limb::BITS) as usize }>; +pub type U4096Mod = MontyForm<{ 4096u32.div_ceil(Limb::BITS) as usize }>; impl ToMontgomery for U512 {} impl ToMontgomery for U1024 {} diff --git a/synedrion/src/www02/entities.rs b/synedrion/src/www02/entities.rs index 4419d7d8..a011243c 100644 --- a/synedrion/src/www02/entities.rs +++ b/synedrion/src/www02/entities.rs @@ -326,6 +326,7 @@ fn apply_tweaks_private(private_key: SigningKey, tweaks: &[PrivateKeyBytes]) -> Ok(private_key) } +#[allow(clippy::indexing_slicing)] #[cfg(test)] mod tests { use alloc::collections::BTreeSet; diff --git a/synedrion/src/www02/key_resharing.rs b/synedrion/src/www02/key_resharing.rs index 09ceec9d..fcd49fd1 100644 --- a/synedrion/src/www02/key_resharing.rs +++ b/synedrion/src/www02/key_resharing.rs @@ -395,9 +395,12 @@ impl Round for Round1 { let vkey = payloads .values() .map(|payload| { - payload.public_polynomial.coeff0() * interpolation_coeff(&old_share_ids, &payload.old_share_id) + payload + .public_polynomial + .coeff0() + .map(|coeff0| coeff0 * &interpolation_coeff(&old_share_ids, &payload.old_share_id)) }) - .sum(); + .sum::>()?; if Point::from_verifying_key(&new_holder.inputs.verifying_key) != vkey { // TODO (#113): this is unattributable. // Should we add an enum variant to `FinalizeError`? @@ -444,6 +447,7 @@ impl Round for Round1 { } } +#[allow(clippy::indexing_slicing)] #[cfg(test)] mod tests { use alloc::collections::{BTreeMap, BTreeSet}; From 4e74fda2312fa51bb2b3c9583d9618fe049c0ab7 Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 9 Dec 2024 15:55:00 +0100 Subject: [PATCH 3/9] More slice indexing --- synedrion/src/cggmp21/interactive_signing.rs | 128 ++++++++++--------- synedrion/src/cggmp21/key_refresh.rs | 88 ++++++++++--- 2 files changed, 134 insertions(+), 82 deletions(-) diff --git a/synedrion/src/cggmp21/interactive_signing.rs b/synedrion/src/cggmp21/interactive_signing.rs index 8d40ed32..2cafebf5 100644 --- a/synedrion/src/cggmp21/interactive_signing.rs +++ b/synedrion/src/cggmp21/interactive_signing.rs @@ -915,7 +915,11 @@ impl Round for Round3 { ephemeral_scalar_share: SecretBox::new(Box::new(self.context.k)), product_share: SecretBox::new(Box::new(P::scalar_from_signed(&self.chi))), product_share_nonreduced: self.chi, - cap_k: self.all_cap_k[&my_id].clone(), + cap_k: self + .all_cap_k + .get(&my_id) + .ok_or_else(|| LocalError::new("m_id={my_id:?} is missing in all_cap_k"))? + .clone(), values, }; @@ -938,16 +942,19 @@ impl Round for Round3 { let cap_gamma = self.context.gamma.mul_by_generator(); - for id_j in self.context.other_ids.iter() { - let r2_artefacts = &self.round2_artifacts[id_j]; - - for id_l in self.context.other_ids.iter().filter(|id| id != &id_j) { - let target_pk = &self.context.aux_info.public_aux[id_j].paillier_pk; - let rp = &self.context.aux_info.public_aux[id_l].rp_params; + for (id_j, (_, r2_artifacts)) in self.context.other_ids.iter().zip(self.round2_artifacts.iter()) { + let cap_c = self + .all_cap_k + .get(id_j) + .ok_or_else(|| LocalError::new("id_j={id_j:?} is missing in all_cap_k"))?; + for id_l in self.context.other_ids.iter().filter(|id| *id != id_j) { + let paux = self.public_aux(id_j)?; + let target_pk = &paux.paillier_pk; + let rp = &paux.rp_params; - let beta = &self.round2_artifacts[id_j].beta; - let r = &self.round2_artifacts[id_j].r; - let s = &self.round2_artifacts[id_j].s; + let beta = &r2_artifacts.beta; + let r = &r2_artifacts.r; + let s = &r2_artifacts.s; let p_aff_g = AffGProof::

::new( rng, @@ -957,9 +964,9 @@ impl Round for Round3 { r.to_precomputed(pk), target_pk, pk, - &self.all_cap_k[id_j], - &r2_artefacts.cap_d, - &r2_artefacts.cap_f, + cap_c, + &r2_artifacts.cap_d, + &r2_artifacts.cap_f, &cap_gamma, rp, &aux, @@ -968,9 +975,9 @@ impl Round for Round3 { assert!(p_aff_g.verify( target_pk, pk, - &self.all_cap_k[id_j], - &r2_artefacts.cap_d, - &r2_artefacts.cap_f, + cap_c, + &r2_artifacts.cap_d, + &r2_artifacts.cap_f, &cap_gamma, rp, &aux, @@ -981,10 +988,17 @@ impl Round for Round3 { } // Mul proof - + let my_id = &self.context.my_id; let rho = Randomizer::random(rng, pk); - let cap_h = (&self.all_cap_g[&self.context.my_id] * P::bounded_from_scalar(&self.context.k)) - .mul_randomizer(&rho.to_wire()); + let cap_x = self + .all_cap_k + .get(my_id) + .ok_or_else(|| LocalError::new("my_id={my_id:?} is missing in all_cap_k"))?; + let cap_y = self + .all_cap_g + .get(my_id) + .ok_or_else(|| LocalError::new("my_id={my_id:?} is missing in all_cap_g"))?; + let cap_h = (cap_y * P::bounded_from_scalar(&self.context.k)).mul_randomizer(&rho.to_wire()); let p_mul = MulProof::

::new( rng, @@ -992,18 +1006,12 @@ impl Round for Round3 { &self.context.rho, &rho, pk, - &self.all_cap_k[&self.context.my_id], - &self.all_cap_g[&self.context.my_id], + cap_x, + cap_y, &cap_h, &aux, ); - assert!(p_mul.verify( - pk, - &self.all_cap_k[&self.context.my_id], - &self.all_cap_g[&self.context.my_id], - &cap_h, - &aux - )); + assert!(p_mul.verify(pk, cap_x, cap_y, &cap_h, &aux)); // Dec proof @@ -1032,16 +1040,10 @@ impl Round for Round3 { pk, &scalar_delta, &ciphertext, - &self.context.aux_info.public_aux[id_j].rp_params, + &self.public_aux(id_j)?.rp_params, &aux, ); - assert!(p_dec.verify( - pk, - &scalar_delta, - &ciphertext, - &self.context.aux_info.public_aux[id_j].rp_params, - &aux - )); + assert!(p_dec.verify(pk, &scalar_delta, &ciphertext, &self.public_aux(id_j)?.rp_params, &aux)); dec_proofs.push((id_j.clone(), p_dec)); } @@ -1073,6 +1075,21 @@ where sigma, } } + fn public_aux(&self, i: &I) -> Result<&PublicAuxInfoPrecomputed

, LocalError> { + self.context + .aux_info + .public_aux + .get(i) + .ok_or_else(|| LocalError::new("Missing public_aux for party Id {i:?}")) + } + + fn public_shares(&self, i: &I) -> Result<&Point, LocalError> { + self.context + .key_share + .public_shares + .get(i) + .ok_or_else(|| LocalError::new("Missing public_aux for party Id {i:?}")) + } } #[derive(Clone, Serialize, Deserialize)] @@ -1165,15 +1182,15 @@ impl Round for Round4 { let mut aff_g_proofs = Vec::new(); for id_j in self.context.other_ids.iter() { - for id_l in self.context.other_ids.iter().filter(|id| id != &id_j) { - let target_pk = &self.context.aux_info.public_aux[id_j].paillier_pk; - let rp = &self.context.aux_info.public_aux[id_l].rp_params; + for id_l in self.context.other_ids.iter().filter(|id| *id != id_j) { + let target_pk = &self.public_aux(id_j)?.paillier_pk; + let rp = &self.public_aux(id_l)?.rp_params; let values = self .presigning .values .get(id_j) - .ok_or_else(|| LocalError::new(format!("Missing presigning values for {id_j:?}")))?; + .ok_or_else(|| LocalError::new("Missing presigning values for {id_j:?}"))?; let p_aff_g = AffGProof::

::new( rng, @@ -1186,7 +1203,7 @@ impl Round for Round4 { &values.cap_k, &values.hat_cap_d, &values.hat_cap_f, - &self.context.key_share.public_shares[&my_id], + self.public_shares(&my_id)?, rp, &aux, ); @@ -1197,7 +1214,7 @@ impl Round for Round4 { &values.cap_k, &values.hat_cap_d, &values.hat_cap_f, - &self.context.key_share.public_shares[&my_id], + self.public_shares(&my_id)?, rp, &aux, )); @@ -1209,7 +1226,7 @@ impl Round for Round4 { // mul* proofs let x = &self.context.key_share.secret_share; - let cap_x = self.context.key_share.public_shares[&my_id]; + let cap_x = self.public_shares(&my_id)?; let rho = Randomizer::random(rng, pk); let hat_cap_h = @@ -1220,6 +1237,7 @@ impl Round for Round4 { let mut mul_star_proofs = Vec::new(); for id_l in self.context.other_ids.iter() { + let paux = self.public_aux(id_l)?; let p_mul = MulStarProof::

::new( rng, &P::signed_from_scalar(x.expose_secret()), @@ -1227,19 +1245,12 @@ impl Round for Round4 { pk, &self.presigning.cap_k, &hat_cap_h, - &cap_x, - &self.context.aux_info.public_aux[id_l].rp_params, + cap_x, + &paux.rp_params, &aux, ); - assert!(p_mul.verify( - pk, - &self.presigning.cap_k, - &hat_cap_h, - &cap_x, - &self.context.aux_info.public_aux[id_l].rp_params, - &aux, - )); + assert!(p_mul.verify(pk, &self.presigning.cap_k, &hat_cap_h, cap_x, &paux.rp_params, &aux,)); mul_star_proofs.push((id_l.clone(), p_mul)); } @@ -1270,6 +1281,7 @@ impl Round for Round4 { let mut dec_proofs = Vec::new(); for id_l in self.context.other_ids.iter() { + let paux = self.public_aux(id_l)?; let p_dec = DecProof::

::new( rng, &s_part_nonreduced, @@ -1277,16 +1289,10 @@ impl Round for Round4 { pk, &self.sigma, &ciphertext, - &self.context.aux_info.public_aux[id_l].rp_params, + &paux.rp_params, &aux, ); - assert!(p_dec.verify( - pk, - &self.sigma, - &ciphertext, - &self.context.aux_info.public_aux[id_l].rp_params, - &aux, - )); + assert!(p_dec.verify(pk, &self.sigma, &ciphertext, &paux.rp_params, &aux,)); dec_proofs.push((id_l.clone(), p_dec)); } diff --git a/synedrion/src/cggmp21/key_refresh.rs b/synedrion/src/cggmp21/key_refresh.rs index 5d7fa820..05e52436 100644 --- a/synedrion/src/cggmp21/key_refresh.rs +++ b/synedrion/src/cggmp21/key_refresh.rs @@ -599,19 +599,39 @@ impl Round for Round3 { let phi = FacProof::new(rng, &self.context.paillier_sk, &data.rp_params, &aux); - let destination_idx = self.context.ids_ordering[destination]; + let destination_idx = *self + .context + .ids_ordering + .get(destination) + .ok_or_else(|| LocalError::new("destination={destination:?} is missing in ids_ordering"))?; - let x_secret = self.context.x_to_send[destination]; - let x_public = self.context.data_precomp.data.cap_x_to_send[destination_idx]; - let ciphertext = Ciphertext::new(rng, &data.paillier_pk, &P::uint_from_scalar(&x_secret)); + let x_secret = self + .context + .x_to_send + .get(destination) + .ok_or_else(|| LocalError::new("destination={destination} is missing in x_to_send"))?; + let x_public = self + .context + .data_precomp + .data + .cap_x_to_send + .get(destination_idx) + .ok_or_else(|| LocalError::new("destination_idx={destination_idx} is missing in cap_x_to_send"))?; + let ciphertext = Ciphertext::new(rng, &data.paillier_pk, &P::uint_from_scalar(x_secret)); + let proof_secret = self + .context + .tau_x + .get(destination) + .ok_or_else(|| LocalError::new("destination_idx={destination_idx} is missing in tau_x"))?; + let commitment = self + .context + .data_precomp + .data + .cap_a_to_send + .get(destination_idx) + .ok_or_else(|| LocalError::new("destination_idx={destination_idx} is missing in cap_a_to_send"))?; - let psi_sch = SchProof::new( - &self.context.tau_x[destination], - &x_secret, - &self.context.data_precomp.data.cap_a_to_send[destination_idx], - &x_public, - &aux, - ); + let psi_sch = SchProof::new(proof_secret, x_secret, commitment, x_public, &aux); let data2 = PublicData2 { psi_mod: self.psi_mod.clone(), @@ -650,9 +670,19 @@ impl Round for Round3 { let x = P::scalar_from_uint(&enc_x.decrypt(&self.context.paillier_sk)); - let my_idx = self.context.ids_ordering[&self.context.my_id]; - - if x.mul_by_generator() != sender_data.data.cap_x_to_send[my_idx] { + let my_idx = *self + .context + .ids_ordering + .get(&self.context.my_id) + .ok_or_else(|| LocalError::new(format!("my_id={:?} is missing in ids_ordering", self.context.my_id)))?; + + if x.mul_by_generator() + != *sender_data + .data + .cap_x_to_send + .get(my_idx) + .ok_or_else(|| LocalError::new("my_idx={my_idx} is missing in cap_x_to_send"))? + { let mu = enc_x.derive_randomizer(&self.context.paillier_sk); return Err(ReceiveError::protocol(KeyRefreshError( KeyRefreshErrorEnum::Round3MismatchedSecret { @@ -692,8 +722,16 @@ impl Round for Round3 { } if !direct_message.data2.psi_sch.verify( - &sender_data.data.cap_a_to_send[my_idx], - &sender_data.data.cap_x_to_send[my_idx], + sender_data + .data + .cap_a_to_send + .get(my_idx) + .ok_or_else(|| LocalError::new("my_idx={my_idx} is missing in cap_a_to_send"))?, + sender_data + .data + .cap_x_to_send + .get(my_idx) + .ok_or_else(|| LocalError::new("my_idx={my_idx} is missing in cap_a_to_send"))?, &aux, ) { return Err(ReceiveError::protocol(KeyRefreshError(KeyRefreshErrorEnum::Round3( @@ -717,7 +755,11 @@ impl Round for Round3 { .collect::>(); // The combined secret share change - let x_star = others_x.values().sum::() + self.context.x_to_send[&self.context.my_id]; + let x_star = + others_x.values().sum::() + + *self.context.x_to_send.get(&self.context.my_id).ok_or_else(|| { + LocalError::new(format!("my_id={:?} is missing in x_to_send", self.context.my_id)) + })?; let my_id = self.context.my_id.clone(); let mut all_ids = self.context.other_ids; @@ -731,12 +773,16 @@ impl Round for Round3 { .iter() .enumerate() .map(|(idx, id)| { - ( + Ok(( id.clone(), - all_data.values().map(|data| data.data.cap_x_to_send[idx]).sum(), - ) + all_data + .values() + .map(|data| data.data.cap_x_to_send.get(idx)) + .sum::>() + .ok_or_else(|| LocalError::new("idx={idx} is missing in cap_x_to_send"))?, + )) }) - .collect(); + .collect::>()?; let public_aux = all_data .into_iter() From b04f7f651dfaae4dcc5c8d632cc073439899fe7c Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 9 Dec 2024 16:32:39 +0100 Subject: [PATCH 4/9] Resolve TODOs --- synedrion/src/cggmp21/entities.rs | 36 +++++---- synedrion/src/cggmp21/interactive_signing.rs | 2 +- synedrion/src/cggmp21/params.rs | 2 +- synedrion/src/curve/arithmetic.rs | 2 +- synedrion/src/tools/hashing.rs | 2 +- synedrion/src/www02/entities.rs | 82 ++++++++++---------- synedrion/src/www02/key_resharing.rs | 4 +- synedrion/tests/threshold.rs | 17 ++-- 8 files changed, 78 insertions(+), 69 deletions(-) diff --git a/synedrion/src/cggmp21/entities.rs b/synedrion/src/cggmp21/entities.rs index 3db76a96..bae444b0 100644 --- a/synedrion/src/cggmp21/entities.rs +++ b/synedrion/src/cggmp21/entities.rs @@ -1,9 +1,11 @@ use alloc::{ boxed::Box, collections::{BTreeMap, BTreeSet}, + format, vec::Vec, }; use core::{fmt::Debug, marker::PhantomData}; +use manul::session::LocalError; use k256::ecdsa::VerifyingKey; use rand_core::CryptoRngCore; @@ -126,38 +128,40 @@ pub(crate) struct PresigningValues { impl KeyShare { /// Updates a key share with a change obtained from KeyRefresh protocol. - pub fn update(self, change: KeyShareChange) -> Self { - // TODO (#68): check that party_idx is the same for both, and the number of parties is the same - assert_eq!(self.owner, change.owner); + pub fn update(self, change: KeyShareChange) -> Result { + if self.owner != change.owner { + return Err(LocalError::new(format!( + "Owning party mismatch. self.owner={:?}, change.owner={:?}", + self.owner, change.owner + ))); + } + if self.public_shares.len() != change.public_share_changes.len() { + return Err(LocalError::new(format!( + "Inconsistent number of public key shares in updated share set (expected {}, was {})", + self.public_shares.len(), + change.public_share_changes.len() + ))); + } let secret_share = SecretBox::new(Box::new( self.secret_share.expose_secret() + change.secret_share_change.expose_secret(), )); - // TODO(dp): return error instead? - assert_eq!( - self.public_shares.len(), - change.public_share_changes.len(), - "Inconsistent number of public key shares in updated share set (expected {}, was {})", - self.public_shares.len(), - change.public_share_changes.len() - ); let public_shares = self .public_shares .iter() .zip(change.public_share_changes) // TODO(dp): this should fail, I'm pretty sure, but doesn't (no test) - // let hh = change.public_share_changes.first_key_value().unwrap().0.clone(); - // .map(|(pub_share, changed_pub_share)| (hh.clone(), pub_share.1 + &changed_pub_share.1)) - // TODO(dp): is this correct? No test! + // let obviously_wrong_value = change.public_share_changes.first_key_value().unwrap().0.clone(); + // .map(|(pub_share, changed_pub_share)| (obviously_wrong_value.clone(), pub_share.1 + &changed_pub_share.1)) .map(|(pub_share, changed_pub_share)| (changed_pub_share.0, pub_share.1 + &changed_pub_share.1)) .collect(); - Self { + Ok(Self { owner: self.owner, secret_share, public_shares, phantom: PhantomData, - } + }) } /// Creates a set of random self-consistent key shares diff --git a/synedrion/src/cggmp21/interactive_signing.rs b/synedrion/src/cggmp21/interactive_signing.rs index 2cafebf5..fc5b4619 100644 --- a/synedrion/src/cggmp21/interactive_signing.rs +++ b/synedrion/src/cggmp21/interactive_signing.rs @@ -376,7 +376,7 @@ struct Round2 { all_cap_g: BTreeMap>, } -// TODO(dp): convenient enough to include in `Round`? +// TODO(dp): @reviewers convenient enough to include in `Round`? impl Round2 { fn public_aux(&self, i: &I) -> Result<&PublicAuxInfoPrecomputed

, LocalError> { self.context diff --git a/synedrion/src/cggmp21/params.rs b/synedrion/src/cggmp21/params.rs index 5c5a9e44..2fab86e5 100644 --- a/synedrion/src/cggmp21/params.rs +++ b/synedrion/src/cggmp21/params.rs @@ -196,7 +196,7 @@ pub trait SchemeParams: Debug + Clone + Send + PartialEq + Eq + Send + Sync + 's Scalar::try_from_bytes( repr.as_ref() .get(uint_len - scalar_len..) - // TODO(dp): Do I need a better proof that this is true? + // TODO(dp): @reviewers Do we need a better proof that this is true? .expect("WideUint is assumed to be bigger than Scalar"), ) .expect("the value was reduced modulo `CURVE_ORDER`, so it's a valid curve scalar") diff --git a/synedrion/src/curve/arithmetic.rs b/synedrion/src/curve/arithmetic.rs index 63c93aac..31fe9833 100644 --- a/synedrion/src/curve/arithmetic.rs +++ b/synedrion/src/curve/arithmetic.rs @@ -116,7 +116,7 @@ impl Scalar { Self(*sk.as_nonzero_scalar().as_ref()) } - // TODO(dp): Does this assume big endian order? + // TODO(dp): @reviewers Does this assume big endian order? pub(crate) fn try_from_bytes(bytes: &[u8]) -> Result { let arr = GenericArray::>::from_exact_iter(bytes.iter().cloned()) .ok_or("Invalid length of a curve scalar")?; diff --git a/synedrion/src/tools/hashing.rs b/synedrion/src/tools/hashing.rs index 11ae3383..edb00e50 100644 --- a/synedrion/src/tools/hashing.rs +++ b/synedrion/src/tools/hashing.rs @@ -165,7 +165,7 @@ where u8::MAX }; - // TODO(dp): this uses `to_le_bytes` (little-endian) but then the original code masks out bits + // TODO(dp): @reviewers this uses `to_le_bytes` (little-endian) but then the original code masks out bits // from the *last* byte, so it'd mask out the *low* bits yeah? Changing to mask the first byte // but this could use a test. let mut bytes = T::zero().to_le_bytes(); diff --git a/synedrion/src/www02/entities.rs b/synedrion/src/www02/entities.rs index a011243c..d8f7c28f 100644 --- a/synedrion/src/www02/entities.rs +++ b/synedrion/src/www02/entities.rs @@ -57,8 +57,13 @@ impl ThresholdKeyShare, threshold: usize, signing_key: Option<&SigningKey>, - ) -> BTreeMap { - debug_assert!(threshold <= ids.len()); // TODO (#68): make the method fallible + ) -> Result, LocalError> { + if threshold > ids.len() { + return Err(LocalError::new(format!( + "Invalid threshold ({threshold}). Must be smaller than {}", + ids.len() + ))); + } let secret = match signing_key { None => Scalar::random(rng), @@ -74,20 +79,19 @@ impl ThresholdKeyShare, LocalError>>() - .expect("TODO(dp): Return error"); + .collect::, LocalError>>()?; ids.iter() .map(|id| { let share_id = share_ids .get(id) - .ok_or(LocalError::new("id={id:?} is missing in the share_ids"))?; + .ok_or_else(|| LocalError::new("id={id:?} is missing in the share_ids"))?; let secret_share = secret_shares .get(share_id) - .ok_or(LocalError::new("share_id={share_id:?} is missing in the secret shares"))?; + .ok_or_else(|| LocalError::new("share_id={share_id:?} is missing in the secret shares"))?; Ok(( id.clone(), Self { @@ -100,12 +104,11 @@ impl ThresholdKeyShare>() - .expect("TODO(dp): Return LocalErr") + .collect() } - pub(crate) fn verifying_key_as_point(&self) -> Point { - shamir_join_points( + pub(crate) fn verifying_key_as_point(&self) -> Result { + Ok(shamir_join_points( &self .share_ids .iter() @@ -116,30 +119,28 @@ impl ThresholdKeyShare>() - .expect("TODO(dp): return LocalError"), - ) + .collect::>()?, + )) } /// Return the verifying key to which this set of shares corresponds. - pub fn verifying_key(&self) -> VerifyingKey { - self.verifying_key_as_point() + pub fn verifying_key(&self) -> Result { + self.verifying_key_as_point()? .to_verifying_key() - .expect("the combined verrifying key is not an identity") + .ok_or_else(|| LocalError::new("The combined verifying key is an identity")) } /// Converts a t-of-n key share into a t-of-t key share /// (for the `t` share indices supplied as `share_ids`) /// that can be used in the presigning/signing protocols. - pub fn to_key_share(&self, ids: &BTreeSet) -> KeyShare { + pub fn to_key_share(&self, ids: &BTreeSet) -> Result, LocalError> { debug_assert!(ids.len() == self.threshold as usize); debug_assert!(ids.iter().any(|id| id == &self.owner)); let owner_share_id = self .share_ids .get(&self.owner) - .ok_or(LocalError::new("id={id:?} is missing in the share_ids")) - .expect("TODO(dp): make method return LocalErr"); + .ok_or_else(|| LocalError::new("id={id:?} is missing in the share_ids"))?; let share_ids = ids .iter() @@ -147,11 +148,10 @@ impl ThresholdKeyShare, LocalError>>() - .expect("TODO(dp): make this method return LocalErr"); + .collect::, LocalError>>()?; let share_ids_set = share_ids.values().cloned().collect(); let secret_share = SecretBox::new(Box::new( @@ -163,25 +163,24 @@ impl ThresholdKeyShare>() - .expect("TODO(dp): make this method return LocalErr"); + .collect::>()?; - KeyShare { + Ok(KeyShare { owner: self.owner.clone(), secret_share, public_shares, phantom: PhantomData, - } + }) } /// Creates a t-of-t threshold keyshare that can be used in KeyResharing protocol. @@ -231,7 +230,8 @@ impl ThresholdKeyShare Result { - let tweaks = derive_tweaks(self.verifying_key(), derivation_path)?; + let pk = self.verifying_key().map_err(|_| bip32::Error::Crypto)?; + let tweaks = derive_tweaks(pk, derivation_path)?; // Will fail here if secret share is zero let secret_share = self @@ -274,7 +274,7 @@ pub trait DeriveChildKey { impl DeriveChildKey for ThresholdKeyShare { fn derive_verifying_key_bip32(&self, derivation_path: &DerivationPath) -> Result { - let public_key = self.verifying_key(); + let public_key = self.verifying_key().map_err(|_| bip32::Error::Crypto)?; let tweaks = derive_tweaks(public_key, derivation_path)?; apply_tweaks_public(public_key, &tweaks) } @@ -350,23 +350,25 @@ mod tests { let ids = signers.iter().map(|signer| signer.verifying_key()).collect::>(); let ids_set = ids.iter().cloned().collect::>(); - let shares = ThresholdKeyShare::::new_centralized(&mut OsRng, &ids_set, 2, Some(&sk)); + let shares = + ThresholdKeyShare::::new_centralized(&mut OsRng, &ids_set, 2, Some(&sk)).unwrap(); - assert_eq!(&shares[&ids[0]].verifying_key(), sk.verifying_key()); - assert_eq!(&shares[&ids[1]].verifying_key(), sk.verifying_key()); - assert_eq!(&shares[&ids[2]].verifying_key(), sk.verifying_key()); + let sk_verifying_key = sk.verifying_key(); + assert_eq!(&shares[&ids[0]].verifying_key().unwrap(), sk_verifying_key); + assert_eq!(&shares[&ids[1]].verifying_key().unwrap(), sk_verifying_key); + assert_eq!(&shares[&ids[2]].verifying_key().unwrap(), sk_verifying_key); - assert_eq!(&shares[&ids[0]].verifying_key(), sk.verifying_key()); + assert_eq!(&shares[&ids[0]].verifying_key().unwrap(), sk_verifying_key); let ids_subset = BTreeSet::from([ids[2], ids[0]]); - let nt_share0 = shares[&ids[0]].to_key_share(&ids_subset); - let nt_share1 = shares[&ids[2]].to_key_share(&ids_subset); + let nt_share0 = shares[&ids[0]].to_key_share(&ids_subset).unwrap(); + let nt_share1 = shares[&ids[2]].to_key_share(&ids_subset).unwrap(); assert_eq!( nt_share0.secret_share.expose_secret() + nt_share1.secret_share.expose_secret(), Scalar::from(sk.as_nonzero_scalar()) ); - assert_eq!(&nt_share0.verifying_key().unwrap(), sk.verifying_key()); - assert_eq!(&nt_share1.verifying_key().unwrap(), sk.verifying_key()); + assert_eq!(&nt_share0.verifying_key().unwrap(), sk_verifying_key); + assert_eq!(&nt_share1.verifying_key().unwrap(), sk_verifying_key); } } diff --git a/synedrion/src/www02/key_resharing.rs b/synedrion/src/www02/key_resharing.rs index fcd49fd1..0ae9dbf0 100644 --- a/synedrion/src/www02/key_resharing.rs +++ b/synedrion/src/www02/key_resharing.rs @@ -471,8 +471,8 @@ mod tests { let new_holders = BTreeSet::from([ids[1], ids[2], ids[3]]); let old_key_shares = - ThresholdKeyShare::::new_centralized(&mut OsRng, &old_holders, 2, None); - let old_vkey = old_key_shares[&ids[0]].verifying_key(); + ThresholdKeyShare::::new_centralized(&mut OsRng, &old_holders, 2, None).unwrap(); + let old_vkey = old_key_shares[&ids[0]].verifying_key().unwrap(); let new_threshold = 2; let party0 = KeyResharing::new( diff --git a/synedrion/tests/threshold.rs b/synedrion/tests/threshold.rs index f44ebfcd..0ec63c65 100644 --- a/synedrion/tests/threshold.rs +++ b/synedrion/tests/threshold.rs @@ -58,13 +58,13 @@ fn full_sequence() { // The full verifying key can be obtained both from the original key shares and child key shares let child_vkey = t_key_shares[&verifiers[0]].derive_verifying_key_bip32(&path).unwrap(); - assert_eq!(child_vkey, child_key_shares[&verifiers[0]].verifying_key()); + assert_eq!(child_vkey, child_key_shares[&verifiers[0]].verifying_key().unwrap()); // Reshare to `n` nodes // This will need to be published so that new holders can see it and verify the received data let new_holder = NewHolder { - verifying_key: t_key_shares[&verifiers[0]].verifying_key(), + verifying_key: t_key_shares[&verifiers[0]].verifying_key().unwrap(), old_threshold: t_key_shares[&verifiers[0]].threshold(), old_holders, }; @@ -108,8 +108,8 @@ fn full_sequence() { .collect::>(); assert_eq!( - new_t_key_shares[&verifiers[0]].verifying_key(), - t_key_shares[&verifiers[0]].verifying_key() + new_t_key_shares[&verifiers[0]].verifying_key().unwrap(), + t_key_shares[&verifiers[0]].verifying_key().unwrap() ); // Check that resharing did not change the derived child key @@ -143,15 +143,18 @@ fn full_sequence() { new_t_key_shares[&verifiers[0]] .derive_bip32(&path) .unwrap() - .to_key_share(&selected_parties), + .to_key_share(&selected_parties) + .unwrap(), new_t_key_shares[&verifiers[2]] .derive_bip32(&path) .unwrap() - .to_key_share(&selected_parties), + .to_key_share(&selected_parties) + .unwrap(), new_t_key_shares[&verifiers[4]] .derive_bip32(&path) .unwrap() - .to_key_share(&selected_parties), + .to_key_share(&selected_parties) + .unwrap(), ]; let selected_aux_infos = [ aux_infos[&verifiers[0]].clone(), From a98aea28241bd2d29d7839c58a25220900a4fae1 Mon Sep 17 00:00:00 2001 From: David Palm Date: Tue, 10 Dec 2024 09:45:56 +0100 Subject: [PATCH 5/9] Better syntax to enable lint on non-test code --- synedrion/src/cggmp21/aux_gen.rs | 1 - synedrion/src/cggmp21/interactive_signing.rs | 2 +- synedrion/src/cggmp21/key_init.rs | 1 - synedrion/src/cggmp21/key_refresh.rs | 1 - synedrion/src/cggmp21/signing_malicious.rs | 2 +- synedrion/src/lib.rs | 5 ++--- synedrion/src/tools/sss.rs | 1 - synedrion/src/www02/entities.rs | 1 - synedrion/src/www02/key_resharing.rs | 1 - 9 files changed, 4 insertions(+), 11 deletions(-) diff --git a/synedrion/src/cggmp21/aux_gen.rs b/synedrion/src/cggmp21/aux_gen.rs index fb3f128c..10722d64 100644 --- a/synedrion/src/cggmp21/aux_gen.rs +++ b/synedrion/src/cggmp21/aux_gen.rs @@ -626,7 +626,6 @@ impl Round for Round3 { } } -#[allow(clippy::indexing_slicing)] #[cfg(test)] mod tests { use alloc::collections::BTreeSet; diff --git a/synedrion/src/cggmp21/interactive_signing.rs b/synedrion/src/cggmp21/interactive_signing.rs index fc5b4619..987ac44a 100644 --- a/synedrion/src/cggmp21/interactive_signing.rs +++ b/synedrion/src/cggmp21/interactive_signing.rs @@ -1366,7 +1366,7 @@ impl Round for SigningErrorRound { } } -#[allow(clippy::indexing_slicing)] + #[cfg(test)] mod tests { use alloc::collections::BTreeSet; diff --git a/synedrion/src/cggmp21/key_init.rs b/synedrion/src/cggmp21/key_init.rs index e4f8750c..fbf8bb82 100644 --- a/synedrion/src/cggmp21/key_init.rs +++ b/synedrion/src/cggmp21/key_init.rs @@ -458,7 +458,6 @@ impl Round for Round3 { } } -#[allow(clippy::indexing_slicing)] #[cfg(test)] mod tests { use alloc::collections::{BTreeMap, BTreeSet}; diff --git a/synedrion/src/cggmp21/key_refresh.rs b/synedrion/src/cggmp21/key_refresh.rs index 05e52436..ed92f126 100644 --- a/synedrion/src/cggmp21/key_refresh.rs +++ b/synedrion/src/cggmp21/key_refresh.rs @@ -820,7 +820,6 @@ impl Round for Round3 { } } -#[allow(clippy::indexing_slicing)] #[cfg(test)] mod tests { diff --git a/synedrion/src/cggmp21/signing_malicious.rs b/synedrion/src/cggmp21/signing_malicious.rs index e2bc67b8..3a6d78e0 100644 --- a/synedrion/src/cggmp21/signing_malicious.rs +++ b/synedrion/src/cggmp21/signing_malicious.rs @@ -57,7 +57,7 @@ impl Misbehaving for MaliciousSignin type MaliciousSigning = MisbehavingEntryPoint>; -#[allow(clippy::indexing_slicing)] + #[test] fn execute_signing() { let signers = (0..3).map(TestSigner::new).collect::>(); diff --git a/synedrion/src/lib.rs b/synedrion/src/lib.rs index 7d3dff46..827c43b3 100644 --- a/synedrion/src/lib.rs +++ b/synedrion/src/lib.rs @@ -9,10 +9,9 @@ rust_2018_idioms, trivial_casts, trivial_numeric_casts, - unused_qualifications, - clippy::indexing_slicing + unused_qualifications )] -#![cfg_attr(not(test), warn(clippy::unwrap_used))] +#![cfg_attr(not(test), warn(clippy::unwrap_used, clippy::indexing_slicing))] extern crate alloc; diff --git a/synedrion/src/tools/sss.rs b/synedrion/src/tools/sss.rs index e0b507c4..1c411e97 100644 --- a/synedrion/src/tools/sss.rs +++ b/synedrion/src/tools/sss.rs @@ -121,7 +121,6 @@ pub(crate) fn shamir_join_points(pairs: &BTreeMap) -> Point { .sum() } -#[allow(clippy::indexing_slicing)] #[cfg(test)] mod tests { use rand_core::OsRng; diff --git a/synedrion/src/www02/entities.rs b/synedrion/src/www02/entities.rs index d8f7c28f..70f4ae76 100644 --- a/synedrion/src/www02/entities.rs +++ b/synedrion/src/www02/entities.rs @@ -326,7 +326,6 @@ fn apply_tweaks_private(private_key: SigningKey, tweaks: &[PrivateKeyBytes]) -> Ok(private_key) } -#[allow(clippy::indexing_slicing)] #[cfg(test)] mod tests { use alloc::collections::BTreeSet; diff --git a/synedrion/src/www02/key_resharing.rs b/synedrion/src/www02/key_resharing.rs index 0ae9dbf0..14b94fe7 100644 --- a/synedrion/src/www02/key_resharing.rs +++ b/synedrion/src/www02/key_resharing.rs @@ -447,7 +447,6 @@ impl Round for Round1 { } } -#[allow(clippy::indexing_slicing)] #[cfg(test)] mod tests { use alloc::collections::{BTreeMap, BTreeSet}; From 558c3f04a44698345b49f83645451c382820b86a Mon Sep 17 00:00:00 2001 From: David Palm Date: Tue, 10 Dec 2024 11:18:52 +0100 Subject: [PATCH 6/9] Cleanup --- synedrion/src/cggmp21/params.rs | 48 --------------------------------- 1 file changed, 48 deletions(-) diff --git a/synedrion/src/cggmp21/params.rs b/synedrion/src/cggmp21/params.rs index dc654538..84060cb4 100644 --- a/synedrion/src/cggmp21/params.rs +++ b/synedrion/src/cggmp21/params.rs @@ -285,54 +285,6 @@ pub(crate) fn secret_bounded_from_scalar( }) } -// <<<<<<< HEAD -// /// Converts an integer to the associated curve scalar type. -// fn scalar_from_uint(value: &::Uint) -> Scalar { -// let r = *value % Self::CURVE_ORDER; - -// let repr = r.to_be_bytes(); -// let uint_len = repr.as_ref().len(); -// let scalar_len = Scalar::repr_len(); - -// // Can unwrap here since the value is within the Scalar range -// Scalar::try_from_bytes( -// repr.as_ref() -// .get(uint_len - scalar_len..) -// .expect("Uint is assumed to be bigger than Scalar"), -// ) -// .expect("the value was reduced modulo `CURVE_ORDER`, so it's a valid curve scalar") -// } - -// /// Converts a `Signed`-wrapped integer to the associated curve scalar type. -// fn scalar_from_signed(value: &Signed<::Uint>) -> Scalar { -// let abs_value = Self::scalar_from_uint(&value.abs()); -// Scalar::conditional_select(&abs_value, &-abs_value, value.is_negative()) -// } - -// /// Converts a wide integer to the associated curve scalar type. -// fn scalar_from_wide_uint(value: &::WideUint) -> Scalar { -// let r = *value % Self::CURVE_ORDER_WIDE; - -// let repr = r.to_be_bytes(); -// let uint_len = repr.as_ref().len(); -// let scalar_len = Scalar::repr_len(); - -// // Can unwrap here since the value is within the Scalar range -// Scalar::try_from_bytes( -// repr.as_ref() -// .get(uint_len - scalar_len..) -// // TODO(dp): @reviewers Do we need a better proof that this is true? -// .expect("WideUint is assumed to be bigger than Scalar"), -// ) -// .expect("the value was reduced modulo `CURVE_ORDER`, so it's a valid curve scalar") -// } - -// /// Converts a `Signed`-wrapped wide integer to the associated curve scalar type. -// fn scalar_from_wide_signed(value: &Signed<::WideUint>) -> Scalar { -// let abs_value = Self::scalar_from_wide_uint(&value.abs()); -// Scalar::conditional_select(&abs_value, &-abs_value, value.is_negative()) -// } -// ======= pub(crate) fn secret_scalar_from_signed( value: &Secret::Uint>>, ) -> Secret { From c302023042a20083de76049a04d2b2eaa9640290 Mon Sep 17 00:00:00 2001 From: David Palm Date: Tue, 10 Dec 2024 23:06:47 +0100 Subject: [PATCH 7/9] fmt --- synedrion/src/cggmp21/signing_malicious.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/synedrion/src/cggmp21/signing_malicious.rs b/synedrion/src/cggmp21/signing_malicious.rs index 3a6d78e0..533b2192 100644 --- a/synedrion/src/cggmp21/signing_malicious.rs +++ b/synedrion/src/cggmp21/signing_malicious.rs @@ -57,7 +57,6 @@ impl Misbehaving for MaliciousSignin type MaliciousSigning = MisbehavingEntryPoint>; - #[test] fn execute_signing() { let signers = (0..3).map(TestSigner::new).collect::>(); From 204d3a96a67d72f94f0844c573acc90ec41ecaf0 Mon Sep 17 00:00:00 2001 From: David Palm Date: Wed, 11 Dec 2024 16:50:03 +0100 Subject: [PATCH 8/9] Review feedback --- synedrion/src/cggmp21/entities.rs | 17 ++-- synedrion/src/cggmp21/interactive_signing.rs | 81 ++++++++------------ 2 files changed, 44 insertions(+), 54 deletions(-) diff --git a/synedrion/src/cggmp21/entities.rs b/synedrion/src/cggmp21/entities.rs index e9463d8d..cb247d18 100644 --- a/synedrion/src/cggmp21/entities.rs +++ b/synedrion/src/cggmp21/entities.rs @@ -146,12 +146,17 @@ impl KeyShare { let public_shares = self .public_shares .iter() - .zip(change.public_share_changes) - // TODO(dp): this should fail, I'm pretty sure, but doesn't (no test) - // let obviously_wrong_value = change.public_share_changes.first_key_value().unwrap().0.clone(); - // .map(|(pub_share, changed_pub_share)| (obviously_wrong_value.clone(), pub_share.1 + &changed_pub_share.1)) - .map(|(pub_share, changed_pub_share)| (changed_pub_share.0, *pub_share.1 + changed_pub_share.1)) - .collect(); + .map(|(id, public_share)| { + Ok(( + id.clone(), + *public_share + + *change + .public_share_changes + .get(id) + .ok_or_else(|| LocalError::new("id={id:?} is missing in public_share_changes"))?, + )) + }) + .collect::>()?; Ok(Self { owner: self.owner, diff --git a/synedrion/src/cggmp21/interactive_signing.rs b/synedrion/src/cggmp21/interactive_signing.rs index f4a2dfd8..7ab056d8 100644 --- a/synedrion/src/cggmp21/interactive_signing.rs +++ b/synedrion/src/cggmp21/interactive_signing.rs @@ -200,6 +200,26 @@ struct Context { nu: Randomizer, } +impl Context +where + P: SchemeParams, + I: Ord + Debug, +{ + pub fn public_share(&self, i: &I) -> Result<&Point, LocalError> { + self.key_share + .public_shares + .get(i) + .ok_or_else(|| LocalError::new("Missing public_share for party Id {i:?}")) + } + + pub fn public_aux(&self, i: &I) -> Result<&PublicAuxInfoPrecomputed

, LocalError> { + self.aux_info + .public_aux + .get(i) + .ok_or_else(|| LocalError::new(format!("Missing public_aux for party Id {i:?}"))) + } +} + #[derive(Debug)] struct Round1 { context: Context, @@ -377,17 +397,6 @@ struct Round2 { all_cap_g: BTreeMap>, } -// TODO(dp): @reviewers convenient enough to include in `Round`? -impl Round2 { - fn public_aux(&self, i: &I) -> Result<&PublicAuxInfoPrecomputed

, LocalError> { - self.context - .aux_info - .public_aux - .get(i) - .ok_or_else(|| LocalError::new(format!("Missing public_aux for party Id {i:?}"))) - } -} - #[derive(Clone, Serialize, Deserialize)] #[serde(bound(serialize = " CiphertextWire: Serialize, @@ -462,7 +471,7 @@ impl Round for Round2 { let cap_gamma = self.context.gamma.mul_by_generator(); let pk = self.context.aux_info.secret_aux.paillier_sk.public_key(); - let target_pk = &self.public_aux(destination)?.paillier_pk; + let target_pk = &self.context.public_aux(destination)?.paillier_pk; let beta = Secret::init_with(|| Signed::random_bounded_bits(rng, P::LP_BOUND)); let hat_beta = Secret::init_with(|| Signed::random_bounded_bits(rng, P::LP_BOUND)); @@ -490,7 +499,7 @@ impl Round for Round2 { * secret_signed_from_scalar::

(&self.context.key_share.secret_share) + Ciphertext::new_with_randomizer_signed(target_pk, &-&hat_beta, &hat_s); - let rp = &self.public_aux(destination)?.rp_params; + let rp = &self.context.public_aux(destination)?.rp_params; let psi = AffGProof::new( rng, @@ -523,11 +532,7 @@ impl Round for Round2 { .ok_or(LocalError::new("destination={destination:?} is missing in all_cap_k"))?, &hat_cap_d, &hat_cap_f, - self.context - .key_share - .public_shares - .get(&self.context.my_id) - .ok_or(LocalError::new("my_id={my_id:?} missing in public_shares"))?, + self.context.public_share(&self.context.my_id)?, rp, &aux, ); @@ -592,16 +597,11 @@ impl Round for Round2 { let aux = (&self.context.ssid_hash, from); let pk = self.context.aux_info.secret_aux.paillier_sk.public_key(); - let from_pk = &self.public_aux(from)?.paillier_pk; + let from_pk = &self.context.public_aux(from)?.paillier_pk; - let cap_x = self - .context - .key_share - .public_shares - .get(from) - .ok_or(LocalError::new("from={from:?} is missing in public_shares"))?; + let cap_x = self.context.public_share(from)?; - let rp = &self.public_aux(&self.context.my_id)?.rp_params; + let rp = &self.context.public_aux(&self.context.my_id)?.rp_params; let cap_d = direct_message.cap_d.to_precomputed(pk); let hat_cap_d = direct_message.hat_cap_d.to_precomputed(pk); @@ -1090,21 +1090,6 @@ where sigma, } } - fn public_aux(&self, i: &I) -> Result<&PublicAuxInfoPrecomputed

, LocalError> { - self.context - .aux_info - .public_aux - .get(i) - .ok_or_else(|| LocalError::new("Missing public_aux for party Id {i:?}")) - } - - fn public_shares(&self, i: &I) -> Result<&Point, LocalError> { - self.context - .key_share - .public_shares - .get(i) - .ok_or_else(|| LocalError::new("Missing public_aux for party Id {i:?}")) - } } #[derive(Clone, Serialize, Deserialize)] @@ -1198,8 +1183,8 @@ impl Round for Round4 { for id_j in self.context.other_ids.iter() { for id_l in self.context.other_ids.iter().filter(|id| *id != id_j) { - let target_pk = &self.public_aux(id_j)?.paillier_pk; - let rp = &self.public_aux(id_l)?.rp_params; + let target_pk = &self.context.public_aux(id_j)?.paillier_pk; + let rp = &self.context.public_aux(id_l)?.rp_params; let values = self .presigning @@ -1218,7 +1203,7 @@ impl Round for Round4 { &values.cap_k, &values.hat_cap_d, &values.hat_cap_f, - self.public_shares(&my_id)?, + self.context.public_share(&my_id)?, rp, &aux, ); @@ -1229,7 +1214,7 @@ impl Round for Round4 { &values.cap_k, &values.hat_cap_d, &values.hat_cap_f, - self.public_shares(&my_id)?, + self.context.public_share(&my_id)?, rp, &aux, )); @@ -1241,7 +1226,7 @@ impl Round for Round4 { // mul* proofs let x = &self.context.key_share.secret_share; - let cap_x = self.public_shares(&my_id)?; + let cap_x = self.context.public_share(&my_id)?; let rho = Randomizer::random(rng, pk); let hat_cap_h = (&self.presigning.cap_k * secret_bounded_from_scalar::

(x)).mul_randomizer(&rho); @@ -1251,7 +1236,7 @@ impl Round for Round4 { let mut mul_star_proofs = Vec::new(); for id_l in self.context.other_ids.iter() { - let paux = self.public_aux(id_l)?; + let paux = self.context.public_aux(id_l)?; let p_mul = MulStarProof::

::new( rng, &secret_signed_from_scalar::

(x), @@ -1295,7 +1280,7 @@ impl Round for Round4 { let mut dec_proofs = Vec::new(); for id_l in self.context.other_ids.iter() { - let paux = self.public_aux(id_l)?; + let paux = self.context.public_aux(id_l)?; let p_dec = DecProof::

::new( rng, &s_part_nonreduced, From 101f04d9a81620ec3252ff3d31334932de923017 Mon Sep 17 00:00:00 2001 From: David Palm Date: Thu, 12 Dec 2024 13:35:39 +0100 Subject: [PATCH 9/9] Review feedback --- synedrion/src/cggmp21/params.rs | 8 +------- synedrion/src/curve/arithmetic.rs | 1 - synedrion/src/tools/hashing.rs | 30 +++++++++++++++--------------- 3 files changed, 16 insertions(+), 23 deletions(-) diff --git a/synedrion/src/cggmp21/params.rs b/synedrion/src/cggmp21/params.rs index 84060cb4..3b4f400c 100644 --- a/synedrion/src/cggmp21/params.rs +++ b/synedrion/src/cggmp21/params.rs @@ -138,10 +138,6 @@ pub(crate) fn uint_from_scalar(value: &Scalar) -> = scalar_len, - "PaillierParams::Uint is expected to be bigger than a Scalar" - ); repr.as_mut() .get_mut(uint_len - scalar_len..) .expect("PaillierParams::Uint is expected to be bigger than a Scalar") @@ -249,8 +245,7 @@ pub(crate) fn secret_uint_from_scalar( repr.expose_secret_mut() .as_mut() .get_mut(uint_len - scalar_len..) - // TODO(dp): @reviewers Do we need a better proof that this is true? If `Uint` is, say, 128 bits long… - .expect("Uint is assumed to be bigger than Scalar") + .expect("::Uint is assumed to be configured to be bigger than Scalar") .copy_from_slice(scalar_bytes.expose_secret()); Secret::init_with(|| ::Uint::from_be_bytes(*repr.expose_secret())) } @@ -291,7 +286,6 @@ pub(crate) fn secret_scalar_from_signed( // TODO: wrap in secrets properly let abs_value = scalar_from_uint::

(&value.expose_secret().abs()); Secret::init_with(|| Scalar::conditional_select(&abs_value, &-abs_value, value.expose_secret().is_negative())) - // >>>>>>> master } impl HashableType for P { diff --git a/synedrion/src/curve/arithmetic.rs b/synedrion/src/curve/arithmetic.rs index 641d0b6c..41017067 100644 --- a/synedrion/src/curve/arithmetic.rs +++ b/synedrion/src/curve/arithmetic.rs @@ -114,7 +114,6 @@ impl Scalar { Secret::init_with(|| Self(*sk.as_nonzero_scalar().as_ref())) } - // TODO(dp): @reviewers Does this assume big endian order? pub(crate) fn try_from_bytes(bytes: &[u8]) -> Result { let arr = GenericArray::>::from_exact_iter(bytes.iter().cloned()) .ok_or("Invalid length of a curve scalar")?; diff --git a/synedrion/src/tools/hashing.rs b/synedrion/src/tools/hashing.rs index edb00e50..f245280a 100644 --- a/synedrion/src/tools/hashing.rs +++ b/synedrion/src/tools/hashing.rs @@ -147,7 +147,8 @@ impl Hashable for T { } } -/// Build a `T` integer from an extendable Reader function +/// Build a `T` integer from an extendable Reader function. The resulting `T` is guaranteed to be +/// smaller than the modulus (uses rejection sampling). pub(crate) fn uint_from_xof(reader: &mut impl XofReader, modulus: &NonZero) -> T where T: Integer + Encoding, @@ -165,22 +166,21 @@ where u8::MAX }; - // TODO(dp): @reviewers this uses `to_le_bytes` (little-endian) but then the original code masks out bits - // from the *last* byte, so it'd mask out the *low* bits yeah? Changing to mask the first byte - // but this could use a test. let mut bytes = T::zero().to_le_bytes(); loop { - if let Some(buf) = bytes.as_mut().get_mut(0..n_bytes) { - reader.read(buf); - bytes.as_mut().first_mut().map(|byte| { - *byte &= mask; - Some(byte) - }); - let n = T::from_le_bytes(bytes); - - if n.ct_lt(backend_modulus).into() { - return n; - } + let buf = bytes + .as_mut() + .get_mut(0..n_bytes) + .expect("The modulus is a T and has at least n_bytes that can be read."); + reader.read(buf); + bytes.as_mut().last_mut().map(|byte| { + *byte &= mask; + Some(byte) + }); + let n = T::from_le_bytes(bytes); + + if n.ct_lt(backend_modulus).into() { + return n; } } }