From 5760ade3258ae92fc855430f5cd45211c8486d33 Mon Sep 17 00:00:00 2001 From: Zaid Marji Date: Tue, 2 Jun 2026 17:43:12 +0300 Subject: [PATCH] Fix and harden the passkey login implementation - Wire key rotation to re-encrypt each passkey's PRF keys, with a superset check in validate_keydata so a passkey can't be left stale. - Persist signature-counter updates and rotated PRF keys with column-scoped writes, avoiding a broad full-row credential update. - Compute prfStatus as the full WebAuthnPrfStatus instead of a 1/0 placeholder. - Move the login challenge from an in-memory cache to a DB-backed table with a scheduled cleanup job. - Use webauthn-rs's discoverable-credential API instead of the JSON state-injection workaround. - Make challenge consumption single-use, rate-limit assertion-options, and return a single generic auth-failure message. - Honor SSO_ONLY at every passkey entry point: login grant, enrollment, refresh, and the unauthenticated assertion-options challenge. - Migrations: real MySQL foreign key and indexes. - Add prfStatus unit tests; codebase-consistency pass. --- .env.template | 4 + Cargo.toml | 3 +- .../up.sql | 9 +- .../down.sql | 1 + .../up.sql | 7 + .../up.sql | 6 +- .../down.sql | 1 + .../up.sql | 7 + .../up.sql | 2 + .../down.sql | 1 + .../up.sql | 7 + src/api/core/accounts.rs | 45 ++++ src/api/core/mod.rs | 32 ++- src/api/identity.rs | 240 ++++++++---------- src/auth.rs | 4 +- src/config.rs | 9 + src/db/models/mod.rs | 4 +- src/db/models/web_authn_credential.rs | 231 +++++++++++++++-- src/db/schema.rs | 15 +- src/main.rs | 7 + 20 files changed, 455 insertions(+), 180 deletions(-) create mode 100644 migrations/mysql/2026-05-21-000000_add_web_authn_login_challenges/down.sql create mode 100644 migrations/mysql/2026-05-21-000000_add_web_authn_login_challenges/up.sql create mode 100644 migrations/postgresql/2026-05-21-000000_add_web_authn_login_challenges/down.sql create mode 100644 migrations/postgresql/2026-05-21-000000_add_web_authn_login_challenges/up.sql create mode 100644 migrations/sqlite/2026-05-21-000000_add_web_authn_login_challenges/down.sql create mode 100644 migrations/sqlite/2026-05-21-000000_add_web_authn_login_challenges/up.sql diff --git a/.env.template b/.env.template index a12559ad..a6548407 100644 --- a/.env.template +++ b/.env.template @@ -187,6 +187,10 @@ ## Cron schedule of the job that cleans sso auth from incomplete flow ## Defaults to daily (20 minutes after midnight). Set blank to disable this job. # PURGE_INCOMPLETE_SSO_AUTH="0 20 0 * * *" +# +## Cron schedule of the job that cleans expired passkey-login challenges from the database. +## Defaults to hourly (30 minutes past the hour). Set blank to disable this job. +# WEBAUTHN_LOGIN_CHALLENGE_PURGE_SCHEDULE="0 30 * * * *" ######################## ### General settings ### diff --git a/Cargo.toml b/Cargo.toml index 599af5c8..9f1ad43c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -154,7 +154,8 @@ yubico = { package = "yubico_ng", version = "0.15.0", default-features = false, # WebAuthn libraries # danger-allow-state-serialisation is needed to save the state in the db # danger-credential-internals is needed to support U2F to Webauthn migration -webauthn-rs = { version = "0.5.5", features = ["danger-allow-state-serialisation", "danger-credential-internals"] } +# conditional-ui is needed for the discoverable-credential APIs used by passkey login +webauthn-rs = { version = "0.5.5", features = ["conditional-ui", "danger-allow-state-serialisation", "danger-credential-internals"] } webauthn-rs-proto = "0.5.5" webauthn-rs-core = "0.5.5" 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 a8cfed5f..b1449955 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 @@ -1,10 +1,13 @@ CREATE TABLE web_authn_credentials ( - uuid VARCHAR(40) NOT NULL PRIMARY KEY, - user_uuid VARCHAR(40) NOT NULL REFERENCES users(uuid), + uuid CHAR(36) NOT NULL PRIMARY KEY, + user_uuid CHAR(36) NOT NULL, name TEXT NOT NULL, credential TEXT NOT NULL, supports_prf BOOLEAN NOT NULL DEFAULT 0, encrypted_user_key TEXT, encrypted_public_key TEXT, - encrypted_private_key TEXT + encrypted_private_key TEXT, + FOREIGN KEY (user_uuid) REFERENCES users (uuid) ); + +CREATE INDEX idx_web_authn_credentials_user_uuid ON web_authn_credentials (user_uuid); diff --git a/migrations/mysql/2026-05-21-000000_add_web_authn_login_challenges/down.sql b/migrations/mysql/2026-05-21-000000_add_web_authn_login_challenges/down.sql new file mode 100644 index 00000000..f03b48b4 --- /dev/null +++ b/migrations/mysql/2026-05-21-000000_add_web_authn_login_challenges/down.sql @@ -0,0 +1 @@ +DROP TABLE web_authn_login_challenges; diff --git a/migrations/mysql/2026-05-21-000000_add_web_authn_login_challenges/up.sql b/migrations/mysql/2026-05-21-000000_add_web_authn_login_challenges/up.sql new file mode 100644 index 00000000..51cf87da --- /dev/null +++ b/migrations/mysql/2026-05-21-000000_add_web_authn_login_challenges/up.sql @@ -0,0 +1,7 @@ +CREATE TABLE web_authn_login_challenges ( + id CHAR(36) NOT NULL PRIMARY KEY, + challenge TEXT NOT NULL, + created_at DATETIME NOT NULL +); + +CREATE INDEX idx_web_authn_login_challenges_created_at ON web_authn_login_challenges (created_at); 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 c07154e9..fcef9704 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 VARCHAR(40) NOT NULL PRIMARY KEY, - user_uuid VARCHAR(40) NOT NULL REFERENCES users(uuid), + uuid CHAR(36) NOT NULL PRIMARY KEY, + user_uuid CHAR(36) NOT NULL REFERENCES users(uuid), name TEXT NOT NULL, credential TEXT NOT NULL, supports_prf BOOLEAN NOT NULL DEFAULT FALSE, @@ -8,3 +8,5 @@ CREATE TABLE web_authn_credentials ( encrypted_public_key TEXT, encrypted_private_key TEXT ); + +CREATE INDEX idx_web_authn_credentials_user_uuid ON web_authn_credentials (user_uuid); diff --git a/migrations/postgresql/2026-05-21-000000_add_web_authn_login_challenges/down.sql b/migrations/postgresql/2026-05-21-000000_add_web_authn_login_challenges/down.sql new file mode 100644 index 00000000..f03b48b4 --- /dev/null +++ b/migrations/postgresql/2026-05-21-000000_add_web_authn_login_challenges/down.sql @@ -0,0 +1 @@ +DROP TABLE web_authn_login_challenges; diff --git a/migrations/postgresql/2026-05-21-000000_add_web_authn_login_challenges/up.sql b/migrations/postgresql/2026-05-21-000000_add_web_authn_login_challenges/up.sql new file mode 100644 index 00000000..8dc20e97 --- /dev/null +++ b/migrations/postgresql/2026-05-21-000000_add_web_authn_login_challenges/up.sql @@ -0,0 +1,7 @@ +CREATE TABLE web_authn_login_challenges ( + id CHAR(36) NOT NULL PRIMARY KEY, + challenge TEXT NOT NULL, + created_at TIMESTAMP NOT NULL +); + +CREATE INDEX idx_web_authn_login_challenges_created_at ON web_authn_login_challenges (created_at); 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 e7770ca4..13beb74d 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 @@ -8,3 +8,5 @@ CREATE TABLE web_authn_credentials ( encrypted_public_key TEXT, encrypted_private_key TEXT ); + +CREATE INDEX idx_web_authn_credentials_user_uuid ON web_authn_credentials (user_uuid); diff --git a/migrations/sqlite/2026-05-21-000000_add_web_authn_login_challenges/down.sql b/migrations/sqlite/2026-05-21-000000_add_web_authn_login_challenges/down.sql new file mode 100644 index 00000000..f03b48b4 --- /dev/null +++ b/migrations/sqlite/2026-05-21-000000_add_web_authn_login_challenges/down.sql @@ -0,0 +1 @@ +DROP TABLE web_authn_login_challenges; diff --git a/migrations/sqlite/2026-05-21-000000_add_web_authn_login_challenges/up.sql b/migrations/sqlite/2026-05-21-000000_add_web_authn_login_challenges/up.sql new file mode 100644 index 00000000..23827b95 --- /dev/null +++ b/migrations/sqlite/2026-05-21-000000_add_web_authn_login_challenges/up.sql @@ -0,0 +1,7 @@ +CREATE TABLE web_authn_login_challenges ( + id TEXT NOT NULL PRIMARY KEY, + challenge TEXT NOT NULL, + created_at DATETIME NOT NULL +); + +CREATE INDEX idx_web_authn_login_challenges_created_at ON web_authn_login_challenges (created_at); diff --git a/src/api/core/accounts.rs b/src/api/core/accounts.rs index 954b35bd..8b92d50b 100644 --- a/src/api/core/accounts.rs +++ b/src/api/core/accounts.rs @@ -23,6 +23,7 @@ use crate::{ AuthRequest, AuthRequestId, Cipher, CipherId, Device, DeviceId, DeviceType, DeviceWithAuthRequest, EmergencyAccess, EmergencyAccessId, EventType, Folder, FolderId, Invitation, Membership, MembershipId, OrgPolicy, OrgPolicyType, Organization, OrganizationId, Send, SendId, User, UserId, UserKdfType, + WebAuthnCredential, WebAuthnCredentialId, }, }, mail, @@ -672,6 +673,14 @@ struct UpdateResetPasswordData { reset_password_key: String, } +#[derive(Deserialize)] +#[serde(rename_all = "camelCase")] +struct UpdateWebAuthnLoginData { + id: WebAuthnCredentialId, + encrypted_public_key: String, + encrypted_user_key: String, +} + #[derive(Deserialize)] #[serde(rename_all = "camelCase")] struct KeyData { @@ -687,6 +696,9 @@ struct RotateAccountUnlockData { emergency_access_unlock_data: Vec, master_password_unlock_data: MasterPasswordUnlockData, organization_account_recovery_unlock_data: Vec, + // Older clients may not send this; default to an empty list so rotation still works. + #[serde(default)] + passkey_unlock_data: Vec, } #[derive(Deserialize)] @@ -716,6 +728,10 @@ struct RotateAccountData { sends: Vec, } +#[expect( + clippy::too_many_arguments, + reason = "Per-entity slices mirror the snapshot reads at the top of post_rotatekey" +)] fn validate_keydata( data: &KeyData, existing_ciphers: &[Cipher], @@ -723,6 +739,7 @@ fn validate_keydata( existing_emergency_access: &[EmergencyAccess], existing_memberships: &[Membership], existing_sends: &[Send], + existing_webauthn_credentials: &[WebAuthnCredential], user: &User, ) -> EmptyResult { if user.client_kdf_type != data.account_unlock_data.master_password_unlock_data.kdf_type @@ -791,6 +808,19 @@ fn validate_keydata( err!("All existing sends must be included in the rotation") } + // 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 + .iter() + .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::>(); + if !provided_passkey_ids.is_superset(&existing_prf_credential_ids) { + err!("All passkeys with encryption enabled must be included in the rotation") + } + Ok(()) } @@ -820,6 +850,7 @@ async fn post_rotatekey(data: Json, headers: Headers, conn: DbConn, nt: // We only rotate the reset password key if it is set. existing_memberships.retain(|m| m.reset_password_key.is_some()); let mut existing_sends = Send::find_by_user(user_id, &conn).await; + let existing_webauthn_credentials = WebAuthnCredential::find_by_user(user_id, &conn).await; validate_keydata( &data, @@ -828,6 +859,7 @@ async fn post_rotatekey(data: Json, headers: Headers, conn: DbConn, nt: &existing_emergency_access, &existing_memberships, &existing_sends, + &existing_webauthn_credentials, &headers.user, )?; @@ -869,6 +901,19 @@ async fn post_rotatekey(data: Json, headers: Headers, conn: DbConn, nt: membership.save(&conn).await?; } + // 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") + }; + 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?; + } + // Update send data for send_data in data.account_data.sends { let Some(send) = existing_sends.iter_mut().find(|s| &s.uuid == send_data.id.as_ref().unwrap()) else { diff --git a/src/api/core/mod.rs b/src/api/core/mod.rs index 922604e3..5aeb00ea 100644 --- a/src/api/core/mod.rs +++ b/src/api/core/mod.rs @@ -216,21 +216,21 @@ fn version() -> Json<&'static str> { async fn get_api_webauthn(headers: Headers, conn: DbConn) -> Json { let user = headers.user; - let data: Vec = WebAuthnCredential::find_all_by_user(&user.uuid, &conn).await; - let data = data + let data: Vec = WebAuthnCredential::find_by_user(&user.uuid, &conn) + .await .into_iter() .map(|wac| { json!({ "id": wac.uuid, "name": wac.name, - // TODO: Generate prfStatus like GetPrfStatus() does in the C# implementation - "prfStatus": i32::from(wac.supports_prf), + // 0 = Enabled, 1 = Supported (PRF-capable, keyset not set up), 2 = Unsupported. + "prfStatus": wac.prf_status(), "encryptedUserKey": wac.encrypted_user_key, "encryptedPublicKey": wac.encrypted_public_key, "object": "webauthnCredential", }) }) - .collect::(); + .collect(); Json(json!({ "object": "list", @@ -248,9 +248,13 @@ async fn post_api_webauthn_attestation_options( let data: PasswordOrOtpData = data.into_inner(); let user = headers.user; + if CONFIG.sso_enabled() && CONFIG.sso_only() { + err!("Passkeys cannot be created when SSO sign-in is required") + } + data.validate(&user, false, &conn).await?; - let all_creds: Vec = WebAuthnCredential::find_all_by_user(&user.uuid, &conn).await; + let all_creds = WebAuthnCredential::find_by_user(&user.uuid, &conn).await; let existing_cred_ids: Vec<_> = all_creds .into_iter() .filter_map(|wac| { @@ -259,7 +263,8 @@ async fn post_api_webauthn_attestation_options( }) .collect(); - let user_uuid = uuid::Uuid::parse_str(&user.uuid).expect("Failed to parse user UUID"); + let user_uuid = uuid::Uuid::parse_str(&user.uuid) + .map_err(|_| Error::new("Invalid user", "Could not parse user UUID for passkey registration"))?; let (mut challenge, state) = WEBAUTHN.start_passkey_registration(user_uuid, &user.email, user.display_name(), Some(existing_cred_ids))?; @@ -275,6 +280,15 @@ async fn post_api_webauthn_attestation_options( asc.resident_key = Some(webauthn_rs_proto::ResidentKeyRequirement::Required); } + // Drop any abandoned challenge from a previous, unfinished registration attempt + // so these rows cannot accumulate in the database. + if let Some(tf) = + TwoFactor::find_by_user_and_type(&user.uuid, TwoFactorType::WebauthnPasskeyRegisterChallenge as i32, &conn) + .await + { + tf.delete(&conn).await?; + } + // Persist the registration state in the database (same pattern as 2FA webauthn) TwoFactor::new(user.uuid, TwoFactorType::WebauthnPasskeyRegisterChallenge, serde_json::to_string(&state)?) .save(&conn) @@ -310,6 +324,10 @@ async fn post_api_webauthn( let data: WebAuthnLoginCredentialCreateRequest = data.into_inner(); let user = headers.user; + if CONFIG.sso_enabled() && CONFIG.sso_only() { + err!("Passkeys cannot be created when SSO sign-in is required") + } + // Retrieve and delete the saved challenge state from the database let type_ = TwoFactorType::WebauthnPasskeyRegisterChallenge as i32; let Some(tf) = TwoFactor::find_by_user_and_type(&user.uuid, type_, &conn).await else { diff --git a/src/api/identity.rs b/src/api/identity.rs index 965b3fe0..4334fd5c 100644 --- a/src/api/identity.rs +++ b/src/api/identity.rs @@ -1,6 +1,3 @@ -use std::sync::{Arc, LazyLock}; -use std::time::Duration; - use chrono::Utc; use num_traits::FromPrimitive; use rocket::{ @@ -11,10 +8,9 @@ use rocket::{ serde::json::Json, }; use serde_json::Value; -use webauthn_rs::prelude::{Base64UrlSafeData, Passkey, PasskeyAuthentication}; +use webauthn_rs::prelude::{Base64UrlSafeData, DiscoverableAuthentication, DiscoverableKey, Passkey}; use webauthn_rs_proto::{ AuthenticationExtensionsClientOutputs, AuthenticatorAssertionResponseRaw, PublicKeyCredential, - RequestAuthenticationExtensions, UserVerificationPolicy, }; use crate::api::core::two_factor::webauthn::WEBAUTHN; @@ -41,7 +37,7 @@ use crate::{ models::{ AuthRequest, AuthRequestId, Device, DeviceId, EventType, Invitation, OIDCCodeResponseError, OrganizationApiKey, OrganizationId, SsoAuth, SsoUser, TwoFactor, TwoFactorIncomplete, TwoFactorType, User, - UserId, WebAuthnCredential, + UserId, WebAuthnCredential, WebAuthnLoginChallenge, WebAuthnLoginChallengeId, }, }, error::MapResult, @@ -82,7 +78,7 @@ async fn login( check_is_some(data.refresh_token.as_ref(), "refresh_token cannot be blank")?; refresh_login(data, &conn, &client_header.ip).await } - "password" if CONFIG.sso_enabled() && CONFIG.sso_only() => err!("SSO sign-in is required"), + "password" | "webauthn" if CONFIG.sso_enabled() && CONFIG.sso_only() => err!("SSO sign-in is required"), "password" => { check_is_some(data.client_id.as_ref(), "client_id cannot be blank")?; check_is_some(data.password.as_ref(), "password cannot be blank")?; @@ -1091,7 +1087,7 @@ async fn register_verification_email( use rand::{RngExt, rngs::SmallRng}; let mut rng: SmallRng = rand::make_rng(); let sleep_ms: u64 = rng.random_range(900..=1100); - tokio::time::sleep(Duration::from_millis(sleep_ms)).await; + tokio::time::sleep(tokio::time::Duration::from_millis(sleep_ms)).await; } else { mail::send_register_verify_email(&data.email, &token).await?; } @@ -1109,14 +1105,6 @@ async fn register_finish(data: Json, conn: DbConn) -> JsonResult { register(data, true, conn).await } -// Cache for webauthn authentication states, keyed by a random token. -// Entries expire after 5 minutes (matching the WebAuthn ceremony timeout of 60s with margin). -// This is used for the discoverable credential (passkey login) flow where we don't know -// the user until the authenticator response arrives. -// Wrapped in Arc because PasskeyAuthentication does not implement Clone. -static WEBAUTHN_AUTHENTICATION_STATES: LazyLock>> = - LazyLock::new(|| moka::sync::Cache::builder().max_capacity(10_000).time_to_live(Duration::from_mins(5)).build()); - // Copied from webauthn-rs to rename clientDataJSON -> clientDataJson for Bitwarden compatibility #[derive(Debug, Deserialize)] #[serde(rename_all = "camelCase")] @@ -1157,21 +1145,28 @@ impl From for PublicKeyCredential { } #[get("/accounts/webauthn/assertion-options")] -fn get_web_authn_assertion_options() -> JsonResult { - let (mut response, state) = WEBAUTHN.start_passkey_authentication(&[])?; - - // Allow any credential (discoverable) and require user verification - response.public_key.allow_credentials = vec![]; - response.public_key.user_verification = UserVerificationPolicy::Required; - response.public_key.extensions = Some(RequestAuthenticationExtensions { - appid: None, - uvm: None, - hmac_get_secret: None, - }); +async fn get_web_authn_assertion_options(ip: ClientIp, conn: DbConn) -> JsonResult { + if CONFIG.sso_enabled() && CONFIG.sso_only() { + err!("SSO sign-in is required") + } - let token = util::get_uuid(); - WEBAUTHN_AUTHENTICATION_STATES.insert(token.clone(), Arc::new(state)); + // This endpoint is unauthenticated; rate-limit it so it cannot be abused to + // flood the challenge table. Expired rows are removed by a scheduled job. + crate::ratelimit::check_limit_login(&ip.ip)?; + + // start_discoverable_authentication() requests an empty allow-list + // (discoverable credentials) and user verification. + let (response, state) = WEBAUTHN.start_discoverable_authentication()?; + // Persist the challenge state so it can be verified on the follow-up token + // request. It is keyed by a random token and consumed exactly once. + let challenge = WebAuthnLoginChallenge::new(serde_json::to_string(&state)?); + let token = challenge.id.clone(); + challenge.save(&conn).await?; + + // Only `public_key` is forwarded: the `mediation: Conditional` field that + // start_discoverable_authentication() sets is intentionally dropped, since + // Bitwarden's "Log in with passkey" is an explicit-button flow, not autofill. let options = serde_json::to_value(response.public_key)?; Ok(Json(json!({ @@ -1182,62 +1177,80 @@ fn get_web_authn_assertion_options() -> JsonResult { } async fn webauthn_login(data: ConnectData, user_id: &mut Option, conn: &DbConn, ip: &ClientIp) -> JsonResult { - // Validate scope - AuthMethod::WebAuthn.check_scope(data.scope.as_ref())?; + // A single generic message is returned to the client for every failure so the + // endpoint cannot be used to probe which accounts exist or have passkeys. + const AUTH_FAILED: &str = "Passkey authentication failed."; - // Ratelimit the login + // Validate scope and rate-limit the login. + AuthMethod::WebAuthn.check_scope(data.scope.as_ref())?; crate::ratelimit::check_limit_login(&ip.ip)?; - // Parse the device response to get the user handle (user UUID) - let device_response: PublicKeyCredentialCopy = serde_json::from_str(data.device_response.as_ref().unwrap())?; - - let user = if let Some(ref uuid_bytes) = device_response.response.user_handle { - // The user_handle contains the raw UUID bytes (16 bytes) set during passkey registration. - // We need to reconstruct the UUID string from these bytes. - let bytes: &[u8] = uuid_bytes.as_ref(); - let uuid_str = uuid::Uuid::from_slice(bytes) - .map(|u| u.to_string()) - .or_else(|_| { - // Fallback: try interpreting as UTF-8 string (for compatibility) - String::from_utf8(bytes.to_vec()) - }) - .map_err(|_| crate::error::Error::new("Invalid user handle encoding", ""))?; - let uuid = UserId::from(uuid_str); - User::find_by_uuid(&uuid, conn).await - } else { - None + // 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: {}. Malformed passkey assertion.", ip.ip), + ErrorEvent { + event: EventType::UserFailedLogIn + } + ) }; + let credential: PublicKeyCredential = device_response.into(); - let Some(user) = user else { + // 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 { err!( - "Passkey authentication failed.", - format!("IP: {}. Could not find user from device response.", ip.ip), + AUTH_FAILED, + format!("IP: {}. Missing or expired passkey login challenge.", ip.ip), ErrorEvent { event: EventType::UserFailedLogIn } ) }; + let state: DiscoverableAuthentication = serde_json::from_str(&saved_challenge.challenge)?; + + // Determine which user the discoverable credential belongs to from its user handle. + let user_uuid = match WEBAUTHN.identify_discoverable_authentication(&credential) { + Ok((user_uuid, _)) => UserId::from(user_uuid.to_string()), + Err(e) => err!( + AUTH_FAILED, + format!("IP: {}. Could not identify passkey credential: {e:?}", ip.ip), + ErrorEvent { + event: EventType::UserFailedLogIn + } + ), + }; - let username = user.display_name().to_owned(); + let Some(user) = User::find_by_uuid(&user_uuid, conn).await else { + err!( + AUTH_FAILED, + format!("IP: {}. No user matches passkey user handle {user_uuid}.", ip.ip), + ErrorEvent { + event: EventType::UserFailedLogIn + } + ) + }; - // Set the user_id here to be passed back used for event logging. + // Record the user for event logging. *user_id = Some(user.uuid.clone()); + let username = user.email.clone(); - // Check if the user is disabled if !user.enabled { err!( - "This user has been disabled", - format!("IP: {}. Username: {username}.", ip.ip), + AUTH_FAILED, + format!("IP: {}. Username: {username}. Account is disabled.", ip.ip), ErrorEvent { event: EventType::UserFailedLogIn } ) } - // Retrieve all webauthn login credentials for this user - let web_authn_credentials: Vec = WebAuthnCredential::find_all_by_user(&user.uuid, conn).await; - - let parsed_credentials: Vec<(WebAuthnCredential, Passkey)> = web_authn_credentials + // Load this user's passkey-login credentials. + let parsed_credentials: Vec<(WebAuthnCredential, Passkey)> = WebAuthnCredential::find_by_user(&user.uuid, conn) + .await .into_iter() .filter_map(|wac| { let passkey: Passkey = serde_json::from_str(&wac.credential).ok()?; @@ -1247,97 +1260,46 @@ async fn webauthn_login(data: ConnectData, user_id: &mut Option, conn: & if parsed_credentials.is_empty() { err!( - "No passkey credentials registered for this user.", - format!("IP: {}. Username: {username}.", ip.ip), + AUTH_FAILED, + format!("IP: {}. Username: {username}. No passkey credentials registered.", ip.ip), ErrorEvent { event: EventType::UserFailedLogIn } ) } - // Retrieve and consume the saved authentication state (one-time use) - let token = data.token.as_ref().unwrap(); - let state = WEBAUTHN_AUTHENTICATION_STATES.get(token); - // Invalidate immediately to prevent replay - WEBAUTHN_AUTHENTICATION_STATES.invalidate(token); - debug!( - "WebAuthn login: found {} credentials for user, state present: {}", - parsed_credentials.len(), - state.is_some() - ); - - let Some(state_arc) = state else { - err!( - "Passkey authentication failed. Please try again.", - format!("IP: {}. Username: {username}. Missing authentication state.", ip.ip), - ErrorEvent { - event: EventType::UserFailedLogIn - } - ) - }; - - // Inject the user's credentials into the state so the library can verify against them. - // We serialize the state to JSON, inject the user's credentials, then deserialize back. - // This is necessary because for discoverable credentials (passkey login), the initial - // assertion was created without knowing which user will authenticate, so the state has - // no credentials to verify against. This is the same pattern used by - // check_and_update_backup_eligible() in two_factor/webauthn.rs. - let passkeys: Vec = - parsed_credentials.iter().map(|(_, p): &(WebAuthnCredential, Passkey)| p.clone()).collect(); - - let mut raw_state = serde_json::to_value(&*state_arc)?; - if let Some(credentials) = - raw_state.get_mut("ast").and_then(|v| v.get_mut("credentials")).and_then(|v| v.as_array_mut()) - { - credentials.clear(); - for passkey in &passkeys { - let passkey_owned: Passkey = passkey.clone(); - let cred = ::from(passkey_owned); - credentials.push(serde_json::to_value(&cred)?); - } - } - let state: PasskeyAuthentication = serde_json::from_value(raw_state).map_err(|e| { - error!("Failed to deserialize PasskeyAuthentication state after credential injection: {e:?}"); - e - })?; - - let rsp: PublicKeyCredential = device_response.into(); - let authentication_result = match WEBAUTHN.finish_passkey_authentication(&rsp, &state) { - Ok(result) => result, - Err(e) => { - err!( - "Passkey authentication failed.", - format!("IP: {}. Username: {username}. WebAuthn error: {e:?}", ip.ip), + let discoverable_keys: Vec = + parsed_credentials.iter().map(|(_, passkey)| DiscoverableKey::from(passkey)).collect(); + + // Verify the assertion. webauthn-rs checks the signature, challenge, origin, + // user verification and the signature counter against the registered keys. + let authentication_result = + match WEBAUTHN.finish_discoverable_authentication(&credential, state, &discoverable_keys) { + Ok(result) => result, + Err(e) => err!( + AUTH_FAILED, + format!("IP: {}. Username: {username}. WebAuthn verification failed: {e:?}", ip.ip), ErrorEvent { event: EventType::UserFailedLogIn } - ) - } - }; - - // Find the matching credential and update its counter - let matched_wac = parsed_credentials.iter().find(|(_, p): &&(WebAuthnCredential, Passkey)| { - crypto::ct_eq(p.cred_id().as_slice(), authentication_result.cred_id().as_slice()) - }); + ), + }; - let Some((matched_wac, _)) = matched_wac else { + // 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!( - "Passkey authentication failed.", - format!("IP: {}. Username: {username}. Credential not found.", ip.ip), + AUTH_FAILED, + format!("IP: {}. Username: {username}. Verified credential is not registered.", ip.ip), ErrorEvent { event: EventType::UserFailedLogIn } ) }; - // Update the credential counter - let mut passkey: Passkey = serde_json::from_str(&matched_wac.credential)?; - if passkey.update_credential(&authentication_result) == Some(true) { - WebAuthnCredential::update_credential_by_uuid(&matched_wac.uuid, serde_json::to_string(&passkey)?, conn) - .await?; - } - - // Email verification check + // Reject an unverified account before persisting anything for this login. let now = Utc::now().naive_utc(); if user.verified_at.is_none() && CONFIG.mail_enabled() && CONFIG.signups_verify() { if user.last_verifying_at.is_none() @@ -1369,6 +1331,12 @@ async fn webauthn_login(data: ConnectData, user_id: &mut Option, conn: & ) } + // Persist any signature-counter advance from this assertion. + if passkey.update_credential(&authentication_result) == Some(true) { + matched_wac.credential = serde_json::to_string(&passkey)?; + matched_wac.update_credential(conn).await?; + } + let mut device = get_device(&data, conn, &user).await?; let auth_tokens = auth::AuthTokens::new(&device, &user, AuthMethod::WebAuthn, data.client_id); diff --git a/src/auth.rs b/src/auth.rs index 7f2e9712..115522a3 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -1292,7 +1292,9 @@ pub async fn refresh_tokens( sso::exchange_refresh_token(&device, &user, client_id, refresh_claims).await? } AuthMethod::Sso => err!("SSO is now disabled, Login again using email and master password"), - AuthMethod::Password if CONFIG.sso_enabled() && CONFIG.sso_only() => err!("SSO is now required, Login again"), + AuthMethod::Password | AuthMethod::WebAuthn if CONFIG.sso_enabled() && CONFIG.sso_only() => { + err!("SSO is now required, Login again") + } AuthMethod::Password | AuthMethod::WebAuthn => AuthTokens::new(&device, &user, refresh_claims.sub, client_id), _ => err!("Invalid auth method, cannot refresh token"), }; diff --git a/src/config.rs b/src/config.rs index 5c826fe9..42a8e5e3 100644 --- a/src/config.rs +++ b/src/config.rs @@ -567,6 +567,9 @@ make_config! { /// Purge incomplete SSO auth. |> Cron schedule of the job that cleans leftover auth in db due to incomplete SSO login. /// Defaults to daily. Set blank to disable this job. purge_incomplete_sso_auth: String, false, def, "0 20 0 * * *".to_owned(); + /// Passkey login challenge cleanup schedule |> Cron schedule of the job that cleans expired passkey-login challenges from the database. + /// Defaults to hourly. Set blank to disable this job. + webauthn_login_challenge_purge_schedule: String, false, def, "0 30 * * * *".to_owned(); }, /// General settings @@ -1242,6 +1245,12 @@ fn validate_config(cfg: &ConfigItems, on_update: bool) -> Result<(), Error> { err!("`AUTH_REQUEST_PURGE_SCHEDULE` is not a valid cron expression") } + if !cfg.webauthn_login_challenge_purge_schedule.is_empty() + && cfg.webauthn_login_challenge_purge_schedule.parse::().is_err() + { + err!("`WEBAUTHN_LOGIN_CHALLENGE_PURGE_SCHEDULE` is not a valid cron expression") + } + if !cfg.disable_admin_token { match cfg.admin_token.as_ref() { Some(t) if t.starts_with("$argon2") => { diff --git a/src/db/models/mod.rs b/src/db/models/mod.rs index aa94d94b..f37a6ff8 100644 --- a/src/db/models/mod.rs +++ b/src/db/models/mod.rs @@ -44,4 +44,6 @@ pub use self::two_factor::{TwoFactor, TwoFactorType}; pub use self::two_factor_duo_context::TwoFactorDuoContext; pub use self::two_factor_incomplete::TwoFactorIncomplete; pub use self::user::{Invitation, SsoUser, User, UserId, UserKdfType, UserStampException}; -pub use self::web_authn_credential::{WebAuthnCredential, WebAuthnCredentialId}; +pub use self::web_authn_credential::{ + WebAuthnCredential, WebAuthnCredentialId, WebAuthnLoginChallenge, WebAuthnLoginChallengeId, +}; diff --git a/src/db/models/web_authn_credential.rs b/src/db/models/web_authn_credential.rs index d432e23c..01d19d78 100644 --- a/src/db/models/web_authn_credential.rs +++ b/src/db/models/web_authn_credential.rs @@ -1,17 +1,21 @@ +use chrono::{NaiveDateTime, TimeDelta, Utc}; use derive_more::{AsRef, Deref, Display, From}; use diesel::prelude::*; use macros::UuidFromParam; use crate::api::EmptyResult; -use crate::db::DbConn; -use crate::db::schema::web_authn_credentials; +use crate::db::schema::{web_authn_credentials, web_authn_login_challenges}; +use crate::db::{DbConn, DbPool}; use crate::error::MapResult; +use crate::util::get_uuid; use super::UserId; -#[derive(Debug, Identifiable, Queryable, Insertable, AsChangeset)] +/// How long a pending passkey-login challenge stays valid before it is rejected. +const WEBAUTHN_LOGIN_CHALLENGE_TTL_SECONDS: i64 = 300; + +#[derive(Debug, Identifiable, Queryable, Insertable)] #[diesel(table_name = web_authn_credentials)] -#[diesel(treat_none_as_null = true)] #[diesel(primary_key(uuid))] pub struct WebAuthnCredential { pub uuid: WebAuthnCredentialId, @@ -35,7 +39,7 @@ impl WebAuthnCredential { encrypted_private_key: Option, ) -> Self { Self { - uuid: WebAuthnCredentialId(crate::util::get_uuid()), + uuid: WebAuthnCredentialId(get_uuid()), user_uuid, name, credential, @@ -46,6 +50,25 @@ impl WebAuthnCredential { } } + /// Whether this credential carries a complete PRF "rotateable key set", + /// i.e. passwordless vault decryption is fully enabled for it. + pub fn has_prf_keyset(&self) -> bool { + self.supports_prf + && self.encrypted_user_key.is_some() + && self.encrypted_public_key.is_some() + && self.encrypted_private_key.is_some() + } + + /// Bitwarden `WebAuthnPrfStatus`: 0 = Enabled, 1 = Supported, 2 = Unsupported. + /// Mirrors `WebAuthnCredential.GetPrfStatus()` in the upstream Bitwarden server. + pub fn prf_status(&self) -> i32 { + match (self.supports_prf, self.has_prf_keyset()) { + (false, _) => 2, // Unsupported + (true, true) => 0, // Enabled + (true, false) => 1, // Supported + } + } + pub async fn save(&self, conn: &DbConn) -> EmptyResult { db_run! { conn: { diesel::insert_into(web_authn_credentials::table) @@ -55,12 +78,50 @@ impl WebAuthnCredential { }} } - pub async fn find_all_by_user(user_uuid: &UserId, conn: &DbConn) -> Vec { + /// Persist the serialized passkey blob after a successful assertion advances + /// its signature counter. Touches only the `credential` column so a concurrent + /// key rotation cannot clobber it (see [`Self::update_keys`]). + pub async fn update_credential(&self, conn: &DbConn) -> EmptyResult { + db_run! { conn: { + diesel::update(web_authn_credentials::table.filter(web_authn_credentials::uuid.eq(&self.uuid))) + .set(web_authn_credentials::credential.eq(&self.credential)) + .execute(conn) + .map_res("Error updating web_authn_credential signature counter") + }} + } + + /// Persist the PRF unlock blobs that the rotation flow re-encrypts under the + /// new account key. Touches only the two columns that key rotation actually + /// changes, so it cannot clobber a concurrent signature-counter advance (see + /// [`Self::update_credential`]) nor the enrollment-time `encrypted_private_key`. + pub async fn update_keys(&self, conn: &DbConn) -> EmptyResult { + db_run! { conn: { + diesel::update(web_authn_credentials::table.filter(web_authn_credentials::uuid.eq(&self.uuid))) + .set(( + web_authn_credentials::encrypted_user_key.eq(&self.encrypted_user_key), + web_authn_credentials::encrypted_public_key.eq(&self.encrypted_public_key), + )) + .execute(conn) + .map_res("Error updating web_authn_credential keys") + }} + } + + pub async fn find_by_user(user_uuid: &UserId, conn: &DbConn) -> Vec { db_run! { conn: { web_authn_credentials::table .filter(web_authn_credentials::user_uuid.eq(user_uuid)) .load::(conn) - .unwrap_or_default() + .expect("Error loading web_authn_credentials") + }} + } + + pub async fn find_by_uuid_and_user(uuid: &WebAuthnCredentialId, user_uuid: &UserId, conn: &DbConn) -> Option { + db_run! { conn: { + web_authn_credentials::table + .filter(web_authn_credentials::uuid.eq(uuid)) + .filter(web_authn_credentials::user_uuid.eq(user_uuid)) + .first::(conn) + .ok() }} } @@ -80,32 +141,89 @@ impl WebAuthnCredential { }} } - pub async fn update_credential_by_uuid( - uuid: &WebAuthnCredentialId, - credential: String, - conn: &DbConn, - ) -> EmptyResult { + pub async fn delete_all_by_user(user_uuid: &UserId, conn: &DbConn) -> EmptyResult { db_run! { conn: { - diesel::update( - web_authn_credentials::table - .filter(web_authn_credentials::uuid.eq(uuid)), - ) - .set(web_authn_credentials::credential.eq(credential)) - .execute(conn) - .map_res("Error updating credential for web_authn_credential") + diesel::delete(web_authn_credentials::table.filter(web_authn_credentials::user_uuid.eq(user_uuid))) + .execute(conn) + .map_res("Error deleting all web_authn_credentials for user") }} } +} - pub async fn delete_all_by_user(user_uuid: &UserId, conn: &DbConn) -> EmptyResult { +/// A pending passkey-login (discoverable credential) authentication challenge. +/// +/// The login ceremony begins before the user is known, so the challenge state +/// cannot be tied to a `twofactor` row. It is persisted here keyed by a random +/// single-use token and consumed exactly once by [`WebAuthnLoginChallenge::take`]. +#[derive(Debug, Queryable, Insertable)] +#[diesel(table_name = web_authn_login_challenges)] +pub struct WebAuthnLoginChallenge { + pub id: WebAuthnLoginChallengeId, + pub challenge: String, + pub created_at: NaiveDateTime, +} + +impl WebAuthnLoginChallenge { + pub fn new(challenge: String) -> Self { + Self { + id: WebAuthnLoginChallengeId(get_uuid()), + challenge, + created_at: Utc::now().naive_utc(), + } + } + + pub async fn save(&self, conn: &DbConn) -> EmptyResult { db_run! { conn: { - diesel::delete( - web_authn_credentials::table - .filter(web_authn_credentials::user_uuid.eq(user_uuid)), - ) - .execute(conn) - .map_res("Error deleting all web_authn_credentials for user") + diesel::insert_into(web_authn_login_challenges::table) + .values(self) + .execute(conn) + .map_res("Error saving web_authn_login_challenge") }} } + + /// Fetch and delete a pending challenge (single-use). Returns `None` when the + /// 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. + let taken = conn + .transaction::, diesel::result::Error, _>(|conn| { + let challenge = web_authn_login_challenges::table + .filter(web_authn_login_challenges::id.eq(id)) + .first::(conn) + .optional()?; + let deleted = diesel::delete( + web_authn_login_challenges::table.filter(web_authn_login_challenges::id.eq(id)), + ) + .execute(conn)?; + Ok(challenge.filter(|_| deleted == 1)) + }) + .unwrap_or(None); + + let cutoff = Utc::now().naive_utc() - TimeDelta::seconds(WEBAUTHN_LOGIN_CHALLENGE_TTL_SECONDS); + taken.filter(|c| c.created_at >= cutoff) + }} + } + + /// Scheduled cleanup of challenges that were started but never consumed. + pub async fn delete_expired(pool: DbPool) -> EmptyResult { + debug!("Purging expired web_authn_login_challenges"); + if let Ok(conn) = pool.get().await { + let cutoff = Utc::now().naive_utc() - TimeDelta::seconds(WEBAUTHN_LOGIN_CHALLENGE_TTL_SECONDS); + db_run! { conn: { + diesel::delete(web_authn_login_challenges::table.filter(web_authn_login_challenges::created_at.lt(cutoff))) + .execute(conn) + .map_res("Error deleting expired web_authn_login_challenges") + }} + } else { + err!("Failed to get DB connection while purging expired web_authn_login_challenges") + } + } } #[derive( @@ -125,3 +243,64 @@ impl WebAuthnCredential { UuidFromParam, )] pub struct WebAuthnCredentialId(String); + +#[derive( + Clone, + Debug, + AsRef, + Deref, + DieselNewType, + Display, + From, + FromForm, + Hash, + PartialEq, + Eq, + Serialize, + Deserialize, + UuidFromParam, +)] +pub struct WebAuthnLoginChallengeId(String); + +#[cfg(test)] +mod tests { + use super::*; + + fn cred( + supports_prf: bool, + user_key: Option<&str>, + pub_key: Option<&str>, + priv_key: Option<&str>, + ) -> WebAuthnCredential { + WebAuthnCredential::new( + UserId::from(String::from("00000000-0000-0000-0000-000000000000")), + String::from("test"), + String::from("{}"), + supports_prf, + user_key.map(String::from), + pub_key.map(String::from), + priv_key.map(String::from), + ) + } + + // Bitwarden WebAuthnPrfStatus: Enabled = 0, Supported = 1, Unsupported = 2. + #[test] + fn prf_status_unsupported_when_authenticator_has_no_prf() { + assert_eq!(cred(false, None, None, None).prf_status(), 2); + // No PRF support means Unsupported even if blobs are somehow present. + assert_eq!(cred(false, Some("u"), Some("p"), Some("k")).prf_status(), 2); + } + + #[test] + fn prf_status_supported_when_prf_capable_but_keyset_incomplete() { + assert_eq!(cred(true, None, None, None).prf_status(), 1); + assert_eq!(cred(true, Some("u"), Some("p"), None).prf_status(), 1); + } + + #[test] + fn prf_status_enabled_only_with_a_complete_keyset() { + let c = cred(true, Some("u"), Some("p"), Some("k")); + assert!(c.has_prf_keyset()); + assert_eq!(c.prf_status(), 0); + } +} diff --git a/src/db/schema.rs b/src/db/schema.rs index 2f411151..bc3905de 100644 --- a/src/db/schema.rs +++ b/src/db/schema.rs @@ -351,9 +351,6 @@ table! { } } -joinable!(archives -> users (user_uuid)); -joinable!(archives -> ciphers (cipher_uuid)); - table! { web_authn_credentials (uuid) { uuid -> Text, @@ -367,6 +364,17 @@ table! { } } +table! { + web_authn_login_challenges (id) { + id -> Text, + challenge -> Text, + created_at -> Timestamp, + } +} + +joinable!(archives -> users (user_uuid)); +joinable!(archives -> ciphers (cipher_uuid)); + joinable!(attachments -> ciphers (cipher_uuid)); joinable!(ciphers -> organizations (organization_uuid)); joinable!(ciphers -> users (user_uuid)); @@ -424,4 +432,5 @@ allow_tables_to_appear_in_same_query!( event, auth_requests, web_authn_credentials, + web_authn_login_challenges, ); diff --git a/src/main.rs b/src/main.rs index 15467ea4..8b589dcb 100644 --- a/src/main.rs +++ b/src/main.rs @@ -745,6 +745,13 @@ fn schedule_jobs(pool: db::DbPool) { })); } + // Purge expired passkey-login challenges (default to hourly). + if !CONFIG.webauthn_login_challenge_purge_schedule().is_empty() { + sched.add(Job::new(CONFIG.webauthn_login_challenge_purge_schedule().parse().unwrap(), || { + runtime.spawn(db::models::WebAuthnLoginChallenge::delete_expired(pool.clone())); + })); + } + // Periodically check for jobs to run. We probably won't need any // jobs that run more often than once a minute, so a default poll // interval of 30 seconds should be sufficient. Users who want to