From c266e11198c8d910362106ec3b91a214d8908ba9 Mon Sep 17 00:00:00 2001
From: Tobin Harding <me@tobin.cc>
Date: Thu, 10 Feb 2022 10:35:15 +0000
Subject: [PATCH] Limit SharedSecret to 32 byte buffer

The `SharedSecret` uses sha256 to hash the secret, this implies the
secret is 32 bytes of data.

Currently we use a buffer of 256 bytes, this seems to be unneeded.

Change the implementation to use a 32 byte buffer. This simplifies the
API and implementation quite considerably.
---
 no_std_test/src/main.rs |  2 +-
 src/ecdh.rs             | 74 +++++++++--------------------------------
 src/macros.rs           | 17 ----------
 3 files changed, 16 insertions(+), 77 deletions(-)

diff --git a/no_std_test/src/main.rs b/no_std_test/src/main.rs
index d245ce9dd..5862e12cc 100644
--- a/no_std_test/src/main.rs
+++ b/no_std_test/src/main.rs
@@ -131,7 +131,7 @@ fn start(_argc: isize, _argv: *const *const u8) -> isize {
         y.into()
     });
     assert_ne!(x_arr, [0u8; 32]);
-    assert_ne!(&y_arr[..], &[0u8; 32][..]);
+    assert!(!y_arr.is_empty());
 
     #[cfg(feature = "alloc")]
     {
diff --git a/src/ecdh.rs b/src/ecdh.rs
index 30ad59ec8..629c8b603 100644
--- a/src/ecdh.rs
+++ b/src/ecdh.rs
@@ -17,7 +17,7 @@
 //!
 
 use core::ptr;
-use core::ops::{FnMut, Deref};
+use core::ops::FnMut;
 
 use key::{SecretKey, PublicKey};
 use ffi::{self, CPtr};
@@ -40,75 +40,26 @@ use secp256k1_sys::types::{c_int, c_uchar, c_void};
 /// assert_eq!(sec1, sec2);
 /// # }
 // ```
-#[derive(Copy, Clone)]
-pub struct SharedSecret {
-    data: [u8; 256],
-    len: usize,
-}
-impl_raw_debug!(SharedSecret);
-
-
-// This implementes `From<N>` for all `[u8; N]` arrays from 128bits(16 byte) to 2048bits allowing known hash lengths.
-// Lower than 128 bits isn't resistant to collisions any more.
-impl_from_array_len!(SharedSecret, 256, (16 20 28 32 48 64 96 128 256));
+#[derive(Copy, Clone, Debug, PartialEq)]
+pub struct SharedSecret([u8; 32]);
 
 impl SharedSecret {
-
     /// Create an empty SharedSecret
     pub(crate) fn empty() ->  SharedSecret {
-        SharedSecret {
-            data: [0u8; 256],
-            len: 0,
-        }
+        SharedSecret([0u8; 32])
     }
 
-    /// Get a pointer to the underlying data with the specified capacity.
+    /// Get a mutable pointer to the underlying data.
     pub(crate) fn get_data_mut_ptr(&mut self) -> *mut u8 {
-        self.data.as_mut_ptr()
-    }
-
-    /// Get the capacity of the underlying data buffer.
-    pub fn capacity(&self) -> usize {
-        self.data.len()
-    }
-
-    /// Get the len of the used data.
-    pub fn len(&self) -> usize {
-        self.len
+        self.0.as_mut_ptr()
     }
 
     /// True if the underlying data buffer is empty.
     pub fn is_empty(&self) -> bool {
-        self.data.is_empty()
-    }
-
-    /// Set the length of the object.
-    pub(crate) fn set_len(&mut self, len: usize) {
-        debug_assert!(len <= self.data.len());
-        self.len = len;
+        self.0.is_empty()
     }
 }
 
-impl PartialEq for SharedSecret {
-    fn eq(&self, other: &SharedSecret) -> bool {
-        self.as_ref() == other.as_ref()
-    }
-}
-
-impl AsRef<[u8]> for SharedSecret {
-    fn as_ref(&self) -> &[u8] {
-        &self.data[..self.len]
-    }
-}
-
-impl Deref for SharedSecret {
-    type Target = [u8];
-    fn deref(&self) -> &[u8] {
-        &self.data[..self.len]
-    }
-}
-
-
 unsafe extern "C" fn c_callback(output: *mut c_uchar, x: *const c_uchar, y: *const c_uchar, _data: *mut c_void) -> c_int {
     ptr::copy_nonoverlapping(x, output, 32);
     ptr::copy_nonoverlapping(y, output.offset(32), 32);
@@ -133,7 +84,6 @@ impl SharedSecret {
         // The default `secp256k1_ecdh_hash_function_default` should always return 1.
         // and the scalar was verified to be valid(0 > scalar > group_order) via the type system
         debug_assert_eq!(res, 1);
-        ss.set_len(32); // The default hash function is SHA256, which is 32 bytes long.
         ss
     }
 
@@ -184,6 +134,12 @@ impl SharedSecret {
     }
 }
 
+impl From<[u8; 32]> for SharedSecret {
+    fn from(inner: [u8; 32]) -> SharedSecret {
+        SharedSecret(inner)
+    }
+}
+
 #[cfg(test)]
 #[allow(unused_imports)]
 mod tests {
@@ -227,7 +183,7 @@ mod tests {
     fn ecdh_with_hash_callback() {
         let s = Secp256k1::signing_only();
         let (sk1, pk1) = s.generate_keypair(&mut thread_rng());
-        let expect_result: [u8; 64] = [123; 64];
+        let expect_result: [u8; 32] = [123; 32];
         let mut x_out = [0u8; 32];
         let mut y_out = [0u8; 32];
         let result = SharedSecret::new_with_hash(&pk1, &sk1, |x, y| {
@@ -235,7 +191,7 @@ mod tests {
             y_out = y;
             expect_result.into()
         });
-        assert_eq!(&expect_result[..], &result[..]);
+        assert_eq!(&expect_result[..], &result.0[..]);
         assert_ne!(x_out, [0u8; 32]);
         assert_ne!(y_out, [0u8; 32]);
     }
diff --git a/src/macros.rs b/src/macros.rs
index f8d2b5e1e..9dad59da7 100644
--- a/src/macros.rs
+++ b/src/macros.rs
@@ -26,20 +26,3 @@ macro_rules! impl_pretty_debug {
         }
      }
 }
-
-macro_rules! impl_from_array_len {
-    ($thing:ident, $capacity:tt, ($($N:tt)+)) => {
-        $(
-            impl From<[u8; $N]> for $thing {
-                fn from(arr: [u8; $N]) -> Self {
-                    let mut data = [0u8; $capacity];
-                    data[..$N].copy_from_slice(&arr);
-                    $thing {
-                        data,
-                        len: $N,
-                    }
-                }
-            }
-        )+
-    }
-}