From ff02f44972b0dd4288de4cf4f4f2ebc0d406025d Mon Sep 17 00:00:00 2001 From: Artem Vorotnikov Date: Sat, 7 Nov 2020 17:49:12 +0300 Subject: [PATCH 1/4] Bump secp256k1 to 0.19, use global context --- parity-crypto/Cargo.toml | 2 +- parity-crypto/src/publickey/ec_math_utils.rs | 3 ++- parity-crypto/src/publickey/ecdh.rs | 2 +- parity-crypto/src/publickey/ecdsa_signature.rs | 7 ++++--- parity-crypto/src/publickey/extended_keys.rs | 6 ++++-- parity-crypto/src/publickey/keypair.rs | 4 ++-- parity-crypto/src/publickey/keypair_generator.rs | 3 ++- parity-crypto/src/publickey/mod.rs | 5 ----- 8 files changed, 16 insertions(+), 16 deletions(-) diff --git a/parity-crypto/Cargo.toml b/parity-crypto/Cargo.toml index 4df7a4522..3c929998e 100644 --- a/parity-crypto/Cargo.toml +++ b/parity-crypto/Cargo.toml @@ -26,7 +26,7 @@ rand = "0.7.2" ripemd160 = "0.8.0" rustc-hex = { version = "2.1.0", default-features = false, optional = true } scrypt = { version = "0.2.0", default-features = false } -secp256k1 = { version = "0.17.2", optional = true, features = ["recovery", "rand-std"] } +secp256k1 = { version = "0.19", optional = true, features = ["global-context", "recovery", "rand-std"] } sha2 = "0.8.0" subtle = "2.2.1" tiny-keccak = { version = "2.0", features = ["keccak"] } diff --git a/parity-crypto/src/publickey/ec_math_utils.rs b/parity-crypto/src/publickey/ec_math_utils.rs index d9dd7e2e4..af2e2bf96 100644 --- a/parity-crypto/src/publickey/ec_math_utils.rs +++ b/parity-crypto/src/publickey/ec_math_utils.rs @@ -8,11 +8,12 @@ //! Multiple primitives for work with public and secret keys and with secp256k1 curve points -use super::{Error, Public, Secret, SECP256K1}; +use super::{Error, Public, Secret}; use ethereum_types::{BigEndianHash as _, H256, U256}; use lazy_static::lazy_static; use secp256k1::constants::CURVE_ORDER as SECP256K1_CURVE_ORDER; use secp256k1::key; +use secp256k1::SECP256K1; /// Generation point array combined from X and Y coordinates /// Equivalent to uncompressed form, see https://tools.ietf.org/id/draft-jivsov-ecc-compact-05.html#rfc.section.3 diff --git a/parity-crypto/src/publickey/ecdh.rs b/parity-crypto/src/publickey/ecdh.rs index f5890a207..a44eaabd3 100644 --- a/parity-crypto/src/publickey/ecdh.rs +++ b/parity-crypto/src/publickey/ecdh.rs @@ -21,7 +21,7 @@ pub fn agree(secret: &Secret, public: &Public) -> Result { let publ = key::PublicKey::from_slice(&pdata)?; let sec = key::SecretKey::from_slice(secret.as_bytes())?; - let shared = ecdh::SharedSecret::new_with_hash(&publ, &sec, |x, _| x.into())?; + let shared = ecdh::SharedSecret::new_with_hash(&publ, &sec, |x, _| x.into()); Secret::import_key(&shared[0..32]).map_err(|_| Error::Secp(secp256k1::Error::InvalidSecretKey)) } diff --git a/parity-crypto/src/publickey/ecdsa_signature.rs b/parity-crypto/src/publickey/ecdsa_signature.rs index 7968967a7..ea3ea7c7f 100644 --- a/parity-crypto/src/publickey/ecdsa_signature.rs +++ b/parity-crypto/src/publickey/ecdsa_signature.rs @@ -8,12 +8,13 @@ //! 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, ZeroesAllowedMessage, SECP256K1}; +use super::{public_to_address, Address, Error, Message, Public, Secret, ZeroesAllowedMessage}; use ethereum_types::{H256, H520}; use rustc_hex::{FromHex, ToHex}; use secp256k1::key::{PublicKey, SecretKey}; use secp256k1::{ recovery::{RecoverableSignature, RecoveryId}, + SECP256K1, Error as SecpError, Message as SecpMessage, }; use std::cmp::PartialEq; @@ -271,12 +272,12 @@ pub fn recover_allowing_all_zero_message( #[cfg(test)] mod tests { - use super::super::{Generator, Message, Random, SECP256K1}; + use super::super::{Generator, Message, Random}; use super::{ recover, recover_allowing_all_zero_message, sign, verify_address, verify_public, Secret, Signature, ZeroesAllowedMessage, }; - use secp256k1::SecretKey; + use secp256k1::{SECP256K1, SecretKey}; use std::str::FromStr; // Copy of `sign()` that allows signing all-zero Messages. diff --git a/parity-crypto/src/publickey/extended_keys.rs b/parity-crypto/src/publickey/extended_keys.rs index d83aa6a37..adc4f3862 100644 --- a/parity-crypto/src/publickey/extended_keys.rs +++ b/parity-crypto/src/publickey/extended_keys.rs @@ -202,11 +202,13 @@ impl ExtendedKeyPair { // https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki mod derivation { use super::super::ec_math_utils::CURVE_ORDER; - use super::super::SECP256K1; use super::{Derivation, Label}; use crate::{hmac, Keccak256}; use ethereum_types::{BigEndianHash, H256, H512, U256, U512}; - use secp256k1::key::{PublicKey, SecretKey}; + use secp256k1::{ + key::{PublicKey, SecretKey}, + SECP256K1, + }; use std::convert::TryInto; #[derive(Debug)] diff --git a/parity-crypto/src/publickey/keypair.rs b/parity-crypto/src/publickey/keypair.rs index 330316012..dbbf637dc 100644 --- a/parity-crypto/src/publickey/keypair.rs +++ b/parity-crypto/src/publickey/keypair.rs @@ -8,9 +8,9 @@ //! Key pair (public + secret) description. -use super::{Address, Error, Public, Secret, SECP256K1}; +use super::{Address, Error, Public, Secret}; use crate::Keccak256; -use secp256k1::key; +use secp256k1::{key, SECP256K1}; use std::fmt; /// Convert public key into the address diff --git a/parity-crypto/src/publickey/keypair_generator.rs b/parity-crypto/src/publickey/keypair_generator.rs index 9dea21de6..6ebef0985 100644 --- a/parity-crypto/src/publickey/keypair_generator.rs +++ b/parity-crypto/src/publickey/keypair_generator.rs @@ -8,7 +8,8 @@ //! Random key pair generator. Relies on the secp256k1 C-library to generate random data. -use super::{Generator, KeyPair, SECP256K1}; +use super::{Generator, KeyPair}; +use secp256k1::SECP256K1; /// Randomly generates new keypair, instantiating the RNG each time. pub struct Random; diff --git a/parity-crypto/src/publickey/mod.rs b/parity-crypto/src/publickey/mod.rs index ec62012f3..b798585bd 100644 --- a/parity-crypto/src/publickey/mod.rs +++ b/parity-crypto/src/publickey/mod.rs @@ -30,7 +30,6 @@ pub use self::keypair_generator::Random; pub use self::secret_key::{Secret, ZeroizeSecretKey}; use ethereum_types::H256; -use lazy_static::lazy_static; pub use ethereum_types::{Address, Public}; pub type Message = H256; @@ -57,10 +56,6 @@ const MINUS_ONE_KEY: &'static [u8] = &[ 0xe6, 0xaf, 0x48, 0xa0, 0x3b, 0xbf, 0xd2, 0x5e, 0x8c, 0xd0, 0x36, 0x41, 0x40, ]; -lazy_static! { - static ref SECP256K1: secp256k1::Secp256k1 = secp256k1::Secp256k1::new(); -} - /// Generates new keypair. pub trait Generator { /// Should be called to generate new keypair. From a4799d2db5e93b27b849de1298e8a3f828c1223f Mon Sep 17 00:00:00 2001 From: Artem Vorotnikov Date: Mon, 9 Nov 2020 11:51:37 +0300 Subject: [PATCH 2/4] fmt --- parity-crypto/src/publickey/ecdsa_signature.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/parity-crypto/src/publickey/ecdsa_signature.rs b/parity-crypto/src/publickey/ecdsa_signature.rs index ea3ea7c7f..39b11e3a8 100644 --- a/parity-crypto/src/publickey/ecdsa_signature.rs +++ b/parity-crypto/src/publickey/ecdsa_signature.rs @@ -14,8 +14,7 @@ use rustc_hex::{FromHex, ToHex}; use secp256k1::key::{PublicKey, SecretKey}; use secp256k1::{ recovery::{RecoverableSignature, RecoveryId}, - SECP256K1, - Error as SecpError, Message as SecpMessage, + Error as SecpError, Message as SecpMessage, SECP256K1, }; use std::cmp::PartialEq; use std::fmt; @@ -277,7 +276,7 @@ mod tests { recover, recover_allowing_all_zero_message, sign, verify_address, verify_public, Secret, Signature, ZeroesAllowedMessage, }; - use secp256k1::{SECP256K1, SecretKey}; + use secp256k1::{SecretKey, SECP256K1}; use std::str::FromStr; // Copy of `sign()` that allows signing all-zero Messages. From 2e6511f943f8a7d80c5134d2699f3c00678db4a7 Mon Sep 17 00:00:00 2001 From: Artem Vorotnikov Date: Thu, 12 Nov 2020 16:39:42 +0300 Subject: [PATCH 3/4] Remove zero-sig infrastructure, fix tests --- .../src/publickey/ecdsa_signature.rs | 66 +++++-------------- parity-crypto/src/publickey/mod.rs | 30 ++------- 2 files changed, 24 insertions(+), 72 deletions(-) diff --git a/parity-crypto/src/publickey/ecdsa_signature.rs b/parity-crypto/src/publickey/ecdsa_signature.rs index 39b11e3a8..b7d924c6c 100644 --- a/parity-crypto/src/publickey/ecdsa_signature.rs +++ b/parity-crypto/src/publickey/ecdsa_signature.rs @@ -8,19 +8,21 @@ //! 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, ZeroesAllowedMessage}; +use super::{public_to_address, Address, Error, Message, Public, Secret}; use ethereum_types::{H256, H520}; use rustc_hex::{FromHex, ToHex}; -use secp256k1::key::{PublicKey, SecretKey}; use secp256k1::{ + key::{PublicKey, SecretKey}, recovery::{RecoverableSignature, RecoveryId}, Error as SecpError, Message as SecpMessage, SECP256K1, }; -use std::cmp::PartialEq; -use std::fmt; -use std::hash::{Hash, Hasher}; -use std::ops::{Deref, DerefMut}; -use std::str::FromStr; +use std::{ + cmp::PartialEq, + fmt, + hash::{Hash, Hasher}, + ops::{Deref, DerefMut}, + str::FromStr, +}; /// Signature encoded as RSV components #[repr(C)] @@ -254,48 +256,14 @@ 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 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}; use super::{ - recover, recover_allowing_all_zero_message, sign, verify_address, verify_public, Secret, Signature, - ZeroesAllowedMessage, + super::{Generator, Message, Random}, + recover, sign, verify_address, verify_public, Signature, }; - use secp256k1::{SecretKey, SECP256K1}; 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 @@ -330,19 +298,19 @@ mod tests { } #[test] - fn sign_and_recover_public_fails_with_zeroed_messages() { + fn sign_and_recover_public_works_with_zeroed_messages() { let keypair = Random.generate(); - let signature = sign_zero_message(keypair.secret()); + let signature = sign(keypair.secret(), &Message::zero()).unwrap(); let zero_message = Message::zero(); - assert!(&recover(&signature, &zero_message).is_err()); + assert_eq!(keypair.public(), &recover(&signature, &zero_message).unwrap()); } #[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()) + let signature = sign(keypair.secret(), &Message::zero()).unwrap(); + let zero_message = Message::zero(); + assert_eq!(keypair.public(), &recover(&signature, &zero_message).unwrap()) } #[test] diff --git a/parity-crypto/src/publickey/mod.rs b/parity-crypto/src/publickey/mod.rs index b798585bd..c7981515c 100644 --- a/parity-crypto/src/publickey/mod.rs +++ b/parity-crypto/src/publickey/mod.rs @@ -20,36 +20,20 @@ pub mod ecdh; pub mod ecies; pub mod error; -pub use self::ecdsa_signature::{ - recover, recover_allowing_all_zero_message, sign, verify_address, verify_public, Signature, +pub use self::{ + ecdsa_signature::{recover, sign, verify_address, verify_public, Signature}, + error::Error, + extended_keys::{Derivation, DerivationError, ExtendedKeyPair, ExtendedPublic, ExtendedSecret}, + keypair::{public_to_address, KeyPair}, + keypair_generator::Random, + secret_key::{Secret, ZeroizeSecretKey}, }; -pub use self::error::Error; -pub use self::extended_keys::{Derivation, DerivationError, ExtendedKeyPair, ExtendedPublic, ExtendedSecret}; -pub use self::keypair::{public_to_address, KeyPair}; -pub use self::keypair_generator::Random; -pub use self::secret_key::{Secret, ZeroizeSecretKey}; use ethereum_types::H256; pub use ethereum_types::{Address, Public}; pub type Message = H256; -use secp256k1::ThirtyTwoByteHash; - -/// 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()`. -pub struct ZeroesAllowedMessage(pub H256); -impl ThirtyTwoByteHash for ZeroesAllowedMessage { - fn into_32(self) -> [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 52ab2100769a2aa8805f56217790ae19a2c13ee7 Mon Sep 17 00:00:00 2001 From: Artem Vorotnikov Date: Thu, 12 Nov 2020 18:38:50 +0300 Subject: [PATCH 4/4] Update changelog --- parity-crypto/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/parity-crypto/CHANGELOG.md b/parity-crypto/CHANGELOG.md index 893082e82..19e2174e4 100644 --- a/parity-crypto/CHANGELOG.md +++ b/parity-crypto/CHANGELOG.md @@ -5,6 +5,7 @@ The format is based on [Keep a Changelog]. [Keep a Changelog]: http://keepachangelog.com/en/1.0.0/ ## [Unreleased] +- Bump `rust-secp256k1` to v0.19, remove infrastructure for handling zero signatures (breaking). [#438](https://github.com/paritytech/parity-common/pull/438) ## [0.6.2] - 2020-06-19 - Put `Secret` memory on heap. [#400](https://github.com/paritytech/parity-common/pull/400)