Closed Bug 1335899 Opened 3 years ago Closed 3 years ago

U2F should tolerate token failures like WebAuthn does

Categories

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

All
Unspecified
defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: jcj, Assigned: jcj)

Details

(Whiteboard: [webauthn])

Attachments

(1 file)

When working with multiple Tokens, U2F.cpp's sign and register functions should not "stop" upon the first result, rather they should only stop upon the first success -- or if all tokens fail.
This is testable with just the Soft Token, but only when the RP misbehaves. Still, it's possible to write a test for it.
Assignee: nobody → jjones
Status: NEW → ASSIGNED
Whiteboard: [webauthn]
Comment on attachment 8832662 [details]
Bug 1335899 - Tolerate token failures in U2F.cpp

https://reviewboard.mozilla.org/r/108828/#review110118

LGTM - just some nits and comments on documentation

::: dom/u2f/U2F.cpp:723
(Diff revision 1)
>              status->Stop(ErrorCode::OK, aResponse);
>            }
>            status->WaitGroupDone();
>          },
>          [&status, this] (ErrorCode aErrorCode) {
> -          if (!status->IsStopped()) {
> +          // Ignore the failing error code, as we only want the first success

What happens if everything fails? Is an error code properly propagated? Oh, I guess it returns DEVICE_INELIGIBLE? We should probably document that in any case (or maybe collect error codes and return the most common one? Not sure it's really worth it...)

::: dom/u2f/tests/frame_multiple_keys.html:62
(Diff revision 1)
> +  });
> +}
> +
> +
> +function registerWithInvalidAndValidKey() {
> +  window.u2f.register(window.location.origin, [regRequest], [invalidKey, testState.key1], function(aRegResponse) {

nit: this line is a bit long

::: dom/u2f/tests/frame_multiple_keys.html:70
(Diff revision 1)
> +
>      testSignSingleKey();
>    });
>  }
>  
> +

nit: probably don't need this extra blank line

::: dom/u2f/tests/frame_multiple_keys.html:103
(Diff revision 1)
> +};
> +
> +function testSignWithInvalidKey() {
> +  window.u2f.sign(window.location.origin, bytesToBase64UrlSafe(challenge),
> +                  [invalidKey, testState.key2], function(aSignResponse) {
> +    local_is(aSignResponse.errorCode, 0, "The signing did not error an invalid key");

maybe "return an error given an invalid key"?

::: dom/u2f/tests/frame_multiple_keys.html:104
(Diff revision 1)
> +
> +function testSignWithInvalidKey() {
> +  window.u2f.sign(window.location.origin, bytesToBase64UrlSafe(challenge),
> +                  [invalidKey, testState.key2], function(aSignResponse) {
> +    local_is(aSignResponse.errorCode, 0, "The signing did not error an invalid key");
> +    local_isnot(aSignResponse.clientData, undefined, "The signing provided clientData with an invalid key");

I find this a bit ambiguous - maybe "given an invalid key"?
Attachment #8832662 - Flags: review?(dkeeler) → review+
Comment on attachment 8832662 [details]
Bug 1335899 - Tolerate token failures in U2F.cpp

https://reviewboard.mozilla.org/r/108828/#review110118

Thanks for the review!

> What happens if everything fails? Is an error code properly propagated? Oh, I guess it returns DEVICE_INELIGIBLE? We should probably document that in any case (or maybe collect error codes and return the most common one? Not sure it's really worth it...)

I changed the comment to be:
```
          // Ignore the failing error code, as we only want the first success.
          // U2F devices don't provide much for error codes anyway, so if
          // they all fail we'll return DEVICE_INELIGIBLE.
```
There are a couple error types that can be returned by a U2F authenticator, but if there's more than one plugged in, how can you disambiguate which one said BAD_REQUEST versus CONFIGURATION_UNSUPPORTED without the JS knowing there even are two? 

In practice, U2F's error codes are pretty much trinary: OK, TIMEOUT, or *cough*<mumble>*cough*. Chrome doesn't bother trying to sort them out either, so this seems fine - unless it someday isn't, in which case hey, it's straightforward to add logic to summarize all the errors in U2FStatus, but seems overkill right now. :)

> nit: this line is a bit long

Made line less long.

> nit: probably don't need this extra blank line

Comforted extra blank line and had security escort it (and the one nearby) out with promise of a severance check.

> maybe "return an error given an invalid key"?

Translated from J.C. to English.

> I find this a bit ambiguous - maybe "given an invalid key"?

Disambiguated the cromulent string.
Earlier try run (https://treeherder.mozilla.org/#/jobs?repo=try&revision=262e18c54c1cfe9d5d0b2c1098afae214e0e2670&selectedJob=73715176) looks good, and the review updates were only comments, so marking checkin-needed.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f898ad6da184
Tolerate token failures in U2F.cpp r=keeler
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f898ad6da184
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.