search

bitwarden / clients

Bitwarden client apps (web, browser extension, desktop, and cli).
https://bitwarden.com
Other
9.31k stars 1.25k forks source link

WebAuthn API violation #7259

Closed zacknewman closed 10 months ago

zacknewman commented 11 months ago

Steps To Reproduce

Register a WebAuthn key and look at the raw HTTP JSON payload.

Expected Result

The JSON to conform to the WebAuthn spec.

Actual Result

The JSON for deviceResponse is invalid. Both AttestationObject and clientDataJson violate the WebAuthn spec. AuthenticatorAttestationResponse requires the first field to be attestationObject, and AuthenticatorResponse requires the latter to be clientDataJSON.

Screenshots or Videos

No response

Additional Context

After replacing all instances of AttestationObject with attestationObject and clientDataJson with clientDataJSON in the below files, the correct payload was sent:

apps/web/src/app/main.b193f247301cc37bf60b.js apps/web/src/app/main.b193f247301cc37bf60b.js.map apps/web/src/connectors/webauthn.b50e3a9527b3e4de41eb.js apps/web/src/connectors/webauthn.b50e3a9527b3e4de41eb.js.map apps/web/src/connectors/webauthn-fallback.4c0785415d6ab725a060.js apps/web/src/connectors/webauthn-fallback.4c0785415d6ab725a060.js.map

Operating System

Linux

Operating System Version

No response

Web Browser

Firefox

Browser Version

No response

Build Version

2023.12.0

Issue Tracking Info

sammbw commented 11 months ago

Thanks for reporting this to us - as this communication would relate specifically to the Bitwarden client and server, do you have a particular requirement or need for Bitwarden to adhere to the standard here?

zacknewman commented 11 months ago

On Dec 18, 2023, at 9:46 PM, sammbw @.***> wrote:

 Thanks for reporting this to us - as this communication would relate specifically to the Bitwarden client and server, do you have a particular requirement or need for Bitwarden to adhere to the standard here?

I’m implementing my own Bitwarden-compliant server, and it is a lot easier when RFCs and other standards are adhered to.

sammbw commented 10 months ago

Since compatibility with 3rd party servers is not something that we officially support, addressing this issue is not going to be high on our prioritization. Please keep in mind that the design of our web API used by our clients favors flexibility and can change without any prior warning as we work to bring our users the latest features as quick as possible.

zacknewman commented 10 months ago

On Dec 21, 2023, at 9:38 PM, sammbw @.***> wrote:

 Since compatibility with 3rd party servers is unfortunately not something that we officially support, addressing this issue is not going to be high on our prioritization. Please keep in mind that the design of our web API used by our clients favors flexibility and can change without any prior warning as we work to bring our users the latest features as quick as possible.

Of course. I was merely answering your question. I don’t expect decisions to be made to help third parties. I think the reason to fix this is if nothing else the optics. As a member of the FIDO Alliance, it doesn’t look good that the WebAuthn spec isn’t adhered to. It also suggests the server isn’t strictly enforcing the spec—if it were, the client wouldn’t work—which then begs the question “what else of the spec is not adhered to, and does that compromise the security?”

coroiu commented 10 months ago

I just want to start out by saying that this is not a violation of the WebAuthn specification. Applications are free to choose whatever format they want when communicating with their backend.

My understaing is that you are referring to the WebAuthn L3 Working Draft document (https://www.w3.org/TR/webauthn-3/) , specifically the toJSON method (https://www.w3.org/TR/webauthn-3/#dom-publickeycredential-tojson) that was added as part of https://github.com/w3c/webauthn/pull/1703 which specifies how browsers must implement JSON serialization of WebAuthn related objects.

The important point here is that the WebAuthn standard defines the interface between javascript applications and the browser. It does not define how RPs (e.g. Bitwarden) should send data to their respective backends. Not taking advantage of certain parts of that interface does not result in a violation of the specification.

Please also keep in mind that the toJSON method is currently marked as experimental https://developer.mozilla.org/en-US/docs/Web/API/PublicKeyCredential/toJSON#browser_compatibility

zacknewman commented 10 months ago

The server makes up the Relying Party, so the communication between it and the client is very much part of the WebAuthn spec. If you want to claim it's not a violation because the JSON methods are still experimental (i.e., part of the Level 3 API), then that simply means it will violate the spec once that is published and becomes official. This is not a case of talking about how the data is stored in a database or something. We are talking about the communication between the WebAuthn client (i.e., the web client) and the WebAuthn Relying Party (i.e., the server).

Instead of going back and forth on attaching a label to this, let's at least agree that the intent was to adhere to the key names mentioned in the Level 2 API. Why not have the web client send foo instead of clientDataJson?

If a fix is not made; then while unfortunate, it doesn't bother me because I've already fixed it myself.

Hinton commented 10 months ago

Hi,

As @coroiu wrote the specification doesn't cover internal communication within a web application. I've cited some relevant sections below explicitly clarifying this.

A Relying Party implementation typically consists of both some client-side script that invokes the Web Authentication API in the client, and a server-side component that executes the Relying Party operations and other application logic. Communication between the two components MUST use HTTPS or equivalent transport security, but is otherwise beyond the scope of this specification.

https://www.w3.org/TR/webauthn-3/#web-application

It must then deliver the contents of this structure to the Relying Party server, using methods outside the scope of this specification.

https://www.w3.org/TR/webauthn-3/#sctn-rp-operations

Closing this.

zacknewman commented 10 months ago

A Relying Party implementation typically consists of both some client-side script that invokes the Web Authentication API in the client, and a server-side component that executes the Relying Party operations and other application logic. Communication between the two components MUST use HTTPS or equivalent transport security, but is otherwise beyond the scope of this specification. https://www.w3.org/TR/webauthn-3/#web-application

It must then deliver the contents of this structure to the Relying Party server, using methods outside the scope of this specification. https://www.w3.org/TR/webauthn-3/#sctn-rp-operations

I don't interpret those sections as you do. While it's beyond the scope of the spec how communication is done, what is communicated is part of the spec. For example section 7.1 Item 3 states:

"Let response be credential.response. If response is not an instance of AuthenticatorAttestationResponse, abort the ceremony with a user-visible error."

AuthenticatorAttestationResponse has a property called attestationObject not AttestationObject. This means a strict interpretation of the spec leads to the conclusion that a "user-visible error" should be returned since an instance of AuthenticatorAttestationResponse was not received by the server-side component.

I will also link a recent PR merge in the FIDO2 library used by this project.

There a Bitwarden employee even states the following:

"For serialization/deserialization it should definitely be named clientDataJSON. That is what is used in the spec."