From 8fffbeab13e8beaef7dfa3333368d481c02f723a Mon Sep 17 00:00:00 2001 From: kwantam Date: Mon, 20 Feb 2023 13:36:32 -0500 Subject: [PATCH] implement "non_secure_erase" methods This PR implements a `non_secure_erase()` method on SecretKey, KeyPair, SharedSecret, Scalar, and DisplaySecret. The purpose of this method is to (attempt to) overwrite secret data with valid default values. This method can be used by libraries to implement Zeroize on structs containing secret values. `non_secure_erase()` attempts to avoid being optimized away or reordered using the same mechanism as the zeroize crate: first, using `std::ptr::write_volatile` (which will not be optimized away) to overwrite the memory, then using a memory fence to prevent subtle issues due to load or store reordering. Note, however, that this method is *very unlikely* to do anything useful on its own. Effective use involves carefully placing these values inside non-Copy structs and pinning those structs in place. See the [`zeroize`](https://docs.rs/zeroize) documentation for tips and tricks, and for further discussion. [this commit includes a squashed-in commit from tcharding to fix docs and helpful suggestions from apoelstra and Kixunil] --- README.md | 10 ++++++++++ secp256k1-sys/Cargo.toml | 1 - secp256k1-sys/src/lib.rs | 32 ++++++++++++++++++++++++++++++++ src/ecdh.rs | 1 + src/key.rs | 21 +++++++++++++++++++++ src/macros.rs | 17 +++++++++++++++++ src/scalar.rs | 1 + src/secret.rs | 1 + 8 files changed, 83 insertions(+), 1 deletion(-) 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]