bug 1529044 - intermediate certificate caching: import on a background thread to not block certificate verification r=mgoodwin

Apparently importing a certificate into the NSS certificate DB is slow enough to
materially impact the time it takes to connect to a site. This patch addresses
this by importing any intermediate certificates we want to cache from verified
connections on a background thread (so the certificate verification thread can
return faster).

Differential Revision: https://phabricator.services.mozilla.com/D24384

--HG--
extra : moz-landing-system : lando

security/certverifier/NSSCertDBTrustDomain.cpp

@@ -8,6 +8,7 @@
 
 #include <stdint.h>
 
+#include "CryptoTask.h"
 #include "ExtendedValidation.h"
 #include "NSSErrorsService.h"
 #include "OCSPVerificationTrustDomain.h"
@@ -19,9 +20,11 @@
 #include "mozilla/Casting.h"
 #include "mozilla/Move.h"
 #include "mozilla/PodOperations.h"
+#include "mozilla/Services.h"
 #include "mozilla/TimeStamp.h"
 #include "mozilla/Unused.h"
 #include "nsCRTGlue.h"
+#include "nsIObserverService.h"
 #include "nsNSSCertHelper.h"
 #include "nsNSSCertValidity.h"
 #include "nsNSSCertificate.h"
@@ -1284,6 +1287,46 @@ nsresult BuildRevocationCheckStrings(const CERTCertificate* cert,
   return NS_OK;
 }
 
+// Helper for SaveIntermediateCerts. Does the actual importing work on a
+// background thread so certificate verification can return its result.
+class BackgroundSaveIntermediateCertsTask final : public CryptoTask {
+ public:
+  explicit BackgroundSaveIntermediateCertsTask(
+      UniqueCERTCertList&& intermediates)
+      : mIntermediates(std::move(intermediates)) {}
+
+ private:
+  virtual nsresult CalculateResult() override {
+    UniquePK11SlotInfo slot(PK11_GetInternalKeySlot());
+    if (!slot) {
+      return NS_ERROR_FAILURE;
+    }
+    for (CERTCertListNode* node = CERT_LIST_HEAD(mIntermediates);
+         !CERT_LIST_END(node, mIntermediates); node = CERT_LIST_NEXT(node)) {
+      // This is a best-effort attempt at avoiding unknown issuer errors in the
+      // future, so ignore failures here.
+      nsAutoCString nickname;
+      if (NS_FAILED(DefaultServerNicknameForCert(node->cert, nickname))) {
+        continue;
+      }
+      Unused << PK11_ImportCert(slot.get(), node->cert, CK_INVALID_HANDLE,
+                                nickname.get(), false);
+    }
+    return NS_OK;
+  }
+
+  virtual void CallCallback(nsresult rv) override {
+    nsCOMPtr<nsIObserverService> observerService =
+        mozilla::services::GetObserverService();
+    if (observerService) {
+      observerService->NotifyObservers(nullptr, "psm:intermediate-certs-cached",
+                                       nullptr);
+    }
+  }
+
+  UniqueCERTCertList mIntermediates;
+};
+
 /**
  * Given a list of certificates representing a verified certificate path from an
  * end-entity certificate to a trust anchor, imports the intermediate
@@ -1298,12 +1341,13 @@ void SaveIntermediateCerts(const UniqueCERTCertList& certList) {
     return;
   }
 
-  UniquePK11SlotInfo slot(PK11_GetInternalKeySlot());
-  if (!slot) {
+  UniqueCERTCertList intermediates(CERT_NewCertList());
+  if (!intermediates) {
     return;
   }
 
   bool isEndEntity = true;
+  size_t numIntermediates = 0;
   for (CERTCertListNode* node = CERT_LIST_HEAD(certList);
        !CERT_LIST_END(node, certList); node = CERT_LIST_NEXT(node)) {
     if (isEndEntity) {
@@ -1333,17 +1377,20 @@ void SaveIntermediateCerts(const UniqueCERTCertList& certList) {
       continue;
     }
 
-    nsAutoCString nickname;
-    nsresult rv = DefaultServerNicknameForCert(node->cert, nickname);
-    if (NS_FAILED(rv)) {
-      continue;
+    UniqueCERTCertificate certHandle(CERT_DupCertificate(node->cert));
+    if (CERT_AddCertToListTail(intermediates.get(), certHandle.get()) !=
+        SECSuccess) {
+      // If this fails, we're probably out of memory. Just return.
+      return;
     }
+    certHandle.release();  // intermediates now owns the reference
+    numIntermediates++;
+  }
 
-    // As mentioned in the documentation of this function, we're importing only
-    // to cope with misconfigured servers. As such, we ignore the return value
-    // below, since it doesn't really matter if the import fails.
-    Unused << PK11_ImportCert(slot.get(), node->cert, CK_INVALID_HANDLE,
-                              nickname.get(), false);
+  if (numIntermediates > 0) {
+    RefPtr<BackgroundSaveIntermediateCertsTask> task =
+        new BackgroundSaveIntermediateCertsTask(std::move(intermediates));
+    Unused << task->Dispatch("ImportInts");
   }
 }
 

security/manager/ssl/tests/unit/test_missing_intermediate.js

@@ -8,23 +8,81 @@
 // Tests that if a server does not send a complete certificate chain, we can
 // make use of cached intermediates to build a trust path.
 
+const {TestUtils} = ChromeUtils.import("resource://testing-common/TestUtils.jsm");
+
 do_get_profile(); // must be called before getting nsIX509CertDB
 const certdb  = Cc["@mozilla.org/security/x509certdb;1"]
                   .getService(Ci.nsIX509CertDB);
 
+function run_certutil_on_directory(directory, args) {
+  let envSvc = Cc["@mozilla.org/process/environment;1"]
+                 .getService(Ci.nsIEnvironment);
+  let greBinDir = Services.dirsvc.get("GreBinD", Ci.nsIFile);
+  envSvc.set("DYLD_LIBRARY_PATH", greBinDir.path);
+  // TODO(bug 1107794): Android libraries are in /data/local/xpcb, but "GreBinD"
+  // does not return this path on Android, so hard code it here.
+  envSvc.set("LD_LIBRARY_PATH", greBinDir.path + ":/data/local/xpcb");
+  let certutilBin = _getBinaryUtil("certutil");
+  let process = Cc["@mozilla.org/process/util;1"].createInstance(Ci.nsIProcess);
+  process.init(certutilBin);
+  args.push("-d");
+  args.push(`sql:${directory}`);
+  process.run(true, args, args.length);
+  Assert.equal(process.exitValue, 0, "certutil should succeed");
+}
+
+registerCleanupFunction(() => {
+  let certDir = Services.dirsvc.get("CurWorkD", Ci.nsIFile);
+  certDir.append("bad_certs");
+  Assert.ok(certDir.exists(), "bad_certs should exist");
+  let args = [ "-D", "-n", "manually-added-missing-intermediate" ];
+  run_certutil_on_directory(certDir.path, args);
+});
+
 function run_test() {
-  addCertFromFile(certdb, "bad_certs/test-ca.pem", "CTu,,");
   add_tls_server_setup("BadCertServer", "bad_certs");
   // If we don't know about the intermediate, we'll get an unknown issuer error.
   add_connection_test("ee-from-missing-intermediate.example.com",
                       SEC_ERROR_UNKNOWN_ISSUER);
+
+  // Make BadCertServer aware of the intermediate.
   add_test(() => {
-    addCertFromFile(certdb, "test_missing_intermediate/missing-intermediate.pem",
-                    ",,");
+    let args = [ "-A", "-n", "manually-added-missing-intermediate", "-a", "-i",
+                 "test_missing_intermediate/missing-intermediate.pem", "-t",
+                 ",," ];
+    let certDir = Services.dirsvc.get("CurWorkD", Ci.nsIFile);
+    certDir.append("bad_certs");
+    Assert.ok(certDir.exists(), "bad_certs should exist");
+    run_certutil_on_directory(certDir.path, args);
     run_next_test();
   });
-  // Now that we've cached the intermediate, the connection should succeed.
+
+  // We have to start observing the topic before there's a chance it gets
+  // emitted.
+  add_test(() => {
+    TestUtils.topicObserved("psm:intermediate-certs-cached").then(run_next_test);
+    run_next_test();
+  });
+  // Connect and cache the intermediate.
   add_connection_test("ee-from-missing-intermediate.example.com",
                       PRErrorCodeSuccess);
+
+  // Add a dummy test so that the only way we advance from here is by observing
+  // "psm:intermediate-certs-cached".
+  add_test(() => {});
+
+  // Confirm that we cached the intermediate by running a certutil command that
+  // will fail if we didn't.
+  add_test(() => {
+    // Here we have to use the name that gecko chooses to give it.
+    let args = [ "-D", "-n", "Missing Intermediate" ];
+    run_certutil_on_directory(do_get_profile().path, args);
+    run_next_test();
+  });
+
+  // Since we cached the intermediate, this should succeed.
+  add_connection_test("ee-from-missing-intermediate.example.com",
+                      PRErrorCodeSuccess);
+
   run_next_test();
 }