Closed Bug 1191936 Opened 10 years ago Closed 9 years ago

Implement RSA-PSS in WebCrypto API

Categories

(Core :: DOM: Security, defect)

Product:
Core
Component:
DOM: Security
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla47
Tracking Flags:
Tracking Status
firefox47 --- fixed
relnote-firefox --- 47+

People

(Reporter: FranklinWhale, Assigned: ttaubert)

References

Details

(Keywords: dev-doc-needed, feature)

Attachments

(4 files, 9 obsolete files)

0001-Bug-1191936-Implement-RSA-PSS-key-generation.patch
7.74 KB, patch
rbarnes
: review+
Details | Diff | Splinter Review
0003-Bug-1191936-Implement-SPKI-PKCS-8-JWK-import-export-.patch
20.31 KB, patch
rbarnes
: review+
Details | Diff | Splinter Review
0002-Bug-1191936-Implement-RSA-PSS-signing-and-verificati.patch, v3
19.84 KB, patch
rbarnes
: review+
Details | Diff | Splinter Review
0004-Bug-1191936-Add-more-test-vectors-and-a-test-for-det.patch
16.29 KB, patch
rbarnes
: review+
Details | Diff | Splinter Review
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Depends on: 158750
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #8673557 - Attachment is obsolete: true
Attachment #8673811 - Attachment is obsolete: true
Attachment #8673845 - Attachment is obsolete: true
Ok, that's pretty much good to go. Now we just need to get these APIs into NSS.
Depends on: 1215295
No longer depends on: 158750
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.
Attachment #8673808 - Attachment is obsolete: true
Attachment #8688415 - Flags: review?(rlb)
Attachment #8673810 - Attachment is obsolete: true
Attachment #8688416 - Flags: review?(rlb)
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
Attachment #8688416 - Flags: review?(bugs)
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 >
Attachment #8688416 - Flags: review?(bugs) → review+
Attachment #8688415 - Flags: review?(rlb) → review+
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
Attachment #8688416 - Flags: review?(rlb) → review-
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.
Attachment #8688417 - Flags: review?(rlb) → review+
> ::: 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 :)
Depends on: 1228410
(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.
(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.
Carrying over r=smaug.
Attachment #8688416 - Attachment is obsolete: true
Attachment #8709607 - Flags: review?(rlb)
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 = ..."
Attachment #8709607 - Flags: review?(rlb) → review-
(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.
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.
Attachment #8711021 - Flags: review?(rlb)
Attachment #8711000 - Flags: review?(rlb) → review+
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.
Attachment #8711021 - Flags: review?(rlb) → review+
(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.
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)]:
relnote-firefox: --- → ?
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.
Keywords: feature
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: