From 43f7295e79083e973f78d1db6e861ab372a0ce1b Mon Sep 17 00:00:00 2001 From: cheme Date: Mon, 22 Jun 2020 18:00:38 +0200 Subject: [PATCH 1/7] Restrict `Protected` to some heap types. --- primitives/core/src/crypto.rs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/primitives/core/src/crypto.rs b/primitives/core/src/crypto.rs index 9b84bd84ca261..ce914de585aff 100644 --- a/primitives/core/src/crypto.rs +++ b/primitives/core/src/crypto.rs @@ -106,8 +106,16 @@ impl std::fmt::Debug for Protected { } } -impl From for Protected { - fn from(t: T) -> Self { +#[cfg(feature = "std")] +impl From for Protected { + fn from(t: String) -> Self { + Protected(t) + } +} + +#[cfg(feature = "std")] +impl From> for Protected> { + fn from(t: Vec) -> Self { Protected(t) } } From c7d7b070c9186f69c8f316c341d6a2535a71fd3a Mon Sep 17 00:00:00 2001 From: cheme Date: Tue, 23 Jun 2020 09:35:04 +0200 Subject: [PATCH 2/7] Comment abut Protected usage. --- primitives/core/src/crypto.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/primitives/core/src/crypto.rs b/primitives/core/src/crypto.rs index ce914de585aff..2fb5ae14231ec 100644 --- a/primitives/core/src/crypto.rs +++ b/primitives/core/src/crypto.rs @@ -80,6 +80,11 @@ impl> UncheckedInto for S { } /// A store for sensitive data. +/// Current API restricts instantiation to +/// `String` and `Vec`, but generally +/// this could be use for any content that +/// is using a fix location on memory (please +/// refer to `zeroize` crate documentation). /// /// Calls `Zeroize::zeroize` upon `Drop`. #[derive(Clone)] From 3f0b9b2101782a2eb57c51e3aa4c3b917c67e7ce Mon Sep 17 00:00:00 2001 From: cheme Date: Tue, 23 Jun 2020 12:50:41 +0200 Subject: [PATCH 3/7] Remove Protected from crypto, use secrecy crate for existing uses. --- Cargo.lock | 10 ++++ client/cli/src/params/keystore_params.rs | 27 +++++++--- client/keystore/src/lib.rs | 27 ++++++---- client/service/src/config.rs | 4 +- primitives/core/Cargo.toml | 2 + primitives/core/src/crypto.rs | 66 +++--------------------- 6 files changed, 59 insertions(+), 77 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 75976823954a1..e7e4a27371236 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6981,6 +6981,15 @@ dependencies = [ "untrusted", ] +[[package]] +name = "secrecy" +version = "0.6.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9182278ed645df3477a9c27bfee0621c621aa16f6972635f7f795dae3d81070f" +dependencies = [ + "zeroize", +] + [[package]] name = "security-framework" version = "0.4.2" @@ -7517,6 +7526,7 @@ dependencies = [ "rand_chacha 0.2.2", "regex", "schnorrkel", + "secrecy", "serde", "serde_json", "sha2", diff --git a/client/cli/src/params/keystore_params.rs b/client/cli/src/params/keystore_params.rs index 840cc51dff33f..4947c0192e3d0 100644 --- a/client/cli/src/params/keystore_params.rs +++ b/client/cli/src/params/keystore_params.rs @@ -56,24 +56,37 @@ pub struct KeystoreParams { pub password_filename: Option, } +impl Drop for KeystoreParams { + fn drop(&mut self) { + use sp_core::crypto::Zeroize; + self.password.zeroize(); + } +} + impl KeystoreParams { /// Get the keystore configuration for the parameters pub fn keystore_config(&self, base_path: &PathBuf) -> Result { let password = if self.password_interactive { #[cfg(not(target_os = "unknown"))] { - Some(input_keystore_password()?.into()) + let mut password = input_keystore_password()?; + let secret = std::str::FromStr::from_str(password.as_str())?; + use sp_core::crypto::Zeroize; + password.zeroize(); + Some(secret) } #[cfg(target_os = "unknown")] None } else if let Some(ref file) = self.password_filename { - Some( - fs::read_to_string(file) - .map_err(|e| format!("{}", e))? - .into(), - ) + let mut password = fs::read_to_string(file) + .map_err(|e| format!("{}", e))?; + let secret = std::str::FromStr::from_str(password.as_str())?; + use sp_core::crypto::Zeroize; + password.zeroize(); + Some(secret) } else if let Some(ref password) = self.password { - Some(password.clone().into()) + let secret = std::str::FromStr::from_str(password.as_str())?; + Some(secret) } else { None }; diff --git a/client/keystore/src/lib.rs b/client/keystore/src/lib.rs index aed60ab0cf823..7fec32bae246a 100644 --- a/client/keystore/src/lib.rs +++ b/client/keystore/src/lib.rs @@ -19,7 +19,7 @@ #![warn(missing_docs)] use std::{collections::{HashMap, HashSet}, path::PathBuf, fs::{self, File}, io::{self, Write}, sync::Arc}; use sp_core::{ - crypto::{IsWrappedBy, CryptoTypePublicPair, KeyTypeId, Pair as PairT, Protected, Public}, + crypto::{IsWrappedBy, CryptoTypePublicPair, KeyTypeId, Pair as PairT, ExposeSecret, SecretString, Public}, traits::{BareCryptoStore, Error as TraitError}, sr25519::{Public as Sr25519Public, Pair as Sr25519Pair}, vrf::{VRFTranscriptData, VRFSignature, make_transcript}, @@ -95,14 +95,14 @@ pub struct Store { path: Option, /// Map over `(KeyTypeId, Raw public key)` -> `Key phrase/seed` additional: HashMap<(KeyTypeId, Vec), String>, - password: Option>, + password: Option, } impl Store { /// Open the store at the given path. /// /// Optionally takes a password that will be used to encrypt/decrypt the keys. - pub fn open>(path: T, password: Option>) -> Result { + pub fn open>(path: T, password: Option) -> Result { let path = path.into(); fs::create_dir_all(&path)?; @@ -155,7 +155,7 @@ impl Store { pub fn insert_by_type(&self, key_type: KeyTypeId, suri: &str) -> Result { let pair = Pair::from_string( suri, - self.password.as_ref().map(|p| &***p) + self.password() ).map_err(|_| Error::InvalidSeed)?; self.insert_unknown(key_type, suri, pair.public().as_slice()) .map_err(|_| Error::Unavailable)?; @@ -173,7 +173,7 @@ impl Store { /// /// Places it into the file system store. pub fn generate_by_type(&self, key_type: KeyTypeId) -> Result { - let (pair, phrase, _) = Pair::generate_with_phrase(self.password.as_ref().map(|p| &***p)); + let (pair, phrase, _) = Pair::generate_with_phrase(self.password()); if let Some(path) = self.key_file_path(pair.public().as_slice(), key_type) { let mut file = File::create(path)?; serde_json::to_writer(&file, &phrase)?; @@ -229,7 +229,7 @@ impl Store { let phrase = self.key_phrase_by_type(public.as_slice(), key_type)?; let pair = Pair::from_string( &phrase, - self.password.as_ref().map(|p| &***p), + self.password(), ).map_err(|_| Error::InvalidPhrase)?; if &pair.public() == public { @@ -434,7 +434,9 @@ impl BareCryptoStore for Store { } fn password(&self) -> Option<&str> { - self.password.as_ref().map(|x| x.as_str()) + self.password.as_ref() + .map(|p| p.expose_secret()) + .map(|p| p.as_str()) } fn has_keys(&self, public_keys: &[(Vec, KeyTypeId)]) -> bool { @@ -464,6 +466,7 @@ mod tests { use super::*; use tempfile::TempDir; use sp_core::{testing::SR25519, crypto::Ss58Codec}; + use std::str::FromStr; #[test] fn basic_store() { @@ -504,7 +507,10 @@ mod tests { fn password_being_used() { let password = String::from("password"); let temp_dir = TempDir::new().unwrap(); - let store = Store::open(temp_dir.path(), Some(password.clone().into())).unwrap(); + let store = Store::open( + temp_dir.path(), + Some(FromStr::from_str(password.as_str()).unwrap()), + ).unwrap(); let pair: ed25519::AppPair = store.write().generate().unwrap(); assert_eq!( @@ -516,7 +522,10 @@ mod tests { let store = Store::open(temp_dir.path(), None).unwrap(); assert!(store.read().key_pair::(&pair.public()).is_err()); - let store = Store::open(temp_dir.path(), Some(password.into())).unwrap(); + let store = Store::open( + temp_dir.path(), + Some(FromStr::from_str(password.as_str()).unwrap()), + ).unwrap(); assert_eq!( pair.public(), store.read().key_pair::(&pair.public()).unwrap().public(), diff --git a/client/service/src/config.rs b/client/service/src/config.rs index b79831d57bba3..3b39b5c8aa1f2 100644 --- a/client/service/src/config.rs +++ b/client/service/src/config.rs @@ -27,7 +27,7 @@ use sc_client_api::execution_extensions::ExecutionStrategies; use std::{io, future::Future, path::{PathBuf, Path}, pin::Pin, net::SocketAddr, sync::Arc}; pub use sc_transaction_pool::txpool::Options as TransactionPoolOptions; use sc_chain_spec::ChainSpec; -use sp_core::crypto::Protected; +use sp_core::crypto::SecretString; pub use sc_telemetry::TelemetryEndpoints; use prometheus_endpoint::Registry; #[cfg(not(target_os = "unknown"))] @@ -127,7 +127,7 @@ pub enum KeystoreConfig { /// The path of the keystore. path: PathBuf, /// Node keystore's password. - password: Option> + password: Option }, /// In-memory keystore. Recommended for in-browser nodes. InMemory, diff --git a/primitives/core/Cargo.toml b/primitives/core/Cargo.toml index 3c37f57e7072e..a67f9d7a73584 100644 --- a/primitives/core/Cargo.toml +++ b/primitives/core/Cargo.toml @@ -31,6 +31,7 @@ tiny-bip39 = { version = "0.7", optional = true } regex = { version = "1.3.1", optional = true } num-traits = { version = "0.2.8", default-features = false } zeroize = { version = "1.0.0", default-features = false } +secrecy = { version = "0.6.0", default-features = false } lazy_static = { version = "1.4.0", default-features = false, optional = true } parking_lot = { version = "0.10.0", optional = true } sp-debug-derive = { version = "2.0.0-rc3", path = "../debug-derive" } @@ -106,6 +107,7 @@ std = [ "sp-storage/std", "sp-runtime-interface/std", "zeroize/alloc", + "secrecy/alloc", "futures", "futures/thread-pool", "libsecp256k1/std", diff --git a/primitives/core/src/crypto.rs b/primitives/core/src/crypto.rs index 2fb5ae14231ec..c19f255a2b759 100644 --- a/primitives/core/src/crypto.rs +++ b/primitives/core/src/crypto.rs @@ -37,10 +37,16 @@ use regex::Regex; use base58::{FromBase58, ToBase58}; #[cfg(feature = "std")] use crate::hexdisplay::HexDisplay; -use zeroize::Zeroize; #[doc(hidden)] pub use sp_std::ops::Deref; use sp_runtime_interface::pass_by::PassByInner; +#[doc(hidden)] +pub use zeroize::Zeroize; +#[doc(hidden)] +pub use secrecy::ExposeSecret; +/// A store for sensitive data. +#[cfg(feature = "std")] +pub use secrecy::SecretString; /// The root phrase for our publicly known keys. pub const DEV_PHRASE: &str = "bottom drive obey lake curtain smoke basket hold race lonely fit walk"; @@ -79,64 +85,6 @@ impl> UncheckedInto for S { } } -/// A store for sensitive data. -/// Current API restricts instantiation to -/// `String` and `Vec`, but generally -/// this could be use for any content that -/// is using a fix location on memory (please -/// refer to `zeroize` crate documentation). -/// -/// Calls `Zeroize::zeroize` upon `Drop`. -#[derive(Clone)] -pub struct Protected(T); - -impl AsRef for Protected { - fn as_ref(&self) -> &T { - &self.0 - } -} - -impl sp_std::ops::Deref for Protected { - type Target = T; - - fn deref(&self) -> &T { - &self.0 - } -} - -#[cfg(feature = "std")] -impl std::fmt::Debug for Protected { - fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result { - write!(fmt, "") - } -} - -#[cfg(feature = "std")] -impl From for Protected { - fn from(t: String) -> Self { - Protected(t) - } -} - -#[cfg(feature = "std")] -impl From> for Protected> { - fn from(t: Vec) -> Self { - Protected(t) - } -} - -impl Zeroize for Protected { - fn zeroize(&mut self) { - self.0.zeroize() - } -} - -impl Drop for Protected { - fn drop(&mut self) { - self.zeroize() - } -} - /// An error with the interpretation of a secret. #[derive(Debug, Clone, PartialEq, Eq)] #[cfg(feature = "full_crypto")] From 2438b4c89bcdd4f8d6e5a476857a88288e37b5b0 Mon Sep 17 00:00:00 2001 From: cheme Date: Wed, 24 Jun 2020 23:10:19 +0200 Subject: [PATCH 4/7] use a parse function --- client/cli/src/params/keystore_params.rs | 16 ++++------------ primitives/core/src/crypto.rs | 11 +++++++++-- 2 files changed, 13 insertions(+), 14 deletions(-) diff --git a/client/cli/src/params/keystore_params.rs b/client/cli/src/params/keystore_params.rs index 4947c0192e3d0..62d952e4b8ed3 100644 --- a/client/cli/src/params/keystore_params.rs +++ b/client/cli/src/params/keystore_params.rs @@ -21,6 +21,7 @@ use sc_service::config::KeystoreConfig; use std::fs; use std::path::PathBuf; use structopt::StructOpt; +use sp_core::crypto::{SecretString, secrety_string_from_str}; /// default sub directory for the key store const DEFAULT_KEYSTORE_CONFIG_PATH: &'static str = "keystore"; @@ -42,9 +43,10 @@ pub struct KeystoreParams { /// Password used by the keystore. #[structopt( long = "password", + parse(try_from_str = secrety_string_from_str), conflicts_with_all = &[ "password-interactive", "password-filename" ] )] - pub password: Option, + pub password: Option, /// File that contains the password used by the keystore. #[structopt( @@ -56,13 +58,6 @@ pub struct KeystoreParams { pub password_filename: Option, } -impl Drop for KeystoreParams { - fn drop(&mut self) { - use sp_core::crypto::Zeroize; - self.password.zeroize(); - } -} - impl KeystoreParams { /// Get the keystore configuration for the parameters pub fn keystore_config(&self, base_path: &PathBuf) -> Result { @@ -84,11 +79,8 @@ impl KeystoreParams { use sp_core::crypto::Zeroize; password.zeroize(); Some(secret) - } else if let Some(ref password) = self.password { - let secret = std::str::FromStr::from_str(password.as_str())?; - Some(secret) } else { - None + self.password.clone() }; let path = self diff --git a/primitives/core/src/crypto.rs b/primitives/core/src/crypto.rs index c19f255a2b759..373e1a0707fcb 100644 --- a/primitives/core/src/crypto.rs +++ b/primitives/core/src/crypto.rs @@ -40,14 +40,21 @@ use crate::hexdisplay::HexDisplay; #[doc(hidden)] pub use sp_std::ops::Deref; use sp_runtime_interface::pass_by::PassByInner; -#[doc(hidden)] +/// Trait to zeroize a memory buffer. pub use zeroize::Zeroize; -#[doc(hidden)] +/// Trait for accessing reference to `SecretString`. pub use secrecy::ExposeSecret; /// A store for sensitive data. #[cfg(feature = "std")] pub use secrecy::SecretString; +#[cfg(feature = "std")] +/// Parse a sercret string, returning a displayable error. +pub fn secrety_string_from_str(s: &str) -> Result { + Ok(std::str::FromStr::from_str(s) + .map_err(|_e| "Could not get SecretString".to_string())?) +} + /// The root phrase for our publicly known keys. pub const DEV_PHRASE: &str = "bottom drive obey lake curtain smoke basket hold race lonely fit walk"; From cf77ff392f76d79e420d0bd1c5b811948180a254 Mon Sep 17 00:00:00 2001 From: Emeric Chevalier Date: Tue, 30 Jun 2020 18:47:33 +0200 Subject: [PATCH 5/7] fix error convert --- client/cli/src/params/keystore_params.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/client/cli/src/params/keystore_params.rs b/client/cli/src/params/keystore_params.rs index 62d952e4b8ed3..5fd1bf0deb028 100644 --- a/client/cli/src/params/keystore_params.rs +++ b/client/cli/src/params/keystore_params.rs @@ -65,7 +65,8 @@ impl KeystoreParams { #[cfg(not(target_os = "unknown"))] { let mut password = input_keystore_password()?; - let secret = std::str::FromStr::from_str(password.as_str())?; + let secret = std::str::FromStr::from_str(password.as_str()) + .map_err(|()| "Error reading password")?; use sp_core::crypto::Zeroize; password.zeroize(); Some(secret) @@ -75,7 +76,8 @@ impl KeystoreParams { } else if let Some(ref file) = self.password_filename { let mut password = fs::read_to_string(file) .map_err(|e| format!("{}", e))?; - let secret = std::str::FromStr::from_str(password.as_str())?; + let secret = std::str::FromStr::from_str(password.as_str()) + .map_err(|()| "Error reading password")?; use sp_core::crypto::Zeroize; password.zeroize(); Some(secret) From 005dfc4a543172911f8d5666f0cc116bb699d3a3 Mon Sep 17 00:00:00 2001 From: cheme Date: Wed, 1 Jul 2020 10:35:31 +0200 Subject: [PATCH 6/7] Rename and move secretY string function. --- client/cli/src/params/keystore_params.rs | 10 ++++++++-- primitives/core/src/crypto.rs | 7 ------- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/client/cli/src/params/keystore_params.rs b/client/cli/src/params/keystore_params.rs index 5fd1bf0deb028..19441215393e6 100644 --- a/client/cli/src/params/keystore_params.rs +++ b/client/cli/src/params/keystore_params.rs @@ -21,7 +21,7 @@ use sc_service::config::KeystoreConfig; use std::fs; use std::path::PathBuf; use structopt::StructOpt; -use sp_core::crypto::{SecretString, secrety_string_from_str}; +use sp_core::crypto::SecretString; /// default sub directory for the key store const DEFAULT_KEYSTORE_CONFIG_PATH: &'static str = "keystore"; @@ -43,7 +43,7 @@ pub struct KeystoreParams { /// Password used by the keystore. #[structopt( long = "password", - parse(try_from_str = secrety_string_from_str), + parse(try_from_str = secret_string_from_str), conflicts_with_all = &[ "password-interactive", "password-filename" ] )] pub password: Option, @@ -58,6 +58,12 @@ pub struct KeystoreParams { pub password_filename: Option, } +/// Parse a sercret string, returning a displayable error. +pub fn secret_string_from_str(s: &str) -> Result { + Ok(std::str::FromStr::from_str(s) + .map_err(|_e| "Could not get SecretString".to_string())?) +} + impl KeystoreParams { /// Get the keystore configuration for the parameters pub fn keystore_config(&self, base_path: &PathBuf) -> Result { diff --git a/primitives/core/src/crypto.rs b/primitives/core/src/crypto.rs index e773e75cdc55f..745f5776fe850 100644 --- a/primitives/core/src/crypto.rs +++ b/primitives/core/src/crypto.rs @@ -48,13 +48,6 @@ pub use secrecy::ExposeSecret; #[cfg(feature = "std")] pub use secrecy::SecretString; -#[cfg(feature = "std")] -/// Parse a sercret string, returning a displayable error. -pub fn secrety_string_from_str(s: &str) -> Result { - Ok(std::str::FromStr::from_str(s) - .map_err(|_e| "Could not get SecretString".to_string())?) -} - /// The root phrase for our publicly known keys. pub const DEV_PHRASE: &str = "bottom drive obey lake curtain smoke basket hold race lonely fit walk"; From 27cbd6a1390068a6f66d6183f08499e30637e96f Mon Sep 17 00:00:00 2001 From: cheme Date: Wed, 1 Jul 2020 11:09:39 +0200 Subject: [PATCH 7/7] std result --- client/cli/src/params/keystore_params.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/cli/src/params/keystore_params.rs b/client/cli/src/params/keystore_params.rs index 19441215393e6..8b20dd247aec6 100644 --- a/client/cli/src/params/keystore_params.rs +++ b/client/cli/src/params/keystore_params.rs @@ -59,7 +59,7 @@ pub struct KeystoreParams { } /// Parse a sercret string, returning a displayable error. -pub fn secret_string_from_str(s: &str) -> Result { +pub fn secret_string_from_str(s: &str) -> std::result::Result { Ok(std::str::FromStr::from_str(s) .map_err(|_e| "Could not get SecretString".to_string())?) }