diff --git a/README.md b/README.md index 86d0f80..6b854bb 100644 --- a/README.md +++ b/README.md @@ -38,6 +38,16 @@ Alternatively add symlinks in your `.git/hooks` directory to any of the githooks We use a custom Rust compiler configuration conditional to guard the bench mark code. To run the bench marks use: `RUSTFLAGS='--cfg=bench' cargo +nightly bench --features=recovery`. +### A note on `non_secure_erase` + +This crate's secret types (`SecretKey`, `KeyPair`, `SharedSecret`, `Scalar`, and `DisplaySecret`) +have a method called `non_secure_erase` that *attempts* to overwrite the contained secret. This +method is provided to assist other libraries in building secure secret erasure. However, this +library makes no guarantees about the security of using `non_secure_erase`. In particular, +the compiler doesn't have any concept of secrets and in most cases can arbitrarily move or copy +values anywhere it pleases. For more information, consult the [`zeroize`](https://docs.rs/zeroize) +documentation. + ## Fuzzing If you want to fuzz this library, or any library which depends on it, you will diff --git a/secp256k1-sys/Cargo.toml b/secp256k1-sys/Cargo.toml index 1e67dc3..7a74b82 100644 --- a/secp256k1-sys/Cargo.toml +++ b/secp256k1-sys/Cargo.toml @@ -32,4 +32,3 @@ recovery = [] lowmemory = [] std = ["alloc"] alloc = [] - diff --git a/secp256k1-sys/src/lib.rs b/secp256k1-sys/src/lib.rs index 6978e99..2750cb0 100644 --- a/secp256k1-sys/src/lib.rs +++ b/secp256k1-sys/src/lib.rs @@ -454,6 +454,38 @@ impl KeyPair { pk } } + + /// Attempts to erase the contents of the underlying array. + /// + /// Note, however, that the compiler is allowed to freely copy or move the + /// contents of this array to other places in memory. Preventing this behavior + /// is very subtle. For more discussion on this, please see the documentation + /// of the [`zeroize`](https://docs.rs/zeroize) crate. + #[inline] + pub fn non_secure_erase(&mut self) { + non_secure_erase_impl(&mut self.0, DUMMY_KEYPAIR); + } +} + +// DUMMY_KEYPAIR is the internal repr of a valid key pair with secret key `[1u8; 32]` +#[cfg(target_endian = "little")] +const DUMMY_KEYPAIR: [c_uchar; 96] = [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 143, 7, 221, 213, 233, 245, 23, 156, 255, 25, 72, 96, 52, 24, 30, 215, 101, 5, 186, 170, 213, 62, 93, 153, 64, 100, 18, 123, 86, 197, 132, 27, 209, 232, 168, 105, 122, 212, 34, 81, 222, 57, 246, 167, 32, 129, 223, 223, 66, 171, 197, 66, 166, 214, 254, 7, 21, 84, 139, 88, 143, 175, 190, 112]; +#[cfg(all(target_endian = "big", target_pointer_width = "32"))] +const DUMMY_KEYPAIR: [c_uchar; 96] = [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 213, 221, 7, 143, 156, 23, 245, 233, 96, 72, 25, 255, 215, 30, 24, 52, 170, 186, 5, 101, 153, 93, 62, 213, 123, 18, 100, 64, 27, 132, 197, 86, 105, 168, 232, 209, 81, 34, 212, 122, 167, 246, 57, 222, 223, 223, 129, 32, 66, 197, 171, 66, 7, 254, 214, 166, 88, 139, 84, 21, 112, 190, 175, 143]; +#[cfg(all(target_endian = "big", target_pointer_width = "64"))] +const DUMMY_KEYPAIR: [c_uchar; 96] = [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 156, 23, 245, 233, 213, 221, 7, 143, 215, 30, 24, 52, 96, 72, 25, 255, 153, 93, 62, 213, 170, 186, 5, 101, 27, 132, 197, 86, 123, 18, 100, 64, 81, 34, 212, 122, 105, 168, 232, 209, 223, 223, 129, 32, 167, 246, 57, 222, 7, 254, 214, 166, 66, 197, 171, 66, 112, 190, 175, 143, 88, 139, 84, 21]; + +/// Does a best attempt at secure erasure using Rust intrinsics. +/// +/// The implementation is based on the approach used by the [`zeroize`](https://docs.rs/zeroize) crate. +#[inline(always)] +pub fn non_secure_erase_impl(dst: &mut T, src: T) { + use core::sync::atomic; + // overwrite using volatile value + unsafe { ptr::write_volatile(dst, src); } + + // prevent future accesses from being reordered to before erasure + atomic::compiler_fence(atomic::Ordering::SeqCst); } #[cfg(not(fuzzing))] diff --git a/src/ecdh.rs b/src/ecdh.rs index 7c01a99..fdf5a16 100644 --- a/src/ecdh.rs +++ b/src/ecdh.rs @@ -46,6 +46,7 @@ const SHARED_SECRET_SIZE: usize = constants::SECRET_KEY_SIZE; #[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct SharedSecret([u8; SHARED_SECRET_SIZE]); impl_display_secret!(SharedSecret); +impl_non_secure_erase!(SharedSecret, 0, [0u8; SHARED_SECRET_SIZE]); impl SharedSecret { /// Creates a new shared secret from a pubkey and secret key. diff --git a/src/key.rs b/src/key.rs index 1cff38b..4593c87 100644 --- a/src/key.rs +++ b/src/key.rs @@ -66,6 +66,7 @@ use crate::{hashes, ThirtyTwoByteHash}; #[derive(Copy, Clone)] pub struct SecretKey([u8; constants::SECRET_KEY_SIZE]); impl_display_secret!(SecretKey); +impl_non_secure_erase!(SecretKey, 0, [1u8; constants::SECRET_KEY_SIZE]); impl PartialEq for SecretKey { /// This implementation is designed to be constant time to help prevent side channel attacks. @@ -992,6 +993,15 @@ impl KeyPair { pub fn sign_schnorr(&self, msg: Message) -> schnorr::Signature { SECP256K1.sign_schnorr(&msg, self) } + + /// Attempts to erase the secret within the underlying array. + /// + /// Note, however, that the compiler is allowed to freely copy or move the contents + /// of this array to other places in memory. Preventing this behavior is very subtle. + /// For more discussion on this, please see the documentation of the + /// [`zeroize`](https://docs.rs/zeroize) crate. + #[inline] + pub fn non_secure_erase(&mut self) { self.0.non_secure_erase(); } } impl From for SecretKey { @@ -1603,6 +1613,17 @@ mod test { assert_eq!(PublicKey::from_slice(&pk1.serialize_uncompressed()[..]), Ok(pk1)); } + #[test] + #[cfg(all(feature = "std", not(fuzzing)))] + fn erased_keypair_is_valid() { + let s = Secp256k1::new(); + let kp = KeyPair::from_seckey_slice(&s, &[1u8; constants::SECRET_KEY_SIZE]) + .expect("valid secret key"); + let mut kp2 = kp; + kp2.non_secure_erase(); + assert!(kp.eq_fast_unstable(&kp2)); + } + #[test] #[rustfmt::skip] fn invalid_secret_key() { diff --git a/src/macros.rs b/src/macros.rs index f7a8d4a..16672da 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -91,6 +91,23 @@ macro_rules! impl_pretty_debug { }; } +macro_rules! impl_non_secure_erase { + ($thing:ident, $target:tt, $value:expr) => { + impl $thing { + /// Attempts to erase the contents of the underlying array. + /// + /// Note, however, that the compiler is allowed to freely copy or move the + /// contents of this array to other places in memory. Preventing this behavior + /// is very subtle. For more discussion on this, please see the documentation + /// of the [`zeroize`](https://docs.rs/zeroize) crate. + #[inline] + pub fn non_secure_erase(&mut self) { + secp256k1_sys::non_secure_erase_impl(&mut self.$target, $value); + } + } + }; +} + /// Formats error. If `std` feature is OFF appends error source (delimited by `: `). We do this /// because `e.source()` is only available in std builds, without this macro the error source is /// lost for no-std builds. diff --git a/src/scalar.rs b/src/scalar.rs index 6ff20d1..57d1e25 100644 --- a/src/scalar.rs +++ b/src/scalar.rs @@ -23,6 +23,7 @@ use crate::constants; #[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd)] pub struct Scalar([u8; 32]); impl_pretty_debug!(Scalar); +impl_non_secure_erase!(Scalar, 0, [0u8; 32]); const MAX_RAW: [u8; 32] = [ 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFE, diff --git a/src/secret.rs b/src/secret.rs index 93951bc..a12e975 100644 --- a/src/secret.rs +++ b/src/secret.rs @@ -86,6 +86,7 @@ macro_rules! impl_display_secret { pub struct DisplaySecret { secret: [u8; SECRET_KEY_SIZE], } +impl_non_secure_erase!(DisplaySecret, secret, [0u8; SECRET_KEY_SIZE]); impl fmt::Debug for DisplaySecret { #[inline]