Closed
Bug 998471
Opened 10 years ago
Closed 10 years ago
Add support for key generation to WebCrypto API
Categories
(Core :: DOM: Security, defect)
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: rbarnes, Assigned: rbarnes)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 5 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•10 years ago
|
Blocks: web-crypto
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → rlb
Assignee | ||
Comment 1•10 years ago
|
||
Note: Assumes Bug 995385
Attachment #8409518 -
Flags: review?(dkeeler)
Attachment #8409518 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8409962 -
Flags: review?(cviecco)
Attachment #8409962 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•10 years ago
|
Attachment #8409518 -
Attachment is obsolete: true
Attachment #8409518 -
Flags: review?(dkeeler)
Attachment #8409518 -
Flags: review?(bzbarsky)
Comment 3•10 years ago
|
||
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 4•10 years ago
|
||
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+
Assignee | ||
Comment 5•10 years ago
|
||
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.
Assignee | ||
Comment 6•10 years ago
|
||
Updated patch addressing review comments.
Attachment #8409962 -
Attachment is obsolete: true
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8427803 -
Attachment is obsolete: true
Comment 8•10 years ago
|
||
> 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.
Assignee | ||
Comment 9•10 years ago
|
||
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
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 10•10 years ago
|
||
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
Assignee | ||
Comment 11•10 years ago
|
||
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
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 12•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b98b158aee4
Keywords: checkin-needed
Comment 13•10 years ago
|
||
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)
Comment 14•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5b98b158aee4
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Assignee | ||
Comment 15•10 years ago
|
||
(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)
Assignee | ||
Comment 16•10 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•