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]

Author: kwantam

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

secp256k1-sys/Cargo.toml

@@ -32,4 +32,3 @@ recovery = []
 lowmemory = []
 std = ["alloc"]
 alloc = []
-

secp256k1-sys/src/lib.rs

@@ -454,6 +454,38 @@ impl KeyPair {
             pk
         }
     }
+
+    /// Attempts to erase the contents of the underlying array.
+    ///
+    /// Note, however, that since this type is `Copy`, ensuring that the contents
+    /// of this array don't end up elsewhere in memory 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<T>(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))]

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.

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,14 @@ 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 since [`KeyPair`] implements `Copy`, ensuring that the secret data
+    /// doesn't end up elsewhere in memory 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<KeyPair> for SecretKey {
@@ -1603,6 +1612,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() {

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 since this type is `Copy`, ensuring that the contents
+            /// of this array don't end up elsewhere in memory 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.

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,

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]