diff --git a/Cargo.lock b/Cargo.lock index d5277158d..49a8c4978 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3312,6 +3312,7 @@ dependencies = [ "headers", "hex", "hyper", + "icu_normalizer", "indexmap 2.9.0", "insta", "lettre", diff --git a/crates/cli/src/commands/manage.rs b/crates/cli/src/commands/manage.rs index 390897ce7..b52c56162 100644 --- a/crates/cli/src/commands/manage.rs +++ b/crates/cli/src/commands/manage.rs @@ -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?); } diff --git a/crates/cli/src/util.rs b/crates/cli/src/util.rs index 84e94aeea..3588b6895 100644 --- a/crates/cli/src/util.rs +++ b/crates/cli/src/util.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!({ diff --git a/crates/config/src/sections/passwords.rs b/crates/config/src/sections/passwords.rs index 07ea71b0e..a72f2efd7 100644 --- a/crates/config/src/sections/passwords.rs +++ b/crates/config/src/sections/passwords.rs @@ -20,6 +20,7 @@ fn default_schemes() -> Vec { 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, Option>)>, anyhow::Error> { + ) -> Result, Option>, 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")] diff --git a/crates/handlers/Cargo.toml b/crates/handlers/Cargo.toml index 0de09fb62..37241ecef 100644 --- a/crates/handlers/Cargo.toml +++ b/crates/handlers/Cargo.toml @@ -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" diff --git a/crates/handlers/src/admin/v1/users/set_password.rs b/crates/handlers/src/admin/v1/users/set_password.rs index 2d83e39d9..06797c3f8 100644 --- a/crates/handlers/src/admin/v1/users/set_password.rs +++ b/crates/handlers/src/admin/v1/users/set_password.rs @@ -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( diff --git a/crates/handlers/src/compat/login.rs b/crates/handlers/src/compat/login.rs index 3dbbb0d93..32bdfacd3 100644 --- a/crates/handlers/src/compat/login.rs +++ b/crates/handlers/src/compat/login.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(); diff --git a/crates/handlers/src/graphql/mutations/mod.rs b/crates/handlers/src/graphql/mutations/mod.rs index 66bb5766c..21fca3d6c 100644 --- a/crates/handlers/src/graphql/mutations/mod.rs +++ b/crates/handlers/src/graphql/mutations/mod.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( diff --git a/crates/handlers/src/graphql/mutations/user.rs b/crates/handlers/src/graphql/mutations/user.rs index 301307d96..f4ae549b9 100644 --- a/crates/handlers/src/graphql/mutations/user.rs +++ b/crates/handlers/src/graphql/mutations/user.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() diff --git a/crates/handlers/src/oauth2/introspection.rs b/crates/handlers/src/oauth2/introspection.rs index 9b3e50e7c..3ae4a3a1c 100644 --- a/crates/handlers/src/oauth2/introspection.rs +++ b/crates/handlers/src/oauth2/introspection.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(); diff --git a/crates/handlers/src/passwords.rs b/crates/handlers/src/passwords.rs index eba028661..f4ce10adb 100644 --- a/crates/handlers/src/passwords.rs +++ b/crates/handlers/src/passwords.rs @@ -1,4 +1,4 @@ -// Copyright 2024 New Vector Ltd. +// Copyright 2024, 2025 New Vector Ltd. // Copyright 2022-2024 The Matrix.org Foundation C.I.C. // // SPDX-License-Identifier: AGPL-3.0-only @@ -116,7 +116,7 @@ impl PasswordManager { pub async fn hash( &self, rng: R, - password: Zeroizing>, + password: Zeroizing, ) -> Result<(SchemeVersion, String), anyhow::Error> { let inner = self.get_inner()?; @@ -130,7 +130,7 @@ impl PasswordManager { let version = inner.current_version; let hashed = tokio::task::spawn_blocking(move || { - span.in_scope(move || inner.current_hasher.hash_blocking(rng, &password)) + span.in_scope(move || inner.current_hasher.hash_blocking(rng, password)) }) .await??; @@ -147,7 +147,7 @@ impl PasswordManager { pub async fn verify( &self, scheme: SchemeVersion, - password: Zeroizing>, + password: Zeroizing, hashed_password: String, ) -> Result<(), anyhow::Error> { let inner = self.get_inner()?; @@ -164,7 +164,7 @@ impl PasswordManager { .context("Hashing scheme not found")? }; - hasher.verify_blocking(&hashed_password, &password) + hasher.verify_blocking(&hashed_password, password) }) }) .await??; @@ -184,7 +184,7 @@ impl PasswordManager { &self, rng: R, scheme: SchemeVersion, - password: Zeroizing>, + password: Zeroizing, hashed_password: String, ) -> Result, anyhow::Error> { let inner = self.get_inner()?; @@ -209,43 +209,78 @@ impl PasswordManager { /// A hashing scheme, with an optional pepper pub struct Hasher { algorithm: Algorithm, + unicode_normalization: bool, pepper: Option>, } impl Hasher { /// Creates a new hashing scheme based on the bcrypt algorithm #[must_use] - pub const fn bcrypt(cost: Option, pepper: Option>) -> Self { + pub const fn bcrypt( + cost: Option, + pepper: Option>, + unicode_normalization: bool, + ) -> Self { let algorithm = Algorithm::Bcrypt { cost }; - Self { algorithm, pepper } + Self { + algorithm, + unicode_normalization, + pepper, + } } /// Creates a new hashing scheme based on the argon2id algorithm #[must_use] - pub const fn argon2id(pepper: Option>) -> Self { + pub const fn argon2id(pepper: Option>, unicode_normalization: bool) -> Self { let algorithm = Algorithm::Argon2id; - Self { algorithm, pepper } + Self { + algorithm, + unicode_normalization, + pepper, + } } /// Creates a new hashing scheme based on the pbkdf2 algorithm #[must_use] - pub const fn pbkdf2(pepper: Option>) -> Self { + pub const fn pbkdf2(pepper: Option>, unicode_normalization: bool) -> Self { let algorithm = Algorithm::Pbkdf2; - Self { algorithm, pepper } + Self { + algorithm, + unicode_normalization, + pepper, + } + } + + fn normalize_password(&self, password: Zeroizing) -> Zeroizing { + if self.unicode_normalization { + // This is the normalization method used by Synapse + let normalizer = icu_normalizer::ComposingNormalizer::new_nfkc(); + Zeroizing::new(normalizer.normalize(&password)) + } else { + password + } } fn hash_blocking( &self, rng: R, - password: &[u8], + password: Zeroizing, ) -> Result { + let password = self.normalize_password(password); + self.algorithm - .hash_blocking(rng, password, self.pepper.as_deref()) + .hash_blocking(rng, password.as_bytes(), self.pepper.as_deref()) } - fn verify_blocking(&self, hashed_password: &str, password: &[u8]) -> Result<(), anyhow::Error> { + fn verify_blocking( + &self, + hashed_password: &str, + password: Zeroizing, + ) -> Result<(), anyhow::Error> { + let password = self.normalize_password(password); + self.algorithm - .verify_blocking(hashed_password, password, self.pepper.as_deref()) + .verify_blocking(hashed_password, password.as_bytes(), self.pepper.as_deref()) } } @@ -461,8 +496,8 @@ mod tests { // after changing the hashing schemes. The salt generation is done with a seeded // RNG, so that we can do stable snapshots of hashed passwords let mut rng = rand_chacha::ChaChaRng::seed_from_u64(42); - let password = Zeroizing::new(b"hunter2".to_vec()); - let wrong_password = Zeroizing::new(b"wrong-password".to_vec()); + let password = Zeroizing::new("hunter2".to_owned()); + let wrong_password = Zeroizing::new("wrong-password".to_owned()); let manager = PasswordManager::new( 0, @@ -470,7 +505,7 @@ mod tests { // Start with one hashing scheme: the one used by synapse, bcrypt + pepper ( 1, - Hasher::bcrypt(Some(10), Some(b"a-secret-pepper".to_vec())), + Hasher::bcrypt(Some(10), Some(b"a-secret-pepper".to_vec()), false), ), ], ) @@ -519,10 +554,10 @@ mod tests { let manager = PasswordManager::new( 0, [ - (2, Hasher::argon2id(None)), + (2, Hasher::argon2id(None, false)), ( 1, - Hasher::bcrypt(Some(10), Some(b"a-secret-pepper".to_vec())), + Hasher::bcrypt(Some(10), Some(b"a-secret-pepper".to_vec()), false), ), ], ) @@ -575,11 +610,14 @@ mod tests { let manager = PasswordManager::new( 0, [ - (3, Hasher::argon2id(Some(b"a-secret-pepper".to_vec()))), - (2, Hasher::argon2id(None)), + ( + 3, + Hasher::argon2id(Some(b"a-secret-pepper".to_vec()), false), + ), + (2, Hasher::argon2id(None, false)), ( 1, - Hasher::bcrypt(Some(10), Some(b"a-secret-pepper".to_vec())), + Hasher::bcrypt(Some(10), Some(b"a-secret-pepper".to_vec()), false), ), ], ) diff --git a/crates/handlers/src/test_utils.rs b/crates/handlers/src/test_utils.rs index d69bda190..cdbc981d1 100644 --- a/crates/handlers/src/test_utils.rs +++ b/crates/handlers/src/test_utils.rs @@ -194,7 +194,7 @@ impl TestState { let password_manager = if site_config.password_login_enabled { PasswordManager::new( site_config.minimum_password_complexity, - [(1, Hasher::argon2id(None))], + [(1, Hasher::argon2id(None, false))], )? } else { PasswordManager::disabled() diff --git a/crates/handlers/src/views/login.rs b/crates/handlers/src/views/login.rs index 87884ddba..1db44285a 100644 --- a/crates/handlers/src/views/login.rs +++ b/crates/handlers/src/views/login.rs @@ -244,7 +244,7 @@ pub(crate) async fn post( .await; }; - let password = Zeroizing::new(form.password.as_bytes().to_vec()); + let password = Zeroizing::new(form.password); // Verify the password, and upgrade it on-the-fly if needed let user_password = match password_manager @@ -581,7 +581,7 @@ mod test { .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(); repo.user_password() diff --git a/crates/handlers/src/views/register/password.rs b/crates/handlers/src/views/register/password.rs index 8c2925505..0745567a9 100644 --- a/crates/handlers/src/views/register/password.rs +++ b/crates/handlers/src/views/register/password.rs @@ -364,7 +364,7 @@ pub(crate) async fn post( .await?; // Hash the password - let password = Zeroizing::new(form.password.into_bytes()); + let password = Zeroizing::new(form.password); let (version, hashed_password) = password_manager .hash(&mut rng, password) .await diff --git a/crates/syn2mas/src/synapse_reader/checks.rs b/crates/syn2mas/src/synapse_reader/checks.rs index 0969d3787..a78f18b1d 100644 --- a/crates/syn2mas/src/synapse_reader/checks.rs +++ b/crates/syn2mas/src/synapse_reader/checks.rs @@ -199,9 +199,9 @@ pub async fn synapse_config_check_against_mas_config( // Look for the MAS password hashing scheme that will be used for imported // Synapse passwords, then check the configuration matches so that Synapse // passwords will be compatible with MAS. - if let Some((_, algorithm, _, secret)) = mas_password_schemes + if let Some((_, algorithm, _, secret, _)) = mas_password_schemes .iter() - .find(|(version, _, _, _)| *version == MIGRATED_PASSWORD_VERSION) + .find(|(version, _, _, _, _)| *version == MIGRATED_PASSWORD_VERSION) { if algorithm != &PasswordAlgorithm::Bcrypt { errors.push(CheckError::PasswordSchemeNotBcrypt); diff --git a/crates/syn2mas/src/synapse_reader/config/mod.rs b/crates/syn2mas/src/synapse_reader/config/mod.rs index 3abf2e567..25a4c0d93 100644 --- a/crates/syn2mas/src/synapse_reader/config/mod.rs +++ b/crates/syn2mas/src/synapse_reader/config/mod.rs @@ -179,6 +179,7 @@ impl Config { cost: self.bcrypt_rounds, secret: self.password_config.pepper, secret_file: None, + unicode_normalization: true, }, // Use the default algorithm MAS uses as a second hashing scheme, so that users // will get their password hash upgraded to a more modern algorithm over time @@ -188,6 +189,7 @@ impl Config { cost: None, secret: None, secret_file: None, + unicode_normalization: false, }, ]; diff --git a/docs/config.schema.json b/docs/config.schema.json index def447302..3bc0f407d 100644 --- a/docs/config.schema.json +++ b/docs/config.schema.json @@ -1613,6 +1613,10 @@ } ] }, + "unicode_normalization": { + "description": "Whether to apply Unicode normalization to the password before hashing\n\nDefaults 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.", + "type": "boolean" + }, "cost": { "description": "Cost for the bcrypt algorithm", "default": 12, diff --git a/docs/setup/migration.md b/docs/setup/migration.md index a10a9bbd1..6cd8ce98a 100644 --- a/docs/setup/migration.md +++ b/docs/setup/migration.md @@ -47,7 +47,7 @@ When using this tool, be careful to examine the log output for any warnings abou #### Local passwords Synapse uses bcrypt as its password hashing scheme, while MAS defaults to using the newer argon2id. -You will have to configure the version 1 scheme as bcrypt for migrated passwords to work. +You will have to configure the version 1 scheme as bcrypt with `unicode_normalization: true` for migrated passwords to work. It is also recommended that you keep argon2id as version 2 so that once users log in, their hashes will be updated to the newer, recommended scheme. Example passwords configuration: @@ -57,6 +57,7 @@ passwords: schemes: - version: 1 algorithm: bcrypt + unicode_normalization: true # Optional, must match the `password_config.pepper` in the Synapse config #secret: secretPepperValue - version: 2