1275479 - Refactor FIDO U2F to use token interfaces

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

The dom/u2f/U2F.cpp class has hardcoded logic to specifically call the NSS Token; it should be refactored to use an interface based on nsINSSU2FToken.

Comment 1

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

Note: One of the reasons for this is to support community contributors who would like to provide alternative token implementations, such as Bluetooth Smart or NFC.

Comment 2

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

Attachment: MozReview Request: Bug 1275479 - Refactor U2F Token Interface (Part 1) r=keeler

Rework U2F.cpp to use a collection of nsINSSU2FToken for U2F/WebAuth operations.

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

Comment 3

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

Attachment: MozReview Request: Bug 1275479 - Create nsIU2FToken base interface (Part 2) r=keeler

Create a base "nsIU2FToken" interface that all tokens must implement. This
patch does not change U2F.cpp from initializing tokens monolithically, but
if/when future tokens are added, the implementer may want to do that.

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

Comment 4

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

Try run in progress: https://treeherder.mozilla.org/#/jobs?repo=try&revision=55259772e6e6

Comment 5

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

Better try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=157aaa8ec16b

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8757493 [details]
MozReview Request: Bug 1275479 - Refactor U2F Token Interface (Part 1) r=keeler

https://reviewboard.mozilla.org/r/55918/#review53354

This looks good.

::: dom/u2f/NSSU2FTokenRemote.cpp:52
(Diff revision 1)
> +{
> +  NS_ENSURE_ARG_POINTER(aKeyHandle);
> +  NS_ENSURE_ARG_POINTER(aIsRegistered);
> +
> +  nsTArray<uint8_t> keyHandle;
> +  keyHandle.ReplaceElementsAt(0, keyHandle.Length(), aKeyHandle, aKeyHandleLen);

This is fallible, right? We should check OOM here.

::: dom/u2f/NSSU2FTokenRemote.cpp:76
(Diff revision 1)
> +  NS_ENSURE_ARG_POINTER(aChallenge);
> +  NS_ENSURE_ARG_POINTER(aRegistration);
> +  NS_ENSURE_ARG_POINTER(aRegistrationLen);
> +
> +  nsTArray<uint8_t> application;
> +  application.ReplaceElementsAt(0, application.Length(), aApplication,

Same idea here and elsewhere.

::: dom/u2f/NSSU2FTokenRemote.cpp:89
(Diff revision 1)
> +  if (!cc->SendNSSU2FTokenRegister(application, challenge,
> +                                   &registrationBuffer)) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  uint32_t dataLen = registrationBuffer.Length();

I think Length() returns a size_t here, so we should check for overflow.

::: dom/u2f/NSSU2FTokenRemote.cpp:129
(Diff revision 1)
> +  if (!cc->SendNSSU2FTokenSign(application, challenge, keyHandle,
> +                               &signatureBuffer)) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  uint32_t dataLen = signatureBuffer.Length();

Same here.

::: dom/u2f/U2F.cpp:154
(Diff revision 1)
>      bool isCompatible = false;
>      bool isRegistered = false;
>  
>      // Determine if the provided keyHandle is registered at any device. If so,
>      // then we'll return DEVICE_INELIGIBLE to signify we're already registered.
> -    if (softTokenEnabled) {
> +    for (size_t a = 0; a < mAuthenticators.Length(); ++a) {

Can you do |for (auto token : mAuthenticators) {| here?

::: dom/u2f/U2F.cpp:163
(Diff revision 1)
>        if (NS_FAILED(rv)) {
>          ReturnError(ErrorCode::OTHER_ERROR);
>          return NS_ERROR_FAILURE;
>        }
>  
> -      rv = NSSTokenIsRegistered(mNSSToken, keyHandle, &isRegistered);
> +      rv = token->IsRegistered(keyHandle.Elements(), keyHandle.Length(),

hmmm - if the token isn't compatible, do we even want to call IsRegistered?

::: dom/u2f/U2F.cpp:227
(Diff revision 1)
>      // Get the registration data from the token
>      CryptoBuffer regData;
>      bool registerSuccess = false;
>      bool isCompatible = false;
>  
> -    if (!registerSuccess && softTokenEnabled) {
> +    for (size_t a = 0; a < mAuthenticators.Length() && !registerSuccess; ++a) {

Same idea with |for (auto token: mAuthenticators) {| (I guess registerSuccess will have to be checked separately...)

::: dom/u2f/U2F.cpp:402
(Diff revision 1)
>        if (NS_FAILED(rv)) {
>          ReturnError(ErrorCode::OTHER_ERROR);
>          return NS_ERROR_FAILURE;
>        }
>  
> -      rv = NSSTokenIsRegistered(mNSSToken, keyHandle, &isRegistered);
> +      rv = token->IsRegistered(keyHandle.Elements(), keyHandle.Length(),

Same question about if !isCompatible

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8757494 [details]
MozReview Request: Bug 1275479 - Create nsIU2FToken base interface (Part 2) r=keeler

https://reviewboard.mozilla.org/r/55920/#review53388

Comment 8

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

Comment on attachment 8757493 [details]
MozReview Request: Bug 1275479 - Refactor U2F Token Interface (Part 1) r=keeler

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

Comment 9

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

Comment on attachment 8757494 [details]
MozReview Request: Bug 1275479 - Create nsIU2FToken base interface (Part 2) r=keeler

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

Comment 10

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

Comment on attachment 8757493 [details]
MozReview Request: Bug 1275479 - Refactor U2F Token Interface (Part 1) r=keeler

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

Comment 11

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

Comment on attachment 8757494 [details]
MozReview Request: Bug 1275479 - Create nsIU2FToken base interface (Part 2) r=keeler

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

Comment 12

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

Thank you for the review, Keeler! Excellent comments as always. Update posted, kept your r+, and try run looks good [1]. Marking checkin-needed.

1) https://treeherder.mozilla.org/#/jobs?repo=try&revision=108817479038

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7d82acfe62e
Refactor U2F Token Interface (Part 1). r=keeler
https://hg.mozilla.org/integration/mozilla-inbound/rev/52bd4c6b7f7c
Create nsIU2FToken base interface (Part 2). r=keeler

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b7d82acfe62e
https://hg.mozilla.org/mozilla-central/rev/52bd4c6b7f7c