From ced3520def64c7f9bb9c147ba780d06a77992561 Mon Sep 17 00:00:00 2001 From: Zaid Marji Date: Sun, 24 May 2026 14:09:04 +0300 Subject: [PATCH] Harden passkey login flow Consume passkey challenges before parsing or user lookup so every submission carrying a valid token is single-use, then delay user-scoped event attribution and account-state checks until the assertion is cryptographically bound to a registered credential. Return the generic passkey auth failure for malformed assertions, missing or corrupt challenges, unverified email state, and failed WebAuthn verification; do not send verification-reminder email from this unauthenticated flow. Add WebAuthn DOMAIN compatibility gates, stricter PRF option gating, key-rotation race checks, cascade deletes, login rate limits on authenticated passkey endpoints, and typed credential IDs on delete. --- .../up.sql | 2 +- .../up.sql | 2 +- .../up.sql | 2 +- src/api/core/accounts.rs | 105 ++++++++++++- src/api/core/mod.rs | 18 ++- src/api/identity.rs | 145 +++++++++++------- src/db/models/web_authn_credential.rs | 14 +- 7 files changed, 218 insertions(+), 70 deletions(-) diff --git a/migrations/mysql/2026-02-12-000000_add_web_authn_credentials/up.sql b/migrations/mysql/2026-02-12-000000_add_web_authn_credentials/up.sql index b1449955..0780fbaf 100644 --- a/migrations/mysql/2026-02-12-000000_add_web_authn_credentials/up.sql +++ b/migrations/mysql/2026-02-12-000000_add_web_authn_credentials/up.sql @@ -7,7 +7,7 @@ CREATE TABLE web_authn_credentials ( encrypted_user_key TEXT, encrypted_public_key TEXT, encrypted_private_key TEXT, - FOREIGN KEY (user_uuid) REFERENCES users (uuid) + FOREIGN KEY (user_uuid) REFERENCES users (uuid) ON DELETE CASCADE ); CREATE INDEX idx_web_authn_credentials_user_uuid ON web_authn_credentials (user_uuid); diff --git a/migrations/postgresql/2026-02-12-000000_add_web_authn_credentials/up.sql b/migrations/postgresql/2026-02-12-000000_add_web_authn_credentials/up.sql index fcef9704..9cf20bec 100644 --- a/migrations/postgresql/2026-02-12-000000_add_web_authn_credentials/up.sql +++ b/migrations/postgresql/2026-02-12-000000_add_web_authn_credentials/up.sql @@ -1,6 +1,6 @@ CREATE TABLE web_authn_credentials ( uuid CHAR(36) NOT NULL PRIMARY KEY, - user_uuid CHAR(36) NOT NULL REFERENCES users(uuid), + user_uuid CHAR(36) NOT NULL REFERENCES users(uuid) ON DELETE CASCADE, name TEXT NOT NULL, credential TEXT NOT NULL, supports_prf BOOLEAN NOT NULL DEFAULT FALSE, diff --git a/migrations/sqlite/2026-02-12-000000_add_web_authn_credentials/up.sql b/migrations/sqlite/2026-02-12-000000_add_web_authn_credentials/up.sql index 13beb74d..0a0b36bc 100644 --- a/migrations/sqlite/2026-02-12-000000_add_web_authn_credentials/up.sql +++ b/migrations/sqlite/2026-02-12-000000_add_web_authn_credentials/up.sql @@ -1,6 +1,6 @@ CREATE TABLE web_authn_credentials ( uuid TEXT NOT NULL PRIMARY KEY, - user_uuid TEXT NOT NULL REFERENCES users(uuid), + user_uuid TEXT NOT NULL REFERENCES users(uuid) ON DELETE CASCADE, name TEXT NOT NULL, credential TEXT NOT NULL, supports_prf BOOLEAN NOT NULL DEFAULT 0, diff --git a/src/api/core/accounts.rs b/src/api/core/accounts.rs index 8b92d50b..b0ac52cd 100644 --- a/src/api/core/accounts.rs +++ b/src/api/core/accounts.rs @@ -808,6 +808,15 @@ fn validate_keydata( err!("All existing sends must be included in the rotation") } + validate_passkey_rotation_data(&data.account_unlock_data.passkey_unlock_data, existing_webauthn_credentials)?; + + Ok(()) +} + +fn validate_passkey_rotation_data( + passkey_unlock_data: &[UpdateWebAuthnLoginData], + existing_webauthn_credentials: &[WebAuthnCredential], +) -> EmptyResult { // Check that all passkeys with PRF encryption enabled are included, so their // encrypted user key is not left stale after the account key is rotated. let existing_prf_credential_ids = existing_webauthn_credentials @@ -815,12 +824,20 @@ fn validate_keydata( .filter(|c| c.has_prf_keyset()) .map(|c| &c.uuid) .collect::>(); - let provided_passkey_ids = - data.account_unlock_data.passkey_unlock_data.iter().map(|p| &p.id).collect::>(); + let provided_passkey_ids = passkey_unlock_data.iter().map(|p| &p.id).collect::>(); if !provided_passkey_ids.is_superset(&existing_prf_credential_ids) { err!("All passkeys with encryption enabled must be included in the rotation") } + for passkey_data in passkey_unlock_data { + let Some(credential) = existing_webauthn_credentials.iter().find(|c| c.uuid == passkey_data.id) else { + err!("Passkey doesn't exist") + }; + if !credential.has_prf_keyset() { + err!("Passkey is not in a PRF-enabled state") + } + } + Ok(()) } @@ -863,6 +880,21 @@ async fn post_rotatekey(data: Json, headers: Headers, conn: DbConn, nt: &headers.user, )?; + let snapshot_prf_ids = existing_webauthn_credentials + .iter() + .filter(|c| c.has_prf_keyset()) + .map(|c| c.uuid.clone()) + .collect::>(); + let current_prf_ids = WebAuthnCredential::find_by_user(user_id, &conn) + .await + .into_iter() + .filter(WebAuthnCredential::has_prf_keyset) + .map(|c| c.uuid) + .collect::>(); + if current_prf_ids != snapshot_prf_ids { + err!("Passkey credentials changed during key rotation; please retry") + } + // Update folder data for folder_data in data.account_data.folders { // Skip `null` folder id entries. @@ -904,11 +936,21 @@ async fn post_rotatekey(data: Json, headers: Headers, conn: DbConn, nt: // Update passkey-login credential keys (the PRF "rotateable key set") so that // passwordless decryption keeps working after the account key is rotated. // The client only sends credentials whose PRF keyset is fully enabled. + // for passkey_data in data.account_unlock_data.passkey_unlock_data { let Some(mut credential) = WebAuthnCredential::find_by_uuid_and_user(&passkey_data.id, user_id, &conn).await else { err!("Passkey doesn't exist") }; + // Refuse to write rotation data to a credential that isn't in the + // "Enabled" PRF state. Otherwise a credential with only the two + // rotated columns set (and `encrypted_private_key` still NULL) would + // be left as a partial keyset that `prf_status` reports as Supported. + if !credential.has_prf_keyset() { + err!("Passkey is not in a PRF-enabled state") + } + // Mutate only the two columns `update_keys` persists; other fields on + // the loaded credential are not written back. credential.encrypted_public_key = Some(passkey_data.encrypted_public_key); credential.encrypted_user_key = Some(passkey_data.encrypted_user_key); credential.update_keys(&conn).await?; @@ -1752,3 +1794,62 @@ pub async fn purge_auth_requests(pool: DbPool) { error!("Failed to get DB connection while purging auth requests"); } } + +#[cfg(test)] +mod tests { + use super::*; + + fn webauthn_credential(id: &str, supports_prf: bool, has_keyset: bool) -> WebAuthnCredential { + let key = has_keyset.then_some(String::from("key")); + let mut credential = WebAuthnCredential::new( + UserId::from(String::from("user")), + String::from("passkey"), + String::from("{}"), + supports_prf, + key.clone(), + key.clone(), + key, + ); + credential.uuid = WebAuthnCredentialId::from(id.to_owned()); + credential + } + + fn passkey_unlock_data(id: &str) -> UpdateWebAuthnLoginData { + UpdateWebAuthnLoginData { + id: WebAuthnCredentialId::from(id.to_owned()), + encrypted_public_key: String::from("public"), + encrypted_user_key: String::from("user"), + } + } + + #[test] + fn passkey_rotation_accepts_exact_prf_set() { + let credentials = vec![webauthn_credential("prf", true, true)]; + let data = vec![passkey_unlock_data("prf")]; + + assert!(validate_passkey_rotation_data(&data, &credentials).is_ok()); + } + + #[test] + fn passkey_rotation_rejects_missing_prf_credential() { + let credentials = vec![webauthn_credential("prf", true, true)]; + + assert!(validate_passkey_rotation_data(&[], &credentials).is_err()); + } + + #[test] + fn passkey_rotation_rejects_unknown_credential() { + let credentials = vec![webauthn_credential("prf", true, true)]; + let data = vec![passkey_unlock_data("prf"), passkey_unlock_data("missing")]; + + assert!(validate_passkey_rotation_data(&data, &credentials).is_err()); + } + + #[test] + fn passkey_rotation_rejects_non_prf_credential() { + let credentials = vec![webauthn_credential("prf", true, true), webauthn_credential("non-prf", false, false)]; + let data = vec![passkey_unlock_data("prf"), passkey_unlock_data("non-prf")]; + + assert!(validate_passkey_rotation_data(&data, &credentials).is_err()); + } +} diff --git a/src/api/core/mod.rs b/src/api/core/mod.rs index 2d4f3306..04172d92 100644 --- a/src/api/core/mod.rs +++ b/src/api/core/mod.rs @@ -284,6 +284,15 @@ async fn post_api_webauthn_attestation_options( headers: Headers, conn: DbConn, ) -> JsonResult { + // Same gate the 2FA WebAuthn entry point uses; cleanly rejects requests + // when DOMAIN is incompatible with WebAuthn rather than panicking inside + // the `WEBAUTHN` `LazyLock` initializer. + if !CONFIG.is_webauthn_2fa_supported() { + err!("Configured `DOMAIN` is not compatible with Webauthn") + } + + crate::ratelimit::check_limit_login(&headers.ip.ip)?; + let data: PasswordOrOtpData = data.into_inner(); let user = headers.user; @@ -433,6 +442,8 @@ async fn post_api_webauthn( headers: Headers, conn: DbConn, ) -> ApiResult { + crate::ratelimit::check_limit_login(&headers.ip.ip)?; + let data: WebAuthnLoginCredentialCreateRequest = data.into_inner(); let user = headers.user; @@ -554,17 +565,18 @@ async fn put_api_webauthn( #[post("/webauthn//delete", data = "")] async fn post_api_webauthn_delete( data: Json, - uuid: &str, + uuid: WebAuthnCredentialId, headers: Headers, conn: DbConn, ) -> ApiResult { + crate::ratelimit::check_limit_login(&headers.ip.ip)?; + let data: PasswordOrOtpData = data.into_inner(); let user = headers.user; data.validate(&user, true, &conn).await?; - WebAuthnCredential::delete_by_uuid_and_user(&WebAuthnCredentialId::from(uuid.to_owned()), &user.uuid, &conn) - .await?; + WebAuthnCredential::delete_by_uuid_and_user(&uuid, &user.uuid, &conn).await?; Ok(Status::Ok) } diff --git a/src/api/identity.rs b/src/api/identity.rs index c4845e49..508d783f 100644 --- a/src/api/identity.rs +++ b/src/api/identity.rs @@ -1158,8 +1158,35 @@ impl From for PublicKeyCredential { } } +fn passkey_credential_id(passkey: &Passkey) -> ApiResult { + serde_json::to_value(passkey.cred_id())? + .as_str() + .map(str::to_owned) + .ok_or_else(|| crate::error::Error::new("Invalid passkey credential", "Could not serialize credential id")) +} + +fn passkey_transports(passkey: &Passkey) -> Vec { + serde_json::to_value(passkey) + .ok() + .and_then(|value| { + value + .pointer("/cred/transports") + .and_then(Value::as_array) + .map(|transports| transports.iter().filter_map(Value::as_str).map(str::to_owned).collect::>()) + }) + .unwrap_or_default() +} + #[get("/accounts/webauthn/assertion-options")] async fn get_web_authn_assertion_options(ip: ClientIp, conn: DbConn) -> JsonResult { + // Same gate the 2FA WebAuthn entry point uses, applied here so a + // misconfigured DOMAIN (e.g., IP literal that has no parseable host) + // returns a clean error instead of panicking inside the `WEBAUTHN` + // `LazyLock` initializer. + if !CONFIG.is_webauthn_2fa_supported() { + err!("Configured `DOMAIN` is not compatible with Webauthn") + } + if CONFIG.sso_enabled() && CONFIG.sso_only() { err!("SSO sign-in is required") } @@ -1199,34 +1226,47 @@ async fn webauthn_login(data: ConnectData, user_id: &mut Option, conn: & AuthMethod::WebAuthn.check_scope(data.scope.as_ref())?; crate::ratelimit::check_limit_login(&ip.ip)?; - // Parse the authenticator assertion. A malformed body must yield the same - // generic error as any other failure, not a raw deserialization error. - let Ok(device_response) = serde_json::from_str::(data.device_response.as_ref().unwrap()) - else { + // Recover and consume (single-use) the saved challenge state. Every + // submission carrying a valid token is spent, regardless of body content + // or which user the assertion later claims — a caller with a valid token + // cannot repeatedly replay it with malformed bodies. + let token = WebAuthnLoginChallengeId::from(data.token.as_ref().unwrap().clone()); + let Some(saved_challenge) = WebAuthnLoginChallenge::take(&token, conn).await else { err!( AUTH_FAILED, - format!("IP: {}. Malformed passkey assertion.", ip.ip), + format!("IP: {}. Missing or expired passkey login challenge.", ip.ip), + ErrorEvent { + event: EventType::UserFailedLogIn + } + ) + }; + let Ok(state) = serde_json::from_str::(&saved_challenge.challenge) else { + err!( + AUTH_FAILED, + format!("IP: {}. Corrupt passkey login challenge state.", ip.ip), ErrorEvent { event: EventType::UserFailedLogIn } ) }; - let credential: PublicKeyCredential = device_response.into(); - // Recover and consume (single-use) the saved challenge state. - let token = WebAuthnLoginChallengeId::from(data.token.as_ref().unwrap().clone()); - let Some(saved_challenge) = WebAuthnLoginChallenge::take(&token, conn).await else { + // Parse the authenticator assertion. A malformed body must yield the same + // generic error as any other failure, not a raw deserialization error. + let Ok(device_response) = serde_json::from_str::(data.device_response.as_ref().unwrap()) + else { err!( AUTH_FAILED, - format!("IP: {}. Missing or expired passkey login challenge.", ip.ip), + format!("IP: {}. Malformed passkey assertion.", ip.ip), ErrorEvent { event: EventType::UserFailedLogIn } ) }; - let state: DiscoverableAuthentication = serde_json::from_str(&saved_challenge.challenge)?; + let credential: PublicKeyCredential = device_response.into(); - // Determine which user the discoverable credential belongs to from its user handle. + // Identify which user the discoverable credential claims to belong to from + // its user handle. This only parses client-supplied data; user-scoped event + // logging is delayed until the assertion is cryptographically verified. let user_uuid = match WEBAUTHN.identify_discoverable_authentication(&credential) { Ok((user_uuid, _)) => UserId::from(user_uuid.to_string()), Err(e) => err!( @@ -1248,20 +1288,8 @@ async fn webauthn_login(data: ConnectData, user_id: &mut Option, conn: & ) }; - // Record the user for event logging. - *user_id = Some(user.uuid.clone()); let username = user.email.clone(); - if !user.enabled { - err!( - AUTH_FAILED, - format!("IP: {}. Username: {username}. Account is disabled.", ip.ip), - ErrorEvent { - event: EventType::UserFailedLogIn - } - ) - } - // Load this user's passkey-login credentials. let parsed_credentials: Vec<(WebAuthnCredential, Passkey)> = WebAuthnCredential::find_by_user(&user.uuid, conn) .await @@ -1299,51 +1327,50 @@ async fn webauthn_login(data: ConnectData, user_id: &mut Option, conn: & ), }; - // Locate the credential that was actually used and persist any counter update. - let Some((mut matched_wac, mut passkey)) = parsed_credentials - .into_iter() - .find(|(_, passkey)| crypto::ct_eq(passkey.cred_id().as_slice(), authentication_result.cred_id().as_slice())) - else { + // The assertion is now bound to a registered credential for this user. From + // this point on, failed account-state checks can be attributed to the user + // without allowing arbitrary user-handle event log pollution. + *user_id = Some(user.uuid.clone()); + + if !user.enabled { err!( AUTH_FAILED, - format!("IP: {}. Username: {username}. Verified credential is not registered.", ip.ip), + format!("IP: {}. Username: {username}. Account is disabled.", ip.ip), ErrorEvent { event: EventType::UserFailedLogIn } ) - }; + } - // Reject an unverified account before persisting anything for this login. - let now = Utc::now().naive_utc(); + // Reject an unverified account before doing any server-side persistence. + // Mirrors the password-login email-verify gate but elides the verification + // reminder email and the distinguishable error message. Returning the same + // `AUTH_FAILED` as every other branch prevents using passkey login as an + // oracle for verification state. The descriptive hint still reaches + // legitimate users via password login. if user.verified_at.is_none() && CONFIG.mail_enabled() && CONFIG.signups_verify() { - if user.last_verifying_at.is_none() - || now.signed_duration_since(user.last_verifying_at.unwrap()).num_seconds() - > CONFIG.signups_verify_resend_time().cast_signed() - { - let resend_limit = CONFIG.signups_verify_resend_limit().cast_signed(); - if resend_limit == 0 || user.login_verify_count < resend_limit { - let mut user = user; - user.last_verifying_at = Some(now); - user.login_verify_count += 1; - - if let Err(e) = user.save(conn).await { - error!("Error updating user: {e:#?}"); - } - - if let Err(e) = mail::send_verify_email(&user.email, &user.uuid).await { - error!("Error auto-sending email verification email: {e:#?}"); - } + err!( + AUTH_FAILED, + format!("IP: {}. Username: {username}. Account is not email-verified.", ip.ip), + ErrorEvent { + event: EventType::UserFailedLogIn } - } + ) + } + // Locate the credential that was actually used and persist any counter update. + let Some((mut matched_wac, mut passkey)) = parsed_credentials + .into_iter() + .find(|(_, passkey)| crypto::ct_eq(passkey.cred_id().as_slice(), authentication_result.cred_id().as_slice())) + else { err!( - "Please verify your email before trying again.", - format!("IP: {}. Username: {username}.", ip.ip), + AUTH_FAILED, + format!("IP: {}. Username: {username}. Verified credential is not registered.", ip.ip), ErrorEvent { event: EventType::UserFailedLogIn } ) - } + }; // Persist any signature-counter advance from this assertion. if passkey.update_credential(&authentication_result) == Some(true) { @@ -1374,12 +1401,18 @@ async fn webauthn_login(data: ConnectData, user_id: &mut Option, conn: & // Build response using the common authenticated_response helper let mut result = authenticated_response(&user, &mut device, auth_tokens, None, conn, ip).await?; - // Add WebAuthnPrfOption if the credential has encrypted keys (PRF-based decryption) - if matched_wac.encrypted_private_key.is_some() && matched_wac.encrypted_user_key.is_some() { + // Add WebAuthnPrfOption only when the credential is in the PRF "Enabled" + // state (supports_prf + all three encrypted blobs present), matching + // Bitwarden's `GetPrfStatus() == Enabled` gate. + if matched_wac.has_prf_keyset() { + let credential_id = passkey_credential_id(&passkey)?; + let transports = passkey_transports(&passkey); let Json(ref mut val) = result; val["UserDecryptionOptions"]["WebAuthnPrfOption"] = json!({ "EncryptedPrivateKey": matched_wac.encrypted_private_key, "EncryptedUserKey": matched_wac.encrypted_user_key, + "CredentialId": credential_id, + "Transports": transports, }); } diff --git a/src/db/models/web_authn_credential.rs b/src/db/models/web_authn_credential.rs index 795a5207..e4cc11de 100644 --- a/src/db/models/web_authn_credential.rs +++ b/src/db/models/web_authn_credential.rs @@ -200,12 +200,14 @@ impl WebAuthnLoginChallenge { /// token is unknown, has already been consumed, or the challenge has expired. pub async fn take(id: &WebAuthnLoginChallengeId, conn: &DbConn) -> Option { db_run! { conn: { - // Single-use: the SELECT and DELETE run in one transaction so the row - // is read and removed atomically. Only the request whose DELETE - // removes the row (deleted == 1) may use the challenge; a concurrent - // request deletes 0 rows and gets `None`. A DB error aborts the - // transaction, leaving the challenge intact rather than silently - // treating it as consumed. + // Single-use is enforced by the `deleted == 1` row-count guard, not + // by isolation: concurrent callers may all see the row in the + // SELECT, but only the one whose DELETE removes it (returns 1) is + // allowed to use the challenge. The remaining callers see + // `deleted == 0` and get `None`. The surrounding transaction + // ensures the SELECT+DELETE pair rolls back atomically on a DB + // error, leaving the challenge intact rather than silently + // consuming it. let taken = conn .transaction::, diesel::result::Error, _>(|conn| { let challenge = web_authn_login_challenges::table