From 8ca3be99e9d1585dbe187e33e10bc5f405009fac Mon Sep 17 00:00:00 2001 From: Isis Lovecruft Date: Mon, 9 Dec 2019 22:39:57 +0000 Subject: [PATCH] Switch to using zeroize rather than clear_on_drop. --- Cargo.toml | 6 +++--- src/ed25519.rs | 25 +------------------------ src/lib.rs | 2 +- src/secret.rs | 51 +++++++++++++++++++++++++++++++------------------- 4 files changed, 37 insertions(+), 47 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index c9d77a6..fc87a11 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -22,13 +22,13 @@ travis-ci = { repository = "dalek-cryptography/ed25519-dalek", branch = "master" features = ["nightly", "batch"] [dependencies] -clear_on_drop = { version = "0.2" } curve25519-dalek = { version = "2", default-features = false } merlin = { version = "1", default-features = false, optional = true, git = "https://github.com/isislovecruft/merlin", branch = "develop" } rand = { version = "0.7", default-features = false, optional = true } rand_core = { version = "0.5", default-features = false, optional = true } serde = { version = "1.0", optional = true } sha2 = { version = "0.8", default-features = false } +zeroize = { version = "1", default-features = false, features = ["zeroize_derive"] } [dev-dependencies] hex = "^0.4" @@ -46,8 +46,8 @@ harness = false [features] default = ["std", "u64_backend"] std = ["curve25519-dalek/std", "sha2/std", "rand/std"] -alloc = ["curve25519-dalek/alloc", "rand/alloc"] -nightly = ["curve25519-dalek/nightly", "clear_on_drop/nightly", "rand/nightly"] +alloc = ["curve25519-dalek/alloc", "rand/alloc", "zeroize/alloc"] +nightly = ["curve25519-dalek/nightly", "rand/nightly"] batch = ["merlin", "rand"] # This feature enables deterministic batch verification. batch_deterministic = ["merlin", "rand", "rand_core"] diff --git a/src/ed25519.rs b/src/ed25519.rs index 7c55a65..dd150bf 100644 --- a/src/ed25519.rs +++ b/src/ed25519.rs @@ -34,7 +34,7 @@ pub use crate::secret::*; pub use crate::signature::*; /// An ed25519 keypair. -#[derive(Debug, Default)] // we derive Default in order to use the clear() method in Drop +#[derive(Debug)] pub struct Keypair { /// The secret half of this keypair. pub secret: SecretKey, @@ -444,26 +444,3 @@ impl<'d> Deserialize<'d> for Keypair { deserializer.deserialize_bytes(KeypairVisitor) } } - -#[cfg(test)] -mod test { - use super::*; - - use clear_on_drop::clear::Clear; - - #[test] - fn keypair_clear_on_drop() { - let mut keypair: Keypair = Keypair::from_bytes(&[1u8; KEYPAIR_LENGTH][..]).unwrap(); - - keypair.clear(); - - fn as_bytes(x: &T) -> &[u8] { - use std::mem; - use std::slice; - - unsafe { slice::from_raw_parts(x as *const T as *const u8, mem::size_of_val(x)) } - } - - assert!(!as_bytes(&keypair).contains(&0x15)); - } -} diff --git a/src/lib.rs b/src/lib.rs index 32aff4e..bee0f7c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -239,7 +239,6 @@ extern crate std; #[cfg(all(feature = "alloc", not(feature = "std")))] extern crate alloc; -extern crate clear_on_drop; extern crate curve25519_dalek; #[cfg(all(any(feature = "batch", feature = "batch_deterministic"), any(feature = "std", feature = "alloc")))] extern crate merlin; @@ -248,6 +247,7 @@ extern crate rand; #[cfg(feature = "serde")] extern crate serde; extern crate sha2; +extern crate zeroize; #[cfg(all(any(feature = "batch", feature = "batch_deterministic"), any(feature = "std", feature = "alloc")))] mod batch; diff --git a/src/secret.rs b/src/secret.rs index 0c54275..f1e751d 100644 --- a/src/secret.rs +++ b/src/secret.rs @@ -11,8 +11,6 @@ use core::fmt::Debug; -use clear_on_drop::clear::Clear; - use curve25519_dalek::constants; use curve25519_dalek::digest::generic_array::typenum::U64; use curve25519_dalek::digest::Digest; @@ -32,13 +30,19 @@ use serde::{Deserialize, Serialize}; #[cfg(feature = "serde")] use serde::{Deserializer, Serializer}; +use zeroize::Zeroize; + use crate::constants::*; use crate::errors::*; use crate::public::*; use crate::signature::*; /// An EdDSA secret key. -#[derive(Default)] // we derive Default in order to use the clear() method in Drop +/// +/// Instances of this secret are automatically overwritten with zeroes when they +/// fall out of scope. +#[derive(Zeroize)] +#[zeroize(drop)] // Overwrite secret key material with null bytes when it goes out of scope. pub struct SecretKey(pub(crate) [u8; SECRET_KEY_LENGTH]); impl Debug for SecretKey { @@ -47,13 +51,6 @@ impl Debug for SecretKey { } } -/// Overwrite secret key material with null bytes when it goes out of scope. -impl Drop for SecretKey { - fn drop(&mut self) { - self.0.clear(); - } -} - impl AsRef<[u8]> for SecretKey { fn as_ref(&self) -> &[u8] { self.as_bytes() @@ -223,6 +220,9 @@ impl<'d> Deserialize<'d> for SecretKey { /// upper half is used a sort of half-baked, ill-designed² pseudo-domain-separation /// "nonce"-like thing, which is used during signature production by /// concatenating it with the message to be signed before the message is hashed. +/// +/// Instances of this secret are automatically overwritten with zeroes when they +/// fall out of scope. // // ¹ This results in a slight bias towards non-uniformity at one spectrum of // the range of valid keys. Oh well: not my idea; not my problem. @@ -250,20 +250,13 @@ impl<'d> Deserialize<'d> for SecretKey { // same signature scheme, and which both fail in exactly the same way. For a // better-designed, Schnorr-based signature scheme, see Trevor Perrin's work on // "generalised EdDSA" and "VXEdDSA". -#[derive(Default)] // we derive Default in order to use the clear() method in Drop +#[derive(Zeroize)] +#[zeroize(drop)] // Overwrite secret key material with null bytes when it goes out of scope. pub struct ExpandedSecretKey { pub(crate) key: Scalar, pub(crate) nonce: [u8; 32], } -/// Overwrite secret key material with null bytes when it goes out of scope. -impl Drop for ExpandedSecretKey { - fn drop(&mut self) { - self.key.clear(); - self.nonce.clear(); - } -} - impl<'a> From<&'a SecretKey> for ExpandedSecretKey { /// Construct an `ExpandedSecretKey` from a `SecretKey`. /// @@ -554,3 +547,23 @@ impl<'d> Deserialize<'d> for ExpandedSecretKey { deserializer.deserialize_bytes(ExpandedSecretKeyVisitor) } } + +#[cfg(test)] +mod test { + use super::*; + + #[test] + fn secret_key_zeroize_on_drop() { + let secret_ptr: *const u8; + + { // scope for the secret to ensure it's been dropped + let secret = SecretKey::from_bytes(&[0x15u8; 32][..]).unwrap(); + + secret_ptr = secret.0.as_ptr(); + } + + let memory: &[u8] = unsafe { ::std::slice::from_raw_parts(secret_ptr, 32) }; + + assert!(!memory.contains(&0x15)); + } +}