Improvements to the net/cert/internal error handling.

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 617b8511f17deb57995c5d63e35ca4d4639ef190..50ce17f70d827eb6df20d9ad6839c3d5ef57bb02 100644
--- a/net/cert/internal/verify_certificate_chain.cc
+++ b/net/cert/internal/verify_certificate_chain.cc
@@ -9,7 +9,6 @@
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "net/cert/internal/cert_error_params.h"
-#include "net/cert/internal/cert_error_scoper.h"
 #include "net/cert/internal/cert_errors.h"
 #include "net/cert/internal/name_constraints.h"
 #include "net/cert/internal/parse_certificate.h"
@@ -57,45 +56,17 @@ DEFINE_CERT_ERROR_ID(kSignatureAlgorithmsDifferentEncoding,
                      "Certificate.signatureAlgorithm is encoded differently "
                      "than TBSCertificate.signature");
 
-DEFINE_CERT_ERROR_ID(kContextTrustAnchor, "Processing Trust Anchor");
-DEFINE_CERT_ERROR_ID(kContextCertificate, "Processing Certificate");
-
-// This class changes the error scope to indicate which certificate in the
-// chain is currently being processed.
-class CertErrorScoperForCert : public CertErrorScoper {
- public:
-  CertErrorScoperForCert(CertErrors* parent_errors, size_t index)
-      : CertErrorScoper(parent_errors), index_(index) {}
-
-  std::unique_ptr<CertErrorNode> BuildRootNode() override {
-    return base::MakeUnique<CertErrorNode>(
-        CertErrorNodeType::TYPE_CONTEXT, kContextCertificate,
-        CreateCertErrorParams1SizeT("index", index_));
-  }
-
- private:
-  size_t index_;
-
-  DISALLOW_COPY_AND_ASSIGN(CertErrorScoperForCert);
-};
-
-// Returns true if the certificate does not contain any unconsumed _critical_
+// Adds errors to |errors| if the certificate contains unconsumed _critical_
 // extensions.
-WARN_UNUSED_RESULT bool VerifyNoUnconsumedCriticalExtensions(
-    const ParsedCertificate& cert,
-    CertErrors* errors) {
-  bool has_unconsumed_critical_extensions = false;
-
+void VerifyNoUnconsumedCriticalExtensions(const ParsedCertificate& cert,
+                                          CertErrors* errors) {
   for (const auto& entry : cert.unparsed_extensions()) {
     if (entry.second.critical) {
-      has_unconsumed_critical_extensions = true;
       errors->AddError(kUnconsumedCriticalExtension,
                        CreateCertErrorParams2Der("oid", entry.second.oid,
                                                  "value", entry.second.value));
     }
   }
-
-  return !has_unconsumed_critical_extensions;
 }
 
 // Returns true if |cert| was self-issued. The definition of self-issuance
@@ -113,30 +84,25 @@ WARN_UNUSED_RESULT bool IsSelfIssued(const ParsedCertificate& cert) {
   return cert.normalized_subject() == cert.normalized_issuer();
 }
 
-// Returns true if |cert| is valid at time |time|.
+// Adds errors to |errors| if |cert| is not valid at time |time|.
 //
 // The certificate's validity requirements are described by RFC 5280 section
 // 4.1.2.5:
 //
 //    The validity period for a certificate is the period of time from
 //    notBefore through notAfter, inclusive.
-WARN_UNUSED_RESULT bool VerifyTimeValidity(const ParsedCertificate& cert,
-                                           const der::GeneralizedTime time,
-                                           CertErrors* errors) {
-  if (time < cert.tbs().validity_not_before) {
+void VerifyTimeValidity(const ParsedCertificate& cert,
+                        const der::GeneralizedTime time,
+                        CertErrors* errors) {
+  if (time < cert.tbs().validity_not_before)
     errors->AddError(kValidityFailedNotBefore);
-    return false;
-  }
 
-  if (cert.tbs().validity_not_after < time) {
+  if (cert.tbs().validity_not_after < time)
     errors->AddError(kValidityFailedNotAfter);
-    return false;
-  }
-
-  return true;
 }
 
-// Returns true if |cert| has internally consistent signature algorithms.
+// Adds errors to |errors| if |cert| has internally inconsistent signature
+// algorithms.
 //
 // X.509 certificates contain two different signature algorithms:
 //  (1) The signatureAlgorithm field of Certificate
@@ -155,16 +121,15 @@ WARN_UNUSED_RESULT bool VerifyTimeValidity(const ParsedCertificate& cert,
 // In practice however there are certificates which use different encodings for
 // specifying RSA with SHA1 (different OIDs). This is special-cased for
 // compatibility sake.
-WARN_UNUSED_RESULT bool VerifySignatureAlgorithmsMatch(
-    const ParsedCertificate& cert,
-    CertErrors* errors) {
+void VerifySignatureAlgorithmsMatch(const ParsedCertificate& cert,
+                                    CertErrors* errors) {
   const der::Input& alg1_tlv = cert.signature_algorithm_tlv();
   const der::Input& alg2_tlv = cert.tbs().signature_algorithm_tlv;
 
   // Ensure that the two DER-encoded signature algorithms are byte-for-byte
   // equal.
   if (alg1_tlv == alg2_tlv)
-    return true;
+    return;
 
   // But make a compatibility concession if alternate encodings are used
   // TODO(eroman): Turn this warning into an error.
@@ -174,20 +139,18 @@ WARN_UNUSED_RESULT bool VerifySignatureAlgorithmsMatch(
         kSignatureAlgorithmsDifferentEncoding,
         CreateCertErrorParams2Der("Certificate.algorithm", alg1_tlv,
                                   "TBSCertificate.signature", alg2_tlv));
-    return true;
+    return;
   }
 
   errors->AddError(
       kSignatureAlgorithmMismatch,
       CreateCertErrorParams2Der("Certificate.algorithm", alg1_tlv,
                                 "TBSCertificate.signature", alg2_tlv));
-
-  return false;
 }
 
 // This function corresponds to RFC 5280 section 6.1.3's "Basic Certificate
 // Processing" procedure.
-WARN_UNUSED_RESULT bool BasicCertificateProcessing(
+void BasicCertificateProcessing(
     const ParsedCertificate& cert,
     bool is_target_cert,
     const SignaturePolicy* signature_policy,
@@ -199,8 +162,7 @@ WARN_UNUSED_RESULT bool BasicCertificateProcessing(
   // Check that the signature algorithms in Certificate vs TBSCertificate
   // match. This isn't part of RFC 5280 section 6.1.3, but is mandated by
   // sections 4.1.1.2 and 4.1.2.3.
-  if (!VerifySignatureAlgorithmsMatch(cert, errors))
-    return false;
+  VerifySignatureAlgorithmsMatch(cert, errors);
 
   // Verify the digital signature using the previous certificate's key (RFC
   // 5280 section 6.1.3 step a.1).
@@ -208,21 +170,18 @@ WARN_UNUSED_RESULT bool BasicCertificateProcessing(
     errors->AddError(
         kInvalidOrUnsupportedSignatureAlgorithm,
         CreateCertErrorParams1Der("algorithm", cert.signature_algorithm_tlv()));
-    return false;
-  }
-
-  if (!VerifySignedData(cert.signature_algorithm(), cert.tbs_certificate_tlv(),
-                        cert.signature_value(), working_spki, signature_policy,
-                        errors)) {
-    errors->AddError(kVerifySignedDataFailed);
-    return false;
+  } else {
+    if (!VerifySignedData(cert.signature_algorithm(),
+                          cert.tbs_certificate_tlv(), cert.signature_value(),
+                          working_spki, signature_policy, errors)) {
+      errors->AddError(kVerifySignedDataFailed);
+    }
   }
 
   // Check the time range for the certificate's validity, ensuring it is valid
   // at |time|.
   // (RFC 5280 section 6.1.3 step a.2)
-  if (!VerifyTimeValidity(cert, time, errors))
-    return false;
+  VerifyTimeValidity(cert, time, errors);
 
   // TODO(eroman): Check revocation (RFC 5280 section 6.1.3 step a.3)
 
@@ -230,7 +189,6 @@ WARN_UNUSED_RESULT bool BasicCertificateProcessing(
   // subject name. (RFC 5280 section 6.1.3 step a.4)
   if (cert.normalized_issuer() != working_normalized_issuer_name) {

[mattm, 2017/03/23 00:32:05]

{} no longer needed here

[eroman, 2017/03/23 01:32:12]

Done.

     errors->AddError(kSubjectDoesNotMatchIssuer);
-    return false;
   }
 
   // Name constraints (RFC 5280 section 6.1.3 step b & c)
@@ -242,20 +200,17 @@ WARN_UNUSED_RESULT bool BasicCertificateProcessing(
       if (!nc->IsPermittedCert(cert.normalized_subject(),
                                cert.subject_alt_names())) {
         errors->AddError(kNotPermittedByNameConstraints);
-        return false;
       }
     }
   }
 
   // TODO(eroman): Steps d-f are omitted, as policy constraints are not yet
   // implemented.
-
-  return true;
 }
 
 // This function corresponds to RFC 5280 section 6.1.4's "Preparation for
 // Certificate i+1" procedure. |cert| is expected to be an intermediate.
-WARN_UNUSED_RESULT bool PrepareForNextCertificate(
+void PrepareForNextCertificate(
     const ParsedCertificate& cert,
     size_t* max_path_length_ptr,
     der::Input* working_spki,
@@ -301,12 +256,8 @@ WARN_UNUSED_RESULT bool PrepareForNextCertificate(
   // can't contain a BasicConstraints extension.
   if (!cert.has_basic_constraints()) {
     errors->AddError(kMissingBasicConstraints);
-    return false;
-  }
-
-  if (!cert.basic_constraints().is_ca) {
+  } else if (!cert.basic_constraints().is_ca) {
     errors->AddError(kBasicConstraintsIndicatesNotCa);
-    return false;
   }
 
   // From RFC 5280 section 6.1.4 step l:
@@ -317,7 +268,6 @@ WARN_UNUSED_RESULT bool PrepareForNextCertificate(
   if (!IsSelfIssued(cert)) {
     if (*max_path_length_ptr == 0) {
       errors->AddError(kMaxPathLengthViolated);
-      return false;
     }
     --(*max_path_length_ptr);

[mattm, 2017/03/23 00:32:05]

underflow if max_path_length is already 0?

[eroman, 2017/03/23 01:32:12]

Good spot, thanks!

(I was afraid of issues like that from removing short circuiting, but didn't
catch this one. Interestingly there is a test for this, however the underflow
doesn't cause it to fail, as it wraps around to something big).

   }
@@ -327,7 +277,7 @@ WARN_UNUSED_RESULT bool PrepareForNextCertificate(
   //    If pathLenConstraint is present in the certificate and is
   //    less than max_path_length, set max_path_length to the value
   //    of pathLenConstraint.
-  if (cert.basic_constraints().has_path_len &&
+  if (cert.has_basic_constraints() && cert.basic_constraints().has_path_len &&
       cert.basic_constraints().path_len < *max_path_length_ptr) {
     *max_path_length_ptr = cert.basic_constraints().path_len;
   }
@@ -339,7 +289,6 @@ WARN_UNUSED_RESULT bool PrepareForNextCertificate(
   if (cert.has_key_usage() &&
       !cert.key_usage().AssertsBit(KEY_USAGE_BIT_KEY_CERT_SIGN)) {
     errors->AddError(kKeyCertSignBitNotSet);
-    return false;
   }
 
   // From RFC 5280 section 6.1.4 step o:
@@ -348,15 +297,12 @@ WARN_UNUSED_RESULT bool PrepareForNextCertificate(
   //    the certificate.  Process any other recognized non-critical
   //    extension present in the certificate that is relevant to path
   //    processing.
-  if (!VerifyNoUnconsumedCriticalExtensions(cert, errors))
-    return false;
-
-  return true;
+  VerifyNoUnconsumedCriticalExtensions(cert, errors);
 }
 
 // Checks that if the target certificate has properties that only a CA should
 // have (keyCertSign, CA=true, pathLenConstraint), then its other properties
-// are consistent with being a CA.
+// are consistent with being a CA. If it does, adds errors to |errors|.
 //
 // This follows from some requirements in RFC 5280 section 4.2.1.9. In
 // particular:
@@ -376,9 +322,8 @@ WARN_UNUSED_RESULT bool PrepareForNextCertificate(
 // TODO(eroman): I don't believe Firefox enforces the keyCertSign restriction
 // for compatibility reasons. Investigate if we need to similarly relax this
 // constraint.
-WARN_UNUSED_RESULT bool VerifyTargetCertHasConsistentCaBits(
-    const ParsedCertificate& cert,
-    CertErrors* errors) {
+void VerifyTargetCertHasConsistentCaBits(const ParsedCertificate& cert,
+                                         CertErrors* errors) {
   // Check if the certificate contains any property specific to CAs.
   bool has_ca_property =
       (cert.has_basic_constraints() &&
@@ -398,17 +343,12 @@ WARN_UNUSED_RESULT bool VerifyTargetCertHasConsistentCaBits(
       // TODO(eroman): Add DER for basic constraints and key usage.
       errors->AddError(kTargetCertInconsistentCaBits);
     }
-
-    return success;
   }
-
-  return true;
 }
 
 // This function corresponds with RFC 5280 section 6.1.5's "Wrap-Up Procedure".
 // It does processing for the final certificate (the target cert).
-WARN_UNUSED_RESULT bool WrapUp(const ParsedCertificate& cert,
-                               CertErrors* errors) {
+void WrapUp(const ParsedCertificate& cert, CertErrors* errors) {
   // TODO(crbug.com/634452): Steps a-b are omitted as policy constraints are not
   // yet implemented.
 
@@ -424,36 +364,29 @@ WARN_UNUSED_RESULT bool WrapUp(const ParsedCertificate& cert,
   //
   // Note that this is duplicated by PrepareForNextCertificate() so as to
   // directly match the procedures in RFC 5280's section 6.1.
-  if (!VerifyNoUnconsumedCriticalExtensions(cert, errors))
-    return false;
+  VerifyNoUnconsumedCriticalExtensions(cert, errors);
 
   // TODO(eroman): Step g is omitted, as policy constraints are not yet
   // implemented.
 
   // The following check is NOT part of RFC 5280 6.1.5's "Wrap-Up Procedure",
   // however is implied by RFC 5280 section 4.2.1.9.
-  if (!VerifyTargetCertHasConsistentCaBits(cert, errors))
-    return false;
-
-  return true;
+  VerifyTargetCertHasConsistentCaBits(cert, errors);
 }
 
 // Initializes the path validation algorithm given anchor constraints. This
 // follows the description in RFC 5937
-WARN_UNUSED_RESULT bool ProcessTrustAnchorConstraints(
+void ProcessTrustAnchorConstraints(
     const TrustAnchor& trust_anchor,
     size_t* max_path_length_ptr,
     std::vector<const NameConstraints*>* name_constraints_list,
     CertErrors* errors) {
-  // Set the trust anchor as the current context for any subsequent errors.
-  CertErrorScoperNoParams error_context(errors, kContextTrustAnchor);
-
   // 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 true;
+    return;
 
   // Anchor constraints are encoded via the attached certificate.
   const ParsedCertificate& cert = *trust_anchor.cert();
@@ -492,29 +425,25 @@ WARN_UNUSED_RESULT bool ProcessTrustAnchorConstraints(
   //    Extensions may be marked critical or not critical.  When trust anchor
   //    constraints are enforced, clients MUST reject certification paths
   //    containing a trust anchor with unrecognized critical extensions.
-  if (!VerifyNoUnconsumedCriticalExtensions(cert, errors))
-    return false;
-
-  return true;
+  VerifyNoUnconsumedCriticalExtensions(cert, errors);
 }
 
-}  // namespace
-
 // This implementation is structured to mimic the description of certificate
 // path verification given by RFC 5280 section 6.1.
-bool VerifyCertificateChain(const ParsedCertificateList& certs,
-                            const TrustAnchor* trust_anchor,
-                            const SignaturePolicy* signature_policy,
-                            const der::GeneralizedTime& time,
-                            CertErrors* errors) {
+void VerifyCertificateChainNoReturnValue(
+    const ParsedCertificateList& certs,
+    const TrustAnchor* trust_anchor,
+    const SignaturePolicy* signature_policy,
+    const der::GeneralizedTime& time,
+    CertPathErrors* errors) {
   DCHECK(trust_anchor);
   DCHECK(signature_policy);
   DCHECK(errors);
 
   // An empty chain is necessarily invalid.
   if (certs.empty()) {
-    errors->AddError(kChainIsEmpty);
-    return false;
+    errors->GetOtherErrors()->AddError(kChainIsEmpty);
+    return;
   }
 
   // Will contain a NameConstraints for each previous cert in the chain which
@@ -557,10 +486,12 @@ bool VerifyCertificateChain(const ParsedCertificateList& certs,
   size_t max_path_length = certs.size();
 
   // Apply any trust anchor constraints per RFC 5937.
-  if (!ProcessTrustAnchorConstraints(*trust_anchor, &max_path_length,
-                                     &name_constraints_list, errors)) {
-    return false;
-  }
+  ProcessTrustAnchorConstraints(*trust_anchor, &max_path_length,
+                                &name_constraints_list,
+                                // Model the trust anchor as certificate
+                                // i=certs.size() for the error context.
+                                // TODO(eroman): Improve this.
+                                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
@@ -580,28 +511,24 @@ bool VerifyCertificateChain(const ParsedCertificateList& certs,
 
     const ParsedCertificate& cert = *certs[index_into_certs];
 
-    // Set the current certificate as the context for any subsequent errors.
-    CertErrorScoperForCert error_context(errors, i);
+    // Output errors for the currently certificate into an error bucket that is

[mattm, 2017/03/23 00:32:05]

missing word or typo in "currently certificate"

[eroman, 2017/03/23 01:32:12]

Done.

+    // associated with that certificate.
+    CertErrors* cert_errors = errors->GetErrorsForCert(index_into_certs);
 
     // Per RFC 5280 section 6.1:
     //  * Do basic processing for each certificate
     //  * If it is the last certificate in the path (target certificate)
     //     - Then run "Wrap up"
     //     - Otherwise run "Prepare for Next cert"
-    if (!BasicCertificateProcessing(
-            cert, is_target_cert, signature_policy, time, working_spki,
-            working_normalized_issuer_name, name_constraints_list, errors)) {
-      return false;
-    }
+    BasicCertificateProcessing(cert, is_target_cert, signature_policy, time,
+                               working_spki, working_normalized_issuer_name,
+                               name_constraints_list, cert_errors);
     if (!is_target_cert) {
-      if (!PrepareForNextCertificate(cert, &max_path_length, &working_spki,
-                                     &working_normalized_issuer_name,
-                                     &name_constraints_list, errors)) {
-        return false;
-      }
+      PrepareForNextCertificate(cert, &max_path_length, &working_spki,
+                                &working_normalized_issuer_name,
+                                &name_constraints_list, cert_errors);
     } else {
-      if (!WrapUp(cert, errors))
-        return false;
+      WrapUp(cert, cert_errors);
     }
   }
 
@@ -609,8 +536,21 @@ bool VerifyCertificateChain(const ParsedCertificateList& certs,
   //
   //    A certificate MUST NOT appear more than once in a prospective
   //    certification path.
+}
 
-  return true;
+}  // namespace
+
+bool VerifyCertificateChain(const ParsedCertificateList& certs,
+                            const TrustAnchor* trust_anchor,
+                            const SignaturePolicy* signature_policy,
+                            const der::GeneralizedTime& time,
+                            CertPathErrors* errors) {
+  // TODO(eroman): This function requires that |errors| is empty upon entry,
+  // which is not part of the API contract.
+  DCHECK(!errors->HasErrors());
+  VerifyCertificateChainNoReturnValue(certs, trust_anchor, signature_policy,
+                                      time, errors);
+  return !errors->HasErrors();
 }
 
 }  // namespace net