Make HSTS headers not clobber preloaded pins.

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 e6c5b4263b3ffa56703f9a85013b8802ab21c7af..d5a01910e376c6f38c444d43d41356b47178d8e5 100644
--- a/net/http/transport_security_state.cc
+++ b/net/http/transport_security_state.cc
@@ -135,52 +135,66 @@ bool TransportSecurityState::DeleteDynamicDataForHost(const std::string& host) {
   return false;
 }
 
+// Given |older| and |newer|, takes the newest parts of |older| and
+// |newer| and converges them into |older|.

[Ryan Sleevi, 2014/01/31 20:24:43]

It took me a little bit to parse this comment, in particular "converges them
into |older|"

style wise,
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Param...


// Updates |other| to contain the most recent observations found
// between the states |state| and |other|.
bool MergeDomainStates(const TransportSecurityState::DomainState& state,
                       TransportSecurityState::DomainState* other) {

[palmer, 2014/01/31 22:20:51]

Done.

+bool MergeDomainStates(TransportSecurityState::DomainState* older,
+                       const TransportSecurityState::DomainState& newer) {
+  if (older->dynamic_pkp.last_observed < newer.dynamic_pkp.last_observed) {
+    older->dynamic_pkp = newer.dynamic_pkp;
+  }
+  if (older->dynamic_sts.last_observed < newer.dynamic_sts.last_observed) {
+    older->dynamic_sts = newer.dynamic_sts;
+  }
+  return true;

[Ryan Sleevi, 2014/01/31 20:24:43]

why bool if you always return true?

[palmer, 2014/01/31 22:20:51]

Same reason as I have curly braces for 1-line if blocks: legacy of debugging. :)
Removed curlies and changed bool to void.

Also moved it up into the anonymous namespace.

+}
+
 bool TransportSecurityState::GetDomainState(const std::string& host,
                                             bool sni_enabled,
                                             DomainState* result) {
   DCHECK(CalledOnValidThread());
 
-  DomainState state;
   const std::string canonicalized_host = CanonicalizeHost(host);
   if (canonicalized_host.empty())
     return false;
 
-  bool has_preload = GetStaticDomainState(canonicalized_host, sni_enabled,
-                                          &state);
-  std::string canonicalized_preload = CanonicalizeHost(state.domain);
-  GetDynamicDomainState(host, &state);
-
+  DomainState static_state;
+  bool has_static = GetStaticDomainState(canonicalized_host, sni_enabled,
+                                         &static_state);
+  std::string canonicalized_preload = CanonicalizeHost(static_state.domain);
+  DomainState dynamic_state = static_state;

[Ryan Sleevi, 2014/01/31 20:24:43]

Add a comment explaining why you initialize dynamic_state to static_state, as
opposed to leaving it uninitialized. This isn't entirely clear to me,
considering line 174

[palmer, 2014/01/31 22:20:51]

I think this is legacy from before I wrote MergeDomainStates. Removing it does
not change the test-passingness, so removed.

+  bool has_dynamic = GetDynamicDomainState(host, &dynamic_state);
   base::Time current_time(base::Time::Now());
 
   for (size_t i = 0; canonicalized_host[i]; i += canonicalized_host[i] + 1) {
     std::string host_sub_chunk(&canonicalized_host[i],
                                canonicalized_host.size() - i);
     // Exact match of a preload always wins.
-    if (has_preload && host_sub_chunk == canonicalized_preload) {
-      *result = state;
+    if (has_static && host_sub_chunk == canonicalized_preload) {
+      if (has_dynamic)
+        MergeDomainStates(&static_state, dynamic_state);

[Ryan Sleevi, 2014/01/31 20:24:43]

Add a comment here as to why you're updating |static_state| with information
that is contained in |dynamic_state|, which is a bit surprising.

[palmer, 2014/01/31 22:20:51]

Done.

+      *result = static_state;
       return true;
     }
 
-    DomainStateMap::iterator j =
-        enabled_hosts_.find(HashHost(host_sub_chunk));
+    DomainStateMap::iterator j = enabled_hosts_.find(HashHost(host_sub_chunk));
     if (j == enabled_hosts_.end())
       continue;
 
-    if (current_time > j->second.upgrade_expiry &&
-        current_time > j->second.dynamic_spki_hashes_expiry) {
+    if (current_time > j->second.dynamic_sts.expiry &&
+        current_time > j->second.dynamic_pkp.expiry) {
       enabled_hosts_.erase(j);
       DirtyNotify();
       continue;
     }
 
-    state = j->second;
-    state.domain = DNSDomainToString(host_sub_chunk);
+    dynamic_state = j->second;
+    dynamic_state.domain = DNSDomainToString(host_sub_chunk);
 
     // Succeed if we matched the domain exactly or if subdomain matches are
     // allowed.
-    if (i == 0 || j->second.sts_include_subdomains ||
-        j->second.pkp_include_subdomains) {
-      *result = state;
+    if (i == 0 || j->second.dynamic_sts.include_subdomains ||
+        j->second.dynamic_pkp.include_subdomains) {
+      *result = dynamic_state;
       return true;
     }
 
@@ -201,18 +215,19 @@ void TransportSecurityState::DeleteAllDynamicDataSince(const base::Time& time) {
   bool dirtied = false;
   DomainStateMap::iterator i = enabled_hosts_.begin();
   while (i != enabled_hosts_.end()) {
-    if (i->second.sts_observed >= time && i->second.pkp_observed >= time) {
+    if (i->second.dynamic_sts.last_observed >= time &&
+        i->second.dynamic_pkp.last_observed >= time) {
       dirtied = true;
       enabled_hosts_.erase(i++);
       continue;
     }
 
-    if (i->second.sts_observed >= time) {
+    if (i->second.dynamic_sts.last_observed >= time) {
       dirtied = true;
-      i->second.upgrade_mode = DomainState::MODE_DEFAULT;
-    } else if (i->second.pkp_observed >= time) {
+      i->second.dynamic_sts.upgrade_mode = DomainState::MODE_DEFAULT;
+    } else if (i->second.dynamic_pkp.last_observed >= time) {
       dirtied = true;
-      i->second.dynamic_spki_hashes.clear();
+      i->second.dynamic_pkp.spki_hashes.clear();
     }
     ++i;
   }
@@ -552,22 +567,28 @@ static bool HasPreload(const struct HSTSPreload* entries, size_t num_entries,
       if (!entries[j].include_subdomains && i != 0) {
         *ret = false;
       } else {
-        out->sts_include_subdomains = entries[j].include_subdomains;
-        out->pkp_include_subdomains = entries[j].include_subdomains;
+        out->static_sts.include_subdomains = entries[j].include_subdomains;
+        out->static_sts.last_observed = base::GetBuildTime();
+        out->static_pkp.include_subdomains = entries[j].include_subdomains;
+        out->static_pkp.last_observed = base::GetBuildTime();
         *ret = true;
+        out->has_static_sts = entries[j].https_required;
         if (!entries[j].https_required)
-          out->upgrade_mode = TransportSecurityState::DomainState::MODE_DEFAULT;
+          out->static_sts.upgrade_mode =
+              TransportSecurityState::DomainState::MODE_DEFAULT;
         if (entries[j].pins.required_hashes) {
+          out->has_static_pkp = true;
           const char* const* sha1_hash = entries[j].pins.required_hashes;
           while (*sha1_hash) {
-            AddHash(*sha1_hash, &out->static_spki_hashes);
+            AddHash(*sha1_hash, &out->static_pkp.spki_hashes);
             sha1_hash++;
           }
         }
         if (entries[j].pins.excluded_hashes) {
+          out->has_static_pkp = true;
           const char* const* sha1_hash = entries[j].pins.excluded_hashes;
           while (*sha1_hash) {
-            AddHash(*sha1_hash, &out->bad_static_spki_hashes);
+            AddHash(*sha1_hash, &out->static_pkp.bad_spki_hashes);
             sha1_hash++;
           }
         }
@@ -615,14 +636,15 @@ bool TransportSecurityState::AddHSTSHeader(const std::string& host,
   base::TimeDelta max_age;
   TransportSecurityState::DomainState domain_state;
   GetDynamicDomainState(host, &domain_state);
-  if (ParseHSTSHeader(value, &max_age, &domain_state.sts_include_subdomains)) {
+  if (ParseHSTSHeader(value, &max_age,
+                      &domain_state.dynamic_sts.include_subdomains)) {
     // Handle max-age == 0
     if (max_age.InSeconds() == 0)
-      domain_state.upgrade_mode = DomainState::MODE_DEFAULT;
+      domain_state.dynamic_sts.upgrade_mode = DomainState::MODE_DEFAULT;
     else
-      domain_state.upgrade_mode = DomainState::MODE_FORCE_HTTPS;
-    domain_state.sts_observed = now;
-    domain_state.upgrade_expiry = now + max_age;
+      domain_state.dynamic_sts.upgrade_mode = DomainState::MODE_FORCE_HTTPS;
+    domain_state.dynamic_sts.last_observed = now;
+    domain_state.dynamic_sts.expiry = now + max_age;
     EnableHost(host, domain_state);
     return true;
   }
@@ -639,11 +661,11 @@ bool TransportSecurityState::AddHPKPHeader(const std::string& host,
   TransportSecurityState::DomainState domain_state;
   GetDynamicDomainState(host, &domain_state);
   if (ParseHPKPHeader(value, ssl_info.public_key_hashes,
-                      &max_age, &domain_state.pkp_include_subdomains,
-                      &domain_state.dynamic_spki_hashes)) {
+                      &max_age, &domain_state.dynamic_pkp.include_subdomains,
+                      &domain_state.dynamic_pkp.spki_hashes)) {
     // TODO(palmer): http://crbug.com/243865 handle max-age == 0.
-    domain_state.pkp_observed = now;
-    domain_state.dynamic_spki_hashes_expiry = now + max_age;
+    domain_state.dynamic_pkp.last_observed = now;
+    domain_state.dynamic_pkp.expiry = now + max_age;
     EnableHost(host, domain_state);
     return true;
   }
@@ -664,10 +686,10 @@ bool TransportSecurityState::AddHSTS(const std::string& host,
   if (i != enabled_hosts_.end())
     domain_state = i->second;
 
-  domain_state.sts_observed = base::Time::Now();
-  domain_state.sts_include_subdomains = include_subdomains;
-  domain_state.upgrade_expiry = expiry;
-  domain_state.upgrade_mode = DomainState::MODE_FORCE_HTTPS;
+  domain_state.dynamic_sts.last_observed = base::Time::Now();
+  domain_state.dynamic_sts.include_subdomains = include_subdomains;
+  domain_state.dynamic_sts.expiry = expiry;
+  domain_state.dynamic_sts.upgrade_mode = DomainState::MODE_FORCE_HTTPS;
   EnableHost(host, domain_state);
   return true;
 }
@@ -687,10 +709,10 @@ bool TransportSecurityState::AddHPKP(const std::string& host,
   if (i != enabled_hosts_.end())
     domain_state = i->second;
 
-  domain_state.pkp_observed = base::Time::Now();
-  domain_state.pkp_include_subdomains = include_subdomains;
-  domain_state.dynamic_spki_hashes_expiry = expiry;
-  domain_state.dynamic_spki_hashes = hashes;
+  domain_state.dynamic_pkp.last_observed = base::Time::Now();
+  domain_state.dynamic_pkp.include_subdomains = include_subdomains;
+  domain_state.dynamic_pkp.expiry = expiry;
+  domain_state.dynamic_pkp.spki_hashes = hashes;
   EnableHost(host, domain_state);
   return true;
 }
@@ -753,9 +775,9 @@ bool TransportSecurityState::GetStaticDomainState(
     DomainState* out) {
   DCHECK(CalledOnValidThread());
 
-  out->upgrade_mode = DomainState::MODE_FORCE_HTTPS;
-  out->sts_include_subdomains = false;
-  out->pkp_include_subdomains = false;
+  out->static_sts.upgrade_mode = DomainState::MODE_FORCE_HTTPS;
+  out->static_sts.include_subdomains = false;
+  out->static_pkp.include_subdomains = false;
 
   const bool is_build_timely = IsBuildTimely();
 
@@ -799,8 +821,8 @@ bool TransportSecurityState::GetDynamicDomainState(const std::string& host,
     if (j == enabled_hosts_.end())
       continue;
 
-    if (current_time > j->second.upgrade_expiry &&
-        current_time > j->second.dynamic_spki_hashes_expiry) {
+    if (current_time > j->second.dynamic_sts.expiry &&
+        current_time > j->second.dynamic_pkp.expiry) {
       enabled_hosts_.erase(j);
       DirtyNotify();
       continue;
@@ -811,8 +833,8 @@ bool TransportSecurityState::GetDynamicDomainState(const std::string& host,
 
     // Succeed if we matched the domain exactly or if subdomain matches are
     // allowed.
-    if (i == 0 || j->second.sts_include_subdomains ||
-        j->second.pkp_include_subdomains) {
+    if (i == 0 || j->second.dynamic_sts.include_subdomains ||
+        j->second.dynamic_pkp.include_subdomains) {
       *result = state;
       return true;
     }
@@ -831,12 +853,11 @@ void TransportSecurityState::AddOrUpdateEnabledHosts(
 }
 
 TransportSecurityState::DomainState::DomainState()
-    : upgrade_mode(MODE_DEFAULT),
-      sts_include_subdomains(false),
-      pkp_include_subdomains(false) {
-  base::Time now(base::Time::Now());
-  sts_observed = now;
-  pkp_observed = now;
+  : has_static_sts(false),
+    has_static_pkp(false) {
+  dynamic_sts.upgrade_mode = MODE_DEFAULT;
+  dynamic_sts.include_subdomains = false;
+  dynamic_pkp.include_subdomains = false;
 }
 
 TransportSecurityState::DomainState::~DomainState() {
@@ -853,32 +874,36 @@ bool TransportSecurityState::DomainState::CheckPublicKeyPins(
     return false;
   }
 
-  if (HashesIntersect(bad_static_spki_hashes, hashes)) {
+  if (HashesIntersect(static_pkp.bad_spki_hashes, hashes) ||
+      HashesIntersect(dynamic_pkp.bad_spki_hashes, hashes)) {
     LOG(ERROR) << "Rejecting public key chain for domain " << domain
                << ". Validated chain: " << HashesToBase64String(hashes)
                << ", matches one or more bad hashes: "
-               << HashesToBase64String(bad_static_spki_hashes);
+               << HashesToBase64String(static_pkp.bad_spki_hashes) <<
+               ", " << HashesToBase64String(dynamic_pkp.bad_spki_hashes);
     return false;
   }
 
   // If there are no pins, then any valid chain is acceptable.
-  if (dynamic_spki_hashes.empty() && static_spki_hashes.empty())
+  if (dynamic_pkp.spki_hashes.empty() && static_pkp.spki_hashes.empty()) {
     return true;
+  }
 
-  if (HashesIntersect(dynamic_spki_hashes, hashes) ||
-      HashesIntersect(static_spki_hashes, hashes)) {
+  if (HashesIntersect(dynamic_pkp.spki_hashes, hashes) ||
+      HashesIntersect(static_pkp.spki_hashes, hashes)) {

[Ryan Sleevi, 2014/01/31 20:24:43]

This bit surprises me.

Consider a preload of (A, B, C) and a header observation of (A, B). This code
will, in effect, allow C even though we've observed a header that says to *not*
allow C

if (HashesIntersect(dynamic_pkp.spki_hashes, hash) ||
    (dynamic_pkp.spki_hashes.empty() && HashesIntersect(static_pkp.spki_hashes,
hashes))) {

}?

(With a corresponding comment explaining the above logic)

[palmer, 2014/01/31 22:20:51]

Have we in fact decided that? (Maybe we have; I haven't looked at the I-D in too
long.)

Note that the current behavior is to fail (slightly) open, which seems prudent
given the risk of bricking and given the expected-known-goodness of static
hashes.
That change causes HttpSecurityHeadersTest.NoClobberPins to fail (perhaps
wrongly).

[palmer, 2014/02/20 22:13:27]

Where do we think we are on this? I'd like to get this bug closed, if we think
we are at all close. Can the remaining concerns be handled in subsequent CLs?

     return true;
   }
 
   LOG(ERROR) << "Rejecting public key chain for domain " << domain
              << ". Validated chain: " << HashesToBase64String(hashes)
-             << ", expected: " << HashesToBase64String(dynamic_spki_hashes)
-             << " or: " << HashesToBase64String(static_spki_hashes);
+             << ", expected: " << HashesToBase64String(dynamic_pkp.spki_hashes)
+             << " or: " << HashesToBase64String(static_pkp.spki_hashes);
   return false;
 }
 
 bool TransportSecurityState::DomainState::ShouldUpgradeToSSL() const {
-  return upgrade_mode == MODE_FORCE_HTTPS;
+  return dynamic_sts.upgrade_mode == MODE_FORCE_HTTPS ||

[Ryan Sleevi, 2014/01/31 20:24:43]

If we observed a dynamic header with an explicit expiration of 0, should we
still use static pins?

(I'm happy to punt on this as being a bug in our implementation, I just wanted
to make sure we agreed that it is a puntable bug and not a critical bug)

[palmer, 2014/01/31 22:20:51]

I think so, following the same logic as above ("static believed known-good").

But I could be convinced either way. I should really get back into the I-D...
I think it is puntable, if it is a bug.

+         static_sts.upgrade_mode == MODE_FORCE_HTTPS;
 }
 
 bool TransportSecurityState::DomainState::ShouldSSLErrorsBeFatal() const {
@@ -886,9 +911,9 @@ bool TransportSecurityState::DomainState::ShouldSSLErrorsBeFatal() const {
 }
 
 bool TransportSecurityState::DomainState::HasPublicKeyPins() const {
-  return static_spki_hashes.size() > 0 ||
-         bad_static_spki_hashes.size() > 0 ||
-         dynamic_spki_hashes.size() > 0;
+  return static_pkp.spki_hashes.size() > 0 ||
+         static_pkp.bad_spki_hashes.size() > 0 ||
+         dynamic_pkp.spki_hashes.size() > 0;
 }
 
 }  // namespace