Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Commit

Permalink
Only support ECDSA compressed public keys (#4667)
Browse files Browse the repository at this point in the history 
Some fixes after: #4502

This removes the unwanted `expect`s from `MultiSigner`. Instead we
convert from full to compressed in `TryFrom` and can return an error on
invalid input.
  • Loading branch information
@bkchr @gavofyork
bkchr authored and gavofyork committed Jan 20, 2020
1 parent 1f9d09d commit 5cd952b
Show file tree
Hide file tree
Showing 3 changed files with 46 additions and 84 deletions.
120 changes: 42 additions & 78 deletions primitives/core/src/ecdsa.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,14 +46,9 @@ use secp256k1::{PublicKey, SecretKey};
#[cfg(feature = "full_crypto")]
type Seed = [u8; 32];

/// The ECDSA public key.
/// The ECDSA compressed public key.
#[derive(Clone, Encode, Decode)]
pub enum Public {
/// A full raw ECDSA public key.
Full([u8; 64]),
/// A compressed ECDSA public key.
Compressed([u8; 33]),
}
pub struct Public([u8; 33]);

impl PartialOrd for Public {
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
Expand Down Expand Up @@ -90,42 +85,12 @@ pub enum PublicError {
}

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`.
/// A new instance from the given 33-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)
pub fn from_raw(data: [u8; 33]) -> Self {
Self(data)
}
}

Expand All @@ -135,52 +100,43 @@ impl TraitPublic for Public {
/// 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)
}
let mut r = [0u8; 33];
r.copy_from_slice(data);
Self(r)
}
}

impl Derive for Public {}

impl Default for Public {
fn default() -> Self {
Public::Full([0u8; 64])
Public([0u8; 33])
}
}

impl AsRef<[u8]> for Public {
fn as_ref(&self) -> &[u8] {
match self {
Self::Full(d) => &d[..],
Self::Compressed(d) => &d[..],
}
&self.0[..]
}
}

impl AsMut<[u8]> for Public {
fn as_mut(&mut self) -> &mut [u8] {
match self {
Self::Full(d) => &mut d[..],
Self::Compressed(d) => &mut d[..],
}
&mut self.0[..]
}
}

impl sp_std::convert::TryFrom<&[u8]> for Public {
type Error = ();

fn try_from(data: &[u8]) -> Result<Self, Self::Error> {
if data.len() == 33 || data.len() == 64 {
if data.len() == 33 {
Ok(Self::from_slice(data))
} else {
Err(())
secp256k1::PublicKey::parse_slice(data, None)
.map(|k| k.serialize_compressed())
.map(Self)
.map_err(|_| ())
}
}
}
Expand All @@ -192,9 +148,9 @@ impl From<Pair> for Public {
}
}

impl UncheckedFrom<[u8; 64]> for Public {
fn unchecked_from(x: [u8; 64]) -> Self {
Public::Full(x)
impl UncheckedFrom<[u8; 33]> for Public {
fn unchecked_from(x: [u8; 33]) -> Self {
Public(x)
}
}

Expand Down Expand Up @@ -354,8 +310,9 @@ impl Signature {
pub fn recover<M: AsRef<[u8]>>(&self, message: M) -> Option<Public> {
let message = secp256k1::Message::parse(&blake2_256(message.as_ref()));
let sig: (_, _) = self.try_into().ok()?;
secp256k1::recover(&message, &sig.0, &sig.1).ok()
.map(|recovered| Public::from_full(recovered.serialize()))
secp256k1::recover(&message, &sig.0, &sig.1)
.ok()
.map(|recovered| Public(recovered.serialize_compressed()))
}
}

Expand Down Expand Up @@ -476,7 +433,7 @@ impl TraitPair for Pair {

/// Get the public key.
fn public(&self) -> Public {
Public::from_full(self.public.serialize())
Public(self.public.serialize_compressed())
}

/// Sign a message.
Expand All @@ -490,9 +447,7 @@ 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.as_compressed()
.map(|p| &p[..] == &actual.serialize_compressed()[..])
.unwrap_or(false),
Ok(actual) => &pubkey.0[..] == &actual.serialize_compressed()[..],
_ => false,
}
}
Expand Down Expand Up @@ -587,9 +542,12 @@ mod test {
&hex!("9d61b19deffd5a60ba844af492ec2cc44449c5697b326919703bac031cae7f60")
);
let public = pair.public();
assert_eq!(public, Public::from_raw(
hex!("8db55b05db86c0b1786ca49f095d76344c9e6056b2f02701a7e7f3c20aabfd913ebbe148dd17c56551a52952371071a6c604b3f3abe8f2c8fa742158ea6dd7d4")
));
assert_eq!(
public,
Public::try_from(
&hex!("8db55b05db86c0b1786ca49f095d76344c9e6056b2f02701a7e7f3c20aabfd913ebbe148dd17c56551a52952371071a6c604b3f3abe8f2c8fa742158ea6dd7d4")[..],
).unwrap(),
);
let message = b"";
let signature = hex!("3dde91174bd9359027be59a428b8146513df80a2a3c7eda2194f64de04a69ab97b753169e94db6ffd50921a2668a48b94ca11e3d32c1ff19cfe88890aa7e8f3c00");
let signature = Signature::from_raw(signature);
Expand All @@ -604,9 +562,12 @@ mod test {
None
).unwrap();
let public = pair.public();
assert_eq!(public, Public::from_raw(
hex!("8db55b05db86c0b1786ca49f095d76344c9e6056b2f02701a7e7f3c20aabfd913ebbe148dd17c56551a52952371071a6c604b3f3abe8f2c8fa742158ea6dd7d4")
));
assert_eq!(
public,
Public::try_from(
&hex!("8db55b05db86c0b1786ca49f095d76344c9e6056b2f02701a7e7f3c20aabfd913ebbe148dd17c56551a52952371071a6c604b3f3abe8f2c8fa742158ea6dd7d4")[..],
).unwrap(),
);
let message = b"";
let signature = hex!("3dde91174bd9359027be59a428b8146513df80a2a3c7eda2194f64de04a69ab97b753169e94db6ffd50921a2668a48b94ca11e3d32c1ff19cfe88890aa7e8f3c00");
let signature = Signature::from_raw(signature);
Expand All @@ -628,9 +589,12 @@ mod test {
fn seeded_pair_should_work() {
let pair = Pair::from_seed(b"12345678901234567890123456789012");
let public = pair.public();
assert_eq!(public, Public::from_raw(
hex!("5676109c54b9a16d271abeb4954316a40a32bcce023ac14c8e26e958aa68fba995840f3de562156558efbfdac3f16af0065e5f66795f4dd8262a228ef8c6d813")
));
assert_eq!(
public,
Public::try_from(
&hex!("5676109c54b9a16d271abeb4954316a40a32bcce023ac14c8e26e958aa68fba995840f3de562156558efbfdac3f16af0065e5f66795f4dd8262a228ef8c6d813")[..],
).unwrap(),
);
let message = hex!("2f8c6129d816cf51c374bc7f08c3e63ed156cf78aefb4a6550d97b87997977ee00000000000000000200d75a980182b10ab7d54bfed3c964073a0ee172f3daa62325af021a68f707511a4500000000000000");
let signature = pair.sign(&message[..]);
println!("Correct signature: {:?}", signature);
Expand Down
6 changes: 2 additions & 4 deletions primitives/runtime/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -238,9 +238,7 @@ 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_compressed().expect("`who` is a valid `ECDSA` public key; qed")[..],
).into(),
MultiSigner::Ecdsa(who) => sp_io::hashing::blake2_256(&who.as_ref()[..]).into(),
}
}
}
Expand Down Expand Up @@ -724,7 +722,7 @@ mod tests {
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());
let multi_signer = MultiSigner::from(pair.public());
assert!(multi_sig.verify(msg, &multi_signer.into_account()));
}
}
4 changes: 2 additions & 2 deletions primitives/runtime/src/traits.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ impl Verify for sp_core::ecdsa::Signature {
self.as_ref(),
&sp_io::hashing::blake2_256(msg.get()),
) {
Ok(pubkey) => signer.as_compressed().map(|s| &s[..] == &pubkey[..]).unwrap_or(false),
Ok(pubkey) => &signer.as_ref()[..] == &pubkey[..],
_ => false,
}
}
Expand Down Expand Up @@ -1357,6 +1357,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()));
assert!(signature.verify(msg, &pair.public()));
}
}

0 comments on commit 5cd952b

Please sign in to comment.