refactor password deadlock and password_security

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<Mutex<HashMap<String, i64>>> = Default::default();
     pub static ref PROD_RENDEZVOUS_SERVER: Arc<RwLock<String>> = Default::default();
     pub static ref APP_NAME: Arc<RwLock<String>> = Arc::new(RwLock::new("RustDesk".to_owned()));
+    static ref KEY_PAIR: Arc<Mutex<Option<(Vec<u8>, Vec<u8>)>>> = 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<T: serde::Serialize + serde::de::DeserializeOwned + Default + s
     cfg
 }
 
+#[inline]
+pub fn store_path<T: serde::Serialize>(path: PathBuf, cfg: T) -> crate::ResultType<()> {
+    Ok(confy::store_path(path, cfg)?)
+}
+
 impl Config {
     fn load_<T: serde::Serialize + serde::de::DeserializeOwned + Default + std::fmt::Debug>(
         suffix: &str,
@@ -292,7 +291,7 @@ impl Config {
 
     fn store_<T: serde::Serialize>(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<u8>, Vec<u8>)) {
-        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<u8>, Vec<u8>) {
-        let mut config = Config::load_::<Config>("");
-        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<u8>, Vec<u8>) {
         // 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());
+            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);
         }
     }

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 mod protos;
-pub use  protos::rendezvous as rendezvous_proto;
 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<u8> = gen_uuid();
-}
-
 #[cfg(feature = "quic")]
 pub type Stream = quic::Connection;
 #[cfg(not(feature = "quic"))]
@@ -206,24 +202,13 @@ pub fn get_modified_time(path: &std::path::Path) -> SystemTime {
         .unwrap_or(UNIX_EPOCH)
 }
 
-fn gen_uuid() -> Vec<u8> {
+pub fn get_uuid() -> Vec<u8> {
     #[cfg(not(any(target_os = "android", target_os = "ios")))]
     if let Ok(id) = machine_uid::get() {
-        id.into()
+        return id.into();
-    } else {
+    }
     Config::get_key_pair().1
 }
-    #[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<u8> {
-    UUID.to_owned()
-}
 
 #[cfg(test)]
 mod tests {

libs/hbb_common/src/password_security.rs

@@ -1,5 +1,5 @@
-pub mod password {
 use crate::config::Config;
+use sodiumoxide::base64;
 use std::sync::{Arc, RwLock};
 
 lazy_static::lazy_static! {
@@ -15,8 +15,7 @@ pub mod password {
 
 // Should only be called in server
 pub fn update_temporary_password() {
-        *TEMPORARY_PASSWORD.write().unwrap() =
+    *TEMPORARY_PASSWORD.write().unwrap() = Config::get_auto_password(temporary_password_length());
-            Config::get_auto_password(temporary_password_length());
 }
 
 // Should only be called in server
@@ -58,11 +57,6 @@ pub mod password {
     temporary_enabled() && !temporary_password().is_empty()
         || permanent_enabled() && !Config::get_permanent_password().is_empty()
 }
-}
-
-pub mod config {
-    use super::base64::decrypt as decrypt00;
-    use super::base64::encrypt as encrypt00;
 
 const VERSION_LEN: usize = 2;
 
@@ -71,14 +65,11 @@ pub mod config {
         log::error!("Duplicate encryption!");
         return s.to_owned();
     }
-        if version.len() == VERSION_LEN {
     if version == "00" {
-                if let Ok(s) = encrypt00(s.as_bytes()) {
+        if let Ok(s) = encrypt(s.as_bytes()) {
             return version.to_owned() + &s;
         }
     }
-        }
-
     s.to_owned()
 }
 
@@ -89,7 +80,7 @@ pub mod config {
     if s.len() > VERSION_LEN {
         let version = &s[..VERSION_LEN];
         if version == "00" {
-                if let Ok(v) = decrypt00(&s[VERSION_LEN..].as_bytes()) {
+            if let Ok(v) = decrypt(&s[VERSION_LEN..].as_bytes()) {
                 return (
                     String::from_utf8_lossy(&v).to_string(),
                     true,
@@ -107,16 +98,13 @@ pub mod config {
         log::error!("Duplicate encryption!");
         return v.to_owned();
     }
-        if version.len() == VERSION_LEN {
     if version == "00" {
-                if let Ok(s) = encrypt00(v) {
+        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()
 }
 
@@ -127,7 +115,7 @@ pub mod config {
     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..]) {
+            if let Ok(v) = decrypt(&v[VERSION_LEN..]) {
                 return (v, true, version != current_version);
             }
         }
@@ -136,11 +124,43 @@ pub mod config {
     (v.to_owned(), false, !v.is_empty())
 }
 
+fn encrypt(v: &[u8]) -> Result<String, ()> {
+    if v.len() > 0 {
+        symmetric_crypt(v, true).map(|v| base64::encode(v, base64::Variant::Original))
+    } else {
+        Err(())
+    }
+}
+
+fn decrypt(v: &[u8]) -> Result<Vec<u8>, ()> {
+    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<Vec<u8>, ()> {
+    use sodiumoxide::crypto::secretbox;
+    use std::convert::TryInto;
+
+    let mut keybuf = crate::get_uuid();
+    keybuf.resize(secretbox::KEYBYTES, 0);
+    let key = secretbox::Key(keybuf.try_into().map_err(|_| ())?);
+    let nonce = secretbox::Nonce([0; secretbox::NONCEBYTES]);
+
+    if encrypt {
+        Ok(secretbox::seal(data, &nonce, &key))
+    } else {
+        secretbox::open(data, &nonce, &key)
+    }
+}
+
 mod test {
 
     #[test]
     fn test() {
-            use crate::password_security::config::*;
+        use super::*;
 
         let version = "00";
 
@@ -196,41 +216,3 @@ pub mod config {
         assert_eq!(succ, false);
     }
 }
-}
-
-mod base64 {
-    use super::symmetric_crypt;
-    use sodiumoxide::base64;
-
-    pub fn encrypt(v: &[u8]) -> Result<String, ()> {
-        if v.len() > 0 {
-            symmetric_crypt(v, true).map(|v| base64::encode(v, base64::Variant::Original))
-        } else {
-            Err(())
-        }
-    }
-
-    pub fn decrypt(v: &[u8]) -> Result<Vec<u8>, ()> {
-        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<Vec<u8>, ()> {
-    use sodiumoxide::crypto::secretbox;
-    use std::convert::TryInto;
-
-    let mut keybuf = crate::get_uuid();
-    keybuf.resize(secretbox::KEYBYTES, 0);
-    let key = secretbox::Key(keybuf.try_into().map_err(|_| ())?);
-    let nonce = secretbox::Nonce([0; secretbox::NONCEBYTES]);
-
-    if encrypt {
-        Ok(secretbox::seal(data, &nonce, &key))
-    } else {
-        secretbox::open(data, &nonce, &key)
-    }
-}

src/ipc.rs

@@ -8,9 +8,7 @@ use hbb_common::{
     config::{self, Config, Config2},
     futures::StreamExt as _,
     futures_util::sink::SinkExt,
-    log,
+    log, password_security as password, timeout, tokio,
-    password_security::password,
-    timeout, tokio,
     tokio::io::{AsyncRead, AsyncWrite},
     tokio_util::codec::Framed,
     ResultType,

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 store_path(Config::file(), config).is_err() {
-        if Config::set(config) {
             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 store_path(Config2::file(), config2).is_err() {
-        if Config2::set(config2) {
             log::info!("config2 written");
         }
     }

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" => {
+                "permanent_password" => Config::set_permanent_password(value),
-                    Config::set_permanent_password(value)
-                }
                 "temporary_password" => {
                     password::update_temporary_password();
                 }

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,