Closed
Bug 1034854
Opened 10 years ago
Closed 9 years ago
Add support for ECDSA to WebCrypto API
Categories
(Core :: DOM: Security, defect)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: rbarnes, Assigned: rbarnes)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-needed)
Attachments
(1 file, 2 obsolete files)
31.33 KB,
patch
|
bzbarsky
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → rlb
Assignee | ||
Comment 1•9 years ago
|
||
This is going to want to wait for Bug 1037892 and Bug 1057161 to land, but I think it's mostly complete, modulo rebasing on top of those.
Assignee | ||
Updated•9 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•9 years ago
|
||
This patch rebases the earlier WIP on top of Bug 1037892 and Bug 1057161. The main interesting things are: (1) the PublicKeyValid() method, which tests for a public key's validity by using the check added in Bug 1057161, and (2) the transformation of RsassaPkcs1Task to a more generic AsymmetricVerifyTask, adding ECDSA and simplifying the NSS verification calls.
Attachment #8484213 -
Attachment is obsolete: true
Attachment #8499102 -
Flags: review?(ttaubert)
Attachment #8499102 -
Flags: review?(dkeeler)
Comment on attachment 8499102 [details] [diff] [review] bug-1034854.1.patch Review of attachment 8499102 [details] [diff] [review]: ----------------------------------------------------------------- NSS-related things look good to me. ::: dom/crypto/CryptoKey.cpp @@ +898,5 @@ > + if (!slot.get()) { > + return false; > + } > + > + CK_OBJECT_HANDLE id = PK11_ImportPublicKey(slot, aPubKey, PR_FALSE); Maybe we should add a comment that this code assumes that if calling PK11_ImportPublicKey and checking for a valid handle succeeds, the key is valid. ::: dom/crypto/WebCryptoTask.cpp @@ +64,5 @@ > TA_SHA_384 = 17, > TA_SHA_512 = 18, > // Later additions > TA_AES_KW = 19, > + TA_ECDSA = 20 nit: include the trailing ',' so we don't have to touch this line if/when we add something else @@ +997,5 @@ > + case CKM_SHA384: > + mOidTag = SEC_OID_PKCS1_SHA384_WITH_RSA_ENCRYPTION; break; > + case CKM_SHA512: > + mOidTag = SEC_OID_PKCS1_SHA512_WITH_RSA_ENCRYPTION; break; > + default: { are the braces necessary here? @@ +1037,5 @@ > + case CKM_SHA384: > + mOidTag = SEC_OID_ANSIX962_ECDSA_SHA384_SIGNATURE; break; > + case CKM_SHA512: > + mOidTag = SEC_OID_ANSIX962_ECDSA_SHA512_SIGNATURE; break; > + default: { same with the braces @@ +1069,5 @@ > virtual nsresult DoCrypto() MOZ_OVERRIDE > { > nsresult rv; > if (mSign) { > ScopedSECItem signature((SECItem*) PORT_Alloc(sizeof(SECItem))); we should probably null-check signature here @@ +1081,5 @@ > + > + if (mEcdsa) { > + // DER-decode the signature > + int signatureLength = PK11_SignatureLen(mPrivKey); > + ScopedSECItem rawSignature(DSAU_DecodeDerSigToLen(signature.get(), Don't forget to add this and the other DSAU_ function to config/external/nss/nss.def @@ +1102,5 @@ > + if (!rawSignature.get()) { > + return NS_ERROR_DOM_UNKNOWN_ERR; > + } > + > + signature = (SECItem*) PORT_Alloc(sizeof(SECItem)); null-check signature here ::: dom/crypto/test/test_WebCrypto_ECDSA.html @@ +51,5 @@ > +); > + > +// ----------------------------------------------------------------------------- > +TestArray.addTest( > + "ECDSA JWK import and verify a known-good signature", Would it be worthwhile to also test rejecting a known-tampered signature? @@ +55,5 @@ > + "ECDSA JWK import and verify a known-good signature", > + function() { > + var that = this; > + var alg = { name: "ECDSA", namedCurve: "P-256", hash: "SHA-256" }; > + nit: trailing whitespace ::: dom/webidl/SubtleCrypto.webidl @@ +89,5 @@ > > +dictionary EcdsaParams : Algorithm { > + required AlgorithmIdentifier hash; > +}; > + nit: unnecessary extra blank line
Attachment #8499102 -
Flags: review?(dkeeler) → review+
Comment 4•9 years ago
|
||
Comment on attachment 8499102 [details] [diff] [review] bug-1034854.1.patch Review of attachment 8499102 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/crypto/CryptoKey.cpp @@ +820,5 @@ > if (!arena) { > return nullptr; > } > > + ScopedSECKEYPublicKey key(PORT_ArenaZNew(arena, SECKEYPublicKey)); That seems magic, I assume this is only used to free key->pkcs11Slot after calling PublicKeyValid()? If PublicKeyValid() succeeds then key->pkcs11Slot always != NULL. So I think we'd only need this if PK11_DestroyObject() fails? It doesn't look like key->pkcs11Slot is set to NULL when PK11_DestroyObject() succeeds? Would we double-free in that case? How about copying the key param to a ScopedSECKEYPublicKey only in PublicKeyValid()? So we wouldn't confuse readers here and would free pkcs11Slot a little closer to where it's allocated? Maybe I'm getting things completely wrong here but in either case maybe add a comment as to why we're using a scoped public key here when allocating in a scoped arena?
Attachment #8499102 -
Flags: review?(ttaubert) → review+
Assignee | ||
Comment 5•9 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #4) > Comment on attachment 8499102 [details] [diff] [review] > bug-1034854.1.patch > > Review of attachment 8499102 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/crypto/CryptoKey.cpp > @@ +820,5 @@ > > if (!arena) { > > return nullptr; > > } > > > > + ScopedSECKEYPublicKey key(PORT_ArenaZNew(arena, SECKEYPublicKey)); > > That seems magic, I assume this is only used to free key->pkcs11Slot after > calling PublicKeyValid()? If PublicKeyValid() succeeds then key->pkcs11Slot > always != NULL. So I think we'd only need this if PK11_DestroyObject() > fails? It doesn't look like key->pkcs11Slot is set to NULL when > PK11_DestroyObject() succeeds? Would we double-free in that case? > > How about copying the key param to a ScopedSECKEYPublicKey only in > PublicKeyValid()? So we wouldn't confuse readers here and would free > pkcs11Slot a little closer to where it's allocated? > > Maybe I'm getting things completely wrong here but in either case maybe add > a comment as to why we're using a scoped public key here when allocating in > a scoped arena? I think this was just a result of me generally switching unscoped pointers to scoped ones. But that seems unnecessary in this case because the pointer is pointing to arena-managed memory, and the arena is scoped. I'll revert it to unscoped.
Assignee | ||
Comment 6•9 years ago
|
||
bz: Could you please review the WebIDL so that the build scripts will let me land this? Happy try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b76a1f13c8ab
Attachment #8499102 -
Attachment is obsolete: true
Attachment #8504520 -
Flags: review?(bzbarsky)
![]() |
||
Comment 7•9 years ago
|
||
Comment on attachment 8504520 [details] [diff] [review] bug-1034854.2.patch r=ttaubert,dkeeler,bz r=me on the IDL bits
Attachment #8504520 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Updated•9 years ago
|
Attachment #8504520 -
Attachment description: bug-1034854.2.patch → bug-1034854.2.patch r=ttaubert,dkeeler,bz
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 8•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c044fae78ad8
Keywords: checkin-needed
Comment 9•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c044fae78ad8
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Updated•9 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 10•9 years ago
|
||
Comment on attachment 8504520 [details] [diff] [review] bug-1034854.2.patch r=ttaubert,dkeeler,bz Approval Request Comment [Feature/regressing bug #]: WebCrypto (865789) [User impact if declined]: ECDSA will be unavailable in WebCrypto [Describe test coverage new/current, TBPL]: Mochitests in patch [Risks and why]: Low risk, only affects WebCrypto [String/UUID change made/needed]: None
Attachment #8504520 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
Attachment #8504520 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in
before you can comment on or make changes to this bug.
Description
•