Add support for ECDH to WebCrypto API

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: rbarnes, Assigned: ttaubert)

Tracking

(Depends on 3 bugs, Blocks 1 bug)

unspecified
mozilla34
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox 34+)

Details

Attachments

(6 attachments, 17 obsolete attachments)

5.97 KB, patch
smaug
: review+
Details | Diff | Splinter Review
8.76 KB, patch
smaug
: review+
Details | Diff | Splinter Review
5.27 KB, patch
rbarnes
: review+
Details | Diff | Splinter Review
10.90 KB, patch
rbarnes
: review+
Details | Diff | Splinter Review
9.69 KB, patch
ttaubert
: review+
Details | Diff | Splinter Review
31.50 KB, patch
ttaubert
: review+
Details | Diff | Splinter Review
No description provided.
Assignee: nobody → ttaubert
Having a hard time finding test vectors for ECDH with NIST curves. The only thing I found is [1], maybe we would just need to transform that to the respective pkcs8/spki formats?

[1] http://csrc.nist.gov/groups/STM/cavp/documents/keymgmt/kastestvectors.zip
I'm not aware of anything better.  If you go through that file, I think the thing you want is KASValidityTest_ECCOnePassDH_NOKC_resp.fax
Looks like this is blocked by bug 327773. PKCS8 import of EC keys isn't supported :(
Depends on: 327773
Maybe you could use JWK?  It looks like you might be able to do the same trick we do with RSA private keys (PK11_CreateGenericObject -> PK11_FindKeyByKeyID), populating a template including the public value in CKA_EC_POINT and the private value in CKA_VALUE.
Looking at JWK import for public keys, SECKEY_ImportDERPublicKey() does unfortunately not support ECDH. We might either patch that or create the SECKEYPublicKey ourselves and populate it with the right information.

That's what we currently do when importing private keys in JWK format IIUIC.
Good news. I was able to work around the shortcomings of NSS - looks like it only implements the minimum necessary ECDH pieces to get that working with TLS :/.

I have a messy patch that needs some cleanup, but it successfully derives the correct bytes from imported JWK keys as verified by using NIST test vectors.

Will update the patches asap.
Status: NEW → ASSIGNED
Attachment #8458620 - Attachment is obsolete: true
Attachment #8458621 - Attachment is obsolete: true
Attachment #8463898 - Flags: review?(rlb)
Oops.
Attachment #8463903 - Attachment is obsolete: true
Attachment #8463903 - Flags: review?(rlb)
Attachment #8463907 - Flags: review?(rlb)
Comment on attachment 8463898 [details] [diff] [review]
0001-Bug-1034855-Introduce-EcKeyAlgorithm.patch

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

You might want to add a test of structured cloning EC keys to catch the errors in WriteStructuredClone and ReadStructuredClone.

::: dom/crypto/EcKeyAlgorithm.cpp
@@ +19,5 @@
> +bool
> +EcKeyAlgorithm::WriteStructuredClone(JSStructuredCloneWriter* aWriter) const
> +{
> +  return WriteString(aWriter, mNamedCurve);
> +         WriteString(aWriter, mName);

You need to put an "&&" between these two lines.

@@ +28,5 @@
> +{
> +  nsString name;
> +  nsString namedCurve;
> +  bool read = ReadString(aReader, namedCurve);
> +              ReadString(aReader, name);

Likewise here.
Attachment #8463898 - Flags: review?(rlb) → review+
Comment on attachment 8463908 [details] [diff] [review]
0005-Bug-1034855-Implement-deriveKey-for-ECDH.patch

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

::: dom/crypto/WebCryptoTask.cpp
@@ +2458,5 @@
>      return mResult.SetLength(mLength) ? NS_OK : NS_ERROR_DOM_UNKNOWN_ERR;
>    }
>  };
>  
> +class DeriveEcdhKeyTask : public DeriveEcdhBitsTask

Noting that this is essentially the same code as DerivePbkdfKeyTask, I would prefer if you were to follow the pattern of ImportKeyTask and template this out.

template<class DeriveBitsTask>
class DeriveKeyTask : public DeriveBitsTask {
  DeriveKeyTask(..., ImportKeyTask* aTask)
}

That way, whenever you implement a new deriveBits() algorithm, you get the corresponding deriveKey() algorithm with just a couple lines to invoke the new template instance.

::: dom/crypto/test/tests.js
@@ +2092,5 @@
> +          if (!hasKeyFields(x)) {
> +            throw "Invalid key; missing field(s)";
> +          }
> +
> +          if (x.algorithm.length != 512) {

Since you're not explicitly indicating the key length, it might be helpful to have a comment reminding people that this is the default for HMAC-SHA-1
Attachment #8463908 - Flags: review?(rlb) → review-
Comment on attachment 8463900 [details] [diff] [review]
0003-Bug-1034855-Implement-deriveBits-for-ECDH.patch

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

::: dom/crypto/WebCryptoTask.cpp
@@ +2312,5 @@
> +      mEarlyRv = NS_ERROR_DOM_DATA_ERR;
> +      return;
> +    }
> +
> +    mLength = mLength >> 3; // bits to bytes

Little more readable to put this up by the length check above.

@@ +2344,5 @@
> +    if (mLength > mResult.Length()) {
> +      return NS_ERROR_DOM_DATA_ERR;
> +    }
> +
> +    return mResult.SetLength(mLength) ? NS_OK : NS_ERROR_DOM_UNKNOWN_ERR;

I have a slight preference for an actual if-statement here.

if (!mResult.SetLength(mLength)) {
  return NS_ERROR_DOM_UNKNOWN_ERR;
}
return NS_OK;

::: dom/crypto/test/tests.js
@@ +1836,5 @@
>  );
> +
> +// -----------------------------------------------------------------------------
> +TestArray.addTest(
> +  "Generate an ECDH key and derive some bits",

It seems like we're missing a test here that verifies that we're invoking the correct crypto, by comparing against known test vectors.  If you're having trouble finding test vectors, I would take a look at generating some with another crypto library, e.g., OpenSSL or PyCrypto.
Attachment #8463900 - Flags: review?(rlb) → review+
Comment on attachment 8463899 [details] [diff] [review]
0002-Bug-1034855-Implement-generateKey-for-ECDH.patch

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

::: dom/crypto/WebCryptoCommon.h
@@ +51,5 @@
>  
> +// WebCrypto named curves
> +#define WEBCRYPTO_NAMED_CURVE_P256  "p-256"
> +#define WEBCRYPTO_NAMED_CURVE_P384  "p-384"
> +#define WEBCRYPTO_NAMED_CURVE_P521  "p-521"

Please make these upper-case.  (See comments about caps in review of Bug 1037501)
https://bugzilla.mozilla.org/show_bug.cgi?id=1037501#c8

@@ +194,5 @@
> +{
> +  SECOidTag curveOIDTag;
> +
> +  if (aNamedCurve.EqualsLiteral(WEBCRYPTO_NAMED_CURVE_P256)) {
> +    curveOIDTag = SEC_OID_ANSIX962_EC_PRIME256V1;

Shouldn't this be SEC_OID_SECG_EC_SECP256R1?  We're looking for the NIST curves here, and according to fipstest.c (et al.), that's the OID that maps to nistp256.

::: dom/crypto/WebCryptoTask.cpp
@@ +2003,5 @@
>                 algName.EqualsLiteral(WEBCRYPTO_ALG_RSA_OAEP)) {
>        privateAllowedUsages = CryptoKey::DECRYPT | CryptoKey::UNWRAPKEY;
>        publicAllowedUsages = CryptoKey::ENCRYPT | CryptoKey::WRAPKEY;
> +    } else if (algName.EqualsLiteral(WEBCRYPTO_ALG_ECDH)) {
> +      privateAllowedUsages = CryptoKey::DERIVEKEY | CryptoKey::DERIVEBITS;

For clarity, you might explicitly set publicAllowedUsages=0.

@@ +2049,5 @@
>      ScopedPK11SlotInfo slot(PK11_GetInternalSlot());
>      MOZ_ASSERT(slot.get());
>  
>      void* param;
> +    if (mMechanism == CKM_RSA_PKCS_KEY_PAIR_GEN) {

Is there a reason for switching away from switch {} here?  I find switch slightly more readable.

::: dom/crypto/test/tests.js
@@ +1816,5 @@
> +    crypto.subtle.generateKey(alg, false, ["deriveKey", "deriveBits"]).then(
> +      complete(that, function(x) {
> +        return exists(x.publicKey) &&
> +               (x.publicKey.algorithm.name == alg.name) &&
> +               (x.publicKey.algorithm.namedCurve == alg.namedCurve.toLowerCase()) &&

See comments on casing.

@@ +1822,5 @@
> +               x.publicKey.extractable &&
> +               (x.publicKey.usages.length == 0) &&
> +               exists(x.privateKey) &&
> +               (x.privateKey.algorithm.name == alg.name) &&
> +               (x.privateKey.algorithm.namedCurve == alg.namedCurve.toLowerCase()) &&

Likewise.
Attachment #8463899 - Flags: review?(rlb) → review+
(In reply to Richard Barnes [:rbarnes] from comment #15)
> You might want to add a test of structured cloning EC keys to catch the
> errors in WriteStructuredClone and ReadStructuredClone.

As discussed on IRC: will open a follow-up and attach a patch there. Structured cloning doesn't currently work because pkcs8/spki isn't implemented for EC yet.

> > +  return WriteString(aWriter, mNamedCurve);
> > +         WriteString(aWriter, mName);
> 
> You need to put an "&&" between these two lines.

Oops, stupid mistake :|
Attachment #8463898 - Attachment is obsolete: true
Attachment #8464664 - Flags: review+
Comment on attachment 8463907 [details] [diff] [review]
0004-Bug-1034855-Implement-JWK-import-export-for-ECDH.patch

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

::: dom/crypto/CryptoKey.cpp
@@ +377,3 @@
>  
> +SECKEYPrivateKey*
> +PrivateKeyTemplateToPrivateKey(SECItem* aObjID,

PrivateKeyFromPrivateKeyTemplate (in all the other methods, the key type comes first)

@@ +563,5 @@
> +    default:
> +      return false;
> +  }
> +
> +  // No support for compressed points.

You might look to see if NSS has a method to uncompress points.  It's not complicated, but it's not trivial.

@@ +704,5 @@
> +        !aJwk.mY.WasPassed() || NS_FAILED(y.FromJwkBase64(aJwk.mY.Value()))) {
> +      return nullptr;
> +    }
> +
> +    ScopedSECKEYPublicKey key(PORT_ZNew(SECKEYPublicKey));

Please have Keeler review these NSS bits.  I don't know enough to make sure they're valid/safe.

::: dom/crypto/WebCryptoTask.cpp
@@ +1681,5 @@
> +
> +      mKey->SetPrivateKey(privKey.get());
> +      mKey->SetType(CryptoKey::PRIVATE);
> +
> +      pubKey = (SECKEYPublicKey*) PORT_ZNew(SECKEYPublicKey);

I don't think there's a need to compute the public key here (as in the RSA case).  In that case, the public key is only computed in order to get the modulusLength and publicExponent.  We don't need that here.

::: dom/crypto/test/test-vectors.js
@@ +488,5 @@
> +  ecdh_p521: {
> +    jwk_pub: {
> +      kty: "EC",
> +      crv: "P-521",
> +      x: "AeCLgRZ-BPqfhq4jt409-E26VHW5l29q74cHbIbQiS_-Gcqdo-087jHdPXUksGprNyp_RcTZd94t3peXzQziQIqo",

Break up for readability.
Attachment #8463907 - Flags: review?(rlb) → review+
(In reply to Richard Barnes [:rbarnes] from comment #18)
> > +#define WEBCRYPTO_NAMED_CURVE_P256  "p-256"
> > +#define WEBCRYPTO_NAMED_CURVE_P384  "p-384"
> > +#define WEBCRYPTO_NAMED_CURVE_P521  "p-521"
> 
> Please make these upper-case.  (See comments about caps in review of Bug
> 1037501)
> https://bugzilla.mozilla.org/show_bug.cgi?id=1037501#c8

Done. Introduced NormalizeNamedCurveValue() to normalize curve names like we do for algorithm names - that's why I'm asking for review again.

> > +  if (aNamedCurve.EqualsLiteral(WEBCRYPTO_NAMED_CURVE_P256)) {
> > +    curveOIDTag = SEC_OID_ANSIX962_EC_PRIME256V1;
> 
> Shouldn't this be SEC_OID_SECG_EC_SECP256R1?  We're looking for the NIST
> curves here, and according to fipstest.c (et al.), that's the OID that maps
> to nistp256.

#define SEC_OID_SECG_EC_SECP256R1 SEC_OID_ANSIX962_EC_PRIME256V1

But yeah, I use SEC_OID_SECG_EC_SECP256R1 now, that's in line with the other curve OIDs.

> > +    } else if (algName.EqualsLiteral(WEBCRYPTO_ALG_ECDH)) {
> > +      privateAllowedUsages = CryptoKey::DERIVEKEY | CryptoKey::DERIVEBITS;
> 
> For clarity, you might explicitly set publicAllowedUsages=0.

Done.

> > +    if (mMechanism == CKM_RSA_PKCS_KEY_PAIR_GEN) {
> 
> Is there a reason for switching away from switch {} here?  I find switch
> slightly more readable.

Done.
Attachment #8463899 - Attachment is obsolete: true
Attachment #8465835 - Flags: review?(rlb)
Comment on attachment 8465835 [details] [diff] [review]
0002-Bug-1034855-Implement-generateKey-for-ECDH.patch, v2

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

Thanks for the normalization stuff.  I think we've got a good design pattern going.
Attachment #8465835 - Flags: review?(rlb) → review+
(In reply to Richard Barnes [:rbarnes] from comment #17)
> It seems like we're missing a test here that verifies that we're invoking
> the correct crypto, by comparing against known test vectors.  If you're
> having trouble finding test vectors, I would take a look at generating some
> with another crypto library, e.g., OpenSSL or PyCrypto.

Included tests for NIST vectors with the JWK import/export patch because I needed to import them for testing :)
Attachment #8463900 - Attachment is obsolete: true
Attachment #8465982 - Flags: review+
(In reply to Richard Barnes [:rbarnes] from comment #20)
> @@ +563,5 @@
> > +    default:
> > +      return false;
> > +  }
> > +
> > +  // No support for compressed points.
> 
> You might look to see if NSS has a method to uncompress points.  It's not
> complicated, but it's not trivial.

NSS defines EC_POINT_FORM_UNCOMPRESSED and constants for the compressed formats but doesn't use them. It only checks for uncompressed points and doesn't support anything else. According to RFC 5480 (SPKI for EC): "Implementations MUST support the uncompressed form and MAY support the compressed form. The hybrid form MUST NOT be used".

> ::: dom/crypto/WebCryptoTask.cpp
> @@ +1681,5 @@
> > +
> > +      mKey->SetPrivateKey(privKey.get());
> > +      mKey->SetType(CryptoKey::PRIVATE);
> > +
> > +      pubKey = (SECKEYPublicKey*) PORT_ZNew(SECKEYPublicKey);
> 
> I don't think there's a need to compute the public key here (as in the RSA
> case).  In that case, the public key is only computed in order to get the
> modulusLength and publicExponent.  We don't need that here.

Ah, good point.
Attachment #8463907 - Attachment is obsolete: true
Attachment #8465987 - Flags: review?(dkeeler)
Sorry, forgot to fold the changes.
Attachment #8465987 - Attachment is obsolete: true
Attachment #8465987 - Flags: review?(dkeeler)
Attachment #8465989 - Flags: review?(dkeeler)
(In reply to Richard Barnes [:rbarnes] from comment #16)
> Noting that this is essentially the same code as DerivePbkdfKeyTask, I would
> prefer if you were to follow the pattern of ImportKeyTask and template this
> out.

Great idea, was thinking about reusing the code.

> > +          if (x.algorithm.length != 512) {
> 
> Since you're not explicitly indicating the key length, it might be helpful
> to have a comment reminding people that this is the default for HMAC-SHA-1

Done.
Attachment #8463908 - Attachment is obsolete: true
Attachment #8465998 - Flags: review?(rlb)
Comment on attachment 8465998 [details] [diff] [review]
0005-Bug-1034855-Implement-deriveKey-for-ECDH.patch, v2

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

Looks good.  Thanks for the refactor.  One question inline.

::: dom/crypto/WebCryptoTask.cpp
@@ +2343,3 @@
>      , mResolved(false)
>    {
> +    if (NS_FAILED(this->mEarlyRv)) {

Are the "this->" qualifiers necessary?  They don't seem to be in UnwrapKeyTask.
Attachment #8465998 - Flags: review?(rlb) → review+
Comment on attachment 8465989 [details] [diff] [review]
0004-Bug-1034855-Implement-JWK-import-export-for-ECDH-r-r.patch, v3

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

I mostly just reviewed NSS issues. In particular, I think it would be a good idea to never use operator new for SECItems (or any NSS types) and instead use ScopedPLArenaPool. This applies to other patches in this bug. I think it would be good to have another look at this after those changes, so r- for now.

::: dom/crypto/CryptoKey.cpp
@@ +352,5 @@
>    return NS_OK;
>  }
>  
> +bool
> +CreateECPointForCoordinates(const CryptoBuffer& aX,

If you pass in an arena and return a SECItem, there wouldn't be a need to allocate/memcpy twice in this function.

@@ +431,5 @@
> +      return nullptr;
> +    }
> +
> +    // Create parameters.
> +    ScopedSECItem params(new SECItem());

Rather than allocating SECItems individually (using operator new, which could potentially mix allocators/deallocators since SECITEM_FreeItem is what's going to clean up this ScopedSECItem), I would make a ScopedPLArenaPool and use that for any/all allocations. Then, when it goes out of scope, anything it allocated memory for gets cleaned up.
So, I would do something like this:

ScopedPLArenaPool arena(PORT_NewArena(DER_DEFAULT_CHUNKSIZE));
SECItem* params = CreateECParamsForCurve(namedCurve, arena.get());
if (!params) {
  return nullptr;
}

Then, use the arena to allocate and return a SECItem in CreateECParamsForCurve (this would get rid of the double allocate/memcpy as it currently is in the other patch).

@@ +462,5 @@
> +      { CKA_EC_POINT,         ecPoint->data,        ecPoint->len },
> +      { CKA_VALUE,            (void*) d.Elements(), d.Length() },
> +    };
> +
> +    return PrivateKeyFromPrivateKeyTemplate(objID, keyTemplate, 9);

PR_ARRAY_SIZE(keyTemplate) instead of 9

@@ +467,5 @@
> +  }
> +
> +  if (aJwk.mKty.Value().EqualsLiteral(JWK_TYPE_RSA)) {
> +    // Verify that all of the required parameters are present
> +    CryptoBuffer n, e, d, p, q, dp, dq, qi;

I'm assuming this is all essentially the same as it was in PrivateKeyFromJwk

@@ +482,5 @@
> +
> +    // Compute the ID for this key
> +    // This is generated with a SHA-1 hash, so unlikely to collide
> +    ScopedSECItem nItem(n.ToSECItem());
> +    ScopedSECItem objID(PK11_MakeIDFromPubKey(nItem.get()));

null-check nItem before calling PK11_MakeIDFromPubKey (it doesn't check its input, unfortunately)

@@ +506,5 @@
> +      { CKA_EXPONENT_2,       (void*) dq.Elements(), dq.Length() },
> +      { CKA_COEFFICIENT,      (void*) qi.Elements(), qi.Length() },
> +    };
> +
> +    return PrivateKeyFromPrivateKeyTemplate(objID, keyTemplate, 14);

PR_ARRAY_SIZE(keyTemplate) instead of 14

@@ +534,5 @@
>  
>    return true;
>  }
>  
> +bool ECKeyToJwk(const PK11ObjectType aKeyType, void *aKey,

nits: return value on previous line, "void* aKey"

@@ +558,5 @@
> +      flen = 384 >> 3;
> +      aRetVal.mCrv.Construct(NS_LITERAL_STRING(WEBCRYPTO_NAMED_CURVE_P384));
> +      break;
> +    case SEC_OID_SECG_EC_SECP521R1:
> +      flen = 521 >> 3;

Are there any kind of rounding issues here? 521 doesn't shift down by 3 without losing a bit...

@@ +627,5 @@
>        return NS_OK;
>      }
> +    case ecKey: {
> +      // Read EC params.
> +      ScopedSECItem params(new SECItem());

Same idea with allocating SECItems
Also, null-check params/other SECItems before using

::: dom/crypto/WebCryptoTask.cpp
@@ +1658,5 @@
> +public:
> +  ImportEcKeyTask(JSContext* aCx,
> +      const nsAString& aFormat,
> +      const ObjectOrString& aAlgorithm, bool aExtractable,
> +      const Sequence<nsString>& aKeyUsages)

nit: we could probably line these up with the JSContext on the first line, but it's not a big deal

@@ +1666,5 @@
> +
> +  ImportEcKeyTask(JSContext* aCx,
> +      const nsAString& aFormat, JS::Handle<JSObject*> aKeyData,
> +      const ObjectOrString& aAlgorithm, bool aExtractable,
> +      const Sequence<nsString>& aKeyUsages)

same here

@@ +1679,5 @@
> +
> +  void Init(JSContext* aCx,
> +      const nsAString& aFormat,
> +      const ObjectOrString& aAlgorithm, bool aExtractable,
> +      const Sequence<nsString>& aKeyUsages)

same

@@ +1692,5 @@
> +  nsString mNamedCurve;
> +
> +  virtual nsresult DoCrypto() MOZ_OVERRIDE
> +  {
> +    nsNSSShutDownPreventionLock locker;

I would put this closer to where it's used

@@ +1733,5 @@
> +  virtual nsresult AfterCrypto() MOZ_OVERRIDE
> +  {
> +    // Check permissions for the requested operation
> +    if ((mKey->GetKeyType() == CryptoKey::PRIVATE &&
> +         mKey->HasUsageOtherThan(CryptoKey::DERIVEBITS | CryptoKey::DERIVEKEY))) {

I think there's one more set of parentheses than necessary here

::: dom/crypto/test/tests.js
@@ +1999,5 @@
>    }
>  );
> +
> +// -----------------------------------------------------------------------------
> +TestArray.addTest(

How about some tests where importing fails because the input lacks the proper attributes, or they're malformed or something?
Attachment #8465989 - Flags: review?(dkeeler) → review-
(In reply to Richard Barnes [:rbarnes] from comment #27)
> ::: dom/crypto/WebCryptoTask.cpp
> @@ +2343,3 @@
> >      , mResolved(false)
> >    {
> > +    if (NS_FAILED(this->mEarlyRv)) {
> 
> Are the "this->" qualifiers necessary?  They don't seem to be in
> UnwrapKeyTask.

UnwrapKeyTask creates a new instance of the given template parameter in its constructor but DeriveKeyTask inherits from the given template param. The compiler complains that it can't resolve mEarlyRv etc. so I had to specify those by using this->.
Updated CreateECParamsForCurve() to take a *PLArenaPool.
Attachment #8465835 - Attachment is obsolete: true
Attachment #8466653 - Flags: review?(dkeeler)
(In reply to David Keeler (:keeler) [use needinfo?] from comment #28)
> > +bool
> > +CreateECPointForCoordinates(const CryptoBuffer& aX,
> 
> If you pass in an arena and return a SECItem, there wouldn't be a need to
> allocate/memcpy twice in this function.
> 
> So, I would do something like this:
> 
> ScopedPLArenaPool arena(PORT_NewArena(DER_DEFAULT_CHUNKSIZE));
> SECItem* params = CreateECParamsForCurve(namedCurve, arena.get());
> if (!params) {
>   return nullptr;
> }
> 
> Then, use the arena to allocate and return a SECItem in
> CreateECParamsForCurve (this would get rid of the double allocate/memcpy as
> it currently is in the other patch).

That's a great idea, done. I also added an OOM check for |arena| everywhere.

> > +    return PrivateKeyFromPrivateKeyTemplate(objID, keyTemplate, 9);
> 
> PR_ARRAY_SIZE(keyTemplate) instead of 9

Done.

> > +  if (aJwk.mKty.Value().EqualsLiteral(JWK_TYPE_RSA)) {
> > +    // Verify that all of the required parameters are present
> > +    CryptoBuffer n, e, d, p, q, dp, dq, qi;
> 
> I'm assuming this is all essentially the same as it was in PrivateKeyFromJwk

Yes.

> > +    ScopedSECItem nItem(n.ToSECItem());
> > +    ScopedSECItem objID(PK11_MakeIDFromPubKey(nItem.get()));
> 
> null-check nItem before calling PK11_MakeIDFromPubKey (it doesn't check its
> input, unfortunately)

Done.

> @@ +558,5 @@
> > +      flen = 384 >> 3;
> > +      aRetVal.mCrv.Construct(NS_LITERAL_STRING(WEBCRYPTO_NAMED_CURVE_P384));
> > +      break;
> > +    case SEC_OID_SECG_EC_SECP521R1:
> > +      flen = 521 >> 3;
> 
> Are there any kind of rounding issues here? 521 doesn't shift down by 3
> without losing a bit...

Good catch, thanks. I fixed that and and set |flen = 66| (the number of bytes) right away.

> > +// -----------------------------------------------------------------------------
> > +TestArray.addTest(
> 
> How about some tests where importing fails because the input lacks the
> proper attributes, or they're malformed or something?

Yeah, I missed that. Added some import tests that should fail.
Attachment #8465989 - Attachment is obsolete: true
Attachment #8466657 - Flags: review?(dkeeler)
We're getting SPKI import/export almost for free luckily.

FTR, I have another patch that implements PKCS8 import/export for ECDH keys but that is a little bigger, we have to come up with and fill the template ourselves because NSS doesn't support that at all. This would also include a test for structured cloning of EcKeyAlgorithms. I would like to move that to a follow-up because I expect that there will be some more discussion around this.
Attachment #8466988 - Flags: review?(rlb)
Attachment #8466988 - Flags: review?(dkeeler)
(In reply to Tim Taubert [:ttaubert] from comment #32)
> We're getting SPKI import/export almost for free luckily.

Looks like CERT_SubjectPublicKeyInfoTemplate doesn't exist on Windows because "DATA symbols on Windows because don't work right"? Would we need to create our own copy of the template if we want to use it then?
(In reply to Tim Taubert [:ttaubert] from comment #33)
> Looks like CERT_SubjectPublicKeyInfoTemplate doesn't exist on Windows
> because "DATA symbols on Windows because don't work right"? Would we need to
> create our own copy of the template if we want to use it then?

Does using SEC_ASN1_GET(CERT_SubjectPublicKeyInfoTemplate) work? (I think that mechanism was added as a workaround for exactly this problem.)
(In reply to David Keeler (:keeler) [use needinfo?] from comment #34)
> (In reply to Tim Taubert [:ttaubert] from comment #33)
> > Looks like CERT_SubjectPublicKeyInfoTemplate doesn't exist on Windows
> > because "DATA symbols on Windows because don't work right"? Would we need to
> > create our own copy of the template if we want to use it then?
> 
> Does using SEC_ASN1_GET(CERT_SubjectPublicKeyInfoTemplate) work? (I think
> that mechanism was added as a workaround for exactly this problem.)

Ah, I saw those NSS_GET_* functions but didn't know what they were for. Thanks! That builds fine on Windows now and I'll need those for the PKCS8 patches for sure.
Attachment #8466988 - Attachment is obsolete: true
Attachment #8466988 - Flags: review?(rlb)
Attachment #8466988 - Flags: review?(dkeeler)
Attachment #8467183 - Flags: review?(rlb)
Attachment #8467183 - Flags: review?(dkeeler)
Comment on attachment 8466653 [details] [diff] [review]
0002-Bug-1034855-Implement-generateKey-for-ECDH-r-rbarnes.patch, v3

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

Great

::: dom/crypto/WebCryptoCommon.h
@@ +234,5 @@
> +  }
> +
> +  // Set parameters.
> +  params->data[0] = SEC_ASN1_OBJECT_ID;
> +  params->data[1] = oidData->oid.len;

We should ensure that oidData->oid.len < 128 here, because otherwise this isn't a valid ASN1 encoding (this is probably guaranteed by that only a few specific OIDs are allowed, but best practices and all that...)
Attachment #8466653 - Flags: review?(dkeeler) → review+
Pushed to try with the Windows fix to make sure everything's still green after all the recent changes:

https://tbpl.mozilla.org/?tree=Try&rev=ab055aec1f66
Comment on attachment 8466657 [details] [diff] [review]
0004-Bug-1034855-Implement-JWK-import-export-for-ECDH-r-r.patch, v4

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

r=me with comments addressed (in particular, make sure the leaks in PrivateKeyToJwk are handled).

::: dom/crypto/CryptoKey.cpp
@@ +357,5 @@
> +                            const CryptoBuffer& aY,
> +                            PLArenaPool* aArena)
> +{
> +  // Create point.
> +  SECItem* point = ::SECITEM_AllocItem(aArena, nullptr, aX.Length() + aY.Length() + 1);

Looking at later code, it appears that aX.Length() should be the same as aY.Length() - should this be enforced in code here?

@@ +550,5 @@
> +  aRetVal.mY.Construct();
> +
> +  // Construct the OID tag.
> +  SECItem oid = { siBuffer, nullptr, 0 };
> +  oid.len = aEcParams->data[1];

We should probably check that aEcParams->data[0] == [the ASN.1 tag for OID] and that aEcParams->data[1] < 128

@@ +643,5 @@
> +      if (!params) {
> +        return NS_ERROR_DOM_OPERATION_ERR;
> +      }
> +
> +      if (PK11_ReadRawAttribute(PK11_TypePrivKey, aPrivKey, CKA_EC_PARAMS, params)) {

Turns out, I sort-of led you astray with the SECItems/PLArenaPools here. The issue now is that while the implementation could use the arena pointed to by params, it does not. This will result in some memory not getting freed. I would do this instead:

SECItem params;
if (PK11_ReadRawAttribute(..., &params)) {
...

PORT_Free(params.data);

(same idea with ecPoint)

::: dom/crypto/WebCryptoTask.cpp
@@ +1682,5 @@
> +            const Sequence<nsString>& aKeyUsages)
> +  {
> +    ImportKeyTask::Init(aCx, aFormat, aAlgorithm, aExtractable, aKeyUsages);
> +    if (NS_FAILED(mEarlyRv)) {
> +      return;

This seems unnecessary (the function returns after this regardless, right?)
Attachment #8466657 - Flags: review?(dkeeler) → review+
Comment on attachment 8467183 [details] [diff] [review]
0006-Bug-1034855-Implement-SPKI-import-export-for-ECDH.patch, v2

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

LGTM with comments addressed.

::: dom/crypto/CryptoKey.cpp
@@ +331,5 @@
> +    if (!oidData) {
> +      return nullptr;
> +    }
> +
> +    if (SECITEM_CopyItem(nullptr, &spki->algorithm.algorithm, &oidData->oid)) {

use != SECSuccess instead of implicitly converting from a SECStatus to a boolean value.
Also, spki->algorithm.algorithm should be SECITEM_FreeItem'd before being copied over.

@@ +366,5 @@
> +  // Per WebCrypto spec we must export ECDH SPKIs with the algorithm OID
> +  // id-ecDH (1.3.132.112). NSS doesn't know about that OID and there is
> +  // no way to specify the algorithm to use when exporting a public key.
> +  if (aPubKey->keyType == ecKey) {
> +    if (SECITEM_CopyItem(nullptr, &spki->algorithm.algorithm, &SEC_OID_DATA_EC_DH)) {

Same comments as above

::: dom/crypto/WebCryptoCommon.h
@@ +93,5 @@
>  // Define an unknown mechanism type
>  #define UNKNOWN_CK_MECHANISM        CKM_VENDOR_DEFINED+1
>  
> +// NSS doesn't know about id-ecDH (1.3.132.112), define it.
> +const unsigned char SEC_OID_EC_DH[] = { 0x2b, 0x81, 0x04, 0x70 };

Can you use security/pkix/tools/DottedOIDToCode.py for this?

::: dom/crypto/WebCryptoTask.cpp
@@ +1742,5 @@
>  
> +      if (mFormat.EqualsLiteral(WEBCRYPTO_KEY_FORMAT_SPKI)) {
> +        // Construct the OID tag.
> +        SECItem oid = { siBuffer, nullptr, 0 };
> +        oid.len = pubKey->u.ec.DEREncodedParams.data[1];

Again, I think we should check that this data is sane.
* pubKey->u.ec.DEREncodedParams.len > 1
* pubKey->u.ec.DEREncodedParams.data[0] == ASN1 OID tag
* pubKey->u.ec.DEREncodedParams.data[1] < 128
* pubKey->u.ec.DEREncodedParams.len == pubKey->u.ec.DEREncodedParams.data[1] + 2
Attachment #8467183 - Flags: review?(dkeeler) → review+
(In reply to David Keeler (:keeler) [use needinfo?] from comment #36)
> > +  params->data[0] = SEC_ASN1_OBJECT_ID;
> > +  params->data[1] = oidData->oid.len;
> 
> We should ensure that oidData->oid.len < 128 here, because otherwise this
> isn't a valid ASN1 encoding (this is probably guaranteed by that only a few
> specific OIDs are allowed, but best practices and all that...)

Done.
Attachment #8466653 - Attachment is obsolete: true
Attachment #8467579 - Flags: review+
(In reply to David Keeler (:keeler) [use needinfo?] from comment #38)
> > +  SECItem* point = ::SECITEM_AllocItem(aArena, nullptr, aX.Length() + aY.Length() + 1);
> 
> Looking at later code, it appears that aX.Length() should be the same as
> aY.Length() - should this be enforced in code here?

Done.

> > +  SECItem oid = { siBuffer, nullptr, 0 };
> > +  oid.len = aEcParams->data[1];
> 
> We should probably check that aEcParams->data[0] == [the ASN.1 tag for OID]
> and that aEcParams->data[1] < 128

Done.

> > +      if (PK11_ReadRawAttribute(PK11_TypePrivKey, aPrivKey, CKA_EC_PARAMS, params)) {
> 
> Turns out, I sort-of led you astray with the SECItems/PLArenaPools here. The
> issue now is that while the implementation could use the arena pointed to by
> params, it does not. This will result in some memory not getting freed. I
> would do this instead:
> 
> SECItem params;
> if (PK11_ReadRawAttribute(..., &params)) {
> ...
> 
> PORT_Free(params.data);

Yeah, I remember that I realized that those SECItems won't be allocated in that arena while working on it but looks like I forgot to fix that. I removed the arena and did the following:

ScopedSECItem params(::SECITEM_AllocItem(nullptr, nullptr, 0));
SECStatus rv = PK11_ReadRawAttribute(PK11_TypePrivKey, aPrivKey, CKA_EC_PARAMS, params);
if (rv != SECSuccess) { ... }

That should properly clean up the data allocated by PK11_ReadRawAttribute(), right? Calling PORT_Free() in all the branches that could return early seems tedious.

(I also seized the chance to properly check the return status.)

> > +    ImportKeyTask::Init(aCx, aFormat, aAlgorithm, aExtractable, aKeyUsages);
> > +    if (NS_FAILED(mEarlyRv)) {
> > +      return;
> 
> This seems unnecessary (the function returns after this regardless, right?)

It is indeed, that was left over from when the function did more than this. Thanks for spotting it.
Attachment #8466657 - Attachment is obsolete: true
Attachment #8467584 - Flags: review+
(In reply to David Keeler (:keeler) [use needinfo?] from comment #39)
> > +    if (SECITEM_CopyItem(nullptr, &spki->algorithm.algorithm, &oidData->oid)) {
> 
> use != SECSuccess instead of implicitly converting from a SECStatus to a
> boolean value.

Done.

> Also, spki->algorithm.algorithm should be SECITEM_FreeItem'd before being
> copied over.

I think we don't need to take care of that SECItem here as it will be free'd together with its arena that was created in SECKEY_DecodeDERSubjectPublicKeyInfo().

I however changed the code to SECITEM_CopyItem(spki->arena, ...) so that the new copy of SEC_OID_DATA_EC_DH is free'd when we're out of scope as well.

> > +  if (aPubKey->keyType == ecKey) {
> > +    if (SECITEM_CopyItem(nullptr, &spki->algorithm.algorithm, &SEC_OID_DATA_EC_DH)) {
> 
> Same comments as above

I've done the same here: SECITEM_CopyItem(spki->arena, ...) to make sure that new copy of the SECItem is destroyed when spki is.

> > +// NSS doesn't know about id-ecDH (1.3.132.112), define it.
> > +const unsigned char SEC_OID_EC_DH[] = { 0x2b, 0x81, 0x04, 0x70 };
> 
> Can you use security/pkix/tools/DottedOIDToCode.py for this?

Done.

> > +      if (mFormat.EqualsLiteral(WEBCRYPTO_KEY_FORMAT_SPKI)) {
> > +        // Construct the OID tag.
> > +        SECItem oid = { siBuffer, nullptr, 0 };
> > +        oid.len = pubKey->u.ec.DEREncodedParams.data[1];
> 
> Again, I think we should check that this data is sane.
> * pubKey->u.ec.DEREncodedParams.len > 1
> * pubKey->u.ec.DEREncodedParams.data[0] == ASN1 OID tag
> * pubKey->u.ec.DEREncodedParams.data[1] < 128
> * pubKey->u.ec.DEREncodedParams.len == pubKey->u.ec.DEREncodedParams.data[1]
> + 2

I'll combine all the sanity checking and introduce a function called CheckEncodedECParameters() in patch #2 for .generateKey() and use it here and in the other patches.
Attachment #8467183 - Attachment is obsolete: true
Attachment #8467183 - Flags: review?(rlb)
Attachment #8467611 - Flags: review?(rlb)
Adding CheckEncodedECParameters() and using that in CreateECParamsForCurve().
Attachment #8467579 - Attachment is obsolete: true
Attachment #8467612 - Flags: review+
Using CheckEncodedECParameters() in ECKeyToJwk() now.
Attachment #8467584 - Attachment is obsolete: true
Attachment #8467614 - Flags: review+
Comment on attachment 8467611 [details] [diff] [review]
0006-Bug-1034855-Implement-SPKI-import-export-for-ECDH-r-.patch, v3

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

LGTM.

::: dom/crypto/CryptoKey.cpp
@@ +323,5 @@
>    }
>  
> +  // Check for id-ecDH. Per the WebCrypto spec we must support it but NSS
> +  // does unfortunately not know about it. Let's change the algorithm to
> +  // id-ecPublicKey to make NSS happy.

Open a bug to fix this in NSS?
Attachment #8467611 - Flags: review?(rlb) → review+
Depends on: 1048914
(In reply to Richard Barnes [:rbarnes] from comment #45)
> > +  // Check for id-ecDH. Per the WebCrypto spec we must support it but NSS
> > +  // does unfortunately not know about it. Let's change the algorithm to
> > +  // id-ecPublicKey to make NSS happy.
> 
> Open a bug to fix this in NSS?

Filed bug 1048914.
Depends on: 1048931
Comment on attachment 8465982 [details] [diff] [review]
0003-Bug-1034855-Implement-deriveBits-for-ECDH-r-rbarnes.patch, v2

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

I forgot about our beloved WebIDL hook :( Olli, can you please take a look? The changes are really simple. Thanks!
Attachment #8465982 - Flags: review+ → review?(bugs)
Attachment #8464664 - Flags: review+ → review?(bugs)
Comment on attachment 8464664 [details] [diff] [review]
0001-Bug-1034855-Introduce-EcKeyAlgorithm-r-rbarnes.patch, v2

r+ for the .webidl changes.
Attachment #8464664 - Flags: review?(bugs) → review+
Comment on attachment 8465982 [details] [diff] [review]
0003-Bug-1034855-Implement-deriveBits-for-ECDH-r-rbarnes.patch, v2

ditto.
Attachment #8465982 - Flags: review?(bugs) → review+
Thanks for the fast reviews!
Sorry for that. I pushed to try a few times but running LSAN builds did unfortunately not occur to me.
Simple fix, the ScopedSECKEYPublicKey only clears its arena when it goes out of scope, so we better use PORT_ArenaZNew() with the arena we create in PublicKeyFromJwk() anyway.

https://tbpl.mozilla.org/?tree=Try&rev=8ecc23e3d187
Keywords: relnote


Added to the release notes with "WebCrypto: RSA-OAEP, PBKDF2, ECDH and AES-KW support" as wording.
bug 1021607 + bug 1034852 + bug 1026398
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.