Merge #582: implement insecure-erase feature (was: Zeroize)

8fffbea implement "non_secure_erase" methods (kwantam)

Pull request description:

  This PR adds [`Zeroize`](https://docs.rs/zeroize) derivations for the following structs:

  - `SecretKey`
  - `KeyPair`
  - `SharedSecret`
  - `Scalar`
  - `DisplaySecret`

  This is *only* a Zeroize impl, and does not make Zeroize happen automatically on drop (doing that would be a breaking change because it would preclude deriving `Copy`). But this is still useful, because it allows downstream libraries to implement `ZeroizeOnDrop` for structs that contain such secrets and/or simply to use the `Zeroizing` container struct.

  Because these new impls are never invoked automatically, performance impact should be zero. Safety-wise, the `Zeroize` library appears to be widely used in cryptographic code. For example, Supranational's [blst](https://github.com/supranational/blst) Rust bindings use it, and in turn are used in one of the most popular eth2 validator implementations.

  Thanks for maintaining a really great library!

ACKs for top commit:
  tcharding:
    FWIW ACK 8fffbea
  apoelstra:
    ACK 8fffbea

Tree-SHA512: 28d2cdcc6bd2d2d6330b67ae8635561882e869199d8fef9a3ebc3f368a7a0c2c00b818281190133f551b099e9c5226f104a56edc14c9b6f699ceba49e4b24563

Author: apoelstra

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]