Restrict vault.json permssion to owner and using random suffix for temp vault.json file (#8932)

* Move dedup retry logic to a separate function

* Use random suffix temp vault filename and restrict permissions to owner

ethstore/src/accounts_dir/disk.rs

@@ -33,9 +33,33 @@ const IGNORED_FILES: &'static [&'static str] = &[
 	"vault.json",
 ];
 
+/// Find a unique filename that does not exist using four-letter random suffix.
+pub fn find_unique_filename_using_random_suffix(parent_path: &Path, original_filename: &str) -> io::Result<String> {
+	let mut path = parent_path.join(original_filename);
+	let mut deduped_filename = original_filename.to_string();
 
+	if path.exists() {
+		const MAX_RETRIES: usize = 500;
+		let mut retries = 0;
+
+		while path.exists() {
+			if retries >= MAX_RETRIES {
+				return Err(io::Error::new(io::ErrorKind::Other, "Exceeded maximum retries when deduplicating filename."));
+			}
+
+			let suffix = ::random::random_string(4);
+			deduped_filename = format!("{}-{}", original_filename, suffix);
+			path.set_file_name(&deduped_filename);
+			retries += 1;
+		}
+	}
+
+	Ok(deduped_filename)
+}
+
+/// Create a new file and restrict permissions to owner only. It errors if the file already exists.
 #[cfg(unix)]
-fn create_new_file_with_permissions_to_owner(file_path: &Path) -> io::Result<fs::File> {
+pub fn create_new_file_with_permissions_to_owner(file_path: &Path) -> io::Result<fs::File> {
 	use libc;
 	use std::os::unix::fs::OpenOptionsExt;
 
@@ -46,16 +70,18 @@ fn create_new_file_with_permissions_to_owner(file_path: &Path) -> io::Result<fs:
 		.open(file_path)
 }
 
+/// Create a new file and restrict permissions to owner only. It errors if the file already exists.
 #[cfg(not(unix))]
-fn create_new_file_with_permissions_to_owner(file_path: &Path) -> io::Result<fs::File> {
+pub fn create_new_file_with_permissions_to_owner(file_path: &Path) -> io::Result<fs::File> {
 	fs::OpenOptions::new()
 		.write(true)
 		.create_new(true)
 		.open(file_path)
 }
 
+/// Create a new file and restrict permissions to owner only. It replaces the existing file if it already exists.
 #[cfg(unix)]
-fn replace_file_with_permissions_to_owner(file_path: &Path) -> io::Result<fs::File> {
+pub fn replace_file_with_permissions_to_owner(file_path: &Path) -> io::Result<fs::File> {
 	use libc;
 	use std::os::unix::fs::PermissionsExt;
 
@@ -67,8 +93,9 @@ fn replace_file_with_permissions_to_owner(file_path: &Path) -> io::Result<fs::Fi
 	Ok(file)
 }
 
+/// Create a new file and restrict permissions to owner only. It replaces the existing file if it already exists.
 #[cfg(not(unix))]
-fn replace_file_with_permissions_to_owner(file_path: &Path) -> io::Result<fs::File> {
+pub fn replace_file_with_permissions_to_owner(file_path: &Path) -> io::Result<fs::File> {
 	fs::File::create(file_path)
 }
 
@@ -177,29 +204,13 @@ impl<T> DiskDirectory<T> where T: KeyFileManager {
 	/// insert account with given filename. if the filename is a duplicate of any stored account and dedup is set to
 	/// true, a random suffix is appended to the filename.
 	pub fn insert_with_filename(&self, account: SafeAccount, mut filename: String, dedup: bool) -> Result<SafeAccount, Error> {
-		// path to keyfile
+		if dedup {
-		let mut keyfile_path = self.path.join(filename.as_str());
+			filename = find_unique_filename_using_random_suffix(&self.path, &filename)?;
-
-		// check for duplicate filename and append random suffix
-		if dedup && keyfile_path.exists() {
-			const MAX_RETRIES: usize = 500;
-			let mut retries = 0;
-			let mut deduped_filename = filename.clone();
-
-			while keyfile_path.exists() {
-				if retries >= MAX_RETRIES {
-					return Err(Error::Custom(format!("Exceeded maximum retries when deduplicating account filename.")));
-				}
-
-				let suffix = ::random::random_string(4);
-				deduped_filename = format!("{}-{}", filename, suffix);
-				keyfile_path.set_file_name(&deduped_filename);
-				retries += 1;
-			}
-
-			filename = deduped_filename;
 		}
 
+		// path to keyfile
+		let keyfile_path = self.path.join(filename.as_str());
+
 		// update account filename
 		let original_account = account.clone();
 		let mut account = account;

ethstore/src/accounts_dir/vault.rs

@@ -21,7 +21,7 @@ use {json, SafeAccount, Error};
 use crypto::Keccak256;
 use super::super::account::Crypto;
 use super::{KeyDirectory, VaultKeyDirectory, VaultKey, SetKeyError};
-use super::disk::{DiskDirectory, KeyFileManager};
+use super::disk::{self, DiskDirectory, KeyFileManager};
 
 /// Name of vault metadata file
 pub const VAULT_FILE_NAME: &'static str = "vault.json";
@@ -237,14 +237,13 @@ fn create_vault_file<P>(vault_dir_path: P, key: &VaultKey, meta: &str) -> Result
 	let password_hash = key.password.keccak256();
 	let crypto = Crypto::with_plain(&password_hash, &key.password, key.iterations)?;
 
-	let mut vault_file_path: PathBuf = vault_dir_path.as_ref().into();
+	let vault_file_path = vault_dir_path.as_ref().join(VAULT_FILE_NAME);
-	vault_file_path.push(VAULT_FILE_NAME);
+	let temp_vault_file_name = disk::find_unique_filename_using_random_suffix(vault_dir_path.as_ref(), &VAULT_TEMP_FILE_NAME)?;
-	let mut temp_vault_file_path: PathBuf = vault_dir_path.as_ref().into();
+	let temp_vault_file_path = vault_dir_path.as_ref().join(&temp_vault_file_name);
-	temp_vault_file_path.push(VAULT_TEMP_FILE_NAME);
 
 	// this method is used to rewrite existing vault file
 	// => write to temporary file first, then rename temporary file to vault file
-	let mut vault_file = fs::File::create(&temp_vault_file_path)?;
+	let mut vault_file = disk::create_new_file_with_permissions_to_owner(&temp_vault_file_path)?;
 	let vault_file_contents = json::VaultFile {
 		crypto: crypto.into(),
 		meta: Some(meta.to_owned()),