avoid empty wallet seeds (#801)

* fix empty wallet seeds

* mnemonic: return actual error with entropy size on recovering

* seed: extend mnemonic error description

* seed: clarify mnemonic errors

* seed: clarify mnemonic errors, fix missing phrase test

* Clean up seed handling add test

---------

Co-authored-by: ardocrat <ardocrat@gri.mw>
(cherry picked from commit 51ce8e2f2a)

api/src/owner.rs

@@ -1726,8 +1726,10 @@ where
 	/// * `name`: Reserved for future use, use `None` for the time being.
 	/// * `mnemonic`: If present, restore the wallet seed from the given mnemonic instead of creating
 	/// a new random seed.
-	/// * `mnemonic_length`: Desired length of mnemonic in bytes (16 or 32, either 12 or 24 words).
-	/// Use 0 if mnemonic isn't being used.
+	/// * `mnemonic_length`: Entropy length in bytes: 16, 20, 24, 28 or 32
+	/// (12, 15, 18, 21 or 24 words). Ignored when `mnemonic` is provided.
+	/// For a new random seed, 0 is also accepted and defaults to 32 bytes.
+	/// All other values return an error.
 	/// * `password`: The password used to encrypt/decrypt the `wallet.seed` file
 	///
 	/// # Returns

impls/src/error.rs

@@ -75,8 +75,8 @@ pub enum Error {
 	Encryption,
 
 	/// BIP 39 word list
-	#[error("BIP39 Mnemonic (word list) Error")]
-	Mnemonic,
+	#[error("BIP39 Mnemonic (word list) Error: {}", _0)]
+	Mnemonic(String),
 
 	/// Command line argument error
 	#[error("{}", _0)]

impls/src/lifecycle/default.rs

@@ -182,7 +182,7 @@ where
 		&mut self,
 		_name: Option<&str>,
 		mnemonic: Option<ZeroingString>,
-		mnemonic_length: usize,
+		entropy_size: usize,
 		password: ZeroingString,
 		test_mode: bool,
 	) -> Result<(), Error> {
@@ -196,16 +196,15 @@ where
 				return Err(Error::WalletSeedExists(msg));
 			}
 		}
+		let entropy_size = if entropy_size == 0 { 32 } else { entropy_size };
 		WalletSeed::init_file(
 			&data_dir_name,
-			mnemonic_length,
+			entropy_size,
 			mnemonic.clone(),
 			password,
 			test_mode,
 		)
-		.map_err(|_| {
-			Error::Lifecycle("Error creating wallet seed (is mnemonic valid?)".to_owned())
-		})?;
+		.map_err(|e| Error::Lifecycle(format!("Error creating wallet seed: {}", e)))?;
 		info!("Wallet seed file created");
 		let mut wallet: WalletBackend<C, K> =
 			match WalletBackend::new(&data_dir_name, self.node_client.clone()) {
@@ -248,6 +247,9 @@ where
 		let wallet_seed = WalletSeed::from_file(&data_dir_name, password).map_err(|_| {
 			Error::Lifecycle("Error opening wallet (is password correct?)".to_owned())
 		})?;
+		if wallet_seed.is_empty() {
+			warn!("This wallet has an empty seed and its funds are unsafe. Move them to a new wallet.");
+		}
 		let keychain = wallet_seed
 			.derive_keychain(global::is_testnet())
 			.map_err(|_| Error::Lifecycle("Error deriving keychain".to_owned()))?;
@@ -286,7 +288,7 @@ where
 			.map_err(|_| Error::Lifecycle("Error opening wallet seed file".into()))?;
 		let res = wallet_seed
 			.to_mnemonic()
-			.map_err(|_| Error::Lifecycle("Error recovering wallet seed".into()))?;
+			.map_err(|e| Error::Lifecycle(format!("Error recovering wallet seed: {}. This wallet has no valid recovery phrase and its funds may be unsafe. Move them to a new wallet.", e)))?;
 		Ok(ZeroingString::from(res))
 	}
 
@@ -382,3 +384,97 @@ where
 		}
 	}
 }
+
+#[cfg(test)]
+mod tests {
+	use super::*;
+	use crate::keychain::ExtKeychain;
+	use crate::lifecycle::seed::{EncryptedWalletSeed, SEED_FILE};
+	use crate::node_clients::HTTPNodeClient;
+	use std::fs::File;
+	use std::io::Write;
+	use std::time::Duration;
+
+	fn provider(test_dir: &str) -> DefaultLCProvider<HTTPNodeClient, ExtKeychain> {
+		let client =
+			HTTPNodeClient::new("http://127.0.0.1:1", None, Duration::from_secs(1)).unwrap();
+		let mut provider = DefaultLCProvider::new(client);
+		provider.set_top_level_directory(test_dir).unwrap();
+		provider
+	}
+
+	#[test]
+	fn default_recovery_phrase() {
+		let test_dir = "test_output/default_recovery_phrase";
+		let _ = fs::remove_dir_all(test_dir);
+		global::set_local_chain_type(global::ChainTypes::AutomatedTesting);
+		let mut provider = provider(test_dir);
+		let password = ZeroingString::from("test");
+
+		provider
+			.create_wallet(None, None, 0, password.clone(), false)
+			.unwrap();
+		let mnemonic = provider.get_mnemonic(None, password).unwrap();
+
+		assert_eq!(mnemonic.split_whitespace().count(), 24);
+		fs::remove_dir_all(test_dir).unwrap();
+	}
+
+	#[test]
+	fn mnemonic_error_redacted() {
+		let test_dir = "test_output/mnemonic_error_redacted";
+		let _ = fs::remove_dir_all(test_dir);
+		let mut provider = provider(test_dir);
+		let input = ZeroingString::from("INVALID_INPUT_MARKER ".repeat(12));
+		let error = provider
+			.create_wallet(None, Some(input), 32, ZeroingString::from("test"), false)
+			.unwrap_err()
+			.to_string();
+
+		assert!(error.contains("invalid bip39 word"));
+		assert!(!error.contains("INVALID_INPUT_MARKER"));
+		assert!(!provider.wallet_exists(None).unwrap());
+		fs::remove_dir_all(test_dir).unwrap();
+	}
+
+	#[test]
+	fn missing_recovery_phrase() {
+		let test_dir = "test_output/missing_recovery_phrase";
+		let _ = fs::remove_dir_all(test_dir);
+		global::set_local_chain_type(global::ChainTypes::AutomatedTesting);
+		let mut provider = provider(test_dir);
+		let password = ZeroingString::from("test");
+		let data_dir = PathBuf::from(test_dir).join(GRIN_WALLET_DIR);
+		fs::create_dir_all(data_dir.clone()).unwrap();
+
+		let seed_file_path = &format!(
+			"{}{}{}",
+			fmt_path(data_dir.to_str().unwrap().to_string()),
+			MAIN_SEPARATOR,
+			SEED_FILE,
+		);
+
+		let enc_seed = EncryptedWalletSeed::from_seed(
+			&WalletSeed::from_bytes(vec![].as_slice()),
+			password.clone(),
+		)
+		.unwrap();
+		let enc_seed_json = serde_json::to_string_pretty(&enc_seed).unwrap();
+		let mut file = File::create(seed_file_path).unwrap();
+		file.write_all(&enc_seed_json.as_bytes()).unwrap();
+
+		let error = match provider.get_mnemonic(None, password.clone()) {
+			Ok(_) => panic!("expected missing recovery phrase error"),
+			Err(error) => error,
+		};
+
+		assert_eq!(
+			error,
+			Error::Lifecycle("Error recovering wallet seed: BIP39 Mnemonic (word list) Error: invalid mnemonic/entropy length 0. This wallet has no valid recovery phrase and its funds may be unsafe. Move them to a new wallet.".into())
+		);
+		provider.open_wallet(None, password, false, false).unwrap();
+		provider.close_wallet(None).unwrap();
+		assert_eq!(fs::read_to_string(seed_file_path).unwrap(), enc_seed_json);
+		fs::remove_dir_all(test_dir).unwrap();
+	}
+}

impls/src/lifecycle/seed.rs

@@ -39,11 +39,18 @@ impl WalletSeed {
 		WalletSeed(bytes.to_vec())
 	}
 
+	pub(super) fn is_empty(&self) -> bool {
+		self.0.is_empty()
+	}
+
 	pub fn from_mnemonic(word_list: util::ZeroingString) -> Result<WalletSeed, Error> {
 		let res = mnemonic::to_entropy(&word_list);
 		match res {
 			Ok(s) => Ok(WalletSeed::from_bytes(&s)),
-			Err(_) => Err(Error::Mnemonic.into()),
+			Err(mnemonic::Error::BadWord(_)) => {
+				Err(Error::Mnemonic("invalid bip39 word".to_owned()))
+			}
+			Err(e) => Err(Error::Mnemonic(format!("{}", e))),
 		}
 	}
 
@@ -61,7 +68,7 @@ impl WalletSeed {
 		let result = mnemonic::from_entropy(&self.0);
 		match result {
 			Ok(r) => Ok(r),
-			Err(_) => Err(Error::Mnemonic.into()),
+			Err(e) => Err(Error::Mnemonic(format!("{}", e))),
 		}
 	}
 
@@ -76,14 +83,14 @@ impl WalletSeed {
 	}
 
 	pub fn init_new(
-		seed_length: usize,
+		entropy_size: usize,
 		test_mode: bool,
 		password: Option<util::ZeroingString>,
-	) -> WalletSeed {
+	) -> Result<WalletSeed, Error> {
 		let mut seed: Vec<u8> = vec![];
 		let mut rng = thread_rng();
 		if !test_mode {
-			for _ in 0..seed_length {
+			for _ in 0..entropy_size {
 				seed.push(rng.gen());
 			}
 		} else {
@@ -93,7 +100,10 @@ impl WalletSeed {
 				.as_bytes()
 				.to_vec();
 		}
-		WalletSeed(seed)
+		let _ = mnemonic::from_entropy(seed.as_slice())
+			.map(util::ZeroingString::from)
+			.map_err(|e| Error::Mnemonic(format!("{}", e)))?;
+		Ok(WalletSeed(seed))
 	}
 
 	pub fn seed_file_exists(data_file_dir: &str) -> Result<bool, Error> {
@@ -168,7 +178,7 @@ impl WalletSeed {
 
 	pub fn init_file(
 		data_file_dir: &str,
-		seed_length: usize,
+		entropy_size: usize,
 		recovery_phrase: Option<util::ZeroingString>,
 		password: util::ZeroingString,
 		test_mode: bool,
@@ -193,7 +203,7 @@ impl WalletSeed {
 
 		let seed = match recovery_phrase {
 			Some(p) => WalletSeed::from_mnemonic(p)?,
-			None => WalletSeed::init_new(seed_length, test_mode, Some(password.clone())),
+			None => WalletSeed::init_new(entropy_size, test_mode, Some(password.clone()))?,
 		};
 
 		let enc_seed = EncryptedWalletSeed::from_seed(&seed, password)?;
@@ -277,7 +287,7 @@ impl EncryptedWalletSeed {
 		let password = password.as_bytes();
 		let mut key = [0; 32];
 		pbkdf2::derive(
-			ring::pbkdf2::PBKDF2_HMAC_SHA512,
+			pbkdf2::PBKDF2_HMAC_SHA512,
 			NonZeroU32::new(100).unwrap(),
 			&salt,
 			password,
@@ -325,7 +335,7 @@ impl EncryptedWalletSeed {
 		let password = password.as_bytes();
 		let mut key = [0; 32];
 		pbkdf2::derive(
-			ring::pbkdf2::PBKDF2_HMAC_SHA512,
+			pbkdf2::PBKDF2_HMAC_SHA512,
 			NonZeroU32::new(100).unwrap(),
 			&salt,
 			password,
@@ -359,8 +369,12 @@ mod tests {
 	use crate::util::ZeroingString;
 	#[test]
 	fn wallet_seed_encrypt() {
+		// Wrong entropy size.
+		let wrong_entropy_size = WalletSeed::init_new(31, false, None).err();
+		assert!(wrong_entropy_size.is_some());
+
 		let password = ZeroingString::from("passwoid");
-		let wallet_seed = WalletSeed::init_new(32, false, None);
+		let wallet_seed = WalletSeed::init_new(32, false, None).unwrap();
 		let mut enc_wallet_seed =
 			EncryptedWalletSeed::from_seed(&wallet_seed, password.clone()).unwrap();
 		println!("EWS: {:?}", enc_wallet_seed);

libwallet/src/types.rs

@@ -81,7 +81,7 @@ where
 		&mut self,
 		name: Option<&str>,
 		mnemonic: Option<ZeroingString>,
-		mnemonic_length: usize,
+		entropy_size: usize,
 		password: ZeroingString,
 		test_mode: bool,
 	) -> Result<(), Error>;