bug 1056341 - introduce a budget for path searching in mozilla::pkix to avoid unbounded search r=fkiefer,jcj
authorDavid Keeler <dkeeler@mozilla.com>
Tue, 27 Mar 2018 15:35:50 -0700
changeset 775165 9dcd9f186bbfb5a13d59eaa1b04cecc181ac631a
parent 775164 a49df97d2ad98b4bfd97e60000b2b2cc1c1663cf
child 775166 aeeb277faf1d195000912931b24f2eca9656afe2
push id104638
push userbmo:jlorenzo@mozilla.com
push dateFri, 30 Mar 2018 13:27:33 +0000
reviewersfkiefer, jcj
bugs1056341
milestone61.0a1
bug 1056341 - introduce a budget for path searching in mozilla::pkix to avoid unbounded search r=fkiefer,jcj MozReview-Commit-ID: Ght1wx5lb34
security/pkix/lib/pkixbuild.cpp
security/pkix/test/gtest/pkixbuild_tests.cpp
--- a/security/pkix/lib/pkixbuild.cpp
+++ b/security/pkix/lib/pkixbuild.cpp
@@ -31,41 +31,44 @@ namespace mozilla { namespace pkix {
 
 static Result BuildForward(TrustDomain& trustDomain,
                            const BackCert& subject,
                            Time time,
                            KeyUsage requiredKeyUsageIfPresent,
                            KeyPurposeId requiredEKUIfPresent,
                            const CertPolicyId& requiredPolicy,
                            /*optional*/ const Input* stapledOCSPResponse,
-                           unsigned int subCACount);
+                           unsigned int subCACount,
+                           unsigned int& buildForwardCallBudget);
 
 TrustDomain::IssuerChecker::IssuerChecker() { }
 TrustDomain::IssuerChecker::~IssuerChecker() { }
 
 // The implementation of TrustDomain::IssuerTracker is in a subclass only to
 // hide the implementation from external users.
 class PathBuildingStep final : public TrustDomain::IssuerChecker
 {
 public:
   PathBuildingStep(TrustDomain& aTrustDomain, const BackCert& aSubject,
                    Time aTime, KeyPurposeId aRequiredEKUIfPresent,
                    const CertPolicyId& aRequiredPolicy,
                    /*optional*/ const Input* aStapledOCSPResponse,
-                   unsigned int aSubCACount, Result aDeferredSubjectError)
+                   unsigned int aSubCACount, Result aDeferredSubjectError,
+                   unsigned int& aBuildForwardCallBudget)
     : trustDomain(aTrustDomain)
     , subject(aSubject)
     , time(aTime)
     , requiredEKUIfPresent(aRequiredEKUIfPresent)
     , requiredPolicy(aRequiredPolicy)
     , stapledOCSPResponse(aStapledOCSPResponse)
     , subCACount(aSubCACount)
     , deferredSubjectError(aDeferredSubjectError)
     , result(Result::FATAL_ERROR_LIBRARY_FAILURE)
     , resultWasSet(false)
+    , buildForwardCallBudget(aBuildForwardCallBudget)
   {
   }
 
   Result Check(Input potentialIssuerDER,
                /*optional*/ const Input* additionalNameConstraints,
                /*out*/ bool& keepGoing) override;
 
   Result CheckResult() const;
@@ -83,16 +86,17 @@ private:
   // Initialized lazily.
   uint8_t subjectSignatureDigestBuf[MAX_DIGEST_SIZE_IN_BYTES];
   der::PublicKeyAlgorithm subjectSignaturePublicKeyAlg;
   SignedDigest subjectSignature;
 
   Result RecordResult(Result currentResult, /*out*/ bool& keepGoing);
   Result result;
   bool resultWasSet;
+  unsigned int& buildForwardCallBudget;
 
   PathBuildingStep(const PathBuildingStep&) = delete;
   void operator=(const PathBuildingStep&) = delete;
 };
 
 Result
 PathBuildingStep::RecordResult(Result newResult, /*out*/ bool& keepGoing)
 {
@@ -187,21 +191,30 @@ PathBuildingStep::Check(Input potentialI
     }
   }
 
   rv = CheckTLSFeatures(subject, potentialIssuer);
   if (rv != Success) {
     return RecordResult(rv, keepGoing);
   }
 
+  // If we've ran out of budget, stop searching.
+  if (buildForwardCallBudget == 0) {
+    Result savedRv = RecordResult(Result::ERROR_UNKNOWN_ISSUER, keepGoing);
+    keepGoing = false;
+    return savedRv;
+  }
+  buildForwardCallBudget--;
+
   // RFC 5280, Section 4.2.1.3: "If the keyUsage extension is present, then the
   // subject public key MUST NOT be used to verify signatures on certificates
   // or CRLs unless the corresponding keyCertSign or cRLSign bit is set."
   rv = BuildForward(trustDomain, potentialIssuer, time, KeyUsage::keyCertSign,
-                    requiredEKUIfPresent, requiredPolicy, nullptr, subCACount);
+                    requiredEKUIfPresent, requiredPolicy, nullptr, subCACount,
+                    buildForwardCallBudget);
   if (rv != Success) {
     return RecordResult(rv, keepGoing);
   }
 
   // Calculate the digest of the subject's signed data if we haven't already
   // done so. We do this lazily to avoid doing it at all if we backtrack before
   // getting to this point. We cache the result to avoid recalculating it if we
   // backtrack after getting to this point.
@@ -280,17 +293,18 @@ PathBuildingStep::Check(Input potentialI
 static Result
 BuildForward(TrustDomain& trustDomain,
              const BackCert& subject,
              Time time,
              KeyUsage requiredKeyUsageIfPresent,
              KeyPurposeId requiredEKUIfPresent,
              const CertPolicyId& requiredPolicy,
              /*optional*/ const Input* stapledOCSPResponse,
-             unsigned int subCACount)
+             unsigned int subCACount,
+             unsigned int& buildForwardCallBudget)
 {
   Result rv;
 
   TrustLevel trustLevel;
   // If this is an end-entity and not a trust anchor, we defer reporting
   // any error found here until after attempting to find a valid chain.
   // See the explanation of error prioritization in pkix.h.
   rv = CheckIssuerIndependentProperties(trustDomain, subject, time,
@@ -338,17 +352,17 @@ BuildForward(TrustDomain& trustDomain,
     assert(subCACount == 0);
   }
 
   // Find a trusted issuer.
 
   PathBuildingStep pathBuilder(trustDomain, subject, time,
                                requiredEKUIfPresent, requiredPolicy,
                                stapledOCSPResponse, subCACount,
-                               deferredEndEntityError);
+                               deferredEndEntityError, buildForwardCallBudget);
 
   // TODO(bug 965136): Add SKI/AKI matching optimizations
   rv = trustDomain.FindIssuer(subject.GetIssuer(), pathBuilder, time);
   if (rv != Success) {
     return rv;
   }
 
   rv = pathBuilder.CheckResult();
@@ -377,14 +391,27 @@ BuildCertChain(TrustDomain& trustDomain,
   // XXX: Support the legacy use of the subject CN field for indicating the
   // domain name the certificate is valid for.
   BackCert cert(certDER, endEntityOrCA, nullptr);
   Result rv = cert.Init();
   if (rv != Success) {
     return rv;
   }
 
+  // See bug 1056341 for context. If mozilla::pkix is being used in an
+  // environment where there are many certificates that all have the same
+  // distinguished name as their subject and issuer (but different SPKIs - see
+  // the loop prevention as per RFC4158 Section 5.2 in PathBuildingStep::Check),
+  // the space to search becomes exponential. Because it would be prohibitively
+  // expensive to explore the entire space, we introduce a budget here that,
+  // when exhausted, terminates the search with the result
+  // Result::ERROR_UNKNOWN_ISSUER. Essentially, we limit the total number of
+  // times `BuildForward` can be called. The current value appears to be a good
+  // balance between finding a path when one exists (when the space isn't too
+  // large) and timing out quickly enough when the space is too large or there
+  // is no valid path to a trust anchor.
+  unsigned int buildForwardCallBudget = 200000;
   return BuildForward(trustDomain, cert, time, requiredKeyUsageIfPresent,
                       requiredEKUIfPresent, requiredPolicy, stapledOCSPResponse,
-                      0/*subCACount*/);
+                      0/*subCACount*/, buildForwardCallBudget);
 }
 
 } } // namespace mozilla::pkix
--- a/security/pkix/test/gtest/pkixbuild_tests.cpp
+++ b/security/pkix/test/gtest/pkixbuild_tests.cpp
@@ -44,17 +44,19 @@ using namespace mozilla::pkix;
 using namespace mozilla::pkix::test;
 
 static ByteString
 CreateCert(const char* issuerCN, // null means "empty name"
            const char* subjectCN, // null means "empty name"
            EndEntityOrCA endEntityOrCA,
            /*optional modified*/ std::map<ByteString, ByteString>*
              subjectDERToCertDER = nullptr,
-           /*optional*/ const ByteString* extension = nullptr)
+           /*optional*/ const ByteString* extension = nullptr,
+           /*optional*/ const TestKeyPair* issuerKeyPair = nullptr,
+           /*optional*/ const TestKeyPair* subjectKeyPair = nullptr)
 {
   static long serialNumberValue = 0;
   ++serialNumberValue;
   ByteString serialNumber(CreateEncodedSerialNumber(serialNumberValue));
   EXPECT_FALSE(ENCODING_FAILED(serialNumber));
 
   ByteString issuerDER(issuerCN ? CNToDERName(issuerCN) : Name(ByteString()));
   ByteString subjectDER(subjectCN ? CNToDERName(subjectCN) : Name(ByteString()));
@@ -70,17 +72,19 @@ CreateCert(const char* issuerCN, // null
     extensions.push_back(*extension);
   }
   extensions.push_back(ByteString()); // marks the end of the list
 
   ScopedTestKeyPair reusedKey(CloneReusedKeyPair());
   ByteString certDER(CreateEncodedCertificate(
                        v3, sha256WithRSAEncryption(), serialNumber, issuerDER,
                        oneDayBeforeNow, oneDayAfterNow, subjectDER,
-                       *reusedKey, extensions.data(), *reusedKey,
+                       subjectKeyPair ? *subjectKeyPair : *reusedKey,
+                       extensions.data(),
+                       issuerKeyPair ? *issuerKeyPair : *reusedKey,
                        sha256WithRSAEncryption()));
   EXPECT_FALSE(ENCODING_FAILED(certDER));
 
   if (subjectDERToCertDER) {
     (*subjectDERToCertDER)[subjectDER] = certDER;
   }
 
   return certDER;
@@ -740,8 +744,150 @@ TEST_F(pkixbuild, RevokedEndEntityWithMu
   ASSERT_EQ(Result::ERROR_REVOKED_CERTIFICATE,
             BuildCertChain(trustDomain, certDERInput, Now(),
                            EndEntityOrCA::MustBeEndEntity,
                            KeyUsage::noParticularKeyUsageRequired,
                            KeyPurposeId::id_kp_serverAuth,
                            CertPolicyId::anyPolicy,
                            nullptr/*stapledOCSPResponse*/));
 }
+
+// This represents a collection of different certificates that all have the same
+// subject and issuer distinguished name.
+class SelfIssuedCertificatesTrustDomain final : public DefaultCryptoTrustDomain
+{
+public:
+  void SetUpCerts(size_t totalCerts)
+  {
+    ASSERT_TRUE(totalCerts > 0);
+    // First we generate a trust anchor.
+    ScopedTestKeyPair rootKeyPair(GenerateKeyPair());
+    rootCACertDER = CreateCert("DN", "DN", EndEntityOrCA::MustBeCA, nullptr,
+                               nullptr, rootKeyPair.get(), rootKeyPair.get());
+    ASSERT_FALSE(ENCODING_FAILED(rootCACertDER));
+    certs.push_back(rootCACertDER);
+    ScopedTestKeyPair issuerKeyPair(rootKeyPair.release());
+    size_t subCAsGenerated;
+    // Then we generate 6 sub-CAs (given that we were requested to generate at
+    // least that many).
+    for (subCAsGenerated = 0;
+         subCAsGenerated < totalCerts - 1 && subCAsGenerated < 6;
+         subCAsGenerated++) {
+      // Each certificate has to have a unique SPKI (mozilla::pkix does loop
+      // detection and stops searching if it encounters two certificates in a
+      // path with the same subject and SPKI).
+      ScopedTestKeyPair keyPair(GenerateKeyPair());
+      ByteString cert(CreateCert("DN", "DN", EndEntityOrCA::MustBeCA, nullptr,
+                                 nullptr, issuerKeyPair.get(), keyPair.get()));
+      ASSERT_FALSE(ENCODING_FAILED(cert));
+      certs.push_back(cert);
+      issuerKeyPair.reset(keyPair.release());
+    }
+    // We set firstIssuerKey here because we can't end up with a path that has
+    // more than 7 CAs in it (because mozilla::pkix limits the path length).
+    firstIssuerKey.reset(issuerKeyPair.release());
+    // For any more sub CAs we generate, it doesn't matter what their keys are
+    // as long as they're different.
+    for (; subCAsGenerated < totalCerts - 1; subCAsGenerated++) {
+      ScopedTestKeyPair keyPair(GenerateKeyPair());
+      ByteString cert(CreateCert("DN", "DN", EndEntityOrCA::MustBeCA, nullptr,
+                                 nullptr, keyPair.get(), keyPair.get()));
+      ASSERT_FALSE(ENCODING_FAILED(cert));
+      certs.insert(certs.begin(), cert);
+    }
+  }
+
+  const TestKeyPair* GetFirstIssuerKey()
+  {
+    return firstIssuerKey.get();
+  }
+
+private:
+  Result GetCertTrust(EndEntityOrCA, const CertPolicyId&, Input candidateCert,
+                      /*out*/ TrustLevel& trustLevel) override
+  {
+    trustLevel = InputEqualsByteString(candidateCert, rootCACertDER)
+               ? TrustLevel::TrustAnchor
+               : TrustLevel::InheritsTrust;
+    return Success;
+  }
+
+  Result FindIssuer(Input, IssuerChecker& checker, Time) override
+  {
+    bool keepGoing;
+    for (auto& cert: certs) {
+      Input certInput;
+      Result rv = certInput.Init(cert.data(), cert.length());
+      if (rv != Success) {
+        return rv;
+      }
+      rv = checker.Check(certInput, nullptr, keepGoing);
+      if (rv != Success || !keepGoing) {
+        return rv;
+      }
+    }
+    return Success;
+  }
+
+  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Duration,
+                         /*optional*/ const Input*, /*optional*/ const Input*)
+                         override
+  {
+    return Success;
+  }
+
+  Result IsChainValid(const DERArray&, Time, const CertPolicyId&) override
+  {
+    return Success;
+  }
+
+  std::vector<ByteString> certs;
+  ByteString rootCACertDER;
+  ScopedTestKeyPair firstIssuerKey;
+};
+
+TEST_F(pkixbuild, AvoidUnboundedPathSearchingFailure)
+{
+  SelfIssuedCertificatesTrustDomain trustDomain;
+  // This creates a few hundred million potential paths of length 8 (end entity
+  // + 6 sub-CAs + root). It would be prohibitively expensive to enumerate all
+  // of these, so we give mozilla::pkix a budget that is spent when searching
+  // paths. If the budget is exhausted, it simply returns an unknown issuer
+  // error. In the future it might be nice to return a specific error that would
+  // give the front-end a hint that maybe it shouldn't have so many certificates
+  // that all have the same subject and issuer DN but different SPKIs.
+  trustDomain.SetUpCerts(18);
+  ByteString certDER(CreateCert("DN", "DN", EndEntityOrCA::MustBeEndEntity,
+                                nullptr, nullptr,
+                                trustDomain.GetFirstIssuerKey()));
+  ASSERT_FALSE(ENCODING_FAILED(certDER));
+  Input certDERInput;
+  ASSERT_EQ(Success, certDERInput.Init(certDER.data(), certDER.length()));
+  ASSERT_EQ(Result::ERROR_UNKNOWN_ISSUER,
+            BuildCertChain(trustDomain, certDERInput, Now(),
+                           EndEntityOrCA::MustBeEndEntity,
+                           KeyUsage::noParticularKeyUsageRequired,
+                           KeyPurposeId::id_kp_serverAuth,
+                           CertPolicyId::anyPolicy,
+                           nullptr/*stapledOCSPResponse*/));
+}
+
+TEST_F(pkixbuild, AvoidUnboundedPathSearchingSuccess)
+{
+  SelfIssuedCertificatesTrustDomain trustDomain;
+  // This creates a few hundred thousand possible potential paths of length 8
+  // (end entity + 6 sub-CAs + root). This will nearly exhaust mozilla::pkix's
+  // search budget, so this should succeed.
+  trustDomain.SetUpCerts(10);
+  ByteString certDER(CreateCert("DN", "DN", EndEntityOrCA::MustBeEndEntity,
+                                nullptr, nullptr,
+                                trustDomain.GetFirstIssuerKey()));
+  ASSERT_FALSE(ENCODING_FAILED(certDER));
+  Input certDERInput;
+  ASSERT_EQ(Success, certDERInput.Init(certDER.data(), certDER.length()));
+  ASSERT_EQ(Success,
+            BuildCertChain(trustDomain, certDERInput, Now(),
+                           EndEntityOrCA::MustBeEndEntity,
+                           KeyUsage::noParticularKeyUsageRequired,
+                           KeyPurposeId::id_kp_serverAuth,
+                           CertPolicyId::anyPolicy,
+                           nullptr/*stapledOCSPResponse*/));
+}