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

security/manager/ssl/PublicKeyPinningService.cpp

@@ -277,8 +277,9 @@ static nsresult CheckPinsForHostname(
           if (!rootCertObj) {
             return NS_ERROR_FAILURE;
           }
-          int32_t binNumber =
+          Span<uint8_t> certSpan =
-              RootCABinNumber(&rootCertObj->derCert, rootCertObj->slot);
+              MakeSpan(rootCertObj->derCert.data, rootCertObj->derCert.len);
+          int32_t binNumber = RootCABinNumber(certSpan);
           if (binNumber != ROOT_CERTIFICATE_UNKNOWN) {
             pinningTelemetryInfo->accumulateForRoot = true;
             pinningTelemetryInfo->rootBucket = binNumber;

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
+// storage (Enterprise Root).
-// this. See also the constants in RootCertificateTelemetryUtils.h.
+// See also the constants in RootCertificateTelemetryUtils.h.
-int32_t RootCABinNumber(const SECItem* cert, PK11SlotInfo* slot) {
+int32_t RootCABinNumber(const Span<uint8_t> 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
+  // Didn't find this certificate in the built-in list. It may be an enterprise
-  // imported from the OS as an "Enterprise Root".
+  // root (gathered from the OS) or it may be from the softoken or an external
-  UniquePK11SlotInfo softokenSlot(PK11_GetInternalKeySlot());
+  // PKCS#11 token.
-  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;
-  }
   nsCOMPtr<nsINSSComponent> 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 &&
+    if (enterpriseRoot.Length() == cert.size() &&
-        memcmp(enterpriseRoot.Elements(), cert->data,
+        memcmp(enterpriseRoot.Elements(), cert.data(),
                enterpriseRoot.Length()) == 0) {
       return ROOT_CERTIFICATE_ENTERPRISE_ROOT;
     }
   }
 
+  SECItem certItem = {siBuffer, cert.data(),
+                      static_cast<unsigned int>(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) {
+                                  const Span<uint8_t> cert) {
-  int32_t binId = RootCABinNumber(&cert->derCert, cert->slot);
+  int32_t binId = RootCABinNumber(cert);
 
   if (binId != ROOT_CERTIFICATE_HASH_FAILURE) {
     Accumulate(probe, binId);

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<uint8_t> cert);
 
 void AccumulateTelemetryForRootCA(mozilla::Telemetry::HistogramID probe,
-                                  const CERTCertificate* cert);
+                                  const Span<uint8_t> cert);
 
 }  // namespace psm
 }  // namespace mozilla

security/manager/ssl/SSLServerCertVerification.cpp

@@ -806,8 +806,10 @@ void GatherRootCATelemetry(const UniqueCERTCertList& certList) {
   if (!rootCert) {
     return;
   }
+  Span<uint8_t> 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<uint8_t> 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:

security/nss.symbols

@@ -317,6 +317,7 @@ PK11_FindCertFromNickname
 PK11_FindCertInSlot
 PK11_FindCertsFromEmailAddress
 PK11_FindCertsFromNickname
+PK11_FindEncodedCertInSlot
 PK11_FindKeyByAnyCert
 PK11_FindKeyByDERCert
 PK11_FindKeyByKeyID