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

@@ -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 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<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,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<KeyPair> 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() {

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.

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]