Encode and Verify AppID in FIDO u2f NSSToken Key Handles

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jcj, Assigned: jcj)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: btpp-backlog)

Attachments

(1 attachment)

From rlb on the review for Bug 1244960 (https://bugzilla.mozilla.org/show_bug.cgi?id=1244960#c22):
> 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.

This is a good idea, and is in the specification. It's out-of-scope for the NSS Token being just a debugging interface, but would be important for future expansion of its purpose.
Whiteboard: btpp-backlog
The current code uses AES Keywrap via PK11_WrapPrivKey and PK11_UnwrapPrivKey to import/export the wrapped EC keys. Those methods can't themselves include additional wrapped data for comparison. I see a couple ways to proceed with this bug:

1) We could use a separate implementation of a key wrapping function which can include additional data. AES Keywrap can do it, we just need to call some form of the function that doesn't try to take impose operation on SECKEYPrivateKey types. I think this would be new code, either in NSS or Gecko.

2) We could define the wrapping key to be a function of U2F_NSSTOKEN (from the local NSS keystore) and ApplicationID, using methods already exposed, such as:

2a) WrapKey = KDF( local_nsstoken | SHA256(ApplicationID) )
2b) WrapKey = local_nsstoken ^ truncate(SHA256(ApplicationID), 128) // local_nsstoken is 128-bits

rbarnes: What do you suggest?
Flags: needinfo?(rlb)
I like (2a) the best.  Just fold whatever metadata you want into the wrapping key via a KDF.  NSS makes it pretty simple to call HKDF with whatever octets you want:

http://searchfox.org/mozilla-central/source/dom/crypto/WebCryptoTask.cpp#2764
Flags: needinfo?(rlb)
Comment hidden (mozreview-request)

Comment 4

2 years ago
mozreview-review
Comment on attachment 8830394 [details]
Bug 1260318 - Scope U2F Soft Tokens to a single AppID

https://reviewboard.mozilla.org/r/106878/#review109040

::: dom/u2f/U2F.cpp:636
(Diff revision 1)
> +  if (!appParam.SetLength(SHA256_LENGTH, fallible)) {
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  }
> +
> +  // Note: This could use nsICryptoHash to avoid having to interact with NSS
> +  // directly.

I don't think that's necessary.

::: security/manager/ssl/nsNSSU2FToken.cpp:385
(Diff revision 1)
>    MOZ_LOG(gNSSTokenLog, LogLevel::Debug, ("U2F Soft Token initialized."));
>    return NS_OK;
>  }
>  
>  // Convert a Private Key object into an opaque key handle, using AES Key Wrap
> -// and aWrappingKey to convert aPrivKey.
> +// and the long-lived aPersistentKey to convert aPrivKey.

Maybe note that the appParam gets mixed in here.

::: security/manager/ssl/nsNSSU2FToken.cpp:401
(Diff revision 1)
>    MOZ_ASSERT(aPrivKey);
> -  if (!aSlot || !aWrappingKey || !aPrivKey) {
> +  if (!aSlot || !aPersistentKey || !aPrivKey || !aAppParam) {
> +    return nullptr;
> +  }
> +
> +  // Generate a random salt

We might be able to get away without this, but it seems conservative to have it.  Cf. https://tools.ietf.org/html/rfc5869#section-3.1

::: security/manager/ssl/nsNSSU2FToken.cpp:411
(Diff revision 1)
> +            ("Failed to generate a salt, NSS error #%d", PORT_GetError()));
> +    return nullptr;
> +  }
> +
> +  // Prepare the HKDF (https://tools.ietf.org/html/rfc5869)
> +  CK_NSS_HKDFParams hkdfParams = { /* bExtract */ true,

Nit: The commented parameter names aren't necessary.

::: security/manager/ssl/nsNSSU2FToken.cpp:451
(Diff revision 1)
> -      MOZ_LOG(gNSSTokenLog, LogLevel::Warning,
> +    MOZ_LOG(gNSSTokenLog, LogLevel::Warning,
> -              ("Failed to wrap U2F key, NSS error #%d", PORT_GetError()));
> +            ("Failed to wrap U2F key, NSS error #%d", PORT_GetError()));
>      return nullptr;
>    }
>  
> -  return wrappedKey;
> +  // Concatenate the salt and the wrapped Private Key together

I'm concerned about forward compatibility here.  If for some reason we decide to change the salt length in the future, existing key handles will all break.

To be conservative, let's add an octet of salt length at the front.  So `saltLen || salt || keyHandle`.

::: security/manager/ssl/nsNSSU2FToken.cpp:456
(Diff revision 1)
> -  return wrappedKey;
> +  // Concatenate the salt and the wrapped Private Key together
> +  mozilla::dom::CryptoBuffer keyHandleBuf;
> +  if (!keyHandleBuf.SetCapacity(wrappedKey.get()->len + kSaltByteLen,
> +                                 mozilla::fallible)) {
> +    MOZ_LOG(gNSSTokenLog, LogLevel::Warning,
> +            ("Failed to allocate memory, NSS error #%d", PORT_GetError()));

Doesn't seem appropriate to log an NSS error here, since the problem is OOM, not anything to do with NSS.

::: security/manager/ssl/nsNSSU2FToken.cpp:466
(Diff revision 1)
> +  // pre-allocated space
> +  keyHandleBuf.AppendElements(saltParam, kSaltByteLen, mozilla::fallible);
> +  keyHandleBuf.AppendSECItem(wrappedKey.get());
> +
> +  UniqueSECItem keyHandle(SECITEM_AllocItem(/* default arena */ nullptr,
> +                                             /* no buffer */ nullptr,

Nit: Mis-aligned by one space.

::: security/manager/ssl/nsNSSU2FToken.cpp:502
(Diff revision 1)
> +    return nullptr;
> +  }
> +
> +  // There should be enough bytes in aKeyHandle to be credible after extracting
> +  // the salt
> +  if (aKeyHandleLen < kSaltByteLen + kMinimumKeyHandleLen) {

See above comment about specifying the length of the salt explicitly.  Then you'll want to decode that length here and change this error condition to:

`aKeyHandleLen < 1 + saltLen + minimumKeyHandleLen`

Note that because AES-KW is authenticated, it should be able to handle any shenanigans due to malicious saltLen.
Attachment #8830394 - Flags: review?(rlb) → review-

Comment 5

2 years ago
mozreview-review
Comment on attachment 8830394 [details]
Bug 1260318 - Scope U2F Soft Tokens to a single AppID

https://reviewboard.mozilla.org/r/106878/#review109346

::: security/manager/ssl/nsIU2FToken.idl:32
(Diff revision 1)
>     * @return True if the Key Handle is ours.
>     */
>    void isRegistered([array, size_is(keyHandleLen)] in octet keyHandle,
>                      in uint32_t keyHandleLen,
> +                    [array, size_is(applicationLen)] in octet application,
> +                    in uint32_t applicationLen,

Why are you sending arrays and lengths, and not nsTArrays? I'm converting all of these to nsTArrays since we lose the XPCOM bits in bug 1323339, can do that with this one too I guess.
Attachment #8830394 - Flags: review?(kyle) → review-

Comment 6

2 years ago
mozreview-review
Comment on attachment 8830394 [details]
Bug 1260318 - Scope U2F Soft Tokens to a single AppID

https://reviewboard.mozilla.org/r/106878/#review109348

r-'ing because other parts of patch already r-'d even though the bits I'm supposed to look at look ok. I'll re-review on new match just in case any parts relevant to me changed. Please split reviews like this into 2 patches in the future.
(Assignee)

Comment 7

2 years ago
mozreview-review-reply
Comment on attachment 8830394 [details]
Bug 1260318 - Scope U2F Soft Tokens to a single AppID

https://reviewboard.mozilla.org/r/106878/#review109040

> I don't think that's necessary.

While not necessary, Keeler advised me to minimize direct dependencies on NSS where I can, so I took that approach in WebAuthn and it was easy.

> Maybe note that the appParam gets mixed in here.

done

> We might be able to get away without this, but it seems conservative to have it.  Cf. https://tools.ietf.org/html/rfc5869#section-3.1

Agreed; I did it because it was conservative.

> Nit: The commented parameter names aren't necessary.

Consolidated.

> I'm concerned about forward compatibility here.  If for some reason we decide to change the salt length in the future, existing key handles will all break.
> 
> To be conservative, let's add an octet of salt length at the front.  So `saltLen || salt || keyHandle`.

Good idea. Done.

> Doesn't seem appropriate to log an NSS error here, since the problem is OOM, not anything to do with NSS.

Good point. I've updated the other ones, too.

> Nit: Mis-aligned by one space.

Thanks; fixed.

> See above comment about specifying the length of the salt explicitly.  Then you'll want to decode that length here and change this error condition to:
> 
> `aKeyHandleLen < 1 + saltLen + minimumKeyHandleLen`
> 
> Note that because AES-KW is authenticated, it should be able to handle any shenanigans due to malicious saltLen.

Absolutely.
Comment hidden (mozreview-request)
Assignee: nobody → jjones
(Assignee)

Comment 9

2 years ago
mozreview-review-reply
Comment on attachment 8830394 [details]
Bug 1260318 - Scope U2F Soft Tokens to a single AppID

https://reviewboard.mozilla.org/r/106878/#review109348

Just for clarity - this would have been easier if I'd had the IDL and function decl changes in one patch, and the crypto changes in a second? That makes sense to me, too. Sorry for the low signal-to-noise, Kyle.
(Assignee)

Comment 10

2 years ago
mozreview-review-reply
Comment on attachment 8830394 [details]
Bug 1260318 - Scope U2F Soft Tokens to a single AppID

https://reviewboard.mozilla.org/r/106878/#review109346

> Why are you sending arrays and lengths, and not nsTArrays? I'm converting all of these to nsTArrays since we lose the XPCOM bits in bug 1323339, can do that with this one too I guess.

Originally because it wasn't clear to me (at the time I wrote this) how to do anything but - but you're right, it's not difficult to do now. I went partway into making this change here in this patch, but it feels like I should just do it all at once instead of having one param that way.

Thanks!
(In reply to J.C. Jones [:jcj] from comment #10)
> Comment on attachment 8830394 [details]
> Bug 1260318 - Scope U2F Soft Tokens to a single AppID
> 
> https://reviewboard.mozilla.org/r/106878/#review109346
> 
> > Why are you sending arrays and lengths, and not nsTArrays? I'm converting all of these to nsTArrays since we lose the XPCOM bits in bug 1323339, can do that with this one too I guess.
> 
> Originally because it wasn't clear to me (at the time I wrote this) how to
> do anything but - but you're right, it's not difficult to do now. I went
> partway into making this change here in this patch, but it feels like I
> should just do it all at once instead of having one param that way.
 
Honestly, if you want to land it as-is, I can take care of all that in my patch. It's not a big deal.

Comment 12

2 years ago
mozreview-review
Comment on attachment 8830394 [details]
Bug 1260318 - Scope U2F Soft Tokens to a single AppID

https://reviewboard.mozilla.org/r/106878/#review112554
Attachment #8830394 - Flags: review?(kyle) → review+

Comment 13

2 years ago
mozreview-review
Comment on attachment 8830394 [details]
Bug 1260318 - Scope U2F Soft Tokens to a single AppID

https://reviewboard.mozilla.org/r/106878/#review118874

::: security/manager/ssl/nsNSSU2FToken.cpp:386
(Diff revision 2)
>    MOZ_LOG(gNSSTokenLog, LogLevel::Debug, ("U2F Soft Token initialized."));
>    return NS_OK;
>  }
>  
>  // Convert a Private Key object into an opaque key handle, using AES Key Wrap
> -// and aWrappingKey to convert aPrivKey.
> +// with the long-lived aPersistentKey mixed with aAppParam to convert aPrivKey.

It would be helpful to document here the format of the key handle you produce, i.e.:

```
keyHandle = version=0 || saltLen || salt || wrappedPrivateKey
```

::: security/manager/ssl/nsNSSU2FToken.cpp:460
(Diff revision 2)
> +    return nullptr;
> +  }
> +
> +  // It's OK to ignore the return values here because we're writing into
> +  // pre-allocated space
> +  keyHandleBuf.AppendElement(sizeof(saltParam), mozilla::fallible);

For future-proofing, you should add a version octet at the front of this.

::: security/manager/ssl/nsNSSU2FToken.cpp:498
(Diff revision 2)
> -  if (!aSlot || !aWrappingKey || !aKeyHandle) {
> +  MOZ_ASSERT(aAppParam);
> +  if (!aSlot || !aPersistentKey || !aKeyHandle || !aAppParam) {
>      return nullptr;
>    }
>  
> -  ScopedAutoSECItem pubKey(kPublicKeyLen);
> +  uint8_t saltLen = aKeyHandle[0];

Need to check that `aKeyHandleLen > 0`!  And so on throughout this function.

Both `aKeyHandleLen` and `aAppParamLen` should have known, constent values, right?  Would be good to assert on that.
Attachment #8830394 - Flags: review?(rlb) → review-
Thanks for the catch! I've reworked this to be much more strict in sizing, since there's no need to be flexible for our own code.
Comment hidden (mozreview-request)

Comment 16

2 years ago
mozreview-review
Comment on attachment 8830394 [details]
Bug 1260318 - Scope U2F Soft Tokens to a single AppID

https://reviewboard.mozilla.org/r/106878/#review119620

::: security/manager/ssl/nsNSSU2FToken.cpp:511
(Diff revision 3)
> +  // we aren't that length
> +  if (aKeyHandleLen != kVersion1KeyHandleLen) {
> +    return nullptr;
> +  }
> +
> +  uint8_t version = aKeyHandle[0];

Nit: This should either go before the previous check, or be combined with it.

```
if ((aKHL == kV1KHL) || (aKH[0] != Version1)) { ... }
```

Then you can just delete the declaration of `version`.
Attachment #8830394 - Flags: review?(rlb) → review+
Comment hidden (mozreview-request)
Thanks for the reviews! Try run looks good [1], so marking checkin-needed.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f9cc16a570d&selectedJob=82211383
Status: NEW → ASSIGNED
Keywords: checkin-needed

Comment 19

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/713c0a78c2c1
Scope U2F Soft Tokens to a single AppID r=qdot,rbarnes
Keywords: checkin-needed
Comment hidden (mozreview-request)
OK, a fixed patch has cleared ASAN / LSAN on Try [1], and I had rbarnes spot-check it again. Ready to re-land.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=4225601f9e50
Flags: needinfo?(jjones)
Keywords: checkin-needed

Comment 23

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ad5adacd8e14
Scope U2F Soft Tokens to a single AppID r=qdot,rbarnes
Keywords: checkin-needed

Comment 24

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