Richard Barnes [:rbarnes] Reporter
	Description • 9 years ago

      No description provided.

	Richard Barnes [:rbarnes] Reporter
	Comment 1 • 9 years ago

This is a sketch of how things might hang together at a high level.  It has:

- WebIDL and an interface object implementation
- An "NSS Token" that implements the token part of the protocol

With those bits, we can now pass a basic U2F test suite (which Chrome's impl also passes with a real U2F device):

https://ipv.sx/u2f/

	J.C. Jones [:jcj] (he/him) Assignee
	Comment 2 • 9 years ago

Thanks for the starting point, rbarnes. I'll pick up from here.

	J.C. Jones [:jcj] (he/him) Assignee
	Comment 3 • 9 years ago

There's an early point-in-time, WIP review open at MozReview here: https://reviewboard.mozilla.org/r/31691/

(I'm still very much working on test coverage.)

	J.C. Jones [:jcj] (he/him) Assignee
	Comment 4 • 9 years ago

[Not using MozReview due to current breakage there.]

This is an initial implementation of the FIDO U2F high-level JS API v1.1. It's based on rbarnes' design and initial code. This patch creates a very basic NSS-based U2F token (NSSToken) which is insecure and inefficient, but serves as a basis for further work. It also creates the skeleton of a USB-based U2F token layer (USBToken), which is assumed to handle addressing zero or more U2F Tokens on the USB bus. In the current design, it is expected that USBToken will encapsulate the state machines to interact successfully across the bus.

Note: this is implementing the JS API v1.1 which is in draft status.

The AppId / FacetId algorithm is currently incomplete: it does not perform remote fetches to obtain the TrustedFacets resource. There are a few tests commented out in dom/u2f/tests/test_frame_appid_facet_remoteload.html which should be useful when implementing the remote load functionality. This will be fixed in Bug 1244959.

The NSSToken similarly needs work, that's going to be handled in Bug 1244960. Note: This currently fails at https://u2fdemo.appspot.com/ due to server-side failures, likely caused by the NSSToken not including an Attestation Certificate or Signature. It does work on https://usr.bin.coffee/u2f/ .

Changes since the first review (https://reviewboard.mozilla.org/r/31691/):
- Upgrade to the U2F high-level JS API v1.1
- Rebase onto the recent nsPIDOMWindow changes (Bug 1241764)
- Rework USBToken and NSSToken to implement re-registration protection
- Add FacetID / AppID tests
- Cleanup from comments.

	J.C. Jones [:jcj] (he/him) Assignee
	Comment 5 • 9 years ago

Apologies; there was a leak in the prior patch due to improper use of the wrappercache for the U2F object. This appears to correct the problem locally.

Also, try has helpfully informed me that I need to find a DOM peer for review; I'll reach out to find an appropriate person this morning.

	J.C. Jones [:jcj] (he/him) Assignee
	Comment 6 • 9 years ago

Revised to fix a problem building release, and update the DOM interface test. This rev's try run is available here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c50e9fa7892c

Baku: I'd appreciate your input on the WebIDL and nsGlobalWindow changes. They're intended to mimic the Crypto module.

	Andrea Marchesini [:baku]
	Comment 7 • 9 years ago

Comment on attachment 8715484 [details] [diff] [review]
1231681-window.u2f.diff r4

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

I think dkeeler will review all the security parts... but I would like to see this patch again with these simple comments fixed. Thanks.
In particular, I need to have the correct URL of the spec in order to check if the implementation is correct :)

::: dom/base/nsGlobalWindow.cpp
@@ +4369,5 @@
> +nsGlobalWindow::GetU2f(ErrorResult& aError)
> +{
> +  MOZ_RELEASE_ASSERT(IsInnerWindow());
> +
> +  if (!Preferences::GetBool("security.webauth.u2f", true)) {

This is not needed. WebIDL takes care of it.
This means that you can remove [Throws] and get rid of ErrorResult here.

::: dom/base/nsGlobalWindow.h
@@ +105,5 @@
>  class Crypto;
>  class External;
>  class Function;
>  class Gamepad;
> +class U2F;

alphabetic order.

::: dom/crypto/CryptoBuffer.cpp
@@ +29,5 @@
>  
>  uint8_t*
> +CryptoBuffer::Assign(const nsACString& aString)
> +{
> +  return Assign(const_cast<uint8_t*>(reinterpret_cast<uint8_t const*>(aString.BeginReading())), aString.Length());

do you need this const_cast too? Assign() take a const uint8_t*.

::: dom/tests/mochitest/general/test_interfaces.html
@@ +1372,5 @@
>  // IMPORTANT: Do not change this list without review from a DOM peer!
>      {name: "TVTuner", b2g: true, permission: ["tv"]},
>  // IMPORTANT: Do not change this list without review from a DOM peer!
> +   "U2F",
> +// IMPORTANT: Do not change this list without review from a DOM peer!

I guess that we want to disable it for release builds.
Add a 'release: false'. And let's enable it in a follow up or a separate patch.

::: dom/u2f/U2F.cpp
@@ +17,5 @@
> +namespace mozilla {
> +namespace dom {
> +
> +enum class ErrorCode {
> +  OK = 0,

Add, somewhere, a MOZ_ASSERT to check that these error codes are in sync with the webidl enum values.

@@ +65,5 @@
> +}
> +
> +void
> +U2F::Init(nsPIDOMWindowInner* aParent)
> +{

MOZ_ASSERT(!mParent), just to avoid double Init().

@@ +74,5 @@
> +  MOZ_ASSERT(doc);
> +
> +  nsIPrincipal* principal = doc->NodePrincipal();
> +  nsresult rv = nsContentUtils::GetUTFOrigin(principal, mOrigin);
> +  NS_ENSURE_SUCCESS_VOID(rv);

Propagate this error result.
This Init should be Init(ErrorResult& aRv);

@@ +81,5 @@
> +    return;
> +  }
> +
> +  rv = mSoftToken.Init();
> +  NS_ENSURE_SUCCESS_VOID(rv);

same here.

@@ +84,5 @@
> +  rv = mSoftToken.Init();
> +  NS_ENSURE_SUCCESS_VOID(rv);
> +
> +  rv = mUSBToken.Init();
> +  NS_ENSURE_SUCCESS_VOID(rv);

here too. Plus use this syntax.

aRv = mUSBToken.Init();
if (NS_WARN_IF(aRv.Failed())) {
  return;
}

@@ +88,5 @@
> +  NS_ENSURE_SUCCESS_VOID(rv);
> +}
> +
> +nsresult
> +U2F::AssembleClientData(const nsAString& aTyp,

aType ?

@@ +93,5 @@
> +                        const nsAString& aChallenge,
> +                        CryptoBuffer& aClientData)
> +{
> +  ClientData clientDataObject;
> +  clientDataObject.mTyp.Construct(aTyp);

sure its' typ and not type?

@@ +98,5 @@
> +  clientDataObject.mChallenge.Construct(aChallenge);
> +  clientDataObject.mOrigin.Construct(mOrigin);
> +
> +  nsAutoString json;
> +  if (!clientDataObject.ToJSON(json)) {

NS_WARN_IF

@@ +103,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  uint8_t* result = aClientData.Assign(NS_ConvertUTF16toUTF8(json));
> +  if (!result) {

NS_WARN_IF

@@ +121,5 @@
> +
> +  MOZ_ASSERT(urlParser);
> +  MOZ_ASSERT(tldService);
> +
> +  if (NS_WARN_IF(mOrigin.IsEmpty())) { return false; }

can you do this check before creating the services?
Then:
if (NS_WARN_IF(...)) {
  return false;
}

@@ +133,5 @@
> +  nsresult rv = urlParser->ParseURL(facetUrl.get(), mOrigin.Length(),
> +                                    &facetSchemePos, &facetSchemeLen,
> +                                    &facetAuthPos, &facetAuthLen,
> +                                    nullptr, nullptr);      // ignore path
> +  if (NS_WARN_IF(NS_FAILED(rv))) { return false; }

new lines {
  return false;
}

@@ +147,5 @@
> +  rv = urlParser->ParseURL(appIdUrl.get(), aAppId.Length(),
> +                           &appIdSchemePos, &appIdSchemeLen,
> +                           &appIdAuthPos, &appIdAuthLen,
> +                           nullptr, nullptr);      // ignore path
> +  if (NS_WARN_IF(NS_FAILED(rv))) { return false; }

everywhere in this file.

@@ +154,5 @@
> +  nsAutoCString appIdAuth(Substring(appIdUrl, appIdAuthPos, appIdAuthLen));
> +
> +  // if the facetId or appId URL is HTTP, reject.
> +  if (facetScheme.LowerCaseEqualsLiteral("http") ||
> +      appIdScheme.LowerCaseEqualsLiteral("http")) {

NS_WARN_IF ?

@@ +198,5 @@
> +
> +  ErrorResult rv;
> +  aCallback.Call(response, rv);
> +  // Useful exceptions already got reported.
> +  rv.SuppressException();

ok but do before that:

NS_WARN_IF(rv.Failed());

@@ +230,5 @@
> +    return;
> +  }
> +
> +  size_t i;
> +  for (i = 0; i < aRegisteredKeys.Length(); i += 1) {

++i ?

@@ +240,5 @@
> +      continue;
> +    }
> +
> +    // Verify the appId for this Registered Key, if set
> +    if (request.mAppId.WasPassed() && !ValidAppID(request.mAppId.Value())) {

80chars max.

@@ +246,5 @@
> +    }
> +
> +    // Decode the key handle
> +    CryptoBuffer keyHandle;
> +    if (NS_FAILED(keyHandle.FromJwkBase64(request.mKeyHandle.Value()))) {

NS_WARN_IF

@@ +258,5 @@
> +
> +    // 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 (usbTokenEnabled &&
> +        mUSBToken.IsCompatibleVersion(request.mVersion.Value())) {

can you unify these 2 if statements?

@@ +268,5 @@
> +    }
> +
> +    if (softTokenEnabled &&
> +        mSoftToken.IsCompatibleVersion(request.mVersion.Value())) {
> +      if (mSoftToken.IsRegistered(keyHandle)) {

same here.

@@ +290,5 @@
> +    CryptoBuffer clientData;
> +    nsresult rv = AssembleClientData(FinishEnrollment,
> +                                     request.mChallenge.Value(),
> +                                     clientData);
> +    if (NS_FAILED(rv)) {

NS_WARN_IF

@@ +330,5 @@
> +
> +    if (usbTokenEnabled &&
> +        mUSBToken.IsCompatibleVersion(request.mVersion.Value())) {
> +      if (NS_FAILED(mUSBToken.Register(opt_aTimeoutSeconds, challengeParam,
> +                         appParam, registrationData))) {

indentation. + NS_WARN_IF

@@ +340,5 @@
> +    }
> +
> +    if (softTokenEnabled &&
> +        mSoftToken.IsCompatibleVersion(request.mVersion.Value())) {
> +      if (NS_FAILED(mSoftToken.Register(challengeParam,

NS_WARN_IF

@@ +357,5 @@
> +
> +    // Assemble a response object to return
> +    nsString clientDataBase64, registrationDataBase64;
> +    if (NS_FAILED(clientData.ToJwkBase64(clientDataBase64)) ||
> +        NS_FAILED(registrationData.ToJwkBase64(registrationDataBase64))) {

ditto.

@@ +369,5 @@
> +    response.mRegistrationData.Construct(registrationDataBase64);
> +    response.mErrorCode.Construct((uint32_t) ErrorCode::OK);
> +
> +    ErrorResult result;
> +    aCallback.Call(response, result);

NS_WARN_IF(result.Failed()) ?
Plus you want to suppress the error.

@@ +432,5 @@
> +
> +    // Assemble a clientData object
> +    CryptoBuffer clientData;
> +    nsresult rv = AssembleClientData(GetAssertion, aChallenge, clientData);
> +    if (NS_FAILED(rv)) {

ditto.

@@ +468,5 @@
> +
> +    // Decode the key handle
> +    CryptoBuffer keyHandle;
> +    rv = keyHandle.FromJwkBase64(request.mKeyHandle.Value());
> +    if (NS_FAILED(rv)) {

ditto.

@@ +525,5 @@
> +    response.mSignatureData.Construct(signatureDataBase64);
> +    response.mErrorCode.Construct((uint32_t) ErrorCode::OK);
> +
> +    ErrorResult result;
> +    aCallback.Call(response, result);

ditto.

::: dom/u2f/U2F.h
@@ +36,5 @@
> +class U2F final : public nsISupports,
> +                  public nsWrapperCache,
> +                  public nsNSSShutDownObject
> +{
> +protected:

no protected if final.

@@ +89,5 @@
> +
> +  nsresult
> +  AssembleClientData(const nsAString& aTyp,
> +                     const nsAString& aChallenge,
> +                     CryptoBuffer& aClientData);

const ?

@@ +92,5 @@
> +                     const nsAString& aChallenge,
> +                     CryptoBuffer& aClientData);
> +
> +  bool
> +  ValidAppID(nsString& aAppId);

const

::: dom/u2f/USBToken.cpp
@@ +19,5 @@
> +nsresult
> +USBToken::Init()
> +{
> +  if (mInitialized) {
> +    return NS_OK;

remove all of this. You are actually doing anything in the Init.
Get rid of the init and this mInitialized.

@@ +35,5 @@
> +  return mVersion == aVersionParam;
> +}
> +
> +bool
> +USBToken::IsRegistered(const CryptoBuffer& aKeyHandle)

const

::: dom/u2f/USBToken.h
@@ +23,5 @@
> +  nsresult Init();
> +
> +  bool IsCompatibleVersion(const nsString& aVersionParam) const ;
> +
> +  bool IsRegistered(const CryptoBuffer& aKeyHandle) ;

const;
No extra space before ';'
Also for other methods.

::: dom/webidl/U2F.webidl
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + *
> + * The origin of this IDL file is
> + * https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#crypto-interface

This is the wrong URL.

@@ +23,5 @@
> +    "usb"
> +};
> +
> +dictionary ClientData {
> +    DOMString             typ;

no default value for these strings?

	J.C. Jones [:jcj] (he/him) Assignee
	Comment 8 • 9 years ago

Baku, thank you for the review! I've accepted almost all of your comments.

> ::: dom/base/nsGlobalWindow.cpp
> This means that you can remove [Throws] and get rid of ErrorResult here.

I didn't do this, as it looks like it's better to propagate the ErrorResult out of Init()

> ::: dom/u2f/U2F.cpp
> aType ?

The spec actually refers to this as Typ, so I've kept that notation.

> ::: dom/u2f/USBToken.cpp
> remove all of this. You are actually doing anything in the Init.
> Get rid of the init and this mInitialized.

I didn't do this, as we'll have to add it back in Bug 1245527 that implements the USB HID state machine. So for symmetry with the NSSToken, and anticipated usefulness, I'd like to leave this as a skeleton.

> ::: dom/webidl/U2F.webidl
> This is the wrong URL.

Whoops. Thanks! For those following along, this implements the v1.1 JS API which is not yet published publicly, though it's implemented in recent versions of Chrome. The older version can be found here: https://fidoalliance.org/specs/fido-u2f-v1.0-nfc-bt-amendment-20150514/fido-u2f-javascript-api.html

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews)
	Comment 9 • 9 years ago

Comment on attachment 8715982 [details] [diff] [review]
1231681-window.u2f.diff r5

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

I think my main concern is that we can do much better than having the key identifier be the key itself for the softoken. I don't think this is quite ready to land, so r- for now.

::: dom/crypto/CryptoBuffer.cpp
@@ +29,5 @@
>  
>  uint8_t*
> +CryptoBuffer::Assign(const nsACString& aString)
> +{
> +  return Assign(reinterpret_cast<uint8_t const*>(aString.BeginReading()), aString.Length());

nit: long line

::: dom/u2f/NSSToken.cpp
@@ +13,5 @@
> +#define U2F_PARAM_LEN 32
> +#define U2F_SIGNED_DATA_LEN 2*U2F_PARAM_LEN + 1 + 4
> +
> +#define P256_OID "\x06\x08\x2A\x86\x48\xCE\x3D\x03\x01\x07"
> +#define P256_OID_LEN 10

Why not use static consts instead of #defines?

@@ +47,5 @@
> +  if (!EnsureNSSInitializedChromeOrContent()) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  mSlot = PK11_GetInternalSlot();

In theory this function would still need to acquire the NSS shutdown prevention lock and check if NSS has shut down.

@@ +64,5 @@
> +}
> +
> +static SECItem*
> +keyHandleFromKeyPair(SECKEYPrivateKey* aPrivKey, SECKEYPublicKey* aPubKey,
> +            const nsNSSShutDownPreventionLock&)

nit: indentation

@@ +66,5 @@
> +static SECItem*
> +keyHandleFromKeyPair(SECKEYPrivateKey* aPrivKey, SECKEYPublicKey* aPubKey,
> +            const nsNSSShutDownPreventionLock&)
> +{
> +  ScopedSECItem privKeyItem(::SECITEM_AllocItem(nullptr, nullptr, 0));

nit: handle OOM?

@@ +78,5 @@
> +    return nullptr;
> +  }
> +
> +  size_t keyHandleLen = U2F_PUBLIC_KEY_LEN + privKeyItem->len;
> +  SECItem *keyHandleItem = ::SECITEM_AllocItem(nullptr, nullptr, keyHandleLen);

nit: SECItem*, also, handle OOM?

@@ +86,5 @@
> +         privKeyItem->data, privKeyItem->len);
> +  return keyHandleItem;
> +}
> +
> +static SECKEYPrivateKey* privateKeyFromKeyHandle(PK11SlotInfo *aSlot,

nit: return type on previous line, PK11SlotInfo*

@@ +90,5 @@
> +static SECKEYPrivateKey* privateKeyFromKeyHandle(PK11SlotInfo *aSlot,
> +                                                 const SECItem* aKeyHandle,
> +                                                 const nsNSSShutDownPreventionLock&)
> +{
> +  SECItem param = {siBuffer, (unsigned char*) P256_OID, P256_OID_LEN};

If P256_OID were a static unsigned char*, this cast wouldn't be necessary.

@@ +138,5 @@
> +    return false;
> +  }
> +
> +  // Protect mSlot
> +  MutexAutoLock lock(mMutex);

Do we not need to protect all member variables? (e.g. mInitialized)

@@ +224,5 @@
> +    return NS_ERROR_FAILURE;
> +  };
> +
> +  // TODO(Bug 1244960): Change this to avoid direct-memory operations
> +  uint8_t *data = aRegistrationData.Elements();

nit: uint8_t*

::: dom/u2f/NSSToken.h
@@ +19,5 @@
> +// This is a barebones and incomplete implementation. Further work will be
> +// done in bug 1244960.
> +//
> +// NOTE: Using this token is NOT SECURE.  Key handles are simply a direct
> +// encoding of the private key, so they can be used to forge signatures.

Eh, this is a bit unfortunate. Can we generate a master key, store it in local storage (like webcrypto does, I think? (but I don't know if that's persistent...)), and then derive keys based on the key handle?

@@ +42,5 @@
> +                const CryptoBuffer& aChallengeParam,
> +                const CryptoBuffer& aKeyHandle,
> +                CryptoBuffer& aSignatureData);
> +
> +  // No NSS resources to release.

mSlot needs to be released :)
(just set it to nullptr)

@@ +44,5 @@
> +                CryptoBuffer& aSignatureData);
> +
> +  // No NSS resources to release.
> +  virtual
> +  void virtualDestroyNSSReference() override {};

nit: this whole declaration should be on one line, I think.

::: dom/u2f/U2F.cpp
@@ +18,5 @@
> +namespace dom {
> +
> +// These enumerations are defined in the FIDO U2F Javascript API under the
> +// interface "ErrorCode" as constant integers, and thus in the U2F.webidl file.
> +// Any chances to these must occur in both locations.

s/chances/changes/

@@ +167,5 @@
> +
> +  nsAutoCString appIdScheme(Substring(appIdUrl, appIdSchemePos, appIdSchemeLen));
> +  nsAutoCString appIdAuth(Substring(appIdUrl, appIdAuthPos, appIdAuthLen));
> +
> +  // if the facetId or appId URL is HTTP, reject.

Wouldn't this be better as a whitelist? We just allow https, right?

@@ +231,5 @@
> +              ErrorResult& aRv)
> +{
> +  nsNSSShutDownPreventionLock locker;
> +  if (isAlreadyShutDown()) {
> +    return;

Should this set the error result? Or is it an error by default?

@@ +245,5 @@
> +
> +  // Verify the global appId first.
> +  if (!ValidAppID(appId)) {
> +    SendError<U2FRegisterCallback, RegisterResponse>(aCallback,
> +                                               ErrorCode::BAD_REQUEST);

nit: indentation

@@ +249,5 @@
> +                                               ErrorCode::BAD_REQUEST);
> +    return;
> +  }
> +
> +  size_t i;

If i isn't needed outside the loop, put it in the for statement.

@@ +282,5 @@
> +    // then we'll return DEVICE_INELIGIBLE to signify we're already registered.
> +    if (usbTokenEnabled &&
> +        mUSBToken.IsCompatibleVersion(request.mVersion.Value()) &&
> +        mUSBToken.IsRegistered(keyHandle)) {
> +        SendError<U2FRegisterCallback, RegisterResponse>(aCallback,

nit: 2 space indentation for bodies of conditionals

@@ +283,5 @@
> +    if (usbTokenEnabled &&
> +        mUSBToken.IsCompatibleVersion(request.mVersion.Value()) &&
> +        mUSBToken.IsRegistered(keyHandle)) {
> +        SendError<U2FRegisterCallback, RegisterResponse>(aCallback,
> +                                                  ErrorCode::DEVICE_INELIGIBLE);

nit: indentation

@@ +291,5 @@
> +    if (softTokenEnabled &&
> +        mSoftToken.IsCompatibleVersion(request.mVersion.Value()) &&
> +        mSoftToken.IsRegistered(keyHandle)) {
> +        SendError<U2FRegisterCallback, RegisterResponse>(aCallback,
> +                                                  ErrorCode::DEVICE_INELIGIBLE);

nit: indentation

@@ +329,5 @@
> +    nsCString cAppId = NS_ConvertUTF16toUTF8(appId);
> +
> +    // Hash the AppID and the ClientData into the AppParam and ChallengeParam
> +    srv = PK11_HashBuf(SEC_OID_SHA256, appParam.Elements(),
> +                       (uint8_t*) cAppId.BeginReading(), cAppId.Length());

I think c++ style casts are generally preferred.

@@ +350,5 @@
> +
> +    if (usbTokenEnabled &&
> +        mUSBToken.IsCompatibleVersion(request.mVersion.Value())) {
> +      rv = mUSBToken.Register(opt_aTimeoutSeconds, challengeParam,
> +                         appParam, registrationData);

nit: indentation

@@ +353,5 @@
> +      rv = mUSBToken.Register(opt_aTimeoutSeconds, challengeParam,
> +                         appParam, registrationData);
> +      if (NS_WARN_IF(NS_FAILED(rv))) {
> +        SendError<U2FRegisterCallback, RegisterResponse>(aCallback,
> +                                                        ErrorCode::OTHER_ERROR);

nit: indentation

@@ +359,5 @@
> +      }
> +      registerSuccess = true;
> +    }
> +
> +    if (softTokenEnabled &&

If both the USB token and the softoken were enabled, would this register on both? Do we want that?

@@ +364,5 @@
> +        mSoftToken.IsCompatibleVersion(request.mVersion.Value())) {
> +      rv = mSoftToken.Register(challengeParam, appParam, registrationData);
> +      if (NS_WARN_IF(NS_FAILED(rv))) {
> +        SendError<U2FRegisterCallback, RegisterResponse>(aCallback,
> +                                                        ErrorCode::OTHER_ERROR);

nit: indentation

@@ +398,5 @@
> +    aCallback.Call(response, result);
> +    NS_WARN_IF(result.Failed());
> +    // Useful exceptions already got reported.
> +    result.SuppressException();
> +    return;

Huh - so we only process the first registration request that succeeds? (Maybe I need to read the spec more closely...)

@@ +417,5 @@
> +          ErrorResult& aRv)
> +{
> +  nsNSSShutDownPreventionLock locker;
> +  if (isAlreadyShutDown()) {
> +    return;

Should this set the error result?

@@ +510,5 @@
> +    // We ignore mTransports, as it is intended to be used for sorting the
> +    // available devices by preference, but is not an exclusion factor.
> +
> +    // Determine if the provided keyHandle is registered at any device. If so,
> +    // then we'll return DEVICE_INELIGIBLE to signify we're already registered.

Update comment?

::: dom/u2f/U2F.h
@@ +49,5 @@
> +    return mParent;
> +  }
> +
> +  void
> +  Init(nsPIDOMWindowInner* aParent, ErrorResult& aRv);

nit: declarations on one line, I think

::: dom/u2f/USBToken.h
@@ +23,5 @@
> +  nsresult Init();
> +
> +  bool IsCompatibleVersion(const nsString& aVersionParam) const ;
> +
> +  bool IsRegistered(const CryptoBuffer& aKeyHandle) const ;

nit: my intuition is that the desired style would be to not have a space after const here.

::: dom/u2f/tests/u2futil.js
@@ +19,5 @@
> +  parent.postMessage(body, "http://mochi.test:8888");
> +}
> +
> +function local_doesThrow(fn, name) {
> +    var gotException = false;

nit: indentation

@@ +44,5 @@
> +function bytesToBase64(u8a){
> +  var CHUNK_SZ = 0x8000;
> +  var c = [];
> +  for (var i=0; i < u8a.length; i+=CHUNK_SZ) {
> +    c.push(String.fromCharCode.apply(null, u8a.subarray(i, i+CHUNK_SZ)));

nit: spaces around operators

@@ +130,5 @@
> +  var padR = lenR - 32;
> +  var padS = lenS - 32;
> +  var sig = new Uint8Array(64);
> +  derSig.slice(4+padR,4+lenR).map((x,i) => sig[i] = x);
> +  derSig.slice(4+lenR+2+padS,4+lenR+2+lenS).map((x,i) => sig[32+i] = x);

spaces around operators

::: dom/webidl/U2F.webidl
@@ +35,5 @@
> +
> +dictionary RegisterRequest {
> +    DOMString version;
> +    DOMString challenge;
> +};

Doesn't this need appId as well?

@@ +37,5 @@
> +    DOMString version;
> +    DOMString challenge;
> +};
> +
> +dictionary RegisterResponse {

The spec I'm looking at only has registrationData and clientData for RegisterResponse. Am I looking at the wrong version? Or is this an implementation limitation that we have to include these other fields?

@@ +47,5 @@
> +    ErrorCode? errorCode;
> +    DOMString? errorMessage;
> +};
> +
> +dictionary RegisteredKey {

Where is this specified? I can't find it in either of the two links up top. (Is this because v1.1 isn't published?)

	J.C. Jones [:jcj] (he/him) Assignee
	Comment 10 • 9 years ago

Keeler, thanks for re-reviewing!

I agree that the NSSToken is unfortunate as-is. It is intended as a unit test mechanism rather than an actual security feature -- it won't be FIDO-compliant anytime soon.

In the interests of safety, I stripped the implementation of NSSToken.cpp down to a stub, so as to not land a half-implemented, obviously-insecure token that could be used unwisely. This has also prompted me to disable some of the unit tests, to be added back in Bug 1244960.

I have accepted all of your comments, and written some specific responses:

> Do we not need to protect all member variables? (e.g. mInitialized)

Is it also appropriate to lock Init()? It should only be called once by nsGlobalWindow.cpp, so it's not performance-critical code, and I guess that would be safer?

Anyway, NSSToken is now a stub, so while I went ahead and put it there (pending advice not to), we can also change it in the follow on Bug 1244960.

> If P256_OID were a static unsigned char*, this cast wouldn't be necessary.

The hardcoded OID byte string was just a bad idea; I left it here from the proof-of-concept code. I'll swap this to the much more appropriate call to SECOID_FindOIDByTag(SEC_OID_SECG_EC_SECP256R1) in the implementation of NSSToken.

> Wouldn't this be better as a whitelist? We just allow https, right?

I agree, it's clearer that way. So far I've been doing the steps exactly as the algorithm specifies, but making it a whitelist seems reasonable, and we're already departing from the algorithm by prohibiting HTTP [1].

> If both the USB token and the softoken were enabled, would this register on both? Do we want that?

Nice catch. In both locations like this, it should try the USB token first, and if that fails, proceed to the soft token.

> Huh - so we only process the first registration request that succeeds? (Maybe I need to read the spec more closely...)

Yep, the first one wins; it's an ordered list. And in our soft/hard token semantics, we want to try physical devices first, since the soft device doesn't have the (required by spec) user presence test. The NSSToken is really intended only for testing purposes at this time.

> Doesn't this need appId as well?

No, that moved into the function signatures in v1.1, to bring it more in-line with FIDO 2.0's definition of AppId.

> The spec I'm looking at only has registrationData and clientData for RegisterResponse. Am I looking at the wrong version? Or is this an implementation limitation that we have to include these other fields?
> Where is this specified? I can't find it in either of the two links up top. (Is this because v1.1 isn't published?)

Unfortunately, v1.1 has not been published yet, so you're looking at the wrong version. You can take a look at the Chromium implementation of v1.1 [2], or I can provide the pre-release spec within MoCo.

[1]: https://fidoalliance.org/specs/fido-u2f-v1.0-nfc-bt-amendment-20150514/fido-appid-and-facets.html#determining-if-a-caller-s-facetid-is-authorized-for-an-appid
[2]: https://chromium.googlesource.com/chromium/src/+/master/chrome/browser/resources/cryptotoken/webrequest.js

	Martin Thomson [:mt:]
	Comment 11 • 9 years ago

JC, just wondering if you actually had looked into [SecureContext] (https://w3c.github.io/webappsec-secure-contexts/#new).  That's bug 1177957.

	J.C. Jones [:jcj] (he/him) Assignee
	Comment 12 • 9 years ago

mt: rbarnes and I had discussed [SecureContext]; seems like I should open a bug to add that WebIDL notation when 1177957 lands. Would that be the right approach here?

	Martin Thomson [:mt:]
	Comment 13 • 9 years ago

I would have said that it would be a precondition for landing.  It's not a particularly large piece of work.  It just needs some attention.

	Andrea Marchesini [:baku]
	Comment 14 • 9 years ago

Comment on attachment 8716532 [details] [diff] [review]
1231681-window.u2f-r6.diff

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

::: dom/base/nsGlobalWindow.cpp
@@ +77,5 @@
>  #include "AudioChannelService.h"
>  #include "nsAboutProtocolUtils.h"
>  #include "nsCharTraits.h" // NS_IS_HIGH/LOW_SURROGATE
>  #include "PostMessageEvent.h"
> +#include "mozilla/dom/U2F.h"

move it to the other mozilla/dom/* headers.

@@ +4371,5 @@
> +  MOZ_RELEASE_ASSERT(IsInnerWindow());
> +
> +  if (!mU2F) {
> +    mU2F = new U2F();
> +    mU2F->Init(AsInner(), aError);

this is wrong. You should do this:

if (!m2UF) {
  RefPtr<U2F> u2f = new U2F();
  u2f->Init(AsInner(), aError);
  if (NS_WARN_IF(aError.Failed()) {
    return nullptr;
  }

  mU2F = u2f;
}

return mU2F;

so basically, assign mU2F only if you don't have any error. And use NS_WARN_IF in case of warning.

::: dom/u2f/NSSToken.cpp
@@ +51,5 @@
> +NSSToken::Init()
> +{
> +  MutexAutoLock lock(mMutex);
> +
> +  if (mInitialized) {

What about asserting this? Do we want to support multiple Init() calls?

@@ +78,5 @@
> +/*
> + * IsRegistered determines if the provided key handle is usable by this token.
> + */
> +bool
> +NSSToken::IsRegistered(const CryptoBuffer& aKeyHandle)

const

@@ +112,5 @@
> +  }
> +
> +  MutexAutoLock lock(mMutex);
> +
> +  if (!mInitialized) {

definitely assert here. MOZ_ASSERT(mInitialized);

@@ +150,5 @@
> +  }
> +
> +  MutexAutoLock lock(mMutex);
> +
> +  if (!mInitialized) {

same here.

::: dom/u2f/U2F.h
@@ +91,5 @@
> +                     const nsAString& aChallenge,
> +                     CryptoBuffer& aClientData) const;
> +
> +  bool
> +  ValidAppID(nsString& aAppId /* in/out */) const;

what do you mean with this in/out? Can you write a better comment?

::: dom/u2f/USBToken.cpp
@@ +21,5 @@
> +{
> +  // This routine does nothing at present, but Bug 1245527 will
> +  // integrate the upcoming USB HID service here, which will likely
> +  // require an initialization upon load.
> +  if (mInitialized) {

MOZ_ASSERT(!mInitialized);

::: netwerk/base/security-prefs.js
@@ +41,5 @@
>  pref("security.OCSP.require", false);
>  pref("security.OCSP.GET.enabled", false);
>  
>  pref("security.pki.cert_short_lifetime_in_days", 10);
>  

#ifndef RELEASE -> true?
otherwise test_interfaces.html will not pass.

	Garrett Robinson [:grobinson]
	Comment 15 • 9 years ago

Comment on attachment 8716532 [details] [diff] [review]
1231681-window.u2f-r6.diff

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

::: dom/webidl/U2F.webidl
@@ +70,5 @@
> +
> +interface U2F {
> +  // These enumerations are defined in the FIDO U2F Javascript API under the
> +  // interface "ErrorCode" as constant integers, and also in the U2F.cpp file.
> +  // Any changes to these must occur in both locations.

Is there any reason not to use the enumeration types provided by WebIDL? [0] Seems like it would help you avoid having to maintain the definitions across two or more locations.

[0]: https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Enumeration_types

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews)
	Comment 16 • 9 years ago

Comment on attachment 8716532 [details] [diff] [review]
1231681-window.u2f-r6.diff

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

Great!

::: dom/u2f/NSSToken.cpp
@@ +6,5 @@
> +
> +#include "NSSToken.h"
> +
> +#include "pk11pub.h"
> +#include "nsNSSComponent.h"

nit: sort these two

@@ +59,5 @@
> +  if (!EnsureNSSInitializedChromeOrContent()) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  mSlot = PK11_GetInternalSlot();

Still need the nsNSSShutDownPreventionLock/isAlreadyShutDown() check here, unfortunately.

@@ +116,5 @@
> +  if (!mInitialized) {
> +    return NS_ERROR_NOT_INITIALIZED;
> +  }
> +
> +  return NS_OK;

Maybe this should be NS_ERROR_NOT_IMPLEMENTED for now? (Or I guess that would prevent the tests from succeeding? I guess that's fine for now.)

@@ +154,5 @@
> +  if (!mInitialized) {
> +    return NS_ERROR_NOT_INITIALIZED;
> +  }
> +
> +  return NS_OK;

Same idea.

::: dom/u2f/U2F.cpp
@@ +320,5 @@
> +                                                       ErrorCode::OTHER_ERROR);
> +      return;
> +    }
> +
> +    CryptoBuffer registrationData, appParam, challengeParam;

These should all be declared one per line, and preferably closer to where they're used:

CryptoBuffer registraitonData;
CryptoBuffer appParam;
CryptoBuffer challengeParam;

@@ +322,5 @@
> +    }
> +
> +    CryptoBuffer registrationData, appParam, challengeParam;
> +    if (!appParam.SetLength(32, fallible) ||
> +        !challengeParam.SetLength(32, fallible)) {

Might be nice to use SHA256_LENGTH from hasht.h here.

@@ +472,5 @@
> +                                               ErrorCode::OTHER_ERROR);
> +      return;
> +    }
> +
> +    // Digest the appId and the clientData

nit: s/clientData/challengeParam/?

@@ +475,5 @@
> +
> +    // Digest the appId and the clientData
> +    SECStatus srv;
> +    nsCString cAppId = NS_ConvertUTF16toUTF8(regKeyAppId);
> +    CryptoBuffer appParam, challengeParam;

nit: declare these on separate lines

	J.C. Jones [:jcj] (he/him) Assignee
	Comment 17 • 9 years ago

[Carrying forward r=keeler, r=baku]

Thank you both again, baku and keeler! I appreciate the lessons here. Soon I won't be a newbie gecko coder!

The only recommendation either of you made that I haven't picked up is this one:

> ::: netwerk/base/security-prefs.js
> @@ +41,5 @@
> >  pref("security.OCSP.require", false);
> >  pref("security.OCSP.GET.enabled", false);
> >  
> >  pref("security.pki.cert_short_lifetime_in_days", 10);
> >  
> 
> #ifndef RELEASE -> true?
> otherwise test_interfaces.html will not pass.

Baku: The semantics we want is for "window.u2f" to be unavailable (for now) unless a user opts-in via the pref. So I think we want security-prefs.js to be set false on all build types. Try runs don't seem to hit errors in test_interfaces.html with the current way it's implemented; maybe they will in release, or am I using the [Pref="security.webauth.u2f"] incorrectly?

Martin: I've done some digging into SecureContext and opened Bug 1247124 to track annotating U2F as [SecureContext]. I'm going to proceed that way since it's not immediately clear to me how to get the document origin into nsContentSecurityManager::IsURIPotentiallyTrustworthy from the right parts of the generated WindowBinding.cpp. I certainly agree  we cannot turn on u2f support without [SecureContext], but I'd like to get this patch landed before it rots. I'm also following up with Richard as to whether to add the SecureContext plumbing to my plate, since someone else asked him about implementing it last night.

There's a try run in progress here:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=81333324f55e

	J.C. Jones [:jcj] (he/him) Assignee
	Comment 18 • 9 years ago

After asking around, I don't believe there's danger in the current pref'd off scenario, so I'm clearing the needinfo for baku and marking checkin-needed. The try run was https://treeherder.mozilla.org/#/jobs?repo=try&revision=81333324f55e&selectedJob=16546738 .

	J.C. Jones [:jcj] (he/him) Assignee
	Comment 19 • 9 years ago

> Comment on attachment 8716532 [details] [diff] [review]
> 1231681-window.u2f-r6.diff
> 
> Review of attachment 8716532 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/U2F.webidl
> @@ +70,5 @@
> > +
> > +interface U2F {
> > +  // These enumerations are defined in the FIDO U2F Javascript API under the
> > +  // interface "ErrorCode" as constant integers, and also in the U2F.cpp file.
> > +  // Any changes to these must occur in both locations.
> 
> Is there any reason not to use the enumeration types provided by WebIDL? [0]
> Seems like it would help you avoid having to maintain the definitions across
> two or more locations.
> 
> [0]:
> https://developer.mozilla.org/en-US/docs/Mozilla/
> WebIDL_bindings#Enumeration_types

For completeness: Garrett and I conversed and decided to keep the WebIDL as integers rather than an enum, in keeping with how the spec is currently written.

(In reply to Garrett Robinson [:grobinson] from comment #15)

	Pulsebot
	Comment 20 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d28a05772db

	Carsten Book [:Tomcat]
	Comment 21 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/6d28a05772db

	Armen [:armenzg]
	Comment 22 • 8 years ago

Is this adding what GitHub needs to use FIDO U2F? I read this when setting up 2-auth.

 Security keys are hardware devices that can be used as your second factor of authentication. When signing in, you press a button on the device rather than typing a verification code. Security keys use the FIDO U2F standard.
This browser doesn’t support the FIDO U2F standard yet. We recommend updating to the latest Google Chrome to start using security key devices.

	J.C. Jones [:jcj] (he/him) Assignee
	Comment 23 • 8 years ago

:armenzg - This adds the interface, but USB support isn't in yet. That's happening in 1245527.