Include cert status in invalid certificate reports

chrome/browser/net/certificate_error_reporter.cc

 // Copyright 2015 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 "chrome/browser/net/certificate_error_reporter.h"
 
 #include <set>
 
 #include "base/logging.h"
 #include "base/stl_util.h"
@@ skipping 10 matching lines @@
 #include "net/base/elements_upload_data_stream.h"
 #include "net/base/load_flags.h"
 #include "net/base/request_priority.h"
 #include "net/base/upload_bytes_element_reader.h"
 #include "net/cert/x509_certificate.h"
 #include "net/ssl/ssl_info.h"
 #include "net/url_request/url_request_context.h"
 
 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,
     0x3c, 0x61, 0xa7, 0x96, 0x76, 0x86, 0x91, 0x40, 0x71, 0x39, 0x5f,
     0x31, 0x1a, 0x39, 0x5b, 0x76, 0xb1, 0x6b, 0x3d, 0x6a, 0x2b};
 static const uint32 kServerPublicKeyVersion = 1;
 
 #if defined(USE_OPENSSL)
 
 static const char kHkdfLabel[] = "certificate report";
@@ skipping 33 matching lines @@
   encrypted_report->set_server_public_key_version(server_public_key_version);
   encrypted_report->set_client_public_key(
       std::string((char*)public_key, sizeof(public_key)));
   encrypted_report->set_algorithm(
       chrome_browser_net::EncryptedCertLoggerRequest::
           AEAD_ECDH_AES_128_CTR_HMAC_SHA256);
   return true;
 }
 #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 {
 
 CertificateErrorReporter::CertificateErrorReporter(
     net::URLRequestContext* request_context,
     const GURL& upload_url,
     CookiesPreference cookies_preference)
     : CertificateErrorReporter(request_context,
                                upload_url,
@@ skipping 160 matching lines @@
 
   std::vector<std::string> pem_encoded_chain;
   if (!ssl_info.cert->GetPEMEncodedChain(&pem_encoded_chain))
     LOG(ERROR) << "Could not get PEM encoded chain.";
 
   std::string* cert_chain = out_request->mutable_cert_chain();
   for (size_t i = 0; i < pem_encoded_chain.size(); ++i)
     *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) {
   std::set<net::URLRequest*>::iterator i = inflight_requests_.find(request);
   DCHECK(i != inflight_requests_.end());
   scoped_ptr<net::URLRequest> url_request(*i);
   inflight_requests_.erase(i);
 }
 
 }  // namespace chrome_browser_net