From 5603d71ad3057ab2572c508b057b7e10f9ae0595 Mon Sep 17 00:00:00 2001 From: Tobin Harding 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 is unnecessary. Change the implementation of `SharedSecret` to use a 32 byte buffer. --- src/ecdh.rs | 132 +++++++++++++++++++------------------------------- src/macros.rs | 17 ------- 2 files changed, 51 insertions(+), 98 deletions(-) diff --git a/src/ecdh.rs b/src/ecdh.rs index 6540e93..9751982 100644 --- a/src/ecdh.rs +++ b/src/ecdh.rs @@ -16,7 +16,7 @@ //! use core::ptr; -use core::ops::Deref; +use core::borrow::Borrow; use key::{SecretKey, PublicKey}; use ffi::{self, CPtr}; @@ -39,106 +39,46 @@ 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` 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)); - -impl SharedSecret { - - /// Creates an empty `SharedSecret`. - pub(crate) fn empty() -> SharedSecret { - SharedSecret { - data: [0u8; 256], - len: 0, - } - } - - /// Gets a pointer to the underlying data with the specified capacity. - pub(crate) fn get_data_mut_ptr(&mut self) -> *mut u8 { - self.data.as_mut_ptr() - } - - /// Gets the capacity of the underlying data buffer. - pub fn capacity(&self) -> usize { - self.data.len() - } - - /// Gets the len of the used data. - pub fn len(&self) -> usize { - self.len - } - - /// Returns true if the underlying data buffer is empty. - pub fn is_empty(&self) -> bool { - self.data.is_empty() - } - - /// Sets the length of the object. - pub(crate) fn set_len(&mut self, len: usize) { - debug_assert!(len <= self.data.len()); - self.len = len; - } -} - -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); - 1 -} +#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub struct SharedSecret([u8; 32]); impl SharedSecret { /// Creates a new shared secret from a pubkey and secret key. #[inline] pub fn new(point: &PublicKey, scalar: &SecretKey) -> SharedSecret { - let mut ss = SharedSecret::empty(); + let mut buf = [0u8; 32]; let res = unsafe { ffi::secp256k1_ecdh( ffi::secp256k1_context_no_precomp, - ss.get_data_mut_ptr(), + buf.as_mut_ptr(), point.as_c_ptr(), scalar.as_c_ptr(), ffi::secp256k1_ecdh_hash_function_default, ptr::null_mut(), ) }; - // 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 + SharedSecret(buf) + } +} + +impl Borrow<[u8]> for SharedSecret { + fn borrow(&self) -> &[u8] { + &self.0 + } +} + +impl AsRef<[u8]> for SharedSecret { + fn as_ref(&self) -> &[u8] { + &self.0 } } /// Creates a shared point from public key and secret key. /// +/// **Important: use of a strong cryptographic hash function may be critical to security! Do NOT use +/// unless you understand cryptographical implications.** If not, use SharedSecret instead. +/// /// Can be used like `SharedSecret` but caller is responsible for then hashing the returned buffer. /// This allows for the use of a custom hash function since `SharedSecret` uses SHA256. /// @@ -183,6 +123,12 @@ pub fn shared_secret_point(point: &PublicKey, scalar: &SecretKey) -> [u8; 64] { xy } +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); + 1 +} + #[cfg(test)] #[allow(unused_imports)] mod tests { @@ -221,6 +167,30 @@ mod tests { assert_eq!(x, new_x); assert_eq!(y, new_y); } + + #[test] + #[cfg(not(fuzzing))] + #[cfg(all(feature="rand-std", feature = "std", feature = "bitcoin_hashes"))] + fn bitcoin_hashes_and_sys_generate_same_secret() { + use hashes::{sha256, Hash, HashEngine}; + + let s = Secp256k1::signing_only(); + let (sk1, _) = s.generate_keypair(&mut thread_rng()); + let (_, pk2) = s.generate_keypair(&mut thread_rng()); + + let secret_sys = SharedSecret::new(&pk2, &sk1); + + let xy = shared_secret_point(&pk2, &sk1); + + // Mimics logic in `bitcoin-core/secp256k1/src/module/main_impl.h` + let version = (xy[63] & 0x01) | 0x02; + let mut engine = sha256::HashEngine::default(); + engine.input(&[version]); + engine.input(&xy.as_ref()[..32]); + let secret_bh = sha256::Hash::from_engine(engine); + + assert_eq!(secret_bh.as_inner(), secret_sys.as_ref()); + } } #[cfg(all(test, feature = "unstable"))] diff --git a/src/macros.rs b/src/macros.rs index f8d2b5e..9dad59d 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, - } - } - } - )+ - } -}