Restrict Protected to some heap types. (#6471)

* Restrict `Protected` to some heap types.

* Comment abut Protected usage.

* Remove Protected from crypto, use secrecy crate for existing uses.

* use a parse function

* fix error convert

* Rename and move secretY string function.

* std result

substrate/Cargo.lock

@@ -7089,6 +7089,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"
@@ -7616,6 +7625,7 @@ dependencies = [
  "rand_chacha 0.2.2",
  "regex",
  "schnorrkel",
+ "secrecy",
  "serde",
  "serde_json",
  "sha2",

substrate/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;
 
 /// 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 = secret_string_from_str),
 		conflicts_with_all = &[ "password-interactive", "password-filename" ]
 	)]
-	pub password: Option<String>,
+	pub password: Option<SecretString>,
 
 	/// File that contains the password used by the keystore.
 	#[structopt(
@@ -56,26 +58,37 @@ pub struct KeystoreParams {
 	pub password_filename: Option<PathBuf>,
 }
 
+/// Parse a sercret string, returning a displayable error.
+pub fn secret_string_from_str(s: &str) -> std::result::Result<SecretString, String> {
+	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<KeystoreConfig> {
 		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())
+					.map_err(|()| "Error reading password")?;
+				use sp_core::crypto::Zeroize;
+				password.zeroize();
+				Some(secret)
 			}
 			#[cfg(target_os = "unknown")]
 			None
 		} else if let Some(ref file) = self.password_filename {
-			Some(
+			let mut password = fs::read_to_string(file)
-				fs::read_to_string(file)
+				.map_err(|e| format!("{}", e))?;
-					.map_err(|e| format!("{}", e))?
+			let secret = std::str::FromStr::from_str(password.as_str())
-					.into(),
+				.map_err(|()| "Error reading password")?;
-			)
+			use sp_core::crypto::Zeroize;
-		} else if let Some(ref password) = self.password {
+			password.zeroize();
-			Some(password.clone().into())
+			Some(secret)
 		} else {
-			None
+			self.password.clone()
 		};
 
 		let path = self

substrate/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<PathBuf>,
 	/// Map over `(KeyTypeId, Raw public key)` -> `Key phrase/seed`
 	additional: HashMap<(KeyTypeId, Vec<u8>), String>,
-	password: Option<Protected<String>>,
+	password: Option<SecretString>,
 }
 
 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<T: Into<PathBuf>>(path: T, password: Option<Protected<String>>) -> Result<KeyStorePtr> {
+	pub fn open<T: Into<PathBuf>>(path: T, password: Option<SecretString>) -> Result<KeyStorePtr> {
 		let path = path.into();
 		fs::create_dir_all(&path)?;
 
@@ -155,7 +155,7 @@ impl Store {
 	pub fn insert_by_type<Pair: PairT>(&self, key_type: KeyTypeId, suri: &str) -> Result<Pair> {
 		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<Pair: PairT>(&self, key_type: KeyTypeId) -> Result<Pair> {
-		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<u8>, 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::<ed25519::AppPair>(&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::<ed25519::AppPair>(&pair.public()).unwrap().public(),

substrate/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"))]
@@ -130,7 +130,7 @@ pub enum KeystoreConfig {
 		/// The path of the keystore.
 		path: PathBuf,
 		/// Node keystore's password.
-		password: Option<Protected<String>>
+		password: Option<SecretString>
 	},
 	/// In-memory keystore. Recommended for in-browser nodes.
 	InMemory,

substrate/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-rc4", 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",

substrate/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;
+/// Trait to zeroize a memory buffer.
+pub use zeroize::Zeroize;
+/// Trait for accessing reference to `SecretString`.
+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,51 +85,6 @@ impl<S, T: UncheckedFrom<S>> UncheckedInto<T> for S {
 	}
 }
 
-/// A store for sensitive data.
-///
-/// Calls `Zeroize::zeroize` upon `Drop`.
-#[derive(Clone)]
-pub struct Protected<T: Zeroize>(T);
-
-impl<T: Zeroize> AsRef<T> for Protected<T> {
-	fn as_ref(&self) -> &T {
-		&self.0
-	}
-}
-
-impl<T: Zeroize> sp_std::ops::Deref for Protected<T> {
-	type Target = T;
-
-	fn deref(&self) -> &T {
-		&self.0
-	}
-}
-
-#[cfg(feature = "std")]
-impl<T: Zeroize> std::fmt::Debug for Protected<T> {
-	fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result {
-		write!(fmt, "<protected>")
-	}
-}
-
-impl<T: Zeroize> From<T> for Protected<T> {
-	fn from(t: T) -> Self {
-		Protected(t)
-	}
-}
-
-impl<T: Zeroize> Zeroize for Protected<T> {
-	fn zeroize(&mut self) {
-		self.0.zeroize()
-	}
-}
-
-impl<T: Zeroize> Drop for Protected<T> {
-	fn drop(&mut self) {
-		self.zeroize()
-	}
-}
-
 /// An error with the interpretation of a secret.
 #[derive(Debug, Clone, PartialEq, Eq)]
 #[cfg(feature = "full_crypto")]