From 264c8366571bc1f258d958d73323dbf31d26c51b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Thu, 26 Dec 2019 13:58:54 +0100 Subject: [PATCH 1/3] Don't use compressed ecdsa public key in verify --- primitives/runtime/src/lib.rs | 18 ++++++++++++++++-- primitives/runtime/src/traits.rs | 16 ++++++++++++++-- 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/primitives/runtime/src/lib.rs b/primitives/runtime/src/lib.rs index 1e8178f05e168..a81b1567f8350 100644 --- a/primitives/runtime/src/lib.rs +++ b/primitives/runtime/src/lib.rs @@ -310,7 +310,7 @@ impl Verify for MultiSignature { (MultiSignature::Sr25519(ref sig), who) => sig.verify(msg, &sr25519::Public::from_slice(who.as_ref())), (MultiSignature::Ecdsa(ref sig), who) => { let m = sp_io::hashing::blake2_256(msg.get()); - match sp_io::crypto::secp256k1_ecdsa_recover_compressed(sig.as_ref(), &m) { + match sp_io::crypto::secp256k1_ecdsa_recover(sig.as_ref(), &m) { Ok(pubkey) => &sp_io::hashing::blake2_256(pubkey.as_ref()) == >::as_ref(who), @@ -688,8 +688,9 @@ pub fn print(print: impl traits::Printable) { #[cfg(test)] mod tests { - use crate::DispatchError; + use super::*; use codec::{Encode, Decode}; + use sp_core::crypto::Pair; #[test] fn opaque_extrinsic_serialization() { @@ -716,4 +717,17 @@ mod tests { }, ); } + + #[test] + fn multi_signature_ecdsa_verify_works() { + let msg = &b"test-message"[..]; + let (pair, _) = ecdsa::Pair::generate(); + + let signature = pair.sign(&msg); + assert!(ecdsa::Pair::verify(&signature, msg, &pair.public())); + + let multi_sig = MultiSignature::from(signature); + let multi_signer = MultiSigner::from(pair.public()); + assert!(multi_sig.verify(msg, &multi_signer.into_account())); + } } diff --git a/primitives/runtime/src/traits.rs b/primitives/runtime/src/traits.rs index 2b9ea98f05431..d40e5b45035d2 100644 --- a/primitives/runtime/src/traits.rs +++ b/primitives/runtime/src/traits.rs @@ -98,7 +98,7 @@ impl Verify for sp_core::sr25519::Signature { impl Verify for sp_core::ecdsa::Signature { type Signer = sp_core::ecdsa::Public; fn verify>(&self, mut msg: L, signer: &sp_core::ecdsa::Public) -> bool { - match sp_io::crypto::secp256k1_ecdsa_recover_compressed( + match sp_io::crypto::secp256k1_ecdsa_recover( self.as_ref(), &sp_io::hashing::blake2_256(msg.get()), ) { @@ -1307,8 +1307,9 @@ pub trait BlockIdTo { #[cfg(test)] mod tests { - use super::AccountIdConversion; + use super::*; use crate::codec::{Encode, Decode, Input}; + use sp_core::{crypto::Pair, ecdsa}; mod t { use sp_core::crypto::KeyTypeId; @@ -1388,4 +1389,15 @@ mod tests { assert_eq!(t.remaining_len(), Ok(None)); assert_eq!(buffer, [0, 0]); } + + #[test] + fn ecdsa_verify_works() { + let msg = &b"test-message"[..]; + let (pair, _) = ecdsa::Pair::generate(); + + let signature = pair.sign(&msg); + assert!(ecdsa::Pair::verify(&signature, msg, &pair.public())); + + assert!(signature.verify(msg, &pair.public())); + } } From f760d65c838ae522fb48022d778da2d0a92589ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Fri, 27 Dec 2019 09:19:37 +0100 Subject: [PATCH 2/3] Make `ECDSA` public support compressed --- primitives/core/Cargo.toml | 5 +- primitives/core/src/ecdsa.rs | 191 ++++++++++++++++-------------- primitives/core/src/hexdisplay.rs | 2 +- primitives/runtime/src/lib.rs | 11 +- primitives/runtime/src/traits.rs | 5 +- 5 files changed, 119 insertions(+), 95 deletions(-) diff --git a/primitives/core/Cargo.toml b/primitives/core/Cargo.toml index 706a9cb276f71..8242ff8044466 100644 --- a/primitives/core/Cargo.toml +++ b/primitives/core/Cargo.toml @@ -28,13 +28,13 @@ parking_lot = { version = "0.9.0", optional = true } sp-debug-derive = { version = "2.0.0", path = "../debug-derive" } sp-externalities = { version = "2.0.0", optional = true, path = "../externalities" } sp-storage = { version = "2.0.0", default-features = false, path = "../storage" } +libsecp256k1 = { version = "0.3.2", default-features = false } # full crypto ed25519-dalek = { version = "1.0.0-pre.3", default-features = false, features = ["u64_backend", "alloc"], optional = true } blake2-rfc = { version = "0.2.18", default-features = false, optional = true } tiny-keccak = { version = "2.0.1", features = ["keccak"], optional = true } schnorrkel = { version = "0.8.5", features = ["preaudit_deprecated", "u64_backend"], default-features = false, optional = true } -libsecp256k1 = { version = "0.3.2", default-features = false, optional = true } sha2 = { version = "0.8.0", default-features = false, optional = true } hex = { version = "0.4", default-features = false, optional = true } twox-hash = { version = "1.5.0", default-features = false, optional = true } @@ -90,7 +90,7 @@ std = [ "schnorrkel/std", "regex", "num-traits/std", - "libsecp256k1", + "libsecp256k1/std", "tiny-keccak", "sp-debug-derive/std", "sp-externalities", @@ -107,7 +107,6 @@ full_crypto = [ "blake2-rfc", "tiny-keccak", "schnorrkel", - "libsecp256k1", "hex", "sha2", "twox-hash", diff --git a/primitives/core/src/ecdsa.rs b/primitives/core/src/ecdsa.rs index e097d0c5e6fe6..fbdb8a56f75ff 100644 --- a/primitives/core/src/ecdsa.rs +++ b/primitives/core/src/ecdsa.rs @@ -46,9 +46,14 @@ use secp256k1::{PublicKey, SecretKey}; #[cfg(feature = "full_crypto")] type Seed = [u8; 32]; -/// The ECDSA 64-byte raw public key. +/// The ECDSA public key. #[derive(Clone, Encode, Decode)] -pub struct Public(pub [u8; 64]); +pub enum Public { + /// A full raw ECDSA public key. + Full([u8; 64]), + /// A compressed ECDSA public key. + Compressed([u8; 33]), +} impl PartialOrd for Public { fn partial_cmp(&self, other: &Self) -> Option { @@ -58,47 +63,113 @@ impl PartialOrd for Public { impl Ord for Public { fn cmp(&self, other: &Self) -> Ordering { - self.0[..].cmp(&other.0[..]) + self.as_ref().cmp(&other.as_ref()) } } impl PartialEq for Public { fn eq(&self, other: &Self) -> bool { - &self.0[..] == &other.0[..] + self.as_ref() == other.as_ref() } } impl Eq for Public {} -impl Default for Public { - fn default() -> Self { - Public([0u8; 64]) +/// An error type for SS58 decoding. +#[cfg(feature = "std")] +#[derive(Clone, Copy, Eq, PartialEq, Debug)] +pub enum PublicError { + /// Bad alphabet. + BadBase58, + /// Bad length. + BadLength, + /// Unknown version. + UnknownVersion, + /// Invalid checksum. + InvalidChecksum, +} + +impl Public { + /// A new instance from the given 64-byte `data`. + /// + /// NOTE: No checking goes on to ensure this is a real public key. Only use it if + /// you are certain that the array actually is a pubkey. GIGO! + pub fn from_raw(data: [u8; 64]) -> Self { + Self::Full(data) + } + + /// A new instance from the given 65-byte `data`. + /// + /// NOTE: No checking goes on to ensure this is a real public key. Only use it if + /// you are certain that the array actually is a pubkey. GIGO! + pub fn from_full(data: [u8; 65]) -> Self { + let raw_key = &data[1..]; + let mut key = [0u8; 64]; + key.copy_from_slice(raw_key); + Self::Full(key) + } + + /// Return in compressed format. + /// + /// Returns an error if `self` is an invalid full public key. + pub fn as_compressed(&self) -> Result<[u8; 33], ()> { + match self { + Self::Full(d) => secp256k1::PublicKey::parse_slice(d, None) + .map(|k| k.serialize_compressed()) + .map_err(|_| ()), + Self::Compressed(d) => Ok(*d), + } + } + + /// Convert `Self` into a compressed public key. + /// + /// Returns an error if `self` is an invalid full public key. + pub fn into_compressed(self) -> Result { + self.as_compressed().map(Self::Compressed) } } -/// A key pair. -#[cfg(feature = "full_crypto")] -#[derive(Clone)] -pub struct Pair { - public: PublicKey, - secret: SecretKey, +impl TraitPublic for Public { + /// A new instance from the given slice that should be 33 bytes long. + /// + /// NOTE: No checking goes on to ensure this is a real public key. Only use it if + /// you are certain that the array actually is a pubkey. GIGO! + fn from_slice(data: &[u8]) -> Self { + if data.len() == 33 { + let mut r = [0u8; 33]; + r.copy_from_slice(data); + Self::Compressed(r) + } else { + let mut r = [0u8; 64]; + r.copy_from_slice(data); + Self::Full(r) + } + } } -impl AsRef<[u8; 64]> for Public { - fn as_ref(&self) -> &[u8; 64] { - &self.0 +impl Derive for Public {} + +impl Default for Public { + fn default() -> Self { + Public::Full([0u8; 64]) } } impl AsRef<[u8]> for Public { fn as_ref(&self) -> &[u8] { - &self.0[..] + match self { + Self::Full(d) => &d[..], + Self::Compressed(d) => &d[..], + } } } impl AsMut<[u8]> for Public { fn as_mut(&mut self) -> &mut [u8] { - &mut self.0[..] + match self { + Self::Full(d) => &mut d[..], + Self::Compressed(d) => &mut d[..], + } } } @@ -106,22 +177,14 @@ impl sp_std::convert::TryFrom<&[u8]> for Public { type Error = (); fn try_from(data: &[u8]) -> Result { - if data.len() == 64 { - let mut inner = [0u8; 64]; - inner.copy_from_slice(data); - Ok(Public(inner)) + if data.len() == 33 || data.len() == 64 { + Ok(Self::from_slice(data)) } else { Err(()) } } } -impl From for [u8; 64] { - fn from(x: Public) -> Self { - x.0 - } -} - #[cfg(feature = "full_crypto")] impl From for Public { fn from(x: Pair) -> Self { @@ -131,7 +194,7 @@ impl From for Public { impl UncheckedFrom<[u8; 64]> for Public { fn unchecked_from(x: [u8; 64]) -> Self { - Public(x) + Public::Full(x) } } @@ -146,7 +209,7 @@ impl std::fmt::Display for Public { impl std::fmt::Debug for Public { fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { let s = self.to_ss58check(); - write!(f, "{} ({}...)", crate::hexdisplay::HexDisplay::from(&&self.0[..]), &s[0..8]) + write!(f, "{} ({}...)", crate::hexdisplay::HexDisplay::from(&self.as_ref()), &s[0..8]) } } @@ -168,7 +231,7 @@ impl<'de> Deserialize<'de> for Public { #[cfg(feature = "full_crypto")] impl sp_std::hash::Hash for Public { fn hash(&self, state: &mut H) { - self.0.hash(state); + self.as_ref().hash(state); } } @@ -317,60 +380,6 @@ impl<'a> TryFrom<&'a Signature> for (secp256k1::Signature, secp256k1::RecoveryId } } -/// An error type for SS58 decoding. -#[cfg(feature = "std")] -#[derive(Clone, Copy, Eq, PartialEq, Debug)] -pub enum PublicError { - /// Bad alphabet. - BadBase58, - /// Bad length. - BadLength, - /// Unknown version. - UnknownVersion, - /// Invalid checksum. - InvalidChecksum, -} - -impl Public { - /// A new instance from the given 64-byte `data`. - /// - /// NOTE: No checking goes on to ensure this is a real public key. Only use it if - /// you are certain that the array actually is a pubkey. GIGO! - pub fn from_raw(data: [u8; 64]) -> Self { - Public(data) - } - - /// A new instance from the given 65-byte `data`. - /// - /// NOTE: No checking goes on to ensure this is a real public key. Only use it if - /// you are certain that the array actually is a pubkey. GIGO! - pub fn from_full(data: [u8; 65]) -> Self { - let raw_key = &data[1..]; - let mut key = [0u8; 64]; - key.copy_from_slice(raw_key); - Public(key) - } - - /// Return a slice filled with raw data. - pub fn as_array_ref(&self) -> &[u8; 64] { - self.as_ref() - } -} - -impl TraitPublic for Public { - /// A new instance from the given slice that should be 33 bytes long. - /// - /// NOTE: No checking goes on to ensure this is a real public key. Only use it if - /// you are certain that the array actually is a pubkey. GIGO! - fn from_slice(data: &[u8]) -> Self { - let mut r = [0u8; 64]; - r.copy_from_slice(data); - Public(r) - } -} - -impl Derive for Public {} - /// Derive a single hard junction. #[cfg(feature = "full_crypto")] fn derive_hard_junction(secret_seed: &Seed, cc: &[u8; 32]) -> Seed { @@ -388,6 +397,14 @@ pub enum DeriveError { SoftKeyInPath, } +/// A key pair. +#[cfg(feature = "full_crypto")] +#[derive(Clone)] +pub struct Pair { + public: PublicKey, + secret: SecretKey, +} + #[cfg(feature = "full_crypto")] impl TraitPair for Pair { type Public = Public; @@ -473,7 +490,9 @@ impl TraitPair for Pair { let message = secp256k1::Message::parse(&blake2_256(message.as_ref())); let sig: (_, _) = match sig.try_into() { Ok(x) => x, _ => return false }; match secp256k1::recover(&message, &sig.0, &sig.1) { - Ok(actual) => &pubkey.0[..] == &actual.serialize()[1..], + Ok(actual) => pubkey.as_compressed() + .map(|p| &p[..] == &actual.serialize_compressed()[..]) + .unwrap_or(false), _ => false, } } diff --git a/primitives/core/src/hexdisplay.rs b/primitives/core/src/hexdisplay.rs index 104aaf812e644..83c2363a4c9ec 100644 --- a/primitives/core/src/hexdisplay.rs +++ b/primitives/core/src/hexdisplay.rs @@ -58,7 +58,7 @@ pub trait AsBytesRef { fn as_bytes_ref(&self) -> &[u8]; } -impl<'a> AsBytesRef for &'a [u8] { +impl AsBytesRef for &[u8] { fn as_bytes_ref(&self) -> &[u8] { self } } diff --git a/primitives/runtime/src/lib.rs b/primitives/runtime/src/lib.rs index a81b1567f8350..686265192d68c 100644 --- a/primitives/runtime/src/lib.rs +++ b/primitives/runtime/src/lib.rs @@ -212,7 +212,7 @@ pub enum MultiSigner { Ed25519(ed25519::Public), /// An Sr25519 identity. Sr25519(sr25519::Public), - /// An SECP256k1/ECDSA identity (actually, the Blake2 hash of the pub key). + /// An SECP256k1/ECDSA identity (actually, the Blake2 hash of the compressed pub key). Ecdsa(ecdsa::Public), } @@ -246,7 +246,9 @@ impl traits::IdentifyAccount for MultiSigner { match self { MultiSigner::Ed25519(who) => <[u8; 32]>::from(who).into(), MultiSigner::Sr25519(who) => <[u8; 32]>::from(who).into(), - MultiSigner::Ecdsa(who) => sp_io::hashing::blake2_256(who.as_ref()).into(), + MultiSigner::Ecdsa(who) => sp_io::hashing::blake2_256( + &who.as_compressed().expect("what should we do?")[..], + ).into(), } } } @@ -310,7 +312,7 @@ impl Verify for MultiSignature { (MultiSignature::Sr25519(ref sig), who) => sig.verify(msg, &sr25519::Public::from_slice(who.as_ref())), (MultiSignature::Ecdsa(ref sig), who) => { let m = sp_io::hashing::blake2_256(msg.get()); - match sp_io::crypto::secp256k1_ecdsa_recover(sig.as_ref(), &m) { + match sp_io::crypto::secp256k1_ecdsa_recover_compressed(sig.as_ref(), &m) { Ok(pubkey) => &sp_io::hashing::blake2_256(pubkey.as_ref()) == >::as_ref(who), @@ -729,5 +731,8 @@ mod tests { let multi_sig = MultiSignature::from(signature); let multi_signer = MultiSigner::from(pair.public()); assert!(multi_sig.verify(msg, &multi_signer.into_account())); + + let multi_signer = MultiSigner::from(pair.public().into_compressed().unwrap()); + assert!(multi_sig.verify(msg, &multi_signer.into_account())); } } diff --git a/primitives/runtime/src/traits.rs b/primitives/runtime/src/traits.rs index d40e5b45035d2..a0970accd8740 100644 --- a/primitives/runtime/src/traits.rs +++ b/primitives/runtime/src/traits.rs @@ -98,11 +98,11 @@ impl Verify for sp_core::sr25519::Signature { impl Verify for sp_core::ecdsa::Signature { type Signer = sp_core::ecdsa::Public; fn verify>(&self, mut msg: L, signer: &sp_core::ecdsa::Public) -> bool { - match sp_io::crypto::secp256k1_ecdsa_recover( + match sp_io::crypto::secp256k1_ecdsa_recover_compressed( self.as_ref(), &sp_io::hashing::blake2_256(msg.get()), ) { - Ok(pubkey) => >::as_ref(signer) == &pubkey[..], + Ok(pubkey) => signer.as_compressed().map(|s| &s[..] == &pubkey[..]).unwrap_or(false), _ => false, } } @@ -1399,5 +1399,6 @@ mod tests { assert!(ecdsa::Pair::verify(&signature, msg, &pair.public())); assert!(signature.verify(msg, &pair.public())); + assert!(signature.verify(msg, &pair.public().into_compressed().unwrap())); } } From 4ba6dc99d0297bae0e3169c7a3736d5fe0205a6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Mon, 30 Dec 2019 12:47:19 +0100 Subject: [PATCH 3/3] Make it a proper `expect` message --- primitives/runtime/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/primitives/runtime/src/lib.rs b/primitives/runtime/src/lib.rs index 686265192d68c..80ef992f6b928 100644 --- a/primitives/runtime/src/lib.rs +++ b/primitives/runtime/src/lib.rs @@ -247,7 +247,7 @@ impl traits::IdentifyAccount for MultiSigner { MultiSigner::Ed25519(who) => <[u8; 32]>::from(who).into(), MultiSigner::Sr25519(who) => <[u8; 32]>::from(who).into(), MultiSigner::Ecdsa(who) => sp_io::hashing::blake2_256( - &who.as_compressed().expect("what should we do?")[..], + &who.as_compressed().expect("`who` is a valid `ECDSA` public key; qed")[..], ).into(), } }