Merge pull request #545 from fufesou/fix/symmetric-crypt-nonce

fix(security): add nonce to local symmetric encryption

src/config.rs

@@ -521,8 +521,9 @@ impl Config2 {
                 encrypt_str_or_original(&socks.password, PASSWORD_ENC_VERSION, ENCRYPT_MAX_LEN);
             config.socks = Some(socks);
         }
+        let stored = Config::load_::<Config2>("2");
         config.unlock_pin =
-            encrypt_str_or_original(&config.unlock_pin, PASSWORD_ENC_VERSION, ENCRYPT_MAX_LEN);
+            keep_encrypted_storage_if_plaintext_unchanged(&config.unlock_pin, &stored.unlock_pin);
         Config::store_(&config, "2");
     }
 
@@ -541,6 +542,14 @@ impl Config2 {
     }
 }
 
+fn keep_encrypted_storage_if_plaintext_unchanged(plain: &str, stored: &str) -> String {
+    let (stored_plain, encrypted, _) = decrypt_str_or_original(stored, PASSWORD_ENC_VERSION);
+    if encrypted && stored_plain == plain {
+        return stored.to_owned();
+    }
+    encrypt_str_or_original(plain, PASSWORD_ENC_VERSION, ENCRYPT_MAX_LEN)
+}
+
 pub fn load_path<T: serde::Serialize + serde::de::DeserializeOwned + Default + std::fmt::Debug>(
     file: PathBuf,
 ) -> T {
@@ -710,7 +719,12 @@ impl Config {
             config.password =
                 encrypt_str_or_original(&config.password, PASSWORD_ENC_VERSION, ENCRYPT_MAX_LEN);
         }
-        config.enc_id = encrypt_str_or_original(&config.id, PASSWORD_ENC_VERSION, ENCRYPT_MAX_LEN);
+        let (stored_id, encrypted, _) =
+            decrypt_str_or_original(&config.enc_id, PASSWORD_ENC_VERSION);
+        if !encrypted || stored_id != config.id {
+            config.enc_id =
+                encrypt_str_or_original(&config.id, PASSWORD_ENC_VERSION, ENCRYPT_MAX_LEN);
+        }
         config.id = "".to_owned();
         Config::store_(&config, "");
     }
@@ -3263,6 +3277,11 @@ mod tests {
         original_hard_settings: HashMap<String, String>,
     }
 
+    struct ConfigFileRestoreGuard {
+        path: PathBuf,
+        original_content: Option<Vec<u8>>,
+    }
+
     impl ConfigStateTestGuard {
         fn new(config: Config, hard_settings: HashMap<String, String>) -> Self {
             let original_config = Config::get();
@@ -3283,6 +3302,29 @@ mod tests {
         }
     }
 
+    impl ConfigFileRestoreGuard {
+        fn new(path: PathBuf) -> Self {
+            let original_content = fs::read(&path).ok();
+            Self {
+                path,
+                original_content,
+            }
+        }
+    }
+
+    impl Drop for ConfigFileRestoreGuard {
+        fn drop(&mut self) {
+            if let Some(content) = &self.original_content {
+                if let Some(parent) = self.path.parent() {
+                    fs::create_dir_all(parent).ok();
+                }
+                fs::write(&self.path, content).ok();
+            } else {
+                fs::remove_file(&self.path).ok();
+            }
+        }
+    }
+
     fn with_config_and_hard_settings<R>(
         config: Config,
         hard_settings: HashMap<String, String>,
@@ -3472,6 +3514,67 @@ mod tests {
         });
     }
 
+    #[test]
+    fn test_store_keeps_existing_enc_id_when_id_is_unchanged() {
+        let mut cfg = Config::default();
+        cfg.id = "123456789".to_owned();
+        cfg.enc_id = encrypt_str_or_original(&cfg.id, PASSWORD_ENC_VERSION, ENCRYPT_MAX_LEN);
+        let original_enc_id = cfg.enc_id.clone();
+
+        with_config_and_hard_settings(Config::default(), HashMap::new(), || {
+            assert!(Config::set(cfg));
+
+            assert_eq!(Config::load().enc_id, original_enc_id);
+            assert_eq!(Config::get().id, "123456789");
+        });
+    }
+
+    #[test]
+    fn test_store_rewrites_enc_id_when_id_changes() {
+        let original_id = "123456789";
+        let updated_id = "987654321";
+        let mut cfg = Config::default();
+        cfg.id = updated_id.to_owned();
+        let original_enc_id =
+            encrypt_str_or_original(original_id, PASSWORD_ENC_VERSION, ENCRYPT_MAX_LEN);
+        cfg.enc_id = original_enc_id.clone();
+
+        with_config_and_hard_settings(Config::default(), HashMap::new(), || {
+            assert!(Config::set(cfg));
+
+            let stored = Config::load().enc_id;
+            let (stored_id, encrypted, _) = decrypt_str_or_original(&stored, PASSWORD_ENC_VERSION);
+            assert_ne!(stored, original_enc_id);
+            assert!(encrypted);
+            assert_eq!(stored_id, updated_id);
+            assert_eq!(Config::get().id, updated_id);
+        });
+    }
+
+    #[test]
+    fn test_config2_store_keeps_existing_unlock_pin_when_pin_is_unchanged() {
+        let _guard = CONFIG_STATE_TEST_LOCK.lock().unwrap();
+        let _file_guard = ConfigFileRestoreGuard::new(Config::file_("2"));
+        let pin = "123456";
+        let original_unlock_pin =
+            encrypt_str_or_original(pin, PASSWORD_ENC_VERSION, ENCRYPT_MAX_LEN);
+        let mut cfg = Config2 {
+            unlock_pin: original_unlock_pin.clone(),
+            ..Default::default()
+        };
+        Config::store_(&cfg, "2");
+        let (unlock_pin, decrypted, _) =
+            decrypt_str_or_original(&cfg.unlock_pin, PASSWORD_ENC_VERSION);
+        assert!(decrypted);
+        cfg.unlock_pin = unlock_pin;
+        cfg.nat_type = 1;
+
+        cfg.store();
+
+        let stored = Config::load_::<Config2>("2");
+        assert_eq!(stored.unlock_pin, original_unlock_pin);
+    }
+
     #[test]
     fn test_set_does_not_convert_plaintext_permanent_password_to_storage_format_in_memory() {
         let mut cfg = Config::default();

src/password_security.rs

@@ -1,5 +1,5 @@
 use crate::config::Config;
-use sodiumoxide::base64;
+use sodiumoxide::{base64, crypto::secretbox};
 use std::sync::{Arc, RwLock};
 
 lazy_static::lazy_static! {
@@ -92,6 +92,7 @@ pub fn hide_cm() -> bool {
 }
 
 const VERSION_LEN: usize = 2;
+const FORMAT_V1: u8 = 1;
 
 // Check if data is already encrypted by verifying:
 // 1) version prefix "00"
@@ -100,6 +101,7 @@ const VERSION_LEN: usize = 2;
 //
 // We intentionally avoid trying to decrypt here because key mismatch would cause
 // false negatives.
+// The decoded payload may be either legacy ciphertext or FORMAT_V1 || nonce || ciphertext.
 // Reference: secretbox::seal returns ciphertext length = plaintext length + MACBYTES
 // https://github.com/sodiumoxide/sodiumoxide/blob/3057acb1a030ad86ed8892a223d64036ab5e8523/src/crypto/secretbox/xsalsa20poly1305.rs#L67
 fn is_encrypted(v: &[u8]) -> bool {
@@ -215,12 +217,17 @@ pub fn symmetric_crypt(data: &[u8], encrypt: bool) -> Result<Vec<u8>, ()> {
     let mut keybuf = uuid.clone();
     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))
+        let nonce = secretbox::gen_nonce();
+        let encrypted = secretbox::seal(data, &nonce, &key);
+        let mut output = Vec::with_capacity(1 + nonce.0.len() + encrypted.len());
+        output.push(FORMAT_V1);
+        output.extend(nonce.0);
+        output.extend(encrypted);
+        Ok(output)
     } else {
-        let res = secretbox::open(data, &nonce, &key);
+        let res = open_secretbox_payload(data, &key);
         #[cfg(not(any(target_os = "android", target_os = "ios")))]
         if res.is_err() {
             // Fallback: try pk if uuid decryption failed (in case encryption used pk due to machine_uid failure)
@@ -230,7 +237,7 @@ pub fn symmetric_crypt(data: &[u8], encrypt: bool) -> Result<Vec<u8>, ()> {
                     let mut keybuf = pk;
                     keybuf.resize(secretbox::KEYBYTES, 0);
                     let pk_key = secretbox::Key(keybuf.try_into().map_err(|_| ())?);
-                    return secretbox::open(data, &nonce, &pk_key);
+                    return open_secretbox_payload(data, &pk_key);
                 }
             }
         }
@@ -238,6 +245,22 @@ pub fn symmetric_crypt(data: &[u8], encrypt: bool) -> Result<Vec<u8>, ()> {
     }
 }
 
+fn open_secretbox_payload(data: &[u8], key: &secretbox::Key) -> Result<Vec<u8>, ()> {
+    if data.first() == Some(&FORMAT_V1)
+        && data.len() >= 1 + secretbox::NONCEBYTES + secretbox::MACBYTES
+    {
+        let mut nonce = [0u8; secretbox::NONCEBYTES];
+        nonce.copy_from_slice(&data[1..1 + secretbox::NONCEBYTES]);
+        let nonce = secretbox::Nonce(nonce);
+        if let Ok(decrypted) = secretbox::open(&data[1 + secretbox::NONCEBYTES..], &nonce, key) {
+            return Ok(decrypted);
+        }
+    }
+
+    let legacy_nonce = secretbox::Nonce([0; secretbox::NONCEBYTES]);
+    secretbox::open(data, &legacy_nonce, key)
+}
+
 mod test {
 
     #[test]
@@ -438,6 +461,116 @@ mod test {
         );
     }
 
+    #[test]
+    fn test_encryption_uses_random_nonce() {
+        use super::*;
+
+        let data = b"test password 123";
+        let encrypted1 = symmetric_crypt(data, true).unwrap();
+        let encrypted2 = symmetric_crypt(data, true).unwrap();
+
+        assert_eq!(encrypted1.first(), Some(&FORMAT_V1));
+        assert_eq!(encrypted2.first(), Some(&FORMAT_V1));
+        assert_eq!(
+            encrypted1.len(),
+            1 + secretbox::NONCEBYTES + data.len() + secretbox::MACBYTES
+        );
+        assert_ne!(encrypted1, encrypted2);
+        assert_eq!(symmetric_crypt(&encrypted1, false).unwrap(), data);
+        assert_eq!(symmetric_crypt(&encrypted2, false).unwrap(), data);
+    }
+
+    #[test]
+    fn test_decrypt_legacy_zero_nonce_payload() {
+        use super::*;
+        use std::convert::TryInto;
+
+        let data = b"test password 123";
+        let uuid = crate::get_uuid();
+        let mut keybuf = uuid.clone();
+        keybuf.resize(secretbox::KEYBYTES, 0);
+        let key = secretbox::Key(keybuf.try_into().unwrap());
+        let nonce = secretbox::Nonce([0; secretbox::NONCEBYTES]);
+        let encrypted = secretbox::seal(data, &nonce, &key);
+
+        assert_eq!(symmetric_crypt(&encrypted, false).unwrap(), data);
+    }
+
+    #[test]
+    fn test_decrypt_legacy_payload_starting_with_v1_marker() {
+        use super::*;
+        use std::convert::TryInto;
+
+        let mut keybuf = crate::get_uuid();
+        keybuf.resize(secretbox::KEYBYTES, 0);
+        let key = secretbox::Key(keybuf.try_into().unwrap());
+        let nonce = secretbox::Nonce([0; secretbox::NONCEBYTES]);
+
+        for i in 0..=u16::MAX {
+            let data = format!("legacy collision payload {i:05}");
+            let encrypted = secretbox::seal(data.as_bytes(), &nonce, &key);
+            if encrypted.first() == Some(&FORMAT_V1) {
+                assert_eq!(symmetric_crypt(&encrypted, false).unwrap(), data.as_bytes());
+                return;
+            }
+        }
+
+        panic!("failed to find legacy payload starting with FORMAT_V1");
+    }
+
+    #[test]
+    fn test_invalid_short_v1_payload_returns_error() {
+        use super::*;
+
+        let encrypted = vec![FORMAT_V1];
+
+        assert!(symmetric_crypt(&encrypted, false).is_err());
+    }
+
+    #[test]
+    fn test_decrypt_legacy_string_does_not_request_store() {
+        use super::*;
+        use sodiumoxide::base64::{encode, Variant};
+        use std::convert::TryInto;
+
+        let data = "test password 123";
+        let uuid = crate::get_uuid();
+        let mut keybuf = uuid.clone();
+        keybuf.resize(secretbox::KEYBYTES, 0);
+        let key = secretbox::Key(keybuf.try_into().unwrap());
+        let nonce = secretbox::Nonce([0; secretbox::NONCEBYTES]);
+        let encrypted = secretbox::seal(data.as_bytes(), &nonce, &key);
+        let encrypted = "00".to_owned() + &encode(encrypted, Variant::Original);
+
+        let (decrypted, success, store) = decrypt_str_or_original(&encrypted, "00");
+
+        assert_eq!(decrypted, data);
+        assert!(success);
+        assert!(!store);
+    }
+
+    #[test]
+    fn test_decrypt_legacy_vec_does_not_request_store() {
+        use super::*;
+        use sodiumoxide::base64::{encode, Variant};
+        use std::convert::TryInto;
+
+        let data = b"test password 123";
+        let uuid = crate::get_uuid();
+        let mut keybuf = uuid.clone();
+        keybuf.resize(secretbox::KEYBYTES, 0);
+        let key = secretbox::Key(keybuf.try_into().unwrap());
+        let nonce = secretbox::Nonce([0; secretbox::NONCEBYTES]);
+        let encrypted = secretbox::seal(data, &nonce, &key);
+        let encrypted = ("00".to_owned() + &encode(encrypted, Variant::Original)).into_bytes();
+
+        let (decrypted, success, store) = decrypt_vec_or_original(&encrypted, "00");
+
+        assert_eq!(decrypted, data);
+        assert!(success);
+        assert!(!store);
+    }
+
     // Test decryption fallback when data was encrypted with key_pair but decryption tries machine_uid first
     #[test]
     #[cfg(not(any(target_os = "android", target_os = "ios")))]
@@ -471,4 +604,48 @@ mod test {
         );
         assert_eq!(decrypted.unwrap(), data);
     }
+
+    #[test]
+    #[cfg(not(any(target_os = "android", target_os = "ios")))]
+    fn test_decrypt_v1_with_pk_fallback() {
+        use super::*;
+        use sodiumoxide::base64::{encode, Variant};
+        use sodiumoxide::crypto::secretbox;
+        use std::convert::TryInto;
+
+        let uuid = crate::get_uuid();
+        let pk = crate::config::Config::get_key_pair().1;
+
+        if uuid == pk {
+            eprintln!("skip: uuid == pk, fallback branch won't be tested");
+            return;
+        }
+
+        let data = b"test password 123";
+        let nonce = secretbox::gen_nonce();
+
+        let mut pk_keybuf = pk;
+        pk_keybuf.resize(secretbox::KEYBYTES, 0);
+        let pk_key = secretbox::Key(pk_keybuf.try_into().unwrap());
+        let ciphertext = secretbox::seal(data, &nonce, &pk_key);
+
+        let mut encrypted = Vec::with_capacity(1 + secretbox::NONCEBYTES + ciphertext.len());
+        encrypted.push(FORMAT_V1);
+        encrypted.extend(nonce.0);
+        encrypted.extend(ciphertext);
+
+        assert_eq!(super::symmetric_crypt(&encrypted, false).unwrap(), data);
+
+        let encrypted_str = "00".to_owned() + &encode(&encrypted, Variant::Original);
+        let (decrypted, success, store) = decrypt_str_or_original(&encrypted_str, "00");
+        assert_eq!(decrypted.as_bytes(), data);
+        assert!(success);
+        assert!(!store);
+
+        let encrypted_vec = encrypted_str.into_bytes();
+        let (decrypted, success, store) = decrypt_vec_or_original(&encrypted_vec, "00");
+        assert_eq!(decrypted, data);
+        assert!(success);
+        assert!(!store);
+    }
 }