Closed Bug 1034856 Opened 5 years ago Closed 5 years ago

Add support for DH to WebCrypto API

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox35 --- fixed
firefox36 --- fixed

People

(Reporter: rbarnes, Assigned: ttaubert)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(6 files, 12 obsolete files)

12.55 KB, patch
rbarnes
: review+
smaug
: review+
Details | Diff | Splinter Review
12.64 KB, patch
rbarnes
: review+
smaug
: review+
Details | Diff | Splinter Review
11.12 KB, patch
rbarnes
: review+
Details | Diff | Splinter Review
7.84 KB, patch
rbarnes
: review+
smaug
: review+
Details | Diff | Splinter Review
16.20 KB, patch
rbarnes
: review+
Details | Diff | Splinter Review
13.23 KB, patch
ttaubert
: review+
Details | Diff | Splinter Review
No description provided.
Assignee: nobody → ttaubert
Attached patch 0003-WIP-key-import.patch (obsolete) — Splinter Review
Key/bit derivation should probably work but key import is making life hard again. Looks like NSS needs a public key internally or else importing DH keys won't work. The spec doesn't require public keys for pkcs8 import though - computing the public key from the private key and the DH params wouldn't be hard but seems like a little too much work to get a key ID (not sure what that's even used for).
FTR, bug 1041328 introduced a crash test that currently passes "RSA-OAEP" as the algorithm. This needs to be changed to "DH" because actually pass a pkcs8 DH key but that wouldn't crash as long as we don't support "DH". This should be done in the patch here.
Attachment #8459338 - Attachment is obsolete: true
Attachment #8459339 - Attachment is obsolete: true
Attachment #8459340 - Attachment is obsolete: true
Attachment #8459341 - Attachment is obsolete: true
Attachment #8476681 - Flags: review?(rlb)
Status: NEW → ASSIGNED
Test vector corrected.
Attachment #8476682 - Attachment is obsolete: true
Attachment #8476682 - Flags: review?(rlb)
Attachment #8476730 - Flags: review?(rlb)
Oops, small fix.
Attachment #8477360 - Attachment is obsolete: true
Attachment #8477360 - Flags: review?(rlb)
Attachment #8477402 - Flags: review?(rlb)
Test vector cleanup.
Attachment #8477403 - Attachment is obsolete: true
Attachment #8477403 - Flags: review?(rlb)
Attachment #8477463 - Flags: review?(rlb)
Comment on attachment 8476681 [details] [diff] [review]
0001-Bug-1034856-Introduce-DhKeyAlgorithm.patch, v2

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

Just a note: The latest WebCrypto spec has changed KeyAlgorithm from an interface to a dictionary type.
https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#key-algorithm-dictionary

So there's going to be some refactoring of how KeyAlgorithms work in the patches for bug 1037892.  But that's still a little while out from landing, so this can go ahead for now.

::: dom/webidl/SubtleCrypto.webidl
@@ +44,5 @@
>  [NoInterfaceObject]
> +interface DhKeyAlgorithm : KeyAlgorithm {
> +  [Throws]
> +  readonly attribute BigInteger prime;
> +  [Throws]

What's the reason for the [Throws] annotation here?  I'm a little surprised that WebIDL lets you apply that to attributes (vs. methods).
Attachment #8476681 - Flags: review?(rlb) → review+
Comment on attachment 8476730 [details] [diff] [review]
0002-Bug-1034856-Implement-generateKey.patch, v3

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

::: dom/crypto/CryptoBuffer.cpp
@@ +161,5 @@
>    return item;
>  }
>  
> +bool
> +CryptoBuffer::ToSECItem(PLArenaPool *aArena, SECItem* aItem) const

Would it be better just to make this the new form of ToSECItem in general?  I'm not an expert, but I don't think there are cases where using moz_malloc() is better than SECITEM_AllocItem().
Attachment #8476730 - Flags: review?(rlb) → review+
Comment on attachment 8476731 [details] [diff] [review]
0003-Bug-1034856-Implement-deriveBits-for-DH.patch

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

::: dom/crypto/CryptoBuffer.cpp
@@ +77,5 @@
>    return nullptr;
>  }
>  
> +bool
> +CryptoBuffer::Equals(const CryptoBuffer& aOther) const

Is this needed?  The parent class nsTArray_impl has an operator== that does the same thing.  (And the corresponding operator!=, which is what you want below)
http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h#798

Note that neither of these should be used when either of the values being compared is supposed to be secret, since the fact that they exit early creates a timing vulnerability.  In those cases, you should use NSS_SecureMemcmp, as in HmacTask::Resolve().  Things look OK here, though.

::: dom/crypto/CryptoBuffer.h
@@ +36,5 @@
>      aArray.ComputeLengthAndData();
>      return Assign(aArray.Data(), aArray.Length());
>    }
>  
> +  bool Equals(const CryptoBuffer& aOther) const;

Ditto here.

::: dom/crypto/WebCryptoTask.cpp
@@ +2661,5 @@
> +  virtual nsresult DoCrypto() MOZ_OVERRIDE
> +  {
> +    ScopedPK11SymKey symKey(PK11_PubDerive(
> +      mPrivKey, mPubKey, PR_FALSE, nullptr, nullptr, CKM_DH_PKCS_DERIVE,
> +      // TODO CKM_CONCATENATE_DATA_AND_BASE?

This and the subsequent argument (CKA_DERIVE) are used by NSS to assign a type to the derived symmetric key.  Since we don't rely on these attributes of the symmetric key, it doesn't matter what we set them to.  I would just pick a consistent set (e.g., an HMAC CKM and CKA_SIGN), and add a comment that they don't matter.  The same should apply to the PK11_PubDeriveWithKDF call in DeriveEcdhBitsTask::DoCrypto().

Which is my other question: Why is this using PK11_PubDerive, while ECDH uses PK11_PubDeriveWithKDF?  It seems like they should be the same.
Attachment #8476731 - Flags: review?(rlb) → review+
Comment on attachment 8477463 [details] [diff] [review]
0005-Bug-1034856-Implement-SPKI-public-key-import-export-.patch, v2

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

::: dom/crypto/test/test-vectors.js
@@ +630,5 @@
> +      "038184000281804fc9904887ac7fabff87f054003547c2d9458c1f6f584c140d" +
> +      "7271f8b266bb390af7e3f625a629bec9c6a057a4cbe1a556d5e3eb2ff1c6ff67" +
> +      "7a08b0c7c509110b9e7c6dbc961ca4360362d3dbcffc5bf2bb7207e0a5922f77" +
> +      "cf5464b316aa49fb62b338ebcdb30bf573d07b663bb7777b69d6317df0a4f636" +
> +      "ba3d9acbf9e8ac"

Where did this SPKI come from?  I decoded it and was surprised to find that the included generator is quite large (1018 bits), rather than 2.  I guess this is still a legal value, but it looked a bit off.
Attachment #8477463 - Flags: review?(rlb) → review+
Comment on attachment 8477402 [details] [diff] [review]
0004-Bug-1034856-Implement-raw-public-key-import-export-f.patch, v2

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

::: dom/crypto/WebCryptoTask.cpp
@@ +1896,5 @@
> +    // Check permissions for the requested operation
> +    if (mKey->GetKeyType() == CryptoKey::PRIVATE &&
> +        mKey->HasUsageOtherThan(CryptoKey::DERIVEBITS | CryptoKey::DERIVEKEY)) {
> +      return NS_ERROR_DOM_DATA_ERR;
> +    }

It seems like restricting this check to private keys is wrong, if only because public keys aren't good for anything else either.  Let's apply this check to public keys as well, and I'll see if there's a spec bug to open.
Attachment #8477402 - Flags: review?(rlb) → review+
> > +      "038184000281804fc9904887ac7fabff87f054003547c2d9458c1f6f584c140d" +
> > +      "7271f8b266bb390af7e3f625a629bec9c6a057a4cbe1a556d5e3eb2ff1c6ff67" +
> > +      "7a08b0c7c509110b9e7c6dbc961ca4360362d3dbcffc5bf2bb7207e0a5922f77" +
> > +      "cf5464b316aa49fb62b338ebcdb30bf573d07b663bb7777b69d6317df0a4f636" +
> > +      "ba3d9acbf9e8ac"
> 
> Where did this SPKI come from?  I decoded it and was surprised to find that
> the included generator is quite large (1018 bits), rather than 2.  I guess
> this is still a legal value, but it looked a bit off.

Aha, I see now that it's from the NIST test vectors.  Carry on.
Comment on attachment 8477402 [details] [diff] [review]
0004-Bug-1034856-Implement-raw-public-key-import-export-f.patch, v2

Olli, can you please take a look at the IDL changes?
Attachment #8477402 - Flags: review?(bugs)
Attachment #8477402 - Flags: review?(bugs) → review+
Re-asking for review due to changing ToSECItem() to do |aItem->data = nullptr| so that SECITEM_AllocItem() doesn't assert.

We could also require the caller to do that but that seems a little uglier? We will override .data anyway.
Attachment #8476730 - Attachment is obsolete: true
Attachment #8483505 - Flags: review?(rlb)
(In reply to Richard Barnes [:rbarnes] from comment #16)
> > +bool
> > +CryptoBuffer::ToSECItem(PLArenaPool *aArena, SECItem* aItem) const
> 
> Would it be better just to make this the new form of ToSECItem in general? 
> I'm not an expert, but I don't think there are cases where using
> moz_malloc() is better than SECITEM_AllocItem().

Did that.
Attachment #8483507 - Flags: review?(rlb)
Attachment #8483507 - Flags: review?(rlb) → review+
Comment on attachment 8483505 [details] [diff] [review]
0002-Bug-1034856-Implement-generateKey.patch, v4

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

::: dom/crypto/CryptoBuffer.cpp
@@ +164,5 @@
> +bool
> +CryptoBuffer::ToSECItem(PLArenaPool *aArena, SECItem* aItem) const
> +{
> +  aItem->type = siBuffer;
> +  aItem->data = nullptr;

I guess this is the right thing to do.  I have some nagging worry that someone will pass in a SECItem with an allocated buffer, which raises a risk of leaks if we don't SECITEM_FreeItem.  But we don't really have a way to distinguish that from a SECItem that's badly initialized, so this seems OK.
Attachment #8483505 - Flags: review?(rlb) → review+
(In reply to Richard Barnes [:rbarnes] from comment #17)
> > +CryptoBuffer::Equals(const CryptoBuffer& aOther) const
> 
> Is this needed?  The parent class nsTArray_impl has an operator== that does
> the same thing.  (And the corresponding operator!=, which is what you want
> below)

Thanks, I was sure I tried that but whatever. Works now :)

> > +    ScopedPK11SymKey symKey(PK11_PubDerive(
> > +      mPrivKey, mPubKey, PR_FALSE, nullptr, nullptr, CKM_DH_PKCS_DERIVE,
> > +      // TODO CKM_CONCATENATE_DATA_AND_BASE?
> 
> Which is my other question: Why is this using PK11_PubDerive, while ECDH
> uses PK11_PubDeriveWithKDF?  It seems like they should be the same.

Using PK11_PubDeriveWithKDF() key now and fixed the other call as well.
(In reply to Richard Barnes [:rbarnes] from comment #15)
> > +interface DhKeyAlgorithm : KeyAlgorithm {
> > +  [Throws]
> > +  readonly attribute BigInteger prime;
> > +  [Throws]
> 
> What's the reason for the [Throws] annotation here?  I'm a little surprised
> that WebIDL lets you apply that to attributes (vs. methods).

We use the same annotation for RsaKeyAlgorithm's BigInteger (publicExponent) so that's why I thought I should use it too? We could be OOM when trying to create the Uint8Array to return for BigInteger attributes. Does that make sense or should I remove [Throws]?
Flags: needinfo?(rlb)
(In reply to Tim Taubert [:ttaubert] from comment #26)
> (In reply to Richard Barnes [:rbarnes] from comment #15)
> > > +interface DhKeyAlgorithm : KeyAlgorithm {
> > > +  [Throws]
> > > +  readonly attribute BigInteger prime;
> > > +  [Throws]
> > 
> > What's the reason for the [Throws] annotation here?  I'm a little surprised
> > that WebIDL lets you apply that to attributes (vs. methods).
> 
> We use the same annotation for RsaKeyAlgorithm's BigInteger (publicExponent)
> so that's why I thought I should use it too? We could be OOM when trying to
> create the Uint8Array to return for BigInteger attributes. Does that make
> sense or should I remove [Throws]?

I see.  Makes sense to keep it, then.
Flags: needinfo?(rlb)
Comment on attachment 8476681 [details] [diff] [review]
0001-Bug-1034856-Introduce-DhKeyAlgorithm.patch, v2

Hey Olli, can you please review the WebIDL changes? Here's a link to the spec:

https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#dh-DhKeyAlgorithm
Attachment #8476681 - Flags: review?(bugs)
Comment on attachment 8476731 [details] [diff] [review]
0003-Bug-1034856-Implement-deriveBits-for-DH.patch

Here are some more IDL changes, spec:

https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#dh-DhKeyDeriveParams
Attachment #8476731 - Flags: review?(bugs)
Comment on attachment 8476681 [details] [diff] [review]
0001-Bug-1034856-Introduce-DhKeyAlgorithm.patch, v2

Could you make sure we have a bug to convert all the
*KeyAlgorithm stuff to use dictionaries as it is in the spec.
Attachment #8476681 - Flags: review?(bugs) → review+
Attachment #8476731 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #30)
> Could you make sure we have a bug to convert all the
> *KeyAlgorithm stuff to use dictionaries as it is in the spec.

That's bug 1037892 and Richard is on it. Shouldn't take too long.
This doesn't compile on Windows and Linux unfortunately. It seems that SECKEY_DHParamKeyTemplate and SECKEY_DHPublicKeyTemplate are not exported by NSS and we can't access them through SEC_ASN1_GET() like I did for ECDH either. Any idea?
(In reply to Tim Taubert [:ttaubert] from comment #32)
> This doesn't compile on Windows and Linux unfortunately. It seems that
> SECKEY_DHParamKeyTemplate and SECKEY_DHPublicKeyTemplate are not exported by
> NSS and we can't access them through SEC_ASN1_GET() like I did for ECDH
> either. Any idea?
Flags: needinfo?(rlb)
Mostly punting this to keeler.

You might look at whether there's a difference in how those two items are handled in nss.def:
http://dxr.mozilla.org/mozilla-central/source/security/nss/lib/nss/nss.def
Flags: needinfo?(rlb) → needinfo?(dkeeler)
Doesn't look like those are exported at all (I think they still have to be exported as data for SEC_ASN1_GET to work). A few options: (a) file a bug to have them exported in NSS. (b) duplicate their definitions. (c) both, and undo (b) after (a) happens.
Flags: needinfo?(dkeeler)
I would note that we basically did (b) for RSA public keys.
http://dxr.mozilla.org/mozilla-central/source/dom/crypto/CryptoKey.cpp?from=CryptoKey.cpp#740

So maybe let's do the same thing here for DH just to get things done, then circle back and file bugs for them both.  (Actually, the RSA one might be doable with SEC_ASN1_GET, I haven't checked.)
Depends on: 1078847
Guess I should ask for review again, things have changed a bit.

https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#dh-DhKeyAlgorithm
Attachment #8476681 - Attachment is obsolete: true
Attachment #8500756 - Flags: review?(rlb)
Attachment #8500756 - Flags: review?(bugs)
Went with comment #36 and copied the templates.
Attachment #8477463 - Attachment is obsolete: true
Attachment #8500800 - Flags: review?(rlb)
Attachment #8500756 - Flags: review?(bugs) → review+
Pushed together with bug 1078847 of course.
Attachment #8500756 - Flags: review?(rlb) → review+
Comment on attachment 8500800 [details] [diff] [review]
0005-Bug-1034856-Implement-SPKI-public-key-import-export-.patch, v3

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

r=me with these comments addressed.

::: dom/crypto/CryptoKey.cpp
@@ +13,5 @@
>  #include "mozilla/dom/ToJSValue.h"
>  
> +// Templates taken from security/nss/lib/cryptohi/seckey.c
> +// These would ideally be exported by NSS and until that
> +// happens we have to keep our own copies.

File follow-up bug?

@@ +411,5 @@
> +  if (isECDHAlgorithm || isDHAlgorithm) {
> +    SECOidTag oid = SEC_OID_UNKNOWN;
> +    if (isECDHAlgorithm) {
> +      oid = SEC_OID_ANSIX962_EC_PUBLIC_KEY;
> +    } else if (isDHAlgorithm) {

Add an "else" branch with MOZ_ASSERT(false), in case the top-level if-statement changes without a change here.

@@ +452,5 @@
> +  ScopedPLArenaPool arena;
> +  ScopedCERTSubjectPublicKeyInfo spki;
> +
> +  // NSS doesn't support exporting DH public keys.
> +  if (aPubKey->keyType == dhKey) {

It would be a little cleaner to isolate the body of this branch in a separate function.

@@ +466,5 @@
> +    if (!spki) {
> +      return NS_ERROR_DOM_OPERATION_ERR;
> +    }
> +
> +    SECItem* params = PORT_ArenaZNew(arena, SECItem);

Null-check params?

@@ +467,5 @@
> +      return NS_ERROR_DOM_OPERATION_ERR;
> +    }
> +
> +    SECItem* params = PORT_ArenaZNew(arena, SECItem);
> +    SECItem* rv_item = SEC_ASN1EncodeItem(arena, params, aPubKey,

Nit: rvItem

@@ +485,5 @@
> +    if (!rv_item) {
> +      return NS_ERROR_DOM_OPERATION_ERR;
> +    }
> +
> +    spki->subjectPublicKey.len <<= 3;

I'm assuming this is converting from bytes to bits?  Why do you need to do that?  A comment here would be good.

@@ +501,5 @@
> +  if (aPubKey->keyType == ecKey || aPubKey->keyType == dhKey) {
> +    const SECItem* oidData = nullptr;
> +    if (aPubKey->keyType == ecKey) {
> +      oidData = &SEC_OID_DATA_EC_DH;
> +    } else if (aPubKey->keyType == dhKey) {

Same need for "else" with MOZ_ASSERT(false) as above.
Attachment #8500800 - Flags: review?(rlb) → review+
Need to ask for review here again because I had to make some non-trivial changes to AsymmetricSignVerifyTask when rebasing due to ECDSA landing.

I would have liked to use a single scoped arena and SECItem*s but unfortunately there is no way to pass an arena to DSAU_EncodeDerSigWithLen().

I additionally converted all the PORT_Alloc() calls to SECITEM_AllocItem().
Attachment #8483507 - Attachment is obsolete: true
Attachment #8506100 - Flags: review?(rlb)
Depends on: 1083765
(In reply to Richard Barnes [:rbarnes] from comment #41)
> r=me with these comments addressed.

All addressed, thanks!

> > +// Templates taken from security/nss/lib/cryptohi/seckey.c
> > +// These would ideally be exported by NSS and until that
> > +// happens we have to keep our own copies.
> 
> File follow-up bug?

Filed bug 1083765.
Attachment #8500800 - Attachment is obsolete: true
Attachment #8506118 - Flags: review+
Attachment #8506100 - Flags: review?(rlb) → review+
Pushed a follow-up to fix a small SECItem leak on ASAN builds:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ace6aa02d442

Richard, can you please do a post-review to sanity check that? The issue here was that the SECITEM_CopyItem() call in CryptoKey::PublicKeyToSpki() was passing spki->arena as the PLArenaPool to use. That is null however as stated above for DH keys.
Flags: needinfo?(rlb)
I backed the follow-up out again:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d78ab6a8c92f

Turns out that arena goes out of scope before spki, so it frees the PLArenaPool and seems to override spki->arena with rubbish and that makes us crash when spki goes out of scope because that tries to free its arena.

I landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c6233f10333

... to really just mimic how the SPKI object is created below and we can use the scoped pointer to free that. I unfortunately forgot to update the PublicDhKeyToSpki() call so I had to push some more:

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc77c762d6a0

If that all still leaks I'll back it out but it really shouldn't. Sorry for the hassle :/
(In reply to Tim Taubert [:ttaubert] from comment #46)
> Pushed a follow-up to fix a small SECItem leak on ASAN builds:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/ace6aa02d442
> 
> Richard, can you please do a post-review to sanity check that? The issue
> here was that the SECITEM_CopyItem() call in CryptoKey::PublicKeyToSpki()
> was passing spki->arena as the PLArenaPool to use. That is null however as
> stated above for DH keys.

That looks sane to me.  Seems like in generally allocating the arena at the top of the method is the right idea, so that it remains in scope for the method.
Flags: needinfo?(rlb)
Comment on attachment 8476731 [details] [diff] [review]
0003-Bug-1034856-Implement-deriveBits-for-DH.patch

Approval Request Comment
[Feature/regressing bug #]: WebCrypto (865789)
[User impact if declined]: DH will be unavailable in WebCrypto
[Describe test coverage new/current, TBPL]: Mochitests in patch
[Risks and why]: Low risk, only affects WebCrypto
[String/UUID change made/needed]: None

If approved, please also uplift the other patches in this bug.
Attachment #8476731 - Flags: approval-mozilla-aurora?
Attachment #8476731 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Tests are failing on Aurora:
https://treeherder.mozilla.org/ui/index.html#/jobs?repo=mozilla-aurora&revision=569c89370744

It appears that the promise returned by the doGenerateDH() method on line 95 of test_WebCrypto_DH.html is being resolved with the value undefined.  I tried making WebCryptoTask.cpp the same as in mozilla-central (there was only a minor difference), and this didn't resolve the issue.

Tim: Could you take a look at this?
We're missing bug 1078847. That needs to be uplifted as well... WFM locally after applying that patch too.
(In reply to Tim Taubert [:ttaubert] from comment #54)
> We're missing bug 1078847. That needs to be uplifted as well... WFM locally
> after applying that patch too.

Were you going to request approval on that? :)
Yes, thanks for the reminder. Got a little sidetracked :)
See Also: → 1564509
You need to log in before you can comment on or make changes to this bug.