998471 - Add support for key generation to WebCrypto API

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Richard Barnes [:rbarnes]]

Attachment: webcrypto-998471.patch

Note: Assumes Bug 995385

Comment 2

[Richard Barnes [:rbarnes]]

Attachment: webcrypto-998471.1.patch

Comment 3

[Camilo Viecco (:cviecco)]

Comment on attachment 8409962 [details] [diff] [review]
webcrypto-998471.1.patch

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

r+ with comments addressed. (still needs a real psm peer review)

::: dom/crypto/WebCryptoTask.cpp
@@ +522,5 @@
> +        // TODO fix error and handle default values
> +        mEarlyRv = NS_ERROR_DOM_SYNTAX_ERR;
> +      }
> +
> +      // Pull relevant info

Why dont we do some sanity cheks here for the exponent and key size?

::: dom/crypto/test/tests.js
@@ +262,5 @@
> +  function() {
> +    var that = this;
> +    var alg = {
> +      name: "RSAES-PKCS1-v1_5",
> +      modulusLength: 1024,

I really think wekcrypto should not allow to generate weak keys. 1024 is considered bad, wh not make it min 2048?

@@ +263,5 @@
> +    var that = this;
> +    var alg = {
> +      name: "RSAES-PKCS1-v1_5",
> +      modulusLength: 1024,
> +      publicExponent: new Uint8Array([0x01, 0x00, 0x01])

and the exponent should not be 1

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8409962 [details] [diff] [review]
webcrypto-998471.1.patch

>+++ b/dom/bindings/Bindings.conf
>+    'headerFile': 'KeyPair.h',

Not needed, since it's in mozilla/dom

>+++ b/dom/crypto/CryptoBuffer.cpp
>+  if (!aItem) { return; }

Shouldn't this clear out the current contents of the buffer?

In any case, body on separate line, please.

>+++ b/dom/crypto/KeyPair.cpp
>+#include "KeyPair.h"

mozilla/dom

>+NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(KeyPair)

You need to CC mPublicKey and mPrivateKey

>+++ b/dom/crypto/KeyPair.h
>+#ifndef mozilla_dom_KeyPair_h__

Nix the __.

>+#include "mozilla/ErrorResult.h"

Not needed.

>+struct JSContext;

js/TypeDecls.h

>+  // TODO: return something sensible here, and change the return type

Yes.

>+  already_AddRefed<Key> PublicKey() const
>+  {
>+    nsRefPtr<Key> ret = mPublicKey;

  Key* PublicKey() const 
  {
    return mPublicKey;
  }

and similar for the private key.

>+  // Convenience methods for C++ access to key contents
>+  Key* PublicKeyPtr() { return mPublicKey.get(); }
>+  Key* PrivateKeyPtr() { return mPrivateKey.get(); }

And you don't need those.

>+++ b/dom/crypto/WebCryptoTask.cpp
>+class GenerateSymmetricKeyTask : public WebCryptoTask
>+  GenerateSymmetricKeyTask(JSContext* aCx,
>+    for (uint32_t i=0; i<aKeyUsages.Length(); ++i) {

Spaces around the '=' and '<', please.

>+      mKey->SetUsage(aKeyUsages[i]);

I assume you plan to update this to compile... ;)

>+    KeyAlgorithm* algorithm;

Make this an nsRefPtr, please, to make it simpler to reason about lifetimes.

>+      if ( (mLength != 128) && (mLength != 192) && (mLength != 256) ) {

This is overparenthesized.  Please remove the extra parens.

>+      HmacKeyGenParams params;

Needs to be a RootedDictionary.

>+      if (!params.mHash.WasPassed()) {

Can't happen.  You just checked this a few lines above.

>+    mLength = mLength >> 3; // bits to bytes

I'm taking on faith that these parts are correct and got review from the crypto folks.

>+  virtual nsresult DoCrypto() MOZ_OVERRIDE
>+    if (!symKey) { return NS_ERROR_DOM_UNKNOWN_ERR; }

Body on new line.

>+    if (NS_FAILED(rv)) { return NS_ERROR_DOM_UNKNOWN_ERR; }

Likewise.

>+
>+    // This doesn't leak, because the SECItem* returned by PK11_GetKeyData
>+    // is internal to symKey, and managed there
>+    mKeyData = PK11_GetKeyData(symKey);

I don't see how.  mKeyData just makes a copy of the data in the SECItem*, no?  So I think this does in fact leak.

>+  virtual void Cleanup() {
>+    mKey.forget();

  mKey = nullptr;

We need 

>+class GenerateAsymmetricKeyTask : public WebCryptoTask
>+  GenerateAsymmetricKeyTask(JSContext* aCx,

>+    // TODO: Handle key usages

Please make sure a followup is filed and blocking whatever our "turn this stuff on by default" bit is.  Cite the bug number here.

>+    KeyAlgorithm* algorithm;

nsRefPtr, please.

>+      RsaHashedKeyGenParams params;

RootedDictionary.

>+        // TODO fix error and handle default values

Cite the bug number.

>+      CryptoBuffer publicExponent(params.mPublicExponent.Value().Obj());

Nix the .Obj().

>+        // TODO fix error and handle default values

File+cite bug.

>+      mKeyPair->PublicKeyPtr()->SetAlgorithm(algorithm);
>+      mKeyPair->PrivateKeyPtr()->SetAlgorithm(algorithm);
>+      mMechanism = CKM_RSA_PKCS_KEY_PAIR_GEN;
>+
>+      // Set up params struct
>+      mRsaParams.keySizeInBits = modulusLength;
>+      bool converted = publicExponent.GetBigIntValue(mRsaParams.pe);
>+      if (!converted) {
>+        mEarlyRv = NS_ERROR_DOM_INVALID_ACCESS_ERR;
>+        return;
>+      }

All that is the same in both branches here.  Worth sharing the code instead of copy/pasting it?

>+  virtual void Cleanup() MOZ_OVERRIDE
>+    mKeyPair.forget();

  = nullptr.

Need to also clean up the NSS bits in the relevant callback, right?

> WebCryptoTask* WebCryptoTask::GenerateKeyTask(JSContext* aCx,
>+  } else if (algName.EqualsLiteral(WEBCRYPTO_ALG_RSAES_PKCS1) ||
>+      algName.EqualsLiteral(WEBCRYPTO_ALG_RSASSA_PKCS1)) {

Weird indent.

You need to adjust test_interfaces.html as well by adding KeyPair there.

r=me with those fixed.

Comment 5

[Richard Barnes [:rbarnes]]

Most things addressed in forthcoming patch.  Couple of comments below.

(In reply to Boris Zbarsky [:bz] from comment #4)
> Comment on attachment 8409962 [details] [diff] [review]
> webcrypto-998471.1.patch
> 
> >+      HmacKeyGenParams params;
> 
> Needs to be a RootedDictionary.

I've added RootedDictionary to all the Coerce targets, because I'm not clear on when it's required and when it isn't, and it's harmless, right?

 
> >+    // This doesn't leak, because the SECItem* returned by PK11_GetKeyData
> >+    // is internal to symKey, and managed there
> >+    mKeyData = PK11_GetKeyData(symKey);
> 
> I don't see how.  mKeyData just makes a copy of the data in the SECItem*,
> no?  So I think this does in fact leak.

The SECItem* returned by PK11_GetKeyData refers to a buffer that is managed by symKey (thus ScopedPK11SymKey).  So there's no new buffer to manage on that side of the equation.  The assignment does copy to mKeyData, but CryptoBuffer manages that buffer internally.  I added a comment to clarify.

 
> >+      mKeyPair->PublicKeyPtr()->SetAlgorithm(algorithm);
> >+      mKeyPair->PrivateKeyPtr()->SetAlgorithm(algorithm);
> >+      mMechanism = CKM_RSA_PKCS_KEY_PAIR_GEN;
> >+
> >+      // Set up params struct
> >+      mRsaParams.keySizeInBits = modulusLength;
> >+      bool converted = publicExponent.GetBigIntValue(mRsaParams.pe);
> >+      if (!converted) {
> >+        mEarlyRv = NS_ERROR_DOM_INVALID_ACCESS_ERR;
> >+        return;
> >+      }
> 
> All that is the same in both branches here.  Worth sharing the code instead
> of copy/pasting it?

The reason for the duplication is that we expect to have other, non-RSA forms of keys handled here, namely EC and DH.  Those *won't* have those bits in common, so I've left them in the algorithm-specifics.

Comment 6

[Richard Barnes [:rbarnes]]

Attachment: webcrypto-998471.2.patch r=bz,cviecco

Updated patch addressing review comments.

Comment 7

[Richard Barnes [:rbarnes]]

Attachment: webcrypto-998471.2.patch

Comment 8

[Boris Zbarsky [:bzbarsky]]

> I've added RootedDictionary to all the Coerce targets, because I'm not clear on when it's
> required and when it isn't, and it's harmless, right?

It's required when the dictionary contains a gcthing (equivalently when the static analysis goes orange on tinderbox).  But yes, adding it to all Coerce stuff is not a problem; it's just a little tiny bit slower, but on the scale of the work Coerce is doing that's not a big deal.

Comment 9

[Richard Barnes [:rbarnes]]

Attachment: webcrypto-998471.3.patch r=bz,dkeeler

Updated patch addressing review comments.

Happy try run covering latest patches for 998802, 998803, 998471 here:
https://tbpl.mozilla.org/?tree=Try&rev=e607e1f42a20

Comment 10

[Carsten Book [:Tomcat]]

In the try run you mentioned is a bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=40295974&tree=Try - not sure which one of the 3 cause this but i guess this has to be fixed first before we can do the checkin.

Comment 11

[Richard Barnes [:rbarnes]]

Attachment: webcrypto-998471.4.patch r=bz,dkeeler

Missed an #include, and it was masked by UNIFIED_SOURCES.  Looking better now:
https://tbpl.mozilla.org/?tree=Try&rev=de75a4759611

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b98b158aee4

Comment 13

[Boris Zbarsky [:bzbarsky]]

Er... That Assign() template wasn't in the patch I reviewed.  Where did it come from and why?

Also, the patch as landed still has those "TODO" comments without bug numbers.  Did those bugs get filed?

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5b98b158aee4

Comment 15

[Richard Barnes [:rbarnes]]

(In reply to Boris Zbarsky [:bz] from comment #13)
> Er... That Assign() template wasn't in the patch I reviewed.  Where did it
> come from and why?

Sorry about that.  It was needed to convert the publicExponent field from RsaKeyGenParams to a CryptoBuffer, which has type Uint8Array.  Confusingly (to me at least), the ArrayBufferView object in WebIDL is not a parent class of Uint8Array.  So the ArrayBufferView version of Assign() is insufficient.


> Also, the patch as landed still has those "TODO" comments without bug
> numbers.  Did those bugs get filed?

TODOs were bogus.

Comment 16

[Richard Barnes [:rbarnes]]

(In reply to Richard Barnes [:rbarnes] from comment #15)
> (In reply to Boris Zbarsky [:bz] from comment #13)
> > Er... That Assign() template wasn't in the patch I reviewed.  Where did it
> > come from and why?
> 
> Sorry about that.  It was needed to convert the publicExponent field from
> RsaKeyGenParams to a CryptoBuffer, which has type Uint8Array.  Confusingly
> (to me at least), the ArrayBufferView object in WebIDL is not a parent class
> of Uint8Array.  So the ArrayBufferView version of Assign() is insufficient.
> 
> 
> > Also, the patch as landed still has those "TODO" comments without bug
> > numbers.  Did those bugs get filed?
> 
> TODOs were bogus.

Pressed "Save" too soon.  Should add: 
1. "Bogus" in the sense that there's nothing to do.  (There are no defaults, and the error is correct.)
2. I went ahead and removed them in the patch for Bug 1018446.