Refactor the assignment of CertVerifyResult::has_md2, etc.

net/cert/cert_verify_proc.cc

 // Copyright (c) 2012 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.h"
 
 #include <stdint.h>
 
 #include <algorithm>
 
@@ skipping 351 matching lines @@
 #if defined(OS_WIN)
   // TODO(rsleevi): Remove this once https://crbug.com/588789 is resolved
   // for Windows 7/2008 users.
   // Note: This must be kept in sync with cert_verify_proc_unittest.cc
   return base::win::GetVersion() < base::win::VERSION_WIN8;
 #else
   return false;
 #endif
 };
 
+// Sets the weak signature hash fields of |verify_result| to true if
+// applicable for |cert|, otherwise does not modify them.
+void FillCertVerifyResultWeakHashAlgorithms(X509Certificate::OSCertHandle cert,
+                                            bool is_leaf,

[Ryan Sleevi, 2017/01/10 01:53:24]

Two suggested changes:
1) It seems like using the X509Certificate::SignatureHashAlgorithm for this
helper function may reduce the "What this function does". It's a minor quibble
though.
2) You can remove the is_leaf parameter with the simplification below.

[eroman, 2017/01/10 02:48:46]

Done.

+                                            CertVerifyResult* verify_result) {
+  X509Certificate::SignatureHashAlgorithm hash =
+      X509Certificate::GetSignatureHashAlgorithm(cert);
+  switch (hash) {
+    case X509Certificate::kSignatureHashAlgorithmMd2:
+      verify_result->has_md2 = true;
+      break;
+    case X509Certificate::kSignatureHashAlgorithmMd4:
+      verify_result->has_md4 = true;
+      break;
+    case X509Certificate::kSignatureHashAlgorithmMd5:
+      verify_result->has_md5 = true;
+      break;
+    case X509Certificate::kSignatureHashAlgorithmSha1:
+      verify_result->has_sha1 = true;
+      if (is_leaf)
+        verify_result->has_sha1_leaf = true;
+      break;
+    case X509Certificate::kSignatureHashAlgorithmOther:
+      break;
+  }
+}
+
+// Fills in the booleans on |verify_result| relating to weak hashing algorithms
+// used by the certificate chain:
+void CheckWeakHashAlgorithms(CertVerifyResult* verify_result) {

[Ryan Sleevi, 2017/01/10 01:53:24]

Naming wise, I think CheckWeakHashAlgorithms and
FillCertVerifyResultHashAlgorithms are probably... overly long and unfortunately
not as descriptive.

To me, "check" implies some conditional behaviour, but instead, it's populating.
FillCertVerifyResultHashAlgorithms is, well, a mouthful :)

ComputeSignatureAlgorithms(CertVerifyResult* verify_result);
MapCertHashAlgorithm ?


I suppose one dimension here is I have never really thought of the "has_*" as
the "weak signature fields" so much as, well, just details about the signature.

I would say this is largely a minor nit, but the wording feels pretty stilted.

[eroman, 2017/01/10 02:48:46]

Done.

+  const X509Certificate::OSCertHandles& intermediates =
+      verify_result->verified_cert->GetIntermediateCertificates();
+
+  // Consider weak hash algorithms in all certificates except trusted
+  // certificates.
+  //
+  // There is some loss of information as to which certificate is the trust
+  // anchor:
+  //
+  //  * If verification was successful, and |!intermediates.empty()|, the trust
+  //    anchor is |intermediates.back()|.
+  //
+  //  * If verification was successful, and |intermediates.empty()|, the leaf
+  //    certificate is trusted.
+  //
+  //  * However, if verification was not successful, there may or may not be a
+  //    trust anchor.
+  //
+  // The following code assumes that the final certificate in the chain is the
+  // trust anchor (which may not be true for the case of failed verifications,
+  // when a partial chain was returned).
+  //
+  // This inaccuracy should not be harmful, as it just impacts the final error.
+  if (!intermediates.empty()) {

[Ryan Sleevi, 2017/01/10 01:53:24]

Does it make sense to make this error-short circuit more explicit

// Explanation of why this short-circuit
if (intermediates.empty())
  return;

FillCertVerifyResultWeakHashAlgorithms(verify_result->verified_cert->os_cert_handle(),
verify_result);
verify_result->has_sha1_leaf = verify_result->has_sha1;

for (const auto& os_cert : intermediates) {
  FillCertVerifyResultWEakHashAlgorithms(os_cert, verify_result);
}

[eroman, 2017/01/10 02:48:46]

Done.

+    for (size_t i = 0; i + 1 < intermediates.size(); ++i) {
+      FillCertVerifyResultWeakHashAlgorithms(intermediates[i],
+                                             false /*is_leaf*/, verify_result);
+    }
+
+    FillCertVerifyResultWeakHashAlgorithms(
+        verify_result->verified_cert->os_cert_handle(), true /*is_leaf*/,
+        verify_result);
+  }
+}
+
 }  // namespace
 
 // static
 CertVerifyProc* CertVerifyProc::CreateDefault() {
 #if defined(USE_NSS_CERTS)
   return new CertVerifyProcNSS();
 #elif defined(USE_OPENSSL_CERTS) && !defined(OS_ANDROID)
   return new CertVerifyProcOpenSSL();
 #elif defined(OS_ANDROID)
   return new CertVerifyProcAndroid();
@@ skipping 31 matching lines @@
   // We do online revocation checking for EV certificates that aren't covered
   // by a fresh CRLSet.
   // TODO(rsleevi): http://crbug.com/142974 - Allow preferences to fully
   // disable revocation checking.
   if (flags & CertVerifier::VERIFY_EV_CERT)
     flags |= CertVerifier::VERIFY_REV_CHECKING_ENABLED_EV_ONLY;
 
   int rv = VerifyInternal(cert, hostname, ocsp_response, flags, crl_set,
                           additional_trust_anchors, verify_result);
 
+  // Fill in verify_result->has_md2 and related booleans early so that
+  // subsequent code can rely on it being meaningful.

[Ryan Sleevi, 2017/01/10 01:53:24]

Seems like an unnecessary comment (or perhaps unnecessarily verbose)

// Fill in the verify_result->has_* booleans
...

Namely, it should be obvious "so that subsequent code can rely on it being
meaningful" is implied from position. I would even go as far as suggesting no
comment needed :)

[eroman, 2017/01/10 02:48:46]

Done.

+  CheckWeakHashAlgorithms(verify_result);
+
   UMA_HISTOGRAM_BOOLEAN("Net.CertCommonNameFallback",
                         verify_result->common_name_fallback_used);
   if (!verify_result->is_issued_by_known_root) {
     UMA_HISTOGRAM_BOOLEAN("Net.CertCommonNameFallbackPrivateCA",
                           verify_result->common_name_fallback_used);
   }
 
   CheckOCSP(ocsp_response, *verify_result->verified_cert,
             &verify_result->ocsp_result);
 
@@ skipping 334 matching lines @@
   if (start >= time_2015_04_01 && month_diff > 39)
     return true;
 
   return false;
 }
 
 // static
 const base::Feature CertVerifyProc::kSHA1LegacyMode{
     "SHA1LegacyMode", base::FEATURE_DISABLED_BY_DEFAULT};
 
-X509Certificate::SignatureHashAlgorithm FillCertVerifyResultWeakSignature(
-    X509Certificate::OSCertHandle cert,
-    bool is_leaf,
-    CertVerifyResult* verify_result) {
-  X509Certificate::SignatureHashAlgorithm hash =
-      X509Certificate::GetSignatureHashAlgorithm(cert);
-  switch (hash) {
-    case X509Certificate::kSignatureHashAlgorithmMd2:
-      verify_result->has_md2 = true;
-      break;
-    case X509Certificate::kSignatureHashAlgorithmMd4:
-      verify_result->has_md4 = true;
-      break;
-    case X509Certificate::kSignatureHashAlgorithmMd5:
-      verify_result->has_md5 = true;
-      break;
-    case X509Certificate::kSignatureHashAlgorithmSha1:
-      verify_result->has_sha1 = true;
-      if (is_leaf)
-        verify_result->has_sha1_leaf = true;
-      break;
-    case X509Certificate::kSignatureHashAlgorithmOther:
-      break;
-  }
-
-  return hash;
-}
-
 }  // namespace net