Bug 1551177 - avoid searching unproductive certificate paths during verification r=jcj,KevinJacobs

In bug 1056341 we introduced a search budget to mozilla::pkix to attempt to work
around the problem of having an extremely large search space given a set of
certificates all with the same subject and issuer distinguished names but
different public keys. In the end, though, there is probably no good value to
choose for the budget that is small enough to run quickly on the wide range of
hardware our users have and yet is large enough that we're confident won't break
someone's complicated pki setup (looking at you, the US federal government).

To address this, use the observation that as long as an intermediate can't *add*
information necessary to build a certificate chain (e.g. stapled SCTs), we
should never need a self-signed intermediate (as in, its own key verifies the
signature on it and its subject and issuer distinguished names are identical) to
build a trusted chain (since the exact same chain without that intermediate
should be valid). Given this, we simply skip all self-signed non-trust anchor
CA certificates during path building.

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

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

security/certverifier/NSSCertDBTrustDomain.cpp

@@ -36,6 +36,7 @@
 #include "mozpkix/Result.h"
 #include "mozpkix/pkix.h"
 #include "mozpkix/pkixnss.h"
+#include "mozpkix/pkixutil.h"
 #include "prerror.h"
 #include "secerr.h"
 
@@ -97,7 +98,52 @@ NSSCertDBTrustDomain::NSSCertDBTrustDomain(
 #endif
       mOCSPStaplingStatus(CertVerifier::OCSP_STAPLING_NEVER_CHECKED),
       mSCTListFromCertificate(),
-      mSCTListFromOCSPStapling() {}
+      mSCTListFromOCSPStapling() {
+}
+
+// A self-signed issuer certificate should never be necessary in order to build
+// a trusted certificate chain unless it is a trust anchor. This is because if
+// it were necessary, there would exist another certificate with the same
+// subject and public key that is also a valid issing certificate. Given this
+// certificate, it is possible to build another chain using just it instead of
+// it and the self-signed certificate. This is only true as long as the
+// certificate extensions we support are restrictive rather than additive in
+// terms of the rest of the chain (for example, we don't support policy mapping
+// and we ignore any SCT information in intermediates).
+bool NSSCertDBTrustDomain::ShouldSkipSelfSignedNonTrustAnchor(Input certDER) {
+  BackCert cert(certDER, EndEntityOrCA::MustBeCA, nullptr);
+  if (cert.Init() != Success) {
+    return false;  // turn any failures into "don't skip trying this cert"
+  }
+  // If subject != issuer, this isn't a self-signed cert.
+  if (!InputsAreEqual(cert.GetSubject(), cert.GetIssuer())) {
+    return false;
+  }
+  TrustLevel trust;
+  if (GetCertTrust(EndEntityOrCA::MustBeCA, CertPolicyId::anyPolicy, certDER,
+                   trust) != Success) {
+    return false;
+  }
+  // If the trust for this certificate is anything other than "inherit", we want
+  // to process it like normal.
+  if (trust != TrustLevel::InheritsTrust) {
+    return false;
+  }
+  uint8_t digestBuf[MAX_DIGEST_SIZE_IN_BYTES];
+  pkix::der::PublicKeyAlgorithm publicKeyAlg;
+  SignedDigest signature;
+  if (DigestSignedData(*this, cert.GetSignedData(), digestBuf, publicKeyAlg,
+                       signature) != Success) {
+    return false;
+  }
+  if (VerifySignedDigest(*this, publicKeyAlg, signature,
+                         cert.GetSubjectPublicKeyInfo()) != Success) {
+    return false;
+  }
+  // This is a self-signed, non-trust-anchor certificate, so we shouldn't use it
+  // for path building. See bug 1056341.
+  return true;
+}
 
 Result NSSCertDBTrustDomain::FindIssuer(Input encodedIssuerName,
                                         IssuerChecker& checker, Time) {
@@ -193,6 +239,9 @@ Result NSSCertDBTrustDomain::FindIssuer(Input encodedIssuerName,
   }
 
   for (Input candidate : rootCandidates) {
+    if (ShouldSkipSelfSignedNonTrustAnchor(candidate)) {
+      continue;
+    }
     bool keepGoing;
     Result rv = checker.Check(candidate, nameConstraintsInputPtr, keepGoing);
     if (rv != Success) {

security/certverifier/NSSCertDBTrustDomain.h

@@ -240,6 +240,7 @@ class NSSCertDBTrustDomain : public mozilla::pkix::TrustDomain {
   Result HandleOCSPFailure(const Result cachedResponseResult,
                            const Result stapledOCSPResponseResult,
                            const Result error);
+  bool ShouldSkipSelfSignedNonTrustAnchor(mozilla::pkix::Input certDER);
 
   const SECTrustType mCertDBTrustType;
   const OCSPFetching mOCSPFetching;

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

@@ -0,0 +1,58 @@
+// -*- indent-tabs-mode: nil; js-indent-level: 2 -*-
+// This Source Code Form is subject to the terms of the Mozilla Public
+// License, v. 2.0. If a copy of the MPL was not distributed with this
+// file, You can obtain one at http://mozilla.org/MPL/2.0/.
+"use strict";
+
+// This test uses a specially-crafted NSS cert DB containing 12 self-signed certificates that all
+// have the same subject and issuer distinguished name. Since they all have different keys and none
+// of them are trust anchors, there are a large number of potential trust paths that could be
+// explored. If our trust domain were naive enough to allow mozilla::pkix to explore them all, it
+// would take a long time to perform (mozilla::pkix does have the concept of a path-building budget,
+// but even on a fast computer, it takes an unacceptable amount of time to exhaust). To prevent the
+// full exploration of this space, NSSCertDBTrustDomain skips searching through self-signed
+// certificates that aren't trust anchors, since those would never otherwise be essential to
+// complete a path (note that this is only true as long as the extensions we support are restrictive
+// rather than additive).
+// When we try to verify one of these certificates in this test, we should finish relatively
+// quickly, even on slow hardware.
+// Should these certificates ever need regenerating, they were produced with the following commands:
+// certutil -N -d . --empty-password
+// for num in 00 01 02 03 04 05 06 07 08 09 10 11; do
+//   echo -ne "5\n6\n9\ny\ny\n\ny\n" | certutil -d . -S -s "CN=self-signed cert" -t ,, \
+//   -q secp256r1 -x -k ec -z <(date +%s) -1 -2 -n cert$num; sleep 2;
+// done
+
+add_task(async function run_test_no_overlong_path_building() {
+  let profile = do_get_profile();
+  const CERT_DB_NAME = "cert9.db";
+  let srcCertDBFile = do_get_file(`test_self_signed_certs/${CERT_DB_NAME}`);
+  srcCertDBFile.copyTo(profile, CERT_DB_NAME);
+
+  let certDB = Cc["@mozilla.org/security/x509certdb;1"].getService(Ci.nsIX509CertDB);
+  let certToVerify = null;
+  for (let cert of certDB.getCerts().getEnumerator()) {
+    if (cert.subjectName == "CN=self-signed cert") {
+      certToVerify = cert;
+      break;
+    }
+  }
+  notEqual(certToVerify, null, "should have found one of the preloaded self-signed certs");
+  let timeBefore = Date.now();
+  // As mentioned above, mozilla::pkix limits how much it will search for a trusted path, even if a
+  // trust domain keeps providing potential issuers. So, if we only tried to verify a certificate
+  // once, this test could potentially pass on a fast computer even if we weren't properly skipping
+  // unnecessary paths. If we were to try and lower our time limit (the comparison with
+  // secondsElapsed, below), this test would intermittently fail on slow hardware. By trying to
+  // verify the certificate 10 times, we hopefully end up with a meaningful test (it should still
+  // fail on fast hardware if we don't properly skip unproductive paths) that won't intermittently
+  // time out on slow hardware.
+  for (let i = 0; i < 10; i++) {
+    let date = new Date("2019-05-15T00:00:00.000Z");
+    await checkCertErrorGenericAtTime(certDB, certToVerify, SEC_ERROR_UNKNOWN_ISSUER,
+                                      certificateUsageSSLCA, date.getTime() / 1000);
+  }
+  let timeAfter = Date.now();
+  let secondsElapsed = (timeAfter - timeBefore) / 1000;
+  ok(secondsElapsed < 120, "verifications shouldn't take too long");
+});

security/manager/ssl/tests/unit/xpcshell.ini

@@ -34,6 +34,7 @@ support-files =
   test_sdr_preexisting/**
   test_sdr_preexisting_with_password/**
   test_sdr_upgraded_with_password/**
+  test_self_signed_certs/**
   test_signed_apps/**
   test_startcom_wosign/**
   test_symantec_apple_google/**
@@ -173,6 +174,7 @@ skip-if = toolkit == 'android'
 [test_sdr_upgraded_with_password.js]
 # Not relevant to Android. See the comment in the test.
 skip-if = toolkit == 'android'
+[test_self_signed_certs.js]
 [test_session_resumption.js]
 run-sequentially = hardcoded ports
 [test_signed_apps.js]