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