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

net/base/x509_certificate.cc

Index: net/base/x509_certificate.cc
diff --git a/net/base/x509_certificate.cc b/net/base/x509_certificate.cc
index efb19eeb718168d8e82a215e65bf925f75eb9364..477c79e15d7689af464650c3ba092380cbd755f6 100644
--- a/net/base/x509_certificate.cc
+++ b/net/base/x509_certificate.cc
@@ -597,6 +597,25 @@ int X509Certificate::Verify(const std::string& hostname,
     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) {
+    bool has_cert_status_error =

[wtc, 2011/12/02 23:04:59]

Nit: has_cert_status_error => cert_status_has_error ?

+        IsCertStatusError(verify_result->cert_status);
+    verify_result->cert_status |= CERT_STATUS_WEAK_SIGNATURE_ALGORITHM;
+    // Only replace the error code if verification was successful or if the
+    // error has also been reported in |cert_status|. This is to avoid the
+    // possibility of replacing a more fatal error (such as an OS/library

[wtc, 2011/12/02 23:04:59]

Nit: remove "the possibility of".

+    // failure), which may not be reported in |cert_status|.
+    if (rv == OK || (IsCertificateError(rv) && has_cert_status_error))

[Ryan Sleevi, 2011/11/20 00:17:00]

I believe the following check should be a sufficient guard against any extra
overwrites, but there still remains the possibility of a more informative error
being masked.

Using simply IsCertificateError(rv) as the check can miss situations in _win and
_mac where the library calls fail, but are mapped to certificate errors.
Normally fatal, if just IsCE() was used, these would be made into warnings.

Using simply IsCertStatusError() equally has problems, in that there are times
when a system API call may fail, and the error code is not set to a certificate
error (eg: ERR_FAILED). ->cert_status may only have minor errors, but we want to
treat the error as fatal.

Using both should hopefully be sufficient.

If/as we add more checks, and the possibility of more warnings, then this may
need to be revisited. 

[wtc, 2011/12/02 23:04:59]

I'm still not convinced that we should check
has_cert_status_error here.

On 2011/11/20 00:17:00, Ryan Sleevi wrote:
Can you give a concrete example?  I think this should
not happen.  If |rv| is a certificate error, it should
be the most serious error in verify_result->cert_status.
If this property is not maintained, it is a bug.

[Ryan Sleevi, 2011/12/02 23:54:28]

Then it's a bug - MapSecurityError() can return a certificate error without
updating CertStatus - these specifically were the two lines that concerned me.
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/net/base/x509_certifi...
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/net/base/x509_certifi...

This is also potentially dangerous if MapSecurityError and
MapCertErrrorToCertStatus get out of sync (but not presently a bug)
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/net/base/x509_certifi...

OS X is fine because NetErrorFromOSStatus will never return a certificate error,
and OpenSSL is fine because it only sets CertStatus and then returns the error
mapped from CertStatus.

If this is unexpected (and it sounds like it is), then I can just update those
three call sites to ensure at least CERT_STATUS_INVALID is set on the cert. The
potential problem with this is that it could mask a more meaningful error, but
that sounds acceptable.

[wtc, 2011/12/06 00:56:17]

It is true that MapSecurityError in x509_certificate_win.cc
may return a certificate error, but that is because
MapSecurityError has some old code.  I don't think
CertGetCertificateChain will fail with a certificate error,
because it has a much better way to report certificate
errors.

In any case, you can add code to update verify_result->cert_status
if MapSecurityError returns a certificate error.

+      rv = MapCertStatusToNetError(verify_result->cert_status);
+  }
+
   return rv;
 }