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.

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.

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

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?

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

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

Comment on attachment 8504520 [details] [diff] [review] bug-1034854.2.patch r=ttaubert,dkeeler,bz r=me on the IDL bits

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