From c65bf624c8a47d7f65a32c1b9b35e931e04842f0 Mon Sep 17 00:00:00 2001 From: Conrado Gouvea Date: Thu, 12 Sep 2024 17:03:29 -0300 Subject: [PATCH] allow identifying culprit in error returned by dkg::part3() --- frost-core/CHANGELOG.md | 3 + frost-core/src/error.rs | 12 +++- frost-core/src/keys.rs | 13 +++- frost-core/src/keys/dkg.rs | 11 ++- frost-core/src/tests/ciphersuite_generic.rs | 75 ++++++++++++++++++++- 5 files changed, 108 insertions(+), 6 deletions(-) diff --git a/frost-core/CHANGELOG.md b/frost-core/CHANGELOG.md index 915ef475..a8f026ae 100644 --- a/frost-core/CHANGELOG.md +++ b/frost-core/CHANGELOG.md @@ -10,6 +10,9 @@ Entries are listed in reverse chronological order. but it's likely to not require any code changes since most ciphersuite implementations are probably just empty structs. The bound makes it possible to use `frost_core::Error` in `Box`. +* It is now possible to identify the culprit in `frost_core::keys::dkg::part3()` + if an invalid secret share was sent by one of the participants (by calling + frost_core::Error::culprit()`). ## 2.0.0-rc.0 diff --git a/frost-core/src/error.rs b/frost-core/src/error.rs index 6fdd3606..78662df7 100644 --- a/frost-core/src/error.rs +++ b/frost-core/src/error.rs @@ -74,7 +74,11 @@ pub enum Error { }, /// Secret share verification failed. #[error("Invalid secret share.")] - InvalidSecretShare, + InvalidSecretShare { + /// The identifier of the signer whose secret share validation failed, + /// if possible to identify. + culprit: Option>, + }, /// Round 1 package not found for Round 2 participant. #[error("Round 1 package not found for Round 2 participant.")] PackageNotFound, @@ -132,8 +136,10 @@ where | Error::InvalidProofOfKnowledge { culprit: identifier, } => Some(*identifier), - Error::InvalidSecretShare - | Error::InvalidMinSigners + Error::InvalidSecretShare { + culprit: identifier, + } => *identifier, + Error::InvalidMinSigners | Error::InvalidMaxSigners | Error::InvalidCoefficients | Error::MalformedIdentifier diff --git a/frost-core/src/keys.rs b/frost-core/src/keys.rs index 5a884e42..f95cdfd6 100644 --- a/frost-core/src/keys.rs +++ b/frost-core/src/keys.rs @@ -423,7 +423,18 @@ where let result = evaluate_vss(self.identifier, &self.commitment); if !(f_result == result) { - return Err(Error::InvalidSecretShare); + // The culprit needs to be identified by the caller if needed, + // because this function is called in two different contexts: + // - after trusted dealer key generation, by the participant who + // receives the SecretShare. In that case it does not make sense + // to identify themselves as the culprit, since the issue was with + // the Coordinator or in the communication. + // - during DKG, where a "fake" SecretShare is built just to reuse + // the verification logic and it does make sense to identify the + // culprit. Note that in this case, self.identifier is the caller's + // identifier and not the culprit's, so we couldn't identify + // the culprit inside this function anyway. + return Err(Error::InvalidSecretShare { culprit: None }); } Ok(( diff --git a/frost-core/src/keys/dkg.rs b/frost-core/src/keys/dkg.rs index aa3a56ae..f3859482 100644 --- a/frost-core/src/keys/dkg.rs +++ b/frost-core/src/keys/dkg.rs @@ -524,7 +524,16 @@ pub fn part3( }; // Verify the share. We don't need the result. - let _ = secret_share.verify()?; + // Identify the culprit if an InvalidSecretShare error is returned. + let _ = secret_share.verify().map_err(|e| { + if let Error::InvalidSecretShare { .. } = e { + Error::InvalidSecretShare { + culprit: Some(*sender_identifier), + } + } else { + e + } + })?; // Round 2, Step 3 // diff --git a/frost-core/src/tests/ciphersuite_generic.rs b/frost-core/src/tests/ciphersuite_generic.rs index 4528e9b7..691b2754 100644 --- a/frost-core/src/tests/ciphersuite_generic.rs +++ b/frost-core/src/tests/ciphersuite_generic.rs @@ -4,6 +4,7 @@ use alloc::collections::BTreeMap; use crate as frost; +use crate::keys::SigningShare; use crate::round2::SignatureShare; use crate::{ keys::PublicKeyPackage, Error, Field, Group, Identifier, Signature, SigningKey, SigningPackage, @@ -497,7 +498,7 @@ where // for each signature before being aggregated. let mut pubkey_packages_by_participant = BTreeMap::new(); - check_part3_different_participants( + check_part3_errors( max_signers, round2_secret_packages.clone(), received_round1_packages.clone(), @@ -536,6 +537,33 @@ where check_sign(min_signers, key_packages, rng, pubkeys).unwrap() } +/// Check for error cases related to DKG part3. +fn check_part3_errors( + max_signers: u16, + round2_secret_packages: BTreeMap, frost::keys::dkg::round2::SecretPackage>, + received_round1_packages: BTreeMap< + Identifier, + BTreeMap, frost::keys::dkg::round1::Package>, + >, + received_round2_packages: BTreeMap< + Identifier, + BTreeMap, frost::keys::dkg::round2::Package>, + >, +) { + check_part3_different_participants( + max_signers, + round2_secret_packages.clone(), + received_round1_packages.clone(), + received_round2_packages.clone(), + ); + check_part3_corrupted_share( + max_signers, + round2_secret_packages, + received_round1_packages, + received_round2_packages, + ); +} + /// Check that calling dkg::part3() with distinct sets of participants fail. fn check_part3_different_participants( max_signers: u16, @@ -573,6 +601,49 @@ fn check_part3_different_participants( } } +/// Check that calling dkg::part3() with a corrupted share fail, and the +/// culprit is correctly identified. +fn check_part3_corrupted_share( + max_signers: u16, + round2_secret_packages: BTreeMap, frost::keys::dkg::round2::SecretPackage>, + received_round1_packages: BTreeMap< + Identifier, + BTreeMap, frost::keys::dkg::round1::Package>, + >, + received_round2_packages: BTreeMap< + Identifier, + BTreeMap, frost::keys::dkg::round2::Package>, + >, +) { + // For each participant, perform the third part of the DKG protocol. + // In practice, each participant will perform this on their own environments. + for participant_index in 1..=max_signers { + let participant_identifier = participant_index.try_into().expect("should be nonzero"); + + // Remove the first package from the map, and reinsert it with an unrelated + // Do the same for Round 2 packages + let mut received_round2_packages = + received_round2_packages[&participant_identifier].clone(); + let culprit = *received_round2_packages.keys().next().unwrap(); + let package = received_round2_packages.get_mut(&culprit).unwrap(); + let one = <::Group as Group>::Field::one(); + package.signing_share = SigningShare::new(package.signing_share().to_scalar() + one); + + let r = frost::keys::dkg::part3( + &round2_secret_packages[&participant_identifier], + &received_round1_packages[&participant_identifier], + &received_round2_packages, + ) + .expect_err("Should have failed due to corrupted share"); + assert_eq!( + r, + Error::InvalidSecretShare { + culprit: Some(culprit) + } + ) + } +} + /// Test FROST signing with trusted dealer with a Ciphersuite, using specified /// Identifiers. pub fn check_sign_with_dealer_and_identifiers( @@ -646,10 +717,12 @@ pub fn check_sign_with_dealer_and_identifiers( round1_secret_package: frost::keys::dkg::round1::SecretPackage, mut round1_packages: BTreeMap, frost::keys::dkg::round1::Package>, ) { + // Check if a corrupted proof of knowledge results in failure. let one = <::Group as Group>::Field::one(); // Corrupt a PoK let id = *round1_packages.keys().next().unwrap();