-
Notifications
You must be signed in to change notification settings - Fork 262
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Limit SharedSecret to 32 byte buffer #402
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,7 @@ | |
//! | ||
|
||
use core::ptr; | ||
use core::ops::{FnMut, Deref}; | ||
use core::borrow::Borrow; | ||
|
||
use key::{SecretKey, PublicKey}; | ||
use ffi::{self, CPtr}; | ||
|
@@ -34,153 +34,99 @@ use secp256k1_sys::types::{c_int, c_uchar, c_void}; | |
/// let s = Secp256k1::new(); | ||
/// let (sk1, pk1) = s.generate_keypair(&mut thread_rng()); | ||
/// let (sk2, pk2) = s.generate_keypair(&mut thread_rng()); | ||
/// let sec1 = SharedSecret::new(&pk1, &sk2); | ||
/// let sec2 = SharedSecret::new(&pk2, &sk1); | ||
/// let sec1 = SharedSecret::new(&pk2, &sk1); | ||
/// let sec2 = SharedSecret::new(&pk1, &sk2); | ||
/// 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)); | ||
|
||
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] | ||
} | ||
} | ||
tcharding marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
|
||
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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure this is better than calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good reason, I believe. OTOH it annoys me that there are two SHA256 implementations in the code if you enable There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean, the implementation inside libsecp as well as the implementation in bitcoin_hashes? That is a problem for all users of libsecp, even upstream, and has been the subject of much discussion e.g. bitcoin-core/secp256k1#702 For now there's nothing we can do about it from the Rust bindings. (Well, ok, we could actually expose the hash functionality ourselves by patching the vendored library, but I think the solution we'd rather have is to somehow use the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I meant it. Would be a nice change later if/when it's available. |
||
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 | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh yes, this was discussed elsewhere already recently, my bad. Just to clarify, we should use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, we should implement both There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Cool, will add.
I read that in the docs but I do not understand what it implies. I'll have to take direction from others on this until I do get it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Your code is correct. It would've been incorrect if |
||
|
||
/// Creates a new shared secret from a pubkey and secret key with applied custom hash function. | ||
/// The custom hash function must be in the form of `fn(x: [u8;32], y: [u8;32]) -> SharedSecret` | ||
/// `SharedSecret` can be easily created via the `From` impl from arrays. | ||
/// # Examples | ||
/// ``` | ||
/// # #[cfg(any(feature = "alloc", features = "std"))] { | ||
/// # use secp256k1::ecdh::SharedSecret; | ||
/// # use secp256k1::{Secp256k1, PublicKey, SecretKey}; | ||
/// # fn sha2(_a: &[u8], _b: &[u8]) -> [u8; 32] {[0u8; 32]} | ||
/// # let secp = Secp256k1::signing_only(); | ||
/// # let secret_key = SecretKey::from_slice(&[3u8; 32]).unwrap(); | ||
/// # let secret_key2 = SecretKey::from_slice(&[7u8; 32]).unwrap(); | ||
/// # let public_key = PublicKey::from_secret_key(&secp, &secret_key2); | ||
/// | ||
/// let secret = SharedSecret::new_with_hash(&public_key, &secret_key, |x,y| { | ||
/// let hash: [u8; 32] = sha2(&x,&y); | ||
/// hash.into() | ||
/// }); | ||
/// # } | ||
/// ``` | ||
pub fn new_with_hash<F>(point: &PublicKey, scalar: &SecretKey, mut hash_function: F) -> SharedSecret | ||
where F: FnMut([u8; 32], [u8; 32]) -> SharedSecret { | ||
let mut xy = [0u8; 64]; | ||
impl AsRef<[u8]> for SharedSecret { | ||
fn as_ref(&self) -> &[u8] { | ||
&self.0 | ||
} | ||
} | ||
|
||
let res = unsafe { | ||
ffi::secp256k1_ecdh( | ||
ffi::secp256k1_context_no_precomp, | ||
xy.as_mut_ptr(), | ||
point.as_ptr(), | ||
scalar.as_ptr(), | ||
Some(c_callback), | ||
ptr::null_mut(), | ||
) | ||
}; | ||
// Our callback *always* returns 1. | ||
// and the scalar was verified to be valid(0 > scalar > group_order) via the type system | ||
debug_assert_eq!(res, 1); | ||
/// 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. | ||
tcharding marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// | ||
/// # Returns | ||
/// | ||
/// 64 bytes representing the (x,y) co-ordinates of a point on the curve (32 bytes each). | ||
/// | ||
/// # Examples | ||
/// ``` | ||
/// # #[cfg(all(feature = "bitcoin_hashes", feature = "rand-std", feature = "std"))] { | ||
/// # use secp256k1::{ecdh, Secp256k1, PublicKey, SecretKey}; | ||
/// # use secp256k1::hashes::{Hash, sha512}; | ||
/// # use secp256k1::rand::thread_rng; | ||
/// | ||
/// let s = Secp256k1::new(); | ||
/// let (sk1, pk1) = s.generate_keypair(&mut thread_rng()); | ||
/// let (sk2, pk2) = s.generate_keypair(&mut thread_rng()); | ||
/// | ||
/// let point1 = ecdh::shared_secret_point(&pk2, &sk1); | ||
/// let secret1 = sha512::Hash::hash(&point1); | ||
/// let point2 = ecdh::shared_secret_point(&pk1, &sk2); | ||
/// let secret2 = sha512::Hash::hash(&point2); | ||
/// assert_eq!(secret1, secret2) | ||
tcharding marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// # } | ||
/// ``` | ||
pub fn shared_secret_point(point: &PublicKey, scalar: &SecretKey) -> [u8; 64] { | ||
let mut xy = [0u8; 64]; | ||
|
||
let res = unsafe { | ||
ffi::secp256k1_ecdh( | ||
ffi::secp256k1_context_no_precomp, | ||
xy.as_mut_ptr(), | ||
point.as_ptr(), | ||
scalar.as_ptr(), | ||
Some(c_callback), | ||
ptr::null_mut(), | ||
) | ||
}; | ||
// Our callback *always* returns 1. | ||
// The scalar was verified to be valid (0 > scalar > group_order) via the type system. | ||
tcharding marked this conversation as resolved.
Show resolved
Hide resolved
|
||
debug_assert_eq!(res, 1); | ||
xy | ||
} | ||
|
||
let mut x = [0u8; 32]; | ||
let mut y = [0u8; 32]; | ||
x.copy_from_slice(&xy[..32]); | ||
y.copy_from_slice(&xy[32..]); | ||
hash_function(x, y) | ||
} | ||
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 | ||
} | ||
tcharding marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
#[cfg(test)] | ||
|
@@ -200,45 +146,13 @@ mod tests { | |
let (sk1, pk1) = s.generate_keypair(&mut thread_rng()); | ||
let (sk2, pk2) = s.generate_keypair(&mut thread_rng()); | ||
|
||
let sec1 = SharedSecret::new(&pk1, &sk2); | ||
let sec2 = SharedSecret::new(&pk2, &sk1); | ||
let sec1 = SharedSecret::new(&pk2, &sk1); | ||
let sec2 = SharedSecret::new(&pk1, &sk2); | ||
let sec_odd = SharedSecret::new(&pk1, &sk1); | ||
assert_eq!(sec1, sec2); | ||
assert!(sec_odd != sec2); | ||
} | ||
|
||
#[test] | ||
#[cfg(all(feature="std", feature = "rand-std"))] | ||
fn ecdh_with_hash() { | ||
let s = Secp256k1::signing_only(); | ||
let (sk1, pk1) = s.generate_keypair(&mut thread_rng()); | ||
let (sk2, pk2) = s.generate_keypair(&mut thread_rng()); | ||
|
||
let sec1 = SharedSecret::new_with_hash(&pk1, &sk2, |x,_| x.into()); | ||
let sec2 = SharedSecret::new_with_hash(&pk2, &sk1, |x,_| x.into()); | ||
let sec_odd = SharedSecret::new_with_hash(&pk1, &sk1, |x,_| x.into()); | ||
assert_eq!(sec1, sec2); | ||
assert_ne!(sec_odd, sec2); | ||
} | ||
|
||
#[test] | ||
#[cfg(all(feature="std", feature = "rand-std"))] | ||
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 mut x_out = [0u8; 32]; | ||
let mut y_out = [0u8; 32]; | ||
let result = SharedSecret::new_with_hash(&pk1, &sk1, |x, y| { | ||
x_out = x; | ||
y_out = y; | ||
expect_result.into() | ||
}); | ||
assert_eq!(&expect_result[..], &result[..]); | ||
assert_ne!(x_out, [0u8; 32]); | ||
assert_ne!(y_out, [0u8; 32]); | ||
} | ||
|
||
#[test] | ||
tcharding marked this conversation as resolved.
Show resolved
Hide resolved
|
||
fn test_c_callback() { | ||
let x = [5u8; 32]; | ||
|
@@ -253,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"))] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In 834f63c:
I think you coulrd've kept these asserts in place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When removing these I could not see what functionality they were testing that was specific to
no-std
, so I removed them. Or am I missing something? If a test that checks the point returned is non-zero is useful I can add it as a unit test, but again, I don't understand the value of it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think they were just there as a sanity check. I agree they don't seem to have a very clear value (and also that they should be a unit test, if they exist at all).