Implement CORS preflights for Expect-CT reports

chrome/browser/ssl/chrome_expect_ct_reporter.cc

Index: chrome/browser/ssl/chrome_expect_ct_reporter.cc
diff --git a/chrome/browser/ssl/chrome_expect_ct_reporter.cc b/chrome/browser/ssl/chrome_expect_ct_reporter.cc
index e56942601b05426dee75926286d1902f0f04351f..d0de033f7b5ae1fafec4b00bb21f9d9a355652c7 100644
--- a/chrome/browser/ssl/chrome_expect_ct_reporter.cc
+++ b/chrome/browser/ssl/chrome_expect_ct_reporter.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/ssl/chrome_expect_ct_reporter.h"
 
+#include <set>
 #include <string>
 
 #include "base/base64.h"
@@ -14,15 +15,39 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/sparse_histogram.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/strings/string_split.h"
+#include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/values.h"
 #include "chrome/common/chrome_features.h"
+#include "net/base/load_flags.h"
 #include "net/cert/ct_serialization.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
 #include "net/url_request/report_sender.h"
+#include "net/url_request/url_request_context.h"
 
 namespace {
 
+// Returns true if |request| contains any of the |expected_values| in a response
+// header field named |header|. |expected_values| are expected to be lower-case
+// and the check is case-insensitive.
+bool FindHeaderValues(net::URLRequest* request,

[meacer, 2017/07/05 23:48:59]

nit: Rename to RequestHasHeaderWithValue or simply HasHeaderValues?

[estark, 2017/07/06 06:39:43]

Done.

+                      const std::string& header,
+                      const std::set<std::string>& expected_values) {

[meacer, 2017/07/05 23:48:59]

nit: allowed_values instead of expected_values? expected_values slightly implies
that all values are expected.

[estark, 2017/07/06 06:39:43]

Done.

+  std::string response_headers;
+  request->GetResponseHeaderByName(header, &response_headers);
+  std::vector<std::string> response_values = base::SplitString(

[meacer, 2017/07/05 23:48:59]

nit: const

[estark, 2017/07/06 06:39:43]

Done.

+      response_headers, ",", base::KEEP_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
+  for (const auto& value : response_values) {
+    for (const auto& expected : expected_values) {
+      if (base::ToLowerASCII(expected) == base::ToLowerASCII(value)) {
+        return true;
+      }
+    }
+  }
+  return false;
+}
+
 std::string TimeToISO8601(const base::Time& t) {
   base::Time::Exploded exploded;
   t.UTCExplode(&exploded);
@@ -129,7 +154,8 @@ constexpr net::NetworkTrafficAnnotationTag kTrafficAnnotation =
 ChromeExpectCTReporter::ChromeExpectCTReporter(
     net::URLRequestContext* request_context)
     : report_sender_(
-          new net::ReportSender(request_context, kTrafficAnnotation)) {}
+          new net::ReportSender(request_context, kTrafficAnnotation)),
+      request_context_(request_context) {}
 
 ChromeExpectCTReporter::~ChromeExpectCTReporter() {}
 
@@ -173,7 +199,78 @@ void ChromeExpectCTReporter::OnExpectCTFailed(
 
   UMA_HISTOGRAM_BOOLEAN("SSL.ExpectCTReportSendingAttempt", true);
 
-  report_sender_->Send(report_uri, "application/json; charset=utf-8",
-                       serialized_report, base::Callback<void()>(),
+  SendPreflight(report_uri, serialized_report);
+}
+
+void ChromeExpectCTReporter::OnResponseStarted(net::URLRequest* request,
+                                               int net_error) {
+  auto preflight_it = inflight_preflights_.find(request);
+  DCHECK(inflight_preflights_.end() != inflight_preflights_.find(request));
+  const InFlightPreflight& preflight = preflight_it->second;
+
+  int response_code = request->GetResponseCode();

[meacer, 2017/07/05 23:48:59]

nit: const

[estark, 2017/07/06 06:39:43]

Done.

+
+  // Check that the preflight succeeded: it must have an HTTP OK status code,
+  // with the following headers:
+  // - Access-Control-Allow-Origin: * or null
+  // - Access-Control-Allow-Methods: POST
+  // - Access-Control-Allow-Headers: Content-Type
+
+  if (!request->status().is_success() ||
+      (response_code < 200 || response_code > 299)) {
+    inflight_preflights_.erase(request);
+    RecordUMAOnFailure(preflight.report_uri, request->status().error(),

[meacer, 2017/07/05 23:48:58]

There is a use after free here: preflight is a reference to the value keyed by
request, and the line above erases that value. Same in line 232-233.

You can move the erase line below this line, but that's still error prone as
someone can add a line below that accesses preflight. Maybe add a comment as
well?

Alternatively, copy (or rather std::move) preflight instead. You can then move
|inflight_preflights_.erase(request)| right after line 209 and remove the
duplicate calls. But then you can't simply copy an InFlightPreflight, you'll
need a move constructor etc etc.

[estark, 2017/07/06 06:39:43]

Done. Moved the erase below and added comments.

+                       request->status().is_success() ? response_code : -1);
+    return;
+  }
+
+  if (!FindHeaderValues(request, "Access-Control-Allow-Origin",
+                        {"*", "null"}) ||
+      !FindHeaderValues(request, "Access-Control-Allow-Methods", {"post"}) ||
+      !FindHeaderValues(request, "Access-Control-Allow-Headers",
+                        {"content-type"})) {
+    inflight_preflights_.erase(request);
+    RecordUMAOnFailure(preflight.report_uri, request->status().error(),
+                       response_code);
+    return;
+  }
+
+  report_sender_->Send(preflight.report_uri,
+                       "application/expect-ct-report+json; charset=utf-8",
+                       preflight.serialized_report, base::Callback<void()>(),
                        base::Bind(RecordUMAOnFailure));
+  inflight_preflights_.erase(request);
+}
+
+void ChromeExpectCTReporter::OnReadCompleted(net::URLRequest* request,
+                                             int bytes_read) {
+  NOTREACHED();
+}
+
+ChromeExpectCTReporter::InFlightPreflight::InFlightPreflight() {}
+ChromeExpectCTReporter::InFlightPreflight::~InFlightPreflight() {}
+
+void ChromeExpectCTReporter::SendPreflight(
+    const GURL& report_uri,
+    const std::string& serialized_report) {
+  std::unique_ptr<net::URLRequest> url_request =
+      request_context_->CreateRequest(report_uri, net::DEFAULT_PRIORITY, this,
+                                      kTrafficAnnotation);
+  url_request->SetLoadFlags(net::LOAD_BYPASS_CACHE | net::LOAD_DISABLE_CACHE |
+                            net::LOAD_DO_NOT_SEND_AUTH_DATA |
+                            net::LOAD_DO_NOT_SEND_COOKIES |
+                            net::LOAD_DO_NOT_SAVE_COOKIES);
+  url_request->set_method("OPTIONS");
+
+  net::HttpRequestHeaders extra_headers;
+  extra_headers.SetHeader("Origin", "null");
+  extra_headers.SetHeader("Access-Control-Request-Method", "POST");
+  extra_headers.SetHeader("Access-Control-Request-Headers", "content-type");
+  url_request->SetExtraRequestHeaders(extra_headers);
+
+  net::URLRequest* raw_request = url_request.get();
+  inflight_preflights_[raw_request].request = std::move(url_request);
+  inflight_preflights_[raw_request].serialized_report = serialized_report;
+  inflight_preflights_[raw_request].report_uri = report_uri;

[meacer, 2017/07/05 23:48:59]

optional nit: To save two extra map lookups you can add a constructor to
InFlightPreflight with these 3 params and then simply build it here.

nflight_preflights_[raw_request] = InflightPreFlight(url_request, ...);

The added benefit is that you can also make the members of InflightPreFlight all
const members.

[estark, 2017/07/06 06:39:43]

Done. Changed to a unique_ptr in the map to avoid a copy.

+  raw_request->Start();
 }