Include cert status in invalid certificate reports

chrome/browser/net/certificate_error_reporter.cc

Index: chrome/browser/net/certificate_error_reporter.cc
diff --git a/chrome/browser/net/certificate_error_reporter.cc b/chrome/browser/net/certificate_error_reporter.cc
index edff489f4ca0762dac6eb03bf554a1bc7e0adef2..b5de02482eede20f84db04b35801659dee01155b 100644
--- a/chrome/browser/net/certificate_error_reporter.cc
+++ b/chrome/browser/net/certificate_error_reporter.cc
@@ -28,6 +28,8 @@
 
 namespace {
 
+using chrome_browser_net::CertLoggerRequest;
+
 // Constants used for crypto
 static const uint8 kServerPublicKey[] = {
     0x51, 0xcc, 0x52, 0x67, 0x42, 0x47, 0x3b, 0x10, 0xe8, 0x63, 0x18,
@@ -81,6 +83,41 @@ bool EncryptSerializedReport(
 }
 #endif
 
+void AddCertStatusToReportErrors(
+    net::CertStatus cert_status,
+    CertLoggerRequest* report) {
+  if (cert_status & net::CERT_STATUS_REVOKED)
+    report->add_cert_error(CertLoggerRequest::ERR_CERT_REVOKED);
+  if (cert_status & net::CERT_STATUS_INVALID)
+    report->add_cert_error(CertLoggerRequest::ERR_CERT_INVALID);
+  if (cert_status & net::CERT_STATUS_PINNED_KEY_MISSING)
+    report->add_cert_error(
+        CertLoggerRequest::ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN);
+  if (cert_status & net::CERT_STATUS_AUTHORITY_INVALID)
+    report->add_cert_error(CertLoggerRequest::ERR_CERT_AUTHORITY_INVALID);
+  if (cert_status & net::CERT_STATUS_COMMON_NAME_INVALID)
+    report->add_cert_error(CertLoggerRequest::ERR_CERT_COMMON_NAME_INVALID);
+  if (cert_status & net::CERT_STATUS_NON_UNIQUE_NAME)
+    report->add_cert_error(CertLoggerRequest::ERR_CERT_NON_UNIQUE_NAME);
+  if (cert_status & net::CERT_STATUS_NAME_CONSTRAINT_VIOLATION)
+    report->add_cert_error(
+        CertLoggerRequest::ERR_CERT_NAME_CONSTRAINT_VIOLATION);
+  if (cert_status & net::CERT_STATUS_WEAK_SIGNATURE_ALGORITHM)
+    report->add_cert_error(
+        CertLoggerRequest::ERR_CERT_WEAK_SIGNATURE_ALGORITHM);
+  if (cert_status & net::CERT_STATUS_WEAK_KEY)
+    report->add_cert_error(CertLoggerRequest::ERR_CERT_WEAK_KEY);
+  if (cert_status & net::CERT_STATUS_DATE_INVALID)
+    report->add_cert_error(CertLoggerRequest::ERR_CERT_DATE_INVALID);
+  if (cert_status & net::CERT_STATUS_VALIDITY_TOO_LONG)
+    report->add_cert_error(CertLoggerRequest::ERR_CERT_VALIDITY_TOO_LONG);
+  if (cert_status & net::CERT_STATUS_UNABLE_TO_CHECK_REVOCATION)
+    report->add_cert_error(
+        CertLoggerRequest::ERR_CERT_UNABLE_TO_CHECK_REVOCATION);
+  if (cert_status & net::CERT_STATUS_NO_REVOCATION_MECHANISM)
+    report->add_cert_error(CertLoggerRequest::ERR_CERT_NO_REVOCATION_MECHANISM);
+}

[meacer, 2015/05/06 22:45:53]

Should there be a DCHECK or CertLoggerRequest::ERR_OTHER or something similar
here as a sanity check?

[estark, 2015/05/06 23:12:34]

Hmm. I think ERR_OTHER would be unnecessary, because the absence of cert errors
would be equivalent to ERR_OTHER.

As for a DCHECK, I don't think that would work out of the box right now. This
class is used by ChromeFraudulentCertificateReporter to send pinning violation
reports on Google properties:
https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur...

It looks like the cert status flag for a pinning violation actually gets set
after the report is sent (down on line 922 of that file). So in the case of
ChromeFraudulentCertificateReporter reports, we actually won't have a
cert_status flag to report.

We could maybe rearrange that code to send the report after the cert status flag
is set, but it seems like an unnecessary guarantee to have to enforce. (Why
shouldn't callers be able to send certificate reports without cert status flags
set if they want to?)

What do you think?

[meacer, 2015/05/07 00:00:12]

Ah, I thought that was a guarantee that you wanted to provide. If not, then this
looks good as it is. Thanks for the details!

+
 }  // namespace
 
 namespace chrome_browser_net {
@@ -261,6 +298,8 @@ void CertificateErrorReporter::BuildReport(const std::string& hostname,
     *cert_chain += pem_encoded_chain[i];
 
   out_request->add_pin(ssl_info.pinning_failure_log);
+
+  AddCertStatusToReportErrors(ssl_info.cert_status, out_request);
 }
 
 void CertificateErrorReporter::RequestComplete(net::URLRequest* request) {