From c4a3fa4f04afd3ad3e4bb900ed788589ae5b26db Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Mon, 9 Nov 2026 16:32:30 +0900 Subject: [PATCH 1/2] fix(server): accept $2x$ in hash check and equalize timing for unusable hashes reload_users warned that users with a $2x$ bcrypt hash cannot log in, although verify_internal dispatches every $2 hash to bcrypt, which accepts version 2x. Add $2x$ to is_supported_hash_format, the warning text, and the UserDefinition::password_hash docs. Dispatch is unchanged. Users whose stored hash is unusable (unsupported prefix, malformed Argon2, malformed bcrypt) returned without any hash work, unlike unknown users. Call verify_dummy_hash on those three paths so they cost the same as other failures; the malformed Argon2 case still returns the same Err and the auth provider still rejects. A cfg(test) counter on verify_dummy_hash lets tests assert this deterministically instead of by wall-clock time. Tests: cargo test --lib server::auth::password, cargo clippy --lib --tests and --lib with -D warnings, cargo fmt --check. Closes #356 --- src/server/auth/password.rs | 132 ++++++++++++++++++++++++++++++++++-- src/server/config/types.rs | 2 +- 2 files changed, 129 insertions(+), 5 deletions(-) diff --git a/src/server/auth/password.rs b/src/server/auth/password.rs index 808b1ff6..92539732 100644 --- a/src/server/auth/password.rs +++ b/src/server/auth/password.rs @@ -114,6 +114,11 @@ pub struct PasswordVerifier { /// Pre-computed dummy hash for timing attack mitigation. /// This hash is verified against when the user doesn't exist. dummy_hash: String, + + /// Number of `verify_dummy_hash` calls, used by tests to check that every + /// failure path does the same hashing work without relying on wall-clock time. + #[cfg(test)] + dummy_hash_calls: std::sync::atomic::AtomicUsize, } impl PasswordVerifier { @@ -136,6 +141,8 @@ impl PasswordVerifier { config, users: RwLock::new(HashMap::new()), dummy_hash, + #[cfg(test)] + dummy_hash_calls: std::sync::atomic::AtomicUsize::new(0), }; verifier.reload_users().await?; @@ -188,7 +195,7 @@ impl PasswordVerifier { if !is_supported_hash_format(&user.password_hash) { tracing::warn!( user = %name, - "Unsupported password hash format; supported formats are Argon2id (bssh-server hash-password) and bcrypt ($2a$/$2b$/$2y$). This user cannot log in with a password." + "Unsupported password hash format; supported formats are Argon2id (bssh-server hash-password) and bcrypt ($2a$/$2b$/$2x$/$2y$). This user cannot log in with a password." ); } } @@ -274,15 +281,25 @@ impl PasswordVerifier { // Try Argon2id first, then fall back to bcrypt for compatibility let verified = if hash_str.starts_with("$argon2") { - self.verify_argon2(password.as_bytes(), hash_str)? + match self.verify_argon2(password.as_bytes(), hash_str) { + Ok(v) => v, + Err(e) => { + // Malformed hash fails fast; do the dummy work so the + // response time matches users with a usable hash. + let _ = self.verify_dummy_hash(password); + return Err(e); + } + } } else if hash_str.starts_with("$2") { - // bcrypt hash format ($2a$, $2b$, $2y$) + // bcrypt hash format ($2a$, $2b$, $2x$, $2y$) self.verify_bcrypt(password, hash_str)? } else { tracing::warn!( user = %validated_username, "Unknown password hash format" ); + // No hash work happened for this user; match the other failure paths. + let _ = self.verify_dummy_hash(password); return Ok(false); }; @@ -321,6 +338,8 @@ impl PasswordVerifier { Ok(verified) => Ok(verified), Err(e) => { tracing::warn!(error = %e, "bcrypt verification error"); + // Malformed hash fails before any hashing; do the dummy work. + let _ = self.verify_dummy_hash(password); Ok(false) } } @@ -331,6 +350,9 @@ impl PasswordVerifier { /// This ensures that verification takes the same amount of time /// regardless of whether the user exists. fn verify_dummy_hash(&self, password: &Zeroizing) -> bool { + #[cfg(test)] + self.dummy_hash_calls + .fetch_add(1, std::sync::atomic::Ordering::Relaxed); // Verify against the pre-computed dummy hash if let Ok(hash) = PasswordHash::new(&self.dummy_hash) { let argon2 = Argon2::default(); @@ -504,7 +526,10 @@ fn is_supported_hash_format(hash: &str) -> bool { if hash.starts_with("$argon2") { return PasswordHash::new(hash).is_ok(); } - hash.starts_with("$2a$") || hash.starts_with("$2b$") || hash.starts_with("$2y$") + hash.starts_with("$2a$") + || hash.starts_with("$2b$") + || hash.starts_with("$2x$") + || hash.starts_with("$2y$") } #[cfg(test)] @@ -815,6 +840,7 @@ mod tests { fn test_is_supported_hash_format() { assert!(is_supported_hash_format(&hash_password("pw").unwrap())); assert!(is_supported_hash_format(&bcrypt::hash("pw", 4).unwrap())); + assert!(is_supported_hash_format(&bcrypt_2x_hash("pw2x"))); for bad in [ "$6$rounds=5000$salt$hash", "$5$salt$hash", @@ -827,6 +853,12 @@ mod tests { } } + /// Build a `$2x$` hash from a cost-4 bcrypt hash by swapping the version. + fn bcrypt_2x_hash(password: &str) -> String { + let hash = bcrypt::hash(password, 4).unwrap(); + format!("$2x{}", &hash[3..]) + } + fn user_with_hash(name: &str, hash: String) -> UserDefinition { UserDefinition { name: name.to_string(), @@ -876,4 +908,96 @@ mod tests { assert!(verifier.verify("bcryptuser", "bcrypt_pw").await.unwrap()); assert!(!verifier.verify("bcryptuser", "wrong").await.unwrap()); } + + #[tokio::test] + async fn test_bcrypt_2x_hash_verifies() { + let hash = bcrypt_2x_hash("pw2x"); + assert!(hash.starts_with("$2x$")); + let users = vec![user_with_hash("legacy", hash)]; + let verifier = PasswordVerifier::new(PasswordAuthConfig::with_users(users)) + .await + .unwrap(); + assert!(verifier.verify("legacy", "pw2x").await.unwrap()); + assert!(!verifier.verify("legacy", "wrong").await.unwrap()); + } + + async fn dummy_calls_for( + verifier: &PasswordVerifier, + user: &str, + password: &str, + ) -> (usize, Result) { + use std::sync::atomic::Ordering; + let before = verifier.dummy_hash_calls.load(Ordering::Relaxed); + let result = verifier + .verify_internal(user, &Zeroizing::new(password.to_string())) + .await; + let after = verifier.dummy_hash_calls.load(Ordering::Relaxed); + (after - before, result) + } + + #[tokio::test] + async fn test_unusable_hashes_run_dummy_hash_once() { + let truncated_bcrypt = format!("$2z$04${}", "A".repeat(53)); + assert_eq!(truncated_bcrypt.len(), 60); + let unusable = [ + ("star", "*".to_string()), + ("bang", "!".to_string()), + ("empty", String::new()), + ("shacrypt", "$6$rounds=5000$saltsalt$abc".to_string()), + ("badargon", "$argon2id$v=19$".to_string()), + ("badbcrypt", truncated_bcrypt), + ]; + let mut users: Vec = unusable + .iter() + .map(|(n, h)| user_with_hash(n, h.clone())) + .collect(); + users.push(user_with_hash( + "argonuser", + hash_password("argon_pw").unwrap(), + )); + users.push(user_with_hash( + "bcryptuser", + bcrypt::hash("bcrypt_pw", 4).unwrap(), + )); + let verifier = PasswordVerifier::new(PasswordAuthConfig::with_users(users)) + .await + .unwrap(); + + for (name, _) in &unusable { + let (calls, result) = dummy_calls_for(&verifier, name, "any_password").await; + assert_eq!(calls, 1, "user {name} should trigger one dummy hash"); + if *name == "badargon" { + assert!(result.is_err(), "malformed Argon2 must stay an error"); + } else { + assert!(!result.unwrap()); + } + } + + let (calls, result) = dummy_calls_for(&verifier, "nosuchuser", "any_password").await; + assert_eq!(calls, 1); + assert!(!result.unwrap()); + + for (name, good, bad) in [ + ("argonuser", "argon_pw", "wrong"), + ("bcryptuser", "bcrypt_pw", "wrong"), + ] { + let (calls, result) = dummy_calls_for(&verifier, name, good).await; + assert_eq!(calls, 0, "valid {name} must not run the dummy hash"); + assert!(result.unwrap()); + let (calls, result) = dummy_calls_for(&verifier, name, bad).await; + assert_eq!(calls, 0, "valid {name} must not run the dummy hash"); + assert!(!result.unwrap()); + } + } + + #[tokio::test] + async fn test_malformed_argon2_user_is_rejected() { + let users = vec![user_with_hash("badargon", "$argon2id$v=19$".to_string())]; + let verifier = PasswordVerifier::new(PasswordAuthConfig::with_users(users)) + .await + .unwrap(); + assert!(verifier.verify("badargon", "pw").await.is_err()); + let result = verifier.verify_password("badargon", "pw").await.unwrap(); + assert!(matches!(result, AuthResult::Reject)); + } } diff --git a/src/server/config/types.rs b/src/server/config/types.rs index 0b76c140..ab92d567 100644 --- a/src/server/config/types.rs +++ b/src/server/config/types.rs @@ -239,7 +239,7 @@ pub struct UserDefinition { /// Password hash. /// /// Argon2id is recommended; generate it with `bssh-server hash-password`. - /// bcrypt (`$2a$`, `$2b$`, `$2y$`) is accepted for compatibility. + /// bcrypt (`$2a$`, `$2b$`, `$2x$`, `$2y$`) is accepted for compatibility. /// SHA-crypt hashes (`openssl passwd -5` / `-6`, `$5$` / `$6$`) and values /// copied from `/etc/shadow` are not supported and are rejected, so such a /// user cannot log in with a password. From 150d0ed1abb4491880dbbc4830f4f95998681e0e Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Mon, 9 Nov 2026 16:41:31 +0900 Subject: [PATCH 2/2] fix(server): treat Argon2 hashes without salt or output as malformed An Argon2 PHC string such as `$argon2id` or one missing the salt or hash output parses, so it passed the load-time format check with no warning, and argon2 reports PasswordInvalid for it without hashing, so it also skipped the dummy hash added for unusable hashes. Reject such hashes in `is_supported_hash_format` and return the malformed-hash error from `verify_argon2`, which routes them through the dummy-hash path. Login outcome is unchanged: these users still cannot authenticate. Found by the security review of this PR. Tests cover the three incomplete forms in the format check and the dummy-hash counter test, and both fail without the fix. Also rename the bad-version bcrypt fixture variable to match what it holds. Refs #356 --- src/server/auth/password.rs | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/src/server/auth/password.rs b/src/server/auth/password.rs index 92539732..525b810c 100644 --- a/src/server/auth/password.rs +++ b/src/server/auth/password.rs @@ -322,6 +322,11 @@ impl PasswordVerifier { fn verify_argon2(&self, password: &[u8], hash_str: &str) -> Result { let hash = PasswordHash::new(hash_str) .map_err(|e| anyhow::anyhow!("Invalid Argon2 hash format: {}", e))?; + if !is_complete_argon2(&hash) { + // argon2 reports PasswordInvalid for these without hashing anything, + // so surface them as malformed to keep the dummy-hash timing path. + anyhow::bail!("Invalid Argon2 hash format: missing salt or hash output"); + } let argon2 = Argon2::default(); @@ -524,7 +529,7 @@ pub fn verify_password_hash(password: &str, hash: &str) -> Result { /// changing its dispatch. Used only to warn at load time. fn is_supported_hash_format(hash: &str) -> bool { if hash.starts_with("$argon2") { - return PasswordHash::new(hash).is_ok(); + return PasswordHash::new(hash).is_ok_and(|h| is_complete_argon2(&h)); } hash.starts_with("$2a$") || hash.starts_with("$2b$") @@ -532,6 +537,12 @@ fn is_supported_hash_format(hash: &str) -> bool { || hash.starts_with("$2y$") } +/// An Argon2 PHC string can parse without a salt or hash output (for example +/// `$argon2id`), but such a hash can never verify a password. +fn is_complete_argon2(hash: &PasswordHash) -> bool { + hash.salt.is_some() && hash.hash.is_some() +} + #[cfg(test)] mod tests { use super::*; @@ -848,6 +859,9 @@ mod tests { "!", "", "$argon2id$v=19$", + "$argon2id", + "$argon2id$v=19$m=19456,t=2,p=1", + "$argon2id$v=19$m=19456,t=2,p=1$c2FsdHNhbHRzYWx0", ] { assert!(!is_supported_hash_format(bad), "should reject {bad:?}"); } @@ -937,15 +951,20 @@ mod tests { #[tokio::test] async fn test_unusable_hashes_run_dummy_hash_once() { - let truncated_bcrypt = format!("$2z$04${}", "A".repeat(53)); - assert_eq!(truncated_bcrypt.len(), 60); + let bad_version_bcrypt = format!("$2z$04${}", "A".repeat(53)); + assert_eq!(bad_version_bcrypt.len(), 60); let unusable = [ ("star", "*".to_string()), ("bang", "!".to_string()), ("empty", String::new()), ("shacrypt", "$6$rounds=5000$saltsalt$abc".to_string()), ("badargon", "$argon2id$v=19$".to_string()), - ("badbcrypt", truncated_bcrypt), + ("nosalt", "$argon2id$v=19$m=19456,t=2,p=1".to_string()), + ( + "nooutput", + "$argon2id$v=19$m=19456,t=2,p=1$c2FsdHNhbHRzYWx0".to_string(), + ), + ("badbcrypt", bad_version_bcrypt), ]; let mut users: Vec = unusable .iter() @@ -966,7 +985,7 @@ mod tests { for (name, _) in &unusable { let (calls, result) = dummy_calls_for(&verifier, name, "any_password").await; assert_eq!(calls, 1, "user {name} should trigger one dummy hash"); - if *name == "badargon" { + if ["badargon", "nosalt", "nooutput"].contains(name) { assert!(result.is_err(), "malformed Argon2 must stay an error"); } else { assert!(!result.unwrap());