check number of commitments in sign() (#480)

* check number of commitments in sign()

* make comment clearer

frost-core/CHANGELOG.md

@@ -9,6 +9,8 @@ Entries are listed in reverse chronological order.
 * Challenge hashing during DKG computation was changed to match the paper.
   This means that code running this version won't interoperate with code
   running previous versions.
+* A new `min_signers` field was added to `KeyPackage`, which changes its
+  `new()` method and its serde serialization.
 
 ## Released
 

frost-core/src/error.rs

@@ -56,10 +56,12 @@ pub enum Error<C: Ciphersuite> {
     /// The participant's commitment is missing from the Signing Package
     #[error("The Signing Package must contain the participant's Commitment.")]
     MissingCommitment,
-
     /// The participant's commitment is incorrect
     #[error("The participant's commitment is incorrect.")]
     IncorrectCommitment,
+    /// Incorrect number of commitments.
+    #[error("Incorrect number of commitments.")]
+    IncorrectNumberOfCommitments,
     /// Signature share verification failed.
     #[error("Invalid signature share.")]
     InvalidSignatureShare {
@@ -144,6 +146,7 @@ where
             | Error::InvalidCoefficient
             | Error::UnknownIdentifier
             | Error::IncorrectNumberOfIdentifiers
+            | Error::IncorrectNumberOfCommitments
             | Error::IdentifierDerivationNotSupported => None,
         }
     }

frost-core/src/frost/keys.rs

@@ -576,6 +576,7 @@ pub struct KeyPackage<C: Ciphersuite> {
     /// The public verifying key that represents the entire group.
     #[zeroize(skip)]
     pub(crate) group_public: VerifyingKey<C>,
+    pub(crate) min_signers: u16,
     /// Ciphersuite ID for serialization
     #[cfg_attr(
         feature = "serde",
@@ -599,12 +600,14 @@ where
         secret_share: SigningShare<C>,
         public: VerifyingShare<C>,
         group_public: VerifyingKey<C>,
+        min_signers: u16,
     ) -> Self {
         Self {
             identifier,
             secret_share,
             public,
             group_public,
+            min_signers,
             ciphersuite: (),
         }
     }
@@ -632,6 +635,7 @@ where
             secret_share: secret_share.value,
             public,
             group_public,
+            min_signers: secret_share.commitment.0.len() as u16,
             ciphersuite: (),
         })
     }

frost-core/src/frost/keys/dkg.rs

@@ -107,6 +107,8 @@ pub mod round1 {
         pub(crate) coefficients: Vec<Scalar<C>>,
         /// The public commitment for the participant (C_i)
         pub(crate) commitment: VerifiableSecretSharingCommitment<C>,
+        /// The minimum number of signers.
+        pub(crate) min_signers: u16,
         /// The total number of signers.
         pub(crate) max_signers: u16,
     }
@@ -120,6 +122,7 @@ pub mod round1 {
                 .field("identifier", &self.identifier)
                 .field("coefficients", &"<redacted>")
                 .field("commitment", &self.commitment)
+                .field("min_signers", &self.min_signers)
                 .field("max_signers", &self.max_signers)
                 .finish()
         }
@@ -197,6 +200,8 @@ pub mod round2 {
         pub(crate) commitment: VerifiableSecretSharingCommitment<C>,
         /// The participant's own secret share (f_i(i)).
         pub(crate) secret_share: Scalar<C>,
+        /// The minimum number of signers.
+        pub(crate) min_signers: u16,
         /// The total number of signers.
         pub(crate) max_signers: u16,
     }
@@ -210,6 +215,7 @@ pub mod round2 {
                 .field("identifier", &self.identifier)
                 .field("commitment", &self.commitment)
                 .field("secret_share", &"<redacted>")
+                .field("min_signers", &self.min_signers)
                 .field("max_signers", &self.max_signers)
                 .finish()
         }
@@ -273,6 +279,7 @@ pub fn part1<C: Ciphersuite, R: RngCore + CryptoRng>(
         identifier,
         coefficients,
         commitment: commitment.clone(),
+        min_signers,
         max_signers,
     };
     let package = round1::Package {
@@ -368,6 +375,7 @@ pub fn part2<C: Ciphersuite>(
             identifier: secret_package.identifier,
             commitment: secret_package.commitment,
             secret_share: fii,
+            min_signers: secret_package.min_signers,
             max_signers: secret_package.max_signers,
         },
         round2_packages,
@@ -518,6 +526,7 @@ pub fn part3<C: Ciphersuite>(
         secret_share: signing_share,
         public: verifying_key,
         group_public,
+        min_signers: round2_secret_package.min_signers,
         ciphersuite: (),
     };
     let public_key_package = PublicKeyPackage {

frost-core/src/frost/round2.rs

@@ -189,6 +189,10 @@ pub fn sign<C: Ciphersuite>(
     signer_nonces: &round1::SigningNonces<C>,
     key_package: &frost::keys::KeyPackage<C>,
 ) -> Result<SignatureShare<C>, Error<C>> {
+    if signing_package.signing_commitments().len() < key_package.min_signers as usize {
+        return Err(Error::IncorrectNumberOfCommitments);
+    }
+
     // Validate the signer's commitment is present in the signing package
     let commitment = signing_package
         .signing_commitments

frost-core/src/tests/ciphersuite_generic.rs

@@ -1,4 +1,6 @@
 //! Ciphersuite-generic test functions.
+#![allow(clippy::type_complexity)]
+
 use std::{
     collections::{BTreeMap, HashMap},
     convert::TryFrom,
@@ -117,8 +119,31 @@ pub fn check_sign_with_dealer<C: Ciphersuite, R: RngCore + CryptoRng>(
         let key_package = frost::keys::KeyPackage::try_from(v).unwrap();
         key_packages.insert(k, key_package);
     }
+    // Check if it fails with not enough signers. Usually this would return an
+    // error before even running the signing procedure, because `KeyPackage`
+    // contains the correct `min_signers` value and the signing procedure checks
+    // if the number of shares is at least `min_signers`. To bypass the check
+    // and test if the protocol itself fails with not enough signers, we modify
+    // the `KeyPackages`s, decrementing their saved `min_signers` value before
+    // running the signing procedure.
+    let r = check_sign(
+        min_signers - 1,
+        key_packages
+            .iter()
+            .map(|(id, k)| {
+                // Decrement `min_signers` as explained above and use
+                // the updated `KeyPackage`.
+                let mut k = k.clone();
+                k.min_signers -= 1;
+                (*id, k)
+            })
+            .collect(),
+        &mut rng,
+        pubkeys.clone(),
+    );
+    assert_eq!(r, Err(Error::InvalidSignature));
 
-    check_sign(min_signers, key_packages, rng, pubkeys)
+    check_sign(min_signers, key_packages, rng, pubkeys).unwrap()
 }
 
 /// Test FROST signing with trusted dealer fails with invalid numbers of signers.
@@ -163,7 +188,7 @@ pub fn check_sign<C: Ciphersuite + PartialEq, R: RngCore + CryptoRng>(
     key_packages: HashMap<frost::Identifier<C>, frost::keys::KeyPackage<C>>,
     mut rng: R,
     pubkey_package: frost::keys::PublicKeyPackage<C>,
-) -> (Vec<u8>, Signature<C>, VerifyingKey<C>) {
+) -> Result<(Vec<u8>, Signature<C>, VerifyingKey<C>), Error<C>> {
     let mut nonces_map: HashMap<frost::Identifier<C>, frost::round1::SigningNonces<C>> =
         HashMap::new();
     let mut commitments_map: BTreeMap<frost::Identifier<C>, frost::round1::SigningCommitments<C>> =
@@ -201,11 +226,16 @@ pub fn check_sign<C: Ciphersuite + PartialEq, R: RngCore + CryptoRng>(
     for participant_identifier in nonces_map.keys() {
         let key_package = key_packages.get(participant_identifier).unwrap();
 
-        let nonces_to_use = &nonces_map.get(participant_identifier).unwrap();
+        let nonces_to_use = nonces_map.get(participant_identifier).unwrap();
+
+        check_sign_errors(
+            signing_package.clone(),
+            nonces_to_use.clone(),
+            key_package.clone(),
+        );
 
         // Each participant generates their signature share.
-        let signature_share =
-            frost::round2::sign(&signing_package, nonces_to_use, key_package).unwrap();
+        let signature_share = frost::round2::sign(&signing_package, nonces_to_use, key_package)?;
         signature_shares.insert(*participant_identifier, signature_share);
     }
 
@@ -221,33 +251,47 @@ pub fn check_sign<C: Ciphersuite + PartialEq, R: RngCore + CryptoRng>(
     );
 
     // Aggregate (also verifies the signature shares)
-    let group_signature =
-        frost::aggregate(&signing_package, &signature_shares, &pubkey_package).unwrap();
+    let group_signature = frost::aggregate(&signing_package, &signature_shares, &pubkey_package)?;
 
     // Check that the threshold signature can be verified by the group public
     // key (the verification key).
-    let is_signature_valid = pubkey_package
+    pubkey_package
         .group_public
-        .verify(message, &group_signature)
-        .is_ok();
-    assert!(is_signature_valid);
+        .verify(message, &group_signature)?;
 
     // Check that the threshold signature can be verified by the group public
     // key (the verification key) from KeyPackage.group_public
     for (participant_identifier, _) in nonces_map.clone() {
         let key_package = key_packages.get(&participant_identifier).unwrap();
 
-        assert!(key_package
-            .group_public
-            .verify(message, &group_signature)
-            .is_ok());
+        key_package.group_public.verify(message, &group_signature)?;
     }
 
-    (
+    Ok((
         message.to_owned(),
         group_signature,
         pubkey_package.group_public,
-    )
+    ))
+}
+
+fn check_sign_errors<C: Ciphersuite + PartialEq>(
+    signing_package: frost::SigningPackage<C>,
+    signing_nonces: frost::round1::SigningNonces<C>,
+    key_package: frost::keys::KeyPackage<C>,
+) {
+    // Check if passing not enough commitments causes an error
+
+    let mut commitments = signing_package.signing_commitments().clone();
+    // Remove one commitment that's not from the key_package owner
+    let id = *commitments
+        .keys()
+        .find(|&&id| id != key_package.identifier)
+        .unwrap();
+    commitments.remove(&id);
+    let signing_package = frost::SigningPackage::new(commitments, signing_package.message());
+
+    let r = frost::round2::sign(&signing_package, &signing_nonces, &key_package);
+    assert_eq!(r, Err(Error::IncorrectNumberOfCommitments));
 }
 
 fn check_aggregate_errors<C: Ciphersuite + PartialEq>(
@@ -461,7 +505,7 @@ where
     let pubkeys = frost::keys::PublicKeyPackage::new(verifying_keys, group_public.unwrap());
 
     // Proceed with the signing test.
-    check_sign(min_signers, key_packages, rng, pubkeys)
+    check_sign(min_signers, key_packages, rng, pubkeys).unwrap()
 }
 
 /// Check that calling dkg::part3() with distinct sets of participants fail.
@@ -571,7 +615,7 @@ pub fn check_sign_with_dealer_and_identifiers<C: Ciphersuite, R: RngCore + Crypt
         let key_package = frost::keys::KeyPackage::try_from(v).unwrap();
         key_packages.insert(k, key_package);
     }
-    check_sign(min_signers, key_packages, rng, pubkeys)
+    check_sign(min_signers, key_packages, rng, pubkeys).unwrap()
 }
 
 fn check_part2_error<C: Ciphersuite>(
@@ -653,6 +697,7 @@ pub fn check_sign_with_missing_identifier<C: Ciphersuite, R: RngCore + CryptoRng
     let id_1 = Identifier::<C>::try_from(1).unwrap();
     let id_2 = Identifier::<C>::try_from(2).unwrap();
     let id_3 = Identifier::<C>::try_from(3).unwrap();
+    let id_4 = Identifier::<C>::try_from(4).unwrap();
     let key_packages_inc = vec![id_1, id_2, id_3];
 
     for participant_identifier in key_packages_inc {
@@ -666,11 +711,14 @@ pub fn check_sign_with_missing_identifier<C: Ciphersuite, R: RngCore + CryptoRng
         );
         nonces_map.insert(participant_identifier, nonces);
 
-        // Participant with id_1 is excluded from the commitments_map so it is missing from the signing package
+        // Participant with id_1 is excluded from the commitments_map so it is missing from the signing package.
+        // To prevent sign() from returning an error due to incorrect number of commitments,
+        // add the commitment under another unrelated participant.
         if participant_identifier == id_1 {
-            continue;
+            commitments_map.insert(id_4, commitments);
+        } else {
+            commitments_map.insert(participant_identifier, commitments);
         }
-        commitments_map.insert(participant_identifier, commitments);
     }
 
     // This is what the signature aggregator / coordinator needs to do:
@@ -690,8 +738,7 @@ pub fn check_sign_with_missing_identifier<C: Ciphersuite, R: RngCore + CryptoRng
     // Each participant generates their signature share.
     let signature_share = frost::round2::sign(&signing_package, nonces_to_use, key_package_1);
 
-    assert!(signature_share.is_err());
-    assert!(signature_share == Err(Error::MissingCommitment))
+    assert_eq!(signature_share, Err(Error::MissingCommitment))
 }
 
 /// Checks the signer's commitment is valid

frost-core/src/tests/vectors.rs

@@ -40,7 +40,7 @@ pub fn parse_test_vectors<C: Ciphersuite>(json_vectors: &Value) -> TestVectors<C
     let message = inputs["message"].as_str().unwrap();
     let message_bytes = hex::decode(message).unwrap();
 
-    let share_polynomial_coefficients = inputs["share_polynomial_coefficients"]
+    let share_polynomial_coefficients: Vec<_> = inputs["share_polynomial_coefficients"]
         .as_array()
         .unwrap()
         .iter()
@@ -67,8 +67,14 @@ pub fn parse_test_vectors<C: Ciphersuite>(json_vectors: &Value) -> TestVectors<C
                 .unwrap();
         let signer_public = secret.into();
 
-        let key_package =
-            KeyPackage::<C>::new(i.try_into().unwrap(), secret, signer_public, group_public);
+        let min_signers = share_polynomial_coefficients.len() + 1;
+        let key_package = KeyPackage::<C>::new(
+            i.try_into().unwrap(),
+            secret,
+            signer_public,
+            group_public,
+            min_signers as u16,
+        );
 
         key_packages.insert(*key_package.identifier(), key_package);
     }

frost-ed25519/tests/helpers/samples.rs

@@ -80,7 +80,7 @@ pub fn key_package() -> KeyPackage {
     let serialized_element = <C as Ciphersuite>::Group::serialize(&element1());
     let verifying_key = VerifyingKey::deserialize(serialized_element).unwrap();
 
-    KeyPackage::new(identifier, signing_share, verifying_share, verifying_key)
+    KeyPackage::new(identifier, signing_share, verifying_share, verifying_key, 2)
 }
 
 /// Generate a sample PublicKeyPackage.

frost-ed25519/tests/recreation_tests.rs

@@ -71,12 +71,14 @@ fn check_key_package_recreation() {
     let signing_share = key_package.secret_share();
     let verifying_share = key_package.public();
     let verifying_key = key_package.group_public();
+    let min_signers = key_package.min_signers();
 
     let new_key_package = KeyPackage::new(
         *identifier,
         *signing_share,
         *verifying_share,
         *verifying_key,
+        *min_signers,
     );
 
     assert!(key_package == new_key_package);

frost-ed25519/tests/serde_tests.rs

@@ -284,6 +284,7 @@ fn check_key_package_serialization() {
         "secret_share": "498d4e9311420c903913a56c94a694b8aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa0a",
         "public": "5866666666666666666666666666666666666666666666666666666666666666",
         "group_public": "5866666666666666666666666666666666666666666666666666666666666666",
+        "min_signers": 2,
         "ciphersuite": "FROST(Ed25519, SHA-512)"
       }"#;
     let decoded_key_package: KeyPackage = serde_json::from_str(json).unwrap();

frost-ed448/tests/helpers/samples.rs

@@ -80,7 +80,7 @@ pub fn key_package() -> KeyPackage {
     let serialized_element = <C as Ciphersuite>::Group::serialize(&element1());
     let verifying_key = VerifyingKey::deserialize(serialized_element).unwrap();
 
-    KeyPackage::new(identifier, signing_share, verifying_share, verifying_key)
+    KeyPackage::new(identifier, signing_share, verifying_share, verifying_key, 2)
 }
 
 /// Generate a sample PublicKeyPackage.

frost-ed448/tests/recreation_tests.rs

@@ -71,12 +71,14 @@ fn check_key_package_recreation() {
     let signing_share = key_package.secret_share();
     let verifying_share = key_package.public();
     let verifying_key = key_package.group_public();
+    let min_signers = key_package.min_signers();
 
     let new_key_package = KeyPackage::new(
         *identifier,
         *signing_share,
         *verifying_share,
         *verifying_key,
+        *min_signers,
     );
 
     assert!(key_package == new_key_package);