WebAuthn specification violations

[zacknewman]

WebAuthn specification violations

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

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

[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?

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