Closed Bug 1460767 Opened 2 years ago Closed 2 years ago

FIDO U2F should return DEVICE_INELIGIBLE when devices are ... ineligible.

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- fixed
firefox60 --- wontfix
firefox61 --- fixed
firefox62 --- fixed

People

(Reporter: jcj, Assigned: jcj)

Details

(Keywords: regression, Whiteboard: [u2f])

Attachments

(1 file)

FIDO U2F support inappropriately returns the U2F Error Code 1 (OTHER_ERROR) instead of code 4 (DEVICE_INELIGIBLE) when devices are actually ineligible.

In U2F.cpp, ConvertNSResultToErrorCode [1] converts an nsresult type to FIDO codes with the conditional:

  /* Emitted by U2F{Soft,HID}TokenManager when we really mean ineligible */
  if (aError == NS_ERROR_DOM_NOT_ALLOWED_ERR) {
    return ErrorCode::DEVICE_INELIGIBLE;
  }

However, U2FResult::GetError [2] only returns NS_ERROR_DOM_NOT_ALLOWED_ERR if it receives U2F_ERROR_NOT_ALLOWED, which is never sent (anymore), in favor of NS_ERROR_DOM_INVALID_STATE_ERR. We need to update the code in U2F.cpp to expect that error, and add a test.

[1] https://searchfox.org/mozilla-central/rev/3f17a234769d25fca5144ebb8abc8e1cb3c56c16/dom/u2f/U2F.cpp#61
[2] https://searchfox.org/mozilla-central/rev/3f17a234769d25fca5144ebb8abc8e1cb3c56c16/dom/webauthn/U2FHIDTokenManager.h#77
Flags: behind-pref+
FIDO U2F's specification says that when the wrong security key responds to a
signature, or when an already-registered key exists, that the UA should return
error code 4, DEVICE_INELIGIBLE. We used to do that, but adjusted some things
for WebAuthn and now we don't. This changes the soft token to return that at
the appropriate times, and updates the expectations of U2F.cpp that it should
use InvalidStateError as the signal to reutrn DEVICE_INELIGIBLE.

Also, note that WebAuthn's specification says that if any authenticator returns
"InvalidStateError" that it should be propagated, as it indicates that the
authenticator obtained user consent and failed to complete its job [1].

This change to the Soft Token affects the WebAuthn tests, but in a good way.
Reading the WebAuthn spec, we should not be returning NotAllowedError when there
is consent from the user via the token (which the softtoken always deliveres).

As such, this adjusts the affected WebAuthn tests, and adds a couple useful
checks to test_webauthn_get_assertion.html for future purposes.

[1] https://w3c.github.io/webauthn/#createCredential section 5.1.3 "Create a new
    credential", Step 20, Note 2: "If any authenticator returns an error status
    equivalent to "InvalidStateError"..."
Comment on attachment 8975179 [details]
Bug 1460767 - Return device ineligible when appropriate for U2F r?ttaubert

Tim Taubert [:ttaubert] has approved the revision.

https://phabricator.services.mozilla.com/D1269
Attachment #8975179 - Flags: review+
Pushed by jjones@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5166f4f5af70
Return device ineligible when appropriate for U2F r=ttaubert
I'm pretty sure we'll want to uplift this to 61. I'll file for uplift later this week.
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/5166f4f5af70
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Is ESR60 a concern for this bug?
Flags: needinfo?(jjones)
Flags: in-testsuite+
Target Milestone: mozilla61 → mozilla62
Yes, we'll want this for ESR60. I'll file for uplift to it at the same time as I do for 61. Thanks, Ryan :)
Flags: needinfo?(jjones)
Comment on attachment 8975179 [details]
Bug 1460767 - Return device ineligible when appropriate for U2F r?ttaubert

Approval Request Comment
[Feature/Bug causing the regression]: n/a
[User impact if declined]: We'll return an incorrect error code for U2F operations. This affects Thunderbird (where U2F is enabled) and enterprises that turn that pref on.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes, since Monday.
[Needs manual test from QE? If yes, steps to reproduce]: No, automated tests cover it.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It's behind a pref for most users, and only changes error return cases.
[String changes made/needed]: None.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:

U2F is only being used by enterprises that already have deployed U2F tokens, and Thunderbird. Both are likely to be using ESR.

User impact if declined: The error codes are incorrect for app developers. Not critical.

Fix Landed on Version: 62, but hopefully being uplifted to 61.
Risk to taking this patch (and alternatives if risky): Low risk.
String or UUID changes made by this patch:  None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8975179 - Flags: approval-mozilla-esr60?
Attachment #8975179 - Flags: approval-mozilla-beta?
Comment on attachment 8975179 [details]
Bug 1460767 - Return device ineligible when appropriate for U2F r?ttaubert

Fix for U2F issues, approved for 61.0b6.
Attachment #8975179 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to J.C. Jones [:jcj] from comment #8)
> [User impact if declined]: We'll return an incorrect error code for U2F
> operations. This affects Thunderbird (where U2F is enabled) and enterprises
> that turn that pref on.

Which pref are you referring to here?
Flags: needinfo?(jjones)
(In reply to Julien Cristau [:jcristau] from comment #11)
> (In reply to J.C. Jones [:jcj] from comment #8)
> > [User impact if declined]: We'll return an incorrect error code for U2F
> > operations. This affects Thunderbird (where U2F is enabled) and enterprises
> > that turn that pref on.
> 
> Which pref are you referring to here?

security.webauth.u2f which was enabled for Thunderbird in Bug 1444101.
Flags: needinfo?(jjones)
Comment on attachment 8975179 [details]
Bug 1460767 - Return device ineligible when appropriate for U2F r?ttaubert

let's fix this for 60.1esr then
Attachment #8975179 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
You need to log in before you can comment on or make changes to this bug.