Implement U2FHIDTokenManager

RESOLVED FIXED in Firefox 57

Status

()

P1
enhancement
RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [webauthn])

Attachments

(1 attachment, 1 obsolete attachment)

In contract to the existing U2FSoftTokenManager, this bug is about landing the U2FHIDTokenManager implementation that talks to our Rust U2F/HID library.

The C API header and implementation can be found in the first patch of bug 1388843.
(Assignee)

Updated

a year ago
Summary: Implemented U2FHIDTokenManager → Implement U2FHIDTokenManager
Created attachment 8895505 [details] [diff] [review]
0001-Bug-1388851-Implement-U2FHIDTokenManager.patch

This patch implements the actual token manager that uses the Rust library from bug 1388843 to talk to U2F tokens.
Blocks: 1294514
OS: Unspecified → All
Priority: P2 → P1
Hardware: Unspecified → All
(Assignee)

Updated

a year ago
Attachment #8895505 - Flags: review?(kyle)
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Comment on attachment 8895505 [details] [diff] [review]
0001-Bug-1388851-Implement-U2FHIDTokenManager.patch

Review of attachment 8895505 [details] [diff] [review]:
-----------------------------------------------------------------

Generally looks very good!

::: dom/webauthn/U2FHIDTokenManager.cpp
@@ +11,5 @@
>  
> +static U2FHIDTokenManager* gInstance;
> +static nsIThread* gPBackgroundThread;
> +
> +static void u2f_register_callback(uint64_t aTransactionId,

Nit: looks like most of the webauthn/ classes put the qualifiers / return type on the previous line.

@@ +14,5 @@
> +
> +static void u2f_register_callback(uint64_t aTransactionId,
> +                                  rust_u2f_res* aResult) {
> +  MOZ_ASSERT(gPBackgroundThread);
> +  if (!gInstance) {

Probably worth an NS_WARN_IF

@@ +30,5 @@
> +
> +static void u2f_sign_callback(uint64_t aTransactionId, rust_u2f_res* aResult) {
> +  MOZ_ASSERT(gPBackgroundThread);
> +
> +  if (!gInstance) {

As above

@@ +171,5 @@
> +  MOZ_ASSERT(!mRegisterPromise.IsEmpty());
> +
> +  nsTArray<uint8_t> registration;
> +  if (!aResult->CopyRegistration(registration)) {
> +    mRegisterPromise.Reject(NS_ERROR_DOM_UNKNOWN_ERR, __func__);

Nit: This would be a NS_ERROR_OUT_OF_MEMORY probably, right? Of course, that's not a DOM error..

@@ +175,5 @@
> +    mRegisterPromise.Reject(NS_ERROR_DOM_UNKNOWN_ERR, __func__);
> +    return;
> +  }
> +
> +  U2FRegisterResult result(Move(registration));

To be honest, I didn't know about Move.h; that's a clever re-implementation.

::: dom/webauthn/U2FHIDTokenManager.h
@@ +25,5 @@
> +    mKeyHandles = rust_u2f_khs_new();
> +    MOZ_ASSERT(mKeyHandles);
> +
> +    for (auto desc: aDescriptors) {
> +      rust_u2f_khs_add(mKeyHandles, desc.id().Elements(), desc.id().Length());

It feels like this (void) method should be possible to fail, and that we should handle it. But maybe not?
Attachment #8895505 - Flags: review?(dkeeler)
Attachment #8895505 - Flags: review-
Comment on attachment 8895505 [details] [diff] [review]
0001-Bug-1388851-Implement-U2FHIDTokenManager.patch

Review of attachment 8895505 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good. I added my thoughts. This is sort-of a meta-comment, but I feel like it might be nice to have a way of distinguishing "this assertion is more of a reminder of an invariant that really is enforced elsewhere" and "if this fails we need to drop everything and abort" (where the latter should have code that runs in release builds that halts whatever operation it's doing).

::: dom/webauthn/U2FHIDTokenManager.cpp
@@ +14,5 @@
> +
> +static void u2f_register_callback(uint64_t aTransactionId,
> +                                  rust_u2f_res* aResult) {
> +  MOZ_ASSERT(gPBackgroundThread);
> +  if (!gInstance) {

Are we on gPBackgroundThread here? If not, seems like we could race on gInstance...

@@ +50,5 @@
> +  MOZ_ASSERT(!NS_IsMainThread());
> +  MOZ_ASSERT(!gInstance);
> +
> +  mU2FManager = rust_u2f_mgr_new();
> +  gPBackgroundThread = NS_GetCurrentThread();

It looks like NS_GetCurrentThread can return nullptr - maybe we should abort or indicate failure in some way?

@@ +93,5 @@
>                               uint32_t aTimeoutMS)
>  {
> +  MOZ_ASSERT(NS_GetCurrentThread() == gPBackgroundThread);
> +
> +  ClearPromises();

If I'm understanding correctly, only one page can have a register/sign operation in flight at a time, right? If we don't queue requests properly, it seems like a malicious site could spam us with sign/register requests and prevent any actual work from happening. Do we guard against this somehow?

::: dom/webauthn/U2FHIDTokenManager.h
@@ +22,5 @@
> +public:
> +  explicit U2FKeyHandles(const nsTArray<WebAuthnScopedCredentialDescriptor>& aDescriptors)
> +  {
> +    mKeyHandles = rust_u2f_khs_new();
> +    MOZ_ASSERT(mKeyHandles);

Is the rust allocator fallible? If not, this doesn't seem necessary. If so, we need to actually bail if mKeyHandles didn't get allocated.

@@ +29,5 @@
> +      rust_u2f_khs_add(mKeyHandles, desc.id().Elements(), desc.id().Length());
> +    }
> +  }
> +
> +  rust_u2f_khs* Get() { return mKeyHandles; }

This shorthand is a bit opaque - can we call it "rust_u2f_key_handles"?

@@ +39,5 @@
> +};
> +
> +class U2FResult {
> +public:
> +  explicit U2FResult(uint64_t aTransactionId, rust_u2f_res* aResult)

Again, seems like we might gain some readability by calling this "rust_u2f_result".
Attachment #8895505 - Flags: review?(dkeeler)
Comment on attachment 8895505 [details] [diff] [review]
0001-Bug-1388851-Implement-U2FHIDTokenManager.patch

Review of attachment 8895505 [details] [diff] [review]:
-----------------------------------------------------------------

Everyone else basically covered my thoughts, looking mostly good, probably r+ on the next patch.
Attachment #8895505 - Flags: review?(kyle) → review-
(In reply to J.C. Jones [:jcj] from comment #2)
> ::: dom/webauthn/U2FHIDTokenManager.cpp
> @@ +11,5 @@
> >  
> > +static U2FHIDTokenManager* gInstance;
> > +static nsIThread* gPBackgroundThread;
> > +
> > +static void u2f_register_callback(uint64_t aTransactionId,
> 
> Nit: looks like most of the webauthn/ classes put the qualifiers / return
> type on the previous line.

Fixed.

> @@ +14,5 @@
> > +
> > +static void u2f_register_callback(uint64_t aTransactionId,
> > +                                  rust_u2f_res* aResult) {
> > +  MOZ_ASSERT(gPBackgroundThread);
> > +  if (!gInstance) {
> 
> Probably worth an NS_WARN_IF

Good idea, added.

> @@ +171,5 @@
> > +  MOZ_ASSERT(!mRegisterPromise.IsEmpty());
> > +
> > +  nsTArray<uint8_t> registration;
> > +  if (!aResult->CopyRegistration(registration)) {
> > +    mRegisterPromise.Reject(NS_ERROR_DOM_UNKNOWN_ERR, __func__);
> 
> Nit: This would be a NS_ERROR_OUT_OF_MEMORY probably, right? Of course,
> that's not a DOM error..

Hmm, yeah. Can't find a DOM error that would be a good fit...

> ::: dom/webauthn/U2FHIDTokenManager.h
> @@ +25,5 @@
> > +    mKeyHandles = rust_u2f_khs_new();
> > +    MOZ_ASSERT(mKeyHandles);
> > +
> > +    for (auto desc: aDescriptors) {
> > +      rust_u2f_khs_add(mKeyHandles, desc.id().Elements(), desc.id().Length());
> 
> It feels like this (void) method should be possible to fail, and that we
> should handle it. But maybe not?

As long as the given pointer `mKeyHandles` is valid it shouldn't be possible to fail here. In Rust Vec::push() always succeeds, or probably panics when OOM. Definitely panics when Vec::len() becomes bigger than what usize can hold.
(In reply to David Keeler [:keeler] (use needinfo?) from comment #3)
> Looking good. I added my thoughts. This is sort-of a meta-comment, but I
> feel like it might be nice to have a way of distinguishing "this assertion
> is more of a reminder of an invariant that really is enforced elsewhere" and
> "if this fails we need to drop everything and abort" (where the latter
> should have code that runs in release builds that halts whatever operation
> it's doing).

We do have MOZ_RELEASE_ASSERT(), we could use that where we think it's appropriate. I find it hard to justify crashing in release for most of the things. The only thing where it might be worth doing so could be the `NS_GetCurrentThread() != nullptr` check. First, that should never fail and second, we can't dispatch the result handling when that's null. But maybe we shouldn't crash here either, I added a check for gPBackgroundThread to the callbacks.

> ::: dom/webauthn/U2FHIDTokenManager.cpp
> @@ +14,5 @@
> > +
> > +static void u2f_register_callback(uint64_t aTransactionId,
> > +                                  rust_u2f_res* aResult) {
> > +  MOZ_ASSERT(gPBackgroundThread);
> > +  if (!gInstance) {
> 
> Are we on gPBackgroundThread here? If not, seems like we could race on
> gInstance...

Good catch, that would indeed be racy if we don't cancel the operation properly when the instance shuts down. I added a StaticMutex, none of these are hot code paths, so locking should be fine.

> @@ +50,5 @@
> > +  MOZ_ASSERT(!NS_IsMainThread());
> > +  MOZ_ASSERT(!gInstance);
> > +
> > +  mU2FManager = rust_u2f_mgr_new();
> > +  gPBackgroundThread = NS_GetCurrentThread();
> 
> It looks like NS_GetCurrentThread can return nullptr - maybe we should abort
> or indicate failure in some way?

Added an assertion like we have in other places.

> @@ +93,5 @@
> >                               uint32_t aTimeoutMS)
> >  {
> > +  MOZ_ASSERT(NS_GetCurrentThread() == gPBackgroundThread);
> > +
> > +  ClearPromises();
> 
> If I'm understanding correctly, only one page can have a register/sign
> operation in flight at a time, right? If we don't queue requests properly,
> it seems like a malicious site could spam us with sign/register requests and
> prevent any actual work from happening. Do we guard against this somehow?

Yeah, we don't queue requests. We simply cancel the running operation. API calls should fail when the page isn't the active tab (in the foremost window). Not sure if we have tests or code for that?

> ::: dom/webauthn/U2FHIDTokenManager.h
> @@ +22,5 @@
> > +public:
> > +  explicit U2FKeyHandles(const nsTArray<WebAuthnScopedCredentialDescriptor>& aDescriptors)
> > +  {
> > +    mKeyHandles = rust_u2f_khs_new();
> > +    MOZ_ASSERT(mKeyHandles);
> 
> Is the rust allocator fallible? If not, this doesn't seem necessary. If so,
> we need to actually bail if mKeyHandles didn't get allocated.

You're right, this actually can't fail. Will remove the assertion.

> @@ +29,5 @@
> > +      rust_u2f_khs_add(mKeyHandles, desc.id().Elements(), desc.id().Length());
> > +    }
> > +  }
> > +
> > +  rust_u2f_khs* Get() { return mKeyHandles; }
> 
> This shorthand is a bit opaque - can we call it "rust_u2f_key_handles"?

Updated the type names. I'll just include the u2f-hid-rs header file change in here.
Created attachment 8908112 [details] [diff] [review]
0001-Bug-1388851-Implement-U2FHIDTokenManager.patch, v2
Attachment #8895505 - Attachment is obsolete: true
Attachment #8908112 - Flags: review?(kyle)
Attachment #8908112 - Flags: review?(jjones)
Attachment #8908112 - Flags: review?(dkeeler)
Comment on attachment 8908112 [details] [diff] [review]
0001-Bug-1388851-Implement-U2FHIDTokenManager.patch, v2

Review of attachment 8908112 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM whether or not you add the nullptr check; it _is_ a helper class, not something used by others.

::: dom/webauthn/U2FHIDTokenManager.h
@@ +41,5 @@
> +public:
> +  explicit U2FResult(uint64_t aTransactionId, rust_u2f_result* aResult)
> +    : mTransactionId(aTransactionId)
> +    , mResult(aResult)
> +  { }

We could assert aResult != nullptr here, but given how this is used that's admittedly extraneous.
Attachment #8908112 - Flags: review?(jjones) → review+
Comment on attachment 8908112 [details] [diff] [review]
0001-Bug-1388851-Implement-U2FHIDTokenManager.patch, v2

Review of attachment 8908112 [details] [diff] [review]:
-----------------------------------------------------------------

Cool - looks good to me!
Attachment #8908112 - Flags: review?(dkeeler) → review+

Comment 10

a year ago
Pushed by ttaubert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c89dde5d6274
Implement U2FHIDTokenManager r=jcj,qdot,keeler

Comment 11

a year ago
Pushed by ttaubert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c115eec567a6
Follow-up to disable parts of browser_webauthn_telemetry.js r=bustage
(In reply to Pulsebot from comment #11)
> Pushed by ttaubert@mozilla.com:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/c115eec567a6
> Follow-up to disable parts of browser_webauthn_telemetry.js r=bustage

J.C., I had to disable the test partially, where it set webauthn.enable_usbtoken=true. How's that supposed to work with the real USB token code now?
Flags: needinfo?(jjones)

Updated

a year ago
Depends on: 1400066
(In reply to Tim Taubert [:ttaubert] from comment #12)
> (In reply to Pulsebot from comment #11)
> > Pushed by ttaubert@mozilla.com:
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/c115eec567a6
> > Follow-up to disable parts of browser_webauthn_telemetry.js r=bustage
> 
> J.C., I had to disable the test partially, where it set
> webauthn.enable_usbtoken=true. How's that supposed to work with the real USB
> token code now?

Gosh, I don't know. I'll give it some thought and open a follow-up
Flags: needinfo?(jjones)
https://hg.mozilla.org/mozilla-central/rev/c89dde5d6274
https://hg.mozilla.org/mozilla-central/rev/c115eec567a6
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Duplicate of this bug: 1301793
You need to log in before you can comment on or make changes to this bug.