From 6a14615ec971d9e1a25747bd2a9fcbda620a58e1 Mon Sep 17 00:00:00 2001 From: David Palm Date: Thu, 9 Apr 2020 22:55:17 +0200 Subject: [PATCH 1/6] Allow pubkey recovery for all-zero messages After https://github.com/openethereum/openethereum/pull/11406 it is no longer possible to to public key recovery from messages that are all-zero. This create issues when using the `ecrecover` builtin because externally produced signatures may well provide a message (i.e. a preimage) that is all-zeroes. This works around the problem at the cost of cloning the incoming message and create a `ZeroesAllowedMessage` wrapper around it. The `ZeroesAllowedMessage` implements the `ThirtyTwoByteHash` trait from `rust-secp256k1` which circumvents the zero-check. In a follow-up PR we'll likely change the interface of `recover()` to take a `ZeroesAllowedMessage` directly, thus removing the unneeded clone. --- .../src/publickey/ecdsa_signature.rs | 30 ++++++++++++++++--- parity-crypto/src/publickey/mod.rs | 8 +++++ 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/parity-crypto/src/publickey/ecdsa_signature.rs b/parity-crypto/src/publickey/ecdsa_signature.rs index 6853b1586..109ebd4b6 100644 --- a/parity-crypto/src/publickey/ecdsa_signature.rs +++ b/parity-crypto/src/publickey/ecdsa_signature.rs @@ -8,7 +8,7 @@ //! Signature based on ECDSA, algorithm's description: https://en.wikipedia.org/wiki/Elliptic_Curve_Digital_Signature_Algorithm -use super::{public_to_address, Address, Error, Message, Public, Secret, SECP256K1}; +use super::{public_to_address, Address, Error, Message, Public, Secret, SECP256K1, ZeroesAllowedMessage}; use ethereum_types::{H256, H520}; use rustc_hex::{FromHex, ToHex}; use secp256k1::key::{PublicKey, SecretKey}; @@ -247,8 +247,9 @@ pub fn verify_address(address: &Address, signature: &Signature, message: &Messag /// Recovers the public key from the signature for the message pub fn recover(signature: &Signature, message: &Message) -> Result { let context = &SECP256K1; + let secp_msg = ZeroesAllowedMessage(message.clone()); let rsig = RecoverableSignature::from_compact(&signature[0..64], RecoveryId::from_i32(signature[64] as i32)?)?; - let pubkey = context.recover(&SecpMessage::from_slice(&message[..])?, &rsig)?; + let pubkey = context.recover(&secp_msg.into(), &rsig)?; let serialized = pubkey.serialize_uncompressed(); let mut public = Public::default(); @@ -258,8 +259,9 @@ pub fn recover(signature: &Signature, message: &Message) -> Result [u8; 32] { + self.0.to_fixed_bytes() + } +} + /// The number -1 encoded as a secret key const MINUS_ONE_KEY: &'static [u8] = &[ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xfe, 0xba, 0xae, 0xdc, From 51d5e179b1590849806ddaf8ba8c2e1972d75088 Mon Sep 17 00:00:00 2001 From: David Palm Date: Thu, 9 Apr 2020 23:04:45 +0200 Subject: [PATCH 2/6] Inner doesn't need to be pub --- parity-crypto/src/publickey/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/parity-crypto/src/publickey/mod.rs b/parity-crypto/src/publickey/mod.rs index 30500dc33..cb5e1bbe8 100644 --- a/parity-crypto/src/publickey/mod.rs +++ b/parity-crypto/src/publickey/mod.rs @@ -34,7 +34,7 @@ pub use ethereum_types::{Address, Public}; pub type Message = H256; use secp256k1::ThirtyTwoByteHash; -pub struct ZeroesAllowedMessage(pub H256); +pub struct ZeroesAllowedMessage(H256); impl ThirtyTwoByteHash for ZeroesAllowedMessage { fn into_32(self) -> [u8; 32] { self.0.to_fixed_bytes() From 31a14e708b1fb83012f9bc9d4eb0aa84360beb06 Mon Sep 17 00:00:00 2001 From: David Date: Fri, 10 Apr 2020 00:07:03 +0200 Subject: [PATCH 3/6] Update parity-crypto/src/publickey/ecdsa_signature.rs Co-Authored-By: Andronik Ordian --- parity-crypto/src/publickey/ecdsa_signature.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/parity-crypto/src/publickey/ecdsa_signature.rs b/parity-crypto/src/publickey/ecdsa_signature.rs index 109ebd4b6..90b62984a 100644 --- a/parity-crypto/src/publickey/ecdsa_signature.rs +++ b/parity-crypto/src/publickey/ecdsa_signature.rs @@ -247,7 +247,7 @@ pub fn verify_address(address: &Address, signature: &Signature, message: &Messag /// Recovers the public key from the signature for the message pub fn recover(signature: &Signature, message: &Message) -> Result { let context = &SECP256K1; - let secp_msg = ZeroesAllowedMessage(message.clone()); + let secp_msg = ZeroesAllowedMessage(*message); let rsig = RecoverableSignature::from_compact(&signature[0..64], RecoveryId::from_i32(signature[64] as i32)?)?; let pubkey = context.recover(&secp_msg.into(), &rsig)?; let serialized = pubkey.serialize_uncompressed(); From 0ad4caa1dd568b5e560e0b9d2b945f101fd82546 Mon Sep 17 00:00:00 2001 From: David Palm Date: Fri, 10 Apr 2020 00:18:42 +0200 Subject: [PATCH 4/6] Docs and review grumbles --- parity-crypto/src/publickey/ecdsa_signature.rs | 4 ++-- parity-crypto/src/publickey/mod.rs | 10 +++++++++- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/parity-crypto/src/publickey/ecdsa_signature.rs b/parity-crypto/src/publickey/ecdsa_signature.rs index 109ebd4b6..3010e3ed3 100644 --- a/parity-crypto/src/publickey/ecdsa_signature.rs +++ b/parity-crypto/src/publickey/ecdsa_signature.rs @@ -247,9 +247,9 @@ pub fn verify_address(address: &Address, signature: &Signature, message: &Messag /// Recovers the public key from the signature for the message pub fn recover(signature: &Signature, message: &Message) -> Result { let context = &SECP256K1; - let secp_msg = ZeroesAllowedMessage(message.clone()); + let message = ZeroesAllowedMessage(*message); let rsig = RecoverableSignature::from_compact(&signature[0..64], RecoveryId::from_i32(signature[64] as i32)?)?; - let pubkey = context.recover(&secp_msg.into(), &rsig)?; + let pubkey = context.recover(&message.into(), &rsig)?; let serialized = pubkey.serialize_uncompressed(); let mut public = Public::default(); diff --git a/parity-crypto/src/publickey/mod.rs b/parity-crypto/src/publickey/mod.rs index cb5e1bbe8..6bcf2909a 100644 --- a/parity-crypto/src/publickey/mod.rs +++ b/parity-crypto/src/publickey/mod.rs @@ -34,7 +34,15 @@ pub use ethereum_types::{Address, Public}; pub type Message = H256; use secp256k1::ThirtyTwoByteHash; -pub struct ZeroesAllowedMessage(H256); + +/// In ethereum we allow public key recovery from a signature + message pair +/// where the message is all-zeroes. This conflicts with the best practise of +/// not allowing such values and so in order to avoid breaking consensus we need +/// this to work around it. The `ZeroesAllowedType` wraps an `H256` that can be +/// converted to a `[u8; 32]` which in turn can be cast to a +/// `secp256k1::Message` by the `ThirtyTwoByteHash` and satisfy the API for +/// `recover()`. +struct ZeroesAllowedMessage(H256); impl ThirtyTwoByteHash for ZeroesAllowedMessage { fn into_32(self) -> [u8; 32] { self.0.to_fixed_bytes() From 88f3d44ff12eb28698f20da9259cdd7ca76a22bc Mon Sep 17 00:00:00 2001 From: David Palm Date: Fri, 10 Apr 2020 10:45:07 +0200 Subject: [PATCH 5/6] Add `recover_allowing_all_zero_message()` Revert `recover()` to previous behaviour: no zero-messages allowed Docs and cleanup --- .../src/publickey/ecdsa_signature.rs | 79 ++++++++++++++----- parity-crypto/src/publickey/mod.rs | 7 +- 2 files changed, 63 insertions(+), 23 deletions(-) diff --git a/parity-crypto/src/publickey/ecdsa_signature.rs b/parity-crypto/src/publickey/ecdsa_signature.rs index 3010e3ed3..c715c9fd1 100644 --- a/parity-crypto/src/publickey/ecdsa_signature.rs +++ b/parity-crypto/src/publickey/ecdsa_signature.rs @@ -246,24 +246,62 @@ pub fn verify_address(address: &Address, signature: &Signature, message: &Messag /// Recovers the public key from the signature for the message pub fn recover(signature: &Signature, message: &Message) -> Result { - let context = &SECP256K1; - let message = ZeroesAllowedMessage(*message); - let rsig = RecoverableSignature::from_compact(&signature[0..64], RecoveryId::from_i32(signature[64] as i32)?)?; - let pubkey = context.recover(&message.into(), &rsig)?; + let rsig = RecoverableSignature::from_compact( + &signature[0..64], + RecoveryId::from_i32(signature[64] as i32)? + )?; + let pubkey = &SECP256K1.recover(&SecpMessage::from_slice(&message[..])?, &rsig)?; let serialized = pubkey.serialize_uncompressed(); - let mut public = Public::default(); public.as_bytes_mut().copy_from_slice(&serialized[1..65]); Ok(public) } +/// Recovers the public key from the signature for the given message. +/// This version of `recover()` allows for all-zero messages, which is necessary +/// for ethereum but is otherwise highly discouraged. Use with caution. +pub fn recover_allowing_all_zero_message( + signature: &Signature, + message: ZeroesAllowedMessage +) -> Result { + let rsig = RecoverableSignature::from_compact( + &signature[0..64], + RecoveryId::from_i32(signature[64] as i32)?, + )?; + let pubkey = &SECP256K1.recover(&message.into(), &rsig)?; + let serialized = pubkey.serialize_uncompressed(); + let mut public = Public::zero(); + public.as_bytes_mut().copy_from_slice(&serialized[1..65]); + Ok(public) +} + #[cfg(test)] mod tests { use super::super::{Generator, Message, Random, SECP256K1}; - use super::{recover, sign, verify_address, verify_public, Signature, ZeroesAllowedMessage}; + use super::{ + recover, recover_allowing_all_zero_message, sign, verify_address, verify_public, + Secret, Signature, ZeroesAllowedMessage + }; use secp256k1::SecretKey; use std::str::FromStr; + // Copy of `sign()` that allows signing all-zero Messages. + // Note: this is for *tests* only. DO NOT USE UNLESS YOU NEED IT. + fn sign_zero_message(secret: &Secret) -> Signature { + let context = &SECP256K1; + let sec = SecretKey::from_slice(secret.as_ref()).unwrap(); + // force an all-zero message into a secp `Message` bypassing the validity check. + let zero_msg = ZeroesAllowedMessage(Message::zero()); + let s = context.sign_recoverable(&zero_msg.into(), &sec); + let (rec_id, data) = s.serialize_compact(); + let mut data_arr = [0; 65]; + + // no need to check if s is low, it always is + data_arr[0..64].copy_from_slice(&data[0..64]); + data_arr[64] = rec_id.to_i32() as u8; + Signature(data_arr) + } + #[test] fn vrs_conversion() { // given @@ -298,23 +336,22 @@ mod tests { } #[test] - fn sign_and_recover_public_works_with_zeroed_messages() { + fn sign_and_recover_public_fails_with_zeroed_messages() { let keypair = Random.generate(); + let signature = sign_zero_message(keypair.secret()); let zero_message = Message::zero(); - let signature = { - let context = &SECP256K1; - let sec = SecretKey::from_slice(keypair.secret().as_ref()).unwrap(); - let secp_msg = ZeroesAllowedMessage(zero_message); - let s = context.sign_recoverable(&secp_msg.into(), &sec); - let (rec_id, data) = s.serialize_compact(); - let mut data_arr = [0; 65]; - - // no need to check if s is low, it always is - data_arr[0..64].copy_from_slice(&data[0..64]); - data_arr[64] = rec_id.to_i32() as u8; - Signature(data_arr) - }; - assert_eq!(keypair.public(), &recover(&signature, &zero_message).unwrap()); + assert!(&recover(&signature, &zero_message).is_err()); + } + + #[test] + fn recover_allowing_all_zero_message_can_recover_from_all_zero_messages() { + let keypair = Random.generate(); + let signature = sign_zero_message(keypair.secret()); + let zero_message = ZeroesAllowedMessage(Message::zero()); + assert_eq!( + keypair.public(), + &recover_allowing_all_zero_message(&signature, zero_message).unwrap() + ) } #[test] diff --git a/parity-crypto/src/publickey/mod.rs b/parity-crypto/src/publickey/mod.rs index 6bcf2909a..a53f34a63 100644 --- a/parity-crypto/src/publickey/mod.rs +++ b/parity-crypto/src/publickey/mod.rs @@ -20,7 +20,10 @@ pub mod ecdh; pub mod ecies; pub mod error; -pub use self::ecdsa_signature::{recover, sign, verify_address, verify_public, Signature}; +pub use self::ecdsa_signature::{ + recover, recover_allowing_all_zero_message, sign, verify_address, verify_public, + Signature +}; pub use self::error::Error; pub use self::extended_keys::{Derivation, DerivationError, ExtendedKeyPair, ExtendedPublic, ExtendedSecret}; pub use self::keypair::{public_to_address, KeyPair}; @@ -42,7 +45,7 @@ use secp256k1::ThirtyTwoByteHash; /// converted to a `[u8; 32]` which in turn can be cast to a /// `secp256k1::Message` by the `ThirtyTwoByteHash` and satisfy the API for /// `recover()`. -struct ZeroesAllowedMessage(H256); +pub struct ZeroesAllowedMessage(H256); impl ThirtyTwoByteHash for ZeroesAllowedMessage { fn into_32(self) -> [u8; 32] { self.0.to_fixed_bytes() From 634ecb5dcf9af2312abed8872cfbe8b537eecbee Mon Sep 17 00:00:00 2001 From: David Palm Date: Fri, 10 Apr 2020 11:49:42 +0200 Subject: [PATCH 6/6] Obey the fmt --- .../src/publickey/ecdsa_signature.rs | 23 ++++++------------- parity-crypto/src/publickey/mod.rs | 3 +-- 2 files changed, 8 insertions(+), 18 deletions(-) diff --git a/parity-crypto/src/publickey/ecdsa_signature.rs b/parity-crypto/src/publickey/ecdsa_signature.rs index c715c9fd1..7968967a7 100644 --- a/parity-crypto/src/publickey/ecdsa_signature.rs +++ b/parity-crypto/src/publickey/ecdsa_signature.rs @@ -8,7 +8,7 @@ //! Signature based on ECDSA, algorithm's description: https://en.wikipedia.org/wiki/Elliptic_Curve_Digital_Signature_Algorithm -use super::{public_to_address, Address, Error, Message, Public, Secret, SECP256K1, ZeroesAllowedMessage}; +use super::{public_to_address, Address, Error, Message, Public, Secret, ZeroesAllowedMessage, SECP256K1}; use ethereum_types::{H256, H520}; use rustc_hex::{FromHex, ToHex}; use secp256k1::key::{PublicKey, SecretKey}; @@ -246,10 +246,7 @@ pub fn verify_address(address: &Address, signature: &Signature, message: &Messag /// Recovers the public key from the signature for the message pub fn recover(signature: &Signature, message: &Message) -> Result { - let rsig = RecoverableSignature::from_compact( - &signature[0..64], - RecoveryId::from_i32(signature[64] as i32)? - )?; + let rsig = RecoverableSignature::from_compact(&signature[0..64], RecoveryId::from_i32(signature[64] as i32)?)?; let pubkey = &SECP256K1.recover(&SecpMessage::from_slice(&message[..])?, &rsig)?; let serialized = pubkey.serialize_uncompressed(); let mut public = Public::default(); @@ -262,12 +259,9 @@ pub fn recover(signature: &Signature, message: &Message) -> Result Result { - let rsig = RecoverableSignature::from_compact( - &signature[0..64], - RecoveryId::from_i32(signature[64] as i32)?, - )?; + let rsig = RecoverableSignature::from_compact(&signature[0..64], RecoveryId::from_i32(signature[64] as i32)?)?; let pubkey = &SECP256K1.recover(&message.into(), &rsig)?; let serialized = pubkey.serialize_uncompressed(); let mut public = Public::zero(); @@ -279,8 +273,8 @@ pub fn recover_allowing_all_zero_message( mod tests { use super::super::{Generator, Message, Random, SECP256K1}; use super::{ - recover, recover_allowing_all_zero_message, sign, verify_address, verify_public, - Secret, Signature, ZeroesAllowedMessage + recover, recover_allowing_all_zero_message, sign, verify_address, verify_public, Secret, Signature, + ZeroesAllowedMessage, }; use secp256k1::SecretKey; use std::str::FromStr; @@ -348,10 +342,7 @@ mod tests { let keypair = Random.generate(); let signature = sign_zero_message(keypair.secret()); let zero_message = ZeroesAllowedMessage(Message::zero()); - assert_eq!( - keypair.public(), - &recover_allowing_all_zero_message(&signature, zero_message).unwrap() - ) + assert_eq!(keypair.public(), &recover_allowing_all_zero_message(&signature, zero_message).unwrap()) } #[test] diff --git a/parity-crypto/src/publickey/mod.rs b/parity-crypto/src/publickey/mod.rs index a53f34a63..53dd12209 100644 --- a/parity-crypto/src/publickey/mod.rs +++ b/parity-crypto/src/publickey/mod.rs @@ -21,8 +21,7 @@ pub mod ecies; pub mod error; pub use self::ecdsa_signature::{ - recover, recover_allowing_all_zero_message, sign, verify_address, verify_public, - Signature + recover, recover_allowing_all_zero_message, sign, verify_address, verify_public, Signature, }; pub use self::error::Error; pub use self::extended_keys::{Derivation, DerivationError, ExtendedKeyPair, ExtendedPublic, ExtendedSecret};