WebAuthn specification violations #1300

New Issue

MrUnknownDE · 2026-04-06T01:44:20+02:00

MrUnknownDE commented

2026-04-06 01:44:20 +02:00

Originally created by @zacknewman on 12/22/2023

The current WebAuthn code violates the specification. I'm not classifying this as a vulnerability, but one could argue any violation of something as important as authentication should be classified as such.

api::core::two_factor::{activate_webauthn, activate_webauthn_put} currently convert the JSON payload via util::UpCase. This violates the spec which states the raw payload MUST be unaltered (e.g., Section 7.1 Item 18).
CredentialID MUST be verified to not already have been assigned. Despite the TODO comment in code, this is not being done.

Why am I not classifying this as a vulnerability? Assuming non-malicious code, the probability of accepting an incorrect challenge response due to case alone is quite small. The probability of a collision of CredentialID is also extremely small (e.g., the probability of a collision after 50K registrations is less than 0.00000000000000000009%). I don't know enough about attacks against this, but hopefully the most sophisticated attack doesn't reduce the security too much (e.g., 128-bit AES is "only" 126-bit secure with the most sophisticated attacks).

How to fix

On my fork I upgraded webauthn-rs to 0.4.8. I created a webauthn table using the DDL query:

CREATE TABLE webauthn (
    credential_id TEXT NOT NULL PRIMARY KEY,
    uuid          TEXT NOT NULL,
    FOREIGN KEY(uuid) REFERENCES twofactor(uuid)
) WITHOUT ROWID;
CREATE INDEX webauthn_uuid_idx ON webauthn(uuid);

I perform all DML queries in a transaction to ensure atomicity between twofactor and webauthn. This also allows me to avoid having to explicitly check the webauthn_rs::prelude::CredentialID doesn't already exist in webauthn since a PRIMARY KEY violation will occur on the INSERT. I only use rocket::serde::json::Json for api::core::two_factor::webauthn::EnableWebauthnData instead of api::JsonUpcase. As added bonuses, deserializing EnableWebauthnData is much easier since all the boilerplate wrapper types can just go away and constructing exclude_credentials to pass to webauthn_rs::Webauthn::start_securitykey_registration is easier and faster due to the INDEX webauthn.webauthn_uuid_idx, the UNIQUE constraint twofactor.user_uuid, and the PRIMARY KEY constraint twofactor.uuid.

Issues

Bitwarden also violates the spec by incorrectly using AttestationObject instead of attestationObject and clientDataJson instead of clientDataJSON. Fortunately only the web-vault is able to register WebAuthn keys—the iOS app, Android app, and Firefox extension all redirect to the web-vault—and I patched the web-vault such that the correct JSON keys are passed. If such a patch is undesired and Bitwarden drag their feet fixing it, then one can only alter those keys while leaving the rest of the payload alone. It's a lot easier to patch the web-vault though since there are so few instances where AttestationObject and clientDataJson need to be fixed.

*Originally created by @zacknewman on 12/22/2023* The current WebAuthn code violates the [specification](https://www.w3.org/TR/webauthn-2/#sctn-rp-operations). I'm not classifying this as a vulnerability, but one could argue any violation of something as important as authentication should be classified as such. * `api::core::two_factor::{activate_webauthn, activate_webauthn_put}` currently convert the JSON payload via `util::UpCase`. This violates the spec which states the raw payload MUST be unaltered (e.g., Section 7.1 Item 18). * `CredentialID` MUST be verified to not already have been assigned. Despite the `TODO` comment in code, this is not being done. Why am I not classifying this as a vulnerability? Assuming _non-malicious_ code, the probability of accepting an incorrect challenge response due to case alone is quite small. The probability of a collision of `CredentialID` is also extremely small (e.g., the probability of a collision after 50K registrations is less than 0.00000000000000000009%). I don't know enough about attacks against this, but hopefully the most sophisticated attack doesn't reduce the security too much (e.g., 128-bit AES is "only" 126-bit secure with the most sophisticated attacks). **How to fix** On my fork I upgraded `webauthn-rs` to `0.4.8`. I created a `webauthn` table using the DDL query: ```sql CREATE TABLE webauthn ( credential_id TEXT NOT NULL PRIMARY KEY, uuid TEXT NOT NULL, FOREIGN KEY(uuid) REFERENCES twofactor(uuid) ) WITHOUT ROWID; CREATE INDEX webauthn_uuid_idx ON webauthn(uuid); ``` I perform all DML queries in a transaction to ensure atomicity between `twofactor` and `webauthn`. This also allows me to avoid having to explicitly check the `webauthn_rs::prelude::CredentialID` doesn't already exist in `webauthn` since a `PRIMARY KEY` violation will occur on the `INSERT`. I only use `rocket::serde::json::Json` for `api::core::two_factor::webauthn::EnableWebauthnData` instead of `api::JsonUpcase`. As added bonuses, deserializing `EnableWebauthnData` is much easier since all the boilerplate wrapper types can just go away and constructing `exclude_credentials` to pass to `webauthn_rs::Webauthn::start_securitykey_registration` is easier and faster due to the `INDEX` `webauthn.webauthn_uuid_idx`, the `UNIQUE` constraint `twofactor.user_uuid`, and the `PRIMARY KEY` constraint `twofactor.uuid`. **Issues** Bitwarden also [violates the spec](https://github.com/bitwarden/clients/issues/7259) by incorrectly using `AttestationObject` instead of `attestationObject` and `clientDataJson` instead of `clientDataJSON`. Fortunately only the web-vault is able to register WebAuthn keys—the iOS app, Android app, and Firefox extension all redirect to the web-vault—and I patched the web-vault such that the correct JSON keys are passed. If such a patch is undesired and Bitwarden drag their feet fixing it, then one can _only_ alter those keys while leaving the rest of the payload alone. It's a lot easier to patch the web-vault though since there are so few instances where `AttestationObject` and `clientDataJson` need to be fixed.

MrUnknownDE closed this issue

2026-04-06 01:44:20 +02:00

MrUnknownDE added the low priority enhancement low priority low priority low priority low priority low priority low priority low priority low priority low priority enhancement enhancement enhancement enhancement enhancement enhancement enhancement enhancement enhancement enhancement enhancement enhancement enhancement enhancement enhancement enhancement enhancement enhancement labels 2026-04-06 01:44:24 +02:00

MrUnknownDE referenced this issue

2026-04-06 03:54:28 +02:00

F2A FIDO since 1.18.0 #2662

Sign in to join this conversation.

No Label enhancement enhancement enhancement enhancement enhancement enhancement enhancement enhancement enhancement enhancement enhancement enhancement enhancement enhancement enhancement enhancement enhancement enhancement enhancement low priority low priority low priority low priority low priority low priority low priority low priority low priority low priority

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: github/vaultwarden#1300