Merge rust-bitcoin#396: Obfuscate shared secret when printing

cf6badf Obfuscate SharedSecret when printing (Tobin Harding)
e4be664 Improve rustdocs for displaying secrets (Tobin Harding)
5c7c76e Rename serialize_secret -> secret_bytes (Tobin Harding)
4ded2c0 Use byte instead of i (Tobin Harding)
91106f5 Remove magic number (Tobin Harding)
6dca996 Mention bitcoin_hashes in obfuscated secret msg (Tobin Harding)

Pull request description:

  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.

  - Patches 1 - 5: Clean up.
  - Patch 6: The meat and potatoes.

  This is the final change needed to:
  Resolve: rust-bitcoin#226

ACKs for top commit:
  apoelstra:
    ACK cf6badf

Tree-SHA512: df14e8c5f5815bd76c585a1cd1db42fab6858004ca2cafa9a158b8b04a44c4a11b1260374a6ff82fee540ca955f262b28efae023012de5ac3832e4f5d1d1815e

Author: apoelstra

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 {

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
     }
 
@@ -299,7 +299,7 @@ impl SecretKey {
 impl ::serde::Serialize for SecretKey {
     fn serialize<S: ::serde::Serializer>(&self, s: S) -> Result<S::Ok, S::Error> {
         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[..])
@@ -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()
     }
 
@@ -925,8 +925,8 @@ impl str::FromStr for KeyPair {
 impl ::serde::Serialize for KeyPair {
     fn serialize<S: ::serde::Serializer>(&self, s: S) -> Result<S::Ok, S::Error> {
         if s.is_human_readable() {
-            let mut buf = [0u8; 64];
-            s.serialize_str(::to_hex(&self.serialize_secret(), &mut buf)
+            let mut buf = [0u8; constants::SECRET_KEY_SIZE * 2];
+            s.serialize_str(::to_hex(&self.secret_bytes(), &mut buf)
                 .expect("fixed-size hex serialization"))
         } else {
             s.serialize_bytes(&self.0[..])

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 {
@@ -35,7 +36,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 +56,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))
@@ -67,7 +68,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, "<secret requires std feature to display>")
+                write!(f, "<secret requires std or bitcoin_hashes feature to display>")
             }
         }
      }
@@ -91,7 +92,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)
@@ -101,8 +102,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(())
     }
@@ -113,33 +114,32 @@ 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())
     /// );
     /// # }
     /// ```
     #[inline]
     pub fn display_secret(&self) -> DisplaySecret {
-        DisplaySecret { secret: self.serialize_secret() }
+        DisplaySecret { secret: self.secret_bytes() }
     }
 }
 
@@ -153,33 +153,66 @@ impl KeyPair {
     /// # Example
     ///
     /// ```
-    /// # #[cfg(all(feature = "std", not(feature = "bitcoin_hashes")))] {
+    /// # #[cfg(feature = "std")] {
     /// use secp256k1::ONE_KEY;
     /// use secp256k1::KeyPair;
     /// use secp256k1::Secp256k1;
     ///
     /// 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!(
+    ///     format!("{:?}", key.display_secret()),
+    ///     format!("DisplaySecret(\"{}\")", key.display_secret())
+    /// );
+    /// # }
+    /// ```
+    #[inline]
+    pub fn display_secret(&self) -> DisplaySecret {
+        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!(
-    ///     "DisplaySecret(\"0000000000000000000000000000000000000000000000000000000000000001\")",
-    ///     format!("{:?}", key.display_secret())
+    ///     format!("{:?}", secret.display_secret()),
+    ///     format!("DisplaySecret(\"{}\")", secret.display_secret())
     /// );
     /// # }
     /// ```
     #[inline]
     pub fn display_secret(&self) -> DisplaySecret {
-        DisplaySecret { secret: self.serialize_secret() }
+        DisplaySecret { secret: self.secret_bytes() }
     }
 }