diff --git a/parity-crypto/Cargo.toml b/parity-crypto/Cargo.toml index 74f564d7f..4304e579a 100644 --- a/parity-crypto/Cargo.toml +++ b/parity-crypto/Cargo.toml @@ -31,6 +31,8 @@ sha2 = "0.8.0" subtle = "2.2.1" tiny-keccak = { version = "2.0", features = ["keccak"] } zeroize = { version = "1.0.0", default-features = false } +region = { version = "2.2.0", optional = true } +log = { version = "0.4.11", optional = true } [dev-dependencies] criterion = "0.3.0" @@ -40,4 +42,4 @@ hex-literal = "0.2.1" default = [] # public key crypto utils # moved from ethkey module in parity ethereum repository -publickey = ["secp256k1", "lazy_static", "ethereum-types", "rustc-hex"] +publickey = ["ethereum-types", "lazy_static", "log", "region", "rustc-hex", "secp256k1"] diff --git a/parity-crypto/src/publickey/secret_key.rs b/parity-crypto/src/publickey/secret_key.rs index 269afdf3e..a6c0335bf 100644 --- a/parity-crypto/src/publickey/secret_key.rs +++ b/parity-crypto/src/publickey/secret_key.rs @@ -21,14 +21,29 @@ use zeroize::Zeroize; use crate::publickey::Error; /// Represents secret key -#[derive(Clone, PartialEq, Eq)] pub struct Secret { + // We're using `region::lock` to avoid swapping secret data to disk. + // + // The potential error is ignored as this security measure is optional + // and the syscall may fail due to multiple reasons, e.g. on windows + // there is a small amount of maximal pages a process can lock by default. + // Also it's not guaranteed that a memory page won't be written to disk, + // e.g. in case of hibernation or memory starvation. + // + // Since we don't propagate the error if a syscall fails, + // the guard returned by `region::lock` is optional. + mlock_guard: Option, inner: Box, } impl Drop for Secret { fn drop(&mut self) { - self.inner.0.zeroize() + self.inner.0.zeroize(); + // `region::LockGuard::drop` will panic only if + // `region::unlock()` has failed for some reason + // AND `debug_assertions` are enabled. + // https://docs.rs/region/2.2.0/src/region/lock.rs.html#86-91 + let _ = self.mlock_guard.take(); } } @@ -57,9 +72,9 @@ impl Secret { if key.len() != 32 { return None; } - let mut h = H256::zero(); - h.as_bytes_mut().copy_from_slice(&key[0..32]); - Some(Secret { inner: Box::new(h) }) + let mut me = Self::zero(); + me.inner.as_bytes_mut().copy_from_slice(&key[0..32]); + Some(me) } /// Creates a `Secret` from the given `str` representation, @@ -67,13 +82,26 @@ impl Secret { /// the secret. /// Caller is responsible to zeroize input slice. pub fn copy_from_str(s: &str) -> Result { - let h = H256::from_str(s).map_err(|e| Error::Custom(format!("{:?}", e)))?; - Ok(Secret { inner: Box::new(h) }) + let mut h = H256::from_str(s).map_err(|e| Error::Custom(format!("{:?}", e)))?; + let mut me = Self::zero(); + me.inner.as_bytes_mut().copy_from_slice(h.as_bytes()); + h.0.zeroize(); + Ok(me) } /// Creates zero key, which is invalid for crypto operations, but valid for math operation. pub fn zero() -> Self { - Secret { inner: Box::new(H256::zero()) } + let inner = Box::new(H256::zero()); + let bytes = inner.as_bytes(); + // Try to lock the memory page and convert the result to `Option`. + let mlock_guard = match region::lock(bytes.as_ptr(), bytes.len()) { + Ok(guard) => Some(guard), + Err(e) => { + log::warn!("Failed to lock memory page with a Secret: {}", e); + None + } + }; + Self { mlock_guard, inner } } /// Imports and validates the key. @@ -98,7 +126,7 @@ impl Secret { match (self.is_zero(), other.is_zero()) { (true, true) | (false, true) => Ok(()), (true, false) => { - *self = other.clone(); + self.clone_from(other); Ok(()) } (false, false) => { @@ -118,7 +146,7 @@ impl Secret { match (self.is_zero(), other.is_zero()) { (true, true) | (false, true) => Ok(()), (true, false) => { - *self = other.clone(); + self.clone_from(other); self.neg() } (false, false) => { @@ -213,6 +241,25 @@ impl Secret { } } +impl Clone for Secret { + fn clone(&self) -> Self { + let mut copy = Self::zero(); + copy.clone_from(self); + copy + } + fn clone_from(&mut self, other: &Self) { + self.inner.as_bytes_mut().copy_from_slice(other.inner.as_bytes()); + } +} + +impl PartialEq for Secret { + fn eq(&self, other: &Self) -> bool { + self.inner == other.inner + } +} + +impl Eq for Secret {} + #[deprecated(since = "0.6.2", note = "please use `copy_from_str` instead, input is not zeroized")] impl FromStr for Secret { type Err = Error; @@ -224,18 +271,22 @@ impl FromStr for Secret { impl From<[u8; 32]> for Secret { #[inline(always)] fn from(mut k: [u8; 32]) -> Self { - let result = Secret { inner: Box::new(H256(k)) }; + let mut me = Self::zero(); + let mut secret = H256(k); + me.inner.as_bytes_mut().copy_from_slice(secret.as_bytes()); + secret.0.zeroize(); k.zeroize(); - result + me } } impl From for Secret { #[inline(always)] fn from(mut s: H256) -> Self { - let result = s.0.into(); + let mut me = Self::zero(); + me.inner.as_bytes_mut().copy_from_slice(s.as_bytes()); s.0.zeroize(); - result + me } } @@ -256,7 +307,7 @@ impl TryFrom<&[u8]> for Secret { if b.len() != SECP256K1_SECRET_KEY_SIZE { return Err(Error::InvalidSecretKey); } - Ok(Self { inner: Box::new(H256::from_slice(b)) }) + Ok(Self::copy_from_slice(b).expect("checked len above; qed")) } }