WebAuthn specification violations #597

New Issue

OVERLORD · 2025-10-09T16:40:25+03:00

OVERLORD commented

2025-10-09 16:40:25 +03:00

Originally created by @zacknewman on GitHub.

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 GitHub. 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.

OVERLORD added the enhancement low priority labels 2025-10-09 16:40:25 +03:00

OVERLORD closed this issue

2025-10-09 16:40:26 +03:00

OVERLORD commented

2025-10-09 16:40:29 +03:00

@tessus commented on GitHub:

The vw devs usually keep in sync what Bitwarden does. Even, if it's questionable on bw's side.
I think it would be best to get this fixed in bw first.

@tessus commented on GitHub: The vw devs usually keep in sync what Bitwarden does. Even, if it's questionable on bw's side. I think it would be best to get this fixed in bw first.

OVERLORD commented

2025-10-09 16:40:29 +03:00

@HammyHavoc commented on GitHub:

Any further updates regarding this?

@HammyHavoc commented on GitHub: Any further updates regarding this?

OVERLORD commented

2025-10-09 16:40:29 +03:00

@zacknewman commented on GitHub:

There is a difference between "works" and "works correctly". It violates the spec in two ways, so it's unfortunate a fix won't be made since it's quite easy regardless if Bitwarden violates the spec. Vaultwarden does other things in the interest of "security" that Bitwarden does not, so doing something differently is clearly not an issue for Vaultwarden.

Also there are two issues here. Bitwarden correctly verifies the credential ID is not currently used. So regarding the second issue, Vaultwarden is not only reducing the security and violating the spec; but it is also not doing what Bitwarden is correctly doing and what the used crate even says you MUST do.

@zacknewman commented on GitHub: There is a difference between "works" and "works correctly". It violates the spec in two ways, so it's unfortunate a fix won't be made since it's quite easy regardless if Bitwarden violates the spec. Vaultwarden does other things in the interest of "security" that Bitwarden does not, so doing something differently is clearly not an issue for Vaultwarden. Also there are _two_ issues here. Bitwarden correctly [verifies the credential ID](https://github.com/bitwarden/server/blob/main/src/Core/Auth/UserFeatures/WebAuthnLogin/Implementations/CreateWebAuthnLoginCredentialCommand.cs#L31) is not currently used. So regarding the second issue, Vaultwarden is not only reducing the security and violating the spec; but it is also not doing what Bitwarden is correctly doing and what the [used crate even says you MUST](https://docs.rs/webauthn-rs/latest/webauthn_rs/struct.Webauthn.html#returns-4) do.

OVERLORD commented

2025-10-09 16:40:29 +03:00

@BlackDex commented on GitHub:

Any further updates regarding this?

Update in what sense? It works currently. And since both client and server communicate this in there way which works, and nothing uses this in any other way, it's not going to be a big issue as-is.

@BlackDex commented on GitHub: > Any further updates regarding this? Update in what sense? It works currently. And since both client and server communicate this in there way which works, and nothing uses this in any other way, it's not going to be a big issue as-is.

OVERLORD commented

2025-10-09 16:40:30 +03:00

@zacknewman commented on GitHub:

On Dec 22, 2023, at 1:25 AM, Helmut K. C. Tessarek @.***> wrote:

The vw devs usually keep in sync what Bitwarden does. Even, if it's questionable on bw's side.
I think it would be best to get this fixed in bw first.

Huh? The issue with Bitwarden is largely unrelated. CredentialD is not being verified to not already exist, and the JSON payload is being altered including the values of the key-value pairs which violates the spec.

The Bitwarden issue only impacts how to solve the latter problem. I solve the problem by not altering any of the payload which besides being correct is much easier since I can just derive Deserialize, and I don’t have to attempt to get the innards of RegisterPublicKeyCredential.

If one insists on not using a 2-line patch involving sed on the web-vault to alter the two incorrect key names, then that doesn’t excuse not solving both problems. The first problem is completely unrelated, and the second problem should be solved by only altering the two problematic key names and leaving the rest of the payload alone especially the values.

@zacknewman commented on GitHub: > On Dec 22, 2023, at 1:25 AM, Helmut K. C. Tessarek ***@***.***> wrote: > > > The vw devs usually keep in sync what Bitwarden does. Even, if it's questionable on bw's side. > I think it would be best to get this fixed in bw first. > Huh? The issue with Bitwarden is largely unrelated. CredentialD is not being verified to not already exist, and the JSON payload is being altered _including_ the values of the key-value pairs which violates the spec. The Bitwarden issue only impacts _how_ to solve the latter problem. I solve the problem by not altering any of the payload which besides being correct is much easier since I can just derive Deserialize, and I don’t have to attempt to get the innards of RegisterPublicKeyCredential. If one insists on not using a 2-line patch involving sed on the web-vault to alter the two incorrect key names, then that doesn’t excuse not solving both problems. The first problem is completely unrelated, and the second problem should be solved by _only_ altering the two problematic key names and leaving the rest of the payload alone _especially_ the values.

OVERLORD commented

2025-10-09 16:40:31 +03:00

@zacknewman commented on GitHub:

Any further updates regarding this?

It's important to keep in mind these WebAuthn violations are probably OK. The spec requires at least 100 bits of entropy to be associated with a non-encrypted credential ID, so the odds a credential ID is already used is as I said "less than 0.00000000000000000009%" after 50K registrations. Of course that's assuming accidental collision. I'm not aware of a malicious way that subverts this, but of course that doesn't mean there isn't one. That's why it's important to err on the side of caution and just adhere to the spec.

@zacknewman commented on GitHub: > Any further updates regarding this? It's important to keep in mind these WebAuthn violations are _probably_ OK. The spec requires at least 100 bits of entropy to be associated with a non-encrypted credential ID, so the odds a credential ID is already used is as I said "less than 0.00000000000000000009%" after 50K registrations. Of course that's assuming _accidental_ collision. I'm not aware of a malicious way that subverts this, but of course that doesn't mean there isn't one. That's why it's important to err on the side of caution and just adhere to the spec.

OVERLORD commented

2025-10-09 16:40:31 +03:00

@BlackDex commented on GitHub:

a fix won't be made

@zacknewman Since you already put in the work to upgrade the webauthn-rs crate can't you make a PR?

That would be nice. But i think the implementation done by @zacknewman doesn't take into account a migration path of the old implementation to the new one reading the comments.

@BlackDex commented on GitHub: > > a fix won't be made > > @zacknewman Since you already put in the work to upgrade the `webauthn-rs` crate can't you make a PR? That would be nice. But i think the implementation done by @zacknewman doesn't take into account a migration path of the old implementation to the new one reading the comments.

OVERLORD commented

2025-10-09 16:40:32 +03:00

@zacknewman commented on GitHub:

@zacknewman Since you already put in the work to upgrade the webauthn-rs crate can't you make a PR?

@stefan0xC, I did this only on my personal fork which has diverged rather substantially from Vaultwarden; so it would require a little time to apply the change to Vaultwarden.

That would be nice. But i think the implementation done by @zacknewman doesn't take into account a migration path of the old implementation to the new one reading the comments.

@BlackDex, indeed; and that requires discussion on how best to handle that. At least initially it would seem the best way to handle that is by using CredentialV3, then eventually move away from that entirely.

Personally, I'm not interested in making a PR that doesn't assume compliant clients which unfortunately requires a very easy patch to the web-vault. As I explain in that comment, it seems like a no-brainer that that is the best way forward.

I might be amenable to the idea of creating a serde::de::Visitor that alters the JSON keys AttestationObject and clientDataJson but otherwise leaves the rest of the payload alone; as that at least allows one to avoid having to get into the internals of webauthn-rs which as explained here and in that comment only perpetuates the problem of not upgrading the crate in addition to making code audits and the like more complex. I still very much dislike that approach though for the reasons provided in that comment.

@zacknewman commented on GitHub: > @zacknewman Since you already put in the work to upgrade the `webauthn-rs` crate can't you make a PR? @stefan0xC, I did this only on my personal fork which has diverged rather substantially from Vaultwarden; so it would require a little time to apply the change to Vaultwarden. > That would be nice. But i think the implementation done by @zacknewman doesn't take into account a migration path of the old implementation to the new one reading the comments. @BlackDex, indeed; and that requires discussion on how best to handle that. At least initially it would seem the best way to handle that is by using [`CredentialV3`](https://docs.rs/webauthn-rs-core/latest/webauthn_rs_core/interface/struct.CredentialV3.html), then eventually move away from that entirely. Personally, I'm not interested in making a PR that doesn't assume compliant clients which unfortunately requires a [very easy patch to the web-vault](https://github.com/dani-garcia/vaultwarden/discussions/4210#discussioncomment-7977014). As I explain in that comment, it seems like a no-brainer that that is the best way forward. I might be amenable to the idea of creating a `serde::de::Visitor` that alters the JSON keys `AttestationObject` and `clientDataJson` but otherwise leaves the rest of the payload alone; as that at least allows one to avoid having to get into the internals of `webauthn-rs` which as explained here and in that comment only perpetuates the problem of not upgrading the crate in addition to making code audits and the like more complex. I still very much dislike that approach though for the reasons provided in that comment.

OVERLORD commented

2025-10-09 16:40:33 +03:00

@zacknewman commented on GitHub:

I should also add that fixing the second issue doesn't require a patch to the web-vault or upgrading webauthn-rs. That can be achieved most easily by creating a webauthn table as I explained and rely on the PRIMARY KEY to prevent duplicate credential IDs. A much less efficient way is to extract the credential ID from every security key associated with every user from twofactor and putting that into a HashSet and verifying the new credential ID is not in that.

@zacknewman commented on GitHub: I should also add that fixing the second issue doesn't require a patch to the web-vault or upgrading `webauthn-rs`. That can be achieved most easily by creating a `webauthn` table as I explained and rely on the `PRIMARY KEY` to prevent duplicate credential IDs. A much less efficient way is to extract the credential ID from every security key associated with every user from `twofactor` and putting that into a `HashSet` and verifying the new credential ID is not in that.

OVERLORD commented

2025-10-09 16:40:34 +03:00

@stefan0xC commented on GitHub:

a fix won't be made

@zacknewman Since you already put in the work to upgrade the webauthn-rs crate can't you make a PR?

@stefan0xC commented on GitHub: > a fix won't be made @zacknewman Since you already put in the work to upgrade the `webauthn-rs` crate can't you make a PR?

OVERLORD commented

2025-10-09 16:40:35 +03:00

@BlackDex commented on GitHub:

Well, since there already is a PR in the make to update the webauthn #5934, which looks like almost ready to merge, I'm going to close this. It doesn't matter really if updating to the newer webauthn solves all these issues or not. Since we are mainly linked to how the Bitwarden clients work, and we need to stay compatible with those, and also do not want to break during a migration. And, we also do not change the cases anymore, I'm going to close this one.

If that PR still doesn't provide or uphold the full RFC specs, you are always free to provide a PR which does.

@BlackDex commented on GitHub: Well, since there already is a PR in the make to update the webauthn #5934, which looks like almost ready to merge, I'm going to close this. It doesn't matter really if updating to the newer webauthn solves all these issues or not. Since we are mainly linked to how the Bitwarden clients work, and we need to stay compatible with those, and also do not want to break during a migration. And, we also do not change the cases anymore, I'm going to close this one. If that PR still doesn't provide or uphold the full RFC specs, you are always free to provide a PR which does.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: starred/vaultwarden#597