Allow the TrustStore interface to return matching intermediates, and identify distrusted certs.

net/cert/internal/verify_certificate_chain.cc

Index: net/cert/internal/verify_certificate_chain.cc
diff --git a/net/cert/internal/verify_certificate_chain.cc b/net/cert/internal/verify_certificate_chain.cc
index 9a1f6e008401812c794b9515943f2fcb8db40413..baff4206f796283696ff93d683c6373c1a0e8d41 100644
--- a/net/cert/internal/verify_certificate_chain.cc
+++ b/net/cert/internal/verify_certificate_chain.cc
@@ -37,6 +37,8 @@ DEFINE_CERT_ERROR_ID(
 DEFINE_CERT_ERROR_ID(kInvalidOrUnsupportedSignatureAlgorithm,
                      "Invalid or unsupported signature algorithm");
 DEFINE_CERT_ERROR_ID(kChainIsEmpty, "Chain is empty");
+DEFINE_CERT_ERROR_ID(kChainIsLength1,
+                     "TODO: Cannot verify a chain of length 1");
 DEFINE_CERT_ERROR_ID(kUnconsumedCriticalExtension,
                      "Unconsumed critical extension");
 DEFINE_CERT_ERROR_ID(
@@ -60,6 +62,9 @@ DEFINE_CERT_ERROR_ID(kEkuLacksServerAuth,
                      "The extended key usage does not include server auth");
 DEFINE_CERT_ERROR_ID(kEkuLacksClientAuth,
                      "The extended key usage does not include client auth");
+DEFINE_CERT_ERROR_ID(kCertIsDistrusted, "Certificate is distrusted");
+DEFINE_CERT_ERROR_ID(kCertIsNotTrustAnchor,
+                     "Certificate is not a trust anchor");
 
 bool IsHandledCriticalExtensionOid(const der::Input& oid) {
   if (oid == BasicConstraintsOid())
@@ -439,24 +444,16 @@ void WrapUp(const ParsedCertificate& cert, CertErrors* errors) {
   VerifyTargetCertHasConsistentCaBits(cert, errors);
 }
 
-// Initializes the path validation algorithm given anchor constraints. This
-// follows the description in RFC 5937
-void ProcessTrustAnchorConstraints(
-    const TrustAnchor& trust_anchor,
+// Enforces trust anchor constraints compatibile with RFC 5937.
+//
+// Note that the anchor constraints are encoded via the attached certificate
+// itself.
+void ApplyTrustAnchorConstraints(
+    const ParsedCertificate& cert,
     KeyPurpose required_key_purpose,
     size_t* max_path_length_ptr,
     std::vector<const NameConstraints*>* name_constraints_list,
     CertErrors* errors) {
-  // In RFC 5937 the enforcement of anchor constraints is governed by the input
-  // enforceTrustAnchorConstraints to path validation. In our implementation
-  // this is always on, and enforcement is controlled solely by whether or not
-  // the trust anchor specified constraints.
-  if (!trust_anchor.enforces_constraints())
-    return;
-
-  // Anchor constraints are encoded via the attached certificate.
-  const ParsedCertificate& cert = *trust_anchor.cert();
-
   // This is not part of RFC 5937 nor RFC 5280, but matches the EKU handling
   // done for intermediates (described in Web PKI's Baseline Requirements).
   VerifyExtendedKeyUsage(cert, required_key_purpose, errors);
@@ -498,16 +495,53 @@ void ProcessTrustAnchorConstraints(
   VerifyNoUnconsumedCriticalExtensions(cert, errors);
 }
 
+// Initializes the path validation algorithm given anchor constraints. This
+// follows the description in RFC 5937
+void ProcessRootCertificate(
+    const ParsedCertificate& cert,
+    const CertificateTrust& trust,
+    KeyPurpose required_key_purpose,
+    size_t* max_path_length_ptr,
+    std::vector<const NameConstraints*>* name_constraints_list,
+    der::Input* working_spki,
+    der::Input* working_normalized_issuer_name,
+    CertErrors* errors) {
+  switch (trust.type) {
+    case CertificateTrustType::UNSPECIFIED:
+      // Doesn't chain to a trust anchor - implicitly distrusted
+      errors->AddError(kCertIsNotTrustAnchor);
+      return;

[mattm, 2017/04/28 20:26:47]

working_spki and working_normalized_issuer_name aren't initialized in the
non-trusted cases even though it looks like VerifyCertificateChain continues to
try to evaluate the rest of the chain. Won't this lead to spurious
CertPathErrors?

[eroman, 2017/04/28 21:48:04]

Good point.

I don't currently have tests for those cases, and need to follow-up with tests
at least for the distrust case (unspecified is probably not reachable in
practice). I would like to simplify the test format before sticking in the
distrust stuff, since I think this will work better layering on top of the
changes in https://codereview.chromium.org/2805213004/  (which can be further
simplified now)

I will go ahead and initialize the subject and spki for all cases for now.

+    case CertificateTrustType::DISTRUSTED:
+      // Chains to an actively distrusted certificate.
+      errors->AddError(kCertIsDistrusted);
+      return;
+    case CertificateTrustType::TRUSTED_ANCHOR:
+    case CertificateTrustType::TRUSTED_ANCHOR_WITH_CONSTRAINTS:
+      // Chains to a trust anchor - use its SPKI and subject when verifying the
+      // next certificate.
+      *working_spki = cert.tbs().spki_tlv;
+      *working_normalized_issuer_name = cert.normalized_subject();
+
+      // If the trust anchor has constraints, enforce them.
+      if (trust.type == CertificateTrustType::TRUSTED_ANCHOR_WITH_CONSTRAINTS) {
+        ApplyTrustAnchorConstraints(cert, required_key_purpose,
+                                    max_path_length_ptr, name_constraints_list,
+                                    errors);
+      }
+      return;
+  }
+}
+
+}  // namespace
+
 // This implementation is structured to mimic the description of certificate
 // path verification given by RFC 5280 section 6.1.
-void VerifyCertificateChainNoReturnValue(
-    const ParsedCertificateList& certs,
-    const TrustAnchor* trust_anchor,
-    const SignaturePolicy* signature_policy,
-    const der::GeneralizedTime& time,
-    KeyPurpose required_key_purpose,
-    CertPathErrors* errors) {
-  DCHECK(trust_anchor);
+void VerifyCertificateChain(const ParsedCertificateList& certs,
+                            const CertificateTrust& last_cert_trust,
+                            const SignaturePolicy* signature_policy,
+                            const der::GeneralizedTime& time,
+                            KeyPurpose required_key_purpose,
+                            CertPathErrors* errors) {
   DCHECK(signature_policy);
   DCHECK(errors);
 
@@ -517,6 +551,13 @@ void VerifyCertificateChainNoReturnValue(
     return;
   }
 
+  // TODO(eroman): Verifying a trusted leaf certificate is not currently
+  // permitted.
+  if (certs.size() == 1) {
+    errors->GetOtherErrors()->AddError(kChainIsLength1);
+    return;
+  }
+
   // Will contain a NameConstraints for each previous cert in the chain which
   // had nameConstraints. This corresponds to the permitted_subtrees and
   // excluded_subtrees state variables from RFC 5280.
@@ -536,15 +577,14 @@ void VerifyCertificateChainNoReturnValue(
   //
   //    working_public_key:  the public key used to verify the
   //    signature of a certificate.
-  der::Input working_spki = trust_anchor->spki();
+  der::Input working_spki;
 
   // |working_normalized_issuer_name| is the normalized value of the
   // working_issuer_name variable in RFC 5280 section 6.1.2:
   //
   //    working_issuer_name:  the issuer distinguished name expected
   //    in the next certificate in the chain.
-  der::Input working_normalized_issuer_name =
-      trust_anchor->normalized_subject();
+  der::Input working_normalized_issuer_name;
 
   // |max_path_length| corresponds with the same named variable in RFC 5280
   // section 6.1.2:
@@ -556,23 +596,12 @@ void VerifyCertificateChainNoReturnValue(
   //    certificate.
   size_t max_path_length = certs.size();
 
-  // Apply any trust anchor constraints per RFC 5937.
-  //
-  // TODO(eroman): Errors on the trust anchor are put into a certificate bucket
-  //               GetErrorsForCert(certs.size()). This is a bit magical, and
-  //               has some integration issues.
-  ProcessTrustAnchorConstraints(*trust_anchor, required_key_purpose,
-                                &max_path_length, &name_constraints_list,
-                                errors->GetErrorsForCert(certs.size()));
-
   // Iterate over all the certificates in the reverse direction: starting from
-  // the certificate signed by trust anchor and progressing towards the target
-  // certificate.
+  // the root certificate and progressing towards the target certificate.
   //
-  // Note that |i| uses 0-based indexing whereas in RFC 5280 it is 1-based.
-  //
-  //   * i=0    :  Certificated signed by trust anchor.
-  //   * i=N-1  :  Target certificate.
+  //   * i=0               :  Root certificate (i.e. trust anchor)
+  //   * i=1               :  Certificated signed by the root certificate
+  //   * i=certs.size()-1  :  Target certificate.
   for (size_t i = 0; i < certs.size(); ++i) {
     const size_t index_into_certs = certs.size() - i - 1;
 
@@ -580,6 +609,7 @@ void VerifyCertificateChainNoReturnValue(
     // certificate being verified. The target certificate isn't necessarily an
     // end-entity certificate.
     const bool is_target_cert = index_into_certs == 0;
+    const bool is_root_cert = i == 0;
 
     const ParsedCertificate& cert = *certs[index_into_certs];
 
@@ -587,6 +617,16 @@ void VerifyCertificateChainNoReturnValue(
     // associated with that certificate.
     CertErrors* cert_errors = errors->GetErrorsForCert(index_into_certs);
 
+    if (is_root_cert) {
+      ProcessRootCertificate(cert, last_cert_trust, required_key_purpose,
+                             &max_path_length, &name_constraints_list,
+                             &working_spki, &working_normalized_issuer_name,
+                             cert_errors);
+
+      // Don't do any other checks for root certificates.
+      continue;
+    }
+
     // Per RFC 5280 section 6.1:
     //  * Do basic processing for each certificate
     //  * If it is the last certificate in the path (target certificate)
@@ -617,20 +657,4 @@ void VerifyCertificateChainNoReturnValue(
   //    certification path.
 }
 
-}  // namespace
-
-bool VerifyCertificateChain(const ParsedCertificateList& certs,
-                            const TrustAnchor* trust_anchor,
-                            const SignaturePolicy* signature_policy,
-                            const der::GeneralizedTime& time,
-                            KeyPurpose required_key_purpose,
-                            CertPathErrors* errors) {
-  // TODO(eroman): This function requires that |errors| is empty upon entry,
-  // which is not part of the API contract.
-  DCHECK(!errors->ContainsHighSeverityErrors());
-  VerifyCertificateChainNoReturnValue(certs, trust_anchor, signature_policy,
-                                      time, required_key_purpose, errors);
-  return !errors->ContainsHighSeverityErrors();
-}
-
 }  // namespace net