Check length of polynomials

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "wsts"
-version = "9.1.0"
+version = "9.2.0"
 edition = "2021"
 authors = ["Joey Yandle <xoloki@gmail.com>"]
 license = "Apache-2.0"

src/errors.rs

@@ -13,7 +13,7 @@ pub enum DkgError {
     /// The private shares which were missing
     MissingPrivateShares(Vec<u32>),
     #[error("bad public shares {0:?}")]
-    /// The bad public shares that failed to verify
+    /// The public shares that failed to verify or were the wrong size
     BadPublicShares(Vec<u32>),
     #[error("not enough shares {0:?}")]
     /// Not enough shares to complete DKG
@@ -36,10 +36,10 @@ impl From<PointError> for DkgError {
 /// Errors which can happen during signature aggregation
 pub enum AggregatorError {
     #[error("bad poly commitment length (expected {0} got {1})")]
-    /// The polynomial commitment was the wrong size
+    /// The number of polynomial commitments was wrong (no longer used)
     BadPolyCommitmentLen(usize, usize),
     #[error("bad poly commitments {0:?}")]
-    /// The polynomial commitments which failed verification
+    /// The polynomial commitments which failed verification or were the wrong size
     BadPolyCommitments(Vec<Scalar>),
     #[error("bad nonce length (expected {0} got {1}")]
     /// The nonce length was the wrong size

src/state_machine/coordinator/fire.rs

@@ -1240,7 +1240,9 @@ impl<Aggregator: AggregatorTrait> CoordinatorTrait for Coordinator<Aggregator> {
 pub mod test {
     use crate::{
         curve::{point::Point, scalar::Scalar},
-        net::{DkgBegin, DkgFailure, DkgPrivateShares, Message, NonceRequest, Packet},
+        net::{
+            DkgBegin, DkgFailure, DkgPrivateShares, DkgPublicShares, Message, NonceRequest, Packet,
+        },
         state_machine::{
             coordinator::{
                 fire::Coordinator as FireCoordinator,
@@ -1928,6 +1930,123 @@ pub mod test {
         (coordinators, signers)
     }
 
+    #[test]
+    fn bad_poly_length_dkg_v1() {
+        bad_poly_length_dkg::<v1::Aggregator, v1::Signer>(5, 2);
+    }
+
+    #[test]
+    fn bad_poly_length_dkg_v2() {
+        bad_poly_length_dkg::<v2::Aggregator, v2::Signer>(5, 2);
+    }
+
+    fn bad_poly_length_dkg<Aggregator: AggregatorTrait, SignerType: SignerTrait>(
+        num_signers: u32,
+        keys_per_signer: u32,
+    ) -> (Vec<FireCoordinator<Aggregator>>, Vec<Signer<SignerType>>) {
+        let (mut coordinators, mut signers) =
+            setup::<FireCoordinator<Aggregator>, SignerType>(num_signers, keys_per_signer);
+
+        // We have started a dkg round
+        let message = coordinators.first_mut().unwrap().start_dkg_round().unwrap();
+        assert!(coordinators.first().unwrap().aggregate_public_key.is_none());
+        assert_eq!(coordinators.first().unwrap().state, State::DkgPublicGather);
+
+        // Send the DkgBegin message to all signers and share their responses with the coordinators and signers, but mutate two signers' DkgPublicShares: make one polynomial larger than the threshold, and the other smaller
+        let (outbound_messages, operation_results) = feedback_mutated_messages(
+            &mut coordinators,
+            &mut signers,
+            &[message],
+            |signer, msgs| {
+                if signer.signer_id == 0 || signer.signer_id == 1 {
+                    msgs.iter()
+                        .map(|packet| {
+                            if let Message::DkgPublicShares(shares) = &packet.msg {
+                                let comms = shares
+                                    .comms
+                                    .iter()
+                                    .map(|(id, comm)| {
+                                        let mut c = comm.clone();
+                                        if signer.signer_id == 0 {
+                                            c.poly.push(Point::new());
+                                        } else {
+                                            c.poly.pop();
+                                        }
+                                        (*id, c)
+                                    })
+                                    .collect();
+                                Packet {
+                                    msg: Message::DkgPublicShares(DkgPublicShares {
+                                        dkg_id: shares.dkg_id,
+                                        signer_id: shares.signer_id,
+                                        comms,
+                                    }),
+                                    sig: vec![],
+                                }
+                            } else {
+                                packet.clone()
+                            }
+                        })
+                        .collect()
+                } else {
+                    msgs
+                }
+            },
+        );
+
+        assert!(operation_results.is_empty());
+        for coordinator in &coordinators {
+            assert_eq!(coordinator.state, State::DkgPrivateGather);
+        }
+
+        assert_eq!(outbound_messages.len(), 1);
+        match &outbound_messages[0].msg {
+            Message::DkgPrivateBegin(_) => {}
+            _ => {
+                panic!("Expected DkgPrivateBegin message");
+            }
+        }
+
+        let (outbound_messages, operation_results) =
+            feedback_messages(&mut coordinators, &mut signers, &outbound_messages);
+        assert_eq!(operation_results.len(), 0);
+        assert_eq!(outbound_messages.len(), 1);
+        match &outbound_messages[0].msg {
+            Message::DkgEndBegin(_) => {}
+            _ => {
+                panic!("Expected DkgEndBegin message");
+            }
+        }
+
+        // Send the DkgEndBegin message to all signers and share their responses with the coordinators and signers
+        let (outbound_messages, operation_results) =
+            feedback_messages(&mut coordinators, &mut signers, &outbound_messages);
+        assert_eq!(outbound_messages.len(), 0);
+        assert_eq!(operation_results.len(), 1);
+        match &operation_results[0] {
+            OperationResult::DkgError(dkg_error) => {
+                // we mutated the public shares themselves, so we should see a BadPublicShares from signer_ids 0 and 1
+                match dkg_error {
+                    DkgError::DkgEndFailure(failure_map) => {
+                        for (_signer_id, dkg_failure) in failure_map {
+                            match dkg_failure {
+                                DkgFailure::BadPublicShares(bad_shares) => {
+                                    for bad_signer_id in bad_shares {
+                                        assert!(*bad_signer_id == 0u32 || *bad_signer_id == 1u32);
+                                    }
+                                }
+                                _ => panic!("Expected DkgFailure::BadPublicShares"),
+                            }
+                        }
+                    }
+                    _ => panic!("Expected DkgError::DkgEndFailure"),
+                }
+            }
+            _ => panic!("Expected OperationResult::DkgError"),
+        }
+        (coordinators, signers)
+    }
+
     #[test]
     fn all_signers_sign_v1() {
         all_signers_sign::<v1::Aggregator, v1::Signer>();

src/state_machine/signer/mod.rs

@@ -343,7 +343,7 @@ impl<SignerType: SignerTrait> Signer<SignerType> {
             for signer_id in &dkg_end_begin.signer_ids {
                 if let Some(shares) = self.dkg_public_shares.get(signer_id) {
                     for (party_id, comm) in shares.comms.iter() {
-                        if !comm.verify() {
+                        if comm.poly.len() != self.threshold.try_into().unwrap() || !comm.verify() {
                             bad_public_shares.insert(*signer_id);
                         } else {
                             self.commitments.insert(*party_id, comm.clone());

src/traits.rs

@@ -153,7 +153,40 @@ pub mod test_helpers {
 
     use crate::{common::PolyCommitment, errors::DkgError};
 
-    #[allow(non_snake_case)]
+    /// Run DKG on the passed signers
+    pub fn dkg<RNG: RngCore + CryptoRng, Signer: super::Signer>(
+        signers: &mut [Signer],
+        rng: &mut RNG,
+    ) -> Result<HashMap<u32, PolyCommitment>, HashMap<u32, DkgError>> {
+        let public_shares: HashMap<u32, PolyCommitment> = signers
+            .iter()
+            .flat_map(|s| s.get_poly_commitments(rng))
+            .map(|comm| (comm.id.id.get_u32(), comm))
+            .collect();
+        let mut private_shares = HashMap::new();
+
+        for signer in signers.iter() {
+            for (signer_id, signer_shares) in signer.get_shares() {
+                private_shares.insert(signer_id, signer_shares);
+            }
+        }
+
+        let mut secret_errors = HashMap::new();
+        for signer in signers.iter_mut() {
+            if let Err(signer_secret_errors) =
+                signer.compute_secrets(&private_shares, &public_shares)
+            {
+                secret_errors.extend(signer_secret_errors.into_iter());
+            }
+        }
+
+        if secret_errors.is_empty() {
+            Ok(public_shares)
+        } else {
+            Err(secret_errors)
+        }
+    }
+
     /// Remove the provided key ids from the list of private shares and execute compute secrets
     fn compute_secrets_not_enough_shares<RNG: RngCore + CryptoRng, Signer: super::Signer>(
         signers: &mut [Signer],
@@ -220,4 +253,50 @@ pub mod test_helpers {
             }
         }
     }
+
+    #[allow(non_snake_case)]
+    /// Check that bad polynomial lengths are properly caught as errors
+    pub fn bad_polynomial_length<
+        Signer: super::Signer,
+        Aggregator: super::Aggregator,
+        F: Fn(u32) -> u32,
+    >(
+        func: F,
+    ) {
+        let Nk: u32 = 10;
+        let Np: u32 = 4;
+        let T: u32 = 7;
+        let signer_ids: Vec<Vec<u32>> = vec![vec![1, 2, 3, 4], vec![5, 6, 7], vec![8, 9], vec![10]];
+        let mut rng = OsRng;
+        let mut signers: Vec<Signer> = signer_ids
+            .iter()
+            .enumerate()
+            .map(|(id, ids)| {
+                if *ids == vec![10] {
+                    Signer::new(id.try_into().unwrap(), ids, Nk, Np, func(T), &mut rng)
+                } else {
+                    Signer::new(id.try_into().unwrap(), ids, Nk, Np, T, &mut rng)
+                }
+            })
+            .collect();
+
+        if dkg(&mut signers, &mut rng).is_ok() {
+            panic!("DKG should have failed")
+        }
+
+        let polys: HashMap<u32, PolyCommitment> = signers
+            .iter()
+            .flat_map(|s| s.get_poly_commitments(&mut rng))
+            .map(|comm| (comm.id.id.get_u32(), comm))
+            .collect();
+
+        let mut aggregator = Aggregator::new(Nk, T);
+        match aggregator.init(&polys) {
+            Ok(_) => panic!("Aggregator::init should not have succeeded"),
+            Err(e) => match e {
+                super::AggregatorError::BadPolyCommitments(_) => (),
+                _ => panic!("Should have failed with BadPolyCommitments"),
+            },
+        }
+    }
 }

src/v1.rs

@@ -149,12 +149,14 @@ impl Party {
         self.private_key = Scalar::zero();
         self.group_key = Point::zero();
 
+        let threshold: usize = self.threshold.try_into().unwrap();
         let mut bad_ids = Vec::new(); //: Vec<u32> = polys
         for (i, comm) in polys.iter() {
-            if !comm.verify() {
+            if comm.poly.len() != threshold || !comm.verify() {
                 bad_ids.push(*i);
+            } else {
+                self.group_key += comm.poly[0];
             }
-            self.group_key += comm.poly[0];
         }
         if !bad_ids.is_empty() {
             return Err(DkgError::BadPublicShares(bad_ids));
@@ -370,17 +372,18 @@ impl traits::Aggregator for Aggregator {
 
     /// Initialize the Aggregator polynomial
     fn init(&mut self, comms: &HashMap<u32, PolyCommitment>) -> Result<(), AggregatorError> {
+        let threshold = self.threshold.try_into().unwrap();
         let mut bad_poly_commitments = Vec::new();
         for (_id, comm) in comms {
-            if !comm.verify() {
+            if comm.poly.len() != threshold || !comm.verify() {
                 bad_poly_commitments.push(comm.id.id);
             }
         }
         if !bad_poly_commitments.is_empty() {
             return Err(AggregatorError::BadPolyCommitments(bad_poly_commitments));
         }
 
-        let mut poly = Vec::with_capacity(self.threshold.try_into().unwrap());
+        let mut poly = Vec::with_capacity(threshold);
 
         for i in 0..poly.capacity() {
             poly.push(Point::zero());
@@ -695,6 +698,7 @@ pub mod test_helpers {
 
 #[cfg(test)]
 mod tests {
+    use crate::traits;
     use crate::traits::test_helpers::run_compute_secrets_not_enough_shares;
     use crate::traits::{Aggregator, Signer};
     use crate::v1;
@@ -799,7 +803,7 @@ mod tests {
             .map(|(id, ids)| v1::Signer::new(id.try_into().unwrap(), ids, N, T, &mut rng))
             .collect();
 
-        let comms = match v1::test_helpers::dkg(&mut signers, &mut rng) {
+        let comms = match traits::test_helpers::dkg(&mut signers, &mut rng) {
             Ok(comms) => comms,
             Err(secret_errors) => {
                 panic!("Got secret errors from DKG: {:?}", secret_errors);
@@ -825,4 +829,14 @@ mod tests {
     pub fn run_compute_secrets_missing_shares() {
         run_compute_secrets_not_enough_shares::<v1::Signer>()
     }
+
+    #[allow(non_snake_case)]
+    #[test]
+    /// Run DKG and aggregator init with a bad polynomial
+    pub fn bad_polynomial_length() {
+        let gt = |t| t + 1;
+        let lt = |t| t - 1;
+        traits::test_helpers::bad_polynomial_length::<v1::Signer, v1::Aggregator, _>(gt);
+        traits::test_helpers::bad_polynomial_length::<v1::Signer, v1::Aggregator, _>(lt);
+    }
 }