Closed Bug 1297552 Opened 8 years ago Closed 8 years ago

dom::U2F must use nsIU2FTokens in parallel

Categories

(Core :: DOM: Device Interfaces, enhancement)

Product:
Core
Component:
DOM: Device Interfaces
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: jcj, Assigned: jcj)

References

Details

Attachments

(4 files, 1 obsolete file)

Bug 1297552 - Perform U2F hash operations more efficiently
58 bytes, text/x-review-board-request
keeler
: review+
Details
Bug 1297552 - Only permit U2F operations in e10s mode
58 bytes, text/x-review-board-request
keeler
: review+
Details
Bug 1297552 - Use MozPromise to run U2F operations in parallel.
58 bytes, text/x-review-board-request
keeler
: review+
Details
Bug 1297552 - Reorder parts of U2F.cpp
58 bytes, text/x-review-board-request
keeler
: review+
Details 
Soon there will be more than one implementation of nsIU2FToken, and for that situation to function properly, dom/u2f/U2F.cpp must interact with multiple tokens in parallel. In particular, the semantics necessary are:

When calling Register():
* check isCompatible() across all nsIU2FTokens.
* check isRegistered() for all RegisteredKeys across all compatible nsIU2FTokens.
  - If any return DEVICE_INELIGIBLE, stop and return that result.
* call Register() for all RegisterRequests across all compatible nsIU2FTokens.
  - The first to return a non-error is the result. Cancel all others.

A similar approach is needed for Sign().

This effort should also at least lay the groundwork for handling timeouts.
Blocks: 1065729
Assignee: nobody → jjones
Status: NEW → ASSIGNED
Comment on attachment 8789947 [details]
Bug 1297552 - Perform U2F hash operations more efficiently

https://reviewboard.mozilla.org/r/77972/#review77148

I think I get the general idea here, but there's a lot going on in this one patch. Maybe a good way to make progress would be to break this up into smaller patches that each do one item from the "does these things" list in the commit message.

Overall, I think this is a good path forward if a) it gets us the timeout functionality fairly easily and b) we're sure that it's actually worth it (how many concurrent operations are we expecting sites to want to do? How often will this code actually be run?) I just want to make sure we're not spending a lot of effort optimizing something that doesn't need it.

::: dom/u2f/U2F.h:211
(Diff revision 1)
> +};
> +
> +// U2FThreads run to completion, performing a single U2F operation such as
> +// registering, or signing.
> +class U2FThread : public Runnable,
> +                  public nsNSSShutDownObject

Style comment:

`
class U2FThread : public Runnable
                , public nsNSSShutDownObject
`

::: dom/u2f/U2F.cpp:78
(Diff revision 1)
> +{}
> +
> +U2FStatus::~U2FStatus()
> +{}
> +
> +void U2FStatus::WaitGroupAdd()

style nit: return values should go on their own lines

::: dom/u2f/U2F.cpp:400
(Diff revision 1)
> -        ReturnError(ErrorCode::DEVICE_INELIGIBLE);
> +    mPromise.Reject(ErrorCode::DEVICE_INELIGIBLE, __func__);
> -        return NS_OK;
> +    return NS_OK;
> -      }
> +  }
> +
> +  CryptoBuffer signatureData;
> +  uint8_t* buffer;

Is there a utility class that encapsulates this paradigm of passing out a pointer to a pointer that will then have some memory allocated for it that we copy elsewhere and then free? I think we have a couple instances of this in this code (and elsewhere) - it might be nice to create such a thing that handles the memory freeing for us.

::: dom/u2f/U2F.cpp:434
(Diff revision 1)
> -    RegisterRequest request(mRegisterRequests[i]);
> +  response.mClientData.Construct(clientDataBase64);
> +  response.mSignatureData.Construct(signatureDataBase64);
> +  response.mErrorCode.Construct(static_cast<uint32_t>(ErrorCode::OK));
> +
> +  nsString responseStr;
> +  if (NS_WARN_IF(!response.ToJSON(responseStr))) {

So this is main-thread-only, right? Let's assert that.

::: dom/u2f/U2F.cpp:1031
(Diff revision 1)
>  
> -  EvaluateAppIDAndRunTask(signTask);
> +  if (!mInitialized) {
> +    aRv.Throw(NS_ERROR_NOT_AVAILABLE);
> +    return;
> +  }
> +  RefPtr<U2FSignThread> thread = new U2FSignThread(mOrigin, aAppId, aChallenge,

So, does this create a new thread each time? Or does it just assign one thread from the pool with some sort of limiting?
Attachment #8789947 - Flags: review?(dkeeler)
(In reply to David Keeler [:keeler] (use needinfo?) from comment #3)
> Comment on attachment 8789947 [details]
> Bug 1297552 - [DO NOT LAND] Use MozPromise to run U2F operations in parallel.
> 
> https://reviewboard.mozilla.org/r/77972/#review77148
> 
> I think I get the general idea here, but there's a lot going on in this one
> patch. Maybe a good way to make progress would be to break this up into
> smaller patches that each do one item from the "does these things" list in
> the commit message.

I'll see what I can do to pull the patch apart as you said, probably into:

1) Disable non-e10s
2) Moving the hash calculations to once-per call
3) Use MozPromises

> 
> Overall, I think this is a good path forward if a) it gets us the timeout
> functionality fairly easily and b) we're sure that it's actually worth it
> (how many concurrent operations are we expecting sites to want to do? How
> often will this code actually be run?) I just want to make sure we're not
> spending a lot of effort optimizing something that doesn't need it.

Fairly stated. The main issue is just: The Authenticators must run in parallel, and they must race. The first to complete (or timeout) must complete the callback, and the others should cancel as soon as they can. I believe this is the minimum complexity possible while using MozPromises - I'm not sure if it'd be simpler without.

{snip the specific comments, which I'll address later}

> ::: dom/u2f/U2F.cpp:1031
> (Diff revision 1)
> >  
> > -  EvaluateAppIDAndRunTask(signTask);
> > +  if (!mInitialized) {
> > +    aRv.Throw(NS_ERROR_NOT_AVAILABLE);
> > +    return;
> > +  }
> > +  RefPtr<U2FSignThread> thread = new U2FSignThread(mOrigin, aAppId, aChallenge,
> 
> So, does this create a new thread each time? Or does it just assign one
> thread from the pool with some sort of limiting?

It queues it for execution in the `SharedThreadPool` named `mThreadPool`, which handles spinning up and down idle threads. In keeping with other usage in Gecko, U2FThread is _not_ a thread, it's a run-to-completion work unit that runs on a thread. Perhaps I should rename `U2FThread` to `U2FRunnable`?
Sorry for the delay in getting this review updated with the split-out patch.

This patch splits things apart, fixes the shutdown crash by using SharedThreadPool correctly, renames the U2FThread to U2FRunnable, and rebases on the Bug 1281932 intermittent fix.

Try run is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e875c5280889523c886d9f2c2a623416b1c5d23c&selectedJob=28931956
Comment on attachment 8789947 [details]
Bug 1297552 - Perform U2F hash operations more efficiently

https://reviewboard.mozilla.org/r/77972/#review83588

Sounds good.

::: dom/u2f/U2F.cpp:180
(Diff revision 2)
>        }
>      }
>    }
>  
> +  // Hash the AppID into the AppParam
> +  SECStatus srv;

nit: declare this when it's used

::: dom/u2f/U2F.cpp:185
(Diff revision 2)
> +  SECStatus srv;
> +  nsCString cAppId = NS_ConvertUTF16toUTF8(mAppId);
> +  CryptoBuffer appParam;
> +  if (!appParam.SetLength(SHA256_LENGTH, fallible)) {
> +    ReturnError(ErrorCode::OTHER_ERROR);
> +    return NS_ERROR_FAILURE;

This is OOM, right?

::: dom/u2f/U2F.cpp:334
(Diff revision 2)
>      ReturnError(ErrorCode::OTHER_ERROR);
>      return NS_ERROR_FAILURE;
>    }
>  
> +  // Hash the AppID into the AppParam
> +  SECStatus srv;

nit: same here

::: dom/u2f/U2F.cpp:339
(Diff revision 2)
> +  SECStatus srv;
> +  nsCString cAppId = NS_ConvertUTF16toUTF8(mAppId);
> +  CryptoBuffer appParam;
> +  if (!appParam.SetLength(SHA256_LENGTH, fallible)) {
> +    ReturnError(ErrorCode::OTHER_ERROR);
> +    return NS_ERROR_FAILURE;

OOM here
Attachment #8789947 - Flags: review?(dkeeler) → review+
Comment on attachment 8799840 [details]
Bug 1297552 - Only permit U2F operations in e10s mode

https://reviewboard.mozilla.org/r/84932/#review83596

Being used to things that can only run in the parent process (and/or if e10s is disabled) this does seem a bit strange, but since we're shipping e10s and if this simplifies things, this sounds good.

::: dom/u2f/U2F.cpp:610
(Diff revision 1)
> +  }
> +
> +  // This only functions in e10s mode
> +  if (XRE_IsParentProcess()) {
> +    MOZ_LOG(gWebauthLog, LogLevel::Debug,
> +      ("Is non-e10s Process, U2F not available"));

nit: indentation
Attachment #8799840 - Flags: review?(dkeeler) → review+
Comment on attachment 8799843 [details]
Bug 1297552 - Disable non-e10s tests

https://reviewboard.mozilla.org/r/84938/#review83600

Might make sense to fold this in with part 2, but it's not a big deal.
Attachment #8799843 - Flags: review?(dkeeler) → review+
Comment on attachment 8799841 [details]
Bug 1297552 - Use MozPromise to run U2F operations in parallel.

https://reviewboard.mozilla.org/r/84934/#review83602

Well, this is a fairly large, somewhat complicated change, but I think I understand it, and it looks like it should work. I noted a few nits and some things we should probably address, but I don't really feel like I'll need to look at the interdiff unless you want me to double-check it, so r=me with comments addressed.

::: dom/u2f/U2F.h:211
(Diff revision 1)
> +};
> +
> +// U2FRunnables run to completion, performing a single U2F operation such as
> +// registering, or signing.
> +class U2FRunnable : public Runnable,
> +                    public nsNSSShutDownObject

nit: it can be nice stylistically to put the ',' from the previous line on this line below the ':' (although I see that this file already has instances where it doesn't follow this convention)

::: dom/u2f/U2F.h:215
(Diff revision 1)
> +class U2FRunnable : public Runnable,
> +                    public nsNSSShutDownObject
> +{
> +public:
> +  U2FRunnable(const nsAString& aOrigin,
> +              const nsAString& aAppId);

nit: let's not split lines where there's enough space to put everything on one line

::: dom/u2f/U2F.cpp:78
(Diff revision 1)
>  {}
>  
> -U2FTask::~U2FTask()
> +U2FStatus::~U2FStatus()
>  {}
>  
> -U2FRegisterTask::U2FRegisterTask(const nsAString& aOrigin,
> +void U2FStatus::WaitGroupAdd()

nit: return type (such as it is) on its own line

::: dom/u2f/U2F.cpp:91
(Diff revision 1)
> +
> +void U2FStatus::WaitGroupDone()
>  {
> -  nsNSSShutDownPreventionLock locker;
> +  ReentrantMonitorAutoEnter mon(mReentrantMonitor);
>  
> -  if (isAlreadyShutDown()) {
> +  mCount -= 1;

Should we assert that mCount >= 0 after this?

::: dom/u2f/U2F.cpp:113
(Diff revision 1)
> +
> +  MOZ_ASSERT(mCount == 0);
> +  MOZ_LOG(gWebauthLog, LogLevel::Debug,
> +          ("U2FStatus::Wait completed, now count=%d stopped=%d", mCount,
> +           mIsStopped));
> -    return;
> +  return;

no need for an explicit return here :)

::: dom/u2f/U2F.cpp:277
(Diff revision 1)
> -  }
> +}
>  
> -  // Hash the AppID into the AppParam
> -  SECStatus srv;
> -  nsCString cAppId = NS_ConvertUTF16toUTF8(mAppId);
> -  CryptoBuffer appParam;
> +NS_IMETHODIMP
> +U2FRegisterTask::Run()
> +{
> +  nsNSSShutDownPreventionLock locker;

Does this need to hold the nsNSSShutDownPreventionLock directly? I'm not seeing any direct NSS usage... (along similar lines, does U2FRegisterTask need to be an nsNSSShutDownObject at all? (Same question with U2FSignTask, looks like))

::: dom/u2f/U2F.cpp:310
(Diff revision 1)
> +    return NS_ERROR_FAILURE;
> -    }
> +  }
>  
> -    CryptoBuffer clientData;
> -    nsresult rv = AssembleClientData(mOrigin, kFinishEnrollment,
> -                                     request.mChallenge.Value(),
> +  MOZ_ASSERT(buffer);
> +  CryptoBuffer regData;
> +  regData.Assign(buffer, bufferlen);

Hmmm - maybe this is a result of not being rebased on the most recent trunk, but we need to handle this not succeeding, right?

::: dom/u2f/U2F.cpp:314
(Diff revision 1)
> -    nsresult rv = AssembleClientData(mOrigin, kFinishEnrollment,
> -                                     request.mChallenge.Value(),
> -                                     clientData);
> -    if (NS_WARN_IF(NS_FAILED(rv))) {
> -      ReturnError(ErrorCode::OTHER_ERROR);
> +  CryptoBuffer regData;
> +  regData.Assign(buffer, bufferlen);
> +  free(buffer);
> +
> +  // Assemble a response object to return
> +  nsString clientDataBase64, registrationDataBase64;

nit: one declaration per line

::: dom/u2f/U2F.cpp:365
(Diff revision 1)
> +}
> +
> +NS_IMETHODIMP
> +U2FSignTask::Run()
> +{
> +  nsNSSShutDownPreventionLock locker;

Same here with the NSS lock.

::: dom/u2f/U2F.cpp:409
(Diff revision 1)
> -          ReturnError(ErrorCode::OTHER_ERROR);
> +    mPromise.Reject(ErrorCode::OTHER_ERROR, __func__);
> -          return NS_ERROR_FAILURE;
> +    return NS_ERROR_FAILURE;
> -        }
> +  }
>  
> -        MOZ_ASSERT(buffer);
> +  MOZ_ASSERT(buffer);
> -        regData.Assign(buffer, bufferlen);
> +  signatureData.Assign(buffer, bufferlen);

Same here with handling failure.

::: dom/u2f/U2F.cpp:413
(Diff revision 1)
> -      // Try another request
> -      continue;
> -    }
>  
> -    // Assemble a response object to return
> +  // Assemble a response object to return
> -    nsString clientDataBase64, registrationDataBase64;
> +  nsString clientDataBase64, signatureDataBase64, keyHandleBase64;

nit: one declaration per line again

::: dom/u2f/U2F.cpp:487
(Diff revision 1)
> +  mRegisteredKeys.SetLength(aRegisteredKeys.Length());
> +  for (size_t i = 0; i < aRegisteredKeys.Length(); ++i) {
> +    RegisteredKey key(aRegisteredKeys[i]);
> +
> +    // Check for required attributes
> +    if (!key.mVersion.WasPassed() || !key.mKeyHandle.WasPassed()) {

It seems like this could result in empty slots in mRegisteredKeys and it's not clear that the code that consumes these keys handles that properly. Maybe just use .AppendElement()?

::: dom/u2f/U2F.cpp:601
(Diff revision 1)
>    if (!appParam.SetLength(SHA256_LENGTH, fallible)) {
> -    ReturnError(ErrorCode::OTHER_ERROR);
>      return NS_ERROR_FAILURE;
>    }
>  
>    srv = PK11_HashBuf(SEC_OID_SHA256, appParam.Elements(),

If we used nsICryptoHash, I don't think U2FRunnable would need to be an nsNSSShutDownObject, but maybe that's an improvement for another bug.

::: dom/u2f/U2F.cpp:699
(Diff revision 1)
> +  mRegisteredKeys.SetLength(aRegisteredKeys.Length());
> +  for (size_t i = 0; i < aRegisteredKeys.Length(); ++i) {
> +    RegisteredKey key(aRegisteredKeys[i]);
> +
> +    // Check for required attributes
> +    if (!key.mVersion.WasPassed() || !key.mKeyHandle.WasPassed()) {

Same idea here with having gaps in mRegisteredKeys.

::: dom/u2f/U2F.cpp:779
(Diff revision 1)
> -      ReturnError(ErrorCode::OTHER_ERROR);
> -      return NS_ERROR_FAILURE;
> +    return NS_ERROR_FAILURE;
> -    }
> +  }
>  
> +  // Search the signing requests for one a token can fulfill
> +  for (size_t i = 0; i < mRegisteredKeys.Length(); i += 1) {

For these iterator-like loops, can we just do `for (LocalRegistered key : mRegisteredKeys)` etc.?
Attachment #8799841 - Flags: review?(dkeeler) → review+
Comment on attachment 8799841 [details]
Bug 1297552 - Use MozPromise to run U2F operations in parallel.

https://reviewboard.mozilla.org/r/84934/#review83602

OK, thanks Keeler! I'm going to ask you to double-check it since I needed to move the `nsNSSShutDownPreventionLock` logic out of U2F*Task and into U2F*Runnable; always good to make sure I didn't do it wrong.

> Does this need to hold the nsNSSShutDownPreventionLock directly? I'm not seeing any direct NSS usage... (along similar lines, does U2FRegisterTask need to be an nsNSSShutDownObject at all? (Same question with U2FSignTask, looks like))

You're right; I left that structure in there from when these actually did the hashes. Thank you.

> Hmmm - maybe this is a result of not being rebased on the most recent trunk, but we need to handle this not succeeding, right?

Yeah, this is fixed in the rebase.

> It seems like this could result in empty slots in mRegisteredKeys and it's not clear that the code that consumes these keys handles that properly. Maybe just use .AppendElement()?

Very, very good catch. Thank you. Did as you recommended.

> If we used nsICryptoHash, I don't think U2FRunnable would need to be an nsNSSShutDownObject, but maybe that's an improvement for another bug.

Cool; added a note, but not opening a bug at this time.

> For these iterator-like loops, can we just do `for (LocalRegistered key : mRegisteredKeys)` etc.?

Hmmm, I guess we can now!
Comment on attachment 8799841 [details]
Bug 1297552 - Use MozPromise to run U2F operations in parallel.

https://reviewboard.mozilla.org/r/84934/#review83994
Attachment #8799843 - Attachment is obsolete: true
Comment on attachment 8799841 [details]
Bug 1297552 - Use MozPromise to run U2F operations in parallel.

https://reviewboard.mozilla.org/r/84934/#review84220

LGTM!

::: dom/u2f/U2F.cpp:456
(Diff revisions 1 - 3)
>    MOZ_ASSERT(NS_IsMainThread());
>  
>    // The WebIDL dictionary types RegisterRequest and RegisteredKey cannot
>    // be copied to this thread, so store them serialized.
> -  mRegisterRequests.SetLength(aRegisterRequests.Length());
>    for (size_t i = 0; i < aRegisterRequests.Length(); ++i) {

I think there are a few more places where we could use the loop iterator style rather than loop index style, but it's not a big deal.

::: dom/u2f/U2F.cpp:498
(Diff revisions 1 - 3)
>    }
>  }
>  
>  U2FRegisterRunnable::~U2FRegisterRunnable()
> -{}
> +{
> +  nsNSSShutDownPreventionLock locker;

Hmmm - we should be able to put this in ~U2FRunnable and share the code, right? (Of course, this only works as long as the subclasses never have to directly release NSS resources, but if we're eventually going to have these not be nsNSSShutDownObjects anyway, I think it's fine for now.)
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f824c01ff5ca
Perform U2F hash operations more efficiently r=keeler
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf9ddecd3f45
Only permit U2F operations in e10s mode r=keeler
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ffab2bebce5
Use MozPromise to run U2F operations in parallel. r=keeler
https://hg.mozilla.org/integration/mozilla-inbound/rev/14d6e492e2bb
Reorder parts of U2F.cpp r=keeler
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: