Closed Bug 998471 Opened 7 years ago Closed 7 years ago

Add support for key generation to WebCrypto API

Categories

(Core :: DOM: Security, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: rbarnes, Assigned: rbarnes)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 5 obsolete files)

No description provided.
Blocks: web-crypto
Assignee: nobody → rlb
Attached patch webcrypto-998471.patch (obsolete) — Splinter Review
Note: Assumes Bug 995385
Attachment #8409518 - Flags: review?(dkeeler)
Attachment #8409518 - Flags: review?(bzbarsky)
Attached patch webcrypto-998471.1.patch (obsolete) — Splinter Review
Attachment #8409962 - Flags: review?(cviecco)
Attachment #8409962 - Flags: review?(bzbarsky)
Attachment #8409518 - Attachment is obsolete: true
Attachment #8409518 - Flags: review?(dkeeler)
Attachment #8409518 - Flags: review?(bzbarsky)
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
Attachment #8409962 - Flags: review?(cviecco) → review+
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.
Attachment #8409962 - Flags: review?(bzbarsky) → review+
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.
Updated patch addressing review comments.
Attachment #8409962 - Attachment is obsolete: true
Attached patch webcrypto-998471.2.patch (obsolete) — Splinter Review
Attachment #8427803 - Attachment is obsolete: true
> 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.
Updated patch addressing review comments.

Happy try run covering latest patches for 998802, 998803, 998471 here:
https://tbpl.mozilla.org/?tree=Try&rev=e607e1f42a20
Attachment #8427806 - Attachment is obsolete: true
Keywords: checkin-needed
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.
Keywords: checkin-needed
Missed an #include, and it was masked by UNIFIED_SOURCES.  Looking better now:
https://tbpl.mozilla.org/?tree=Try&rev=de75a4759611
Attachment #8428086 - Attachment is obsolete: true
Keywords: checkin-needed
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?
Flags: needinfo?(rlb)
https://hg.mozilla.org/mozilla-central/rev/5b98b158aee4
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
(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.
Flags: needinfo?(rlb)
(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.
Depends on: 1033492
You need to log in before you can comment on or make changes to this bug.