Implement Expect-Staple

net/http/transport_security_state.cc

Index: net/http/transport_security_state.cc
diff --git a/net/http/transport_security_state.cc b/net/http/transport_security_state.cc
index 16217a0c022e9f2149c698e9b45e9b1e1b6ed9ec..2663ec96d3413cb92b6aab4fb4ed1401554bbcd5 100644
--- a/net/http/transport_security_state.cc
+++ b/net/http/transport_security_state.cc
@@ -24,6 +24,7 @@
 #include "crypto/sha2.h"
 #include "net/base/host_port_pair.h"
 #include "net/cert/ct_policy_status.h"
+#include "net/cert/internal/parse_ocsp.h"
 #include "net/cert/x509_cert_types.h"
 #include "net/cert/x509_certificate.h"
 #include "net/dns/dns_util.h"
@@ -41,11 +42,58 @@ const size_t kMaxHPKPReportCacheEntries = 50;
 const int kTimeToRememberHPKPReportsMins = 60;
 const size_t kReportCacheKeyLength = 16;
 
+struct OCSPStaple {

[estark, 2016/06/09 21:24:14]

could probably use some comments

(in particular, OCSPStaple is the stuff about a stapled response that
Expect-Staple cares about, right? at first I was thinking that it was going to
hold the actual response. maybe it should be named ExpectStapleResult or
something)

[dadrian, 2016/06/10 01:05:52]

In retrospect, I think I'm going to move this and related functions to a
separate file to make everything easier to test. It can handle going from the
representations in parse_ocsp.h to a representation not encumbered by
der::Input, etc. I've tentatively started filling out net/cert/ocsp_staple.h
with this.

+  OCSPStaple();
+  ~OCSPStaple();
+
+  bool is_date_valid;
+  bool is_correct_certificate;
+  OCSPCertStatus::Status status;
+};
+
+OCSPStaple::OCSPStaple()
+    : is_date_valid(false),
+      is_correct_certificate(false),
+      status(OCSPCertStatus::Status::UNKNOWN) {}
+
+OCSPStaple::~OCSPStaple() {}
+
 void RecordUMAForHPKPReportFailure(const GURL& report_uri, int net_error) {
   UMA_HISTOGRAM_SPARSE_SLOWLY("Net.PublicKeyPinReportSendingFailure",
                               net_error);
 }
 
+der::GeneralizedTime ConvertExplodedTime(const base::Time::Exploded& exploded) {
+  der::GeneralizedTime result;
+  result.year = exploded.year;
+  result.month = exploded.month;
+  result.day = exploded.day_of_month;
+  result.hours = exploded.hour;
+  result.minutes = exploded.minute;
+  result.seconds = exploded.second;
+  return result;
+}
+
+bool CheckOCSPDateValid(bool has_next_update,

[svaldez, 2016/06/13 14:03:03]

Add a comment to the RFC referencing the this_update/next_update validity
periods.

+                        const base::Time& check_time,
+                        const der::GeneralizedTime& this_update,
+                        const der::GeneralizedTime& next_update) {
+  if (has_next_update && !(this_update < next_update))
+    return false;
+  base::Time::Exploded exploded;
+  check_time.UTCExplode(&exploded);
+  der::GeneralizedTime check_time_der = ConvertExplodedTime(exploded);
+  return (this_update < check_time_der) &&

[svaldez, 2016/06/13 14:03:03]

You possibly need a fudge factor in case of clock skew? Might only matter for
super special servers that generate the stapled OCSP live.

+         (!has_next_update || check_time_der < next_update);

[estark, 2016/06/09 21:24:14]

Wait, so it's considered valid if there's no next update? Why is that?

[dadrian, 2016/06/10 01:05:52]

This part is still kind of early. nextUpdate is optional because CA's may choose
to always provide a fresh response (i.e. an optional nextUpdate implies
nextUpdate == thisUpdate). We should probably set some hard limit on the delta
between both thisUpdate and now(), and thisUpdate and nextUpdate. This will
prevent sites from constantly replaying the same result for the lifetime of the
certificate.

[estark, 2016/06/14 02:10:27]

Is this what the new |max_age| parameter addresses, or is that something else?

The optional |nextUpdate| thing still kinda makes my brain hurt. If missing
nextUpdate implies that nextUpdate == thisUpdate, doesn't that mean that the
client should always *reject* responses with a missing nextUpdate? (i.e. if they
are received after thisUpdate == nextUpdate, then they are received after
nextUpdate, so they should be rejected as expired?)

And is the client free to choose whatever maximum validity period it wants, or
are there any limits on how long a response can be valid?

[svaldez, 2016/06/14 18:56:13]

The validity period is set by the OCSP responder. Basically according to the RFC
there are two validity cases here. If nextUpdate is set, then we require the
current time to be in (thisUpdate, nextUpdate), otherwise we require the current
time to be in (thisUpdate, thisUpdate+max_age).

I think what we actually want is to check that the current time is in the range
(thisUpdate, max(thisUpdate+max_age, nextUpdate)), with necessary clock skew
fudge factors. Since otherwise we have edge conditions of OCSP responders that
decide that nextUpdate should be a second from now, making the responses
validity period 1 second.

Additionally you might want a check that producedAt isn't too far in the past,
though I don't think that gives security benefits, other than revealing OCSP
responders that have decided to pre-sign a bunch of validity responses.

+}
+
+bool CompareCertIDToCertificate(const OCSPCertID& cert_id,
+                                const X509Certificate& certificate) {
+  // TODO: Verify name and key hashes

[estark, 2016/06/09 21:24:14]

todo format:

// TODO(dadrian): ... https://crbug.com/XXXXXX

[dadrian, 2016/06/10 01:05:52]

Acknowledged.

+  der::Input serial(&certificate.serial_number());
+  return serial == cert_id.serial_number;
+}
+
 std::string TimeToISO8601(const base::Time& t) {
   base::Time::Exploded exploded;
   t.UTCExplode(&exploded);
@@ -155,6 +203,96 @@ bool GetHPKPReport(const HostPortPair& host_port_pair,
   return true;
 }
 
+std::string OCSPCertStatusToString(OCSPCertStatus::Status status) {
+  switch (status) {
+    case OCSPCertStatus::Status::GOOD:
+      return "GOOD";
+    case OCSPCertStatus::Status::REVOKED:
+      return "REVOKED";
+    case OCSPCertStatus::Status::UNKNOWN:
+      return "UNKNOWN";
+    default:
+      NOTREACHED();

[estark, 2016/06/09 21:24:14]

nit: I like to write these with no default and just a `return ""` at the end of
the function so that if someone adds a new enum value without updating the
switch statement, it won't compile.

[dadrian, 2016/06/10 01:05:52]

Oh, that causes compile failure? Nice.

[svaldez, 2016/06/13 14:03:03]

Might even make sense to move this into the parse_ocsp file.

+      return "";
+  }
+}
+
+static bool ParseAndCheckOCSPResponse(const std::string& raw_response,

[estark, 2016/06/09 21:24:14]

the 'static' shouldn't be necessary

[dadrian, 2016/06/10 01:05:52]

Done.

+                                      const X509Certificate& certificate,
+                                      const base::Time& check_time,
+                                      std::vector<OCSPStaple>* staples) {
+  der::Input response_der(&raw_response);
+  OCSPResponse response;
+  if (!ParseOCSPResponse(response_der, &response))
+    return false;
+
+  // If the OCSP response isn't status SUCCESSFUL, don't parse the rest of the
+  // data.
+  if (response.status != OCSPResponse::ResponseStatus::SUCCESSFUL)
+    return false;

[estark, 2016/06/09 21:24:14]

Hrm. Should we be somehow reporting these error conditions (doesn't parse, or
has a status other than successful)? We can modify the report format however we
want, it's not sent in stone or anything. If I'm reading correctly a site
operator wouldn't be able to distinguish no-staples from any of these other
error conditions, which seems odd.

[dadrian, 2016/06/10 01:05:52]

I was contemplating this as well. An extra top-level enum for failure type might
be useful, something along the lines of { MISSING, PARSE_RESPONSE, PARSE_DATA,
PARSE_SINGLE_RESPONSE, INVALID }.

[svaldez, 2016/06/13 14:03:04]

++, we should consider adding reporting to get a better idea of the state of the
world for OCSP responses. Though we can already get a fairly good idea from
checking commonly visited site.

+  OCSPResponseData response_data;
+  if (!ParseOCSPResponseData(response.data, &response_data))
+    return false;
+
+  bool contains_correct_response = false;
+  for (const auto& single_response_der : response_data.responses) {
+    OCSPSingleResponse single_response;
+    if (!ParseOCSPSingleResponse(single_response_der, &single_response))
+      continue;
+    OCSPStaple staple;
+    staple.status = single_response.cert_status.status;
+    staple.is_date_valid = CheckOCSPDateValid(
+        single_response.has_next_update, check_time,
+        single_response.this_update, single_response.next_update);
+    OCSPCertID cert_id;
+    if (ParseOCSPCertID(single_response.cert_id_tlv, &cert_id)) {
+      staple.is_correct_certificate =
+          CompareCertIDToCertificate(cert_id, certificate);
+    }
+    if (staple.is_date_valid && staple.is_correct_certificate &&
+        staple.status == OCSPCertStatus::Status::GOOD) {
+      contains_correct_response = true;
+    }
+    staples->push_back(staple);
+  }
+  return contains_correct_response;
+}
+
+bool GetExpectStapleReport(
+    const HostPortPair& host_port_pair,
+    const TransportSecurityState::ExpectStapleState& expect_staple_state,
+    const X509Certificate& certificate,
+    const base::Time& check_time,
+    const std::vector<OCSPStaple>& ocsp_staples,
+    std::string* serialized_report) {
+  base::DictionaryValue report;
+  report.SetString("date-time", TimeToISO8601(check_time));
+  report.SetString("hostname", host_port_pair.host());
+  report.SetInteger("port", host_port_pair.port());
+
+  // Add the list of each stapled OCSP response
+  std::unique_ptr<base::Value> ocsp_responses(new base::ListValue);
+  for (const auto& staple : ocsp_staples) {
+    std::unique_ptr<base::DictionaryValue> response(new base::DictionaryValue);
+    response->SetBoolean("is-date-valid", staple.is_date_valid);
+    response->SetBoolean("is-correct-certificate",
+                         staple.is_correct_certificate);
+    response->SetString("status", OCSPCertStatusToString(staple.status));
+    base::ListValue* responses_list =
+        reinterpret_cast<base::ListValue*>(ocsp_responses.get());
+    responses_list->Append(std::move(response));
+  }
+  report.Set("ocsp-responses", std::move(ocsp_responses));
+  report.Set("served-certificate-chain",
+             GetPEMEncodedChainAsList(&certificate));
+
+  if (!base::JSONWriter::Write(report, serialized_report)) {
+    LOG(ERROR) << "Failed to serialize Expect-Staple report";
+    return false;
+  }
+  return true;
+}
+
 // Do not send a report over HTTPS to the same host that set the
 // pin. Such report URIs will result in loops. (A.com has a pinning
 // violation which results in a report being sent to A.com, which
@@ -617,8 +755,8 @@ TransportSecurityState::TransportSecurityState()
       report_sender_(nullptr),
       enable_static_pins_(true),
       enable_static_expect_ct_(true),
-      enable_static_expect_staple_(false),
       expect_ct_reporter_(nullptr),

[svaldez, 2016/06/13 14:03:03]

Extra change?

[dadrian, 2016/06/13 23:03:32]

Formatting error. Fixed.

+      enable_static_expect_staple_(false),
       sent_reports_cache_(kMaxHPKPReportCacheEntries) {
 // Static pinning is only enabled for official builds to make sure that
 // others don't end up with pins that cannot be easily updated.
@@ -1098,6 +1236,31 @@ void TransportSecurityState::ProcessExpectCTHeader(
                                         ssl_info);
 }
 
+void TransportSecurityState::CheckExpectStaple(
+    const HostPortPair& host_port_pair,
+    const ExpectStapleState& expect_staple_state,
+    const X509Certificate& certificate,
+    const std::string& ocsp_response) {
+  DCHECK(CalledOnValidThread());
+  if (!enable_static_expect_staple_ || !report_sender_)

[estark, 2016/06/09 21:24:14]

might as well check for an empty report_uri here as well

[dadrian, 2016/06/10 01:05:52]

Done.

+    return;
+
+  // Check the stapled information.
+  std::vector<OCSPStaple> ocsp_staples;
+  base::Time check_time = base::Time::Now();
+  if (ParseAndCheckOCSPResponse(ocsp_response, certificate, check_time,
+                                &ocsp_staples))
+    return;
+
+  // Report on failure.
+  std::string serialized_report;
+  if (!GetExpectStapleReport(host_port_pair, expect_staple_state, certificate,
+                             check_time, ocsp_staples, &serialized_report)) {

[estark, 2016/06/09 21:24:14]

nit: curly braces inconsistent with line 1251

[dadrian, 2016/06/10 01:05:52]

Done.

+    return;
+  }
+  report_sender_->Send(expect_staple_state.report_uri, serialized_report);
+}
+
 // static
 void TransportSecurityState::ReportUMAOnPinFailure(const std::string& host) {
   PreloadResult result;