1335899 - U2F should tolerate token failures like WebAuthn does

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[J.C. Jones [:jcj] (he/they)]

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.

Comment 1

[J.C. Jones [:jcj] (he/they)]

This is testable with just the Soft Token, but only when the RP misbehaves. Still, it's possible to write a test for it.

Comment 2

[J.C. Jones [:jcj] (he/they)]

Attachment: Bug 1335899 - Tolerate token failures in U2F.cpp

If there's a second token (say, USB anyone?) that fails early, U2F.cpp's
U2FStatus object should not be told to "stop" unless it's actually done.

So basically, in the promise failures for U2F::Sign and U2F::Register, don't
call Stop - let the stop come implicitly when no tokens respond correctly.

This changes U2FStatus to be used the same way WebAuthn does its WebAuthnRequest
object, for the same purpose.

Review commit: https://reviewboard.mozilla.org/r/108828/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/108828/

Comment 3

[Dana Keeler (she/her) [:keeler]]

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

Comment 4

[J.C. Jones [:jcj] (he/they)]

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.

Comment 5

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8832662 [details]
Bug 1335899 - Tolerate token failures in U2F.cpp

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/108828/diff/1-2/

Comment 6

[J.C. Jones [:jcj] (he/they)]

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.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f898ad6da184
Tolerate token failures in U2F.cpp r=keeler

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f898ad6da184