Allow applying unicode normalisation to passwords before hashing (#4599)

Author: sandhose

Cargo.lock

@@ -31,6 +31,7 @@ use mas_storage_pg::{DatabaseError, PgRepository};
 use rand::{RngCore, SeedableRng};
 use sqlx::{Acquire, types::Uuid};
 use tracing::{error, info, info_span, warn};
+use zeroize::Zeroizing;
 
 use crate::util::{
     database_connection_from_config, homeserver_connection_from_config,
@@ -210,7 +211,7 @@ impl Options {
                     return Ok(ExitCode::from(1));
                 }
 
-                let password = password.into_bytes().into();
+                let password = Zeroizing::new(password);
 
                 let (version, hashed_password) = password_manager.hash(&mut rng, password).await?;
 
@@ -566,7 +567,7 @@ impl Options {
 
                 // Hash the password if it's provided
                 let hashed_password = if let Some(password) = password {
-                    let password = password.into_bytes().into();
+                    let password = Zeroizing::new(password);
                     Some(password_manager.hash(&mut rng, password).await?)
                 } else {
                     None
@@ -641,7 +642,7 @@ impl Options {
                                     .interact()
                             })
                             .await??;
-                            let password = password.into_bytes().into();
+                            let password = Zeroizing::new(password);
                             req.hashed_password =
                                 Some(password_manager.hash(&mut rng, password).await?);
                         }

crates/cli/src/commands/manage.rs

@@ -36,20 +36,24 @@ pub async fn password_manager_from_config(
         return Ok(PasswordManager::disabled());
     }
 
-    let schemes = config
-        .load()
-        .await?
-        .into_iter()
-        .map(|(version, algorithm, cost, secret)| {
+    let schemes = config.load().await?.into_iter().map(
+        |(version, algorithm, cost, secret, unicode_normalization)| {
             use mas_handlers::passwords::Hasher;
             let hasher = match algorithm {
-                mas_config::PasswordAlgorithm::Pbkdf2 => Hasher::pbkdf2(secret),
-                mas_config::PasswordAlgorithm::Bcrypt => Hasher::bcrypt(cost, secret),
-                mas_config::PasswordAlgorithm::Argon2id => Hasher::argon2id(secret),
+                mas_config::PasswordAlgorithm::Pbkdf2 => {
+                    Hasher::pbkdf2(secret, unicode_normalization)
+                }
+                mas_config::PasswordAlgorithm::Bcrypt => {
+                    Hasher::bcrypt(cost, secret, unicode_normalization)
+                }
+                mas_config::PasswordAlgorithm::Argon2id => {
+                    Hasher::argon2id(secret, unicode_normalization)
+                }
             };
 
             (version, hasher)
-        });
+        },
+    );
 
     PasswordManager::new(config.minimum_complexity(), schemes)
 }
@@ -492,7 +496,7 @@ mod tests {
     #[tokio::test]
     async fn test_password_manager_from_config() {
         let mut rng = rand_chacha::ChaChaRng::seed_from_u64(42);
-        let password = Zeroizing::new(b"hunter2".to_vec());
+        let password = Zeroizing::new("hunter2".to_owned());
 
         // Test a valid, enabled config
         let config = serde_json::from_value(serde_json::json!({

crates/cli/src/util.rs

@@ -20,6 +20,7 @@ fn default_schemes() -> Vec<HashingScheme> {
         cost: None,
         secret: None,
         secret_file: None,
+        unicode_normalization: false,
     }]
 }
 
@@ -124,7 +125,7 @@ impl PasswordsConfig {
     /// not be read.
     pub async fn load(
         &self,
-    ) -> Result<Vec<(u16, Algorithm, Option<u32>, Option<Vec<u8>>)>, anyhow::Error> {
+    ) -> Result<Vec<(u16, Algorithm, Option<u32>, Option<Vec<u8>>, bool)>, anyhow::Error> {
         let mut schemes: Vec<&HashingScheme> = self.schemes.iter().collect();
         schemes.sort_unstable_by_key(|a| Reverse(a.version));
         schemes.dedup_by_key(|a| a.version);
@@ -151,13 +152,24 @@ impl PasswordsConfig {
                 (None, None) => None,
             };
 
-            mapped_result.push((scheme.version, scheme.algorithm, scheme.cost, secret));
+            mapped_result.push((
+                scheme.version,
+                scheme.algorithm,
+                scheme.cost,
+                secret,
+                scheme.unicode_normalization,
+            ));
         }
 
         Ok(mapped_result)
     }
 }
 
+#[allow(clippy::trivially_copy_pass_by_ref)]
+const fn is_default_false(value: &bool) -> bool {
+    !*value
+}
+
 /// Parameters for a password hashing scheme
 #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)]
 pub struct HashingScheme {
@@ -168,6 +180,14 @@ pub struct HashingScheme {
     /// The hashing algorithm to use
     pub algorithm: Algorithm,
 
+    /// Whether to apply Unicode normalization to the password before hashing
+    ///
+    /// Defaults to `false`, and generally recommended to stay false. This is
+    /// although recommended when importing password hashs from Synapse, as it
+    /// applies an NFKC normalization to the password before hashing it.
+    #[serde(default, skip_serializing_if = "is_default_false")]
+    pub unicode_normalization: bool,
+
     /// Cost for the bcrypt algorithm
     #[serde(skip_serializing_if = "Option::is_none")]
     #[schemars(default = "default_bcrypt_cost")]

crates/config/src/sections/passwords.rs

@@ -74,6 +74,7 @@ chrono.workspace = true
 elliptic-curve.workspace = true
 hex.workspace = true
 governor.workspace = true
+icu_normalizer = "1.5.0"
 indexmap.workspace = true
 pkcs8.workspace = true
 psl = "2.1.111"

crates/handlers/Cargo.toml

@@ -122,7 +122,7 @@ pub async fn handler(
         return Err(RouteError::PasswordTooWeak);
     }
 
-    let password = Zeroizing::new(params.password.into_bytes());
+    let password = Zeroizing::new(params.password);
     let (version, hashed_password) = password_manager
         .hash(&mut rng, password)
         .await
@@ -184,7 +184,7 @@ mod tests {
         // Check that the user now has a password
         let mut repo = state.repository().await.unwrap();
         let user_password = repo.user_password().active(&user).await.unwrap().unwrap();
-        let password = Zeroizing::new(b"this is a good enough password".to_vec());
+        let password = Zeroizing::new(String::from("this is a good enough password"));
         state
             .password_manager
             .verify(
@@ -243,7 +243,7 @@ mod tests {
         // Check that the user now has a password
         let mut repo = state.repository().await.unwrap();
         let user_password = repo.user_password().active(&user).await.unwrap().unwrap();
-        let password = Zeroizing::new(b"password".to_vec());
+        let password = Zeroizing::new("password".to_owned());
         state
             .password_manager
             .verify(

crates/handlers/src/admin/v1/users/set_password.rs

@@ -574,7 +574,7 @@ async fn user_password_login(
         .ok_or(RouteError::NoPassword)?;
 
     // Verify the password
-    let password = Zeroizing::new(password.into_bytes());
+    let password = Zeroizing::new(password);
 
     let new_password_hash = password_manager
         .verify_and_upgrade(
@@ -787,7 +787,7 @@ mod tests {
             .unwrap();
         let (version, hash) = state
             .password_manager
-            .hash(&mut rng, Zeroizing::new(password.as_bytes().to_vec()))
+            .hash(&mut rng, Zeroizing::new(password.to_owned()))
             .await
             .unwrap();
 

crates/handlers/src/compat/login.rs

@@ -76,7 +76,7 @@ async fn verify_password_if_needed(
         return Ok(false);
     };
 
-    let password = Zeroizing::new(password.into_bytes());
+    let password = Zeroizing::new(password);
 
     let res = password_manager
         .verify(

crates/handlers/src/graphql/mutations/mod.rs

@@ -742,7 +742,7 @@ impl UserMutations {
             if let Err(_err) = password_manager
                 .verify(
                     active_password.version,
-                    Zeroizing::new(current_password_attempt.into_bytes()),
+                    Zeroizing::new(current_password_attempt),
                     active_password.hashed_password,
                 )
                 .await
@@ -754,7 +754,7 @@ impl UserMutations {
         }
 
         let (new_password_version, new_password_hash) = password_manager
-            .hash(state.rng(), Zeroizing::new(input.new_password.into_bytes()))
+            .hash(state.rng(), Zeroizing::new(input.new_password))
             .await?;
 
         repo.user_password()
@@ -849,7 +849,7 @@ impl UserMutations {
         }
 
         let (new_password_version, new_password_hash) = password_manager
-            .hash(state.rng(), Zeroizing::new(input.new_password.into_bytes()))
+            .hash(state.rng(), Zeroizing::new(input.new_password))
             .await?;
 
         repo.user_password()

crates/handlers/src/graphql/mutations/user.rs

@@ -844,7 +844,7 @@ mod tests {
 
         let (version, hashed_password) = state
             .password_manager
-            .hash(&mut state.rng(), Zeroizing::new(b"password".to_vec()))
+            .hash(&mut state.rng(), Zeroizing::new("password".to_owned()))
             .await
             .unwrap();