From de86aa671eec9d08ab0e0d4cdd30584606882732 Mon Sep 17 00:00:00 2001 From: BlackDex Date: Mon, 14 Dec 2020 19:58:23 +0100 Subject: [PATCH] Fix Key Rotation during password change When ticking the 'Also rotate my account's encryption key' box, the key rotated ciphers are posted after the change of password. During the password change the security stamp was reseted which made the posted key's return an invalid auth. This reset is needed to prevent other clients from still being able to read/write. This fixes this by adding a new database column which stores a stamp exception which includes the allowed route and the current security stamp before it gets reseted. When the security stamp check fails it will check if there is a stamp exception and tries to match the route and security stamp. Currently it only allows for one exception. But if needed we could expand it by using a Vec and change the functions accordingly. fixes #1240 --- .../down.sql | 0 .../up.sql | 1 + .../down.sql | 0 .../up.sql | 1 + .../down.sql | 0 .../up.sql | 1 + src/api/core/accounts.rs | 9 ++-- src/auth.rs | 32 ++++++++---- src/db/models/mod.rs | 2 +- src/db/models/user.rs | 52 +++++++++++++++++-- src/db/schemas/mysql/schema.rs | 1 + src/db/schemas/postgresql/schema.rs | 1 + src/db/schemas/sqlite/schema.rs | 1 + 13 files changed, 83 insertions(+), 18 deletions(-) create mode 100644 migrations/mysql/2020-12-09-173101_add_stamp_exception/down.sql create mode 100644 migrations/mysql/2020-12-09-173101_add_stamp_exception/up.sql create mode 100644 migrations/postgresql/2020-12-09-173101_add_stamp_exception/down.sql create mode 100644 migrations/postgresql/2020-12-09-173101_add_stamp_exception/up.sql create mode 100644 migrations/sqlite/2020-12-09-173101_add_stamp_exception/down.sql create mode 100644 migrations/sqlite/2020-12-09-173101_add_stamp_exception/up.sql diff --git a/migrations/mysql/2020-12-09-173101_add_stamp_exception/down.sql b/migrations/mysql/2020-12-09-173101_add_stamp_exception/down.sql new file mode 100644 index 0000000..e69de29 diff --git a/migrations/mysql/2020-12-09-173101_add_stamp_exception/up.sql b/migrations/mysql/2020-12-09-173101_add_stamp_exception/up.sql new file mode 100644 index 0000000..bcad392 --- /dev/null +++ b/migrations/mysql/2020-12-09-173101_add_stamp_exception/up.sql @@ -0,0 +1 @@ +ALTER TABLE users ADD COLUMN stamp_exception TEXT DEFAULT NULL; \ No newline at end of file diff --git a/migrations/postgresql/2020-12-09-173101_add_stamp_exception/down.sql b/migrations/postgresql/2020-12-09-173101_add_stamp_exception/down.sql new file mode 100644 index 0000000..e69de29 diff --git a/migrations/postgresql/2020-12-09-173101_add_stamp_exception/up.sql b/migrations/postgresql/2020-12-09-173101_add_stamp_exception/up.sql new file mode 100644 index 0000000..bcad392 --- /dev/null +++ b/migrations/postgresql/2020-12-09-173101_add_stamp_exception/up.sql @@ -0,0 +1 @@ +ALTER TABLE users ADD COLUMN stamp_exception TEXT DEFAULT NULL; \ No newline at end of file diff --git a/migrations/sqlite/2020-12-09-173101_add_stamp_exception/down.sql b/migrations/sqlite/2020-12-09-173101_add_stamp_exception/down.sql new file mode 100644 index 0000000..e69de29 diff --git a/migrations/sqlite/2020-12-09-173101_add_stamp_exception/up.sql b/migrations/sqlite/2020-12-09-173101_add_stamp_exception/up.sql new file mode 100644 index 0000000..bcad392 --- /dev/null +++ b/migrations/sqlite/2020-12-09-173101_add_stamp_exception/up.sql @@ -0,0 +1 @@ +ALTER TABLE users ADD COLUMN stamp_exception TEXT DEFAULT NULL; \ No newline at end of file diff --git a/src/api/core/accounts.rs b/src/api/core/accounts.rs index 9068513..9131575 100644 --- a/src/api/core/accounts.rs +++ b/src/api/core/accounts.rs @@ -115,7 +115,7 @@ fn register(data: JsonUpcase, conn: DbConn) -> EmptyResult { user.client_kdf_type = client_kdf_type; } - user.set_password(&data.MasterPasswordHash); + user.set_password(&data.MasterPasswordHash, None); user.akey = data.Key; // Add extra fields if present @@ -232,7 +232,7 @@ fn post_password(data: JsonUpcase, headers: Headers, conn: DbCon err!("Invalid password") } - user.set_password(&data.NewMasterPasswordHash); + user.set_password(&data.NewMasterPasswordHash, Some("post_rotatekey")); user.akey = data.Key; user.save(&conn) } @@ -259,7 +259,7 @@ fn post_kdf(data: JsonUpcase, headers: Headers, conn: DbConn) -> user.client_kdf_iter = data.KdfIterations; user.client_kdf_type = data.Kdf; - user.set_password(&data.NewMasterPasswordHash); + user.set_password(&data.NewMasterPasswordHash, None); user.akey = data.Key; user.save(&conn) } @@ -338,6 +338,7 @@ fn post_rotatekey(data: JsonUpcase, headers: Headers, conn: DbConn, nt: user.akey = data.Key; user.private_key = Some(data.PrivateKey); user.reset_security_stamp(); + user.reset_stamp_exception(); user.save(&conn) } @@ -445,7 +446,7 @@ fn post_email(data: JsonUpcase, headers: Headers, conn: DbConn) user.email_new = None; user.email_new_token = None; - user.set_password(&data.NewMasterPasswordHash); + user.set_password(&data.NewMasterPasswordHash, None); user.akey = data.Key; user.save(&conn) diff --git a/src/auth.rs b/src/auth.rs index 53a2535..667783c 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -215,12 +215,10 @@ pub fn generate_admin_claims() -> AdminJWTClaims { // // Bearer token authentication // -use rocket::{ - request::{FromRequest, Request, Outcome}, -}; +use rocket::request::{FromRequest, Outcome, Request}; use crate::db::{ - models::{Device, User, UserOrgStatus, UserOrgType, UserOrganization, CollectionUser}, + models::{CollectionUser, Device, User, UserOrgStatus, UserOrgType, UserOrganization, UserStampException}, DbConn, }; @@ -298,7 +296,25 @@ impl<'a, 'r> FromRequest<'a, 'r> for Headers { }; if user.security_stamp != claims.sstamp { - err_handler!("Invalid security stamp") + if let Some(stamp_exception) = user + .stamp_exception + .as_deref() + .and_then(|s| serde_json::from_str::(s).ok()) + { + let current_route = match request.route().and_then(|r| r.name) { + Some(name) => name, + _ => err_handler!("Error getting current route for stamp exception"), + }; + + // Check if both match, if not this route is not allowed with the current security stamp. + if stamp_exception.route != current_route { + err_handler!("Invalid security stamp: Current route and exception route do not match") + } else if stamp_exception.security_stamp != claims.sstamp { + err_handler!("Invalid security stamp for matched stamp exception") + } + } else { + err_handler!("Invalid security stamp") + } } Outcome::Success(Headers { host, device, user }) @@ -423,10 +439,6 @@ impl Into for AdminHeaders { } } - - - - // col_id is usually the forth param ("/organizations//collections/") // But there cloud be cases where it is located in a query value. // First check the param, if this is not a valid uuid, we will try the query value. @@ -478,7 +490,7 @@ impl<'a, 'r> FromRequest<'a, 'r> for ManagerHeaders { None => err_handler!("The current user isn't a manager for this collection"), } } - }, + } _ => err_handler!("Error getting the collection id"), } diff --git a/src/db/models/mod.rs b/src/db/models/mod.rs index 1e5349f..8c886fb 100644 --- a/src/db/models/mod.rs +++ b/src/db/models/mod.rs @@ -18,4 +18,4 @@ pub use self::folder::{Folder, FolderCipher}; pub use self::org_policy::{OrgPolicy, OrgPolicyType}; pub use self::organization::{Organization, UserOrgStatus, UserOrgType, UserOrganization}; pub use self::two_factor::{TwoFactor, TwoFactorType}; -pub use self::user::{Invitation, User}; +pub use self::user::{Invitation, User, UserStampException}; diff --git a/src/db/models/user.rs b/src/db/models/user.rs index a2473c0..eab46e5 100644 --- a/src/db/models/user.rs +++ b/src/db/models/user.rs @@ -37,6 +37,7 @@ db_object! { pub totp_recover: Option, pub security_stamp: String, + pub stamp_exception: Option, pub equivalent_domains: String, pub excluded_globals: String, @@ -60,6 +61,12 @@ enum UserStatus { _Disabled = 2, } +#[derive(Serialize, Deserialize)] +pub struct UserStampException { + pub route: String, + pub security_stamp: String +} + /// Local methods impl User { pub const CLIENT_KDF_TYPE_DEFAULT: i32 = 0; // PBKDF2: 0 @@ -88,6 +95,7 @@ impl User { password_iterations: CONFIG.password_iterations(), security_stamp: crate::util::get_uuid(), + stamp_exception: None, password_hint: None, private_key: None, @@ -121,14 +129,52 @@ impl User { } } - pub fn set_password(&mut self, password: &str) { + /// Set the password hash generated + /// And resets the security_stamp. Based upon the allow_next_route the security_stamp will be different. + /// + /// # Arguments + /// + /// * `password` - A str which contains a hashed version of the users master password. + /// * `allow_next_route` - A Option<&str> with the function name of the next allowed (rocket) route. + /// + pub fn set_password(&mut self, password: &str, allow_next_route: Option<&str>) { self.password_hash = crypto::hash_password(password.as_bytes(), &self.salt, self.password_iterations as u32); - self.reset_security_stamp(); + + if let Some(route) = allow_next_route { + self.set_stamp_exception(route); + } + + self.reset_security_stamp() } pub fn reset_security_stamp(&mut self) { self.security_stamp = crate::util::get_uuid(); } + + /// Set the stamp_exception to only allow a subsequent request matching a specific route using the current security-stamp. + /// + /// # Arguments + /// * `route_exception` - A str with the function name of the next allowed (rocket) route. + /// + /// ### Future + /// In the future it could be posible that we need more of these exception routes. + /// In that case we could use an Vec and add multiple exceptions. + pub fn set_stamp_exception(&mut self, route_exception: &str) { + let stamp_exception = UserStampException { + route: route_exception.to_string(), + security_stamp: self.security_stamp.to_string() + }; + self.stamp_exception = Some(serde_json::to_string(&stamp_exception).unwrap_or_default()); + } + + /// Resets the stamp_exception to prevent re-use of the previous security-stamp + /// + /// ### Future + /// In the future it could be posible that we need more of these exception routes. + /// In that case we could use an Vec and add multiple exceptions. + pub fn reset_stamp_exception(&mut self) { + self.stamp_exception = None; + } } use super::{Cipher, Device, Favorite, Folder, TwoFactor, UserOrgType, UserOrganization}; @@ -295,7 +341,7 @@ impl User { match Device::find_latest_active_by_user(&self.uuid, conn) { Some(device) => Some(device.updated_at), None => None - } + } } } diff --git a/src/db/schemas/mysql/schema.rs b/src/db/schemas/mysql/schema.rs index cc91a73..d1a9962 100644 --- a/src/db/schemas/mysql/schema.rs +++ b/src/db/schemas/mysql/schema.rs @@ -136,6 +136,7 @@ table! { totp_secret -> Nullable, totp_recover -> Nullable, security_stamp -> Text, + stamp_exception -> Nullable, equivalent_domains -> Text, excluded_globals -> Text, client_kdf_type -> Integer, diff --git a/src/db/schemas/postgresql/schema.rs b/src/db/schemas/postgresql/schema.rs index 69571e8..5af0ece 100644 --- a/src/db/schemas/postgresql/schema.rs +++ b/src/db/schemas/postgresql/schema.rs @@ -136,6 +136,7 @@ table! { totp_secret -> Nullable, totp_recover -> Nullable, security_stamp -> Text, + stamp_exception -> Nullable, equivalent_domains -> Text, excluded_globals -> Text, client_kdf_type -> Integer, diff --git a/src/db/schemas/sqlite/schema.rs b/src/db/schemas/sqlite/schema.rs index 69571e8..5af0ece 100644 --- a/src/db/schemas/sqlite/schema.rs +++ b/src/db/schemas/sqlite/schema.rs @@ -136,6 +136,7 @@ table! { totp_secret -> Nullable, totp_recover -> Nullable, security_stamp -> Text, + stamp_exception -> Nullable, equivalent_domains -> Text, excluded_globals -> Text, client_kdf_type -> Integer,