Disable SHA-1 for Enterprise Certs

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 461 matching lines @@
     rv = MapCertStatusToNetError(verify_result->cert_status);
   }
 
   if (verify_result->has_sha1)
     verify_result->cert_status |= CERT_STATUS_SHA1_SIGNATURE_PRESENT;
 
   // Flag certificates using weak signature algorithms.
   // The CA/Browser Forum Baseline Requirements (beginning with v1.2.1)
   // prohibits SHA-1 certificates from being issued beginning on
   // 1 January 2016. Ideally, all of SHA-1 in new certificates would be
   // disabled on this date, but enterprises need more time to transition.

[eroman, 2016/12/13 01:22:47]

Should this comment be updated?

[Ryan Sleevi, 2016/12/13 01:29:57]

I'm not sure it should be, but I'm admittedly thinking in terms of "We wouldn't
even offer the flag" (it's scheduled for 2019 to be removed)

   // As the risk is greatest for publicly trusted certificates, prevent
   // those certificates from being trusted from that date forward.
-  //
-  // TODO(mattm): apply the SHA-1 deprecation check to all certs unless
-  // CertVerifier::VERIFY_ENABLE_SHA1_LOCAL_ANCHORS flag is present.
+  bool disable_sha1 = verify_result->is_issued_by_known_root ||
+                      !(flags & CertVerifier::VERIFY_ENABLE_SHA1_LOCAL_ANCHORS);
   if (verify_result->has_md5 ||
       // Current SHA-1 behaviour:
       // - Reject all publicly trusted SHA-1
       // - ... unless it's in the intermediate and SHA-1 intermediates are

[eroman, 2016/12/13 01:22:47]

This comment also seems incomplete now.

[Ryan Sleevi, 2016/12/13 01:29:57]

Why does it seem incomplete?

It's consistent gramatically with 489, and logically with lines 494-495.

[eroman, 2016/12/13 01:49:33]

It doesn't note the per-request override.

       //   allowed for that platform. See https://crbug.com/588789
       (!sha1_legacy_mode_enabled &&

[eroman, 2016/12/13 01:22:47]

This big nested if-statement is really hard to read. I had to do several
pass-throughs to gain confidence it does what it claims to do.

How about splitting this into smaller sections using short-circuits, and less
nesting? Say:

bool UsesWeakSignature(...) {
  if (verify_result->has_md5)
    return true;

  if (...)
    return true;

  ...

  return false;
}

Another point of confusion when reading this is the inconsistency between
"sha1_legacy_mode_enabled" being an affirmative boolean for sha1 being allowed,
whereas the per-request version, "disable_sha1" is in the opposite direction.

[Ryan Sleevi, 2016/12/13 01:29:57]

I explicitly wanted to avoid this. I'd be curious to better understand what
caused trouble, because I tried to match both the textual description and the
condition, and to holistically group all of the necessary conditions together
(legacy mode vs non-legacy mode)
I'm not sure I fully grok this. Certainly, legacy_mode is temporary (it was
going to be removed in M57, but after the CT timebomb, I'm more inclined to wait
until 56 has fully hit stable before removing this), but I don't think having
two booleans is fundamentally at odds, nor is it necessary to have both their
default states equal the same conditional.

[eroman, 2016/12/13 01:49:33]

Similar concerns as hilghted in this article:
https://goto.google.com/nxqkx

Large nested expressions with parens, or, and, take more effort to visually
parse. Readers tend to rely on the whitespace indentation to decipher, which can
also be the source of bugs. Forget some parens around comment-delineated
subexpressions and the meaning changes.

If your goal is clarity I would definitely break this up.

[Ryan Sleevi, 2016/12/13 02:22:47]

I guess this doesn't really help me understand why.

That is, there are quite a few ways to botch this conditional by simplifying,
and the current structure seems to me to be clearer because it describes the
three states, and it makes sure prose and code are kept in sync and close by. At
least judging by the CT policy bugs, this was a source of issues in the past -
by 'simplifying' the conditional, several CT policy bugs were introduced.

For example, upon re-reading this, there's a bug in that legacy mode (our
emergency fallback for any SHA-1 issues) would have fallen back to fully
disabling SHA-1 after 2016-01-01, rather than truly representing the 'legacy'
mode.

-       (verify_result->is_issued_by_known_root &&
+       (disable_sha1 &&
         (verify_result->has_sha1_leaf ||
          (verify_result->has_sha1 && !AreSHA1IntermediatesAllowed())))) ||
       // Legacy SHA-1 behaviour:
       // - Reject all publicly trusted SHA-1 leaf certs issued after
       //   2016-01-01.
-      (sha1_legacy_mode_enabled && (verify_result->has_sha1_leaf &&
-                                    verify_result->is_issued_by_known_root &&
-                                    IsPastSHA1DeprecationDate(*cert)))) {
+      (sha1_legacy_mode_enabled &&
+       (verify_result->has_sha1_leaf && disable_sha1 &&
+        IsPastSHA1DeprecationDate(*cert)))) {
     verify_result->cert_status |= CERT_STATUS_WEAK_SIGNATURE_ALGORITHM;
     // Avoid replacing a more serious error, such as an OS/library failure,
     // by ensuring that if verification failed, it failed with a certificate
     // error.
     if (rv == OK || IsCertificateError(rv))
       rv = MapCertStatusToNetError(verify_result->cert_status);
   }
 
   // Flag certificates from publicly-trusted CAs that are issued to intranet
   // hosts. While the CA/Browser Forum Baseline Requirements (v1.1) permit
@@ skipping 257 matching lines @@
     return true;
 
   return false;
 }
 
 // static
 const base::Feature CertVerifyProc::kSHA1LegacyMode{
     "SHA1LegacyMode", base::FEATURE_DISABLED_BY_DEFAULT};
 
 }  // namespace net