From f93b959b4a921c94f8659018184cb8061db15b47 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Wed, 15 Aug 2018 17:05:17 +0000 Subject: [PATCH 1/3] disallow Messages that are not valid secret keys to prevent mistakes related to 0 --- src/lib.rs | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 81cbc57..b75993f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -504,6 +504,10 @@ impl Message { /// Converts a `MESSAGE_SIZE`-byte slice to a message object #[inline] pub fn from_slice(data: &[u8]) -> Result { + if data == &[0; constants::MESSAGE_SIZE] { + return Err(Error::InvalidMessage); + } + match data.len() { constants::MESSAGE_SIZE => { let mut ret = [0; constants::MESSAGE_SIZE]; @@ -515,13 +519,6 @@ impl Message { } } -/// Creates a message from a `MESSAGE_SIZE` byte array -impl From<[u8; constants::MESSAGE_SIZE]> for Message { - fn from(buf: [u8; constants::MESSAGE_SIZE]) -> Message { - Message(buf) - } -} - /// An ECDSA error #[derive(Copy, PartialEq, Eq, Clone, Debug)] pub enum Error { @@ -990,17 +987,16 @@ mod tests { s.randomize(&mut thread_rng()); // Wild keys: 1, CURVE_ORDER - 1 - // Wild msgs: 0, 1, CURVE_ORDER - 1, CURVE_ORDER + // Wild msgs: 1, CURVE_ORDER - 1 let mut wild_keys = [[0; 32]; 2]; - let mut wild_msgs = [[0; 32]; 4]; + let mut wild_msgs = [[0; 32]; 2]; wild_keys[0][0] = 1; - wild_msgs[1][0] = 1; + wild_msgs[0][0] = 1; use constants; wild_keys[1][..].copy_from_slice(&constants::CURVE_ORDER[..]); wild_msgs[1][..].copy_from_slice(&constants::CURVE_ORDER[..]); - wild_msgs[2][..].copy_from_slice(&constants::CURVE_ORDER[..]); wild_keys[1][0] -= 1; wild_msgs[1][0] -= 1; @@ -1079,7 +1075,11 @@ mod tests { Err(InvalidMessage)); assert_eq!(Message::from_slice(&[0; constants::MESSAGE_SIZE + 1]), Err(InvalidMessage)); - assert!(Message::from_slice(&[0; constants::MESSAGE_SIZE]).is_ok()); + assert_eq!( + Message::from_slice(&[0; constants::MESSAGE_SIZE]), + Err(InvalidMessage) + ); + assert!(Message::from_slice(&[1; constants::MESSAGE_SIZE]).is_ok()); } #[test] From 1f4a4c11a3e41319d228adc1ed9fc187786f0892 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Wed, 15 Aug 2018 17:11:32 +0000 Subject: [PATCH 2/3] change add_*_assign and mul_*_assign to use a byteslice as a tweak, rather than a `SecretKey` This makes more conceptual sense and does not add any new error paths, since even valid `SecretKey`s were able to be invalid tweaks. --- src/key.rs | 72 +++++++++++++++++++++++++++++++++++++++--------------- src/lib.rs | 5 +++- 2 files changed, 56 insertions(+), 21 deletions(-) diff --git a/src/key.rs b/src/key.rs index c5d7b33..285f436 100644 --- a/src/key.rs +++ b/src/key.rs @@ -150,8 +150,16 @@ impl SecretKey { } #[inline] - /// Adds one secret key to another, modulo the curve order - pub fn add_assign(&mut self, other: &SecretKey) -> Result<(), Error> { + /// Adds one secret key to another, modulo the curve order. WIll + /// return an error if the resulting key would be invalid or if + /// the tweak was not a 32-byte length slice. + pub fn add_assign( + &mut self, + other: &[u8], + ) -> Result<(), Error> { + if other.len() != 32 { + return Err(Error::InvalidTweak); + } unsafe { if ffi::secp256k1_ec_privkey_tweak_add( ffi::secp256k1_context_no_precomp, @@ -159,7 +167,7 @@ impl SecretKey { other.as_ptr(), ) != 1 { - Err(InvalidSecretKey) + Err(Error::InvalidTweak) } else { Ok(()) } @@ -167,8 +175,16 @@ impl SecretKey { } #[inline] - /// Multiplies one secret key by another, modulo the curve order - pub fn mul_assign(&mut self, other: &SecretKey) -> Result<(), Error> { + /// Multiplies one secret key by another, modulo the curve order. Will + /// return an error if the resulting key would be invalid or if + /// the tweak was not a 32-byte length slice. + pub fn mul_assign( + &mut self, + other: &[u8], + ) -> Result<(), Error> { + if other.len() != 32 { + return Err(Error::InvalidTweak); + } unsafe { if ffi::secp256k1_ec_privkey_tweak_mul( ffi::secp256k1_context_no_precomp, @@ -176,7 +192,7 @@ impl SecretKey { other.as_ptr(), ) != 1 { - Err(InvalidSecretKey) + Err(Error::InvalidTweak) } else { Ok(()) } @@ -293,28 +309,44 @@ impl PublicKey { #[inline] /// Adds the pk corresponding to `other` to the pk `self` in place - pub fn add_exp_assign(&mut self, secp: &Secp256k1, other: &SecretKey) - -> Result<(), Error> { + /// Will return an error if the resulting key would be invalid or + /// if the tweak was not a 32-byte length slice. + pub fn add_exp_assign( + &mut self, + secp: &Secp256k1, + other: &[u8] + ) -> Result<(), Error> { + if other.len() != 32 { + return Err(Error::InvalidTweak); + } unsafe { if ffi::secp256k1_ec_pubkey_tweak_add(secp.ctx, &mut self.0 as *mut _, other.as_ptr()) == 1 { Ok(()) } else { - Err(InvalidSecretKey) + Err(Error::InvalidTweak) } } } #[inline] /// Muliplies the pk `self` in place by the scalar `other` - pub fn mul_assign(&mut self, secp: &Secp256k1, other: &SecretKey) - -> Result<(), Error> { + /// Will return an error if the resulting key would be invalid or + /// if the tweak was not a 32-byte length slice. + pub fn mul_assign( + &mut self, + secp: &Secp256k1, + other: &[u8], + ) -> Result<(), Error> { + if other.len() != 32 { + return Err(Error::InvalidTweak); + } unsafe { if ffi::secp256k1_ec_pubkey_tweak_mul(secp.ctx, &mut self.0 as *mut _, other.as_ptr()) == 1 { Ok(()) } else { - Err(InvalidSecretKey) + Err(Error::InvalidTweak) } } } @@ -592,13 +624,13 @@ mod test { let (mut sk2, mut pk2) = s.generate_keypair(&mut thread_rng()); assert_eq!(PublicKey::from_secret_key(&s, &sk1), pk1); - assert!(sk1.add_assign(&sk2).is_ok()); - assert!(pk1.add_exp_assign(&s, &sk2).is_ok()); + assert!(sk1.add_assign(&sk2[..]).is_ok()); + assert!(pk1.add_exp_assign(&s, &sk2[..]).is_ok()); assert_eq!(PublicKey::from_secret_key(&s, &sk1), pk1); assert_eq!(PublicKey::from_secret_key(&s, &sk2), pk2); - assert!(sk2.add_assign(&sk1).is_ok()); - assert!(pk2.add_exp_assign(&s, &sk1).is_ok()); + assert!(sk2.add_assign(&sk1[..]).is_ok()); + assert!(pk2.add_exp_assign(&s, &sk1[..]).is_ok()); assert_eq!(PublicKey::from_secret_key(&s, &sk2), pk2); } @@ -610,13 +642,13 @@ mod test { let (mut sk2, mut pk2) = s.generate_keypair(&mut thread_rng()); assert_eq!(PublicKey::from_secret_key(&s, &sk1), pk1); - assert!(sk1.mul_assign(&sk2).is_ok()); - assert!(pk1.mul_assign(&s, &sk2).is_ok()); + assert!(sk1.mul_assign(&sk2[..]).is_ok()); + assert!(pk1.mul_assign(&s, &sk2[..]).is_ok()); assert_eq!(PublicKey::from_secret_key(&s, &sk1), pk1); assert_eq!(PublicKey::from_secret_key(&s, &sk2), pk2); - assert!(sk2.mul_assign(&sk1).is_ok()); - assert!(pk2.mul_assign(&s, &sk1).is_ok()); + assert!(sk2.mul_assign(&sk1[..]).is_ok()); + assert!(pk2.mul_assign(&s, &sk1[..]).is_ok()); assert_eq!(PublicKey::from_secret_key(&s, &sk2), pk2); } diff --git a/src/lib.rs b/src/lib.rs index b75993f..422cb5b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -535,6 +535,8 @@ pub enum Error { InvalidSecretKey, /// Bad recovery id InvalidRecoveryId, + /// Invalid tweak for add_*_assign or mul_*_assign + InvalidTweak, } // Passthrough Debug to Display, since errors should be user-visible @@ -554,7 +556,8 @@ impl error::Error for Error { Error::InvalidPublicKey => "secp: malformed public key", Error::InvalidSignature => "secp: malformed signature", Error::InvalidSecretKey => "secp: malformed or out-of-range secret key", - Error::InvalidRecoveryId => "secp: bad recovery id" + Error::InvalidRecoveryId => "secp: bad recovery id", + Error::InvalidTweak => "secp: bad tweak", } } } From e5a02bd9a0493f2bf0582e2082b5f79d0c8b6f45 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Wed, 15 Aug 2018 17:33:51 +0000 Subject: [PATCH 3/3] add `ThirtyTwoByteHash` hash trait which can be implemented for easier conversion of things to `Message`s --- src/lib.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/lib.rs b/src/lib.rs index 422cb5b..a95c64c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -205,6 +205,14 @@ fn from_str(s: &str) -> Result { #[derive(Copy, Clone, PartialEq, Eq, Debug)] pub struct RecoverableSignature(ffi::RecoverableSignature); +/// Trait describing something that promises to be a 32-byte random number; in particular, +/// it has negligible probability of being zero or overflowing the group order. Such objects +/// may be converted to `Message`s without any error paths. +pub trait ThirtyTwoByteHash { + /// Converts the object into a 32-byte array + fn into_32(self) -> [u8; 32]; +} + impl RecoveryId { #[inline] /// Allows library users to create valid recovery IDs from i32. @@ -519,6 +527,13 @@ impl Message { } } +impl From for Message { + /// Converts a 32-byte hash directly to a message without error paths + fn from(t: T) -> Message { + Message(t.into_32()) + } +} + /// An ECDSA error #[derive(Copy, PartialEq, Eq, Clone, Debug)] pub enum Error {