Add validation for missing identifier in signing package (#452)

* Add validation for missing identifier in signing package (#442)

* Add error handling for identifier having an invalid commitment (#442)

* Fixed comment (#442)

* Improve comment for number of signers validation

Co-authored-by: Conrado Gouvea <conrado@zfnd.org>

---------

Co-authored-by: Conrado Gouvea <conrado@zfnd.org>

frost-core/src/error.rs

@@ -53,6 +53,13 @@ pub enum Error<C: Ciphersuite> {
     /// Commitment equals the identity
     #[error("Commitment equals the identity.")]
     IdentityCommitment,
+    /// 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,
     /// Signature share verification failed.
     #[error("Invalid signature share.")]
     InvalidSignatureShare {
@@ -125,6 +132,8 @@ where
             | Error::DuplicatedShares
             | Error::IncorrectNumberOfShares
             | Error::IdentityCommitment
+            | Error::MissingCommitment
+            | Error::IncorrectCommitment
             | Error::PackageNotFound
             | Error::IncorrectNumberOfPackages
             | Error::IncorrectPackage

frost-core/src/frost/round2.rs

@@ -11,6 +11,8 @@ use crate::{
 #[cfg(feature = "serde")]
 use crate::ScalarSerialization;
 
+use super::round1::SigningCommitments;
+
 // Used to help encoding a SignatureShare. Since it has a Scalar<C> it can't
 // be directly encoded with serde, so we use this struct to wrap the scalar.
 #[cfg(feature = "serde")]
@@ -187,6 +189,19 @@ pub fn sign<C: Ciphersuite>(
     signer_nonces: &round1::SigningNonces<C>,
     key_package: &frost::keys::KeyPackage<C>,
 ) -> Result<SignatureShare<C>, Error<C>> {
+    // Validate the signer's commitment is present in the signing package
+    let commitment = signing_package
+        .signing_commitments
+        .get(&key_package.identifier)
+        .ok_or(Error::MissingCommitment)?;
+
+    let signing_commitments = SigningCommitments::from(signer_nonces);
+
+    // Validate if the signer's commitment exists
+    if &signing_commitments != commitment {
+        return Err(Error::IncorrectCommitment);
+    }
+
     // Encodes the signing commitment list produced in round one as part of generating [`BindingFactor`], the
     // binding factor.
     let binding_factor_list: BindingFactorList<C> =

frost-core/src/tests/ciphersuite_generic.rs

@@ -448,3 +448,151 @@ pub fn check_identifier_derivation<C: Ciphersuite>() {
     assert!(id1a == id1b);
     assert!(id1a != id2);
 }
+
+/// Checks the signer's identifier is included in the package
+pub fn check_sign_with_missing_identifier<C: Ciphersuite, R: RngCore + CryptoRng>(mut rng: R) {
+    ////////////////////////////////////////////////////////////////////////////
+    // Key generation
+    ////////////////////////////////////////////////////////////////////////////
+
+    let max_signers = 5;
+    let min_signers = 3;
+    let (shares, _pubkeys) = frost::keys::generate_with_dealer(
+        max_signers,
+        min_signers,
+        frost::keys::IdentifierList::Default,
+        &mut rng,
+    )
+    .unwrap();
+
+    // Verifies the secret shares from the dealer
+    let mut key_packages: HashMap<frost::Identifier<C>, frost::keys::KeyPackage<C>> =
+        HashMap::new();
+
+    for (k, v) in shares {
+        let key_package = frost::keys::KeyPackage::try_from(v).unwrap();
+        key_packages.insert(k, key_package);
+    }
+
+    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>> =
+        BTreeMap::new();
+
+    ////////////////////////////////////////////////////////////////////////////
+    // Round 1: generating nonces and signing commitments for each participant
+    ////////////////////////////////////////////////////////////////////////////
+
+    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 key_packages_inc = vec![id_1, id_2, id_3];
+
+    for participant_identifier in key_packages_inc {
+        // The nonces and commitments for each participant are generated.
+        let (nonces, commitments) = frost::round1::commit(
+            key_packages
+                .get(&participant_identifier)
+                .unwrap()
+                .secret_share(),
+            &mut rng,
+        );
+        nonces_map.insert(participant_identifier, nonces);
+
+        // Participant with id_1 is excluded from the commitments_map so it is missing from the signing package
+        if participant_identifier == id_1 {
+            continue;
+        }
+        commitments_map.insert(participant_identifier, commitments);
+    }
+
+    // This is what the signature aggregator / coordinator needs to do:
+    // - decide what message to sign
+    // - take one (unused) commitment per signing participant
+    let message = "message to sign".as_bytes();
+    let signing_package = frost::SigningPackage::new(commitments_map, message);
+
+    ////////////////////////////////////////////////////////////////////////////
+    // Round 2: Participant with id_1 signs
+    ////////////////////////////////////////////////////////////////////////////
+
+    let key_package_1 = key_packages.get(&id_1).unwrap();
+
+    let nonces_to_use = &nonces_map.get(&id_1).unwrap();
+
+    // 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))
+}
+
+/// Checks the signer's commitment is valid
+pub fn check_sign_with_incorrect_commitments<C: Ciphersuite, R: RngCore + CryptoRng>(mut rng: R) {
+    ////////////////////////////////////////////////////////////////////////////
+    // Key generation
+    ////////////////////////////////////////////////////////////////////////////
+
+    let max_signers = 5;
+    let min_signers = 3;
+    let (shares, _pubkeys) = frost::keys::generate_with_dealer(
+        max_signers,
+        min_signers,
+        frost::keys::IdentifierList::Default,
+        &mut rng,
+    )
+    .unwrap();
+
+    // Verifies the secret shares from the dealer
+    let mut key_packages: HashMap<frost::Identifier<C>, frost::keys::KeyPackage<C>> =
+        HashMap::new();
+
+    for (k, v) in shares {
+        let key_package = frost::keys::KeyPackage::try_from(v).unwrap();
+        key_packages.insert(k, key_package);
+    }
+
+    let mut commitments_map: BTreeMap<frost::Identifier<C>, frost::round1::SigningCommitments<C>> =
+        BTreeMap::new();
+
+    ////////////////////////////////////////////////////////////////////////////
+    // Round 1: generating nonces and signing commitments for each participant
+    ////////////////////////////////////////////////////////////////////////////
+
+    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 key_packages_inc = vec![id_1, id_2, id_3];
+
+    let (_nonces_1, commitments_1) =
+        frost::round1::commit(key_packages[&id_1].secret_share(), &mut rng);
+
+    let (_nonces_2, commitments_2) =
+        frost::round1::commit(key_packages[&id_2].secret_share(), &mut rng);
+
+    let (nonces_3, _commitments_3) =
+        frost::round1::commit(key_packages[&id_3].secret_share(), &mut rng);
+
+    commitments_map.insert(id_1, commitments_1);
+    commitments_map.insert(id_2, commitments_2);
+    // Invalid commitment for id_3
+    commitments_map.insert(id_3, commitments_1);
+
+    // This is what the signature aggregator / coordinator needs to do:
+    // - decide what message to sign
+    // - take one (unused) commitment per signing participant
+    let message = "message to sign".as_bytes();
+    let signing_package = frost::SigningPackage::new(commitments_map, message);
+
+    ////////////////////////////////////////////////////////////////////////////
+    // Round 2: Participant with id_3 signs
+    ////////////////////////////////////////////////////////////////////////////
+
+    let key_package_3 = key_packages.get(&id_3).unwrap();
+
+    // Each participant generates their signature share.
+    let signature_share = frost::round2::sign(&signing_package, &nonces_3, key_package_3);
+
+    assert!(signature_share.is_err());
+    assert!(signature_share == Err(Error::IncorrectCommitment))
+}

frost-ed25519/tests/integration_tests.rs

@@ -72,3 +72,19 @@ fn check_sign_with_dealer_and_identifiers() {
         _,
     >(rng);
 }
+
+#[test]
+fn check_sign_with_missing_identifier() {
+    let rng = thread_rng();
+    frost_core::tests::ciphersuite_generic::check_sign_with_missing_identifier::<Ed25519Sha512, _>(
+        rng,
+    );
+}
+
+#[test]
+fn check_sign_with_incorrect_commitments() {
+    let rng = thread_rng();
+    frost_core::tests::ciphersuite_generic::check_sign_with_incorrect_commitments::<Ed25519Sha512, _>(
+        rng,
+    );
+}

frost-ed448/tests/integration_tests.rs

@@ -72,3 +72,19 @@ fn check_sign_with_dealer_and_identifiers() {
         _,
     >(rng);
 }
+
+#[test]
+fn check_sign_with_missing_identifier() {
+    let rng = thread_rng();
+    frost_core::tests::ciphersuite_generic::check_sign_with_missing_identifier::<Ed448Shake256, _>(
+        rng,
+    );
+}
+
+#[test]
+fn check_sign_with_incorrect_commitments() {
+    let rng = thread_rng();
+    frost_core::tests::ciphersuite_generic::check_sign_with_incorrect_commitments::<Ed448Shake256, _>(
+        rng,
+    );
+}

frost-p256/tests/integration_tests.rs

@@ -69,3 +69,19 @@ fn check_sign_with_dealer_and_identifiers() {
         rng,
     );
 }
+
+#[test]
+fn check_sign_with_missing_identifier() {
+    let rng = thread_rng();
+    frost_core::tests::ciphersuite_generic::check_sign_with_missing_identifier::<P256Sha256, _>(
+        rng,
+    );
+}
+
+#[test]
+fn check_sign_with_incorrect_commitments() {
+    let rng = thread_rng();
+    frost_core::tests::ciphersuite_generic::check_sign_with_incorrect_commitments::<P256Sha256, _>(
+        rng,
+    );
+}

frost-ristretto255/tests/integration_tests.rs

@@ -72,3 +72,21 @@ fn check_sign_with_dealer_and_identifiers() {
         _,
     >(rng);
 }
+
+#[test]
+fn check_sign_with_missing_identifier() {
+    let rng = thread_rng();
+    frost_core::tests::ciphersuite_generic::check_sign_with_missing_identifier::<
+        Ristretto255Sha512,
+        _,
+    >(rng);
+}
+
+#[test]
+fn check_sign_with_incorrect_commitments() {
+    let rng = thread_rng();
+    frost_core::tests::ciphersuite_generic::check_sign_with_incorrect_commitments::<
+        Ristretto255Sha512,
+        _,
+    >(rng);
+}

frost-secp256k1/tests/integration_tests.rs

@@ -72,3 +72,20 @@ fn check_sign_with_dealer_and_identifiers() {
         _,
     >(rng);
 }
+
+#[test]
+fn check_sign_with_missing_identifier() {
+    let rng = thread_rng();
+    frost_core::tests::ciphersuite_generic::check_sign_with_missing_identifier::<Secp256K1Sha256, _>(
+        rng,
+    );
+}
+
+#[test]
+fn check_sign_with_incorrect_commitments() {
+    let rng = thread_rng();
+    frost_core::tests::ciphersuite_generic::check_sign_with_incorrect_commitments::<
+        Secp256K1Sha256,
+        _,
+    >(rng);
+}