Closed Bug 1463170 Opened 2 years ago Closed 2 years ago

Web Authentication - Set AuthenticatorAssertionResponse.userHandle to null

Categories

(Core :: DOM: Device Interfaces, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox-esr60 61+ fixed
firefox60 --- wontfix
firefox61 + fixed
firefox62 --- fixed

People

(Reporter: jcj, Assigned: jcj)

References

(Blocks 1 open bug, )

Details

(Whiteboard: [webauthn] [webauthn-interop])

Attachments

(1 file)

The WebAuthn spec says to set `AuthenticatorAssertionResponse.userHandle` to null when the authenticator returns no user handle (e.g., when allowList is set), but we return an empty ArrayBuffer. This is because of the defaults in AuthenticatorAssertionResponse.h, as the field is itself unset. For spec-compliance, let's fix it.

[1]: https://www.w3.org/TR/webauthn/#ref-for-dom-authenticatorassertionresponse-userhandle%E2%91%A0
The WebAuthn spec says to set `AuthenticatorAssertionResponse.userHandle` to
null when the authenticator returns no user handle (e.g., when allowList is set),
but we return an empty ArrayBuffer. This is because of the defaults in
AuthenticatorAssertionResponse.h, as the field is itself unset.

We missed this change to the spec that happened in December [2], so this also
has a corresponding WebIDL update. I don't see any other instances of WebIDL
differences.

[1] https://www.w3.org/TR/webauthn/#ref-for-dom-authenticatorassertionresponse-userhandle%E2%91%A0
[2] https://github.com/w3c/webauthn/commit/3b2a1d141cbd8f2954f073a6b6598d954398a986
Comment on attachment 8979373 [details]
Bug 1463170 - Set AuthenticatorAssertionResponse.userHandle to null r?ttaubert r?smaug

Tim Taubert [:ttaubert] has approved the revision.

https://phabricator.services.mozilla.com/D1337
Attachment #8979373 - Flags: review+
Comment on attachment 8979373 [details]
Bug 1463170 - Set AuthenticatorAssertionResponse.userHandle to null r?ttaubert r?smaug

Phab didn't mark r? for :smaug :(
Attachment #8979373 - Flags: review?(bugs)
Please don't link to W3C TR/ specs if there is an editor's draft available.
(TR stands for trash, well, not officially.)
Comment on attachment 8979373 [details]
Bug 1463170 - Set AuthenticatorAssertionResponse.userHandle to null r?ttaubert r?smaug

Olli Pettay [:smaug] has approved the revision.

https://phabricator.services.mozilla.com/D1337
Attachment #8979373 - Flags: review+
Comment on attachment 8979373 [details]
Bug 1463170 - Set AuthenticatorAssertionResponse.userHandle to null r?ttaubert r?smaug

(uh, Phabricator's bugzilla integration is so broken)
Attachment #8979373 - Flags: review?(bugs)
Comment on attachment 8979373 [details]
Bug 1463170 - Set AuthenticatorAssertionResponse.userHandle to null r?ttaubert r?smaug

Flag set by Tim Taubert [:ttaubert] is no longer active.
Flag set by Olli Pettay [:smaug] is no longer active.

https://phabricator.services.mozilla.com/D1337
Attachment #8979373 - Flags: review+
Comment on attachment 8979373 [details]
Bug 1463170 - Set AuthenticatorAssertionResponse.userHandle to null r?ttaubert r?smaug

Olli Pettay [:smaug] has approved the revision.
Tim Taubert [:ttaubert] has approved the revision.

https://phabricator.services.mozilla.com/D1337
Attachment #8979373 - Flags: review+
Pushed by jjones@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c95faed62ee
Set AuthenticatorAssertionResponse.userHandle to null r=ttaubert r=smaug
Marking ESR60, 61 and 62 as affected. I think we'll want to uplift this into ESR 60 since it's a signature feature and a spec incompatibility. We might want to uplift to Beta also, but I'll wait on that until we get 24 hours in Nightly at least.
https://hg.mozilla.org/mozilla-central/rev/3c95faed62ee
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
If we want this on ESR60, then we want it on Beta also. Please nominate for approval when you feel that it's baked long enough on Nightly.
Flags: needinfo?(jjones)
Comment on attachment 8979373 [details]
Bug 1463170 - Set AuthenticatorAssertionResponse.userHandle to null r?ttaubert r?smaug

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
This is a spec-compliance issue with Web Authentication.

User impact if declined: 
Divergent behavior between ESR and Release for WebAuthn.

Fix Landed on Version: 
62

Risk to taking this patch (and alternatives if risky): 
This is isolated to one part of WebAuthn, and has automated tests. It's basically a change to WebIDL and a null-check.

String or UUID changes made by this patch: 
None


Approval Request Comment
[Feature/Bug causing the regression]:
n/a

[User impact if declined]:
Divergent behavior between 61 and the version Chrome is releasing in June.

[Is this code covered by automated tests?]:
Yes

[Has the fix been verified in Nightly?]:
Yes, 4 days

[Needs manual test from QE? If yes, steps to reproduce]: 
No.

[List of other uplifts needed for the feature/fix]:
None.

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
This is isolated to one part of WebAuthn, and has automated tests. It's basically a change to WebIDL and a null-check.

[String changes made/needed]:
None
Flags: needinfo?(jjones)
Attachment #8979373 - Flags: approval-mozilla-esr60?
Attachment #8979373 - Flags: approval-mozilla-beta?
Comment on attachment 8979373 [details]
Bug 1463170 - Set AuthenticatorAssertionResponse.userHandle to null r?ttaubert r?smaug

Fixes a WebAuthn spec compliance issue. Approved for 61.0b10 and ESR 60.1.
Attachment #8979373 - Flags: approval-mozilla-esr60?
Attachment #8979373 - Flags: approval-mozilla-esr60+
Attachment #8979373 - Flags: approval-mozilla-beta?
Attachment #8979373 - Flags: approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.