Make it a fatal SSL error when encountering certs signed with md[2,4], and interstitial md5

net/base/x509_certificate.cc

 // Copyright (c) 2011 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/base/x509_certificate.h"
 
 #include <stdlib.h>
 
 #include <algorithm>
 #include <map>
@@ skipping 572 matching lines @@
 
   int rv = VerifyInternal(hostname, flags, crl_set, verify_result);
 
   // This check is done after VerifyInternal so that VerifyInternal can fill in
   // the list of public key hashes.
   if (IsPublicKeyBlacklisted(verify_result->public_key_hashes)) {
     verify_result->cert_status |= CERT_STATUS_AUTHORITY_INVALID;
     rv = MapCertStatusToNetError(verify_result->cert_status);
   }
 
+  // Treat certificates signed using broken signature algorithms as invalid.
+  if (verify_result->has_md2 || verify_result->has_md4) {
+    verify_result->cert_status |= CERT_STATUS_INVALID;
+    rv = MapCertStatusToNetError(verify_result->cert_status);
+  }
+
+  // Flag certificates using weak signature algorithms.
+  if (verify_result->has_md5) {
+    verify_result->cert_status |= CERT_STATUS_WEAK_SIGNATURE_ALGORITHM;
+    rv = MapCertStatusToNetError(verify_result->cert_status);

[joth, 2011/11/03 09:09:38]

I just noticed all three of these MapCertStatusToNetError may overwrite a more
serious error in |rv| with a less critical one. I can't see a pretty way to
factor it out, but possibly we want to guard them:

  if (rv == OK) rv = MapCertStatusToNetError(verify_result->cert_status);

(internally MapCertStatusToNetError attempts to report the most serious error in
rv, we can do that in the general case here as rv may already contain a platform
specific error of unknown severity)

[palmer, 2011/11/03 17:47:04]

As I understand it, only CERT_STATUS_WEAK_SIGNATURE_ALGORITHM is potentially
less severe than what might already be set. CERT_STATUS_INVALID and
CERT_STATUS_AUTHORITY_INVALID are already fully fatal, right?

[joth, 2011/11/03 20:01:27]

The case I was thinking of was VerifyInternal may return something
super-duper-severe, but then any of there overwrite it here.
In practice though I agree this would only be a risk for the non-fatal
WEAK_SIGNATURE_ALGORITHM case .

[Ryan Sleevi, 2011/11/03 23:22:45]

Good catch. You're absolutely correct that this could potentially mask errors
(primarily, library errors, from looking at the implementations).

I've updated it to reflect your suggestion - to only replace |rv| when the
existing rv == OK, at least for the MD5 case.

This does mean that there is the potential for system library errors to be
replaced with a more generic error (ERR_CERT_INVALID) when using MD2/MD4 . These
will still result in non-user-overridable errors though.

I'm not thrilled with this, but I'm less thrilled about the idea of pushing the
'severity' of errors (user-overridable vs fatal) further into net/ from content/
&& chrome/.

+  }
+
   return rv;
 }
 
 #if !defined(USE_NSS)
 bool X509Certificate::VerifyNameMatch(const std::string& hostname) const {
   std::vector<std::string> dns_names, ip_addrs;
   GetSubjectAltName(&dns_names, &ip_addrs);
   return VerifyHostname(hostname, subject_.common_name, dns_names, ip_addrs);
 }
 #endif
@@ skipping 138 matching lines @@
 bool X509Certificate::IsSHA1HashInSortedArray(const SHA1Fingerprint& hash,
                                               const uint8* array,
                                               size_t array_byte_len) {
   DCHECK_EQ(0u, array_byte_len % base::kSHA1Length);
   const size_t arraylen = array_byte_len / base::kSHA1Length;
   return NULL != bsearch(hash.data, array, arraylen, base::kSHA1Length,
                          CompareSHA1Hashes);
 }
 
 }  // namespace net