From 125f6dd81077dd538350a30b890fd34c4693e64e Mon Sep 17 00:00:00 2001 From: rustdesk Date: Sat, 30 Jul 2022 02:01:40 +0800 Subject: [PATCH] refactor password deadlock and password_security --- libs/hbb_common/src/config.rs | 65 ++-- libs/hbb_common/src/lib.rs | 27 +- libs/hbb_common/src/password_security.rs | 386 +++++++++++------------ src/ipc.rs | 4 +- src/main.rs | 10 +- src/mobile_ffi.rs | 11 +- src/server/connection.rs | 5 +- 7 files changed, 225 insertions(+), 283 deletions(-) diff --git a/libs/hbb_common/src/config.rs b/libs/hbb_common/src/config.rs index 8f15c4b8f..4af3cd9f9 100644 --- a/libs/hbb_common/src/config.rs +++ b/libs/hbb_common/src/config.rs @@ -1,6 +1,6 @@ use crate::{ log, - password_security::config::{ + password_security::{ decrypt_str_or_original, decrypt_vec_or_original, encrypt_str_or_original, encrypt_vec_or_original, }, @@ -46,6 +46,7 @@ lazy_static::lazy_static! { pub static ref ONLINE: Arc>> = Default::default(); pub static ref PROD_RENDEZVOUS_SERVER: Arc> = Default::default(); pub static ref APP_NAME: Arc> = Arc::new(RwLock::new("RustDesk".to_owned())); + static ref KEY_PAIR: Arc, Vec)>>> = Default::default(); } #[cfg(target_os = "android")] lazy_static::lazy_static! { @@ -229,13 +230,6 @@ impl Config2 { config } - pub fn decrypt_password(&mut self) { - if let Some(mut socks) = self.socks.clone() { - socks.password = decrypt_str_or_original(&socks.password, PASSWORD_ENC_VERSION).0; - self.socks = Some(socks); - } - } - pub fn file() -> PathBuf { Config::file_("2") } @@ -277,6 +271,11 @@ pub fn load_path(path: PathBuf, cfg: T) -> crate::ResultType<()> { + Ok(confy::store_path(path, cfg)?) +} + impl Config { fn load_( suffix: &str, @@ -292,7 +291,7 @@ impl Config { fn store_(config: &T, suffix: &str) { let file = Self::file_(suffix); - if let Err(err) = confy::store_path(file, config) { + if let Err(err) = store_path(file, config) { log::error!("Failed to store config: {}", err); } } @@ -307,10 +306,6 @@ impl Config { config } - pub fn decrypt_password(&mut self) { - self.password = decrypt_str_or_original(&self.password, PASSWORD_ENC_VERSION).0; - } - fn store(&self) { let mut config = self.clone(); config.password = encrypt_str_or_original(&config.password, PASSWORD_ENC_VERSION); @@ -587,36 +582,26 @@ impl Config { config.store(); } - pub fn set_key_pair(pair: (Vec, Vec)) { - let mut config = CONFIG.write().unwrap(); - if config.key_pair == pair { - return; - } - config.key_pair = pair; - config.store(); - } - - // * Manually make sure no gen_keypair more than once - // for uuid, avoid deadlock - pub fn get_key_pair_without_lock() -> (Vec, Vec) { - let mut config = Config::load_::(""); - if config.key_pair.0.is_empty() { - let (pk, sk) = sign::gen_keypair(); - config.key_pair = (sk.0.to_vec(), pk.0.into()); - Config::store_(&config, ""); - } - config.key_pair.clone() - } - pub fn get_key_pair() -> (Vec, Vec) { // lock here to make sure no gen_keypair more than once - let mut config = CONFIG.write().unwrap(); + // no use of CONFIG directly here to ensure no recursive calling in Config::load because of password dec which calling this function + let mut lock = KEY_PAIR.lock().unwrap(); + if let Some(p) = lock.as_ref() { + return p.clone(); + } + let mut config = Config::load(); if config.key_pair.0.is_empty() { let (pk, sk) = sign::gen_keypair(); - config.key_pair = (sk.0.to_vec(), pk.0.into()); - config.store(); + let key_pair = (sk.0.to_vec(), pk.0.into()); + config.key_pair = key_pair.clone(); + std::thread::spawn(|| { + let mut config = CONFIG.write().unwrap(); + config.key_pair = key_pair; + config.store(); + }); } - config.key_pair.clone() + *lock = Some(config.key_pair.clone()); + return config.key_pair; } pub fn get_id() -> String { @@ -805,7 +790,7 @@ impl PeerConfig { .options .get_mut("os-password") .map(|v| *v = encrypt_str_or_original(v, PASSWORD_ENC_VERSION)); - if let Err(err) = confy::store_path(Self::path(id), config) { + if let Err(err) = store_path(Self::path(id), config) { log::error!("Failed to store config: {}", err); } } @@ -975,7 +960,7 @@ impl LanPeers { let f = LanPeers { peers: peers.clone(), }; - if let Err(err) = confy::store_path(Config::file_("_lan_peers"), f) { + if let Err(err) = store_path(Config::file_("_lan_peers"), f) { log::error!("Failed to store lan peers: {}", err); } } diff --git a/libs/hbb_common/src/lib.rs b/libs/hbb_common/src/lib.rs index a9f25d31b..2fdd74cd5 100644 --- a/libs/hbb_common/src/lib.rs +++ b/libs/hbb_common/src/lib.rs @@ -1,12 +1,12 @@ pub mod compress; -pub mod protos; pub mod platform; -pub use protos::message as message_proto; -pub use protos::rendezvous as rendezvous_proto; +pub mod protos; pub use bytes; use config::Config; pub use futures; pub use protobuf; +pub use protos::message as message_proto; +pub use protos::rendezvous as rendezvous_proto; use std::{ fs::File, io::{self, BufRead}, @@ -39,10 +39,6 @@ pub use tokio_socks::IntoTargetAddr; pub use tokio_socks::TargetAddr; pub mod password_security; -lazy_static::lazy_static!{ - static ref UUID: Vec = gen_uuid(); -} - #[cfg(feature = "quic")] pub type Stream = quic::Connection; #[cfg(not(feature = "quic"))] @@ -206,23 +202,12 @@ pub fn get_modified_time(path: &std::path::Path) -> SystemTime { .unwrap_or(UNIX_EPOCH) } -fn gen_uuid() -> Vec { +pub fn get_uuid() -> Vec { #[cfg(not(any(target_os = "android", target_os = "ios")))] if let Ok(id) = machine_uid::get() { - id.into() - } else { - Config::get_key_pair().1 + return id.into(); } - #[cfg(any(target_os = "android", target_os = "ios"))] - Config::get_key_pair_without_lock().1 -} - -pub fn init_uuid() { - let _ = *UUID; -} - -pub fn get_uuid() -> Vec { - UUID.to_owned() + Config::get_key_pair().1 } #[cfg(test)] diff --git a/libs/hbb_common/src/password_security.rs b/libs/hbb_common/src/password_security.rs index 413aa7c7f..ba57c11c4 100644 --- a/libs/hbb_common/src/password_security.rs +++ b/libs/hbb_common/src/password_security.rs @@ -1,222 +1,143 @@ -pub mod password { - use crate::config::Config; - use std::sync::{Arc, RwLock}; +use crate::config::Config; +use sodiumoxide::base64; +use std::sync::{Arc, RwLock}; - lazy_static::lazy_static! { - pub static ref TEMPORARY_PASSWORD:Arc> = Arc::new(RwLock::new(Config::get_auto_password(temporary_password_length()))); - } +lazy_static::lazy_static! { + pub static ref TEMPORARY_PASSWORD:Arc> = Arc::new(RwLock::new(Config::get_auto_password(temporary_password_length()))); +} - #[derive(Debug, Clone, Copy, PartialEq, Eq)] - enum VerificationMethod { - OnlyUseTemporaryPassword, - OnlyUsePermanentPassword, - UseBothPasswords, - } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum VerificationMethod { + OnlyUseTemporaryPassword, + OnlyUsePermanentPassword, + UseBothPasswords, +} - // Should only be called in server - pub fn update_temporary_password() { - *TEMPORARY_PASSWORD.write().unwrap() = - Config::get_auto_password(temporary_password_length()); - } +// Should only be called in server +pub fn update_temporary_password() { + *TEMPORARY_PASSWORD.write().unwrap() = Config::get_auto_password(temporary_password_length()); +} - // Should only be called in server - pub fn temporary_password() -> String { - TEMPORARY_PASSWORD.read().unwrap().clone() - } +// Should only be called in server +pub fn temporary_password() -> String { + TEMPORARY_PASSWORD.read().unwrap().clone() +} - fn verification_method() -> VerificationMethod { - let method = Config::get_option("verification-method"); - if method == "use-temporary-password" { - VerificationMethod::OnlyUseTemporaryPassword - } else if method == "use-permanent-password" { - VerificationMethod::OnlyUsePermanentPassword - } else { - VerificationMethod::UseBothPasswords // default - } - } - - pub fn temporary_password_length() -> usize { - let length = Config::get_option("temporary-password-length"); - if length == "8" { - 8 - } else if length == "10" { - 10 - } else { - 6 // default - } - } - - pub fn temporary_enabled() -> bool { - verification_method() != VerificationMethod::OnlyUsePermanentPassword - } - - pub fn permanent_enabled() -> bool { - verification_method() != VerificationMethod::OnlyUseTemporaryPassword - } - - pub fn has_valid_password() -> bool { - temporary_enabled() && !temporary_password().is_empty() - || permanent_enabled() && !Config::get_permanent_password().is_empty() +fn verification_method() -> VerificationMethod { + let method = Config::get_option("verification-method"); + if method == "use-temporary-password" { + VerificationMethod::OnlyUseTemporaryPassword + } else if method == "use-permanent-password" { + VerificationMethod::OnlyUsePermanentPassword + } else { + VerificationMethod::UseBothPasswords // default } } -pub mod config { - use super::base64::decrypt as decrypt00; - use super::base64::encrypt as encrypt00; - - const VERSION_LEN: usize = 2; - - pub fn encrypt_str_or_original(s: &str, version: &str) -> String { - if decrypt_str_or_original(s, version).1 { - log::error!("Duplicate encryption!"); - return s.to_owned(); - } - if version.len() == VERSION_LEN { - if version == "00" { - if let Ok(s) = encrypt00(s.as_bytes()) { - return version.to_owned() + &s; - } - } - } - - s.to_owned() - } - - // String: password - // bool: whether decryption is successful - // bool: whether should store to re-encrypt when load - pub fn decrypt_str_or_original(s: &str, current_version: &str) -> (String, bool, bool) { - if s.len() > VERSION_LEN { - let version = &s[..VERSION_LEN]; - if version == "00" { - if let Ok(v) = decrypt00(&s[VERSION_LEN..].as_bytes()) { - return ( - String::from_utf8_lossy(&v).to_string(), - true, - version != current_version, - ); - } - } - } - - (s.to_owned(), false, !s.is_empty()) - } - - pub fn encrypt_vec_or_original(v: &[u8], version: &str) -> Vec { - if decrypt_vec_or_original(v, version).1 { - log::error!("Duplicate encryption!"); - return v.to_owned(); - } - if version.len() == VERSION_LEN { - if version == "00" { - if let Ok(s) = encrypt00(v) { - let mut version = version.to_owned().into_bytes(); - version.append(&mut s.into_bytes()); - return version; - } - } - } - - v.to_owned() - } - - // String: password - // bool: whether decryption is successful - // bool: whether should store to re-encrypt when load - pub fn decrypt_vec_or_original(v: &[u8], current_version: &str) -> (Vec, bool, bool) { - if v.len() > VERSION_LEN { - let version = String::from_utf8_lossy(&v[..VERSION_LEN]); - if version == "00" { - if let Ok(v) = decrypt00(&v[VERSION_LEN..]) { - return (v, true, version != current_version); - } - } - } - - (v.to_owned(), false, !v.is_empty()) - } - - mod test { - - #[test] - fn test() { - use crate::password_security::config::*; - - let version = "00"; - - println!("test str"); - let data = "Hello World"; - let encrypted = encrypt_str_or_original(data, version); - let (decrypted, succ, store) = decrypt_str_or_original(&encrypted, version); - println!("data: {}", data); - println!("encrypted: {}", encrypted); - println!("decrypted: {}", decrypted); - assert_eq!(data, decrypted); - assert_eq!(version, &encrypted[..2]); - assert_eq!(succ, true); - assert_eq!(store, false); - let (_, _, store) = decrypt_str_or_original(&encrypted, "99"); - assert_eq!(store, true); - assert_eq!(decrypt_str_or_original(&decrypted, version).1, false); - assert_eq!(encrypt_str_or_original(&encrypted, version), encrypted); - - println!("test vec"); - let data: Vec = vec![1, 2, 3, 4, 5, 6]; - let encrypted = encrypt_vec_or_original(&data, version); - let (decrypted, succ, store) = decrypt_vec_or_original(&encrypted, version); - println!("data: {:?}", data); - println!("encrypted: {:?}", encrypted); - println!("decrypted: {:?}", decrypted); - assert_eq!(data, decrypted); - assert_eq!(version.as_bytes(), &encrypted[..2]); - assert_eq!(store, false); - assert_eq!(succ, true); - let (_, _, store) = decrypt_vec_or_original(&encrypted, "99"); - assert_eq!(store, true); - assert_eq!(decrypt_vec_or_original(&decrypted, version).1, false); - assert_eq!(encrypt_vec_or_original(&encrypted, version), encrypted); - - println!("test original"); - let data = version.to_string() + "Hello World"; - let (decrypted, succ, store) = decrypt_str_or_original(&data, version); - assert_eq!(data, decrypted); - assert_eq!(store, true); - assert_eq!(succ, false); - let verbytes = version.as_bytes(); - let data: Vec = vec![verbytes[0] as u8, verbytes[1] as u8, 1, 2, 3, 4, 5, 6]; - let (decrypted, succ, store) = decrypt_vec_or_original(&data, version); - assert_eq!(data, decrypted); - assert_eq!(store, true); - assert_eq!(succ, false); - let (_, succ, store) = decrypt_str_or_original("", version); - assert_eq!(store, false); - assert_eq!(succ, false); - let (_, succ, store) = decrypt_vec_or_original(&vec![], version); - assert_eq!(store, false); - assert_eq!(succ, false); - } +pub fn temporary_password_length() -> usize { + let length = Config::get_option("temporary-password-length"); + if length == "8" { + 8 + } else if length == "10" { + 10 + } else { + 6 // default } } -mod base64 { - use super::symmetric_crypt; - use sodiumoxide::base64; +pub fn temporary_enabled() -> bool { + verification_method() != VerificationMethod::OnlyUsePermanentPassword +} - pub fn encrypt(v: &[u8]) -> Result { - if v.len() > 0 { - symmetric_crypt(v, true).map(|v| base64::encode(v, base64::Variant::Original)) - } else { - Err(()) +pub fn permanent_enabled() -> bool { + verification_method() != VerificationMethod::OnlyUseTemporaryPassword +} + +pub fn has_valid_password() -> bool { + temporary_enabled() && !temporary_password().is_empty() + || permanent_enabled() && !Config::get_permanent_password().is_empty() +} + +const VERSION_LEN: usize = 2; + +pub fn encrypt_str_or_original(s: &str, version: &str) -> String { + if decrypt_str_or_original(s, version).1 { + log::error!("Duplicate encryption!"); + return s.to_owned(); + } + if version == "00" { + if let Ok(s) = encrypt(s.as_bytes()) { + return version.to_owned() + &s; + } + } + s.to_owned() +} + +// String: password +// bool: whether decryption is successful +// bool: whether should store to re-encrypt when load +pub fn decrypt_str_or_original(s: &str, current_version: &str) -> (String, bool, bool) { + if s.len() > VERSION_LEN { + let version = &s[..VERSION_LEN]; + if version == "00" { + if let Ok(v) = decrypt(&s[VERSION_LEN..].as_bytes()) { + return ( + String::from_utf8_lossy(&v).to_string(), + true, + version != current_version, + ); + } } } - pub fn decrypt(v: &[u8]) -> Result, ()> { - if v.len() > 0 { - base64::decode(v, base64::Variant::Original).and_then(|v| symmetric_crypt(&v, false)) - } else { - Err(()) + (s.to_owned(), false, !s.is_empty()) +} + +pub fn encrypt_vec_or_original(v: &[u8], version: &str) -> Vec { + if decrypt_vec_or_original(v, version).1 { + log::error!("Duplicate encryption!"); + return v.to_owned(); + } + if version == "00" { + if let Ok(s) = encrypt(v) { + let mut version = version.to_owned().into_bytes(); + version.append(&mut s.into_bytes()); + return version; } } + v.to_owned() +} + +// String: password +// bool: whether decryption is successful +// bool: whether should store to re-encrypt when load +pub fn decrypt_vec_or_original(v: &[u8], current_version: &str) -> (Vec, bool, bool) { + if v.len() > VERSION_LEN { + let version = String::from_utf8_lossy(&v[..VERSION_LEN]); + if version == "00" { + if let Ok(v) = decrypt(&v[VERSION_LEN..]) { + return (v, true, version != current_version); + } + } + } + + (v.to_owned(), false, !v.is_empty()) +} + +fn encrypt(v: &[u8]) -> Result { + if v.len() > 0 { + symmetric_crypt(v, true).map(|v| base64::encode(v, base64::Variant::Original)) + } else { + Err(()) + } +} + +fn decrypt(v: &[u8]) -> Result, ()> { + if v.len() > 0 { + base64::decode(v, base64::Variant::Original).and_then(|v| symmetric_crypt(&v, false)) + } else { + Err(()) + } } fn symmetric_crypt(data: &[u8], encrypt: bool) -> Result, ()> { @@ -234,3 +155,64 @@ fn symmetric_crypt(data: &[u8], encrypt: bool) -> Result, ()> { secretbox::open(data, &nonce, &key) } } + +mod test { + + #[test] + fn test() { + use super::*; + + let version = "00"; + + println!("test str"); + let data = "Hello World"; + let encrypted = encrypt_str_or_original(data, version); + let (decrypted, succ, store) = decrypt_str_or_original(&encrypted, version); + println!("data: {}", data); + println!("encrypted: {}", encrypted); + println!("decrypted: {}", decrypted); + assert_eq!(data, decrypted); + assert_eq!(version, &encrypted[..2]); + assert_eq!(succ, true); + assert_eq!(store, false); + let (_, _, store) = decrypt_str_or_original(&encrypted, "99"); + assert_eq!(store, true); + assert_eq!(decrypt_str_or_original(&decrypted, version).1, false); + assert_eq!(encrypt_str_or_original(&encrypted, version), encrypted); + + println!("test vec"); + let data: Vec = vec![1, 2, 3, 4, 5, 6]; + let encrypted = encrypt_vec_or_original(&data, version); + let (decrypted, succ, store) = decrypt_vec_or_original(&encrypted, version); + println!("data: {:?}", data); + println!("encrypted: {:?}", encrypted); + println!("decrypted: {:?}", decrypted); + assert_eq!(data, decrypted); + assert_eq!(version.as_bytes(), &encrypted[..2]); + assert_eq!(store, false); + assert_eq!(succ, true); + let (_, _, store) = decrypt_vec_or_original(&encrypted, "99"); + assert_eq!(store, true); + assert_eq!(decrypt_vec_or_original(&decrypted, version).1, false); + assert_eq!(encrypt_vec_or_original(&encrypted, version), encrypted); + + println!("test original"); + let data = version.to_string() + "Hello World"; + let (decrypted, succ, store) = decrypt_str_or_original(&data, version); + assert_eq!(data, decrypted); + assert_eq!(store, true); + assert_eq!(succ, false); + let verbytes = version.as_bytes(); + let data: Vec = vec![verbytes[0] as u8, verbytes[1] as u8, 1, 2, 3, 4, 5, 6]; + let (decrypted, succ, store) = decrypt_vec_or_original(&data, version); + assert_eq!(data, decrypted); + assert_eq!(store, true); + assert_eq!(succ, false); + let (_, succ, store) = decrypt_str_or_original("", version); + assert_eq!(store, false); + assert_eq!(succ, false); + let (_, succ, store) = decrypt_vec_or_original(&vec![], version); + assert_eq!(store, false); + assert_eq!(succ, false); + } +} diff --git a/src/ipc.rs b/src/ipc.rs index 2324f691c..7ed6f4fd5 100644 --- a/src/ipc.rs +++ b/src/ipc.rs @@ -8,9 +8,7 @@ use hbb_common::{ config::{self, Config, Config2}, futures::StreamExt as _, futures_util::sink::SinkExt, - log, - password_security::password, - timeout, tokio, + log, password_security as password, timeout, tokio, tokio::io::{AsyncRead, AsyncWrite}, tokio_util::codec::Framed, ResultType, diff --git a/src/main.rs b/src/main.rs index 8a6d09e57..06d67ceee 100644 --- a/src/main.rs +++ b/src/main.rs @@ -171,21 +171,19 @@ fn import_config(path: &str) { let path2 = std::path::Path::new(&path2); let path = std::path::Path::new(path); log::info!("import config from {:?} and {:?}", path, path2); - let mut config: Config = load_path(path.into()); + let config: Config = load_path(path.into()); if config.id.is_empty() || config.key_pair.0.is_empty() { log::info!("Empty source config, skipped"); return; } if get_modified_time(&path) > get_modified_time(&Config::file()) { - config.decrypt_password(); - if Config::set(config) { + if store_path(Config::file(), config).is_err() { log::info!("config written"); } } - let mut config2: Config2 = load_path(path2.into()); + let config2: Config2 = load_path(path2.into()); if get_modified_time(&path2) > get_modified_time(&Config2::file()) { - config2.decrypt_password(); - if Config2::set(config2) { + if store_path(Config2::file(), config2).is_err() { log::info!("config2 written"); } } diff --git a/src/mobile_ffi.rs b/src/mobile_ffi.rs index 2b456c24f..c3ed39764 100644 --- a/src/mobile_ffi.rs +++ b/src/mobile_ffi.rs @@ -1,13 +1,11 @@ use crate::client::file_trait::FileManager; +use crate::common::make_fd_to_json; use crate::mobile::connection_manager::{self, get_clients_length, get_clients_state}; use crate::mobile::{self, Session}; -use crate::common::{make_fd_to_json}; use flutter_rust_bridge::{StreamSink, ZeroCopyBuffer}; -use hbb_common::{ResultType, init_uuid}; -use hbb_common::password_security::password; use hbb_common::{ config::{self, Config, LocalConfig, PeerConfig, ONLINE}, - fs, log, + fs, log, password_security as password, ResultType, }; use serde_json::{Number, Value}; use std::{ @@ -31,7 +29,6 @@ fn initialize(app_dir: &str) { init_from_env(Env::default().filter_or(DEFAULT_FILTER_ENV, "debug")); } *config::APP_DIR.write().unwrap() = app_dir.to_owned(); - init_uuid(); crate::common::test_rendezvous_server(); crate::common::test_nat_type(); #[cfg(target_os = "android")] @@ -462,9 +459,7 @@ unsafe extern "C" fn set_by_name(name: *const c_char, value: *const c_char) { } } // Server Side - "permanent_password" => { - Config::set_permanent_password(value) - } + "permanent_password" => Config::set_permanent_password(value), "temporary_password" => { password::update_temporary_password(); } diff --git a/src/server/connection.rs b/src/server/connection.rs index 53903ee2d..880d5bbac 100644 --- a/src/server/connection.rs +++ b/src/server/connection.rs @@ -7,15 +7,14 @@ use crate::video_service; #[cfg(any(target_os = "android", target_os = "ios"))] use crate::{common::MOBILE_INFO2, mobile::connection_manager::start_channel}; use crate::{ipc, VERSION}; -use hbb_common::fs::can_enable_overwrite_detection; -use hbb_common::password_security::password; use hbb_common::{ config::Config, fs, + fs::can_enable_overwrite_detection, futures::{SinkExt, StreamExt}, get_version_number, message_proto::{option_message::BoolOption, permission_info::Permission}, - sleep, timeout, + password_security as password, sleep, timeout, tokio::{ net::TcpStream, sync::mpsc,