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

      No description provided.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 3 • 10 years ago

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

	Richard Barnes [:rbarnes] Reporter
	Comment 4 • 10 years ago

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

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 5 • 10 years ago

Looks like this is blocked by bug 327773. PKCS8 import of EC keys isn't supported :(

	Richard Barnes [:rbarnes] Reporter
	Comment 6 • 10 years ago

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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 7 • 10 years ago

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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 8 • 10 years ago

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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 13 • 10 years ago

Oops.

	Richard Barnes [:rbarnes] Reporter
	Comment 15 • 10 years ago

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.

	Richard Barnes [:rbarnes] Reporter
	Comment 16 • 10 years ago

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

	Richard Barnes [:rbarnes] Reporter
	Comment 17 • 10 years ago

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.

	Richard Barnes [:rbarnes] Reporter
	Comment 18 • 10 years ago

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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 19 • 10 years ago

(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 :|

	Richard Barnes [:rbarnes] Reporter
	Comment 20 • 10 years ago

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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 21 • 10 years ago

(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.

	Richard Barnes [:rbarnes] Reporter
	Comment 22 • 10 years ago

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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 23 • 10 years ago

(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 :)

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 24 • 10 years ago

(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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 25 • 10 years ago

Sorry, forgot to fold the changes.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 26 • 10 years ago

(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.

	Richard Barnes [:rbarnes] Reporter
	Comment 27 • 10 years ago

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.

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

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?

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 29 • 10 years ago

(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->.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 30 • 10 years ago

Updated CreateECParamsForCurve() to take a *PLArenaPool.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 31 • 10 years ago

(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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 32 • 10 years ago

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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 33 • 10 years ago

(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?

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

(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.)

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 35 • 10 years ago

(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.

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

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...)

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 37 • 10 years ago

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

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

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?)

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

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

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 40 • 10 years ago

(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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 41 • 10 years ago

(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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 42 • 10 years ago

(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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 43 • 10 years ago

Adding CheckEncodedECParameters() and using that in CreateECParamsForCurve().

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 44 • 10 years ago

Using CheckEncodedECParameters() in ECKeyToJwk() now.

	Richard Barnes [:rbarnes] Reporter
	Comment 45 • 10 years ago

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?

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 46 • 10 years ago

(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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 47 • 10 years ago

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!

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 48 • 10 years ago

Comment on attachment 8464664 [details] [diff] [review]
0001-Bug-1034855-Introduce-EcKeyAlgorithm-r-rbarnes.patch, v2

r+ for the .webidl changes.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 49 • 10 years ago

Comment on attachment 8465982 [details] [diff] [review]
0003-Bug-1034855-Implement-deriveBits-for-ECDH-r-rbarnes.patch, v2

ditto.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 50 • 10 years ago

Thanks for the fast reviews!

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 51 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/50b8670329aa
https://hg.mozilla.org/integration/mozilla-inbound/rev/33510bb7ad60
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4f5f5b8421d
https://hg.mozilla.org/integration/mozilla-inbound/rev/4af8993f1534
https://hg.mozilla.org/integration/mozilla-inbound/rev/4eb0ee62db1e
https://hg.mozilla.org/integration/mozilla-inbound/rev/0dc569f74991

	Ryan VanderMeulen [:RyanVM]
	Comment 52 • 10 years ago

Backed out for LSAN leaks.
https://hg.mozilla.org/integration/mozilla-inbound/rev/591fb2c4ee15

https://tbpl.mozilla.org/php/getParsedLog.php?id=45259594&tree=Mozilla-Inbound

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 53 • 10 years ago

Sorry for that. I pushed to try a few times but running LSAN builds did unfortunately not occur to me.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 54 • 10 years ago

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

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 55 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ef5bdb483f9
https://hg.mozilla.org/integration/mozilla-inbound/rev/8305c22206c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b61628b375c
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e38ab4b2b00
https://hg.mozilla.org/integration/mozilla-inbound/rev/46fd409366ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0c7741f70b6

	Nigel Babu [:nigelb]
	Comment 56 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/1ef5bdb483f9
https://hg.mozilla.org/mozilla-central/rev/8305c22206c2
https://hg.mozilla.org/mozilla-central/rev/2b61628b375c
https://hg.mozilla.org/mozilla-central/rev/9e38ab4b2b00
https://hg.mozilla.org/mozilla-central/rev/46fd409366ca
https://hg.mozilla.org/mozilla-central/rev/d0c7741f70b6

	Sylvestre Ledru [:Sylvestre]
	Comment 57 • 10 years ago

Added to the release notes with "WebCrypto: RSA-OAEP, PBKDF2, ECDH and AES-KW support" as wording.
bug 1021607 + bug 1034852 + bug 1026398

	Sylvestre Ledru [:Sylvestre]
	Comment 58 • 10 years ago

34, my bad!