Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Limit SharedSecret to 32 byte buffer #402

Merged
merged 3 commits into from Feb 24, 2022
Merged

Limit SharedSecret to 32 byte buffer #402

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
834f63c
Separate new_with_hash into public function
tcharding Feb 18, 2022
d5eeb09
Use more intuitive local var numbering
tcharding Feb 21, 2022
5603d71
Limit SharedSecret to 32 byte buffer
tcharding Feb 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 2 additions & 8 deletions no_std_test/src/main.rs
View file
Open in desktop
Expand Up @@ -65,7 +65,7 @@ use core::fmt::{self, write, Write};
use core::intrinsics;
use core::panic::PanicInfo;

use secp256k1::ecdh::SharedSecret;
use secp256k1::ecdh::{self, SharedSecret};
use secp256k1::ffi::types::AlignedType;
use secp256k1::rand::{self, RngCore};
use secp256k1::serde::Serialize;
Expand Down Expand Up @@ -125,13 +125,7 @@ fn start(_argc: isize, _argv: *const *const u8) -> isize {
assert_eq!(sig, new_sig);

let _ = SharedSecret::new(&public_key, &secret_key);
let mut x_arr = [0u8; 32];
let y_arr = SharedSecret::new_with_hash(&public_key, &secret_key, |x,y| {
x_arr = x;
y.into()
});
assert_ne!(x_arr, [0u8; 32]);
assert_ne!(&y_arr[..], &[0u8; 32][..]);
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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).

let _ = ecdh::shared_secret_point(&public_key, &secret_key);

#[cfg(feature = "alloc")]
{
Expand Down
124 changes: 46 additions & 78 deletions src/ecdh.rs
View file
Open in desktop
Expand Up @@ -16,7 +16,7 @@
//!

use core::ptr;
use core::ops::{FnMut, Deref};
use core::ops::Deref;

use key::{SecretKey, PublicKey};
use ffi::{self, CPtr};
Expand Down Expand Up @@ -135,52 +135,52 @@ impl SharedSecret {
ss.set_len(32); // The default hash function is SHA256, which is 32 bytes long.
ss
}
}


/// 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];

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);

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)
}
/// Creates a shared point from public key and secret key.
///
/// 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
}

#[cfg(test)]
Expand All @@ -207,38 +207,6 @@ mod tests {
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];
Expand Down