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

--HG--
extra : rebase_source : 4994db9de80a6c1aec3d7e322ff30d040140ce92

security/certverifier/CTLogVerifier.cpp

@@ -162,6 +162,34 @@ CTLogVerifier::Init(Input subjectPublicKeyInfo,
     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;
   }
@@ -240,6 +268,38 @@ CTLogVerifier::SignatureParametersMatch(const DigitallySigned& signature)
     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)
 {
@@ -272,7 +332,7 @@ CTLogVerifier::VerifySignature(Input data, Input signature)
       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

security/certverifier/CTLogVerifier.h

@@ -11,6 +11,7 @@
 #include "pkix/Input.h"
 #include "pkix/pkix.h"
 #include "pkix/Result.h"
+#include "ScopedNSSTypes.h"
 #include "SignedCertificateTimestamp.h"
 #include "SignedTreeHead.h"
 
@@ -72,6 +73,11 @@ private:
   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;

security/certverifier/tests/gtest/CTLogVerifierTest.cpp

@@ -66,7 +66,7 @@ TEST_F(CTLogVerifierTest, FailsInvalidTimestamp)
   // 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)
@@ -74,19 +74,21 @@ 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)
@@ -101,7 +103,8 @@ TEST_F(CTLogVerifierTest, FailsInvalidLogID)
   // 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)
@@ -116,7 +119,7 @@ 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.

security/manager/ssl/nsNSSComponent.cpp

@@ -1839,6 +1839,10 @@ nsNSSComponent::ShutdownNSS()
       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"));

security/nss.symbols

@@ -439,6 +439,7 @@ PK11_UnwrapSymKey
 PK11_UpdateSlotAttribute
 PK11_UserDisableSlot
 PK11_UserEnableSlot
+PK11_Verify
 PK11_VerifyWithMechanism
 PK11_WrapPrivKey
 PK11_WrapSymKey