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

Obfuscate shared secret when printing #396

Merged
merged 6 commits into from Feb 28, 2022
+83 −39
Merged

Obfuscate shared secret when printing #396

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6dca996
Mention bitcoin_hashes in obfuscated secret msg
tcharding Feb 8, 2022
91106f5
Remove magic number
tcharding Feb 8, 2022
4ded2c0
Use byte instead of i
tcharding Feb 9, 2022
5c7c76e
Rename serialize_secret -> secret_bytes
tcharding Feb 18, 2022
e4be664
Improve rustdocs for displaying secrets
tcharding Feb 18, 2022
cf6badf
Obfuscate SharedSecret when printing
tcharding Feb 18, 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
17 changes: 14 additions & 3 deletions src/ecdh.rs
View file
Open in desktop
Expand Up @@ -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.
///
Expand All @@ -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,
Expand All @@ -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 {
Expand Down
14 changes: 7 additions & 7 deletions src/key.rs
View file
Open in desktop
Expand Up @@ -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
}

Expand Down Expand Up @@ -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[..])
Expand Down Expand Up @@ -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()
}

Expand Down Expand Up @@ -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[..])
Expand Down
91 changes: 62 additions & 29 deletions src/secret.rs
View file
Open in desktop
Expand Up @@ -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 {
Expand All @@ -35,7 +36,7 @@ macro_rules! impl_display_secret {

hasher.write(DEBUG_HASH_TAG);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the test fails because DefaultHasher::new() above returns hasher seeded with a different random seed each time. Which also means this implementation is actually wrong if we want to allow comparing different secrets.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, thanks for debugging it for me. So shared_secret_point is non-trivial to use then. I'll put some more work into it tomorrow. Cheers.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On further thought, I don't get that, the same data hashed multiple times should give the same output irrespective of the hashing implementation. I haven't looked at the code yet, I was just pondering your message.

Copy link
Collaborator

@Kixunil Kixunil Feb 19, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: something doesn't seem right about this, DefaultHasher::new is guaranteed to return same hasher and the implementation uses fixed seed 0, 0.

Original message:

std hasher is one intended for HashMap and implements DoS protection by randomizing among other things. It produces same hash for same input for same instances of the hasher but since we call new on each debug we're randomizing it.

if we want to allow comparing different secrets

I meant humans comparing, not computers here. Only the Debug implementation is wrong.
Unless we want to implement our own hasher or store DefaultHasher in static variable I suggest to just remove the hash when compiled without bitcoin_hashes.

hasher.write(DEBUG_HASH_TAG);
hasher.write(&self.serialize_secret());
hasher.write(&self.secret_bytes());
let hash = hasher.finish();

f.debug_tuple(stringify!($thing))
Expand All @@ -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))
Expand All @@ -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>")
}
}
}
Expand All @@ -91,7 +92,7 @@ pub struct DisplaySecret {
impl fmt::Debug for DisplaySecret {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated but I think this should be called just Display - same as std::path::Display

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to leave this as is for now, can re-visit if you feel strongly.

#[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)
Expand All @@ -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(())
}
Expand All @@ -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() }
}
}

Expand All @@ -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() }
}
}