From 8ecc450fd9af86d136e4b0875f491d45a9dbb8ca Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bastian=20K=C3=B6cher?= <bkchr@users.noreply.github.com>
Date: Tue, 31 Dec 2019 20:04:53 +0100
Subject: [PATCH] Make `MultiSigner` use compressed ECDSA public key (#4502)
* Don't use compressed ecdsa public key in verify
* Make `ECDSA` public support compressed
* Make it a proper `expect` message
---
substrate/primitives/core/Cargo.toml | 5 +-
substrate/primitives/core/src/ecdsa.rs | 191 +++++++++++---------
substrate/primitives/core/src/hexdisplay.rs | 2 +-
substrate/primitives/runtime/src/lib.rs | 25 ++-
substrate/primitives/runtime/src/traits.rs | 17 +-
5 files changed, 145 insertions(+), 95 deletions(-)
diff --git a/substrate/primitives/core/Cargo.toml b/substrate/primitives/core/Cargo.toml
index 706a9cb276f..8242ff80444 100644
--- a/substrate/primitives/core/Cargo.toml
+++ b/substrate/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/substrate/primitives/core/src/ecdsa.rs b/substrate/primitives/core/src/ecdsa.rs
index e097d0c5e6f..fbdb8a56f75 100644
--- a/substrate/primitives/core/src/ecdsa.rs
+++ b/substrate/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<Ordering> {
@@ -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, ()> {
+ 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<Self, Self::Error> {
- 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<Public> for [u8; 64] {
- fn from(x: Public) -> Self {
- x.0
- }
-}
-
#[cfg(feature = "full_crypto")]
impl From<Pair> for Public {
fn from(x: Pair) -> Self {
@@ -131,7 +194,7 @@ impl From<Pair> 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<H: sp_std::hash::Hasher>(&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/substrate/primitives/core/src/hexdisplay.rs b/substrate/primitives/core/src/hexdisplay.rs
index 104aaf812e6..83c2363a4c9 100644
--- a/substrate/primitives/core/src/hexdisplay.rs
+++ b/substrate/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/substrate/primitives/runtime/src/lib.rs b/substrate/primitives/runtime/src/lib.rs
index 1e8178f05e1..80ef992f6b9 100644
--- a/substrate/primitives/runtime/src/lib.rs
+++ b/substrate/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("`who` is a valid `ECDSA` public key; qed")[..],
+ ).into(),
}
}
}
@@ -688,8 +690,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 +719,20 @@ 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()));
+
+ let multi_signer = MultiSigner::from(pair.public().into_compressed().unwrap());
+ assert!(multi_sig.verify(msg, &multi_signer.into_account()));
+ }
}
diff --git a/substrate/primitives/runtime/src/traits.rs b/substrate/primitives/runtime/src/traits.rs
index 2b9ea98f054..a0970accd87 100644
--- a/substrate/primitives/runtime/src/traits.rs
+++ b/substrate/primitives/runtime/src/traits.rs
@@ -102,7 +102,7 @@ impl Verify for sp_core::ecdsa::Signature {
self.as_ref(),
&sp_io::hashing::blake2_256(msg.get()),
) {
- Ok(pubkey) => <dyn AsRef<[u8]>>::as_ref(signer) == &pubkey[..],
+ Ok(pubkey) => signer.as_compressed().map(|s| &s[..] == &pubkey[..]).unwrap_or(false),
_ => false,
}
}
@@ -1307,8 +1307,9 @@ pub trait BlockIdTo<Block: self::Block> {
#[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,16 @@ 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()));
+ assert!(signature.verify(msg, &pair.public().into_compressed().unwrap()));
+ }
}
--
GitLab