From 0ca7b665de9496f4ef2c4c19733751f1e1ff3ffb Mon Sep 17 00:00:00 2001 From: Dana Keeler Date: Thu, 16 Jul 2020 21:17:53 +0000 Subject: [PATCH] Bug 1653029 - pass a span of bytes to RootCABinNumber instead of NSS types r=rmf,kjacobs PSM internals are currently tightly-copuled with NSS data types. In many cases this is unnecessary, because PSM often needs only a sequence of bytes (in the case of certificates, for example). This tight coupling can also have performance and architectural impacts. For example, thread contention for NSS resources has caused performance issues in the past. This patch starts the process of avoiding using these types as much as possible in PSM. More specifically, RootCABinNumber can take a Span instead of a SECItem. Instead of taking a PK11SlotInfo (which essentially requires having a CERTCertificate), we can use PK11_FindEncodedCertInSlot to see if the certificate exists on a small number of specific slots to achieve the same effect as before. Doing this should eventually allow us to avoid creating a CERTCertificate, which implicitly involves searching all slots on all modules. Differential Revision: https://phabricator.services.mozilla.com/D83682 --- .../manager/ssl/PublicKeyPinningService.cpp | 5 +- .../ssl/RootCertificateTelemetryUtils.cpp | 62 +++++++++++-------- .../ssl/RootCertificateTelemetryUtils.h | 6 +- .../manager/ssl/SSLServerCertVerification.cpp | 10 ++- security/nss.symbols | 1 + 5 files changed, 51 insertions(+), 33 deletions(-) diff --git a/security/manager/ssl/PublicKeyPinningService.cpp b/security/manager/ssl/PublicKeyPinningService.cpp index e39b5c5686f4..5c34ee95a9a9 100644 --- a/security/manager/ssl/PublicKeyPinningService.cpp +++ b/security/manager/ssl/PublicKeyPinningService.cpp @@ -277,8 +277,9 @@ static nsresult CheckPinsForHostname( if (!rootCertObj) { return NS_ERROR_FAILURE; } - int32_t binNumber = - RootCABinNumber(&rootCertObj->derCert, rootCertObj->slot); + Span certSpan = + MakeSpan(rootCertObj->derCert.data, rootCertObj->derCert.len); + int32_t binNumber = RootCABinNumber(certSpan); if (binNumber != ROOT_CERTIFICATE_UNKNOWN) { pinningTelemetryInfo->accumulateForRoot = true; pinningTelemetryInfo->rootBucket = binNumber; diff --git a/security/manager/ssl/RootCertificateTelemetryUtils.cpp b/security/manager/ssl/RootCertificateTelemetryUtils.cpp index 7c2d02a11209..d8b979731b01 100644 --- a/security/manager/ssl/RootCertificateTelemetryUtils.cpp +++ b/security/manager/ssl/RootCertificateTelemetryUtils.cpp @@ -12,6 +12,7 @@ #include "mozilla/Logging.h" #include "nsINSSComponent.h" #include "nsNSSCertHelper.h" +#include "pk11pub.h" namespace mozilla { namespace psm { @@ -43,13 +44,13 @@ class BinaryHashSearchArrayComparator { // structure for a matching bin number. // If no matching root is found, this may be a CA from the softoken (cert9.db), // it may be a CA from an external PKCS#11 token, or it may be a CA from OS -// storage (Enterprise Root). The slot argument is used to attempt to determine -// this. See also the constants in RootCertificateTelemetryUtils.h. -int32_t RootCABinNumber(const SECItem* cert, PK11SlotInfo* slot) { +// storage (Enterprise Root). +// See also the constants in RootCertificateTelemetryUtils.h. +int32_t RootCABinNumber(const Span cert) { Digest digest; // Compute SHA256 hash of the certificate - nsresult rv = digest.DigestBuf(SEC_OID_SHA256, cert->data, cert->len); + nsresult rv = digest.DigestBuf(SEC_OID_SHA256, cert.data(), cert.size()); if (NS_WARN_IF(NS_FAILED(rv))) { return ROOT_CERTIFICATE_HASH_FAILURE; } @@ -73,23 +74,9 @@ int32_t RootCABinNumber(const SECItem* cert, PK11SlotInfo* slot) { return (int32_t)ROOT_TABLE[idx].binNumber; } - // Didn't match. It may be from the softoken, an external PKCS#11 token, or - // imported from the OS as an "Enterprise Root". - UniquePK11SlotInfo softokenSlot(PK11_GetInternalKeySlot()); - if (!softokenSlot) { - return ROOT_CERTIFICATE_UNKNOWN; - } - if (slot == softokenSlot.get()) { - return ROOT_CERTIFICATE_SOFTOKEN; - } - UniquePK11SlotInfo nssInternalSlot(PK11_GetInternalSlot()); - UniqueSECMODModule rootsModule(SECMOD_FindModule(kRootModuleName)); - if (!rootsModule || rootsModule->slotCount != 1) { - return ROOT_CERTIFICATE_UNKNOWN; - } - if (slot && slot != nssInternalSlot.get() && slot != rootsModule->slots[0]) { - return ROOT_CERTIFICATE_EXTERNAL_TOKEN; - } + // Didn't find this certificate in the built-in list. It may be an enterprise + // root (gathered from the OS) or it may be from the softoken or an external + // PKCS#11 token. nsCOMPtr component(do_GetService(PSM_COMPONENT_CONTRACTID)); if (!component) { return ROOT_CERTIFICATE_UNKNOWN; @@ -100,13 +87,38 @@ int32_t RootCABinNumber(const SECItem* cert, PK11SlotInfo* slot) { return ROOT_CERTIFICATE_UNKNOWN; } for (const auto& enterpriseRoot : enterpriseRoots) { - if (enterpriseRoot.Length() == cert->len && - memcmp(enterpriseRoot.Elements(), cert->data, + if (enterpriseRoot.Length() == cert.size() && + memcmp(enterpriseRoot.Elements(), cert.data(), enterpriseRoot.Length()) == 0) { return ROOT_CERTIFICATE_ENTERPRISE_ROOT; } } + SECItem certItem = {siBuffer, cert.data(), + static_cast(cert.size())}; + UniquePK11SlotInfo softokenSlot(PK11_GetInternalKeySlot()); + if (!softokenSlot) { + return ROOT_CERTIFICATE_UNKNOWN; + } + CK_OBJECT_HANDLE softokenCertHandle = + PK11_FindEncodedCertInSlot(softokenSlot.get(), &certItem, nullptr); + if (softokenCertHandle != CK_INVALID_HANDLE) { + return ROOT_CERTIFICATE_SOFTOKEN; + } + // In theory this should never find the certificate in the root module, + // because then it should have already matched our built-in list. This is + // here as a backstop to catch situations where a built-in root was added but + // the built-in telemetry information was not updated. + UniqueSECMODModule rootsModule(SECMOD_FindModule(kRootModuleName)); + if (!rootsModule || rootsModule->slotCount != 1) { + return ROOT_CERTIFICATE_UNKNOWN; + } + CK_OBJECT_HANDLE builtinCertHandle = + PK11_FindEncodedCertInSlot(rootsModule->slots[0], &certItem, nullptr); + if (builtinCertHandle == CK_INVALID_HANDLE) { + return ROOT_CERTIFICATE_EXTERNAL_TOKEN; + } + // We have no idea what this is. return ROOT_CERTIFICATE_UNKNOWN; } @@ -114,8 +126,8 @@ int32_t RootCABinNumber(const SECItem* cert, PK11SlotInfo* slot) { // Attempt to increment the appropriate bin in the provided Telemetry probe ID. // If there was a hash failure, we do nothing. void AccumulateTelemetryForRootCA(mozilla::Telemetry::HistogramID probe, - const CERTCertificate* cert) { - int32_t binId = RootCABinNumber(&cert->derCert, cert->slot); + const Span cert) { + int32_t binId = RootCABinNumber(cert); if (binId != ROOT_CERTIFICATE_HASH_FAILURE) { Accumulate(probe, binId); diff --git a/security/manager/ssl/RootCertificateTelemetryUtils.h b/security/manager/ssl/RootCertificateTelemetryUtils.h index bb8dd53b39e7..af2c89d51d0e 100644 --- a/security/manager/ssl/RootCertificateTelemetryUtils.h +++ b/security/manager/ssl/RootCertificateTelemetryUtils.h @@ -7,8 +7,8 @@ #ifndef RootCertificateTelemetryUtils_h #define RootCertificateTelemetryUtils_h +#include "mozilla/Span.h" #include "mozilla/Telemetry.h" -#include "certt.h" namespace mozilla { namespace psm { @@ -26,10 +26,10 @@ namespace psm { #define ROOT_CERTIFICATE_ENTERPRISE_ROOT 3 #define ROOT_CERTIFICATE_HASH_FAILURE -1 -int32_t RootCABinNumber(const SECItem* cert, PK11SlotInfo* slot); +int32_t RootCABinNumber(const Span cert); void AccumulateTelemetryForRootCA(mozilla::Telemetry::HistogramID probe, - const CERTCertificate* cert); + const Span cert); } // namespace psm } // namespace mozilla diff --git a/security/manager/ssl/SSLServerCertVerification.cpp b/security/manager/ssl/SSLServerCertVerification.cpp index 8f7635e0af44..d8cb518aaabe 100644 --- a/security/manager/ssl/SSLServerCertVerification.cpp +++ b/security/manager/ssl/SSLServerCertVerification.cpp @@ -806,8 +806,10 @@ void GatherRootCATelemetry(const UniqueCERTCertList& certList) { if (!rootCert) { return; } + Span certSpan = + MakeSpan(rootCert->derCert.data, rootCert->derCert.len); AccumulateTelemetryForRootCA(Telemetry::CERT_VALIDATION_SUCCESS_BY_CA, - rootCert); + certSpan); } // There are various things that we want to measure about certificate @@ -924,15 +926,17 @@ void GatherCertificateTransparencyTelemetry( } // Report CT Policy compliance by CA. + Span certSpan = + MakeSpan(rootCert->derCert.data, rootCert->derCert.len); switch (info.policyCompliance) { case ct::CTPolicyCompliance::Compliant: AccumulateTelemetryForRootCA( - Telemetry::SSL_CT_POLICY_COMPLIANT_CONNECTIONS_BY_CA, rootCert); + Telemetry::SSL_CT_POLICY_COMPLIANT_CONNECTIONS_BY_CA, certSpan); break; case ct::CTPolicyCompliance::NotEnoughScts: case ct::CTPolicyCompliance::NotDiverseScts: AccumulateTelemetryForRootCA( - Telemetry::SSL_CT_POLICY_NON_COMPLIANT_CONNECTIONS_BY_CA, rootCert); + Telemetry::SSL_CT_POLICY_NON_COMPLIANT_CONNECTIONS_BY_CA, certSpan); break; case ct::CTPolicyCompliance::Unknown: default: diff --git a/security/nss.symbols b/security/nss.symbols index f72212176fd4..7dffdb0746a9 100644 --- a/security/nss.symbols +++ b/security/nss.symbols @@ -317,6 +317,7 @@ PK11_FindCertFromNickname PK11_FindCertInSlot PK11_FindCertsFromEmailAddress PK11_FindCertsFromNickname +PK11_FindEncodedCertInSlot PK11_FindKeyByAnyCert PK11_FindKeyByDERCert PK11_FindKeyByKeyID