WebAuthn specification violations

[zacknewman]

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.

[tessus]

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.

[zacknewman]

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.

[HammyHavoc]

Any further updates regarding this?

[BlackDex]

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.

[zacknewman]

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]

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.

[stefan0xC]

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?

[BlackDex]

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.

[zacknewman]

@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]

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.