Fix ecdsa_bls verify in BEEFY primitives

substrate/primitives/consensus/beefy/src/lib.rs

@@ -133,7 +133,7 @@ pub mod bls_crypto {
 		<MsgHash as Hash>::Output: Into<[u8; 32]>,
 	{
 		fn verify(&self, signature: &<Self as RuntimeAppPublic>::Signature, msg: &[u8]) -> bool {
-			// `w3f-bls` library uses IETF hashing standard and as such does not exposes
+			// `w3f-bls` library uses IETF hashing standard and as such does not expose
 			// a choice of hash to field function.
 			// We are directly calling into the library to avoid introducing new host call.
 			// and because BeefyAuthorityId::verify is being called in the runtime so we don't have
@@ -157,7 +157,7 @@ pub mod bls_crypto {
 pub mod ecdsa_bls_crypto {
 	use super::{BeefyAuthorityId, Hash, RuntimeAppPublic, KEY_TYPE};
 	use sp_application_crypto::{app_crypto, ecdsa_bls377};
-	use sp_core::{crypto::Wraps, ecdsa_bls377::Pair as EcdsaBlsPair, Pair as _};
+	use sp_core::{crypto::Wraps, ecdsa_bls377::Pair as EcdsaBlsPair};
 
 	app_crypto!(ecdsa_bls377, KEY_TYPE);
 
@@ -167,17 +167,24 @@ pub mod ecdsa_bls_crypto {
 	/// Signature for a BEEFY authority using (ECDSA,BLS) as its crypto.
 	pub type AuthoritySignature = Signature;
 
-	impl<MsgHash: Hash> BeefyAuthorityId<MsgHash> for AuthorityId
+	impl<H> BeefyAuthorityId<H> for AuthorityId
 	where
-		<MsgHash as Hash>::Output: Into<[u8; 32]>,
+		H: Hash,
+		H::Output: Into<[u8; 32]>,
 	{
 		fn verify(&self, signature: &<Self as RuntimeAppPublic>::Signature, msg: &[u8]) -> bool {
-			// `w3f-bls` library uses IETF hashing standard and as such does not exposes
-			// a choice of hash to field function.
-			// We are directly calling into the library to avoid introducing new host call.
-			// and because BeefyAuthorityId::verify is being called in the runtime so we don't have
-
-			EcdsaBlsPair::verify(signature.as_inner_ref(), msg, self.as_inner_ref())
+			// We can not simply call
+			// `EcdsaBlsPair::verify(signature.as_inner_ref(), msg, self.as_inner_ref())`
+			// because that invokes ECDSA default verification which perfoms Blake2b hash
+			// which we don't want. This is because ECDSA signatures are meant to be verified
+			// on Ethereum network where Keccak hasher is significantly cheaper than Blake2b.
+			// See Figure 3 of [OnSc21](https://www.scitepress.org/Papers/2021/106066/106066.pdf)
+			// for comparison.
+			EcdsaBlsPair::verify_with_hasher::<H>(
+				signature.as_inner_ref(),
+				msg,
+				self.as_inner_ref(),
+			)
 		}
 	}
 }
@@ -257,6 +264,7 @@ pub enum ConsensusLog<AuthorityId: Codec> {
 ///
 /// A vote message is a direct vote created by a BEEFY node on every voting round
 /// and is gossiped to its peers.
+// TODO: Remove `Signature` generic type, instead get it from `Id::Signature`.
 #[derive(Clone, Debug, Decode, Encode, PartialEq, TypeInfo)]
 pub struct VoteMessage<Number, Id, Signature> {
 	/// Commit to information extracted from a finalized block
@@ -507,11 +515,15 @@ mod tests {
 		let msg = &b"test-message"[..];
 		let (pair, _) = ecdsa_bls_crypto::Pair::generate();
 
-		let signature: ecdsa_bls_crypto::Signature = pair.as_inner_ref().sign(&msg).into();
+		let signature: ecdsa_bls_crypto::Signature =
+			pair.as_inner_ref().sign_with_hasher::<Keccak256>(&msg).into();
 
 		// Verification works if same hashing function is used when signing and verifying.
 		assert!(BeefyAuthorityId::<Keccak256>::verify(&pair.public(), &signature, msg));
 
+		// Verification doesn't work if we verify function provided by pair_crypto implementation
+		assert!(!ecdsa_bls_crypto::Pair::verify(&signature, msg, &pair.public()));
+
 		// Other public key doesn't work
 		let (other_pair, _) = ecdsa_bls_crypto::Pair::generate();
 		assert!(!BeefyAuthorityId::<Keccak256>::verify(&other_pair.public(), &signature, msg,));

substrate/primitives/core/src/paired_crypto.rs

@@ -39,7 +39,13 @@ use sp_std::convert::TryFrom;
 /// ECDSA and BLS12-377 paired crypto scheme
 #[cfg(feature = "bls-experimental")]
 pub mod ecdsa_bls377 {
-	use crate::{bls377, crypto::CryptoTypeId, ecdsa};
+	#[cfg(feature = "full_crypto")]
+	use crate::Hasher;
+	use crate::{
+		bls377,
+		crypto::{CryptoTypeId, Pair as PairT, UncheckedFrom},
+		ecdsa,
+	};
 
 	/// An identifier used to match public keys against BLS12-377 keys
 	pub const CRYPTO_ID: CryptoTypeId = CryptoTypeId(*b"ecb7");
@@ -71,6 +77,60 @@ pub mod ecdsa_bls377 {
 	impl super::CryptoType for Pair {
 		type Pair = Pair;
 	}
+
+	#[cfg(feature = "full_crypto")]
+	impl Pair {
+		/// Hashes the `message` with the specified [`Hasher`] before signing sith the ECDSA secret
+		/// component.
+		///
+		/// The hasher does not affect the BLS12-377 component. This generates BLS12-377 Signature
+		/// according to IETF standard.
+		pub fn sign_with_hasher<H>(&self, message: &[u8]) -> Signature
+		where
+			H: Hasher,
+			H::Out: Into<[u8; 32]>,
+		{
+			let msg_hash = H::hash(message).into();
+
+			let mut raw: [u8; SIGNATURE_LEN] = [0u8; SIGNATURE_LEN];
+			raw[..ecdsa::SIGNATURE_SERIALIZED_SIZE]
+				.copy_from_slice(self.left.sign_prehashed(&msg_hash).as_ref());
+			raw[ecdsa::SIGNATURE_SERIALIZED_SIZE..]
+				.copy_from_slice(self.right.sign(message).as_ref());
+			<Self as PairT>::Signature::unchecked_from(raw)
+		}
+
+		/// Hashes the `message` with the specified [`Hasher`] before verifying with the ECDSA
+		/// public component.
+		///
+		/// The hasher does not affect the the BLS12-377 component. This verifies whether the
+		/// BLS12-377 signature was hashed and signed according to IETF standard
+		pub fn verify_with_hasher<H>(sig: &Signature, message: &[u8], public: &Public) -> bool
+		where
+			H: Hasher,
+			H::Out: Into<[u8; 32]>,
+		{
+			let msg_hash = H::hash(message).into();
+
+			let Ok(left_pub) = public.0[..ecdsa::PUBLIC_KEY_SERIALIZED_SIZE].try_into() else {
+				return false
+			};
+			let Ok(left_sig) = sig.0[0..ecdsa::SIGNATURE_SERIALIZED_SIZE].try_into() else {
+				return false
+			};
+			if !ecdsa::Pair::verify_prehashed(&left_sig, &msg_hash, &left_pub) {
+				return false
+			}
+
+			let Ok(right_pub) = public.0[ecdsa::PUBLIC_KEY_SERIALIZED_SIZE..].try_into() else {
+				return false
+			};
+			let Ok(right_sig) = sig.0[ecdsa::SIGNATURE_SERIALIZED_SIZE..].try_into() else {
+				return false
+			};
+			bls377::Pair::verify(&right_sig, message.as_ref(), &right_pub)
+		}
+	}
 }
 
 /// Secure seed length.
@@ -455,12 +515,12 @@ where
 #[cfg(all(test, feature = "bls-experimental"))]
 mod test {
 	use super::*;
-	use crate::crypto::DEV_PHRASE;
+	use crate::{crypto::DEV_PHRASE, KeccakHasher};
 	use ecdsa_bls377::{Pair, Signature};
 
 	use crate::{bls377, ecdsa};
-	#[test]
 
+	#[test]
 	fn test_length_of_paired_ecdsa_and_bls377_public_key_and_signature_is_correct() {
 		assert_eq!(
 			<Pair as PairT>::Public::LEN,
@@ -617,6 +677,16 @@ mod test {
 		assert_eq!(cmp, public);
 	}
 
+	#[test]
+	fn sign_and_verify_with_hasher_works() {
+		let pair =
+			Pair::from_seed(&(b"12345678901234567890123456789012".as_slice().try_into().unwrap()));
+		let message = b"Something important";
+		let signature = pair.sign_with_hasher::<KeccakHasher>(&message[..]);
+
+		assert!(Pair::verify_with_hasher::<KeccakHasher>(&signature, &message[..], &pair.public()));
+	}
+
 	#[test]
 	fn signature_serialization_works() {
 		let pair =