From 89a9ef9afadb8809fbb30f51045acb1e3a1485e0 Mon Sep 17 00:00:00 2001 From: Zaid Marji Date: Sat, 23 May 2026 12:51:55 +0300 Subject: [PATCH] Prevent duplicate passkey credential registration Add a credential_id_hash column containing sha256-hex of the raw WebAuthn credential ID and enforce it with a unique index across all three backends. Check credential_id_hash before saving to return a clean duplicate-passkey error in the common case, and map database unique violations to the same error to close the TOCTOU window. Keep PRF keyset/status unit coverage exhaustive so a refactor cannot accidentally report partial PRF state as enabled. --- .../up.sql | 2 + .../up.sql | 2 + .../up.sql | 2 + src/api/core/accounts.rs | 1 + src/api/core/mod.rs | 163 ++++++++++++++++++ src/api/core/two_factor/webauthn.rs | 42 +++++ src/api/identity.rs | 50 ++++++ src/db/models/web_authn_credential.rs | 98 ++++++++++- src/db/schema.rs | 2 +- 9 files changed, 358 insertions(+), 4 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 0780fbaf..29d7f544 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 @@ -3,6 +3,7 @@ CREATE TABLE web_authn_credentials ( user_uuid CHAR(36) NOT NULL, name TEXT NOT NULL, credential TEXT NOT NULL, + credential_id_hash VARCHAR(64) NOT NULL, supports_prf BOOLEAN NOT NULL DEFAULT 0, encrypted_user_key TEXT, encrypted_public_key TEXT, @@ -11,3 +12,4 @@ CREATE TABLE web_authn_credentials ( ); CREATE INDEX idx_web_authn_credentials_user_uuid ON web_authn_credentials (user_uuid); +CREATE UNIQUE INDEX idx_web_authn_credentials_credential_id_hash ON web_authn_credentials (credential_id_hash); 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 9cf20bec..0819a037 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 @@ -3,6 +3,7 @@ CREATE TABLE web_authn_credentials ( user_uuid CHAR(36) NOT NULL REFERENCES users(uuid) ON DELETE CASCADE, name TEXT NOT NULL, credential TEXT NOT NULL, + credential_id_hash VARCHAR(64) NOT NULL, supports_prf BOOLEAN NOT NULL DEFAULT FALSE, encrypted_user_key TEXT, encrypted_public_key TEXT, @@ -10,3 +11,4 @@ CREATE TABLE web_authn_credentials ( ); CREATE INDEX idx_web_authn_credentials_user_uuid ON web_authn_credentials (user_uuid); +CREATE UNIQUE INDEX idx_web_authn_credentials_credential_id_hash ON web_authn_credentials (credential_id_hash); 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 0a0b36bc..2b286ea6 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 @@ -3,6 +3,7 @@ CREATE TABLE web_authn_credentials ( user_uuid TEXT NOT NULL REFERENCES users(uuid) ON DELETE CASCADE, name TEXT NOT NULL, credential TEXT NOT NULL, + credential_id_hash TEXT NOT NULL, supports_prf BOOLEAN NOT NULL DEFAULT 0, encrypted_user_key TEXT, encrypted_public_key TEXT, @@ -10,3 +11,4 @@ CREATE TABLE web_authn_credentials ( ); CREATE INDEX idx_web_authn_credentials_user_uuid ON web_authn_credentials (user_uuid); +CREATE UNIQUE INDEX idx_web_authn_credentials_credential_id_hash ON web_authn_credentials (credential_id_hash); diff --git a/src/api/core/accounts.rs b/src/api/core/accounts.rs index b0ac52cd..6f9ea483 100644 --- a/src/api/core/accounts.rs +++ b/src/api/core/accounts.rs @@ -1805,6 +1805,7 @@ mod tests { UserId::from(String::from("user")), String::from("passkey"), String::from("{}"), + format!("{id}-credential-id-hash"), supports_prf, key.clone(), key.clone(), diff --git a/src/api/core/mod.rs b/src/api/core/mod.rs index 04172d92..632f08a1 100644 --- a/src/api/core/mod.rs +++ b/src/api/core/mod.rs @@ -278,6 +278,10 @@ fn passkey_assertion_challenge_state(data: &str, token: &str) -> ApiResult String { + crypto::sha256_hex(passkey.cred_id().as_slice()) +} + #[post("/webauthn/attestation-options", data = "")] async fn post_api_webauthn_attestation_options( data: Json, @@ -461,11 +465,17 @@ async fn post_api_webauthn( }; let state = passkey_registration_challenge_state(&tf.data, data.token.as_deref())?; let credential = WEBAUTHN.finish_passkey_registration(&data.device_response.into(), &state)?; + let credential_id_hash = passkey_credential_id_hash(&credential); + + if WebAuthnCredential::credential_id_hash_exists(&credential_id_hash, &conn).await { + err!("Passkey is already registered") + } WebAuthnCredential::new( user.uuid, data.name, serde_json::to_string(&credential)?, + credential_id_hash, data.supports_prf, data.encrypted_user_key, data.encrypted_public_key, @@ -581,6 +591,159 @@ async fn post_api_webauthn_delete( Ok(Status::Ok) } +#[cfg(test)] +mod tests { + use super::*; + use webauthn_rs::prelude::{ + AttestationFormat, COSEAlgorithm, COSEEC2Key, COSEKey, COSEKeyType, Credential, ECDSACurve, ParsedAttestation, + Url, Webauthn, WebauthnBuilder, + }; + use webauthn_rs_proto::{AuthenticatorTransport, RegisteredExtensions}; + + fn webauthn() -> Webauthn { + let origin = Url::parse("http://localhost").unwrap(); + WebauthnBuilder::new("localhost", &origin).unwrap().rp_name("localhost").build().unwrap() + } + + fn passkey() -> Passkey { + Credential { + cred_id: [1, 2, 3, 4].into(), + cred: COSEKey { + type_: COSEAlgorithm::ES256, + key: COSEKeyType::EC_EC2(COSEEC2Key { + curve: ECDSACurve::SECP256R1, + x: [1; 32].into(), + y: [2; 32].into(), + }), + }, + counter: 0, + transports: Some(vec![AuthenticatorTransport::Internal, AuthenticatorTransport::Hybrid]), + user_verified: true, + backup_eligible: false, + backup_state: false, + registration_policy: UserVerificationPolicy::Required, + extensions: RegisteredExtensions::none(), + attestation: ParsedAttestation::default(), + attestation_format: AttestationFormat::None, + } + .into() + } + + fn registration_state() -> PasskeyRegistration { + let user_uuid = uuid::Uuid::parse_str("00000000-0000-0000-0000-000000000000").unwrap(); + let (_challenge, state) = + webauthn().start_passkey_registration(user_uuid, "user@example.com", "user", None).unwrap(); + state + } + + #[test] + fn registration_challenge_accepts_wrapped_state_with_matching_token() { + let saved = WebAuthnPasskeyRegistrationChallenge { + token: String::from("token"), + state: registration_state(), + }; + let data = serde_json::to_string(&saved).unwrap(); + + assert!(passkey_registration_challenge_state(&data, Some("token")).is_ok()); + } + + #[test] + fn registration_challenge_rejects_wrapped_state_without_matching_token() { + let saved = WebAuthnPasskeyRegistrationChallenge { + token: String::from("token"), + state: registration_state(), + }; + let data = serde_json::to_string(&saved).unwrap(); + + assert!(passkey_registration_challenge_state(&data, Some("wrong")).is_err()); + assert!(passkey_registration_challenge_state(&data, None).is_err()); + } + + #[test] + fn legacy_registration_challenge_rejects_finish_token() { + let data = serde_json::to_string(®istration_state()).unwrap(); + + assert!(passkey_registration_challenge_state(&data, None).is_ok()); + assert!(passkey_registration_challenge_state(&data, Some("token")).is_err()); + } + + #[test] + fn assertion_challenge_rejects_mismatched_token() { + let (_response, state) = webauthn().start_passkey_authentication(&[passkey()]).unwrap(); + let saved = WebAuthnPasskeyAssertionChallenge { + token: String::from("token"), + state, + }; + let data = serde_json::to_string(&saved).unwrap(); + + assert!(passkey_assertion_challenge_state(&data, "token").is_ok()); + assert!(passkey_assertion_challenge_state(&data, "wrong").is_err()); + } + + #[test] + fn passkey_credential_id_hash_uses_raw_credential_id_bytes() { + assert_eq!( + passkey_credential_id_hash(&passkey()), + "9f64a747e1b97f131fabb6b447296c9b6f0201e79fb3c5356e6c77e89b6a806a" + ); + } + + fn passkey_with_cred_id(cred_id: &[u8]) -> Passkey { + Credential { + cred_id: cred_id.to_vec().into(), + cred: COSEKey { + type_: COSEAlgorithm::ES256, + key: COSEKeyType::EC_EC2(COSEEC2Key { + curve: ECDSACurve::SECP256R1, + x: [1; 32].into(), + y: [2; 32].into(), + }), + }, + counter: 0, + transports: None, + user_verified: true, + backup_eligible: false, + backup_state: false, + registration_policy: UserVerificationPolicy::Required, + extensions: RegisteredExtensions::none(), + attestation: ParsedAttestation::default(), + attestation_format: AttestationFormat::None, + } + .into() + } + + #[test] + fn passkey_credential_id_hash_is_deterministic() { + let cred_id: &[u8] = &[10, 20, 30, 40, 50]; + assert_eq!( + passkey_credential_id_hash(&passkey_with_cred_id(cred_id)), + passkey_credential_id_hash(&passkey_with_cred_id(cred_id)), + ); + } + + #[test] + fn passkey_credential_id_hash_distinguishes_different_credentials() { + let a = passkey_credential_id_hash(&passkey_with_cred_id(&[1, 2, 3, 4])); + let b = passkey_credential_id_hash(&passkey_with_cred_id(&[4, 3, 2, 1])); + let c = passkey_credential_id_hash(&passkey_with_cred_id(&[1, 2, 3])); + assert_ne!(a, b, "different bytes must produce different hashes"); + assert_ne!(a, c, "different lengths must produce different hashes"); + assert_ne!(b, c); + } + + /// `passkey_assertion_challenge_state` has no legacy unwrapped fallback — + /// the assertion-options endpoint was introduced together with the + /// wrapping struct, so any persisted state must carry the binding token. + #[test] + fn assertion_challenge_rejects_unwrapped_legacy_state() { + let (_response, state) = webauthn().start_passkey_authentication(&[passkey()]).unwrap(); + let bare = serde_json::to_string(&state).unwrap(); + + assert!(passkey_assertion_challenge_state(&bare, "any-token").is_err()); + assert!(passkey_assertion_challenge_state(&bare, "").is_err()); + } +} + #[get("/config")] fn config() -> Json { let domain = CONFIG.domain(); diff --git a/src/api/core/two_factor/webauthn.rs b/src/api/core/two_factor/webauthn.rs index c4884c3a..d8f6feea 100644 --- a/src/api/core/two_factor/webauthn.rs +++ b/src/api/core/two_factor/webauthn.rs @@ -518,3 +518,45 @@ fn check_and_update_backup_eligible( } Ok(false) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn register_public_key_credential_copy_preserves_transports() { + let transports = vec![AuthenticatorTransport::Internal, AuthenticatorTransport::Hybrid]; + let copy = RegisterPublicKeyCredentialCopy { + id: String::from("credential"), + raw_id: Base64UrlSafeData::from([1, 2, 3]), + response: AuthenticatorAttestationResponseRawCopy { + attestation_object: Base64UrlSafeData::from([4, 5, 6]), + client_data_json: Base64UrlSafeData::from([7, 8, 9]), + transports: Some(transports.clone()), + }, + r#type: String::from("public-key"), + }; + + let converted: RegisterPublicKeyCredential = copy.into(); + + assert_eq!(converted.response.transports, Some(transports)); + } + + #[test] + fn register_public_key_credential_copy_keeps_absent_transports_absent() { + let copy = RegisterPublicKeyCredentialCopy { + id: String::from("credential"), + raw_id: Base64UrlSafeData::from([1, 2, 3]), + response: AuthenticatorAttestationResponseRawCopy { + attestation_object: Base64UrlSafeData::from([4, 5, 6]), + client_data_json: Base64UrlSafeData::from([7, 8, 9]), + transports: None, + }, + r#type: String::from("public-key"), + }; + + let converted: RegisterPublicKeyCredential = copy.into(); + + assert_eq!(converted.response.transports, None); + } +} diff --git a/src/api/identity.rs b/src/api/identity.rs index 508d783f..13339d09 100644 --- a/src/api/identity.rs +++ b/src/api/identity.rs @@ -1644,3 +1644,53 @@ async fn authorize(data: AuthorizeData, cookies: &CookieJar<'_>, secure: Secure, Ok(Redirect::temporary(String::from(auth_url))) } + +#[cfg(test)] +mod tests { + use super::*; + use webauthn_rs::prelude::{ + AttestationFormat, COSEAlgorithm, COSEEC2Key, COSEKey, COSEKeyType, Credential, ECDSACurve, ParsedAttestation, + }; + use webauthn_rs_proto::{AuthenticatorTransport, RegisteredExtensions, UserVerificationPolicy}; + + fn passkey(transports: Option>) -> Passkey { + Credential { + cred_id: [1, 2, 3, 4].into(), + cred: COSEKey { + type_: COSEAlgorithm::ES256, + key: COSEKeyType::EC_EC2(COSEEC2Key { + curve: ECDSACurve::SECP256R1, + x: [1; 32].into(), + y: [2; 32].into(), + }), + }, + counter: 0, + transports, + user_verified: true, + backup_eligible: false, + backup_state: false, + registration_policy: UserVerificationPolicy::Required, + extensions: RegisteredExtensions::none(), + attestation: ParsedAttestation::default(), + attestation_format: AttestationFormat::None, + } + .into() + } + + #[test] + fn passkey_credential_id_returns_browser_credential_id() { + assert_eq!(passkey_credential_id(&passkey(None)).unwrap(), "AQIDBA"); + } + + #[test] + fn passkey_transports_returns_saved_transport_hints() { + let passkey = passkey(Some(vec![AuthenticatorTransport::Internal, AuthenticatorTransport::Hybrid])); + + assert_eq!(passkey_transports(&passkey), vec![String::from("internal"), String::from("hybrid")]); + } + + #[test] + fn passkey_transports_defaults_to_empty_when_absent() { + assert!(passkey_transports(&passkey(None)).is_empty()); + } +} diff --git a/src/db/models/web_authn_credential.rs b/src/db/models/web_authn_credential.rs index e4cc11de..4e79b064 100644 --- a/src/db/models/web_authn_credential.rs +++ b/src/db/models/web_authn_credential.rs @@ -22,6 +22,7 @@ pub struct WebAuthnCredential { pub user_uuid: UserId, pub name: String, pub credential: String, + pub credential_id_hash: String, pub supports_prf: bool, pub encrypted_user_key: Option, pub encrypted_public_key: Option, @@ -29,10 +30,15 @@ pub struct WebAuthnCredential { } impl WebAuthnCredential { + #[expect( + clippy::too_many_arguments, + reason = "Matches positional-arg constructor pattern used by all other model `new` functions" + )] pub fn new( user_uuid: UserId, name: String, credential: String, + credential_id_hash: String, supports_prf: bool, encrypted_user_key: Option, encrypted_public_key: Option, @@ -43,6 +49,7 @@ impl WebAuthnCredential { user_uuid, name, credential, + credential_id_hash, supports_prf, encrypted_user_key, encrypted_public_key, @@ -71,10 +78,30 @@ impl WebAuthnCredential { pub async fn save(&self, conn: &DbConn) -> EmptyResult { db_run! { conn: { - diesel::insert_into(web_authn_credentials::table) + let result = diesel::insert_into(web_authn_credentials::table) .values(self) - .execute(conn) - .map_res("Error saving web_authn_credential") + .execute(conn); + + match result { + Err(diesel::result::Error::DatabaseError(diesel::result::DatabaseErrorKind::UniqueViolation, _)) => { + Err(crate::error::Error::new( + "Passkey is already registered", + "Duplicate WebAuthn credential ID", + )) + } + result => result.map_res("Error saving web_authn_credential"), + } + }} + } + + pub async fn credential_id_hash_exists(credential_id_hash: &str, conn: &DbConn) -> bool { + db_run! { conn: { + web_authn_credentials::table + .filter(web_authn_credentials::credential_id_hash.eq(credential_id_hash)) + .select(web_authn_credentials::credential_id_hash) + .first::(conn) + .optional() + .is_ok_and(|credential| credential.is_some()) }} } @@ -293,6 +320,7 @@ mod tests { UserId::from(String::from("00000000-0000-0000-0000-000000000000")), String::from("test"), String::from("{}"), + String::from("credential-id-hash"), supports_prf, user_key.map(String::from), pub_key.map(String::from), @@ -320,4 +348,68 @@ mod tests { assert!(c.has_prf_keyset()); assert_eq!(c.prf_status(), 0); } + + /// Each `WebAuthnCredential::new` argument must land in the field that + /// shares its name. Without this, two adjacent `Option` args + /// could be swapped at a call site without the compiler noticing. + #[test] + fn new_assigns_each_argument_to_its_named_field() { + let cred = WebAuthnCredential::new( + UserId::from(String::from("user-uuid")), + String::from("display-name"), + String::from("credential-json"), + String::from("hash-value"), + true, + Some(String::from("user-key")), + Some(String::from("public-key")), + Some(String::from("private-key")), + ); + assert_eq!(cred.user_uuid.as_ref(), "user-uuid"); + assert_eq!(cred.name, "display-name"); + assert_eq!(cred.credential, "credential-json"); + assert_eq!(cred.credential_id_hash, "hash-value"); + assert!(cred.supports_prf); + assert_eq!(cred.encrypted_user_key.as_deref(), Some("user-key")); + assert_eq!(cred.encrypted_public_key.as_deref(), Some("public-key")); + assert_eq!(cred.encrypted_private_key.as_deref(), Some("private-key")); + } + + /// Exhaust the 2^4 truth table for `has_prf_keyset` and `prf_status`: + /// only `(supports_prf=true, all three blobs Some)` reports + /// `has_prf_keyset() == true` / `prf_status() == 0` (Enabled). Any + /// `supports_prf=false` row is Unsupported (2). Any `supports_prf=true` + /// row with at least one blob missing is Supported (1). The login + /// response's `WebAuthnPrfOption` gating depends on this enum, so the + /// full matrix is enforced to prevent a refactor accidentally + /// advertising PRF capability on a credential with an incomplete + /// keyset (which would leak partial state to the client). + #[test] + fn prf_status_full_truth_table() { + for supports_prf in [false, true] { + for user in [None, Some("u")] { + for pub_ in [None, Some("p")] { + for priv_ in [None, Some("k")] { + let c = cred(supports_prf, user, pub_, priv_); + let all_keys_present = user.is_some() && pub_.is_some() && priv_.is_some(); + let expected_has_keyset = supports_prf && all_keys_present; + let expected_status = match (supports_prf, expected_has_keyset) { + (false, _) => 2, // Unsupported + (true, false) => 1, // Supported (capable but incomplete) + (true, true) => 0, // Enabled + }; + assert_eq!( + c.has_prf_keyset(), + expected_has_keyset, + "has_prf_keyset(supports_prf={supports_prf}, user={user:?}, pub={pub_:?}, priv={priv_:?})", + ); + assert_eq!( + c.prf_status(), + expected_status, + "prf_status(supports_prf={supports_prf}, user={user:?}, pub={pub_:?}, priv={priv_:?})", + ); + } + } + } + } + } } diff --git a/src/db/schema.rs b/src/db/schema.rs index bc3905de..2f61707a 100644 --- a/src/db/schema.rs +++ b/src/db/schema.rs @@ -357,6 +357,7 @@ table! { user_uuid -> Text, name -> Text, credential -> Text, + credential_id_hash -> Text, supports_prf -> Bool, encrypted_user_key -> Nullable, encrypted_public_key -> Nullable, @@ -374,7 +375,6 @@ table! { 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));