Refactor FIDO U2F Token to use runnables

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jcj, Assigned: jcj)

Tracking

unspecified
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
The U2F API is asynchronous, and many of its actions are asynchronous, so we should rewrite the U2F.cpp code to use Runnables (or similar) to operate asynchronously.
(Assignee)

Updated

3 years ago
Assignee: nobody → jjones
Depends on: 1231681
Whiteboard: btpp-active
(Assignee)

Comment 1

3 years ago
Created attachment 8742577 [details]
MozReview Request: Bug 1264472 - Use nsRunnables in FIDO U2F r?keeler

- Move the AppID/FacetID algorithm into its own (potentially reentrant) method
  to facilitate Bug 1244959
- Change the Register and Sign operations to be Runnables
- Clean up error handling
- Remove unnecessary remote-load Facet test files; we'll re-add some form of
  them when the remote load algorithm is completed

Review commit: https://reviewboard.mozilla.org/r/47331/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47331/
Attachment #8742577 - Flags: review?(dkeeler)
Comment on attachment 8742577 [details]
MozReview Request: Bug 1264472 - Use nsRunnables in FIDO U2F r?keeler

https://reviewboard.mozilla.org/r/47331/#review44247

This generally looks good, but I have some questions. Most importantly, I'm not understanding how this actually makes things asynchronous. Are the U2FTasks supposed to be dispatched to a background thread? Or as subsequent events on the same thread? Right now I think they're just run synchronously and then they call their callbacks synchronously.

::: dom/u2f/U2F.cpp:42
(Diff revision 1)
> -U2F::WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto)
> -{
> -  return U2FBinding::Wrap(aCx, this, aGivenProto);
> -}
>  
> +template <class CB, class Rsp>

It might be helpful to spell these template type names out ("Callback" and "Response" I'm assuming?)

::: dom/u2f/U2F.cpp:117
(Diff revision 1)
>    }
>    return NS_OK;
>  }
>  
> -nsresult
> -U2F::NSSTokenRegister(CryptoBuffer& aApplication, CryptoBuffer& aChallenge,
> +static nsresult
> +NSSTokenSign(nsINSSU2FToken* aNSSToken, CryptoBuffer& aKeyHandle,

For future reviews, having code reorderings in a separate commit would be helpful.

::: dom/u2f/U2F.cpp:391
(Diff revision 1)
> -U2F::Sign(const nsAString& aAppId,
> +                         const nsAString& aAppId,
> -          const nsAString& aChallenge,
> +                         const nsAString& aChallenge,
> -          const Sequence<RegisteredKey>& aRegisteredKeys,
> +                         const Sequence<RegisteredKey>& aRegisteredKeys,
> -          U2FSignCallback& aCallback,
> -          const Optional<Nullable<int32_t>>& opt_aTimeoutSeconds,
> -          ErrorResult& aRv)
> +                         U2FSignCallback* aCallback,
> +                         const nsCOMPtr<nsINSSU2FToken>& aNSSToken)
> +  : U2FTask(aOrigin, aAppId), mChallenge(aChallenge),

You might format the initializers like so:

: U2FTask(aOrigin, aAppId)
, mChallenge(aChallenge)
, etc...

(but no big deal)

::: dom/u2f/U2F.cpp:556
(Diff revision 1)
> +// EvaluateAppIDAndRunTask determines whether the supplied FIDO AppID is valid for
> +// the current FacetID, e.g., the current origin.
> +// See https://fidoalliance.org/specs/fido-u2f-v1.0-nfc-bt-amendment-20150514/fido-appid-and-facets.html
> +// for a description of the algorithm.
> +static void
> +EvaluateAppIDAndRunTask(U2FTask* aCallback)

Is "aCallback" the right name for this parameter? Seems like "aTask" would be better suited.

::: dom/u2f/U2F.cpp:593
(Diff revision 1)
> +  nsAutoCString appIdUrl = NS_ConvertUTF16toUTF8(callback->mAppId);
> +  rv = urlParser->ParseURL(appIdUrl.get(), callback->mAppId.Length(),
> +                           &appIdSchemePos, &appIdSchemeLen,
> +                           &appIdAuthPos, &appIdAuthLen,
> +                           nullptr, nullptr);      // ignore path
> +  if (NS_WARN_IF(NS_FAILED(rv))) {

Is the appId from untrusted input? If so, I'm not sure the NS_WARN_IF is useful here, since bogus input from web content could trigger it.

::: dom/u2f/U2F.cpp:608
(Diff revision 1)
> +    aCallback->ReturnError(ErrorCode::BAD_REQUEST);
> -  return;
> +    return;
> -}
> +  }
>  
> +  // If the appId is empty or null, overwrite it with the facetId and accept
> +  if (callback->mAppId.IsEmpty() || callback->mAppId.EqualsLiteral("null")) {

Looks like sometimes this code uses aCallback and other times uses callback. Is having the local RefPtr to callback necessary?

::: dom/u2f/U2F.cpp:699
(Diff revision 1)
> +              const Sequence<RegisteredKey>& aRegisteredKeys,
> +              U2FRegisterCallback& aCallback,
> +              const Optional<Nullable<int32_t>>& opt_aTimeoutSeconds,
> +              ErrorResult& aRv)
> +{
> +  U2FRegisterTask* registerTask = new U2FRegisterTask(mOrigin, aAppId,

The ownership of this memory isn't that clear here. From what I can tell, it's basically managed by the RefPtr in EvaluateAppIdAndRunTask, but that's a long way to go to understand that this doesn't leak. We should probably just have a RefPtr here as well (and like I said earlier, it may not be necessary to have one in EvaluateAppIdAndRunTask).

::: dom/u2f/tests/test_frame_register_sign.html:87
(Diff revision 1)
>        var clientDataJSON = "";
>        base64ToBytesUrlSafe(regResponse.clientData).map(x => clientDataJSON += String.fromCharCode(x));
>        var clientData = JSON.parse(clientDataJSON);
>        local_is(clientData.typ, "navigator.id.finishEnrollment", "Data type matches");
>        local_is(clientData.challenge, state.regRequest.challenge, "Register challenge matches");
> -      local_is(clientData.origin, window.location.origin, "Origins are the same");
> +      local_is(clientData.origin, window.location.origin, "testRegistering Origins are the same");

nit: space after "test"?

::: dom/u2f/tests/test_frame_register_sign.html:166
(Diff revision 1)
>        var clientDataJSON = "";
>        base64ToBytesUrlSafe(signResponse.clientData).map(x => clientDataJSON += String.fromCharCode(x));
>        var clientData = JSON.parse(clientDataJSON);
>        local_is(clientData.typ, "navigator.id.getAssertion", "Data type matches");
>        local_is(clientData.challenge, state.signChallenge, "Sign challenge matches");
> -      local_is(clientData.origin, window.location.origin, "Origins are the same");
> +      local_is(clientData.origin, window.location.origin, "testSigning Origins are the same");

Same here?
Attachment #8742577 - Flags: review?(dkeeler)
(Assignee)

Comment 3

3 years ago
https://reviewboard.mozilla.org/r/47331/#review44247

This is true. These run synchronously presently, but the refactor is to permit asynchronous fetches in Bug 1244959, where we'll support doing remote fetches of the Trusted Facet List. I should rename the commit, I guess, to "Prepare to be asynchronous..." or perhaps "Use nsRunnables..."

Thanks for reviewing! I'll ping you offline regarding the memory/RefPtr notes before reworking.

> It might be helpful to spell these template type names out ("Callback" and "Response" I'm assuming?)

Sure, though now that this is only called twice I considered removing the template method entirely. But it's not hurting anything.

> For future reviews, having code reorderings in a separate commit would be helpful.

Will do.
(Assignee)

Comment 4

3 years ago
https://reviewboard.mozilla.org/r/47331/#review44247

> Is "aCallback" the right name for this parameter? Seems like "aTask" would be better suited.

Agreed.

> Is the appId from untrusted input? If so, I'm not sure the NS_WARN_IF is useful here, since bogus input from web content could trigger it.

Ah, good distinction. It is untrusted input. I'll remove the NS_WARN_IF; I had been pretty much sprinkling it everywhere, as I haven't had much experience with production debugging yet.

> Looks like sometimes this code uses aCallback and other times uses callback. Is having the local RefPtr to callback necessary?

Good catch; this is left over from when this method was calling dom::Fetch. It doesn't belong in this patch, and is likely unnecessary when calling out to something like dom::Fetch as well, but that's for our later conversation.
(Assignee)

Comment 5

3 years ago
Comment on attachment 8742577 [details]
MozReview Request: Bug 1264472 - Use nsRunnables in FIDO U2F r?keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47331/diff/1-2/
Attachment #8742577 - Attachment description: MozReview Request: Bug 1264472 - Be asynchronous in FIDO U2F r?keeler → MozReview Request: Bug 1264472 - Use nsRunnables in FIDO U2F r?keeler
Attachment #8742577 - Flags: review?(dkeeler)
Comment on attachment 8742577 [details]
MozReview Request: Bug 1264472 - Use nsRunnables in FIDO U2F r?keeler

https://reviewboard.mozilla.org/r/47331/#review45037

LGTM.
Attachment #8742577 - Flags: review?(dkeeler) → review+
(Assignee)

Comment 7

3 years ago
Thanks for the reviews. Try run looks alright: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d138ad7397b9

Marking checkin-needed.
Status: NEW → ASSIGNED
Keywords: checkin-needed

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3aa298c8b91d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.