From 4f65a0b147c72b1ba2b2b59dd20776ee5d7d7644 Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Wed, 23 Jul 2025 15:13:27 -0400 Subject: [PATCH] Remove Clone from ClsagMultisigMask{Sender, Receiver} This had ill-defined properties on Clone, as a mask could be sent multiple times (unintended) and multiple algorithms may receive the same mask from a singular sender. Requires removing the Clone bound within modular-frost and expanding the test helpers accordingly. This was not raised in the audit yet upon independent review. --- crypto/frost/src/algorithm.rs | 2 +- crypto/frost/src/sign.rs | 6 +-- crypto/frost/src/tests/mod.rs | 55 +++++++++++++++----- networks/monero/ringct/clsag/src/multisig.rs | 8 +-- networks/monero/ringct/clsag/src/tests.rs | 38 +++++++++----- 5 files changed, 75 insertions(+), 34 deletions(-) diff --git a/crypto/frost/src/algorithm.rs b/crypto/frost/src/algorithm.rs index 0b0abd6c..b595e03b 100644 --- a/crypto/frost/src/algorithm.rs +++ b/crypto/frost/src/algorithm.rs @@ -25,7 +25,7 @@ pub trait Addendum: Send + Sync + Clone + PartialEq + Debug + WriteAddendum {} impl Addendum for A {} /// Algorithm trait usable by the FROST signing machine to produce signatures.. -pub trait Algorithm: Send + Sync + Clone { +pub trait Algorithm: Send + Sync { /// The transcript format this algorithm uses. This likely should NOT be the IETF-compatible /// transcript included in this crate. type Transcript: Sync + Clone + Debug + Transcript; diff --git a/crypto/frost/src/sign.rs b/crypto/frost/src/sign.rs index 5115244f..693960d5 100644 --- a/crypto/frost/src/sign.rs +++ b/crypto/frost/src/sign.rs @@ -47,7 +47,7 @@ impl Writable for Vec { } // Pairing of an Algorithm with a ThresholdKeys instance. -#[derive(Clone, Zeroize)] +#[derive(Zeroize)] struct Params> { // Skips the algorithm due to being too large a bound to feasibly enforce on users #[zeroize(skip)] @@ -193,7 +193,7 @@ impl SignatureShare { /// Trait for the second machine of a two-round signing protocol. pub trait SignMachine: Send + Sync + Sized { /// Params used to instantiate this machine which can be used to rebuild from a cache. - type Params: Clone; + type Params; /// Keys used for signing operations. type Keys; /// Preprocess message for this machine. @@ -397,7 +397,7 @@ impl> SignMachine for AlgorithmSignMachi Ok(( AlgorithmSignatureMachine { - params: self.params.clone(), + params: self.params, view, B, Rs, diff --git a/crypto/frost/src/tests/mod.rs b/crypto/frost/src/tests/mod.rs index f93a5fbf..db6553aa 100644 --- a/crypto/frost/src/tests/mod.rs +++ b/crypto/frost/src/tests/mod.rs @@ -37,10 +37,10 @@ pub fn clone_without( } /// Spawn algorithm machines for a random selection of signers, each executing the given algorithm. -pub fn algorithm_machines>( +pub fn algorithm_machines_without_clone>( rng: &mut R, - algorithm: &A, keys: &HashMap>, + machines: HashMap>, ) -> HashMap> { let mut included = vec![]; while included.len() < usize::from(keys[&Participant::new(1).unwrap()].params().t()) { @@ -54,18 +54,28 @@ pub fn algorithm_machines>( included.push(n); } - keys - .iter() - .filter_map(|(i, keys)| { - if included.contains(i) { - Some((*i, AlgorithmMachine::new(algorithm.clone(), keys.clone()))) - } else { - None - } - }) + machines + .into_iter() + .filter_map(|(i, machine)| if included.contains(&i) { Some((i, machine)) } else { None }) .collect() } +/// Spawn algorithm machines for a random selection of signers, each executing the given algorithm. +pub fn algorithm_machines>( + rng: &mut R, + algorithm: &A, + keys: &HashMap>, +) -> HashMap> { + algorithm_machines_without_clone( + rng, + keys, + keys + .values() + .map(|keys| (keys.params().i(), AlgorithmMachine::new(algorithm.clone(), keys.clone()))) + .collect(), + ) +} + // Run the preprocess step pub(crate) fn preprocess< R: RngCore + CryptoRng, @@ -165,10 +175,10 @@ pub fn sign_without_caching( /// Execute the signing protocol, randomly caching various machines to ensure they can cache /// successfully. -pub fn sign( +pub fn sign_without_clone( rng: &mut R, - params: &>::Params, mut keys: HashMap>::Keys>, + mut params: HashMap>::Params>, machines: HashMap, msg: &[u8], ) -> M::Signature { @@ -183,7 +193,8 @@ pub fn sign( let cache = machines.remove(&i).unwrap().cache(); machines.insert( i, - M::SignMachine::from_cache(params.clone(), keys.remove(&i).unwrap(), cache).0, + M::SignMachine::from_cache(params.remove(&i).unwrap(), keys.remove(&i).unwrap(), cache) + .0, ); } } @@ -192,6 +203,22 @@ pub fn sign( ) } +/// Execute the signing protocol, randomly caching various machines to ensure they can cache +/// successfully. +pub fn sign< + R: RngCore + CryptoRng, + M: PreprocessMachine>, +>( + rng: &mut R, + params: &>::Params, + keys: HashMap>::Keys>, + machines: HashMap, + msg: &[u8], +) -> M::Signature { + let params = keys.keys().map(|i| (*i, params.clone())).collect(); + sign_without_clone(rng, keys, params, machines, msg) +} + /// Test a basic Schnorr signature with the provided keys. pub fn test_schnorr_with_keys>( rng: &mut R, diff --git a/networks/monero/ringct/clsag/src/multisig.rs b/networks/monero/ringct/clsag/src/multisig.rs index d4ddd180..2cd7ea6e 100644 --- a/networks/monero/ringct/clsag/src/multisig.rs +++ b/networks/monero/ringct/clsag/src/multisig.rs @@ -57,11 +57,11 @@ impl ClsagContext { /// A channel to send the mask to use for the pseudo-out (rerandomized commitment) with. /// /// A mask must be sent along this channel before any preprocess addendums are handled. -#[derive(Clone, Debug)] +#[derive(Debug)] pub struct ClsagMultisigMaskSender { buf: Arc>>, } -#[derive(Clone, Debug)] +#[derive(Debug)] struct ClsagMultisigMaskReceiver { buf: Arc>>, } @@ -73,6 +73,8 @@ impl ClsagMultisigMaskSender { /// Send a mask to a CLSAG multisig instance. pub fn send(self, mask: Scalar) { + // There is no risk this was prior set as this consumes `self`, which does not implement + // `Clone` *self.buf.lock() = Some(mask); } } @@ -118,7 +120,7 @@ struct Interim { /// The message signed is expected to be a 32-byte value. Per Monero, it's the keccak256 hash of /// the transaction data which is signed. This will panic if the message is not a 32-byte value. #[allow(non_snake_case)] -#[derive(Clone, Debug)] +#[derive(Debug)] pub struct ClsagMultisig { transcript: RecommendedTranscript, diff --git a/networks/monero/ringct/clsag/src/tests.rs b/networks/monero/ringct/clsag/src/tests.rs index d4ae1f41..ff994445 100644 --- a/networks/monero/ringct/clsag/src/tests.rs +++ b/networks/monero/ringct/clsag/src/tests.rs @@ -19,7 +19,8 @@ use crate::ClsagMultisig; #[cfg(feature = "multisig")] use frost::{ Participant, - tests::{key_gen, algorithm_machines, sign}, + sign::AlgorithmMachine, + tests::{key_gen, algorithm_machines_without_clone, sign_without_clone}, }; const RING_LEN: u64 = 11; @@ -99,21 +100,32 @@ fn clsag_multisig() { ring.push([dest, Commitment::new(mask, amount).calculate()]); } - let (algorithm, mask_send) = ClsagMultisig::new( - RecommendedTranscript::new(b"Monero Serai CLSAG Test"), - ClsagContext::new( - Decoys::new((1 ..= RING_LEN).collect(), RING_INDEX, ring.clone()).unwrap(), - Commitment::new(randomness, AMOUNT), - ) - .unwrap(), - ); - mask_send.send(Scalar::random(&mut OsRng)); + let mask = Scalar::random(&mut OsRng); + let params = || { + let (algorithm, mask_send) = ClsagMultisig::new( + RecommendedTranscript::new(b"Monero Serai CLSAG Test"), + ClsagContext::new( + Decoys::new((1 ..= RING_LEN).collect(), RING_INDEX, ring.clone()).unwrap(), + Commitment::new(randomness, AMOUNT), + ) + .unwrap(), + ); + mask_send.send(mask); + algorithm + }; - sign( + sign_without_clone( &mut OsRng, - &algorithm, keys.clone(), - algorithm_machines(&mut OsRng, &algorithm, &keys), + keys.values().map(|keys| (keys.params().i(), params())).collect(), + algorithm_machines_without_clone( + &mut OsRng, + &keys, + keys + .values() + .map(|keys| (keys.params().i(), AlgorithmMachine::new(params(), keys.clone()))) + .collect(), + ), &[1; 32], ); }