Include both certificate chains in invalid cert reporting

chrome/browser/net/chrome_fraudulent_certificate_reporter.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 "chrome/browser/net/chrome_fraudulent_certificate_reporter.h"
 
 #include <set>
 
 #include "base/base64.h"
 #include "base/logging.h"
@@ skipping 22 matching lines @@
     net::URLRequestContext* request_context)
     : request_context_(request_context),
       pinning_violation_upload_url_(kFraudulentCertificateUploadEndpoint),
       invalid_chain_upload_url_(kInvalidCertificateChainUploadEndpoint) {
 }
 
 ChromeFraudulentCertificateReporter::~ChromeFraudulentCertificateReporter() {
   STLDeleteElements(&inflight_requests_);
 }
 
-static std::string BuildReport(const std::string& hostname,
-                               const net::SSLInfo& ssl_info) {
+// Helper function for |BuildReport|. Appends each PEM-encoded
+// certificate in the chain starting at |cert| to |chain_from_report|.
+static void AddCertChainToReport(const scoped_refptr<net::X509Certificate> cert,
+                                 std::string* chain_from_report) {
+  std::vector<std::string> pem_encoded_chain;
+  if (!cert || !cert->GetPEMEncodedChain(&pem_encoded_chain)) {

[felt, 2015/02/25 16:41:32]

is it possible and expected that this can be called with !cert? (maybe this can
happen for INVALID_CERT errors, but I am not sure; have you checked?) should
this be a DCHECK making sure that the cert has a value instead?

+    LOG(ERROR) << "Could not get PEM encoded chain.";

[felt, 2015/02/25 16:41:32]

should there also be a return here?

+  }
+  for (size_t i = 0; i < pem_encoded_chain.size(); ++i)
+    *chain_from_report += pem_encoded_chain[i];
+}
+
+std::string ChromeFraudulentCertificateReporter::BuildReport(
+    ChromeFraudulentCertificateReporter::ReportType type,
+    const std::string& hostname,
+    const net::SSLInfo& ssl_info) {
   CertLoggerRequest request;
   base::Time now = base::Time::Now();
   request.set_time_usec(now.ToInternalValue());
   request.set_hostname(hostname);
 
-  std::vector<std::string> pem_encoded_chain;
-  if (!ssl_info.cert->GetPEMEncodedChain(&pem_encoded_chain)) {
-    LOG(ERROR) << "Could not get PEM encoded chain.";
+  AddCertChainToReport(ssl_info.cert, request.mutable_cert_chain());
+  if (type ==
+      ChromeFraudulentCertificateReporter::REPORT_TYPE_EXTENDED_REPORTING) {
+    AddCertChainToReport(ssl_info.unverified_server_cert,
+                         request.mutable_unverified_server_cert_chain());
   }
-  std::string* cert_chain = request.mutable_cert_chain();
-  for (size_t i = 0; i < pem_encoded_chain.size(); ++i)
-    *cert_chain += pem_encoded_chain[i];
 
   request.add_pin(ssl_info.pinning_failure_log);
 
   std::string out;
   request.SerializeToString(&out);
   return out;
 }
 
 scoped_ptr<net::URLRequest>
 ChromeFraudulentCertificateReporter::CreateURLRequest(
     net::URLRequestContext* context,
     const GURL& upload_url) {
   scoped_ptr<net::URLRequest> request =
       context->CreateRequest(upload_url, net::DEFAULT_PRIORITY, this, NULL);
   request->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
                         net::LOAD_DO_NOT_SAVE_COOKIES);
   return request.Pass();
 }
 
 void ChromeFraudulentCertificateReporter::SendReport(
     ReportType type,
     const std::string& hostname,
     const net::SSLInfo& ssl_info) {
   if (type == REPORT_TYPE_EXTENDED_REPORTING) {
     // TODO(estark): Double-check that the user is opted in.
 
     // TODO(estark): Temporarily, since there is no upload endpoint, just log
     // the information.
     LOG(ERROR) << "SSL report for " << hostname << ":\n"
-               << BuildReport(hostname, ssl_info) << "\n\n";
+               << BuildReport(type, hostname, ssl_info) << "\n\n";
     return;
   }
 
   // We do silent/automatic reporting ONLY for Google properties. For other
   // domains (when we start supporting that), we will ask for user permission.
   if (!net::TransportSecurityState::IsGooglePinnedProperty(hostname)) {
     return;
   }
 
-  std::string report = BuildReport(hostname, ssl_info);
+  std::string report = BuildReport(type, hostname, ssl_info);
 
   scoped_ptr<net::URLRequest> url_request =
       CreateURLRequest(request_context_, pinning_violation_upload_url_);
   url_request->set_method("POST");
 
   scoped_ptr<net::UploadElementReader> reader(
       net::UploadOwnedBytesElementReader::CreateWithString(report));
   url_request->set_upload(
       net::ElementsUploadDataStream::CreateWithReader(reader.Pass(), 0));
 
@@ skipping 34 matching lines @@
     LOG(WARNING) << "Certificate upload HTTP status: "
                  << request->GetResponseCode();
   }
   RequestComplete(request);
 }
 
 void ChromeFraudulentCertificateReporter::OnReadCompleted(
     net::URLRequest* request, int bytes_read) {}
 
 }  // namespace chrome_browser_net