From 2d8c8e18f74726303f7789af9731e8a80ce05610 Mon Sep 17 00:00:00 2001 From: BlackDex Date: Tue, 24 Jan 2023 13:06:31 +0100 Subject: [PATCH 1/3] Update KDF Configuration and processing - Change default Password Hash KDF Storage from 100_000 to 600_000 iterations - Update Password Hash when the default iteration value is different - Validate password_iterations - Validate client-side KDF to prevent it from being set lower than 100_000 --- .env.template | 6 +++--- src/api/core/accounts.rs | 11 ++++++++--- src/api/core/emergency_access.rs | 2 +- src/api/identity.rs | 13 +++++++++++-- src/config.rs | 10 +++++++--- src/db/models/user.rs | 8 +++++--- 6 files changed, 35 insertions(+), 15 deletions(-) diff --git a/.env.template b/.env.template index 4b323706..1b691298 100644 --- a/.env.template +++ b/.env.template @@ -298,9 +298,9 @@ ## This setting applies globally to all users. # INCOMPLETE_2FA_TIME_LIMIT=3 -## Controls the PBBKDF password iterations to apply on the server -## The change only applies when the password is changed -# PASSWORD_ITERATIONS=100000 +## Number of server-side passwords hashing iterations for the password hash. +## The default for new users. If changed, it will be updated during login for existing users. +# PASSWORD_ITERATIONS=350000 ## Controls whether users can set password hints. This setting applies globally to all users. # PASSWORD_HINTS_ALLOWED=true diff --git a/src/api/core/accounts.rs b/src/api/core/accounts.rs index 758d9028..5faff713 100644 --- a/src/api/core/accounts.rs +++ b/src/api/core/accounts.rs @@ -161,7 +161,7 @@ pub async fn _register(data: JsonUpcase, mut conn: DbConn) -> Json user.client_kdf_type = client_kdf_type; } - user.set_password(&data.MasterPasswordHash, None); + user.set_password(&data.MasterPasswordHash, true, None); user.akey = data.Key; user.password_hint = password_hint; @@ -318,6 +318,7 @@ async fn post_password( user.set_password( &data.NewMasterPasswordHash, + true, Some(vec![String::from("post_rotatekey"), String::from("get_contacts"), String::from("get_public_keys")]), ); user.akey = data.Key; @@ -348,9 +349,13 @@ async fn post_kdf(data: JsonUpcase, headers: Headers, mut conn: D err!("Invalid password") } + if data.KdfIterations < 100_000 { + err!("KDF iterations lower then 100000 are not allowed.") + } + user.client_kdf_iter = data.KdfIterations; user.client_kdf_type = data.Kdf; - user.set_password(&data.NewMasterPasswordHash, None); + user.set_password(&data.NewMasterPasswordHash, true, None); user.akey = data.Key; let save_result = user.save(&mut conn).await; @@ -560,7 +565,7 @@ async fn post_email( user.email_new = None; user.email_new_token = None; - user.set_password(&data.NewMasterPasswordHash, None); + user.set_password(&data.NewMasterPasswordHash, true, None); user.akey = data.Key; let save_result = user.save(&mut conn).await; diff --git a/src/api/core/emergency_access.rs b/src/api/core/emergency_access.rs index f15c1b8e..64ed6d86 100644 --- a/src/api/core/emergency_access.rs +++ b/src/api/core/emergency_access.rs @@ -662,7 +662,7 @@ async fn password_emergency_access( }; // change grantor_user password - grantor_user.set_password(new_master_password_hash, None); + grantor_user.set_password(new_master_password_hash, true, None); grantor_user.akey = key; grantor_user.save(&mut conn).await?; diff --git a/src/api/identity.rs b/src/api/identity.rs index 0cb1c03a..7d004a9c 100644 --- a/src/api/identity.rs +++ b/src/api/identity.rs @@ -130,7 +130,7 @@ async fn _password_login( // Get the user let username = data.username.as_ref().unwrap().trim(); - let user = match User::find_by_mail(username, conn).await { + let mut user = match User::find_by_mail(username, conn).await { Some(user) => user, None => err!("Username or password is incorrect. Try again", format!("IP: {}. Username: {}.", ip.ip, username)), }; @@ -150,6 +150,16 @@ async fn _password_login( ) } + // Change the KDF Iterations + if user.password_iterations != CONFIG.password_iterations() { + user.password_iterations = CONFIG.password_iterations(); + user.set_password(password, false, None); + + if let Err(e) = user.save(conn).await { + error!("Error updating user: {:#?}", e); + } + } + // Check if the user is disabled if !user.enabled { err!( @@ -172,7 +182,6 @@ async fn _password_login( if resend_limit == 0 || user.login_verify_count < resend_limit { // We want to send another email verification if we require signups to verify // their email address, and we haven't sent them a reminder in a while... - let mut user = user; user.last_verifying_at = Some(now); user.login_verify_count += 1; diff --git a/src/config.rs b/src/config.rs index f8990dc0..46deed54 100644 --- a/src/config.rs +++ b/src/config.rs @@ -463,9 +463,9 @@ make_config! { invitation_expiration_hours: u32, false, def, 120; /// Allow emergency access |> Controls whether users can enable emergency access to their accounts. This setting applies globally to all users. emergency_access_allowed: bool, true, def, true; - /// Password iterations |> Number of server-side passwords hashing iterations. - /// The changes only apply when a user changes their password. Not recommended to lower the value - password_iterations: i32, true, def, 100_000; + /// Password iterations |> Number of server-side passwords hashing iterations for the password hash. + /// The default for new users. If changed, it will be updated during login for existing users. + password_iterations: i32, true, def, 600_000; /// Allow password hints |> Controls whether users can set password hints. This setting applies globally to all users. password_hints_allowed: bool, true, def, true; /// Show password hint |> Controls whether a password hint should be shown directly in the web page @@ -673,6 +673,10 @@ fn validate_config(cfg: &ConfigItems) -> Result<(), Error> { } } + if cfg.password_iterations < 100_000 { + err!("PASSWORD_ITERATIONS should be at least 100000 or higher. The default is 600000!"); + } + let limit = 256; if cfg.database_max_conns < 1 || cfg.database_max_conns > limit { err!(format!("`DATABASE_MAX_CONNS` contains an invalid value. Ensure it is between 1 and {limit}.",)); diff --git a/src/db/models/user.rs b/src/db/models/user.rs index 611c4ebb..15aacf1f 100644 --- a/src/db/models/user.rs +++ b/src/db/models/user.rs @@ -74,7 +74,7 @@ pub struct UserStampException { /// Local methods impl User { pub const CLIENT_KDF_TYPE_DEFAULT: i32 = 0; // PBKDF2: 0 - pub const CLIENT_KDF_ITER_DEFAULT: i32 = 100_000; + pub const CLIENT_KDF_ITER_DEFAULT: i32 = 600_000; pub fn new(email: String) -> Self { let now = Utc::now().naive_utc(); @@ -151,14 +151,16 @@ impl User { /// These routes are able to use the previous stamp id for the next 2 minutes. /// After these 2 minutes this stamp will expire. /// - pub fn set_password(&mut self, password: &str, allow_next_route: Option>) { + pub fn set_password(&mut self, password: &str, reset_security_stamp: bool, allow_next_route: Option>) { self.password_hash = crypto::hash_password(password.as_bytes(), &self.salt, self.password_iterations as u32); if let Some(route) = allow_next_route { self.set_stamp_exception(route); } - self.reset_security_stamp() + if reset_security_stamp { + self.reset_security_stamp() + } } pub fn reset_security_stamp(&mut self) { From cc91ac6cc02d6b95f21067edee2242733d97445c Mon Sep 17 00:00:00 2001 From: sirux88 Date: Sat, 14 Jan 2023 10:16:03 +0100 Subject: [PATCH 2/3] include key into user.set_password --- src/api/core/accounts.rs | 13 ++++++------- src/api/core/emergency_access.rs | 5 ++--- src/api/identity.rs | 2 +- src/db/models/user.rs | 13 ++++++++++++- 4 files changed, 21 insertions(+), 12 deletions(-) diff --git a/src/api/core/accounts.rs b/src/api/core/accounts.rs index 5faff713..b8d3bd8a 100644 --- a/src/api/core/accounts.rs +++ b/src/api/core/accounts.rs @@ -161,8 +161,7 @@ pub async fn _register(data: JsonUpcase, mut conn: DbConn) -> Json user.client_kdf_type = client_kdf_type; } - user.set_password(&data.MasterPasswordHash, true, None); - user.akey = data.Key; + user.set_password(&data.MasterPasswordHash, Some(data.Key), true, None); user.password_hint = password_hint; // Add extra fields if present @@ -318,10 +317,11 @@ async fn post_password( user.set_password( &data.NewMasterPasswordHash, + Some(data.Key), true, Some(vec![String::from("post_rotatekey"), String::from("get_contacts"), String::from("get_public_keys")]), ); - user.akey = data.Key; + let save_result = user.save(&mut conn).await; nt.send_user_update(UpdateType::LogOut, &user).await; @@ -355,8 +355,7 @@ async fn post_kdf(data: JsonUpcase, headers: Headers, mut conn: D user.client_kdf_iter = data.KdfIterations; user.client_kdf_type = data.Kdf; - user.set_password(&data.NewMasterPasswordHash, true, None); - user.akey = data.Key; + user.set_password(&data.NewMasterPasswordHash, Some(data.Key), true, None); let save_result = user.save(&mut conn).await; nt.send_user_update(UpdateType::LogOut, &user).await; @@ -565,8 +564,8 @@ async fn post_email( user.email_new = None; user.email_new_token = None; - user.set_password(&data.NewMasterPasswordHash, true, None); - user.akey = data.Key; + user.set_password(&data.NewMasterPasswordHash, Some(data.Key), true, None); + let save_result = user.save(&mut conn).await; nt.send_user_update(UpdateType::LogOut, &user).await; diff --git a/src/api/core/emergency_access.rs b/src/api/core/emergency_access.rs index 64ed6d86..fcabc617 100644 --- a/src/api/core/emergency_access.rs +++ b/src/api/core/emergency_access.rs @@ -644,7 +644,7 @@ async fn password_emergency_access( let data: EmergencyAccessPasswordData = data.into_inner().data; let new_master_password_hash = &data.NewMasterPasswordHash; - let key = data.Key; + //let key = &data.Key; let requesting_user = headers.user; let emergency_access = match EmergencyAccess::find_by_uuid(&emer_id, &mut conn).await { @@ -662,8 +662,7 @@ async fn password_emergency_access( }; // change grantor_user password - grantor_user.set_password(new_master_password_hash, true, None); - grantor_user.akey = key; + grantor_user.set_password(new_master_password_hash, Some(data.Key), true, None); grantor_user.save(&mut conn).await?; // Disable TwoFactor providers since they will otherwise block logins diff --git a/src/api/identity.rs b/src/api/identity.rs index 7d004a9c..e52608e9 100644 --- a/src/api/identity.rs +++ b/src/api/identity.rs @@ -153,7 +153,7 @@ async fn _password_login( // Change the KDF Iterations if user.password_iterations != CONFIG.password_iterations() { user.password_iterations = CONFIG.password_iterations(); - user.set_password(password, false, None); + user.set_password(password, None, false, None); if let Err(e) = user.save(conn).await { error!("Error updating user: {:#?}", e); diff --git a/src/db/models/user.rs b/src/db/models/user.rs index 15aacf1f..5ce87e14 100644 --- a/src/db/models/user.rs +++ b/src/db/models/user.rs @@ -147,17 +147,28 @@ impl User { /// # Arguments /// /// * `password` - A str which contains a hashed version of the users master password. + /// * `new_key` - A String which contains the new aKey value of the users master password. /// * `allow_next_route` - A Option> with the function names of the next allowed (rocket) routes. /// These routes are able to use the previous stamp id for the next 2 minutes. /// After these 2 minutes this stamp will expire. /// - pub fn set_password(&mut self, password: &str, reset_security_stamp: bool, allow_next_route: Option>) { + pub fn set_password( + &mut self, + password: &str, + new_key: Option, + reset_security_stamp: bool, + allow_next_route: Option>, + ) { self.password_hash = crypto::hash_password(password.as_bytes(), &self.salt, self.password_iterations as u32); if let Some(route) = allow_next_route { self.set_stamp_exception(route); } + if let Some(new_key) = new_key { + self.akey = new_key; + } + if reset_security_stamp { self.reset_security_stamp() } From e38e1a5d5f33329f3083ed65223424f0dc77c4db Mon Sep 17 00:00:00 2001 From: BlackDex Date: Fri, 20 Jan 2023 15:43:45 +0100 Subject: [PATCH 3/3] Validate note sizes on key-rotation. We also need to validate the note sizes on key-rotation. If we do not validate them before we store them, that could lead to a partial or total loss of the password vault. Validating these restrictions before actually processing them to store/replace the existing ciphers should prevent this. There was also a small bug when using web-sockets. The client which is triggering the password/key-rotation change should not be forced to logout via a web-socket request. That is something the client will handle it self. Refactored the logout notification to either send the device uuid or not on specific actions. Fixes #3152 --- src/api/admin.rs | 6 +++--- src/api/core/accounts.rs | 22 +++++++++++++++++----- src/api/notifications.rs | 10 ++++++++++ 3 files changed, 30 insertions(+), 8 deletions(-) diff --git a/src/api/admin.rs b/src/api/admin.rs index 6e0c2acf..f22d3bc2 100644 --- a/src/api/admin.rs +++ b/src/api/admin.rs @@ -13,7 +13,7 @@ use rocket::{ }; use crate::{ - api::{core::log_event, ApiResult, EmptyResult, JsonResult, Notify, NumberOrString, UpdateType}, + api::{core::log_event, ApiResult, EmptyResult, JsonResult, Notify, NumberOrString}, auth::{decode_admin, encode_jwt, generate_admin_claims, ClientIp}, config::ConfigBuilder, db::{backup_database, get_sql_server_version, models::*, DbConn, DbConnType}, @@ -372,7 +372,7 @@ async fn deauth_user(uuid: String, _token: AdminToken, mut conn: DbConn, nt: Not let save_result = user.save(&mut conn).await; - nt.send_user_update(UpdateType::LogOut, &user).await; + nt.send_logout(&user, None).await; save_result } @@ -386,7 +386,7 @@ async fn disable_user(uuid: String, _token: AdminToken, mut conn: DbConn, nt: No let save_result = user.save(&mut conn).await; - nt.send_user_update(UpdateType::LogOut, &user).await; + nt.send_logout(&user, None).await; save_result } diff --git a/src/api/core/accounts.rs b/src/api/core/accounts.rs index b8d3bd8a..a2816a4c 100644 --- a/src/api/core/accounts.rs +++ b/src/api/core/accounts.rs @@ -324,7 +324,10 @@ async fn post_password( let save_result = user.save(&mut conn).await; - nt.send_user_update(UpdateType::LogOut, &user).await; + // Prevent loging out the client where the user requested this endpoint from. + // If you do logout the user it will causes issues at the client side. + // Adding the device uuid will prevent this. + nt.send_logout(&user, Some(headers.device.uuid)).await; save_result } @@ -358,7 +361,7 @@ async fn post_kdf(data: JsonUpcase, headers: Headers, mut conn: D user.set_password(&data.NewMasterPasswordHash, Some(data.Key), true, None); let save_result = user.save(&mut conn).await; - nt.send_user_update(UpdateType::LogOut, &user).await; + nt.send_logout(&user, Some(headers.device.uuid)).await; save_result } @@ -396,6 +399,12 @@ async fn post_rotatekey( err!("Invalid password") } + // Validate the import before continuing + // Bitwarden does not process the import if there is one item invalid. + // Since we check for the size of the encrypted note length, we need to do that here to pre-validate it. + // TODO: See if we can optimize the whole cipher adding/importing and prevent duplicate code and checks. + Cipher::validate_notes(&data.Ciphers)?; + let user_uuid = &headers.user.uuid; // Update folder data @@ -442,7 +451,10 @@ async fn post_rotatekey( let save_result = user.save(&mut conn).await; - nt.send_user_update(UpdateType::LogOut, &user).await; + // Prevent loging out the client where the user requested this endpoint from. + // If you do logout the user it will causes issues at the client side. + // Adding the device uuid will prevent this. + nt.send_logout(&user, Some(headers.device.uuid)).await; save_result } @@ -465,7 +477,7 @@ async fn post_sstamp( user.reset_security_stamp(); let save_result = user.save(&mut conn).await; - nt.send_user_update(UpdateType::LogOut, &user).await; + nt.send_logout(&user, None).await; save_result } @@ -568,7 +580,7 @@ async fn post_email( let save_result = user.save(&mut conn).await; - nt.send_user_update(UpdateType::LogOut, &user).await; + nt.send_logout(&user, None).await; save_result } diff --git a/src/api/notifications.rs b/src/api/notifications.rs index b51e1380..b4dc55e9 100644 --- a/src/api/notifications.rs +++ b/src/api/notifications.rs @@ -170,6 +170,16 @@ impl WebSocketUsers { self.send_update(&user.uuid, &data).await; } + pub async fn send_logout(&self, user: &User, acting_device_uuid: Option) { + let data = create_update( + vec![("UserId".into(), user.uuid.clone().into()), ("Date".into(), serialize_date(user.updated_at))], + UpdateType::LogOut, + acting_device_uuid, + ); + + self.send_update(&user.uuid, &data).await; + } + pub async fn send_folder_update(&self, ut: UpdateType, folder: &Folder, acting_device_uuid: &String) { let data = create_update( vec![