diff --git a/security/certverifier/NSSCertDBTrustDomain.cpp b/security/certverifier/NSSCertDBTrustDomain.cpp index 2a6ebbcfad44..3fa38a499af5 100644 --- a/security/certverifier/NSSCertDBTrustDomain.cpp +++ b/security/certverifier/NSSCertDBTrustDomain.cpp @@ -8,6 +8,7 @@ #include +#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 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 task = + new BackgroundSaveIntermediateCertsTask(std::move(intermediates)); + Unused << task->Dispatch("ImportInts"); } } diff --git a/security/manager/ssl/tests/unit/test_missing_intermediate.js b/security/manager/ssl/tests/unit/test_missing_intermediate.js index d5119469f353..46e21a2fc31c 100644 --- a/security/manager/ssl/tests/unit/test_missing_intermediate.js +++ b/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(); }