Refactor the assignment of CertVerifyResult::has_md2, etc.

net/cert/cert_verify_proc.cc

Index: net/cert/cert_verify_proc.cc
diff --git a/net/cert/cert_verify_proc.cc b/net/cert/cert_verify_proc.cc
index f047a7e2e45b7703718460709f8a0c7a341f22ea..2f4ec360920e2ce5e8d2141c1425a71fcf6bf8c7 100644
--- a/net/cert/cert_verify_proc.cc
+++ b/net/cert/cert_verify_proc.cc
@@ -369,6 +369,71 @@ bool AreSHA1IntermediatesAllowed() {
 #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
@@ -420,6 +485,10 @@ int CertVerifyProc::Verify(X509Certificate* cert,
   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) {
@@ -774,32 +843,4 @@ bool CertVerifyProc::HasTooLongValidity(const X509Certificate& cert) {
 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