Skip to content

Commit

Permalink
implement insecure-erase feature
Browse files Browse the repository at this point in the history
  • Loading branch information
kwantam committed Feb 20, 2023
1 parent 6ec968a commit 6b0961c
Show file tree
Hide file tree
Showing 9 changed files with 74 additions and 1 deletion.
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ global-context = ["std"]
# if you are doing a no-std build, then this feature does nothing
# and is not necessary.)
global-context-less-secure = ["global-context"]
insecure-erase = ["secp256k1-sys/insecure-erase"]

[dependencies]
secp256k1-sys = { version = "0.8.0", default-features = false, path = "./secp256k1-sys" }
Expand Down
4 changes: 4 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ 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 the `insecure-erase` feature

The `insecure-erase` feature is provided to assist other libraries in building secure secret erasure. When the `insecure-erase` feature is enabled, secret types (`SecretKey`, `KeyPair`, `SharedSecret`, `Scalar`, and `DisplaySecret`) have a method called `insecure_erase` that *attempts* to overwrite the contained secret. This library makes no guarantees about the security of using `insecure_erase`. In particular, since all of these types are `Copy`, the compiler is free to move and copy them, thereby making additional copies in memory. 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
Expand Down
2 changes: 1 addition & 1 deletion secp256k1-sys/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,4 @@ recovery = []
lowmemory = []
std = ["alloc"]
alloc = []

insecure-erase = []
27 changes: 27 additions & 0 deletions secp256k1-sys/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,33 @@ impl KeyPair {
pk
}
}

/// This function 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`] crate.
/// [`zeroize`]: https://docs.rs/zeroize
#[cfg(feature = "insecure-erase")]
#[inline]
pub fn insecure_erase(&mut self) {
// DUMMY is a valid key pair with secret key `[1u8; 32]`
const DUMMY: [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];
insecure_erase_impl(&mut self.0, DUMMY);
}
}

/// This function implements a best attempt at secure erasure using Rust intrinsics.
/// The implementation is based on the approach used by the [`zeroize`] crate.
/// [`zeroize`]: https://docs.rs/zeroize
#[cfg(feature = "insecure-erase")]
#[inline(always)]
pub fn insecure_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))]
Expand Down
1 change: 1 addition & 0 deletions src/ecdh.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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_insecure_erase!(SharedSecret, 0, [0u8; SHARED_SECRET_SIZE]);

impl SharedSecret {
/// Creates a new shared secret from a pubkey and secret key.
Expand Down
21 changes: 21 additions & 0 deletions src/key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ use crate::{hashes, ThirtyTwoByteHash};
#[derive(Copy, Clone)]
pub struct SecretKey([u8; constants::SECRET_KEY_SIZE]);
impl_display_secret!(SecretKey);
impl_insecure_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.
Expand Down Expand Up @@ -992,6 +993,15 @@ impl KeyPair {
pub fn sign_schnorr(&self, msg: Message) -> schnorr::Signature {
SECP256K1.sign_schnorr(&msg, self)
}

/// This function 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`] crate.
/// [`zeroize`]: https://docs.rs/zeroize
#[inline]
#[cfg(feature = "insecure-erase")]
pub fn insecure_erase(&mut self) { self.0.insecure_erase(); }
}

impl From<KeyPair> for SecretKey {
Expand Down Expand Up @@ -1603,6 +1613,17 @@ mod test {
assert_eq!(PublicKey::from_slice(&pk1.serialize_uncompressed()[..]), Ok(pk1));
}

#[test]
#[cfg(feature = "insecure-erase")]
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.insecure_erase();
assert!(kp.eq_fast_unstable(&kp2));
}

#[test]
#[rustfmt::skip]
fn invalid_secret_key() {
Expand Down
17 changes: 17 additions & 0 deletions src/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,23 @@ macro_rules! impl_pretty_debug {
};
}

macro_rules! impl_insecure_erase {
($thing:ident, $target:tt, $value:expr) => {
#[cfg(feature = "insecure-erase")]
impl $thing {
/// This function 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`] crate.
/// [`zeroize`]: https://docs.rs/zeroize
#[inline]
pub fn insecure_erase(&mut self) {
secp256k1_sys::insecure_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.
Expand Down
1 change: 1 addition & 0 deletions src/scalar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ use crate::constants;
#[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd)]
pub struct Scalar([u8; 32]);
impl_pretty_debug!(Scalar);
impl_insecure_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,
Expand Down
1 change: 1 addition & 0 deletions src/secret.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ macro_rules! impl_display_secret {
pub struct DisplaySecret {
secret: [u8; SECRET_KEY_SIZE],
}
impl_insecure_erase!(DisplaySecret, secret, [0u8; SECRET_KEY_SIZE]);

impl fmt::Debug for DisplaySecret {
#[inline]
Expand Down

0 comments on commit 6b0961c

Please sign in to comment.