1034854 - Add support for ECDSA to WebCrypto API

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Richard Barnes [:rbarnes]]

Attachment: bug-1034854.patch

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.

Comment 2

[Richard Barnes [:rbarnes]]

Attachment: bug-1034854.1.patch

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 3

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

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 4

[Tim Taubert [:ttaubert] (inactive)]

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?

Comment 5

[Richard Barnes [:rbarnes]]

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

Comment 6

[Richard Barnes [:rbarnes]]

Attachment: bug-1034854.2.patch r=ttaubert,dkeeler,bz

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 7

[Boris Zbarsky [:bzbarsky]]

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

r=me on the IDL bits

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c044fae78ad8

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c044fae78ad8

Comment 10

[Richard Barnes [:rbarnes]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5de40c8a968f