From bb888420fade98857b35904973dedb4b367cb7c1 Mon Sep 17 00:00:00 2001 From: Dr Maxim Orlovsky Date: Sat, 19 Jun 2021 11:44:31 +0200 Subject: [PATCH 1/9] Using dedicated safe_debug for secret key containing types --- src/key.rs | 11 +++++-- src/macros.rs | 74 +++++++++++++++++++++++++++++++++++++++++++++-- src/schnorrsig.rs | 3 +- 3 files changed, 83 insertions(+), 5 deletions(-) diff --git a/src/key.rs b/src/key.rs index 834137c8d..699e1f688 100644 --- a/src/key.rs +++ b/src/key.rs @@ -29,7 +29,7 @@ use ffi::{self, CPtr}; /// Secret 256-bit key used as `x` in an ECDSA signature pub struct SecretKey(pub(crate) [u8; constants::SECRET_KEY_SIZE]); impl_array_newtype!(SecretKey, u8, constants::SECRET_KEY_SIZE); -impl_pretty_debug!(SecretKey); +impl_safe_debug!(SecretKey); impl fmt::LowerHex for SecretKey { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { @@ -675,8 +675,15 @@ mod test { let s = Secp256k1::new(); let (sk, _) = s.generate_keypair(&mut DumbRng(0)); + #[cfg(all(feature = "bitcoin_hashes", not(fuzzing)))] assert_eq!(&format!("{:?}", sk), - "SecretKey(0100000000000000020000000000000003000000000000000400000000000000)"); + "SecretKey(#73e200e2...29d234a4)"); + #[cfg(all(not(feature = "bitcoin_hashes"), feature = "std"))] + assert_eq!(&format!("{:?}", sk), + "SecretKey(#a463cd1b7ffe86b0)"); + #[allow(deprecated)] { + assert_eq!(sk.format_secret_key(), + "0100000000000000020000000000000003000000000000000400000000000000"); }; } #[test] diff --git a/src/macros.rs b/src/macros.rs index bfd41b7bb..684bf102e 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -18,10 +18,80 @@ macro_rules! impl_pretty_debug { impl ::core::fmt::Debug for $thing { fn fmt(&self, f: &mut ::core::fmt::Formatter) -> ::core::fmt::Result { write!(f, "{}(", stringify!($thing))?; - for i in self[..].iter().cloned() { + for i in &self[..] { write!(f, "{:02x}", i)?; } - write!(f, ")") + f.write_str(")") + } + } + } +} + +macro_rules! impl_safe_debug { + ($thing:ident) => { + #[cfg(feature = "bitcoin_hashes")] + impl ::core::fmt::Debug for $thing { + fn fmt(&self, f: &mut ::core::fmt::Formatter) -> ::core::fmt::Result { + use ::bitcoin_hashes::{Hash, sha256}; + write!(f, "{}(#", stringify!($thing))?; + let hash = sha256::Hash::hash(&self.0[..]); + for i in &hash[..4] { + write!(f, "{:02x}", i)?; + } + f.write_str("...")?; + for i in &hash[28..] { + write!(f, "{:02x}", i)?; + } + f.write_str(")") + } + } + + #[cfg(all(not(feature = "bitcoin_hashes"), feature = "std"))] + impl ::core::fmt::Debug for $thing { + fn fmt(&self, f: &mut ::core::fmt::Formatter) -> ::core::fmt::Result { + use ::core::hash::Hasher; + let mut hasher = ::std::collections::hash_map::DefaultHasher::new(); + + hasher.write(&self.0[..]); + let hash = hasher.finish(); + + write!(f, "{}(#{:016x})", stringify!($thing), hash) + } + } + + impl $thing { + /// Formats the explicit byte value of the secret key kept inside the type as a + /// little-endian hexadecimal string using the provided formatter. + /// + /// This is the only method that outputs the actual secret key value, and, thus, + /// should be used with extreme precaution. + #[deprecated( + note = "Caution: you are explicitly outputting secret key value! This can be done + only in debug environment and that's why always considered as ``deprecated''" + )] + pub fn fmt_secret_key(&self, f: &mut ::core::fmt::Formatter) -> ::core::fmt::Result { + for i in &self.0[..] { + write!(f, "{:02x}", i)?; + } + Ok(()) + } + + /// Formats the explicit byte value of the secret key kept inside the type as a + /// little-endian hexadecimal string. + /// + /// This is the only method that outputs the actual secret key value, and, thus, + /// should be used with extreme precaution. + #[deprecated( + note = "Caution: you are explicitly outputting secret key value! This can be done + only in debug environment and that's why always considered as ``deprecated''" + )] + #[cfg(feature = "std")] + pub fn format_secret_key(&self) -> String { + let mut s = Vec::with_capacity(self.0.len() * 2); + for i in &self.0[..] { + s.push(format!("{:02x}", i)); + } + s.join("") } } } diff --git a/src/schnorrsig.rs b/src/schnorrsig.rs index d8c48a07f..667f2f723 100644 --- a/src/schnorrsig.rs +++ b/src/schnorrsig.rs @@ -76,8 +76,9 @@ impl str::FromStr for Signature { } /// Opaque data structure that holds a keypair consisting of a secret and a public key. -#[derive(Copy, Clone, PartialEq, Eq, Debug, PartialOrd, Ord, Hash)] +#[derive(Clone)] pub struct KeyPair(ffi::KeyPair); +impl_safe_debug!(KeyPair); /// A Schnorr public key, used for verification of Schnorr signatures #[derive(Copy, Clone, PartialEq, Eq, Debug, PartialOrd, Ord, Hash)] From 4c11c7f20daa0144e8e6254f28327680f52490f5 Mon Sep 17 00:00:00 2001 From: Dr Maxim Orlovsky Date: Sat, 19 Jun 2021 12:20:06 +0200 Subject: [PATCH 2/9] Safe array type macro impl for secret-key containing types --- secp256k1-sys/src/macros.rs | 81 ++++++++++++++++++++++--------------- src/key.rs | 39 +++++++++--------- src/lib.rs | 2 +- 3 files changed, 69 insertions(+), 53 deletions(-) diff --git a/secp256k1-sys/src/macros.rs b/secp256k1-sys/src/macros.rs index 5ca019840..6fdbff095 100644 --- a/secp256k1-sys/src/macros.rs +++ b/secp256k1-sys/src/macros.rs @@ -13,23 +13,23 @@ // If not, see . // -// This is a macro that routinely comes in handy +/// Implements array accessing methods for a type that must be considered safe #[macro_export] -macro_rules! impl_array_newtype { +macro_rules! impl_safe_array_newtype { ($thing:ident, $ty:ty, $len:expr) => { - impl Copy for $thing {} - impl $thing { #[inline] + #[allow(unused)] /// Converts the object to a raw pointer for FFI interfacing - pub fn as_ptr(&self) -> *const $ty { + pub(crate) fn as_ptr(&self) -> *const $ty { let &$thing(ref dat) = self; dat.as_ptr() } #[inline] + #[allow(unused)] /// Converts the object to a mutable raw pointer for FFI interfacing - pub fn as_mut_ptr(&mut self) -> *mut $ty { + pub(crate) fn as_mut_ptr(&mut self) -> *mut $ty { let &mut $thing(ref mut dat) = self; dat.as_mut_ptr() } @@ -43,6 +43,47 @@ macro_rules! impl_array_newtype { pub fn is_empty(&self) -> bool { false } } + impl $crate::CPtr for $thing { + type Target = $ty; + + fn as_c_ptr(&self) -> *const Self::Target { + if self.is_empty() { + ::core::ptr::null() + } else { + let &$thing(ref dat) = self; + dat.as_ptr() + } + } + + fn as_mut_c_ptr(&mut self) -> *mut Self::Target { + if self.is_empty() { + ::core::ptr::null::() as *mut _ + } else { + let &mut $thing(ref mut dat) = self; + dat.as_mut_ptr() + } + } + } + } +} + +/// Generates implementation of main array accessing methods for a newtype wrapping some inner array +/// type +#[macro_export] +macro_rules! impl_array_newtype { + ($thing:ident, $ty:ty, $len:expr) => { + impl_safe_array_newtype!($thing, $ty, $len); + + impl Copy for $thing {} + + impl Clone for $thing { + #[inline] + fn clone(&self) -> $thing { + let &$thing(ref dat) = self; + $thing(dat.clone()) + } + } + impl AsRef<[$ty; $len]> for $thing { #[inline] /// Gets a reference to the underlying array @@ -55,7 +96,7 @@ macro_rules! impl_array_newtype { impl PartialEq for $thing { #[inline] fn eq(&self, other: &$thing) -> bool { - &self[..] == &other[..] + &self.0[..] == &other[..] } } @@ -75,14 +116,6 @@ macro_rules! impl_array_newtype { } } - impl Clone for $thing { - #[inline] - fn clone(&self) -> $thing { - let &$thing(ref dat) = self; - $thing(dat.clone()) - } - } - impl ::core::ops::Index for $thing { type Output = $ty; @@ -132,24 +165,6 @@ macro_rules! impl_array_newtype { &dat[..] } } - impl $crate::CPtr for $thing { - type Target = $ty; - fn as_c_ptr(&self) -> *const Self::Target { - if self.is_empty() { - ::core::ptr::null() - } else { - self.as_ptr() - } - } - - fn as_mut_c_ptr(&mut self) -> *mut Self::Target { - if self.is_empty() { - ::core::ptr::null::() as *mut _ - } else { - self.as_mut_ptr() - } - } - } } } diff --git a/src/key.rs b/src/key.rs index 699e1f688..aeb70e3f5 100644 --- a/src/key.rs +++ b/src/key.rs @@ -27,8 +27,9 @@ use constants; use ffi::{self, CPtr}; /// Secret 256-bit key used as `x` in an ECDSA signature +#[derive(Clone, PartialEq, Eq, PartialOrd, Ord)] pub struct SecretKey(pub(crate) [u8; constants::SECRET_KEY_SIZE]); -impl_array_newtype!(SecretKey, u8, constants::SECRET_KEY_SIZE); +impl_safe_array_newtype!(SecretKey, u8, constants::SECRET_KEY_SIZE); impl_safe_debug!(SecretKey); impl fmt::LowerHex for SecretKey { @@ -220,7 +221,7 @@ impl ::serde::Serialize for SecretKey { if s.is_human_readable() { s.collect_str(self) } else { - s.serialize_bytes(&self[..]) + s.serialize_bytes(&self.0[..]) } } } @@ -529,7 +530,7 @@ mod test { let s = Secp256k1::new(); let (sk1, pk1) = s.generate_keypair(&mut thread_rng()); - assert_eq!(SecretKey::from_slice(&sk1[..]), Ok(sk1)); + assert_eq!(SecretKey::from_slice(&sk1.0[..]), Ok(sk1)); assert_eq!(PublicKey::from_slice(&pk1.serialize()[..]), Ok(pk1)); assert_eq!(PublicKey::from_slice(&pk1.serialize_uncompressed()[..]), Ok(pk1)); } @@ -784,13 +785,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.0[..]).is_ok()); + assert!(pk1.add_exp_assign(&s, &sk2.0[..]).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.0[..]).is_ok()); + assert!(pk2.add_exp_assign(&s, &sk1.0[..]).is_ok()); assert_eq!(PublicKey::from_secret_key(&s, &sk2), pk2); } @@ -802,13 +803,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.0[..]).is_ok()); + assert!(pk1.mul_assign(&s, &sk2.0[..]).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.0[..]).is_ok()); + assert!(pk2.mul_assign(&s, &sk1.0[..]).is_ok()); assert_eq!(PublicKey::from_secret_key(&s, &sk2), pk2); } @@ -818,7 +819,7 @@ mod test { let (mut sk, mut pk) = s.generate_keypair(&mut thread_rng()); - let original_sk = sk; + let original_sk = sk.clone(); let original_pk = pk; assert_eq!(PublicKey::from_secret_key(&s, &sk), pk); @@ -913,7 +914,7 @@ mod test { assert!(sum2.is_ok()); assert_eq!(sum1, sum2); - assert!(sk1.add_assign(&sk2.as_ref()[..]).is_ok()); + assert!(sk1.add_assign(&sk2.0[..]).is_ok()); let sksum = PublicKey::from_secret_key(&s, &sk1); assert_eq!(Ok(sksum), sum1); } @@ -974,13 +975,13 @@ mod test { #[cfg(fuzzing)] let pk = PublicKey::from_slice(&PK_BYTES).expect("pk"); - assert_tokens(&sk.compact(), &[Token::BorrowedBytes(&SK_BYTES[..])]); - assert_tokens(&sk.compact(), &[Token::Bytes(&SK_BYTES)]); - assert_tokens(&sk.compact(), &[Token::ByteBuf(&SK_BYTES)]); + assert_tokens(&sk.clone().compact(), &[Token::BorrowedBytes(&SK_BYTES[..])]); + assert_tokens(&sk.clone().compact(), &[Token::Bytes(&SK_BYTES)]); + assert_tokens(&sk.clone().compact(), &[Token::ByteBuf(&SK_BYTES)]); - assert_tokens(&sk.readable(), &[Token::BorrowedStr(SK_STR)]); - assert_tokens(&sk.readable(), &[Token::Str(SK_STR)]); - assert_tokens(&sk.readable(), &[Token::String(SK_STR)]); + assert_tokens(&sk.clone().readable(), &[Token::BorrowedStr(SK_STR)]); + assert_tokens(&sk.clone().readable(), &[Token::Str(SK_STR)]); + assert_tokens(&sk.clone().readable(), &[Token::String(SK_STR)]); assert_tokens(&pk.compact(), &[Token::BorrowedBytes(&PK_BYTES[..])]); assert_tokens(&pk.compact(), &[Token::Bytes(&PK_BYTES)]); diff --git a/src/lib.rs b/src/lib.rs index a32a2afe2..6905815ba 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -995,7 +995,7 @@ mod tests { assert!(full.verify(&msg, &sig, &pk).is_ok()); // Check that we can produce keys from slices with no precomputation - let (pk_slice, sk_slice) = (&pk.serialize(), &sk[..]); + let (pk_slice, sk_slice) = (&pk.serialize(), &sk.0[..]); let new_pk = PublicKey::from_slice(pk_slice).unwrap(); let new_sk = SecretKey::from_slice(sk_slice).unwrap(); assert_eq!(sk, new_sk); From da211a57e4d99a359ba2afa6d587aa3517bb288f Mon Sep 17 00:00:00 2001 From: Dr Maxim Orlovsky Date: Sat, 19 Jun 2021 12:23:29 +0200 Subject: [PATCH 3/9] Getting rid of SecretKey display methods --- src/key.rs | 23 +++++------------------ 1 file changed, 5 insertions(+), 18 deletions(-) diff --git a/src/key.rs b/src/key.rs index aeb70e3f5..1970f4aa9 100644 --- a/src/key.rs +++ b/src/key.rs @@ -32,21 +32,6 @@ pub struct SecretKey(pub(crate) [u8; constants::SECRET_KEY_SIZE]); impl_safe_array_newtype!(SecretKey, u8, constants::SECRET_KEY_SIZE); impl_safe_debug!(SecretKey); -impl fmt::LowerHex for SecretKey { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - for ch in &self.0[..] { - write!(f, "{:02x}", *ch)?; - } - Ok(()) - } -} - -impl fmt::Display for SecretKey { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - fmt::LowerHex::fmt(self, f) - } -} - impl str::FromStr for SecretKey { type Err = Error; fn from_str(s: &str) -> Result { @@ -219,7 +204,8 @@ impl SecretKey { impl ::serde::Serialize for SecretKey { fn serialize(&self, s: S) -> Result { if s.is_human_readable() { - s.collect_str(self) + #[allow(deprecated)] + s.serialize_str(&self.format_secret_key()) } else { s.serialize_bytes(&self.0[..]) } @@ -706,10 +692,11 @@ mod test { #[cfg(fuzzing)] let pk = PublicKey::from_slice(&[0x02, 0x18, 0x84, 0x57, 0x81, 0xf6, 0x31, 0xc4, 0x8f, 0x1c, 0x97, 0x09, 0xe2, 0x30, 0x92, 0x06, 0x7d, 0x06, 0x83, 0x7f, 0x30, 0xaa, 0x0c, 0xd0, 0x54, 0x4a, 0xc8, 0x87, 0xfe, 0x91, 0xdd, 0xd1, 0x66]).expect("pk"); + #[allow(deprecated)] { assert_eq!( - sk.to_string(), + sk.format_secret_key(), "01010101010101010001020304050607ffff0000ffff00006363636363636363" - ); + ) }; assert_eq!( SecretKey::from_str("01010101010101010001020304050607ffff0000ffff00006363636363636363").unwrap(), sk From 3cf586a46c8b941275ab3ab6ec2e6b304feada79 Mon Sep 17 00:00:00 2001 From: Dr Maxim Orlovsky Date: Sat, 19 Jun 2021 12:54:43 +0200 Subject: [PATCH 4/9] Fixing warning in context mod under feature-specific ompillation --- src/context.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/context.rs b/src/context.rs index ec8b193bc..59831d38a 100644 --- a/src/context.rs +++ b/src/context.rs @@ -1,5 +1,5 @@ use core::marker::PhantomData; -use core::mem::{self, ManuallyDrop}; +use core::mem::ManuallyDrop; use ffi::{self, CPtr, types::AlignedType}; use ffi::types::{c_uint, c_void}; use Error; @@ -105,7 +105,7 @@ mod alloc_only { impl private::Sealed for VerifyOnly {} use super::*; - const ALIGN_TO: usize = mem::align_of::(); + const ALIGN_TO: usize = ::core::mem::align_of::(); /// Represents the set of capabilities needed for signing. pub enum SignOnly {} From 49130d7e484b881bacc3618ab2fec153b6c8ef55 Mon Sep 17 00:00:00 2001 From: Dr Maxim Orlovsky Date: Sat, 19 Jun 2021 12:56:03 +0200 Subject: [PATCH 5/9] Introducing serde-secrets feature flag --- Cargo.toml | 1 + contrib/test.sh | 2 +- src/key.rs | 39 ++++++++++++++++++++++++++------------- src/lib.rs | 2 +- 4 files changed, 29 insertions(+), 15 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index e545b2ee9..93b80d3b9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -27,6 +27,7 @@ recovery = ["secp256k1-sys/recovery"] lowmemory = ["secp256k1-sys/lowmemory"] global-context = ["std", "rand-std", "global-context-less-secure"] global-context-less-secure = [] +serde-secrets = ["serde", "std"] [dependencies] secp256k1-sys = { version = "0.4.1", default-features = false, path = "./secp256k1-sys" } diff --git a/contrib/test.sh b/contrib/test.sh index dc936ab75..0ba977a06 100755 --- a/contrib/test.sh +++ b/contrib/test.sh @@ -1,6 +1,6 @@ #!/bin/sh -ex -FEATURES="bitcoin_hashes global-context lowmemory rand rand-std recovery serde" +FEATURES="bitcoin_hashes global-context lowmemory rand rand-std recovery serde serde-secrets" # Use toolchain if explicitly specified if [ -n "$TOOLCHAIN" ] diff --git a/src/key.rs b/src/key.rs index 1970f4aa9..f3c51a702 100644 --- a/src/key.rs +++ b/src/key.rs @@ -200,7 +200,7 @@ impl SecretKey { } } -#[cfg(feature = "serde")] +#[cfg(all(feature = "serde", feature = "serde-secrets"))] impl ::serde::Serialize for SecretKey { fn serialize(&self, s: S) -> Result { if s.is_human_readable() { @@ -930,7 +930,7 @@ mod test { #[cfg(feature = "serde")] #[test] - fn test_serde() { + fn test_serde_pk() { use serde_test::{Configure, Token, assert_tokens}; static SK_BYTES: [u8; 32] = [ 1, 1, 1, 1, 1, 1, 1, 1, @@ -938,9 +938,6 @@ mod test { 0xff, 0xff, 0, 0, 0xff, 0xff, 0, 0, 99, 99, 99, 99, 99, 99, 99, 99 ]; - static SK_STR: &'static str = "\ - 01010101010101010001020304050607ffff0000ffff00006363636363636363\ - "; static PK_BYTES: [u8; 33] = [ 0x02, 0x18, 0x84, 0x57, 0x81, 0xf6, 0x31, 0xc4, 0x8f, @@ -962,14 +959,6 @@ mod test { #[cfg(fuzzing)] let pk = PublicKey::from_slice(&PK_BYTES).expect("pk"); - assert_tokens(&sk.clone().compact(), &[Token::BorrowedBytes(&SK_BYTES[..])]); - assert_tokens(&sk.clone().compact(), &[Token::Bytes(&SK_BYTES)]); - assert_tokens(&sk.clone().compact(), &[Token::ByteBuf(&SK_BYTES)]); - - assert_tokens(&sk.clone().readable(), &[Token::BorrowedStr(SK_STR)]); - assert_tokens(&sk.clone().readable(), &[Token::Str(SK_STR)]); - assert_tokens(&sk.clone().readable(), &[Token::String(SK_STR)]); - assert_tokens(&pk.compact(), &[Token::BorrowedBytes(&PK_BYTES[..])]); assert_tokens(&pk.compact(), &[Token::Bytes(&PK_BYTES)]); assert_tokens(&pk.compact(), &[Token::ByteBuf(&PK_BYTES)]); @@ -977,6 +966,30 @@ mod test { assert_tokens(&pk.readable(), &[Token::BorrowedStr(PK_STR)]); assert_tokens(&pk.readable(), &[Token::Str(PK_STR)]); assert_tokens(&pk.readable(), &[Token::String(PK_STR)]); + } + + #[cfg(all(feature = "serde", feature = "serde-secrets"))] + #[test] + fn test_serde_sk() { + use serde_test::{Configure, Token, assert_tokens}; + static SK_BYTES: [u8; 32] = [ + 1, 1, 1, 1, 1, 1, 1, 1, + 0, 1, 2, 3, 4, 5, 6, 7, + 0xff, 0xff, 0, 0, 0xff, 0xff, 0, 0, + 99, 99, 99, 99, 99, 99, 99, 99 + ]; + static SK_STR: &'static str = "\ + 01010101010101010001020304050607ffff0000ffff00006363636363636363\ + "; + let sk = SecretKey::from_slice(&SK_BYTES).unwrap(); + + assert_tokens(&sk.clone().compact(), &[Token::BorrowedBytes(&SK_BYTES[..])]); + assert_tokens(&sk.clone().compact(), &[Token::Bytes(&SK_BYTES)]); + assert_tokens(&sk.clone().compact(), &[Token::ByteBuf(&SK_BYTES)]); + + assert_tokens(&sk.clone().readable(), &[Token::BorrowedStr(SK_STR)]); + assert_tokens(&sk.clone().readable(), &[Token::Str(SK_STR)]); + assert_tokens(&sk.clone().readable(), &[Token::String(SK_STR)]); } } diff --git a/src/lib.rs b/src/lib.rs index 6905815ba..686f9999f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -795,7 +795,7 @@ impl Secp256k1 { /// which OpenSSL would verify but not libsecp256k1, or vice-versa. Requires a /// verify-capable context. /// - /// ```rust + /// ``` /// # #[cfg(feature="rand")] { /// # use secp256k1::rand::rngs::OsRng; /// # use secp256k1::{Secp256k1, Message, Error}; From 2ff407e12a4c95fb5223744915e82ee2ac24561a Mon Sep 17 00:00:00 2001 From: Dr Maxim Orlovsky Date: Sat, 19 Jun 2021 13:23:02 +0200 Subject: [PATCH 6/9] Improving safe ptr type implementation --- secp256k1-sys/src/macros.rs | 42 ++++++++++++++++--------------------- src/key.rs | 2 +- 2 files changed, 19 insertions(+), 25 deletions(-) diff --git a/secp256k1-sys/src/macros.rs b/secp256k1-sys/src/macros.rs index 6fdbff095..08df21134 100644 --- a/secp256k1-sys/src/macros.rs +++ b/secp256k1-sys/src/macros.rs @@ -13,10 +13,10 @@ // If not, see . // -/// Implements array accessing methods for a type that must be considered safe +/// Implements newtype wrapping methods #[macro_export] -macro_rules! impl_safe_array_newtype { - ($thing:ident, $ty:ty, $len:expr) => { +macro_rules! impl_ptr_newtype { + ($thing:ident, $ty:ty) => { impl $thing { #[inline] #[allow(unused)] @@ -33,35 +33,19 @@ macro_rules! impl_safe_array_newtype { let &mut $thing(ref mut dat) = self; dat.as_mut_ptr() } - - #[inline] - /// Returns the length of the object as an array - pub fn len(&self) -> usize { $len } - - #[inline] - /// Returns whether the object as an array is empty - pub fn is_empty(&self) -> bool { false } } impl $crate::CPtr for $thing { type Target = $ty; fn as_c_ptr(&self) -> *const Self::Target { - if self.is_empty() { - ::core::ptr::null() - } else { - let &$thing(ref dat) = self; - dat.as_ptr() - } + let &$thing(ref dat) = self; + dat.as_ptr() } fn as_mut_c_ptr(&mut self) -> *mut Self::Target { - if self.is_empty() { - ::core::ptr::null::() as *mut _ - } else { - let &mut $thing(ref mut dat) = self; - dat.as_mut_ptr() - } + let &mut $thing(ref mut dat) = self; + dat.as_mut_ptr() } } } @@ -72,7 +56,17 @@ macro_rules! impl_safe_array_newtype { #[macro_export] macro_rules! impl_array_newtype { ($thing:ident, $ty:ty, $len:expr) => { - impl_safe_array_newtype!($thing, $ty, $len); + impl_ptr_newtype!($thing, $ty); + + impl $thing { + #[inline] + /// Returns the length of the object as an array + pub fn len(&self) -> usize { $len } + + #[inline] + /// Returns whether the object as an array is empty + pub fn is_empty(&self) -> bool { false } + } impl Copy for $thing {} diff --git a/src/key.rs b/src/key.rs index f3c51a702..bf21d2e66 100644 --- a/src/key.rs +++ b/src/key.rs @@ -29,7 +29,7 @@ use ffi::{self, CPtr}; /// Secret 256-bit key used as `x` in an ECDSA signature #[derive(Clone, PartialEq, Eq, PartialOrd, Ord)] pub struct SecretKey(pub(crate) [u8; constants::SECRET_KEY_SIZE]); -impl_safe_array_newtype!(SecretKey, u8, constants::SECRET_KEY_SIZE); +impl_ptr_newtype!(SecretKey, u8); impl_safe_debug!(SecretKey); impl str::FromStr for SecretKey { From 9379fd8e97c558e1581d3cede69e6deb0c232fc8 Mon Sep 17 00:00:00 2001 From: Dr Maxim Orlovsky Date: Sat, 19 Jun 2021 13:34:51 +0200 Subject: [PATCH 7/9] Requiring for array newtypes to manually derive `Copy` --- secp256k1-sys/src/lib.rs | 3 +++ secp256k1-sys/src/macros.rs | 4 +--- secp256k1-sys/src/recovery.rs | 1 + src/lib.rs | 1 + src/schnorrsig.rs | 1 + 5 files changed, 7 insertions(+), 3 deletions(-) diff --git a/secp256k1-sys/src/lib.rs b/secp256k1-sys/src/lib.rs index 7577eca5b..1ccbe1784 100644 --- a/secp256k1-sys/src/lib.rs +++ b/secp256k1-sys/src/lib.rs @@ -99,6 +99,7 @@ pub type SchnorrNonceFn = Option bool { false } } - impl Copy for $thing {} - impl Clone for $thing { #[inline] fn clone(&self) -> $thing { @@ -167,7 +165,7 @@ macro_rules! impl_raw_debug { ($thing:ident) => { impl ::core::fmt::Debug for $thing { fn fmt(&self, f: &mut ::core::fmt::Formatter) -> ::core::fmt::Result { - for i in self[..].iter().cloned() { + for i in &self[..] { write!(f, "{:02x}", i)?; } Ok(()) diff --git a/secp256k1-sys/src/recovery.rs b/secp256k1-sys/src/recovery.rs index db4f5b334..ba975975a 100644 --- a/secp256k1-sys/src/recovery.rs +++ b/secp256k1-sys/src/recovery.rs @@ -20,6 +20,7 @@ use {Context, Signature, NonceFn, PublicKey}; /// Library-internal representation of a Secp256k1 signature + recovery ID #[repr(C)] +#[derive(Copy)] pub struct RecoverableSignature([c_uchar; 65]); impl_array_newtype!(RecoverableSignature, c_uchar, 65); impl_raw_debug!(RecoverableSignature); diff --git a/src/lib.rs b/src/lib.rs index 686f9999f..4c6329dd4 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -458,6 +458,7 @@ impl<'de> ::serde::Deserialize<'de> for Signature { } /// A (hashed) message input to an ECDSA signature +#[derive(Copy)] pub struct Message([u8; constants::MESSAGE_SIZE]); impl_array_newtype!(Message, u8, constants::MESSAGE_SIZE); impl_pretty_debug!(Message); diff --git a/src/schnorrsig.rs b/src/schnorrsig.rs index 667f2f723..adb1a2ea1 100644 --- a/src/schnorrsig.rs +++ b/src/schnorrsig.rs @@ -16,6 +16,7 @@ use {Message, Signing, Verification}; use SecretKey; /// Represents a Schnorr signature. +#[derive(Copy)] pub struct Signature([u8; constants::SCHNORRSIG_SIGNATURE_SIZE]); impl_array_newtype!(Signature, u8, constants::SCHNORRSIG_SIGNATURE_SIZE); impl_pretty_debug!(Signature); From 9dab0ee826fb7d547dc0bf6770fad23598ad41ec Mon Sep 17 00:00:00 2001 From: Dr Maxim Orlovsky Date: Sat, 19 Jun 2021 13:37:13 +0200 Subject: [PATCH 8/9] Zeroing secret keys on drop operations --- secp256k1-sys/src/lib.rs | 6 ++++++ src/key.rs | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/secp256k1-sys/src/lib.rs b/secp256k1-sys/src/lib.rs index 1ccbe1784..2956acd35 100644 --- a/secp256k1-sys/src/lib.rs +++ b/secp256k1-sys/src/lib.rs @@ -224,6 +224,12 @@ pub struct KeyPair([c_uchar; 96]); impl_array_newtype!(KeyPair, c_uchar, 96); impl_raw_debug!(KeyPair); +impl Drop for KeyPair { + fn drop(&mut self) { + self.0.copy_from_slice(&[0u8; 96]); + } +} + impl KeyPair { /// Creates an "uninitialized" FFI keypair which is zeroed out /// diff --git a/src/key.rs b/src/key.rs index bf21d2e66..238a702f3 100644 --- a/src/key.rs +++ b/src/key.rs @@ -32,6 +32,12 @@ pub struct SecretKey(pub(crate) [u8; constants::SECRET_KEY_SIZE]); impl_ptr_newtype!(SecretKey, u8); impl_safe_debug!(SecretKey); +impl Drop for SecretKey { + fn drop(&mut self) { + self.0.copy_from_slice(&[0u8; constants::SECRET_KEY_SIZE]); + } +} + impl str::FromStr for SecretKey { type Err = Error; fn from_str(s: &str) -> Result { From fd7e5c74350d740b4ef6ab68a9d8dbcd938450a0 Mon Sep 17 00:00:00 2001 From: Dr Maxim Orlovsky Date: Sat, 19 Jun 2021 13:57:37 +0200 Subject: [PATCH 9/9] Serde impl for KeyPair --- src/schnorrsig.rs | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/src/schnorrsig.rs b/src/schnorrsig.rs index adb1a2ea1..9c160db48 100644 --- a/src/schnorrsig.rs +++ b/src/schnorrsig.rs @@ -81,6 +81,48 @@ impl str::FromStr for Signature { pub struct KeyPair(ffi::KeyPair); impl_safe_debug!(KeyPair); +impl ::core::str::FromStr for KeyPair { + type Err = Error; + + fn from_str(s: &str) -> Result { + let ctx = unsafe { + Secp256k1::from_raw_all(ffi::secp256k1_context_no_precomp as *mut ffi::Context) + }; + KeyPair::from_seckey_str(&ctx, s) + } +} + +#[cfg(all(feature = "serde", feature = "serde-secrets"))] +impl ::serde::Serialize for KeyPair { + fn serialize(&self, s: S) -> Result { + if s.is_human_readable() { + #[allow(deprecated)] + s.serialize_str(&self.format_secret_key()) + } else { + s.serialize_bytes(&self.0[..]) + } + } +} + +#[cfg(feature = "serde")] +impl<'de> ::serde::Deserialize<'de> for KeyPair { + fn deserialize>(d: D) -> Result { + if d.is_human_readable() { + d.deserialize_str(super::serde_util::FromStrVisitor::new( + "a hex string representing 32 byte KeyPair" + )) + } else { + d.deserialize_bytes(super::serde_util::BytesVisitor::new( + "raw 32 bytes KeyPair", + |data| unsafe { + let ctx = Secp256k1::from_raw_all(ffi::secp256k1_context_no_precomp as *mut ffi::Context); + KeyPair::from_seckey_slice(&ctx, data) + } + )) + } + } +} + /// A Schnorr public key, used for verification of Schnorr signatures #[derive(Copy, Clone, PartialEq, Eq, Debug, PartialOrd, Ord, Hash)] pub struct PublicKey(ffi::XOnlyPublicKey);