Closed Bug 1244960 Opened 8 years ago Closed 8 years ago

Complete FIDO u2f NSSToken

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: jcj, Assigned: jcj)

References

Details

Attachments

(5 files)

The NSSToken produced in Bug 1065729 requires more work.

It:
  * Needs to use KeyWrap methods on the KeyHandle before returning it
    - Perhaps HMAC with the AppId
  * Avoid direct memory accesses on CryptoBuffers
  * Reuse more code from WebCrypto where possible
  * Possibly include a certificate, if it's needed for sites like https://u2fdemo.appspot.com/
Assignee: nobody → jjones
Depends on: 1231681
- In order to store persistent crypto parameters, the NSSToken had to move
  onto the main thread and be interfaced with via IDL/IPDL.
- Support Register/Sign via NSS using a long-lived secret key
- Rename the softtoken/usbtoken "enable" prefs, because of hierarchy issues
  with the WebIDL Pref shadowing.
- Also orders the includes on nsNSSModule.cpp
- Does not support an attestation certificate, that will be in Part 2.

Review commit: https://reviewboard.mozilla.org/r/37653/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37653/
Attachment #8725825 - Flags: review?(dkeeler)
Comment on attachment 8725825 [details]
MozReview Request: Bug 1244960 - Complete FIDO u2f NSSToken (Part 1) r?keeler r?baku

https://reviewboard.mozilla.org/r/37653/#review34241

This looks good, but I think there are two significant areas that should be improved:
1. documentation for NSS calls - just a bit more about what we're doing and why would be good
2. the way the interface is designed necessitates some manual memory management, which is a bummer. Let's rework it so we don't have to do any of that (one lame suggestion was to just use nsCStrings everywhere - they manage their own memory and you can just .Append to them without worrying if you've allocated enough space, but maybe there's a better abstraction or helper class).

::: dom/ipc/ContentParent.h:748
(Diff revision 1)
> +  virtual bool RecvNSSU2FTokenIsCompatibleVersion(const nsString& version,

A DOM and/or IPC peer should review these.

::: dom/ipc/ContentParent.cpp:4259
(Diff revision 1)
> +  return  NS_SUCCEEDED(rv);

nit: extra space after return

::: dom/ipc/ContentParent.cpp:4323
(Diff revision 1)
> +  memcpy(signature->Elements(), buffer, bufferlen);

Does nsTArray not have a replaceElementsAt or Assign or something?

::: dom/ipc/ContentParent.cpp:4324
(Diff revision 1)
> +  free(buffer);

Manual memory management is a bummer. Can we do something like have the API take a nsCString?

::: dom/u2f/tests/test_frame_register_sign.html:66
(Diff revision 1)
> +  state.keyHandle = bytesToBase64UrlSafe(registrationData.slice(67, 67+keyHandleLength));

nit: spaces around operator +

::: security/manager/ssl/nsNSSModule.cpp:212
(Diff revision 1)
> +NS_NSS_GENERIC_FACTORY_CONSTRUCTOR_INIT(nssEnsureChromeOrContent, nsNSSU2FToken, Init)

This should be nssEnsure, I believe.

::: security/manager/ssl/nsNSSU2FToken.cpp:22
(Diff revision 1)
> +nsNSSU2FToken::mSecretNickname = NS_LITERAL_CSTRING("FIDO_U2F_NSSTOKEN");

nit: I would break this like so:

const nsCString nsNSSU2FToken::mSecretNickname =
  NS_LITERAL_CSTRING("FIDO_U2F_VERSION");

::: security/manager/ssl/nsNSSU2FToken.cpp:75
(Diff revision 1)
> +      PK11SymKey* freeKey = keyList;

nit: indent 2 spaces, not 4

::: security/manager/ssl/nsNSSU2FToken.cpp:109
(Diff revision 1)
> +  if (!EnsureNSSInitializedChromeOrContent()) {

This shouldn't be necessary due to the setup in nsNSSModule.cpp.

::: security/manager/ssl/nsNSSU2FToken.cpp:141
(Diff revision 1)
> +  mWrappingKey = PK11_MoveSymKey(slot.get(), CKA_WRAP | CKA_UNWRAP, 0, PR_TRUE,

Why is this necessary? Let's document what's going on.

::: security/manager/ssl/nsNSSU2FToken.cpp:194
(Diff revision 1)
> +    return nullptr;

Won't this leak wrappedKey?

::: security/manager/ssl/nsNSSU2FToken.cpp:376
(Diff revision 1)
> +  uint32_t bufLen = 1 + kPublicKeyLen + 1 + keyHandleItem->len;

Let's either always use kPublicKeyLen for the length of this or pubKey->u.ec.publicValue.len (that way it's clear that this is memory-safe). Alternatively, re-work the API so we don't have to do any manual allocation/memcpy/etc. and instead can just do something like buf.append(this); buf.append(that); buf.append(the_other_thing);

::: security/manager/ssl/nsNSSU2FToken.cpp:429
(Diff revision 1)
> +  if (!NS_IsMainThread()) {

If all of the public functions are main-thread-only, we don't need mMutex.

::: security/manager/ssl/nsNSSU2FToken.cpp:505
(Diff revision 1)
> +  uint32_t bufLen = 1 + 4 + signatureItem->len;

Let's use counterItem.len here instead (even better - update the API so we can just append the data in the right order without worrying about array bounds, etc.)
Attachment #8725825 - Flags: review?(dkeeler)
Component: DOM: Security → DOM: Device Interfaces
Comment on attachment 8725825 [details]
MozReview Request: Bug 1244960 - Complete FIDO u2f NSSToken (Part 1) r?keeler r?baku

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37653/diff/1-2/
Attachment #8725825 - Attachment description: MozReview Request: Bug 1244960 - Complete FIDO u2f NSSToken (Part 1) r?keeler → MozReview Request: Bug 1244960 - Complete FIDO u2f NSSToken (Part 1) r?keeler r?baku
Attachment #8725825 - Flags: review?(dkeeler)
Attachment #8725825 - Flags: review?(amarchesini)
With Part2 posted, this code now complies with the U2F v1.1 spec sufficiently to pass the tests at https://u2fdemo.appspot.com:

http://i.imgur.com/SoXlYQA.png
Status: NEW → ASSIGNED
Apologies; that screenshot was older; I removed the word "Certificate" from the CN:

http://i.imgur.com/7flJ4vW.png
Comment on attachment 8725825 [details]
MozReview Request: Bug 1244960 - Complete FIDO u2f NSSToken (Part 1) r?keeler r?baku

https://reviewboard.mozilla.org/r/37653/#review37963

This looks good with comments addressed, although I think this is a large enough patch that we want at least one more seceng reviewer.

::: dom/crypto/CryptoBuffer.h:50
(Diff revision 2)
>  
>    nsresult FromJwkBase64(const nsString& aBase64);
>    nsresult ToJwkBase64(nsString& aBase64);
>    bool ToSECItem(PLArenaPool* aArena, SECItem* aItem) const;
>    JSObject* ToUint8Array(JSContext* aCx) const;
> +  bool ToUnsignedBuffer(uint8_t** buf, uint32_t* bufLen) const;

Let's document that this allocates memory (and/or call it ToNewUnsignedBuffer?)

::: dom/u2f/U2F.cpp:131
(Diff revision 2)
> +      return rv;
> +    }
> +
> +    signatureData.Assign(buffer, bufferlen);
> +    free(buffer);
> +    return rv;

This should probably just be "return NS_OK;"

::: dom/u2f/U2F.cpp:166
(Diff revision 2)
> +      return rv;
> +    }
> +
> +    registrationData.Assign(buffer, bufferlen);
> +    free(buffer);
> +    return rv;

Same here.

::: dom/u2f/U2F.cpp:416
(Diff revision 2)
>      }
> +    if (softTokenEnabled) {
> +      rv = SoftTokenIsCompatible(request.mVersion.Value(), &isCompatible);
> +      if (NS_FAILED(rv)) {
> +        SendError<U2FRegisterCallback, RegisterResponse>(aCallback,
> +                                                        ErrorCode::OTHER_ERROR);

nit: indentation

::: dom/u2f/U2F.cpp:423
(Diff revision 2)
> +      }
>  
> -    if (softTokenEnabled &&
> -        mSoftToken.IsCompatibleVersion(request.mVersion.Value()) &&
> -        mSoftToken.IsRegistered(keyHandle)) {
> +      rv = SoftTokenIsRegistered(keyHandle, &isRegistered);
> +      if (NS_FAILED(rv)) {
> +        SendError<U2FRegisterCallback, RegisterResponse>(aCallback,
> +                                                        ErrorCode::OTHER_ERROR);

nit: indentation

::: security/manager/ssl/nsNSSU2FToken.cpp:30
(Diff revision 2)
> +
> +const uint32_t kParamLen = 32;
> +const uint32_t kPublicKeyLen = 65;
> +const uint32_t kWrappedKeyBufLen = 256;
> +const uint32_t kSignedDataLen = (2 * kParamLen) + 1 + 4;
> +const uint32_t kWrappingKeyBits = 128/8;

I think 'kWrappingKeyBytes' would be a better name for this (or kWrappingKeyByteLen?)

::: security/manager/ssl/nsNSSU2FToken.cpp:33
(Diff revision 2)
> +const uint32_t kWrappedKeyBufLen = 256;
> +const uint32_t kSignedDataLen = (2 * kParamLen) + 1 + 4;
> +const uint32_t kWrappingKeyBits = 128/8;
> +const uint32_t kSigKeyUsageCount = 2;
> +
> +static mozilla::LazyLogModule gNssTokenLog("fido_u2f");

nit: let's call this gNSSTokenLog

::: security/manager/ssl/nsNSSU2FToken.cpp:96
(Diff revision 2)
> +NS_IMETHODIMP nsNSSU2FToken::Init()
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(!mInitialized);
> +  if (mInitialized) {
> +    return NS_OK;

Seems like this should be NS_ERROR_FAILURE

::: security/manager/ssl/nsNSSU2FToken.cpp:136
(Diff revision 2)
> +
> +  // Now that the wrapping key is generated, we need to persist it to the NSS
> +  // Key Database, so we "move" it from the "session" to the "token" store
> +  mWrappingKey = PK11_MoveSymKey(slot.get(), CKA_WRAP | CKA_UNWRAP, 0,
> +                                 /* Persist permanently */ PR_TRUE, key.get());
> +  if (NS_WARN_IF(!mWrappingKey)) {

We don't really need NS_WARN_IF if we have logging.

::: security/manager/ssl/nsNSSU2FToken.cpp:153
(Diff revision 2)
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  MOZ_LOG(gNssTokenLog, LogLevel::Debug,
> +          ("Key nickname set to %s. FIDO U2F NSS Token initialized.",
> +           PK11_GetSymKeyNickname(mWrappingKey)));

This leaks - the data returned needs to be PORT_Free'd, it looks like

::: security/manager/ssl/nsNSSU2FToken.cpp:161
(Diff revision 2)
> +  mInitialized = true;
> +  return NS_OK;
> +}
> +
> +/*
> + * Convert a Private Key object into an opaque key handle, using AES Key Wrap

Let's stick to //-style comments (except for where they're inline, I guess)

::: security/manager/ssl/nsNSSU2FToken.cpp:198
(Diff revision 2)
> +/*
> + * Convert a opaque key handle aKeyHandle back into a Private Key object, using
> + * aWrappingKey and the AES Key Wrap algorithm.
> + */
> +static SECKEYPrivateKey*
> +PrivateKeyFromKeyHandle(PK11SlotInfo* aSlot,

It looks like every time this is called the code has to first construct a SECItem out of a uint8_t pointer and a length. Why not just factor that out and have this method take the uint8_t* and length directly?

::: security/manager/ssl/nsNSSU2FToken.cpp:228
(Diff revision 2)
> +
> +  return unwrappedKey;
> +}
> +
> +/* void isCompatibleVersion (in AString version, [retval] out boolean result); */
> +NS_IMETHODIMP nsNSSU2FToken::IsCompatibleVersion(const nsAString& aVersion,

NS_IMETHODIMP on its own line here and elsewhere
Attachment #8725825 - Flags: review?(dkeeler) → review+
https://reviewboard.mozilla.org/r/37653/#review34241

> A DOM and/or IPC peer should review these.

Agreed. I'll pull in Baku again.

> Manual memory management is a bummer. Can we do something like have the API take a nsCString?

It doesn't look like we can; we need unsigned values, and short of using Base64 strings to trasnfer in/out, I don't think there's a better option in the [XPIDL](https://developer.mozilla.org/en-US/docs/Mozilla/XPIDL).
https://reviewboard.mozilla.org/r/37653/#review37963

Thanks again, Keeler. Thinking about it last night, there's no reason for me to store the (potentially-trackable) U2F attestation certificate persistently; we can just generate a new one on each call to Register. Sure, it's not efficient, but it's untrackable and it's not currently envisioned as production code anyway. The protocol lets us get away with it.

I may do that first before pulling in another SecEng reviewer.
https://reviewboard.mozilla.org/r/37653/#review37963

Update: Now using ephemeral certs, genereated each Register.

> Same here.

Thanks.

> I think 'kWrappingKeyBytes' would be a better name for this (or kWrappingKeyByteLen?)

Whoops. Making it `kWrappingKeyByteLen`

> Seems like this should be NS_ERROR_FAILURE

That seems like a good idea. Good catch, thanks!

> We don't really need NS_WARN_IF if we have logging.

Got it; I cleaned up all the files with this anti-pattern.

> This leaks - the data returned needs to be PORT_Free'd, it looks like

There was another one; got them both. Thanks!

> It looks like every time this is called the code has to first construct a SECItem out of a uint8_t pointer and a length. Why not just factor that out and have this method take the uint8_t* and length directly?

Good idea!
Comment on attachment 8725825 [details]
MozReview Request: Bug 1244960 - Complete FIDO u2f NSSToken (Part 1) r?keeler r?baku

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37653/diff/2-3/
Comment on attachment 8732435 [details]
MozReview Request: Bug 1244960 - FIDO u2f NSSToken (Part 2): Use Attestation Certificates r?keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41171/diff/1-2/
Attachment #8732435 - Attachment description: MozReview Request: Bug 1244960 - Complete FIDO u2f NSSToken (Part 2) r?keeler → MozReview Request: Bug 1244960 - FIDO u2f NSSToken: Use Attestation Certificates (Part 2) r?keeler
Attachment #8725825 - Flags: review?(rlb)
Attachment #8732435 - Flags: review?(rlb)
Comment on attachment 8725825 [details]
MozReview Request: Bug 1244960 - Complete FIDO u2f NSSToken (Part 1) r?keeler r?baku

https://reviewboard.mozilla.org/r/37653/#review38301

::: security/manager/ssl/nsNSSU2FToken.cpp:76
(Diff revisions 2 - 3)
>    PK11SymKey* keyList;
>    keyList = PK11_ListFixedKeysInSlot(aSlot, const_cast<char*>(aNickname.get()),
>                                       /* wincx */ nullptr);
>    while (keyList) {
>      PK11SymKey* freeKey = keyList;
> -    if (aNickname == PK11_GetSymKeyNickname(freeKey)) {
> +    char* freeKeyName = PK11_GetSymKeyNickname(freeKey);

This is a nice way to handle this, but no big deal: https://dxr.mozilla.org/mozilla-central/rev/ea6298e1b4f7e22ce2311b2b6a918822f0adb112/security/manager/ssl/nsNSSCertificate.cpp#558
Attachment #8725825 - Flags: review+
Comment on attachment 8725825 [details]
MozReview Request: Bug 1244960 - Complete FIDO u2f NSSToken (Part 1) r?keeler r?baku

I guess commenting on patches without changing the review status isn't a thing, reviewboard?
Attachment #8725825 - Flags: review+
Comment on attachment 8725825 [details]
MozReview Request: Bug 1244960 - Complete FIDO u2f NSSToken (Part 1) r?keeler r?baku

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37653/diff/3-4/
Attachment #8725825 - Flags: review?(rlb)
Attachment #8725825 - Flags: review?(dkeeler)
Attachment #8725825 - Flags: review+
Comment on attachment 8732435 [details]
MozReview Request: Bug 1244960 - FIDO u2f NSSToken (Part 2): Use Attestation Certificates r?keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41171/diff/2-3/
Attachment #8732435 - Flags: review?(rlb)
Comment on attachment 8725825 [details]
MozReview Request: Bug 1244960 - Complete FIDO u2f NSSToken (Part 1) r?keeler r?baku

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37653/diff/4-5/
Attachment #8725825 - Attachment description: MozReview Request: Bug 1244960 - Complete FIDO u2f NSSToken (Part 1) r?keeler r?baku → MozReview Request: Bug 1244960 - Complete FIDO u2f NSSToken (Part 1) r?keeler r?baku r?rbarnes
Attachment #8725825 - Flags: review?(rlb)
Comment on attachment 8732435 [details]
MozReview Request: Bug 1244960 - FIDO u2f NSSToken (Part 2): Use Attestation Certificates r?keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41171/diff/3-4/
Attachment #8732435 - Attachment description: MozReview Request: Bug 1244960 - FIDO u2f NSSToken: Use Attestation Certificates (Part 2) r?keeler → MozReview Request: Bug 1244960 - FIDO u2f NSSToken (Part 2): Use Attestation Certificates r?keeler r?rbarnes
Attachment #8732435 - Flags: review?(rlb)
Attachment #8725825 - Flags: review?(dkeeler) → review+
Attachment #8732435 - Flags: review?(dkeeler) → review+
Comment on attachment 8732435 [details]
MozReview Request: Bug 1244960 - FIDO u2f NSSToken (Part 2): Use Attestation Certificates r?keeler

https://reviewboard.mozilla.org/r/41171/#review38503

Is the attestation certificate required by the spec? Since we don't have hardware storage for a shared private key, it's not clear that having it is particularly useful in this case. Otherwise, looks good to me.

::: dom/u2f/tests/test_frame_register_sign.html:71
(Diff revision 4)
>      state.keyHandle = bytesToBase64UrlSafe(registrationData.slice(67, 67 + keyHandleLength));
>      state.attestation = registrationData.slice(67 + keyHandleLength);
>  
> +    local_is(state.attestation[0], 0x30, "Attestation Certificate has correct starting byte");
> +    var attestationCertLen = getAsn1Length(state.attestation.slice(1));
> +    state.attestationCert = state.attestation.slice(1, attestationCertLen);

Doesn't this also need to take into account the length of the length field itself? (i.e. what we have is <tag: 1 byte><length: n bytes><data: length bytes>. I think state.attestationCert will result in <length: n bytes><data: length - n bytes> whereas we want <data: length bytes>)

::: dom/u2f/tests/test_frame_register_sign.html:73
(Diff revision 4)
>  
> +    local_is(state.attestation[0], 0x30, "Attestation Certificate has correct starting byte");
> +    var attestationCertLen = getAsn1Length(state.attestation.slice(1));
> +    state.attestationCert = state.attestation.slice(1, attestationCertLen);
> +    state.attestationSig = state.attestation.slice(attestationCertLen);
> +    local_ok(state.attestationCert.length > 0, "Attestation Certificate has length");

We should maybe consider either implementing or importing a js library to decode certificates here. That way, we can verify that a) we're correctly generating the attestation cert and b) that the signature is correct.

::: dom/u2f/tests/u2futil.js:146
(Diff revision 4)
>  
>    var alg = {name: "ECDSA", hash: "SHA-256"};
>    return crypto.subtle.verify(alg, key, sig, data);
>  }
> +
> +function getAsn1Length(data) {

Let's call this getASN1Length (for whatever reason I'm really not a fan of lower-casing acronyms in identifiers)

::: dom/u2f/tests/u2futil.js:148
(Diff revision 4)
>    return crypto.subtle.verify(alg, key, sig, data);
>  }
> +
> +function getAsn1Length(data) {
> +  var first = data[0] & 0x7F;
> +  if (first == data[0])

nit: braces

::: dom/u2f/tests/u2futil.js:154
(Diff revision 4)
> +      return first;
> +  if (first === 0)
> +      return null; // invalid tag length!
> +  var length = 0;
> +  for (var idx = 1; idx < first + 1; ++idx)
> +      length = (length << 8) + data[idx];

This can overflow fairly easily. I realize we're only using it to verify our own implementation, but maybe we should limit ASN.1 lengths to 16k or something.

::: security/manager/ssl/nsNSSU2FToken.cpp:252
(Diff revision 4)
> +    MOZ_LOG(gNSSTokenLog, LogLevel::Warning,
> +            ("Failed to gen validity, NSS error #%d", PORT_GetError()));
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  unsigned long serial;

We should guarantee that the serial is positive (similar to this: https://dxr.mozilla.org/mozilla-central/rev/3381aa98edf72e02b9d6b4db6efa0865063a2329/security/manager/ssl/tests/unit/pycert.py#383 )

::: security/manager/ssl/nsNSSU2FToken.cpp:254
(Diff revision 4)
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  unsigned long serial;
> +  SECStatus srv = PK11_GenerateRandomOnSlot(aSlot,
> +                                     reinterpret_cast<unsigned char *>(&serial),

nit: indentation
For awkward breaks like this, it's acceptable to do something like this:
SECStatus srv = PK11_GenerateRandomOnSlot(
  aSlot, reinterpret_cast<unsigned char *>(&serial), sizeof(serial));

::: security/manager/ssl/nsNSSU2FToken.cpp:290
(Diff revision 4)
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  SECItem *signedCert = PORT_ArenaZNew(arena, SECItem);
> +  if (!signedCert) {
> +    return NS_ERROR_DOM_UNKNOWN_ERR;

Why return a different error here?
Comment on attachment 8725825 [details]
MozReview Request: Bug 1244960 - Complete FIDO u2f NSSToken (Part 1) r?keeler r?baku

https://reviewboard.mozilla.org/r/37653/#review38585

::: dom/crypto/CryptoBuffer.cpp:195
(Diff revision 5)
>  {
>    return Uint8Array::Create(aCx, Length(), Elements());
>  }
>  
> +bool
> +CryptoBuffer::ToNewUnsignedBuffer(uint8_t** buf, uint32_t* bufLen) const

In light of comments below, this method should be unnecessary.

::: dom/u2f/U2F.cpp:42
(Diff revision 5)
>  
> -#define PREF_U2F_SOFTTOKEN_ENABLED "security.webauth.u2f.softtoken"
> -#define PREF_U2F_USBTOKEN_ENABLED  "security.webauth.u2f.usbtoken"
> +#define PREF_U2F_SOFTTOKEN_ENABLED "security.webauth.u2f_enable_softtoken"
> +#define PREF_U2F_USBTOKEN_ENABLED  "security.webauth.u2f_enable_usbtoken"
>  
> -const nsString
> -U2F::FinishEnrollment = NS_LITERAL_STRING("navigator.id.finishEnrollment");
> +const nsString U2F::FinishEnrollment =
> +  NS_LITERAL_STRING("navigator.id.finishEnrollment");

Nit: Can this not fit on one line?

::: dom/u2f/U2F.cpp:111
(Diff revision 5)
>    }
> +}
>  
> -  aRv = mUSBToken.Init();
> -  if (NS_WARN_IF(aRv.Failed())) {
> -    return;
> +
> +static nsresult
> +SoftTokenSign(CryptoBuffer& keyHandle, CryptoBuffer& application,

Nit: I would put Register() before Sign() in here, just because that's the order they're used in.

Nit: NSSTokenSign/Register?  Softoken is a registered trademark of NSS.

::: dom/u2f/U2F.cpp:117
(Diff revision 5)
> +              CryptoBuffer& challenge, CryptoBuffer& signatureData)
> +{
> +  if (XRE_IsParentProcess()) {
> +    uint8_t* buffer;
> +    uint32_t bufferlen;
> +    nsCOMPtr<nsINSSU2FToken> nssToken(do_GetService(NS_NSSU2FTOKEN_CONTRACTID));

Wouldn't it be better to load this service when you initialize the object, instead of on every call?

Actually do you even need this branch?  It looks like this call is being handled in RecvNSSU2FTokenSign.  Maybe you can just assert(XRE_IsParentProcess()) here.  (Likewise below.)

::: dom/u2f/U2F.cpp:134
(Diff revision 5)
> +    signatureData.Assign(buffer, bufferlen);
> +    free(buffer);
> +    return NS_OK;
> +  }
> +
> +  InfallibleTArray<uint8_t> signatureBuffer;

Are you sure this is OK?  ISTR CryptoBuffer is fallible because bad things happen with Infallible when you hit OOM.

::: dom/u2f/tests/test_frame_register_sign.html:92
(Diff revision 5)
> +      local_ok(false, "Imported public key");
> +      local_finished();
> +    });
> +  }
> +
> +  function testSigning() {

Rename; this is not testing signing.

::: dom/u2f/tests/test_frame_register_sign.html:98
(Diff revision 5)
> +    state.regKey = {
> +      version: state.version,
> +      keyHandle: state.keyHandle,
> +    };
> +
> +    // Test that we don't re-register

Is it actually required that a second register() request for the same origin will fail?  That seems like a serious bug, not a feature.  What if you have multiple accounts?  How do you ever clear this state?

If this behavior is going to be present, I want to see the spec text that justifies it, and we need to address the above issues.  (And if we can't address those issues, I suggest we just ignore the spec.)  Otherwise, just remove this behavior.

::: dom/u2f/tests/test_frame_register_sign.html:102
(Diff revision 5)
> +
> +    // Test that we don't re-register
> +    u2f.register(state.appId, [state.regRequest], [state.regKey], reRegisterCb);
> +  }
> +
> +  function reRegisterCb(regResponse) {

I find the way these functions are laid out really confusing and unreadable.  I would prefer something like:

function testRegister() {
  u2f.register(..., reg => {
    // test properties of the reg response
  })
}

function testSign() {
  function verifySign(sign) {
    // test properties of the sign response
  }

  u2f.register(..., reg => {
    u2f.sign(..., verifySign);
  });
}

This of course runs into issues with re-registration, but as above, that's almost certainly a bug.

::: security/manager/ssl/nsINSSU2FToken.idl:15
(Diff revision 5)
> + */
> +[scriptable, uuid(d9104a00-140b-4f86-a4b0-4998878ef4e6 )]
> +interface nsINSSU2FToken : nsISupports {
> +  /**
> +   * Initializes the token and constructs and persists keys, if needed. Should
> +   * only called by the main thread.

Should probably assert / throw / return an error if called off the main thread?

::: security/manager/ssl/nsNSSU2FToken.cpp:26
(Diff revision 5)
> +
> +const nsCString nsNSSU2FToken::mSecretNickname =
> +  NS_LITERAL_CSTRING("FIDO_U2F_NSSTOKEN");
> +const nsString nsNSSU2FToken::mVersion = NS_LITERAL_STRING("U2F_V2");
> +
> +const uint32_t kParamLen = 32;

This would be a good place to explain the crypto that's being used, i.e.

* ECDSA for key pairs (as required by spec)
* AES-KW for key wrapping (with what key size?)

That will help people understand why the values of these parameters are what they are.

::: security/manager/ssl/nsNSSU2FToken.cpp:75
(Diff revision 5)
> +
> +  PK11SymKey* keyList;
> +  keyList = PK11_ListFixedKeysInSlot(aSlot, const_cast<char*>(aNickname.get()),
> +                                     /* wincx */ nullptr);
> +  while (keyList) {
> +    PK11SymKey* freeKey = keyList;

This seems like a good use for ScopedPK11SymKey.

::: security/manager/ssl/nsNSSU2FToken.cpp:81
(Diff revision 5)
> +
> +    UniquePtr<char, void(&)(void*)>
> +      freeKeyName(PK11_GetSymKeyNickname(freeKey), PORT_Free);
> +
> +    if (aNickname == freeKeyName.get()) {
> +      MOZ_LOG(gNSSTokenLog, LogLevel::Debug, ("Symmetric key found!"));

A symmetric "Key not found" log message would probably be useful.

::: security/manager/ssl/nsNSSU2FToken.cpp:95
(Diff revision 5)
> +
> +static nsresult
> +GenEcKeypair(PK11SlotInfo* aSlot, ScopedSECKEYPrivateKey& aPrivKey,
> +             ScopedSECKEYPublicKey& aPubKey, const nsNSSShutDownPreventionLock&)
> +{
> +  // Set the curve parameters

Save yourself some work and just use CreateECParamsForCurve from WebCrypto.

https://dxr.mozilla.org/mozilla-central/source/dom/crypto/WebCryptoCommon.h#306

::: security/manager/ssl/nsNSSU2FToken.cpp:157
(Diff revision 5)
> +
> +  MOZ_LOG(gNSSTokenLog, LogLevel::Info,
> +          ("No keys found. Generating new U2F Soft Token wrapping key."));
> +
> +  // We did not find an existing wrapping key, so we generate one.
> +  ScopedPK11SymKey key(PK11_KeyGen(aSlot, CKM_AES_KEY_GEN,

It seems like using PK11_TokenKeyGen would save you the PK11_MoveSymKey step.

::: security/manager/ssl/nsNSSU2FToken.cpp:232
(Diff revision 5)
> +                        SECKEYPrivateKey* aPrivKey,
> +                        const nsNSSShutDownPreventionLock&)
> +{
> +  ScopedSECItem param(PK11_ParamFromIV(CKM_NSS_AES_KEY_WRAP_PAD,
> +                                       /* default IV */ nullptr ));
> +  ScopedSECItem wrappedKey(SECITEM_AllocItem(/* default arena */ nullptr,

It seems like it would be a worthwhile for us to prevent use of the same key handle across origins, i.e., to prevent the same key being used to sign assertions for two origins.  We could do that including the application parameter under the wrapping, and checking it on unwrap.

That might not need to be done here, but would be a good follow-up.

::: security/manager/ssl/nsNSSU2FToken.cpp:266
(Diff revision 5)
> +
> +  ScopedAutoSECItem keyHandleItem(aKeyHandleLen);
> +  memcpy(keyHandleItem.data, aKeyHandle, keyHandleItem.len);
> +
> +  CK_ATTRIBUTE_TYPE usages[] = { CKA_SIGN };
> +  ScopedSECItem param(PK11_ParamFromIV(CKM_NSS_AES_KEY_WRAP_PAD,

If you just leave param as NULL, then PK11_UnwrapPrivKey will do this for you.

https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/pk11wrap/pk11obj.c?from=PK11_UnwrapPrivKey#1198

Likewise for wrapping.

::: security/manager/ssl/nsNSSU2FToken.cpp:325
(Diff revision 5)
> +
> +  ScopedPK11SlotInfo slot(PK11_GetInternalSlot());
> +  MOZ_ASSERT(slot.get());
> +
> +  // Decode the key handle
> +  ScopedSECKEYPrivateKey privKey(PrivateKeyFromKeyHandle(slot.get(),

As above, it would be preferable to include the appParam here, so that we can check that the key was registered for the origin in question.

::: security/manager/ssl/nsNSSU2FToken.cpp:346
(Diff revision 5)
> +// The KeyHandleFromPrivateKey and PrivateKeyFromKeyHandle methods perform
> +// the actual key wrap/unwrap operations.
> +//
> +// The format of the return registration data is as follows:
> +//
> +// Bytes  Value

Nit: Please left- or right-align the values in the left column.

::: security/manager/ssl/nsNSSU2FToken.cpp:407
(Diff revision 5)
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  // Serialize the registration data
> +  mozilla::dom::CryptoBuffer registrationBuf;
> +  if (!registrationBuf.AppendElement(0x05, mozilla::fallible)) {

Don't bother futzing around with CryptoBuffer here.  You know the size of the output buffer.  Just allocate aRegistration to the size needed and write to it.

::: security/manager/ssl/nsNSSU2FToken.cpp:504
(Diff revision 5)
> +  counterItem.data[1] = (counter >> 16) & 0xFF;
> +  counterItem.data[2] = (counter >>  8) & 0xFF;
> +  counterItem.data[3] = (counter >>  0) & 0xFF;
> +
> +  // Compute the signature
> +  mozilla::dom::CryptoBuffer signedDataBuf;

Again, don't do all the CryptoBuffer stuff, just allocate a fixed-size buffer and write to it.

::: security/manager/ssl/nsNSSU2FToken.cpp:537
(Diff revision 5)
> +            ("Signature failure: %d", PORT_GetError()));
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  // Assmeble the signature data into a buffer for return
> +  mozilla::dom::CryptoBuffer signatureBuf;

CryptoBuffer again.
Attachment #8725825 - Flags: review?(rlb)
Blocks: 1260318
https://reviewboard.mozilla.org/r/37653/#review38585

> In light of comments below, this method should be unnecessary.

After some offline discussion, we've agreed to keep the CryptoBuffer methods but to allocate up-front where possible.

> Nit: Can this not fit on one line?

Nope. :(

> Nit: I would put Register() before Sign() in here, just because that's the order they're used in.
> 
> Nit: NSSTokenSign/Register?  Softoken is a registered trademark of NSS.

Alright; reoredered and renamed to keep from violating the trademark of the great NSS Republic. :)

> Wouldn't it be better to load this service when you initialize the object, instead of on every call?
> 
> Actually do you even need this branch?  It looks like this call is being handled in RecvNSSU2FTokenSign.  Maybe you can just assert(XRE_IsParentProcess()) here.  (Likewise below.)

We definitely need the branches, to differentiate between being on the parent thread or not.

I've refactored the static methods into private ones and pulled nssToken into a single member variable initialzied iff `XRE_IsParentProcess` is true during `Init`.

> Are you sure this is OK?  ISTR CryptoBuffer is fallible because bad things happen with Infallible when you hit OOM.

I can't make them `FallibleTArray` objects as the contract requires infallible `nsTArray` objects; this appears to be the convention for this kind of IPC. The good news is that the machine-generated code that fills in this array, `PContentChild::Read`, uses an InfallibleTArray and a call to `SwapElements` to ensure that there won't be a runtime allocation failure at the InfallibleTArray during IPC; it'll happen earlier... and FatalError.

I went ahead and redeclared these as type `nsTArray` (which is a define of `InfallibleTArray`) just to be clearer.

> Is it actually required that a second register() request for the same origin will fail?  That seems like a serious bug, not a feature.  What if you have multiple accounts?  How do you ever clear this state?
> 
> If this behavior is going to be present, I want to see the spec text that justifies it, and we need to address the above issues.  (And if we can't address those issues, I suggest we just ignore the spec.)  Otherwise, just remove this behavior.

This is not a bug, this is a feature. I added the comment above the duplicate call to `u2f.register`:
```
    // Test that we don't re-register if we provide regKey as an
    // "already known" key handle. The U2F module should recognize regKey
    // as being usable and, thus, give back errorCode 4.
    u2f.register(state.appId, [state.regRequest], [state.regKey], reRegisterCb);
```

For those following along, the analogous feature in v1.0 is in Section 4.1.1 of the JS API: https://fidoalliance.org/specs/fido-u2f-v1.0-nfc-bt-amendment-20150514/fido-u2f-javascript-api.html#dictionary-registerrequest-members

> I find the way these functions are laid out really confusing and unreadable.  I would prefer something like:
> 
> function testRegister() {
>   u2f.register(..., reg => {
>     // test properties of the reg response
>   })
> }
> 
> function testSign() {
>   function verifySign(sign) {
>     // test properties of the sign response
>   }
> 
>   u2f.register(..., reg => {
>     u2f.sign(..., verifySign);
>   });
> }
> 
> This of course runs into issues with re-registration, but as above, that's almost certainly a bug.

Thanks; refactored.

> Should probably assert / throw / return an error if called off the main thread?

Updated comment, as it already asserts.

> This would be a good place to explain the crypto that's being used, i.e.
> 
> * ECDSA for key pairs (as required by spec)
> * AES-KW for key wrapping (with what key size?)
> 
> That will help people understand why the values of these parameters are what they are.

Added:
```
// This U2F-compatible soft token uses FIDO U2F-compatible ECDSA keypairs
// on the SEC_OID_SECG_EC_SECP256R1 curve. When asked to Register, it will
// generate and return a new keypair KP, where the private component is wrapped
// using AES-KW with the 128-bit mWrappingKey to make an opaque "key handle".
// In other words, Register yields { KP_pub, AES-KW(KP_priv, key=mWrappingKey) }
//
// The value mWrappingKey is long-lived; it is persisted as part of the NSS DB
// for the current profile. The attestation certificates that are produced are
// ephemeral to counteract profiling. They have little use for a soft-token
// at any rate, but are required by the specification.
```

> Save yourself some work and just use CreateECParamsForCurve from WebCrypto.
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/crypto/WebCryptoCommon.h#306

Oh nice; thanks!

I made a new constant up-top for the name:
`NS_NAMED_LITERAL_STRING(kEcAlgorithm, WEBCRYPTO_NAMED_CURVE_P256);`

> It seems like using PK11_TokenKeyGen would save you the PK11_MoveSymKey step.

Indeed; thanks for bringing this up. Once I eventually figured out the correct flags, I intended to go back to straight Token generation.

> It seems like it would be a worthwhile for us to prevent use of the same key handle across origins, i.e., to prevent the same key being used to sign assertions for two origins.  We could do that including the application parameter under the wrapping, and checking it on unwrap.
> 
> That might not need to be done here, but would be a good follow-up.

Agreed. Opened Bug 1260318.

> As above, it would be preferable to include the appParam here, so that we can check that the key was registered for the origin in question.

Also covered in Bug 1260318.

> Nit: Please left- or right-align the values in the left column.

:+1:

> Don't bother futzing around with CryptoBuffer here.  You know the size of the output buffer.  Just allocate aRegistration to the size needed and write to it.

As discussed above, changed this to a pre-allocated CryptoBuffer.

> Again, don't do all the CryptoBuffer stuff, just allocate a fixed-size buffer and write to it.

Changed to pre-allocate.

> CryptoBuffer again.

Changed to pre-allocate.
https://reviewboard.mozilla.org/r/41171/#review38503

It's required as part of the message; the various real u2f test tools all bail if there's not an x509 cert.

> Doesn't this also need to take into account the length of the length field itself? (i.e. what we have is <tag: 1 byte><length: n bytes><data: length bytes>. I think state.attestationCert will result in <length: n bytes><data: length - n bytes> whereas we want <data: length bytes>)

It does, you're right. And so does the line after it.

> This can overflow fairly easily. I realize we're only using it to verify our own implementation, but maybe we should limit ASN.1 lengths to 16k or something.

Fair enough. Added bounds checking!

> Why return a different error here?

Heh, merge error. thanks!
Comment on attachment 8725825 [details]
MozReview Request: Bug 1244960 - Complete FIDO u2f NSSToken (Part 1) r?keeler r?baku

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37653/diff/5-6/
Attachment #8725825 - Flags: review?(rlb)
Comment on attachment 8732435 [details]
MozReview Request: Bug 1244960 - FIDO u2f NSSToken (Part 2): Use Attestation Certificates r?keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41171/diff/4-5/
https://reviewboard.mozilla.org/r/37653/#review42151

Fix a bug exposed on try where providing `PK11_UnwrapPrivKey` a `nullptr` for the `param` parameter to indicate "default IV" leaks an allocation (as the callee allocates in that case), and remove r?rbarnes per his request.
Comment on attachment 8725825 [details]
MozReview Request: Bug 1244960 - Complete FIDO u2f NSSToken (Part 1) r?keeler r?baku

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37653/diff/6-7/
Attachment #8725825 - Attachment description: MozReview Request: Bug 1244960 - Complete FIDO u2f NSSToken (Part 1) r?keeler r?baku r?rbarnes → MozReview Request: Bug 1244960 - Complete FIDO u2f NSSToken (Part 1) r?keeler r?baku
Attachment #8732435 - Attachment description: MozReview Request: Bug 1244960 - FIDO u2f NSSToken (Part 2): Use Attestation Certificates r?keeler r?rbarnes → MozReview Request: Bug 1244960 - FIDO u2f NSSToken (Part 2): Use Attestation Certificates r?keeler
Attachment #8725825 - Flags: review?(rlb)
Attachment #8732435 - Flags: review?(rlb)
Comment on attachment 8732435 [details]
MozReview Request: Bug 1244960 - FIDO u2f NSSToken (Part 2): Use Attestation Certificates r?keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41171/diff/5-6/
Comment on attachment 8725825 [details]
MozReview Request: Bug 1244960 - Complete FIDO u2f NSSToken (Part 1) r?keeler r?baku

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37653/diff/7-8/
Comment on attachment 8732435 [details]
MozReview Request: Bug 1244960 - FIDO u2f NSSToken (Part 2): Use Attestation Certificates r?keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41171/diff/6-7/
Since this no longer mergeed cleanly, I've rebased. Re-run try from the rebase is clean: https://treeherder.mozilla.org/#/jobs?repo=try&revision=57a6ef332a98
Comment on attachment 8725825 [details]
MozReview Request: Bug 1244960 - Complete FIDO u2f NSSToken (Part 1) r?keeler r?baku

https://reviewboard.mozilla.org/r/37653/#review37861

::: dom/crypto/CryptoBuffer.h:50
(Diff revision 2)
>  
>    nsresult FromJwkBase64(const nsString& aBase64);
>    nsresult ToJwkBase64(nsString& aBase64);
>    bool ToSECItem(PLArenaPool* aArena, SECItem* aItem) const;
>    JSObject* ToUint8Array(JSContext* aCx) const;
> +  bool ToUnsignedBuffer(uint8_t** buf, uint32_t* bufLen) const;

aBuf and aBufLen

::: dom/crypto/CryptoBuffer.cpp:195
(Diff revision 2)
>  {
>    return Uint8Array::Create(aCx, Length(), Elements());
>  }
>  
> +bool
> +CryptoBuffer::ToUnsignedBuffer(uint8_t** buf, uint32_t* bufLen) const

aBuf, aBufLen

::: dom/crypto/CryptoBuffer.cpp:196
(Diff revision 2)
>    return Uint8Array::Create(aCx, Length(), Elements());
>  }
>  
> +bool
> +CryptoBuffer::ToUnsignedBuffer(uint8_t** buf, uint32_t* bufLen) const
> +{

MOZ_ASSERT(aBuf);
MOZ_ASSERT(aBufLen);

::: dom/crypto/CryptoBuffer.cpp:197
(Diff revision 2)
>  }
>  
> +bool
> +CryptoBuffer::ToUnsignedBuffer(uint8_t** buf, uint32_t* bufLen) const
> +{
> +  uint8_t* tmp = reinterpret_cast<uint8_t*>(moz_xmalloc(Length()));

You use Length() 3 times in this method.
Can you store it in a variable instead?

::: dom/ipc/ContentParent.h:752
(Diff revision 2)
>    DeallocPCrashReporterParent(PCrashReporterParent* crashreporter) override;
>  
>    virtual bool RecvGetRandomValues(const uint32_t& length,
>                                     InfallibleTArray<uint8_t>* randomValues) override;
>  
> +  virtual bool RecvNSSU2FTokenIsCompatibleVersion(const nsString& version,

aVersion, aIsCompatible, and all the rest too.

::: dom/ipc/ContentParent.cpp:4273
(Diff revision 2)
>  
>    return true;
>  }
>  
>  bool
> +ContentParent::RecvNSSU2FTokenIsCompatibleVersion(const nsString& version,

aVersion, ...

::: dom/crypto/CryptoBuffer.cpp:195
(Diff revision 6)
>  {
>    return Uint8Array::Create(aCx, Length(), Elements());
>  }
>  
> +bool
> +CryptoBuffer::ToNewUnsignedBuffer(uint8_t** buf, uint32_t* bufLen) const

aBuf, aBufLen

::: dom/crypto/CryptoBuffer.cpp:198
(Diff revision 6)
>  
> +bool
> +CryptoBuffer::ToNewUnsignedBuffer(uint8_t** buf, uint32_t* bufLen) const
> +{
> +  uint8_t* tmp = reinterpret_cast<uint8_t*>(moz_xmalloc(Length()));
> +  if (!tmp) {

NS_WARN_IF

::: dom/ipc/ContentParent.h:752
(Diff revision 6)
>    DeallocPCrashReporterParent(PCrashReporterParent* crashreporter) override;
>  
>    virtual bool RecvGetRandomValues(const uint32_t& length,
>                                     InfallibleTArray<uint8_t>* randomValues) override;
>  
> +  virtual bool RecvNSSU2FTokenIsCompatibleVersion(const nsString& version,

aVersion, aIsCompatible, and so on...

::: dom/ipc/ContentParent.cpp:4273
(Diff revision 6)
>  
>    return true;
>  }
>  
>  bool
> +ContentParent::RecvNSSU2FTokenIsCompatibleVersion(const nsString& version,

aVersion, aIsCompatible, and so on

::: dom/ipc/ContentParent.cpp:4316
(Diff revision 6)
> +                                   challenge.Elements(), challenge.Length(),
> +                                   &buffer, &bufferlen);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return false;
> +  }
> +

MOZ_ASSERT(buffer);

::: dom/ipc/ContentParent.cpp:4341
(Diff revision 6)
> +                               keyHandle.Elements(), keyHandle.Length(),
> +                               &buffer, &bufferlen);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return false;
> +  }
> +

MOZ_ASSERT(buffer);

::: dom/u2f/U2F.h:104
(Diff revision 6)
>    // for a description of the algorithm.
>    bool
>    ValidAppID(/* in/out */ nsString& aAppId) const;
> +
> +  nsresult
> +  NSSTokenIsCompatible(const nsString& versionString, bool* isCompatible);

aVersionString, aIsCompatible, etc..

::: dom/u2f/U2F.cpp:116
(Diff revision 6)
>      return;
>    }
>  }
>  
>  nsresult
> +U2F::NSSTokenIsCompatible(const nsString& versionString, bool* isCompatible)

ditto.

::: dom/u2f/U2F.cpp:117
(Diff revision 6)
>    }
>  }
>  
>  nsresult
> +U2F::NSSTokenIsCompatible(const nsString& versionString, bool* isCompatible)
> +{

MOZ_ASSERT(aIsCompatible);

::: dom/u2f/U2F.cpp:133
(Diff revision 6)
> +  return NS_OK;
> +}
> +
> +nsresult
> +U2F::NSSTokenIsRegistered(CryptoBuffer& keyHandle, bool* isRegistered)
> +{

MOZ_ASSERT(aIsRegistered);

::: dom/u2f/U2F.cpp:394
(Diff revision 6)
>      }
>  
>      // We ignore mTransports, as it is intended to be used for sorting the
>      // available devices by preference, but is not an exclusion factor.
>  
> +    bool isCompatible = false;

why here? can you move this into the if() stmt?

::: dom/u2f/U2F.cpp:423
(Diff revision 6)
> +        return;
> +      }
> +
> +      if (isCompatible && isRegistered) {
> -      SendError<U2FRegisterCallback, RegisterResponse>(aCallback,
> +        SendError<U2FRegisterCallback, RegisterResponse>(aCallback,
>                                                    ErrorCode::DEVICE_INELIGIBLE);

Indentation.

::: dom/crypto/CryptoBuffer.h:50
(Diff revision 8)
>  
>    nsresult FromJwkBase64(const nsString& aBase64);
>    nsresult ToJwkBase64(nsString& aBase64);
>    bool ToSECItem(PLArenaPool* aArena, SECItem* aItem) const;
>    JSObject* ToUint8Array(JSContext* aCx) const;
> +  bool ToNewUnsignedBuffer(uint8_t** buf, uint32_t* bufLen) const;

aBuf, aBufLen

::: dom/crypto/CryptoBuffer.cpp:195
(Diff revision 8)
>  {
>    return Uint8Array::Create(aCx, Length(), Elements());
>  }
>  
> +bool
> +CryptoBuffer::ToNewUnsignedBuffer(uint8_t** buf, uint32_t* bufLen) const

aBuf, aBufLen

::: dom/ipc/ContentParent.h:752
(Diff revision 8)
>                         const BlobConstructorParams& params) override;
>  
>    virtual bool
>    DeallocPCrashReporterParent(PCrashReporterParent* crashreporter) override;
>  
> +  virtual bool RecvNSSU2FTokenIsCompatibleVersion(const nsString& version,

aVersion, aIsCompatible.

Everywhere in this patch.

::: dom/u2f/U2F.cpp:98
(Diff revision 8)
>    if (NS_WARN_IF(mOrigin.IsEmpty())) {
>      return;
>    }
>  
>    if (!EnsureNSSInitializedChromeOrContent()) {
> +    MOZ_LOG(gU2FLog, LogLevel::Debug, ("Failed to get NSS context for U2F"));

aRv.Throw(something?)

::: dom/u2f/U2F.cpp:104
(Diff revision 8)
>      return;
>    }
>  
> -  aRv = mSoftToken.Init();
> -  if (NS_WARN_IF(aRv.Failed())) {
> +  if (XRE_IsParentProcess()) {
> +    mNSSToken = do_GetService(NS_NSSU2FTOKEN_CONTRACTID);
> +    if (NS_WARN_IF(!mNSSToken)) {

aRv.Throw(NS_ERROR_FAILURE);

::: dom/u2f/U2F.cpp:394
(Diff revision 8)
>      }
>  
>      // We ignore mTransports, as it is intended to be used for sorting the
>      // available devices by preference, but is not an exclusion factor.
>  
> +    bool isCompatible = false;

move these 2 variables closer to when you actually use them. In particular inside the if(softTokenEnabled) { block }.

::: dom/u2f/U2F.cpp:481
(Diff revision 8)
>      }
>  
>      // Get the registration data from the token
>      CryptoBuffer registrationData;
>      bool registerSuccess = false;
> -
> +    bool isCompatible = false;

same here. move this variable inside if (!registerSuccess && softOkenEnabled) { ..
Attachment #8725825 - Flags: review?(amarchesini) → review+
https://reviewboard.mozilla.org/r/37653/#review37861

Thank you again, baku! I really appreciate your expertise on the DOM and IPC. I'm putting these into a Part 3 commit for cleanliness, as they affect both Part 1 and Part 2.

One set of your inputs, regarding the `xxxxTokenEnabled` booleans, I've deferred handling because of that section being refactored in the WIP code in bug 1244959.

> You use Length() 3 times in this method.
> Can you store it in a variable instead?

Good idea. Thanks.

> MOZ_ASSERT(aIsRegistered);

Thanks for this; it makes perfect sense to assert raw pointers like this everywhere, so I'll make sure I do that going forward. :)

> why here? can you move this into the if() stmt?

This'll get refactored pretty heavily in the next patch, bug 1244959, so I'll skip fixing this one this time.

> Indentation.

I know I go back and forth about whether to extend out past 80 in these cases. I added the necessary spaces here. :)

> aRv.Throw(something?)

Oh yeah, thanks! I forgot about the availability of that here.

> move these 2 variables closer to when you actually use them. In particular inside the if(softTokenEnabled) { block }.

As above; since this gets heavily refactored in bug 1244959, I'll skip fixing this one this time.

> same here. move this variable inside if (!registerSuccess && softOkenEnabled) { ..

As above; since this gets heavily refactored in bug 1244959, I'll skip fixing this one this time.
Work on the FacetID/AppID algorithm showed this patch had incorrect usage of
the eTLD+1 checking, so this patch removes those checks until the more
sophisticated algorithm lands in Bug 1244959.

Review commit: https://reviewboard.mozilla.org/r/46137/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46137/
Comment on attachment 8741040 [details]
MozReview Request: Bug 1244960 - FIDO u2f NSSToken (Part 3): Review updates r?keeler

https://reviewboard.mozilla.org/r/46135/#review43045

::: dom/u2f/U2F.cpp:94
(Diff revision 1)
>    if (NS_WARN_IF(aRv.Failed())) {
>      return;
>    }
>  
>    if (NS_WARN_IF(mOrigin.IsEmpty())) {
>      return;

Do we need to throw here too?
Attachment #8741040 - Flags: review?(dkeeler) → review+
Comment on attachment 8741041 [details]
MozReview Request: Bug 1244960 - FIDO u2f NSSToken (Part 4): Correct FacetID base algorithm r?keeler

https://reviewboard.mozilla.org/r/46137/#review43047

::: dom/u2f/U2F.cpp:249
(Diff revision 1)
>  bool
>  U2F::ValidAppID(/* in/out */ nsString& aAppId) const
>  {
>    nsCOMPtr<nsIURLParser> urlParser =
>        do_GetService(NS_STDURLPARSER_CONTRACTID);
>    nsCOMPtr<nsIEffectiveTLDService> tldService =

Maybe also remove the tldService for now if it's not used?

::: dom/u2f/tests/test_frame_appid_facet_subdomain.html:21
(Diff revision 1)
> -// eTLD+1 check
> -u2f.register("https://example.com/appId", [{
> +// same domain check
> +u2f.register("https://test1.example.com/appId", [{
>    version: version,
>    challenge: bytesToBase64UrlSafe(challenge),
>  }], [], function(res){
> -  local_is(res.errorCode, 0, "AppID should work from a subdomain");
> +  local_is(res.errorCode, 0, "AppID should work from a subfolder of this domain");

Is "subfolder" the right term? Isn't it just a different path?

::: dom/u2f/tests/test_frame_appid_facet_subdomain.html:31
(Diff revision 1)
> +  // same domain check, but wrong scheme
> +  u2f.register("http://test1.example.com/appId", [{
> -  version: version,
> +    version: version,
> -  challenge: bytesToBase64UrlSafe(challenge),
> +    challenge: bytesToBase64UrlSafe(challenge),
> -}], [], function(res){
> +  }], [], function(res){
> -  local_isnot(res.errorCode, 0, "AppID should not work from other domains");
> +    local_isnot(res.errorCode, 0, "AppID should work from a subfolder of this domain");

Should this be "should not work"? (And maybe add something about how it's a different scheme?)
Attachment #8741041 - Flags: review?(dkeeler) → review+
https://reviewboard.mozilla.org/r/46137/#review43047

> Maybe also remove the tldService for now if it's not used?

Since it's not critical, I'm going to defer this to Bug 1264472, the next in the series. That code is such a significant rewrite of U2F.cpp that the rebase would be unpleasant.

> Should this be "should not work"? (And maybe add something about how it's a different scheme?)

Yup, thanks!
https://reviewboard.mozilla.org/r/46135/#review43045

Thanks again for re-reviews, keeler. :)

> Do we need to throw here too?

We should; I've added the throw to the code for Bug 1264472, to avoid the ugly rebase again.
Try run is looking good. Carried r=keeler forward on the part 5 review updates. Marking checkin-needed.
Keywords: checkin-needed
has problems to apply:

patching file security/manager/ssl/nsNSSModule.cpp
Hunk #1 FAILED at 0
Hunk #3 FAILED at 202
Hunk #4 FAILED at 239
Hunk #5 FAILED at 274
Hunk #6 FAILED at 314
5 out of 6 hunks FAILED -- saving rejects to file security/manager/ssl/nsNSSModule.cpp.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue
Tomcats-MacBook-Pro-2:mozilla-inbound Tomcat$ 

can you take a look ? thanks!
Flags: needinfo?(jjones)
Comment on attachment 8725825 [details]
MozReview Request: Bug 1244960 - Complete FIDO u2f NSSToken (Part 1) r?keeler r?baku

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37653/diff/8-9/
Comment on attachment 8732435 [details]
MozReview Request: Bug 1244960 - FIDO u2f NSSToken (Part 2): Use Attestation Certificates r?keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41171/diff/7-8/
Comment on attachment 8741040 [details]
MozReview Request: Bug 1244960 - FIDO u2f NSSToken (Part 3): Review updates r?keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46135/diff/1-2/
Comment on attachment 8741041 [details]
MozReview Request: Bug 1244960 - FIDO u2f NSSToken (Part 4): Correct FacetID base algorithm r?keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46137/diff/1-2/
Comment on attachment 8741443 [details]
MozReview Request: Bug 1244960 - FIDO u2f NSSToken (Part 5): Review updates r=keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46483/diff/1-2/
Carsten - thanks. I rebased, tested locally, and updated the patches. Should be ready to land now.
Flags: needinfo?(jjones)
Keywords: checkin-needed
Comment on attachment 8741443 [details]
MozReview Request: Bug 1244960 - FIDO u2f NSSToken (Part 5): Review updates r=keeler

https://reviewboard.mozilla.org/r/46483/#review43379
Attachment #8741443 - Flags: review?(dkeeler) → review+
Depends on: 1275197
You need to log in before you can comment on or make changes to this bug.