Franklin Tse Reporter
	Description • 9 years ago

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; Trident/7.0; .NET4.0E; .NET4.0C; .NET CLR 3.5.30729; .NET CLR 2.0.50727; .NET CLR 3.0.30729; GWX:RESERVED; rv:11.0) like Gecko

Steps to reproduce:

RSA-PSS is currently not supported in the WebCrypto API of Firefox. I think it should be supported for interoperability, as both Microsoft Edge and Google Chrome support that.

A test page written by Daniel Roesler is available at https://diafygi.github.io/webcrypto-examples/


Actual results:

All RSA-PSS operations fail


Expected results:

All RSA-PSS operations should work

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 8 • 9 years ago

Ok, that's pretty much good to go. Now we just need to get these APIs into NSS.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 9 • 9 years ago

We need to wait until bug 1215295 hits m-c but I figured we could get the patches reviewed before. If you want to build just check out latest NSS to security/nss/ in your working copy.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 12 • 9 years ago

Comment on attachment 8688416 [details] [diff] [review]
0002-Bug-1191936-Implement-RSA-PSS-signing-and-verificati.patch

Olli, can you please sign off the SubtleCrypto.webidl changes?

https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#RsaPssParams-dictionary

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 13 • 9 years ago

Comment on attachment 8688416 [details] [diff] [review]
0002-Bug-1191936-Implement-RSA-PSS-signing-and-verificati.patch

>From 608ed7485bbf31c9185cd7134371a775ce08233b Mon Sep 17 00:00:00 2001
>From: Tim Taubert <ttaubert@mozilla.com>
>Date: Tue, 13 Oct 2015 20:22:43 +0200
>Subject: Bug 1191936 - Implement RSA-PSS signing and verification
>
>
>diff --git a/dom/crypto/WebCryptoTask.cpp b/dom/crypto/WebCryptoTask.cpp
>index 6ac769a..5573610 100644
>--- a/dom/crypto/WebCryptoTask.cpp
>+++ b/dom/crypto/WebCryptoTask.cpp
>@@ -259,16 +259,51 @@ MapOIDTagToNamedCurve(SECOidTag aOIDTag, nsString& aResult)
>       break;
>     default:
>       return false;
>   }
> 
>   return true;
> }
> 
>+inline SECOidTag
>+MapHashAlgorithmNameToOID(const nsString& aName)
>+{
>+  SECOidTag hashOID(SEC_OID_UNKNOWN);
>+
>+  if (aName.EqualsLiteral(WEBCRYPTO_ALG_SHA1)) {
>+    hashOID = SEC_OID_SHA1;
>+  } else if (aName.EqualsLiteral(WEBCRYPTO_ALG_SHA256)) {
>+    hashOID = SEC_OID_SHA256;
>+  } else if (aName.EqualsLiteral(WEBCRYPTO_ALG_SHA384)) {
>+    hashOID = SEC_OID_SHA384;
>+  } else if (aName.EqualsLiteral(WEBCRYPTO_ALG_SHA512)) {
>+    hashOID = SEC_OID_SHA512;
>+  }
>+
>+  return hashOID;
>+}
>+
>+inline CK_MECHANISM_TYPE
>+MapHashAlgorithmNameToMgfMechanism(const nsString& aName) {
>+  CK_MECHANISM_TYPE mech(UNKNOWN_CK_MECHANISM);
>+
>+  if (aName.EqualsLiteral(WEBCRYPTO_ALG_SHA1)) {
>+    mech = CKG_MGF1_SHA1;
>+  } else if (aName.EqualsLiteral(WEBCRYPTO_ALG_SHA256)) {
>+    mech = CKG_MGF1_SHA256;
>+  } else if (aName.EqualsLiteral(WEBCRYPTO_ALG_SHA384)) {
>+    mech = CKG_MGF1_SHA384;
>+  } else if (aName.EqualsLiteral(WEBCRYPTO_ALG_SHA512)) {
>+    mech = CKG_MGF1_SHA512;
>+  }
>+
>+  return mech;
>+}
>+
> // Helper function to clone data from an ArrayBuffer or ArrayBufferView object
> inline bool
> CloneData(JSContext* aCx, CryptoBuffer& aDst, JS::Handle<JSObject*> aSrc)
> {
>   MOZ_ASSERT(NS_IsMainThread());
> 
>   // Try ArrayBuffer
>   RootedTypedArray<ArrayBuffer> ab(aCx);
>@@ -833,34 +868,25 @@ public:
>       }
> 
>       if (params.mLabel.WasPassed()) {
>         ATTEMPT_BUFFER_INIT(mLabel, params.mLabel.Value());
>       }
>     }
>     // Otherwise mLabel remains the empty octet string, as intended
> 
>-    // Look up the MGF based on the KeyAlgorithm.
>-    // static_cast is safe because we only get here if the algorithm name
>-    // is RSA-OAEP, and that only happens if we've constructed
>-    // an RsaHashedKeyAlgorithm.
>-    mHashMechanism = KeyAlgorithmProxy::GetMechanism(aKey.Algorithm().mRsa.mHash);
>-
>-    switch (mHashMechanism) {
>-      case CKM_SHA_1:
>-        mMgfMechanism = CKG_MGF1_SHA1; break;
>-      case CKM_SHA256:
>-        mMgfMechanism = CKG_MGF1_SHA256; break;
>-      case CKM_SHA384:
>-        mMgfMechanism = CKG_MGF1_SHA384; break;
>-      case CKM_SHA512:
>-        mMgfMechanism = CKG_MGF1_SHA512; break;
>-      default:
>-        mEarlyRv = NS_ERROR_DOM_NOT_SUPPORTED_ERR;
>-        return;
>+    KeyAlgorithm& hashAlg = aKey.Algorithm().mRsa.mHash;
>+    mHashMechanism = KeyAlgorithmProxy::GetMechanism(hashAlg);
>+    mMgfMechanism = MapHashAlgorithmNameToMgfMechanism(hashAlg.mName);
>+
>+    // Check we found appropriate mechanisms.
>+    if (mHashMechanism == UNKNOWN_CK_MECHANISM ||
>+        mMgfMechanism == UNKNOWN_CK_MECHANISM) {
>+      mEarlyRv = NS_ERROR_DOM_NOT_SUPPORTED_ERR;
>+      return;
>     }
>   }
> 
> private:
>   CK_MECHANISM_TYPE mHashMechanism;
>   CK_MECHANISM_TYPE mMgfMechanism;
>   ScopedSECKEYPrivateKey mPrivKey;
>   ScopedSECKEYPublicKey mPubKey;
>@@ -1035,165 +1061,187 @@ class AsymmetricSignVerifyTask : public WebCryptoTask
> public:
>   AsymmetricSignVerifyTask(JSContext* aCx,
>                            const ObjectOrString& aAlgorithm,
>                            CryptoKey& aKey,
>                            const CryptoOperationData& aSignature,
>                            const CryptoOperationData& aData,
>                            bool aSign)
>     : mOidTag(SEC_OID_UNKNOWN)
>+    , mHashMechanism(UNKNOWN_CK_MECHANISM)
>+    , mMgfMechanism(UNKNOWN_CK_MECHANISM)
>     , mPrivKey(aKey.GetPrivateKey())
>     , mPubKey(aKey.GetPublicKey())
>+    , mSaltLength(0)
>     , mSign(aSign)
>     , mVerified(false)
>-    , mEcdsa(false)
>+    , mIsRsaPkcs1(false)
>+    , mIsRsaPss(false)
>   {
>     ATTEMPT_BUFFER_INIT(mData, aData);
>     if (!aSign) {
>       ATTEMPT_BUFFER_INIT(mSignature, aSignature);
>     }
> 
>     nsString algName;
>+    nsString hashAlgName;
>     mEarlyRv = GetAlgorithmName(aCx, aAlgorithm, algName);
>     if (NS_FAILED(mEarlyRv)) {
>       return;
>     }
> 
>-    // Look up the SECOidTag
>     if (algName.EqualsLiteral(WEBCRYPTO_ALG_RSASSA_PKCS1)) {
>-      mEcdsa = false;
>+      mIsRsaPkcs1 = true;
>       Telemetry::Accumulate(Telemetry::WEBCRYPTO_ALG, TA_RSASSA_PKCS1);
>       CHECK_KEY_ALGORITHM(aKey.Algorithm(), WEBCRYPTO_ALG_RSASSA_PKCS1);
>+      hashAlgName = aKey.Algorithm().mRsa.mHash.mName;
>+    } else if (algName.EqualsLiteral(WEBCRYPTO_ALG_RSA_PSS)) {
>+      mIsRsaPss = true;
>+      Telemetry::Accumulate(Telemetry::WEBCRYPTO_ALG, TA_RSA_PSS);
>+      CHECK_KEY_ALGORITHM(aKey.Algorithm(), WEBCRYPTO_ALG_RSA_PSS);
>+
>+      KeyAlgorithm& hashAlg = aKey.Algorithm().mRsa.mHash;
>+      hashAlgName = hashAlg.mName;
>+      mHashMechanism = KeyAlgorithmProxy::GetMechanism(hashAlg);
>+      mMgfMechanism = MapHashAlgorithmNameToMgfMechanism(hashAlgName);
>+
>+      // Check we found appropriate mechanisms.
>+      if (mHashMechanism == UNKNOWN_CK_MECHANISM ||
>+          mMgfMechanism == UNKNOWN_CK_MECHANISM) {
>+        mEarlyRv = NS_ERROR_DOM_NOT_SUPPORTED_ERR;
>+        return;
>+      }
> 
>-      // For RSA, the hash name comes from the key algorithm
>-      nsString hashName = aKey.Algorithm().mRsa.mHash.mName;
>-      switch (MapAlgorithmNameToMechanism(hashName)) {
>-        case CKM_SHA_1:
>-          mOidTag = SEC_OID_PKCS1_SHA1_WITH_RSA_ENCRYPTION; break;
>-        case CKM_SHA256:
>-          mOidTag = SEC_OID_PKCS1_SHA256_WITH_RSA_ENCRYPTION; break;
>-        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:
>-          mEarlyRv = NS_ERROR_DOM_NOT_SUPPORTED_ERR;
>-          return;
>+      RootedDictionary<RsaPssParams> params(aCx);
>+      mEarlyRv = Coerce(aCx, params, aAlgorithm);
>+      if (NS_FAILED(mEarlyRv)) {
>+        mEarlyRv = NS_ERROR_DOM_SYNTAX_ERR;
>+        return;
>       }
>+
>+      mSaltLength = params.mSaltLength;
>     } else if (algName.EqualsLiteral(WEBCRYPTO_ALG_ECDSA)) {
>-      mEcdsa = true;
>       Telemetry::Accumulate(Telemetry::WEBCRYPTO_ALG, TA_ECDSA);
>       CHECK_KEY_ALGORITHM(aKey.Algorithm(), WEBCRYPTO_ALG_ECDSA);
> 
>       // For ECDSA, the hash name comes from the algorithm parameter
>       RootedDictionary<EcdsaParams> params(aCx);
>       mEarlyRv = Coerce(aCx, params, aAlgorithm);
>       if (NS_FAILED(mEarlyRv)) {
>         mEarlyRv = NS_ERROR_DOM_SYNTAX_ERR;
>         return;
>       }
> 
>-      nsString hashName;
>-      mEarlyRv = GetAlgorithmName(aCx, params.mHash, hashName);
>+      mEarlyRv = GetAlgorithmName(aCx, params.mHash, hashAlgName);
>       if (NS_FAILED(mEarlyRv)) {
>         mEarlyRv = NS_ERROR_DOM_SYNTAX_ERR;
>         return;
>       }
>-
>-      CK_MECHANISM_TYPE hashMechanism = MapAlgorithmNameToMechanism(hashName);
>-      if (hashMechanism == UNKNOWN_CK_MECHANISM) {
>-        mEarlyRv = NS_ERROR_DOM_SYNTAX_ERR;
>-        return;
>-      }
>-
>-      switch (hashMechanism) {
>-        case CKM_SHA_1:
>-          mOidTag = SEC_OID_ANSIX962_ECDSA_SHA1_SIGNATURE; break;
>-        case CKM_SHA256:
>-          mOidTag = SEC_OID_ANSIX962_ECDSA_SHA256_SIGNATURE; break;
>-        case CKM_SHA384:
>-          mOidTag = SEC_OID_ANSIX962_ECDSA_SHA384_SIGNATURE; break;
>-        case CKM_SHA512:
>-          mOidTag = SEC_OID_ANSIX962_ECDSA_SHA512_SIGNATURE; break;
>-        default:
>-          mEarlyRv = NS_ERROR_DOM_NOT_SUPPORTED_ERR;
>-          return;
>-      }
>     } else {
>       // This shouldn't happen; CreateSignVerifyTask shouldn't create
>       // one of these unless it's for the above algorithms.
>       MOZ_ASSERT(false);
>     }
> 
>+    // Determine hash algorithm to use.
>+    mOidTag = MapHashAlgorithmNameToOID(hashAlgName);
>+    if (mOidTag == SEC_OID_UNKNOWN) {
>+      mEarlyRv = NS_ERROR_DOM_NOT_SUPPORTED_ERR;
>+      return;
>+    }
>+
>     // Check that we have the appropriate key
>     if ((mSign && !mPrivKey) || (!mSign && !mPubKey)) {
>       mEarlyRv = NS_ERROR_DOM_INVALID_ACCESS_ERR;
>       return;
>     }
>   }
> 
> private:
>   SECOidTag mOidTag;
>+  CK_MECHANISM_TYPE mHashMechanism;
>+  CK_MECHANISM_TYPE mMgfMechanism;
>   ScopedSECKEYPrivateKey mPrivKey;
>   ScopedSECKEYPublicKey mPubKey;
>   CryptoBuffer mSignature;
>   CryptoBuffer mData;
>+  uint32_t mSaltLength;
>   bool mSign;
>   bool mVerified;
>-  bool mEcdsa;
>+  bool mIsRsaPkcs1;
>+  bool mIsRsaPss;
> 
>   virtual nsresult DoCrypto() override
>   {
>-    nsresult rv;
>-    if (mSign) {
>-      ScopedSECItem signature(::SECITEM_AllocItem(nullptr, nullptr, 0));
>-      if (!signature.get()) {
>+    SECStatus rv;
>+    ScopedSECItem hash(::SECITEM_AllocItem(nullptr, nullptr,
>+                                           HASH_ResultLenByOidTag(mOidTag)));
>+    if (!hash) {
>+      return NS_ERROR_DOM_OPERATION_ERR;
>+    }
>+
>+    // Compute digest over given data.
>+    rv = PK11_HashBuf(mOidTag, hash->data, mData.Elements(), mData.Length());
>+    NS_ENSURE_SUCCESS(MapSECStatus(rv), NS_ERROR_DOM_OPERATION_ERR);
>+
>+    // Wrap hash in a digest info template (RSA-PKCS1 only).
>+    if (mIsRsaPkcs1) {
>+      ScopedSGNDigestInfo di(SGN_CreateDigestInfo(mOidTag, hash->data, hash->len));
>+      if (!di) {
>         return NS_ERROR_DOM_OPERATION_ERR;
>       }
> 
>-      rv = MapSECStatus(SEC_SignData(signature, mData.Elements(),
>-                                     mData.Length(), mPrivKey, mOidTag));
>+      // Reuse |hash|.
>+      SECITEM_FreeItem(hash, false);
>+      if (!SGN_EncodeDigestInfo(nullptr, hash, di)) {
>+        return NS_ERROR_DOM_OPERATION_ERR;
>+      }
>+    }
> 
>-      if (mEcdsa) {
>-        // DER-decode the signature
>-        int signatureLength = PK11_SignatureLen(mPrivKey);
>-        ScopedSECItem rawSignature(DSAU_DecodeDerSigToLen(signature.get(),
>-                                                          signatureLength));
>-        if (!rawSignature.get()) {
>-          return NS_ERROR_DOM_OPERATION_ERR;
>-        }
>+    SECItem* params = nullptr;
>+    CK_MECHANISM_TYPE mech = PK11_MapSignKeyType((mSign ? mPrivKey->keyType :
>+                                                          mPubKey->keyType));
> 
>-        ATTEMPT_BUFFER_ASSIGN(mSignature, rawSignature);
>-      } else {
>-        ATTEMPT_BUFFER_ASSIGN(mSignature, signature);
>-      }
>+    CK_RSA_PKCS_PSS_PARAMS rsaPssParams;
>+    SECItem rsaPssParamsItem = { siBuffer, };
> 
>-    } else {
>-      ScopedSECItem signature(::SECITEM_AllocItem(nullptr, nullptr, 0));
>-      if (!signature.get()) {
>-        return NS_ERROR_DOM_UNKNOWN_ERR;
>-      }
>+    // Set up parameters for RSA-PSS.
>+    if (mIsRsaPss) {
>+      rsaPssParams.hashAlg = mHashMechanism;
>+      rsaPssParams.mgf = mMgfMechanism;
>+      rsaPssParams.sLen = mSaltLength;
> 
>-      if (mEcdsa) {
>-        // DER-encode the signature
>-        ScopedSECItem rawSignature(::SECITEM_AllocItem(nullptr, nullptr, 0));
>-        if (!rawSignature || !mSignature.ToSECItem(nullptr, rawSignature)) {
>-          return NS_ERROR_DOM_UNKNOWN_ERR;
>-        }
>+      rsaPssParamsItem.data = (unsigned char*)&rsaPssParams;
>+      rsaPssParamsItem.len = sizeof(rsaPssParams);
>+      params = &rsaPssParamsItem;
> 
>-        rv = MapSECStatus(DSAU_EncodeDerSigWithLen(signature, rawSignature,
>-                                                   rawSignature->len));
>-        NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_OPERATION_ERR);
>-      } else if (!mSignature.ToSECItem(nullptr, signature)) {
>-        return NS_ERROR_DOM_UNKNOWN_ERR;
>+      mech = CKM_RSA_PKCS_PSS;
>+    }
>+
>+    // Allocate SECItem to hold the signature.
>+    uint32_t len = mSign ? PK11_SignatureLen(mPrivKey) : 0;
>+    ScopedSECItem sig(::SECITEM_AllocItem(nullptr, nullptr, len));
>+    if (!sig) {
>+      return NS_ERROR_DOM_OPERATION_ERR;
>+    }
>+
>+    if (mSign) {
>+      // Sign the hash.
>+      rv = PK11_SignWithMechanism(mPrivKey, mech, params, sig, hash);
>+      NS_ENSURE_SUCCESS(MapSECStatus(rv), NS_ERROR_DOM_OPERATION_ERR);
>+      ATTEMPT_BUFFER_ASSIGN(mSignature, sig);
>+    } else {
>+      // Copy the given signature to the SECItem.
>+      if (!mSignature.ToSECItem(nullptr, sig)) {
>+        return NS_ERROR_DOM_OPERATION_ERR;
>       }
> 
>-      rv = MapSECStatus(VFY_VerifyData(mData.Elements(), mData.Length(),
>-                                       mPubKey, signature, mOidTag, nullptr));
>-      mVerified = NS_SUCCEEDED(rv);
>+      // Verify the signature.
>+      rv = PK11_VerifyWithMechanism(mPubKey, mech, params, sig, hash, nullptr);
>+      mVerified = NS_SUCCEEDED(MapSECStatus(rv));
>     }
> 
>     return NS_OK;
>   }
> 
>   virtual void Resolve() override
>   {
>     if (mSign) {
>@@ -1218,32 +1266,29 @@ public:
>     mEarlyRv = GetAlgorithmName(aCx, aAlgorithm, algName);
>     if (NS_FAILED(mEarlyRv)) {
>       mEarlyRv = NS_ERROR_DOM_SYNTAX_ERR;
>       return;
>     }
> 
>     TelemetryAlgorithm telemetryAlg;
>     if (algName.EqualsLiteral(WEBCRYPTO_ALG_SHA1))   {
>-      mOidTag = SEC_OID_SHA1;
>       telemetryAlg = TA_SHA_1;
>     } else if (algName.EqualsLiteral(WEBCRYPTO_ALG_SHA256)) {
>-      mOidTag = SEC_OID_SHA256;
>       telemetryAlg = TA_SHA_224;
>     } else if (algName.EqualsLiteral(WEBCRYPTO_ALG_SHA384)) {
>-      mOidTag = SEC_OID_SHA384;
>       telemetryAlg = TA_SHA_256;
>     } else if (algName.EqualsLiteral(WEBCRYPTO_ALG_SHA512)) {
>-      mOidTag = SEC_OID_SHA512;
>       telemetryAlg = TA_SHA_384;
>     } else {
>       mEarlyRv = NS_ERROR_DOM_SYNTAX_ERR;
>       return;
>     }
>     Telemetry::Accumulate(Telemetry::WEBCRYPTO_ALG, telemetryAlg);
>+    mOidTag = MapHashAlgorithmNameToOID(algName);
>   }
> 
> private:
>   SECOidTag mOidTag;
>   CryptoBuffer mData;
> 
>   virtual nsresult DoCrypto() override
>   {
>@@ -3034,16 +3079,17 @@ WebCryptoTask::CreateSignVerifyTask(JSContext* aCx,
>   nsresult rv = GetAlgorithmName(aCx, aAlgorithm, algName);
>   if (NS_FAILED(rv)) {
>     return new FailureTask(rv);
>   }
> 
>   if (algName.EqualsLiteral(WEBCRYPTO_ALG_HMAC)) {
>     return new HmacTask(aCx, aAlgorithm, aKey, aSignature, aData, aSign);
>   } else if (algName.EqualsLiteral(WEBCRYPTO_ALG_RSASSA_PKCS1) ||
>+             algName.EqualsLiteral(WEBCRYPTO_ALG_RSA_PSS) ||
>              algName.EqualsLiteral(WEBCRYPTO_ALG_ECDSA)) {
>     return new AsymmetricSignVerifyTask(aCx, aAlgorithm, aKey, aSignature,
>                                         aData, aSign);
>   }
> 
>   return new FailureTask(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
> }
> 
>diff --git a/dom/crypto/test/test_WebCrypto_RSA_PSS.html b/dom/crypto/test/test_WebCrypto_RSA_PSS.html
>index d4835cd..9fa7d1c 100644
>--- a/dom/crypto/test/test_WebCrypto_RSA_PSS.html
>+++ b/dom/crypto/test/test_WebCrypto_RSA_PSS.html
>@@ -36,16 +36,47 @@ TestArray.addTest(
>       modulusLength: 1024,
>       publicExponent: new Uint8Array([0x01, 0x00, 0x01])
>     };
> 
>     crypto.subtle.generateKey(alg, false, ["sign", "verify"])
>       .then(complete(that), error(that));
>   }
> );
>+
>+// -----------------------------------------------------------------------------
>+TestArray.addTest(
>+  "RSA-PSS key generation and sign/verify round-trip (SHA-256, 2048-bit)",
>+  function () {
>+    var that = this;
>+    var alg = {
>+      name: "RSA-PSS",
>+      hash: "SHA-256",
>+      modulusLength: 2048,
>+      publicExponent: new Uint8Array([0x01, 0x00, 0x01])
>+    };
>+
>+    var privKey, pubKey, data = crypto.getRandomValues(new Uint8Array(128));
>+    function setKey(x) { pubKey = x.publicKey; privKey = x.privateKey; }
>+    function doSign() {
>+      var alg = {name: "RSA-PSS", saltLength: 32};
>+      return crypto.subtle.sign(alg, privKey, data);
>+    }
>+    function doVerify(x) {
>+      var alg = {name: "RSA-PSS", saltLength: 32};
>+      return crypto.subtle.verify(alg, pubKey, x, data);
>+    }
>+
>+    crypto.subtle.generateKey(alg, false, ["sign", "verify"])
>+      .then(setKey, error(that))
>+      .then(doSign, error(that))
>+      .then(doVerify, error(that))
>+      .then(complete(that, x => x), error(that))
>+  }
>+);
> /*]]>*/</script>
> </head>
> 
> <body>
> 
> <div id="content">
> 	<div id="head">
> 		<b>Web</b>Crypto<br>
>diff --git a/dom/webidl/SubtleCrypto.webidl b/dom/webidl/SubtleCrypto.webidl
>index 9bb530e..dc0b744 100644
>--- a/dom/webidl/SubtleCrypto.webidl
>+++ b/dom/webidl/SubtleCrypto.webidl
>@@ -61,16 +61,20 @@ dictionary RsaHashedKeyGenParams : Algorithm {
>   required BigInteger publicExponent;
>   required AlgorithmIdentifier hash;
> };
> 
> dictionary RsaOaepParams : Algorithm {
>   CryptoOperationData label;
> };
> 
>+dictionary RsaPssParams : Algorithm {
>+  [EnforceRange] required unsigned long saltLength;
>+};
>+
> dictionary DhKeyGenParams : Algorithm {
>   required BigInteger prime;
>   required BigInteger generator;
> };
> 
> dictionary EcKeyGenParams : Algorithm {
>   required NamedCurve namedCurve;
> };
>diff --git a/security/manager/ssl/ScopedNSSTypes.h b/security/manager/ssl/ScopedNSSTypes.h
>index 7859a32..2b041b1 100644
>--- a/security/manager/ssl/ScopedNSSTypes.h
>+++ b/security/manager/ssl/ScopedNSSTypes.h
>@@ -134,16 +134,19 @@ VFY_DestroyContext_true(VFYContext * ctx) {
> } // namespace mozilla::psm
> 
> MOZ_TYPE_SPECIFIC_SCOPED_POINTER_TEMPLATE(ScopedPK11Context,
>                                           PK11Context,
>                                           mozilla::psm::PK11_DestroyContext_true)
> MOZ_TYPE_SPECIFIC_SCOPED_POINTER_TEMPLATE(ScopedSGNContext,
>                                           SGNContext,
>                                           mozilla::psm::SGN_DestroyContext_true)
>+MOZ_TYPE_SPECIFIC_SCOPED_POINTER_TEMPLATE(ScopedSGNDigestInfo,
>+                                          SGNDigestInfo,
>+                                          SGN_DestroyDigestInfo)
> MOZ_TYPE_SPECIFIC_SCOPED_POINTER_TEMPLATE(ScopedVFYContext,
>                                           VFYContext,
>                                           mozilla::psm::VFY_DestroyContext_true)
> 
> /** A more convenient way of dealing with digests calculated into
>  *  stack-allocated buffers. NSS must be initialized on the main thread before
>  *  use, and the caller must ensure NSS isn't shut down, typically by
>  *  subclassing nsNSSShutDownObject, while Digest is in use.
>-- 
>2.6.3
>

	Richard Barnes [:rbarnes]
	Comment 14 • 9 years ago

Comment on attachment 8688416 [details] [diff] [review]
0002-Bug-1191936-Implement-RSA-PSS-signing-and-verificati.patch

Review of attachment 8688416 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/crypto/WebCryptoTask.cpp
@@ +297,5 @@
> +  }
> +
> +  return mech;
> +}
> +

This is duplicative with one of the other patches, yes?

@@ +1074,4 @@
>      , mSign(aSign)
>      , mVerified(false)
> +    , mIsRsaPkcs1(false)
> +    , mIsRsaPss(false)

For this as well as the generateKey patch, I wonder if it would be better to just have an internal enum class instead of a bunch of flags.

@@ +1222,5 @@
> +    if (!sig) {
> +      return NS_ERROR_DOM_OPERATION_ERR;
> +    }
> +
> +    if (mSign) {

You've lost the necessary logic to do ECDSA signatures.  You need DSAU_DecodeDerSigToLen() after sign and DSAU_EncodeDerSigWithLen() before verify.  As it is, you should be failing the ECDSA tests.

::: dom/crypto/test/test_WebCrypto_RSA_PSS.html
@@ +70,5 @@
> +      .then(doSign, error(that))
> +      .then(doVerify, error(that))
> +      .then(complete(that, x => x), error(that))
> +  }
> +);

It would be great to have a known-answer verify test.  Looks like there are some test vectors here:

http://www.emc.com/emc-plus/rsa-labs/standards-initiatives/pkcs-rsa-cryptography-standard.htm

	Richard Barnes [:rbarnes]
	Comment 15 • 9 years ago

Comment on attachment 8688417 [details] [diff] [review]
0003-Bug-1191936-Implement-SPKI-PKCS-8-JWK-import-export-.patch

Review of attachment 8688417 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/crypto/test/test_WebCrypto_RSA_PSS.html
@@ +209,5 @@
> +TestArray.addTest(
> +  "RSA-PSS JWK export a public key",
> +  function () {
> +    var that = this;
> +    var alg = {name: "RSA-PSS", hash: "SHA-1"};

Given all the hating on SHA-1 lately, it would be nice to include a signature with SHA-256 and a 2048-bit key.

	Richard Barnes [:rbarnes]
	Comment 16 • 9 years ago

> ::: dom/crypto/test/test_WebCrypto_RSA_PSS.html
> @@ +70,5 @@
> > +      .then(doSign, error(that))
> > +      .then(doVerify, error(that))
> > +      .then(complete(that, x => x), error(that))
> > +  }
> > +);
> 
> It would be great to have a known-answer verify test.  Looks like there are
> some test vectors here:
> 
> http://www.emc.com/emc-plus/rsa-labs/standards-initiatives/pkcs-rsa-
> cryptography-standard.htm

Ah, I see you're ahead of me here, and I was just looking at the wrong patch :)

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 17 • 8 years ago

(In reply to Richard Barnes [:rbarnes] from comment #14)
> ::: dom/crypto/WebCryptoTask.cpp
> @@ +297,5 @@
> > +  }
> > +
> > +  return mech;
> > +}
> > +
> 
> This is duplicative with one of the other patches, yes?

I don't understand this comment, is this code somewhere else already?

> @@ +1074,4 @@
> >      , mSign(aSign)
> >      , mVerified(false)
> > +    , mIsRsaPkcs1(false)
> > +    , mIsRsaPss(false)
> 
> For this as well as the generateKey patch, I wonder if it would be better to
> just have an internal enum class instead of a bunch of flags.

Will do. The generateKey patch wouldn't benefit much though, would it?

> @@ +1222,5 @@
> > +    if (!sig) {
> > +      return NS_ERROR_DOM_OPERATION_ERR;
> > +    }
> > +
> > +    if (mSign) {
> 
> You've lost the necessary logic to do ECDSA signatures.  You need
> DSAU_DecodeDerSigToLen() after sign and DSAU_EncodeDerSigWithLen() before
> verify.  As it is, you should be failing the ECDSA tests.

Yeah, that seems true. ECDSA tests are passing... I'll investigate.

(In reply to Richard Barnes [:rbarnes] from comment #15)
> ::: dom/crypto/test/test_WebCrypto_RSA_PSS.html
> @@ +209,5 @@
> > +TestArray.addTest(
> > +  "RSA-PSS JWK export a public key",
> > +  function () {
> > +    var that = this;
> > +    var alg = {name: "RSA-PSS", hash: "SHA-1"};
> 
> Given all the hating on SHA-1 lately, it would be nice to include a
> signature with SHA-256 and a 2048-bit key.

At the top we generate/sign/verify with SHA-256 and a 2048-bit key. The "official" RSA test vectors don't include SHA-256 unfortunately.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 18 • 8 years ago

(In reply to Tim Taubert [:ttaubert] from comment #17)
> (In reply to Richard Barnes [:rbarnes] from comment #14)
> > @@ +1222,5 @@
> > > +    if (!sig) {
> > > +      return NS_ERROR_DOM_OPERATION_ERR;
> > > +    }
> > > +
> > > +    if (mSign) {
> > 
> > You've lost the necessary logic to do ECDSA signatures.  You need
> > DSAU_DecodeDerSigToLen() after sign and DSAU_EncodeDerSigWithLen() before
> > verify.  As it is, you should be failing the ECDSA tests.
> 
> Yeah, that seems true. ECDSA tests are passing... I'll investigate.

We talked about this on IRC but here again for the record:

The current WebCrypto code calls DSAU_DecodeDerSigToLen() on the result of SEC_SignData() because we want to get rid of that encoding. It calls DSAU_EncodeDerSigWithLen() before passing the signature to VFY_VerifyData() because the API expects the same encoding it returns. The new APIs I'm using don't require this dance anymore.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 19 • 8 years ago

Carrying over r=smaug.

	Richard Barnes [:rbarnes]
	Comment 20 • 8 years ago

Comment on attachment 8709607 [details] [diff] [review]
0002-Bug-1191936-Implement-RSA-PSS-signing-and-verificati.patch, v2

Review of attachment 8709607 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, this looks good.  Just need a known-anwer test.

::: dom/crypto/WebCryptoTask.cpp
@@ +1166,5 @@
>    bool mSign;
>    bool mVerified;
> +
> +  // The signature algorithm to use.
> +  enum class Algorithm: uint8_t { RSA_PKCS1, RSA_PSS, ECDSA};

Nit: Remove space before RSA_PKCS1.  Maybe alphabetize :)

Might be useful to have an Unknown value here, that mAlgorithm is set to by default.  That might help avoid hard-to-debug errors later, in cases where mAlgorithm might accidentally not get initialized.  ("Why does it think mAlgorithm is X?")

::: dom/crypto/test/test_WebCrypto_RSA_PSS.html
@@ +43,5 @@
>  );
> +
> +// -----------------------------------------------------------------------------
> +TestArray.addTest(
> +  "RSA-PSS key generation and sign/verify round-trip (SHA-256, 2048-bit)",

This test looks fine, but please add a known answer test, i.e., verifying a known-good signature.  ECDSA example here: 

https://dxr.mozilla.org/mozilla-central/source/dom/crypto/test/test_WebCrypto_ECDSA.html#166

It looks like the same file from which the OAEP tests were taken also has PSS tests.

http://www.emc.com/emc-plus/rsa-labs/standards-initiatives/pkcs-rsa-cryptography-standard.htm

@@ +53,5 @@
> +      modulusLength: 2048,
> +      publicExponent: new Uint8Array([0x01, 0x00, 0x01])
> +    };
> +
> +    var privKey, pubKey, data = crypto.getRandomValues(new Uint8Array(128));

Nit: Maybe I've been writing too much in languages with destructuring assignments, but it really looked to me like you were assigning all three values from one call :)  Maybe split this into "var privKey, pubKey;\nvar data = ..."

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 21 • 8 years ago

(In reply to Richard Barnes [:rbarnes] from comment #20)
> ::: dom/crypto/WebCryptoTask.cpp
> @@ +1166,5 @@
> >    bool mSign;
> >    bool mVerified;
> > +
> > +  // The signature algorithm to use.
> > +  enum class Algorithm: uint8_t { RSA_PKCS1, RSA_PSS, ECDSA};
> 
> Nit: Remove space before RSA_PKCS1.  Maybe alphabetize :)
> 
> Might be useful to have an Unknown value here, that mAlgorithm is set to by
> default.  That might help avoid hard-to-debug errors later, in cases where
> mAlgorithm might accidentally not get initialized.  ("Why does it think
> mAlgorithm is X?")

Done, with assertion.

> ::: dom/crypto/test/test_WebCrypto_RSA_PSS.html
> > +// -----------------------------------------------------------------------------
> > +TestArray.addTest(
> > +  "RSA-PSS key generation and sign/verify round-trip (SHA-256, 2048-bit)",
> 
> This test looks fine, but please add a known answer test, i.e., verifying a
> known-good signature.

We have that already in part 3. Will consolidate the tests in a separate patch next time.

> @@ +53,5 @@
> > +      modulusLength: 2048,
> > +      publicExponent: new Uint8Array([0x01, 0x00, 0x01])
> > +    };
> > +
> > +    var privKey, pubKey, data = crypto.getRandomValues(new Uint8Array(128));
> 
> Nit: Maybe I've been writing too much in languages with destructuring
> assignments, but it really looked to me like you were assigning all three
> values from one call :)  Maybe split this into "var privKey, pubKey;\nvar
> data = ..."

Done.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 23 • 8 years ago

Added a few more test vectors for SHA-2 hash/mask functions and a test to ensure that invalid signatures fail. Added a test for deterministic signatures with saltLength=0.

	Richard Barnes [:rbarnes]
	Comment 24 • 8 years ago

Comment on attachment 8711021 [details] [diff] [review]
0004-Bug-1191936-Add-more-test-vectors-and-a-test-for-det.patch

Review of attachment 8711021 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/crypto/test/test_WebCrypto_RSA_PSS.html
@@ +214,5 @@
> +    crypto.subtle.importKey("spki", vec.spki, alg, false, ["verify"])
> +      .then(doVerify, error(that))
> +      .then(complete(that, x => x), error(that));
> +  }
> +);

It seems like you could probably collapse this into a single test, like you did with the negative tests above.  Just define a verifyCase() method that does import.then(verify), then wrap them all with Promise.all().every().  But I can live with it as-is.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 25 • 8 years ago

(In reply to Richard Barnes [:rbarnes] from comment #24)
> It seems like you could probably collapse this into a single test, like you
> did with the negative tests above.  Just define a verifyCase() method that
> does import.then(verify), then wrap them all with Promise.all().every(). 
> But I can live with it as-is.

Done.

	Pulsebot
	Comment 26 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/1faac7557b25
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fba432e1cab
https://hg.mozilla.org/integration/mozilla-inbound/rev/29a076e8bc0d
https://hg.mozilla.org/integration/mozilla-inbound/rev/92e279613f0e

	Carsten Book [:Tomcat]
	Comment 27 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/1faac7557b25
https://hg.mozilla.org/mozilla-central/rev/1fba432e1cab
https://hg.mozilla.org/mozilla-central/rev/29a076e8bc0d
https://hg.mozilla.org/mozilla-central/rev/92e279613f0e

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 28 • 8 years ago

Release Note Request (optional, but appreciated)
[Why is this notable]: I think devs might want to know they can now use RSA-PSS in WebCrypto with Firefox.
[Suggested wording]: WebCrypto: RSA-PSS signature support
[Links (documentation, blog post, etc)]:

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 29 • 8 years ago

Added to Fx 47 (Aurora) release notes

	Franklin Tse Reporter
	Comment 30 • 8 years ago

Thank Tim for the fix. My RSA-PSS code that works with Chrome now also works with Firefox 47 Nightly.

Thank Ritu for adding the release notes. There may be a trivial issue though: The bug reference on https://developer.mozilla.org/en-US/Firefox/Releases/47 is missing the closing bracket.