Merge pull request #1243 from BlackDex/fix-key-rotate

Fix Key Rotation during password change.

migrations/mysql/2020-12-09-173101_add_stamp_exception/up.sql

@@ -0,0 +1 @@
+ALTER TABLE users ADD COLUMN stamp_exception TEXT DEFAULT NULL;

migrations/postgresql/2020-12-09-173101_add_stamp_exception/up.sql

@@ -0,0 +1 @@
+ALTER TABLE users ADD COLUMN stamp_exception TEXT DEFAULT NULL;

migrations/sqlite/2020-12-09-173101_add_stamp_exception/up.sql

@@ -0,0 +1 @@
+ALTER TABLE users ADD COLUMN stamp_exception TEXT DEFAULT NULL;

src/api/core/accounts.rs

@@ -115,7 +115,7 @@ fn register(data: JsonUpcase<RegisterData>, 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<ChangePassData>, 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<ChangeKdfData>, 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<KeyData>, 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<ChangeEmailData>, 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)

src/auth.rs

@@ -215,12 +215,10 @@ pub fn generate_admin_claims() -> AdminJWTClaims {
 //
 // Bearer token authentication
 //
-use rocket::{
+use rocket::request::{FromRequest, Outcome, Request};
-    request::{FromRequest, Request, Outcome},
-};
 
 use crate::db::{
-    models::{Device, User, UserOrgStatus, UserOrgType, UserOrganization, CollectionUser},
+    models::{CollectionUser, Device, User, UserOrgStatus, UserOrgType, UserOrganization, UserStampException},
     DbConn,
 };
 
@@ -298,8 +296,26 @@ impl<'a, 'r> FromRequest<'a, 'r> for Headers {
         };
 
         if user.security_stamp != claims.sstamp {
+            if let Some(stamp_exception) = user
+                .stamp_exception
+                .as_deref()
+                .and_then(|s| serde_json::from_str::<UserStampException>(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<Headers> for AdminHeaders {
     }
 }
 
-
-
-
-
 // col_id is usually the forth param ("/organizations/<org_id>/collections/<col_id>")
 // 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"),
                     }
 

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};

src/db/models/user.rs

@@ -37,6 +37,7 @@ db_object! {
         pub totp_recover: Option<String>,
 
         pub security_stamp: String,
+        pub stamp_exception: Option<String>,
 
         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<UserStampException> 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<UserStampException> and add multiple exceptions.
+    pub fn reset_stamp_exception(&mut self) {
+        self.stamp_exception = None;
+    }
 }
 
 use super::{Cipher, Device, Favorite, Folder, TwoFactor, UserOrgType, UserOrganization};

src/db/schemas/mysql/schema.rs

@@ -136,6 +136,7 @@ table! {
         totp_secret -> Nullable<Text>,
         totp_recover -> Nullable<Text>,
         security_stamp -> Text,
+        stamp_exception -> Nullable<Text>,
         equivalent_domains -> Text,
         excluded_globals -> Text,
         client_kdf_type -> Integer,

src/db/schemas/postgresql/schema.rs

@@ -136,6 +136,7 @@ table! {
         totp_secret -> Nullable<Text>,
         totp_recover -> Nullable<Text>,
         security_stamp -> Text,
+        stamp_exception -> Nullable<Text>,
         equivalent_domains -> Text,
         excluded_globals -> Text,
         client_kdf_type -> Integer,

src/db/schemas/sqlite/schema.rs

@@ -136,6 +136,7 @@ table! {
         totp_secret -> Nullable<Text>,
         totp_recover -> Nullable<Text>,
         security_stamp -> Text,
+        stamp_exception -> Nullable<Text>,
         equivalent_domains -> Text,
         excluded_globals -> Text,
         client_kdf_type -> Integer,