From 8b468ec34a770f8857408382b8a71624a9176ad1 Mon Sep 17 00:00:00 2001 From: Oscar Hinton Date: Wed, 31 Jan 2024 14:20:04 +0100 Subject: [PATCH] Fix device keys incorrectly being treated as strings (#564) Device keys are generated from random u8 vectors. We incorrectly attempted to parse these as strings when decrypting them in the last trust device step. Added a complete test which confirms the whole device approval process. --- Cargo.lock | 1 + crates/bitwarden/Cargo.toml | 1 + crates/bitwarden/src/auth/auth_request.rs | 86 ++++++++++++++++++++--- 3 files changed, 80 insertions(+), 8 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c4e1fe3c2..8ebf52136 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -373,6 +373,7 @@ dependencies = [ "uniffi", "uuid", "wiremock", + "zeroize", "zxcvbn", ] diff --git a/crates/bitwarden/Cargo.toml b/crates/bitwarden/Cargo.toml index 1f666e21c..b52e2f8f3 100644 --- a/crates/bitwarden/Cargo.toml +++ b/crates/bitwarden/Cargo.toml @@ -78,3 +78,4 @@ reqwest = { version = "*", features = [ rand_chacha = "0.3.1" tokio = { version = "1.35.1", features = ["rt", "macros"] } wiremock = "0.5.22" +zeroize = { version = ">=1.7.0, <2.0", features = ["derive", "aarch64"] } diff --git a/crates/bitwarden/src/auth/auth_request.rs b/crates/bitwarden/src/auth/auth_request.rs index facdc8f82..98fe4996a 100644 --- a/crates/bitwarden/src/auth/auth_request.rs +++ b/crates/bitwarden/src/auth/auth_request.rs @@ -58,9 +58,9 @@ pub(crate) fn auth_request_decrypt_user_key( user_key: AsymmetricEncString, ) -> Result { let key = AsymmetricCryptoKey::from_der(&STANDARD.decode(private_key)?)?; - let key: String = user_key.decrypt_with_key(&key)?; + let mut key: Vec = user_key.decrypt_with_key(&key)?; - Ok(key.parse()?) + Ok(SymmetricCryptoKey::try_from(key.as_mut_slice())?) } /// Approve an auth request. @@ -83,20 +83,25 @@ pub(crate) fn approve_auth_request( #[test] fn test_auth_request() { + use zeroize::Zeroizing; + let request = new_auth_request("test@bitwarden.com").unwrap(); - let secret = - "w2LO+nwV4oxwswVYCxlOfRUseXfvU03VzvKQHrqeklPgiMZrspUe6sOBToCnDn9Ay0tuCBn8ykVVRb7PWhub2Q=="; + let secret: &[u8] = &[ + 111, 32, 97, 169, 4, 241, 174, 74, 239, 206, 113, 86, 174, 68, 216, 238, 52, 85, 156, 27, + 134, 149, 54, 55, 91, 147, 45, 130, 131, 237, 51, 31, 191, 106, 155, 14, 160, 82, 47, 40, + 96, 31, 114, 127, 212, 187, 167, 110, 205, 116, 198, 243, 218, 72, 137, 53, 248, 43, 255, + 67, 35, 61, 245, 93, + ]; let private_key = AsymmetricCryptoKey::from_der(&STANDARD.decode(&request.private_key).unwrap()).unwrap(); - let encrypted = - AsymmetricEncString::encrypt_rsa2048_oaep_sha1(secret.as_bytes(), &private_key).unwrap(); + let encrypted = AsymmetricEncString::encrypt_rsa2048_oaep_sha1(secret, &private_key).unwrap(); let decrypted = auth_request_decrypt_user_key(request.private_key, encrypted).unwrap(); - assert_eq!(decrypted.to_base64(), secret); + assert_eq!(decrypted.to_vec(), Zeroizing::new(secret.to_owned())); } #[cfg(test)] @@ -106,7 +111,10 @@ mod tests { use bitwarden_crypto::Kdf; use super::*; - use crate::client::{LoginMethod, UserLoginMethod}; + use crate::{ + client::{LoginMethod, UserLoginMethod}, + mobile::crypto::{InitUserCryptoMethod, InitUserCryptoRequest}, + }; #[test] fn test_approve() { @@ -134,4 +142,66 @@ mod tests { approve_auth_request(&mut client, public_key.to_owned()).unwrap(); } + + #[tokio::test] + async fn test_device_login() { + let kdf = Kdf::PBKDF2 { + iterations: NonZeroU32::new(600_000).unwrap(), + }; + let email = "test@bitwarden.com"; + + let user_key = "2.Q/2PhzcC7GdeiMHhWguYAQ==|GpqzVdr0go0ug5cZh1n+uixeBC3oC90CIe0hd/HWA/pTRDZ8ane4fmsEIcuc8eMKUt55Y2q/fbNzsYu41YTZzzsJUSeqVjT8/iTQtgnNdpo=|dwI+uyvZ1h/iZ03VQ+/wrGEFYVewBUUl/syYgjsNMbE=".parse().unwrap(); + let private_key = "2.yN7l00BOlUE0Sb0M//Q53w==|EwKG/BduQRQ33Izqc/ogoBROIoI5dmgrxSo82sgzgAMIBt3A2FZ9vPRMY+GWT85JiqytDitGR3TqwnFUBhKUpRRAq4x7rA6A1arHrFp5Tp1p21O3SfjtvB3quiOKbqWk6ZaU1Np9HwqwAecddFcB0YyBEiRX3VwF2pgpAdiPbSMuvo2qIgyob0CUoC/h4Bz1be7Qa7B0Xw9/fMKkB1LpOm925lzqosyMQM62YpMGkjMsbZz0uPopu32fxzDWSPr+kekNNyLt9InGhTpxLmq1go/pXR2uw5dfpXc5yuta7DB0EGBwnQ8Vl5HPdDooqOTD9I1jE0mRyuBpWTTI3FRnu3JUh3rIyGBJhUmHqGZvw2CKdqHCIrQeQkkEYqOeJRJVdBjhv5KGJifqT3BFRwX/YFJIChAQpebNQKXe/0kPivWokHWwXlDB7S7mBZzhaAPidZvnuIhalE2qmTypDwHy22FyqV58T8MGGMchcASDi/QXI6kcdpJzPXSeU9o+NC68QDlOIrMVxKFeE7w7PvVmAaxEo0YwmuAzzKy9QpdlK0aab/xEi8V4iXj4hGepqAvHkXIQd+r3FNeiLfllkb61p6WTjr5urcmDQMR94/wYoilpG5OlybHdbhsYHvIzYoLrC7fzl630gcO6t4nM24vdB6Ymg9BVpEgKRAxSbE62Tqacxqnz9AcmgItb48NiR/He3n3ydGjPYuKk/ihZMgEwAEZvSlNxYONSbYrIGDtOY+8Nbt6KiH3l06wjZW8tcmFeVlWv+tWotnTY9IqlAfvNVTjtsobqtQnvsiDjdEVtNy/s2ci5TH+NdZluca2OVEr91Wayxh70kpM6ib4UGbfdmGgCo74gtKvKSJU0rTHakQ5L9JlaSDD5FamBRyI0qfL43Ad9qOUZ8DaffDCyuaVyuqk7cz9HwmEmvWU3VQ+5t06n/5kRDXttcw8w+3qClEEdGo1KeENcnXCB32dQe3tDTFpuAIMLqwXs6FhpawfZ5kPYvLPczGWaqftIs/RXJ/EltGc0ugw2dmTLpoQhCqrcKEBDoYVk0LDZKsnzitOGdi9mOWse7Se8798ib1UsHFUjGzISEt6upestxOeupSTOh0v4+AjXbDzRUyogHww3V+Bqg71bkcMxtB+WM+pn1XNbVTyl9NR040nhP7KEf6e9ruXAtmrBC2ah5cFEpLIot77VFZ9ilLuitSz+7T8n1yAh1IEG6xxXxninAZIzi2qGbH69O5RSpOJuJTv17zTLJQIIc781JwQ2TTwTGnx5wZLbffhCasowJKd2EVcyMJyhz6ru0PvXWJ4hUdkARJs3Xu8dus9a86N8Xk6aAPzBDqzYb1vyFIfBxP0oO8xFHgd30Cgmz8UrSE3qeWRrF8ftrI6xQnFjHBGWD/JWSvd6YMcQED0aVuQkuNW9ST/DzQThPzRfPUoiL10yAmV7Ytu4fR3x2sF0Yfi87YhHFuCMpV/DsqxmUizyiJuD938eRcH8hzR/VO53Qo3UIsqOLcyXtTv6THjSlTopQ+JOLOnHm1w8dzYbLN44OG44rRsbihMUQp+wUZ6bsI8rrOnm9WErzkbQFbrfAINdoCiNa6cimYIjvvnMTaFWNymqY1vZxGztQiMiHiHYwTfwHTXrb9j0uPM=|09J28iXv9oWzYtzK2LBT6Yht4IT4MijEkk0fwFdrVQ4="; + + // Initialize an existing client which is unlocked + let mut existing_device = Client::new(None); + existing_device.set_login_method(LoginMethod::User(UserLoginMethod::Username { + client_id: "123".to_owned(), + email: email.to_owned(), + kdf: kdf.clone(), + })); + + existing_device + .initialize_user_crypto("asdfasdfasdf", user_key, private_key.parse().unwrap()) + .unwrap(); + + // Initialize a new device which will request to be logged in + let mut new_device = Client::new(None); + + // Initialize an auth request, and approve it on the existing device + let auth_req = new_auth_request(email).unwrap(); + let approved_req = approve_auth_request(&mut existing_device, auth_req.public_key).unwrap(); + + // Unlock the vault using the approved request + new_device + .crypto() + .initialize_user_crypto(InitUserCryptoRequest { + kdf_params: kdf, + email: email.to_owned(), + private_key: private_key.to_owned(), + method: InitUserCryptoMethod::AuthRequest { + request_private_key: auth_req.private_key, + protected_user_key: approved_req, + }, + }) + .await + .unwrap(); + + // We can validate that the vault is unlocked correctly by confirming the user key is the + // same + assert_eq!( + existing_device + .get_encryption_settings() + .unwrap() + .get_key(&None) + .unwrap() + .to_base64(), + new_device + .get_encryption_settings() + .unwrap() + .get_key(&None) + .unwrap() + .to_base64() + ); + } }