From 5ae8e8a9caf523326d83ac24d2547e41e5e55f62 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Fri, 22 Jun 2018 19:30:48 +0800 Subject: [PATCH] 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 | 61 ++++++++++++++++++------------ ethstore/src/accounts_dir/vault.rs | 11 +++--- 2 files changed, 41 insertions(+), 31 deletions(-) diff --git a/ethstore/src/accounts_dir/disk.rs b/ethstore/src/accounts_dir/disk.rs index 6917a6278..220f58d54 100644 --- a/ethstore/src/accounts_dir/disk.rs +++ b/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 { + 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 { +pub fn create_new_file_with_permissions_to_owner(file_path: &Path) -> io::Result { 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 io::Result { +pub fn create_new_file_with_permissions_to_owner(file_path: &Path) -> io::Result { 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 { +pub fn replace_file_with_permissions_to_owner(file_path: &Path) -> io::Result { use libc; use std::os::unix::fs::PermissionsExt; @@ -67,8 +93,9 @@ fn replace_file_with_permissions_to_owner(file_path: &Path) -> io::Result io::Result { +pub fn replace_file_with_permissions_to_owner(file_path: &Path) -> io::Result { fs::File::create(file_path) } @@ -177,29 +204,13 @@ impl DiskDirectory 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 { - // path to keyfile - let mut keyfile_path = self.path.join(filename.as_str()); - - // 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; + if dedup { + filename = find_unique_filename_using_random_suffix(&self.path, &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; diff --git a/ethstore/src/accounts_dir/vault.rs b/ethstore/src/accounts_dir/vault.rs index b2f6ce616..631376dc9 100644 --- a/ethstore/src/accounts_dir/vault.rs +++ b/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

(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(); - vault_file_path.push(VAULT_FILE_NAME); - let mut temp_vault_file_path: PathBuf = vault_dir_path.as_ref().into(); - temp_vault_file_path.push(VAULT_TEMP_FILE_NAME); + let vault_file_path = vault_dir_path.as_ref().join(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 temp_vault_file_path = vault_dir_path.as_ref().join(&temp_vault_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()),