Improvements to the net/cert/internal error handling.

net/cert/cert_verify_proc_builtin.cc

 // Copyright (c) 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/cert/cert_verify_proc_builtin.h"
 
 #include <string>
 #include <vector>
 
 #if defined(USE_NSS_CERTS)
@@ skipping 232 matching lines @@
 // anchor) in |partial_path| to |*hashes|.
 void AppendPublicKeyHashes(const CertPathBuilder::ResultPath& partial_path,
                            HashValueVector* hashes) {
   for (const scoped_refptr<ParsedCertificate>& cert : partial_path.path.certs)
     AppendPublicKeyHashes(cert->tbs().spki_tlv, hashes);
 
   if (partial_path.path.trust_anchor)
     AppendPublicKeyHashes(partial_path.path.trust_anchor->spki(), hashes);
 }
 
-// Sets the bits on |cert_status| for all the errors encountered during the path
-// building of |partial_path|.
-void MapPathBuilderErrorsToCertStatus(
-    const CertPathBuilder::ResultPath& partial_path,
-    const std::string& hostname,
-    CertStatus* cert_status) {
-  // If path building was successful, there are no errors to map (there may have
-  // been warnings but they do not map to CertStatus).
-  if (partial_path.valid)
+// Sets the bits on |cert_status| for all the errors present in |errors| (the
+// errors for a particular path).
+void MapPathBuilderErrorsToCertStatus(const CertPathErrors& errors,
+                                      CertStatus* cert_status) {
+  // If there were no errors, nothing to do.
+  if (!errors.ContainsHighSeverityErrors())
     return;
 
-  LOG(ERROR) << "CertVerifyProcBuiltin for " << hostname << " failed:\n"
-             << partial_path.errors.ToDebugString();
-
-  if (partial_path.errors.ContainsError(kRsaModulusTooSmall))
+  if (errors.ContainsError(kRsaModulusTooSmall))
     *cert_status |= CERT_STATUS_WEAK_KEY;
 
-  if (partial_path.errors.ContainsError(kValidityFailedNotAfter) ||
-      partial_path.errors.ContainsError(kValidityFailedNotBefore)) {
+  if (errors.ContainsError(kValidityFailedNotAfter) ||
+      errors.ContainsError(kValidityFailedNotBefore)) {
     *cert_status |= CERT_STATUS_DATE_INVALID;
   }
 
   // IMPORTANT: If the path was invalid for a reason that was not
   // explicity checked above, set a general error. This is important as
   // |cert_status| is what ultimately indicates whether verification was
   // successful or not (absense of errors implies success).
   if (!IsCertStatusError(*cert_status))
     *cert_status |= CERT_STATUS_INVALID;
 }
@@ skipping 39 matching lines @@
 //
 // Any failure short-circuits from the function must set
 // |verify_result->cert_status|.
 void DoVerify(X509Certificate* input_cert,
               const std::string& hostname,
               const std::string& ocsp_response,
               int flags,
               CRLSet* crl_set,
               const CertificateList& additional_trust_anchors,
               CertVerifyResult* verify_result) {
-  CertErrors errors;
+  CertErrors parsing_errors;
 
   // Parse the target certificate.
-  scoped_refptr<ParsedCertificate> target =
-      ParseCertificateFromOSHandle(input_cert->os_cert_handle(), &errors);
+  scoped_refptr<ParsedCertificate> target = ParseCertificateFromOSHandle(
+      input_cert->os_cert_handle(), &parsing_errors);
   if (!target) {
     // TODO(crbug.com/634443): Surface these parsing errors?
     verify_result->cert_status |= CERT_STATUS_INVALID;
     return;
   }
 
   std::unique_ptr<SystemTrustStore> trust_store =
       CreateSystemTrustStore(additional_trust_anchors);
 
   // TODO(eroman): The path building code in this file enforces its idea of weak
@@ skipping 47 matching lines @@
   const CertPathBuilder::ResultPath& partial_path =
       *result.paths[result.best_result_index].get();
 
   if (partial_path.path.trust_anchor) {
     verify_result->is_issued_by_known_root =
         trust_store->IsKnownRoot(partial_path.path.trust_anchor);
 
     verify_result->is_issued_by_additional_trust_anchor =
         trust_store->IsAdditionalTrustAnchor(partial_path.path.trust_anchor);
   } else {
+    // TODO(eroman): This shouldn't be necessary -- partial_path.errors should
+    // contain an error if it didn't chain to trust anchor.
     verify_result->cert_status |= CERT_STATUS_AUTHORITY_INVALID;
   }
 
   verify_result->verified_cert =
       CreateVerifiedCertChain(input_cert, partial_path);
 
   AppendPublicKeyHashes(partial_path, &verify_result->public_key_hashes);
-  MapPathBuilderErrorsToCertStatus(partial_path, hostname,
+  MapPathBuilderErrorsToCertStatus(partial_path.errors,
                                    &verify_result->cert_status);
+
+  // TODO(eroman): Is it possible that IsValid() fails but no errors were set in
+  // partial_path.errors?
+  CHECK(partial_path.IsValid() ||
+        IsCertStatusError(verify_result->cert_status));
+
+  if (partial_path.errors.ContainsHighSeverityErrors()) {
+    LOG(ERROR) << "CertVerifyProcBuiltin for " << hostname << " failed:\n"
+               << partial_path.errors.ToDebugString(partial_path.path.certs);
+  }
 }
 
 int CertVerifyProcBuiltin::VerifyInternal(
     X509Certificate* input_cert,
     const std::string& hostname,
     const std::string& ocsp_response,
     int flags,
     CRLSet* crl_set,
     const CertificateList& additional_trust_anchors,
     CertVerifyResult* verify_result) {
   DoVerify(input_cert, hostname, ocsp_response, flags, crl_set,
            additional_trust_anchors, verify_result);
 
   return IsCertStatusError(verify_result->cert_status)
              ? MapCertStatusToNetError(verify_result->cert_status)
              : OK;
 }
 
 }  // namespace
 
 scoped_refptr<CertVerifyProc> CreateCertVerifyProcBuiltin() {
   return scoped_refptr<CertVerifyProc>(new CertVerifyProcBuiltin());
 }
 
 }  // namespace net