bug 1357226 - work around a library inefficiency with EC keys when verifying ECDSA signatures r?fkiefer,jcj draft
authorDavid Keeler <dkeeler@mozilla.com>
Tue, 11 Apr 2017 14:11:28 -0700
changeset 565995 6ff3dec489bf5070fd669c1e6dbdcc0014081e90
parent 565752 27311156637f9b5d4504373967e01c4241902ae7
child 625168 a3fa852a7e9f84944a261ff95dab8d1cd65d0e6d
push id55055
push userbmo:dkeeler@mozilla.com
push dateThu, 20 Apr 2017 17:33:34 +0000
reviewersfkiefer, jcj
bugs1357226
milestone55.0a1
bug 1357226 - work around a library inefficiency with EC keys when verifying ECDSA signatures r?fkiefer,jcj Calling VFY_VerifyDigestDirect causes the provided SECKEYPublicKey to be reimported to the softoken regardless of if it already exists on it. EC keys must be verified upon import (to see if the point is on the curve to avoid some small subgroup attacks), and so repeatedly doing this with a static key (say, for example, a key corresponding to a built-in certificate transparency log) is inefficient. This patch alters the certificate transparency implementation to import these keys each once and then use PK11_Verify for ECDSA signature verification, which doesn't have the same drawback. Since this change causes CertVerifier to hold an NSS resource (via its MultiLogCTVerifier having a list of CTLogVerifier, each of which now has a SECKEYPublicKey), nsNSSComponent has to make sure it goes away before shutting down NSS. This patch ensures this happens in nsNSSComponent::ShutdownNSS(). MozReview-Commit-ID: 6VSmz7S53y2
security/certverifier/CTLogVerifier.cpp
security/certverifier/CTLogVerifier.h
security/certverifier/tests/gtest/CTLogVerifierTest.cpp
security/manager/ssl/nsNSSComponent.cpp
security/nss.symbols
--- a/security/certverifier/CTLogVerifier.cpp
+++ b/security/certverifier/CTLogVerifier.cpp
@@ -157,16 +157,44 @@ CTLogVerifier::Init(Input subjectPublicK
   }
   mSignatureAlgorithm = trustDomain.mSignatureAlgorithm;
 
   rv = InputToBuffer(subjectPublicKeyInfo, mSubjectPublicKeyInfo);
   if (rv != Success) {
     return rv;
   }
 
+  if (mSignatureAlgorithm == DigitallySigned::SignatureAlgorithm::ECDSA) {
+    SECItem spkiSECItem = {
+      siBuffer,
+      mSubjectPublicKeyInfo.begin(),
+      static_cast<unsigned int>(mSubjectPublicKeyInfo.length())
+    };
+    UniqueCERTSubjectPublicKeyInfo spki(
+      SECKEY_DecodeDERSubjectPublicKeyInfo(&spkiSECItem));
+    if (!spki) {
+      return MapPRErrorCodeToResult(PR_GetError());
+    }
+    mPublicECKey.reset(SECKEY_ExtractPublicKey(spki.get()));
+    if (!mPublicECKey) {
+      return MapPRErrorCodeToResult(PR_GetError());
+    }
+    UniquePK11SlotInfo slot(PK11_GetInternalSlot());
+    if (!slot) {
+      return MapPRErrorCodeToResult(PR_GetError());
+    }
+    CK_OBJECT_HANDLE handle = PK11_ImportPublicKey(slot.get(),
+                                                   mPublicECKey.get(), false);
+    if (handle == CK_INVALID_HANDLE) {
+      return MapPRErrorCodeToResult(PR_GetError());
+    }
+  } else {
+    mPublicECKey.reset(nullptr);
+  }
+
   if (!mKeyId.resizeUninitialized(SHA256_LENGTH)) {
     return Result::FATAL_ERROR_NO_MEMORY;
   }
   rv = DigestBufNSS(subjectPublicKeyInfo, DigestAlgorithm::sha256,
                     mKeyId.begin(), mKeyId.length());
   if (rv != Success) {
     return rv;
   }
@@ -235,16 +263,48 @@ CTLogVerifier::VerifySignedTreeHead(cons
 
 bool
 CTLogVerifier::SignatureParametersMatch(const DigitallySigned& signature)
 {
   return signature.SignatureParametersMatch(
     DigitallySigned::HashAlgorithm::SHA256, mSignatureAlgorithm);
 }
 
+static Result
+FasterVerifyECDSASignedDigestNSS(const SignedDigest& sd,
+                                 UniqueSECKEYPublicKey& pubkey)
+{
+  MOZ_ASSERT(pubkey);
+  if (!pubkey) {
+    return Result::FATAL_ERROR_LIBRARY_FAILURE;
+  }
+  // The signature is encoded as a DER SEQUENCE of two INTEGERs. PK11_Verify
+  // expects the signature as only the two integers r and s (so no encoding -
+  // just two series of bytes each half as long as SECKEY_SignatureLen(pubkey)).
+  // DSAU_DecodeDerSigToLen converts from the former format to the latter.
+  SECItem derSignatureSECItem(UnsafeMapInputToSECItem(sd.signature));
+  size_t signatureLen = SECKEY_SignatureLen(pubkey.get());
+  if (signatureLen == 0) {
+    return MapPRErrorCodeToResult(PR_GetError());
+  }
+  UniqueSECItem signatureSECItem(DSAU_DecodeDerSigToLen(&derSignatureSECItem,
+                                                        signatureLen));
+  if (!signatureSECItem) {
+    return MapPRErrorCodeToResult(PR_GetError());
+  }
+  SECItem digestSECItem(UnsafeMapInputToSECItem(sd.digest));
+  SECStatus srv = PK11_Verify(pubkey.get(), signatureSECItem.get(),
+                              &digestSECItem, nullptr);
+  if (srv != SECSuccess) {
+    return MapPRErrorCodeToResult(PR_GetError());
+  }
+
+  return Success;
+}
+
 Result
 CTLogVerifier::VerifySignature(Input data, Input signature)
 {
   uint8_t digest[SHA256_LENGTH];
   Result rv = DigestBufNSS(data, DigestAlgorithm::sha256, digest,
                            ArrayLength(digest));
   if (rv != Success) {
     return rv;
@@ -267,17 +327,17 @@ CTLogVerifier::VerifySignature(Input dat
     return rv;
   }
 
   switch (mSignatureAlgorithm) {
     case DigitallySigned::SignatureAlgorithm::RSA:
       rv = VerifyRSAPKCS1SignedDigestNSS(signedDigest, spki, nullptr);
       break;
     case DigitallySigned::SignatureAlgorithm::ECDSA:
-      rv = VerifyECDSASignedDigestNSS(signedDigest, spki, nullptr);
+      rv = FasterVerifyECDSASignedDigestNSS(signedDigest, mPublicECKey);
       break;
     // We do not expect new values added to this enum any time soon,
     // so just listing all the available ones seems to be the easiest way
     // to suppress warning C4061 on MSVC (which expects all values of the
     // enum to be explicitly handled).
     case DigitallySigned::SignatureAlgorithm::Anonymous:
     case DigitallySigned::SignatureAlgorithm::DSA:
     default:
--- a/security/certverifier/CTLogVerifier.h
+++ b/security/certverifier/CTLogVerifier.h
@@ -6,16 +6,17 @@
 
 #ifndef CTLogVerifier_h
 #define CTLogVerifier_h
 
 #include "CTLog.h"
 #include "pkix/Input.h"
 #include "pkix/pkix.h"
 #include "pkix/Result.h"
+#include "ScopedNSSTypes.h"
 #include "SignedCertificateTimestamp.h"
 #include "SignedTreeHead.h"
 
 namespace mozilla { namespace ct {
 
 // Verifies Signed Certificate Timestamps (SCTs) provided by a specific log
 // using the public key of that log. Assumes the SCT being verified
 // matches the log by log key ID and signature parameters (an error is returned
@@ -67,16 +68,21 @@ private:
   // Performs the underlying verification using the log's public key. Note
   // that |signature| contains the raw signature data (i.e. without any
   // DigitallySigned struct encoding).
   // Returns Success if passed verification, ERROR_BAD_SIGNATURE if failed
   // verification, or other result on error.
   pkix::Result VerifySignature(pkix::Input data, pkix::Input signature);
   pkix::Result VerifySignature(const Buffer& data, const Buffer& signature);
 
+  // mPublicECKey works around an architectural deficiency in NSS. In the case
+  // of EC, if we don't create, import, and cache this key, NSS will import and
+  // verify it every signature verification, which is slow. For RSA, this is
+  // unused and will be null.
+  UniqueSECKEYPublicKey mPublicECKey;
   Buffer mSubjectPublicKeyInfo;
   Buffer mKeyId;
   DigitallySigned::SignatureAlgorithm mSignatureAlgorithm;
   CTLogOperatorId mOperatorId;
   bool mDisqualified;
   uint64_t mDisqualificationTime;
 };
 
--- a/security/certverifier/tests/gtest/CTLogVerifierTest.cpp
+++ b/security/certverifier/tests/gtest/CTLogVerifierTest.cpp
@@ -61,67 +61,70 @@ TEST_F(CTLogVerifierTest, FailsInvalidTi
   GetX509CertLogEntry(certEntry);
 
   SignedCertificateTimestamp certSct;
   GetX509CertSCT(certSct);
 
   // Mangle the timestamp, so that it should fail signature validation.
   certSct.timestamp = 0;
 
-  EXPECT_EQ(Result::ERROR_BAD_SIGNATURE, mLog.Verify(certEntry, certSct));
+  EXPECT_EQ(pkix::Result::ERROR_BAD_SIGNATURE, mLog.Verify(certEntry, certSct));
 }
 
 TEST_F(CTLogVerifierTest, FailsInvalidSignature)
 {
   LogEntry certEntry;
   GetX509CertLogEntry(certEntry);
 
-  // Mangle the signature, making VerifyECDSASignedDigestNSS (used by
-  // CTLogVerifier) return ERROR_BAD_SIGNATURE.
+  // Mangle the value of the signature, making the underlying signature
+  // verification code return ERROR_BAD_SIGNATURE.
   SignedCertificateTimestamp certSct;
   GetX509CertSCT(certSct);
   certSct.signature.signatureData[20] ^= '\xFF';
-  EXPECT_EQ(Result::ERROR_BAD_SIGNATURE, mLog.Verify(certEntry, certSct));
+  EXPECT_EQ(pkix::Result::ERROR_BAD_SIGNATURE, mLog.Verify(certEntry, certSct));
 
-  // Make VerifyECDSASignedDigestNSS return ERROR_BAD_DER. We still expect
-  // the verifier to return ERROR_BAD_SIGNATURE.
+  // Mangle the encoding of the signature, making the underlying implementation
+  // return ERROR_BAD_DER. We still expect the verifier to return
+  // ERROR_BAD_SIGNATURE.
   SignedCertificateTimestamp certSct2;
   GetX509CertSCT(certSct2);
   certSct2.signature.signatureData[0] ^= '\xFF';
-  EXPECT_EQ(Result::ERROR_BAD_SIGNATURE, mLog.Verify(certEntry, certSct2));
+  EXPECT_EQ(pkix::Result::ERROR_BAD_SIGNATURE, mLog.Verify(certEntry,
+                                                           certSct2));
 }
 
 TEST_F(CTLogVerifierTest, FailsInvalidLogID)
 {
   LogEntry certEntry;
   GetX509CertLogEntry(certEntry);
 
   SignedCertificateTimestamp certSct;
   GetX509CertSCT(certSct);
 
   // Mangle the log ID, which should cause it to match a different log before
   // attempting signature validation.
   MOZ_RELEASE_ASSERT(certSct.logId.append('\x0'));
 
-  EXPECT_EQ(Result::FATAL_ERROR_INVALID_ARGS, mLog.Verify(certEntry, certSct));
+  EXPECT_EQ(pkix::Result::FATAL_ERROR_INVALID_ARGS, mLog.Verify(certEntry,
+                                                                certSct));
 }
 
 TEST_F(CTLogVerifierTest, VerifiesValidSTH)
 {
   SignedTreeHead sth;
   GetSampleSignedTreeHead(sth);
   EXPECT_EQ(Success, mLog.VerifySignedTreeHead(sth));
 }
 
 TEST_F(CTLogVerifierTest, DoesNotVerifyInvalidSTH)
 {
   SignedTreeHead sth;
   GetSampleSignedTreeHead(sth);
   sth.sha256RootHash[0] ^= '\xFF';
-  EXPECT_EQ(Result::ERROR_BAD_SIGNATURE, mLog.VerifySignedTreeHead(sth));
+  EXPECT_EQ(pkix::Result::ERROR_BAD_SIGNATURE, mLog.VerifySignedTreeHead(sth));
 }
 
 // Test that excess data after the public key is rejected.
 TEST_F(CTLogVerifierTest, ExcessDataInPublicKey)
 {
   Buffer key = GetTestPublicKey();
   MOZ_RELEASE_ASSERT(key.append("extra", 5));
 
--- a/security/manager/ssl/nsNSSComponent.cpp
+++ b/security/manager/ssl/nsNSSComponent.cpp
@@ -1834,16 +1834,20 @@ nsNSSComponent::ShutdownNSS()
     // those resources are cleaned up.
     Unused << SSL_ShutdownServerSessionIDCache();
 
     MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("evaporating psm resources"));
     if (NS_FAILED(nsNSSShutDownList::evaporateAllNSSResourcesAndShutDown())) {
       MOZ_LOG(gPIPNSSLog, LogLevel::Error, ("failed to evaporate resources"));
       return;
     }
+    // Release the default CertVerifier. This will cause any held NSS resources
+    // to be released (it's not an nsNSSShutDownObject, so we have to do this
+    // manually).
+    mDefaultCertVerifier = nullptr;
     UnloadLoadableRoots();
     if (SECSuccess != ::NSS_Shutdown()) {
       MOZ_LOG(gPIPNSSLog, LogLevel::Error, ("NSS SHUTDOWN FAILURE"));
     } else {
       MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("NSS shutdown =====>> OK <<====="));
     }
   }
 }
--- a/security/nss.symbols
+++ b/security/nss.symbols
@@ -434,16 +434,17 @@ PK11_Sign
 PK11_SignatureLen
 PK11_SignWithMechanism
 PK11_TokenKeyGenWithFlags
 PK11_UnwrapPrivKey
 PK11_UnwrapSymKey
 PK11_UpdateSlotAttribute
 PK11_UserDisableSlot
 PK11_UserEnableSlot
+PK11_Verify
 PK11_VerifyWithMechanism
 PK11_WrapPrivKey
 PK11_WrapSymKey
 PORT_Alloc
 PORT_Alloc_Util
 PORT_ArenaAlloc
 PORT_ArenaAlloc_Util
 PORT_ArenaGrow_Util