Bug 1033563, Part 3: Change mozilla::pkix::TrustDomain::FindPotentialIssuers API to be iterator-like, r=keeler
authorBrian Smith <brian@briansmith.org>
Wed, 02 Jul 2014 16:15:16 -0700
changeset 206535 44c19e8283c2b4e590b3ffdfbfbdef6f370056cc
parent 206534 072f999edcc4b2ada51787af863f253329079cb4
child 206536 3acf9162f52d566e5d446ddc7ce24dd5d390e365
push idunknown
push userunknown
push dateunknown
reviewerskeeler
bugs1033563
milestone33.0a1
Bug 1033563, Part 3: Change mozilla::pkix::TrustDomain::FindPotentialIssuers API to be iterator-like, r=keeler
security/apps/AppTrustDomain.cpp
security/apps/AppTrustDomain.h
security/certverifier/NSSCertDBTrustDomain.cpp
security/certverifier/NSSCertDBTrustDomain.h
security/pkix/include/pkix/pkixtypes.h
security/pkix/lib/pkixbuild.cpp
security/pkix/test/gtest/pkix_cert_chain_length_tests.cpp
security/pkix/test/gtest/pkix_cert_extension_tests.cpp
--- a/security/apps/AppTrustDomain.cpp
+++ b/security/apps/AppTrustDomain.cpp
@@ -81,28 +81,51 @@ AppTrustDomain::SetTrustedRoot(AppTruste
   if (!mTrustedRoot) {
     return SECFailure;
   }
 
   return SECSuccess;
 }
 
 SECStatus
-AppTrustDomain::FindPotentialIssuers(const SECItem* encodedIssuerName,
-                                     PRTime time,
-                             /*out*/ mozilla::pkix::ScopedCERTCertList& results)
+AppTrustDomain::FindIssuer(const SECItem& encodedIssuerName,
+                           IssuerChecker& checker, PRTime time)
+
 {
   MOZ_ASSERT(mTrustedRoot);
   if (!mTrustedRoot) {
     PR_SetError(PR_INVALID_STATE_ERROR, 0);
     return SECFailure;
   }
 
-  results = CERT_CreateSubjectCertList(nullptr, CERT_GetDefaultCertDB(),
-                                       encodedIssuerName, time, true);
+  // TODO(bug 1035418): If/when mozilla::pkix relaxes the restriction that
+  // FindIssuer must only pass certificates with a matching subject name to
+  // checker.Check, we can stop using CERT_CreateSubjectCertList and instead
+  // use logic like this:
+  //
+  // 1. First, try the trusted trust anchor.
+  // 2. Secondly, iterate through the certificates that were stored in the CMS
+  //    message, passing each one to checker.Check.
+  mozilla::pkix::ScopedCERTCertList
+    candidates(CERT_CreateSubjectCertList(nullptr, CERT_GetDefaultCertDB(),
+                                          &encodedIssuerName, time, true));
+  if (candidates) {
+    for (CERTCertListNode* n = CERT_LIST_HEAD(candidates);
+         !CERT_LIST_END(n, candidates); n = CERT_LIST_NEXT(n)) {
+      bool keepGoing;
+      SECStatus srv = checker.Check(n->cert->derCert, keepGoing);
+      if (srv != SECSuccess) {
+        return SECFailure;
+      }
+      if (!keepGoing) {
+        break;
+      }
+    }
+  }
+
   return SECSuccess;
 }
 
 SECStatus
 AppTrustDomain::GetCertTrust(EndEntityOrCA endEntityOrCA,
                              const CertPolicyId& policy,
                              const SECItem& candidateCertDER,
                      /*out*/ TrustLevel* trustLevel)
--- a/security/apps/AppTrustDomain.h
+++ b/security/apps/AppTrustDomain.h
@@ -19,20 +19,18 @@ public:
   AppTrustDomain(void* pinArg);
 
   SECStatus SetTrustedRoot(AppTrustedRoot trustedRoot);
 
   SECStatus GetCertTrust(mozilla::pkix::EndEntityOrCA endEntityOrCA,
                          const mozilla::pkix::CertPolicyId& policy,
                          const SECItem& candidateCertDER,
                  /*out*/ mozilla::pkix::TrustLevel* trustLevel) MOZ_OVERRIDE;
-  SECStatus FindPotentialIssuers(const SECItem* encodedIssuerName,
-                                 PRTime time,
-                         /*out*/ mozilla::pkix::ScopedCERTCertList& results)
-                                 MOZ_OVERRIDE;
+  SECStatus FindIssuer(const SECItem& encodedIssuerName,
+                       IssuerChecker& checker, PRTime time) MOZ_OVERRIDE;
   SECStatus VerifySignedData(const CERTSignedData& signedData,
                              const SECItem& subjectPublicKeyInfo) MOZ_OVERRIDE;
   SECStatus CheckRevocation(mozilla::pkix::EndEntityOrCA endEntityOrCA,
                             const mozilla::pkix::CertID& certID, PRTime time,
                             /*optional*/ const SECItem* stapledOCSPresponse,
                             /*optional*/ const SECItem* aiaExtension);
   SECStatus IsChainValid(const CERTCertList* certChain) { return SECSuccess; }
 
--- a/security/certverifier/NSSCertDBTrustDomain.cpp
+++ b/security/certverifier/NSSCertDBTrustDomain.cpp
@@ -51,25 +51,38 @@ NSSCertDBTrustDomain::NSSCertDBTrustDoma
   , mOCSPCache(ocspCache)
   , mPinArg(pinArg)
   , mOCSPGetConfig(ocspGETConfig)
   , mCheckChainCallback(checkChainCallback)
 {
 }
 
 SECStatus
-NSSCertDBTrustDomain::FindPotentialIssuers(
-  const SECItem* encodedIssuerName, PRTime time,
-  /*out*/ mozilla::pkix::ScopedCERTCertList& results)
+NSSCertDBTrustDomain::FindIssuer(const SECItem& encodedIssuerName,
+                                 IssuerChecker& checker, PRTime time)
 {
-  // TODO: normalize encodedIssuerName
   // TODO: NSS seems to be ambiguous between "no potential issuers found" and
   // "there was an error trying to retrieve the potential issuers."
-  results = CERT_CreateSubjectCertList(nullptr, CERT_GetDefaultCertDB(),
-                                       encodedIssuerName, time, true);
+  mozilla::pkix::ScopedCERTCertList
+    candidates(CERT_CreateSubjectCertList(nullptr, CERT_GetDefaultCertDB(),
+                                          &encodedIssuerName, time, true));
+  if (candidates) {
+    for (CERTCertListNode* n = CERT_LIST_HEAD(candidates);
+         !CERT_LIST_END(n, candidates); n = CERT_LIST_NEXT(n)) {
+      bool keepGoing;
+      SECStatus srv = checker.Check(n->cert->derCert, keepGoing);
+      if (srv != SECSuccess) {
+        return SECFailure;
+      }
+      if (!keepGoing) {
+        break;
+      }
+    }
+  }
+
   return SECSuccess;
 }
 
 SECStatus
 NSSCertDBTrustDomain::GetCertTrust(EndEntityOrCA endEntityOrCA,
                                    const CertPolicyId& policy,
                                    const SECItem& candidateCertDER,
                                    /*out*/ TrustLevel* trustLevel)
--- a/security/certverifier/NSSCertDBTrustDomain.h
+++ b/security/certverifier/NSSCertDBTrustDomain.h
@@ -48,20 +48,18 @@ public:
     FetchOCSPForEV = 3,
     LocalOnlyOCSPForEV = 4,
   };
   NSSCertDBTrustDomain(SECTrustType certDBTrustType, OCSPFetching ocspFetching,
                        OCSPCache& ocspCache, void* pinArg,
                        CertVerifier::ocsp_get_config ocspGETConfig,
                        CERTChainVerifyCallback* checkChainCallback = nullptr);
 
-  virtual SECStatus FindPotentialIssuers(
-                        const SECItem* encodedIssuerName,
-                        PRTime time,
-                /*out*/ mozilla::pkix::ScopedCERTCertList& results);
+  virtual SECStatus FindIssuer(const SECItem& encodedIssuerName,
+                               IssuerChecker& checker, PRTime time);
 
   virtual SECStatus GetCertTrust(mozilla::pkix::EndEntityOrCA endEntityOrCA,
                                  const mozilla::pkix::CertPolicyId& policy,
                                  const SECItem& candidateCertDER,
                          /*out*/ mozilla::pkix::TrustLevel* trustLevel);
 
   virtual SECStatus VerifySignedData(const CERTSignedData& signedData,
                                      const SECItem& subjectPublicKeyInfo);
--- a/security/pkix/include/pkix/pkixtypes.h
+++ b/security/pkix/include/pkix/pkixtypes.h
@@ -137,27 +137,72 @@ public:
   // list of EV roots, then GetCertTrust must result in
   // *trustLevel == InheritsTrust instead of *trustLevel == TrustAnchor
   // (assuming the candidate cert is not actively distrusted).
   virtual SECStatus GetCertTrust(EndEntityOrCA endEntityOrCA,
                                  const CertPolicyId& policy,
                                  const SECItem& candidateCertDER,
                          /*out*/ TrustLevel* trustLevel) = 0;
 
-  // Find all certificates (intermediate and/or root) in the certificate
-  // database that have a subject name matching |encodedIssuerName| at
-  // the given time. Certificates where the given time is not within the
-  // certificate's validity period may be excluded. On input, |results|
-  // will be null on input. If no potential issuers are found, then this
-  // function should return SECSuccess with results being either null or
-  // an empty list. Otherwise, this function should construct a
-  // CERTCertList and return it in |results|, transfering ownership.
-  virtual SECStatus FindPotentialIssuers(const SECItem* encodedIssuerName,
-                                         PRTime time,
-                                 /*out*/ ScopedCERTCertList& results) = 0;
+  class IssuerChecker
+  {
+  public:
+    virtual SECStatus Check(const SECItem& potentialIssuerDER,
+                            /*out*/ bool& keepGoing) = 0;
+  protected:
+    IssuerChecker();
+    virtual ~IssuerChecker();
+  private:
+    IssuerChecker(const IssuerChecker&) /*= delete*/;
+    void operator=(const IssuerChecker&) /*= delete*/;
+  };
+
+  // Search for a CA certificate with the given name. The implementation must
+  // call checker.Check with the DER encoding of the potential issuer
+  // certificate. The implementation must follow these rules:
+  //
+  // * The subject name of the certificate given to checker.Check must be equal
+  //   to encodedIssuerName.
+  // * The implementation must be reentrant and must limit the amount of stack
+  //   space it uses; see the note on reentrancy and stack usage below.
+  // * When checker.Check does not return SECSuccess then immediately return
+  //   SECFailure.
+  // * When checker.Check returns SECSuccess and sets keepGoing = false, then
+  //   immediately return SECSuccess.
+  // * When checker.Check returns SECSuccess and sets keepGoing = true, then
+  //   call checker.Check again with a different potential issuer certificate,
+  //   if any more are available.
+  // * When no more potential issuer certificates are available, return
+  //   SECSuccess.
+  // * Don't call checker.Check with the same potential issuer certificate more
+  //   than once in a given call of FindIssuer.
+  // * The given time parameter may be used to filter out certificates that are
+  //   not valid at the given time, or it may be ignored.
+  //
+  // Note on reentrancy and stack usage: checker.Check will attempt to
+  // recursively build a certificate path from the potential issuer it is given
+  // to a trusted root, as determined by this TrustDomain. That means that
+  // checker.Check may call any/all of the methods on this TrustDomain. In
+  // particular, there will be call stacks that look like this:
+  //
+  //    BuildCertChain
+  //      [...]
+  //        TrustDomain::FindIssuer
+  //          [...]
+  //            IssuerChecker::Check
+  //              [...]
+  //                TrustDomain::FindIssuer
+  //                  [...]
+  //                    IssuerChecker::Check
+  //                      [...]
+  //
+  // checker.Check is responsible for limiting the recursion to a reasonable
+  // limit.
+  virtual SECStatus FindIssuer(const SECItem& encodedIssuerName,
+                               IssuerChecker& checker, PRTime time) = 0;
 
   // Verify the given signature using the given public key.
   //
   // Most implementations of this function should probably forward the call
   // directly to mozilla::pkix::VerifySignedData.
   virtual SECStatus VerifySignedData(const CERTSignedData& signedData,
                                      const SECItem& subjectPublicKeyInfo) = 0;
 
--- a/security/pkix/lib/pkixbuild.cpp
+++ b/security/pkix/lib/pkixbuild.cpp
@@ -36,17 +36,22 @@ static Result BuildForward(TrustDomain& 
                            EndEntityOrCA endEntityOrCA,
                            KeyUsage requiredKeyUsageIfPresent,
                            KeyPurposeId requiredEKUIfPresent,
                            const CertPolicyId& requiredPolicy,
                            /*optional*/ const SECItem* stapledOCSPResponse,
                            unsigned int subCACount,
                            /*out*/ ScopedCERTCertList& results);
 
-class PathBuildingStep
+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 : public TrustDomain::IssuerChecker
 {
 public:
   PathBuildingStep(TrustDomain& trustDomain, const BackCert& subject,
                    PRTime time, EndEntityOrCA endEntityOrCA,
                    KeyPurposeId requiredEKUIfPresent,
                    const CertPolicyId& requiredPolicy,
                    /*optional*/ const SECItem* stapledOCSPResponse,
                    unsigned int subCACount,
@@ -60,17 +65,17 @@ public:
     , stapledOCSPResponse(stapledOCSPResponse)
     , subCACount(subCACount)
     , results(results)
     , result(SEC_ERROR_LIBRARY_FAILURE)
     , resultWasSet(false)
   {
   }
 
-  SECStatus Build(const SECItem& potentialIssuerDER,
+  SECStatus Check(const SECItem& potentialIssuerDER,
                   /*out*/ bool& keepGoing);
 
   Result CheckResult() const;
 
 private:
   TrustDomain& trustDomain;
   const BackCert& subject;
   const PRTime time;
@@ -127,17 +132,17 @@ PathBuildingStep::CheckResult() const
     return Success;
   }
   PR_SetError(result, 0);
   return MapSECStatus(SECFailure);
 }
 
 // The code that executes in the inner loop of BuildForward
 SECStatus
-PathBuildingStep::Build(const SECItem& potentialIssuerDER,
+PathBuildingStep::Check(const SECItem& potentialIssuerDER,
                         /*out*/ bool& keepGoing)
 {
   BackCert potentialIssuer(potentialIssuerDER, &subject,
                            BackCert::IncludeCN::No);
   Result rv = potentialIssuer.Init();
   if (rv != Success) {
     return RecordResult(PR_GetError(), keepGoing);
   }
@@ -186,17 +191,17 @@ PathBuildingStep::Build(const SECItem& p
                 subject.GetSerialNumber());
   srv = trustDomain.CheckRevocation(endEntityOrCA, certID, time,
                                     stapledOCSPResponse,
                                     subject.GetAuthorityInfoAccess());
   if (srv != SECSuccess) {
     return RecordResult(PR_GetError(), keepGoing);
   }
 
-  return RecordResult(0, keepGoing);
+  return RecordResult(0/*PRErrorCode::success*/, keepGoing);
 }
 
 // Recursively build the path from the given subject certificate to the root.
 //
 // Be very careful about changing the order of checks. The order is significant
 // because it affects which error we return when a certificate or certificate
 // chain has multiple problems. See the error ranking documentation in
 // pkix/pkix.h.
@@ -276,35 +281,20 @@ BuildForward(TrustDomain& trustDomain,
 
   // Find a trusted issuer.
 
   PathBuildingStep pathBuilder(trustDomain, subject, time, endEntityOrCA,
                                requiredEKUIfPresent, requiredPolicy,
                                stapledOCSPResponse, subCACount, results);
 
   // TODO(bug 965136): Add SKI/AKI matching optimizations
-  ScopedCERTCertList candidates;
-  if (trustDomain.FindPotentialIssuers(&subject.GetIssuer(), time,
-                                       candidates) != SECSuccess) {
+  if (trustDomain.FindIssuer(subject.GetIssuer(), pathBuilder, time)
+        != SECSuccess) {
     return MapSECStatus(SECFailure);
   }
-  if (!candidates) {
-    return Fail(RecoverableError, SEC_ERROR_UNKNOWN_ISSUER);
-  }
-  for (CERTCertListNode* n = CERT_LIST_HEAD(candidates);
-       !CERT_LIST_END(n, candidates); n = CERT_LIST_NEXT(n)) {
-    bool keepGoing;
-    SECStatus srv = pathBuilder.Build(n->cert->derCert, keepGoing);
-    if (srv != SECSuccess) {
-      return MapSECStatus(SECFailure);
-    }
-    if (!keepGoing) {
-      break;
-    }
-  }
 
   rv = pathBuilder.CheckResult();
   if (rv != Success) {
     return rv;
   }
 
   // We've built a valid chain from the subject cert up to a trusted root.
   // At this point, we know the results contains the complete cert chain.
--- a/security/pkix/test/gtest/pkix_cert_chain_length_tests.cpp
+++ b/security/pkix/test/gtest/pkix_cert_chain_length_tests.cpp
@@ -121,22 +121,36 @@ private:
     if (SECITEM_ItemsAreEqual(&candidateCert, &certChainTail[0]->derCert)) {
       *trustLevel = TrustLevel::TrustAnchor;
     } else {
       *trustLevel = TrustLevel::InheritsTrust;
     }
     return SECSuccess;
   }
 
-  SECStatus FindPotentialIssuers(const SECItem* encodedIssuerName,
-                                 PRTime time,
-                                 /*out*/ ScopedCERTCertList& results)
+  SECStatus FindIssuer(const SECItem& encodedIssuerName,
+                       IssuerChecker& checker, PRTime time)
   {
-    results = CERT_CreateSubjectCertList(nullptr, CERT_GetDefaultCertDB(),
-                                         encodedIssuerName, time, true);
+    mozilla::pkix::ScopedCERTCertList
+      candidates(CERT_CreateSubjectCertList(nullptr, CERT_GetDefaultCertDB(),
+                                            &encodedIssuerName, time, true));
+    if (candidates) {
+      for (CERTCertListNode* n = CERT_LIST_HEAD(candidates);
+           !CERT_LIST_END(n, candidates); n = CERT_LIST_NEXT(n)) {
+        bool keepGoing;
+        SECStatus srv = checker.Check(n->cert->derCert, keepGoing);
+        if (srv != SECSuccess) {
+          return SECFailure;
+        }
+        if (!keepGoing) {
+          break;
+        }
+      }
+    }
+
     return SECSuccess;
   }
 
   SECStatus VerifySignedData(const CERTSignedData& signedData,
                              const SECItem& subjectPublicKeyInfo)
   {
     return ::mozilla::pkix::VerifySignedData(signedData, subjectPublicKeyInfo,
                                              nullptr);
--- a/security/pkix/test/gtest/pkix_cert_extension_tests.cpp
+++ b/security/pkix/test/gtest/pkix_cert_extension_tests.cpp
@@ -76,21 +76,22 @@ private:
                          const CertPolicyId&,
                          const SECItem& candidateCert,
                          /*out*/ TrustLevel* trustLevel)
   {
     *trustLevel = TrustLevel::TrustAnchor;
     return SECSuccess;
   }
 
-  SECStatus FindPotentialIssuers(const SECItem* encodedIssuerName,
-                                 PRTime time,
-                                 /*out*/ ScopedCERTCertList& results)
+  SECStatus FindIssuer(const SECItem& /*encodedIssuerName*/,
+                       IssuerChecker& /*checker*/, PRTime /*time*/)
   {
-    return SECSuccess;
+    PR_NOT_REACHED("FindIssuer should not be called");
+    PR_SetError(SEC_ERROR_LIBRARY_FAILURE, 0);
+    return SECFailure;
   }
 
   SECStatus VerifySignedData(const CERTSignedData& signedData,
                              const SECItem& subjectPublicKeyInfo)
   {
     return ::mozilla::pkix::VerifySignedData(signedData, subjectPublicKeyInfo,
                                              nullptr);
   }