From 6dca99631f77f1ede61b09bb2f1ff82bbecc694c Mon Sep 17 00:00:00 2001 From: Tobin Harding Date: Tue, 8 Feb 2022 08:32:17 +0000 Subject: [PATCH 1/6] Mention bitcoin_hashes in obfuscated secret msg Hashing the debug output for secrets can be done with `bitcoin_hashes` not just `std`. Mention this in the obfuscated string output when neither are available. --- src/secret.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/secret.rs b/src/secret.rs index ba61ee6..4d179f2 100644 --- a/src/secret.rs +++ b/src/secret.rs @@ -67,7 +67,7 @@ macro_rules! impl_display_secret { #[cfg(all(not(feature = "std"), not(feature = "bitcoin_hashes")))] impl ::core::fmt::Debug for $thing { fn fmt(&self, f: &mut ::core::fmt::Formatter) -> ::core::fmt::Result { - write!(f, "") + write!(f, "") } } } From 91106f56852601aeb77664df331462cd6f6d71c9 Mon Sep 17 00:00:00 2001 From: Tobin Harding Date: Tue, 8 Feb 2022 08:41:43 +0000 Subject: [PATCH 2/6] Remove magic number In array initialisation we use magic number 64, this is the secret bytes length multiplied by 2. Please note; we still use the magic number 32, left as such because it is used in various ways and its not immediately clear that using a single const would be any more descriptive. Use `SECRET_KEY_SIZE * 2` instead of magic number 64. --- src/key.rs | 4 ++-- src/secret.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/key.rs b/src/key.rs index 1d66ab4..6ec8618 100644 --- a/src/key.rs +++ b/src/key.rs @@ -299,7 +299,7 @@ impl SecretKey { impl ::serde::Serialize for SecretKey { fn serialize(&self, s: S) -> Result { if s.is_human_readable() { - let mut buf = [0u8; 64]; + let mut buf = [0u8; constants::SECRET_KEY_SIZE * 2]; s.serialize_str(::to_hex(&self.0, &mut buf).expect("fixed-size hex serialization")) } else { s.serialize_bytes(&self[..]) @@ -925,7 +925,7 @@ impl str::FromStr for KeyPair { impl ::serde::Serialize for KeyPair { fn serialize(&self, s: S) -> Result { if s.is_human_readable() { - let mut buf = [0u8; 64]; + let mut buf = [0u8; constants::SECRET_KEY_SIZE * 2]; s.serialize_str(::to_hex(&self.serialize_secret(), &mut buf) .expect("fixed-size hex serialization")) } else { diff --git a/src/secret.rs b/src/secret.rs index 4d179f2..6c93ce9 100644 --- a/src/secret.rs +++ b/src/secret.rs @@ -91,7 +91,7 @@ pub struct DisplaySecret { impl fmt::Debug for DisplaySecret { #[inline] fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - let mut slice = [0u8; 64]; + let mut slice = [0u8; SECRET_KEY_SIZE * 2]; let hex = to_hex(&self.secret, &mut slice).expect("fixed-size hex serializer failed"); f.debug_tuple("DisplaySecret") .field(&hex) From 4ded2c0478cec3a14a10d955e43b5a45a2ec61ce Mon Sep 17 00:00:00 2001 From: Tobin Harding Date: Wed, 9 Feb 2022 06:56:42 +0000 Subject: [PATCH 3/6] Use byte instead of i The identifier `i` is predominantly used for indexing an array but we are using it as a place holder for the iterated value of an array that is then printed. The identifier `byte` is more descriptive. Done in preparation for adding similar code to the `ecdh` module. --- src/secret.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/secret.rs b/src/secret.rs index 6c93ce9..2ef766b 100644 --- a/src/secret.rs +++ b/src/secret.rs @@ -101,8 +101,8 @@ impl fmt::Debug for DisplaySecret { impl fmt::Display for DisplaySecret { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - for i in &self.secret { - write!(f, "{:02x}", i)?; + for byte in &self.secret { + write!(f, "{:02x}", byte)?; } Ok(()) } From 5c7c76eb7494d3a031301d40d1ccebd0f9b9d7ae Mon Sep 17 00:00:00 2001 From: Tobin Harding Date: Fri, 18 Feb 2022 11:48:56 +0000 Subject: [PATCH 4/6] Rename serialize_secret -> secret_bytes The `serialize_secret` method is a getter method, it does not do any serialisation. However we use the method on secret keys and key types so in order for the name to be uniform use the descriptive name `secret_bytes`. Rename `serialize_secret` to be `secret_bytes`. --- src/key.rs | 10 +++++----- src/secret.rs | 8 ++++---- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/key.rs b/src/key.rs index 6ec8618..80acaf6 100644 --- a/src/key.rs +++ b/src/key.rs @@ -212,9 +212,9 @@ impl SecretKey { SecretKey(sk) } - /// Serializes the secret key as byte value. + /// Returns the secret key as a byte value. #[inline] - pub fn serialize_secret(&self) -> [u8; constants::SECRET_KEY_SIZE] { + pub fn secret_bytes(&self) -> [u8; constants::SECRET_KEY_SIZE] { self.0 } @@ -809,9 +809,9 @@ impl KeyPair { KeyPair::new(SECP256K1, rng) } - /// Serializes the key pair as a secret key byte value. + /// Returns the secret bytes for this key pair. #[inline] - pub fn serialize_secret(&self) -> [u8; constants::SECRET_KEY_SIZE] { + pub fn secret_bytes(&self) -> [u8; constants::SECRET_KEY_SIZE] { *SecretKey::from_keypair(self).as_ref() } @@ -926,7 +926,7 @@ impl ::serde::Serialize for KeyPair { fn serialize(&self, s: S) -> Result { if s.is_human_readable() { let mut buf = [0u8; constants::SECRET_KEY_SIZE * 2]; - s.serialize_str(::to_hex(&self.serialize_secret(), &mut buf) + s.serialize_str(::to_hex(&self.secret_bytes(), &mut buf) .expect("fixed-size hex serialization")) } else { s.serialize_bytes(&self.0[..]) diff --git a/src/secret.rs b/src/secret.rs index 2ef766b..b9eaba0 100644 --- a/src/secret.rs +++ b/src/secret.rs @@ -35,7 +35,7 @@ macro_rules! impl_display_secret { hasher.write(DEBUG_HASH_TAG); hasher.write(DEBUG_HASH_TAG); - hasher.write(&self.serialize_secret()); + hasher.write(&self.secret_bytes()); let hash = hasher.finish(); f.debug_tuple(stringify!($thing)) @@ -55,7 +55,7 @@ macro_rules! impl_display_secret { let tag_hash = sha256::Hash::hash(tag.as_bytes()); engine.input(&tag_hash[..]); engine.input(&tag_hash[..]); - engine.input(&self.serialize_secret()); + engine.input(&self.secret_bytes()); let hash = sha256::Hash::from_engine(engine); f.debug_tuple(stringify!($thing)) @@ -139,7 +139,7 @@ impl SecretKey { /// ``` #[inline] pub fn display_secret(&self) -> DisplaySecret { - DisplaySecret { secret: self.serialize_secret() } + DisplaySecret { secret: self.secret_bytes() } } } @@ -180,6 +180,6 @@ impl KeyPair { /// ``` #[inline] pub fn display_secret(&self) -> DisplaySecret { - DisplaySecret { secret: self.serialize_secret() } + DisplaySecret { secret: self.secret_bytes() } } } From e4be664d97410fccc8072e3a6292cbaf9b046d15 Mon Sep 17 00:00:00 2001 From: Tobin Harding Date: Fri, 18 Feb 2022 12:09:42 +0000 Subject: [PATCH 5/6] Improve rustdocs for displaying secrets Improve rustdocs on `display_secret` by doing: - Minor improvements to the rustdocs to aid readability in the editor. - Do not guarantee (`assert_eq!`) debug output --- src/secret.rs | 36 +++++++++++++++--------------------- 1 file changed, 15 insertions(+), 21 deletions(-) diff --git a/src/secret.rs b/src/secret.rs index b9eaba0..1cfacab 100644 --- a/src/secret.rs +++ b/src/secret.rs @@ -113,27 +113,26 @@ impl SecretKey { /// 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. + /// should be used with extreme caution. /// - /// # Example + /// # Examples /// /// ``` - /// # #[cfg(all(feature = "std", not(feature = "bitcoin_hashes")))] { - /// use secp256k1::ONE_KEY; - /// let key = ONE_KEY; - /// // Normal display hides value - /// assert_eq!( - /// "SecretKey(#2518682f7819fb2d)", - /// format!("{:?}", key) - /// ); + /// # #[cfg(feature = "std")] { + /// let key = secp256k1::ONE_KEY; + /// + /// // Normal debug hides value (`Display` is not implemented for `SecretKey`). + /// // E.g., `format!("{:?}", key)` prints "SecretKey(#2518682f7819fb2d)". + /// /// // Here we explicitly display the secret value: /// assert_eq!( /// "0000000000000000000000000000000000000000000000000000000000000001", /// format!("{}", key.display_secret()) /// ); + /// // Also, we can explicitly display with `Debug`: /// assert_eq!( - /// "DisplaySecret(\"0000000000000000000000000000000000000000000000000000000000000001\")", - /// format!("{:?}", key.display_secret()) + /// format!("{:?}", key.display_secret()), + /// format!("DisplaySecret(\"{}\")", key.display_secret()) /// ); /// # } /// ``` @@ -153,7 +152,7 @@ impl KeyPair { /// # Example /// /// ``` - /// # #[cfg(all(feature = "std", not(feature = "bitcoin_hashes")))] { + /// # #[cfg(feature = "std")] { /// use secp256k1::ONE_KEY; /// use secp256k1::KeyPair; /// use secp256k1::Secp256k1; @@ -161,20 +160,15 @@ impl KeyPair { /// let secp = Secp256k1::new(); /// let key = ONE_KEY; /// let key = KeyPair::from_secret_key(&secp, key); - /// - /// // Normal display hides value - /// assert_eq!( - /// "KeyPair(#2518682f7819fb2d)", - /// format!("{:?}", key) - /// ); /// // Here we explicitly display the secret value: /// assert_eq!( /// "0000000000000000000000000000000000000000000000000000000000000001", /// format!("{}", key.display_secret()) /// ); + /// // Also, we can explicitly display with `Debug`: /// assert_eq!( - /// "DisplaySecret(\"0000000000000000000000000000000000000000000000000000000000000001\")", - /// format!("{:?}", key.display_secret()) + /// format!("{:?}", key.display_secret()), + /// format!("DisplaySecret(\"{}\")", key.display_secret()) /// ); /// # } /// ``` From cf6badf96a20dfbbfce30d655571837717afd8c7 Mon Sep 17 00:00:00 2001 From: Tobin Harding Date: Fri, 18 Feb 2022 12:10:02 +0000 Subject: [PATCH 6/6] Obfuscate SharedSecret when printing Currently printing the `SharedSecret` using `Display` or `Debug` prints the real secret, this is sub-optimal. We have a solution for other secrets in the project where printing is obfuscated and we provide a `display_secret` method for explicitly printing. Mirror the logic for other secrets and obfuscate the `SharedSecret` when printing. --- src/ecdh.rs | 17 ++++++++++++++--- src/secret.rs | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 3 deletions(-) diff --git a/src/ecdh.rs b/src/ecdh.rs index 9751982..e36553e 100644 --- a/src/ecdh.rs +++ b/src/ecdh.rs @@ -21,6 +21,10 @@ use core::borrow::Borrow; use key::{SecretKey, PublicKey}; use ffi::{self, CPtr}; use secp256k1_sys::types::{c_int, c_uchar, c_void}; +use constants; + +// The logic for displaying shared secrets relies on this (see `secret.rs`). +const SHARED_SECRET_SIZE: usize = constants::SECRET_KEY_SIZE; /// Enables two parties to create a shared secret without revealing their own secrets. /// @@ -39,14 +43,15 @@ use secp256k1_sys::types::{c_int, c_uchar, c_void}; /// assert_eq!(sec1, sec2); /// # } // ``` -#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] -pub struct SharedSecret([u8; 32]); +#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub struct SharedSecret([u8; SHARED_SECRET_SIZE]); +impl_display_secret!(SharedSecret); impl SharedSecret { /// Creates a new shared secret from a pubkey and secret key. #[inline] pub fn new(point: &PublicKey, scalar: &SecretKey) -> SharedSecret { - let mut buf = [0u8; 32]; + let mut buf = [0u8; SHARED_SECRET_SIZE]; let res = unsafe { ffi::secp256k1_ecdh( ffi::secp256k1_context_no_precomp, @@ -60,6 +65,12 @@ impl SharedSecret { debug_assert_eq!(res, 1); SharedSecret(buf) } + + /// Returns the shared secret as a byte value. + #[inline] + pub fn secret_bytes(&self) -> [u8; SHARED_SECRET_SIZE] { + self.0 + } } impl Borrow<[u8]> for SharedSecret { diff --git a/src/secret.rs b/src/secret.rs index 1cfacab..493070f 100644 --- a/src/secret.rs +++ b/src/secret.rs @@ -16,6 +16,7 @@ use ::core::fmt; use ::{SecretKey, KeyPair, to_hex}; +use ecdh::SharedSecret; use constants::SECRET_KEY_SIZE; macro_rules! impl_display_secret { @@ -177,3 +178,41 @@ impl KeyPair { DisplaySecret { secret: self.secret_bytes() } } } + +impl SharedSecret { + /// Formats the explicit byte value of the shared secret kept inside the type as a + /// little-endian hexadecimal string using the provided formatter. + /// + /// This is the only method that outputs the actual shared secret value, and, thus, + /// should be used with extreme caution. + /// + /// # Examples + /// + /// ``` + /// # #[cfg(not(fuzzing))] + /// # #[cfg(feature = "std")] { + /// # use std::str::FromStr; + /// # use secp256k1::{SecretKey, PublicKey}; + /// use secp256k1::ecdh::SharedSecret; + /// + /// # let pk = PublicKey::from_slice(&[3, 23, 183, 225, 206, 31, 159, 148, 195, 42, 67, 115, 146, 41, 248, 140, 11, 3, 51, 41, 111, 180, 110, 143, 114, 134, 88, 73, 198, 174, 52, 184, 78]).expect("hard coded slice should parse correctly"); + /// # let sk = SecretKey::from_str("57f0148f94d13095cfda539d0da0d1541304b678d8b36e243980aab4e1b7cead").unwrap(); + /// + /// let secret = SharedSecret::new(&pk, &sk); + /// // Here we explicitly display the secret value: + /// assert_eq!( + /// format!("{}", secret.display_secret()), + /// "cf05ae7da039ddce6d56dd57d3000c6dd91c6f1695eae47e05389f11e2467043" + /// ); + /// // Also, we can explicitly display with `Debug`: + /// assert_eq!( + /// format!("{:?}", secret.display_secret()), + /// format!("DisplaySecret(\"{}\")", secret.display_secret()) + /// ); + /// # } + /// ``` + #[inline] + pub fn display_secret(&self) -> DisplaySecret { + DisplaySecret { secret: self.secret_bytes() } + } +}