Make HSTS headers not clobber preloaded pins.

net/http/transport_security_state.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 "net/http/transport_security_state.h"
 
 #if defined(USE_OPENSSL)
 #include <openssl/ecdsa.h>
 #include <openssl/ssl.h>
 #else  // !defined(USE_OPENSSL)
@@ skipping 117 matching lines @@
   DomainStateMap::iterator i = enabled_hosts_.find(
       HashHost(canonicalized_host));
   if (i != enabled_hosts_.end()) {
     enabled_hosts_.erase(i);
     DirtyNotify();
     return true;
   }
   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;
     }
 
     return false;
   }
 
   return false;
 }
 
 void TransportSecurityState::ClearDynamicData() {
   DCHECK(CalledOnValidThread());
   enabled_hosts_.clear();
 }
 
 void TransportSecurityState::DeleteAllDynamicDataSince(const base::Time& time) {
   DCHECK(CalledOnValidThread());
 
   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;
   }
 
   if (dirtied)
     DirtyNotify();
 }
 
 TransportSecurityState::~TransportSecurityState() {
   DCHECK(CalledOnValidThread());
@@ skipping 319 matching lines @@
 static bool HasPreload(const struct HSTSPreload* entries, size_t num_entries,
                        const std::string& canonicalized_host, size_t i,
                        TransportSecurityState::DomainState* out, bool* ret) {
   for (size_t j = 0; j < num_entries; j++) {
     if (entries[j].length == canonicalized_host.size() - i &&
         memcmp(entries[j].dns_name, &canonicalized_host[i],
                entries[j].length) == 0) {
       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++;
           }
         }
       }
       return true;
     }
   }
   return false;
 }
 
@@ skipping 27 matching lines @@
 }
 
 bool TransportSecurityState::AddHSTSHeader(const std::string& host,
                                            const std::string& value) {
   DCHECK(CalledOnValidThread());
 
   base::Time now = base::Time::Now();
   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;
   }
   return false;
 }
 
 bool TransportSecurityState::AddHPKPHeader(const std::string& host,
                                            const std::string& value,
                                            const SSLInfo& ssl_info) {
   DCHECK(CalledOnValidThread());
 
   base::Time now = base::Time::Now();
   base::TimeDelta max_age;
   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;
   }
   return false;
 }
 
 bool TransportSecurityState::AddHSTS(const std::string& host,
                                      const base::Time& expiry,
                                      bool include_subdomains) {
   DCHECK(CalledOnValidThread());
 
   // Copy-and-modify the existing DomainState for this host (if any).
   TransportSecurityState::DomainState domain_state;
   const std::string canonicalized_host = CanonicalizeHost(host);
   const std::string hashed_host = HashHost(canonicalized_host);
   DomainStateMap::const_iterator i = enabled_hosts_.find(
       hashed_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;
 }
 
 bool TransportSecurityState::AddHPKP(const std::string& host,
                                      const base::Time& expiry,
                                      bool include_subdomains,
                                      const HashValueVector& hashes) {
   DCHECK(CalledOnValidThread());
 
   // Copy-and-modify the existing DomainState for this host (if any).
   TransportSecurityState::DomainState domain_state;
   const std::string canonicalized_host = CanonicalizeHost(host);
   const std::string hashed_host = HashHost(canonicalized_host);
   DomainStateMap::const_iterator i = enabled_hosts_.find(
       hashed_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;
 }
 
 // static
 bool TransportSecurityState::IsGooglePinnedProperty(const std::string& host,
                                                     bool sni_enabled) {
   std::string canonicalized_host = CanonicalizeHost(host);
   const struct HSTSPreload* entry =
       GetHSTSPreload(canonicalized_host, kPreloadedSTS, kNumPreloadedSTS);
@@ skipping 42 matching lines @@
   // We consider built-in information to be timely for 10 weeks.
   return (base::Time::Now() - build_time).InDays() < 70 /* 10 weeks */;
 }
 
 bool TransportSecurityState::GetStaticDomainState(
     const std::string& canonicalized_host,
     bool sni_enabled,
     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();
 
   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);
     out->domain = DNSDomainToString(host_sub_chunk);
     bool ret;
     if (is_build_timely &&
         HasPreload(kPreloadedSTS, kNumPreloadedSTS, canonicalized_host, i, out,
@@ skipping 23 matching lines @@
   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);
     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);
 
     // 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;
     }
 
     return false;
   }
 
   return false;
 }
 
 
 void TransportSecurityState::AddOrUpdateEnabledHosts(
     const std::string& hashed_host, const DomainState& state) {
   DCHECK(CalledOnValidThread());
   enabled_hosts_[hashed_host] = state;
 }
 
 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() {
 }
 
 bool TransportSecurityState::DomainState::CheckPublicKeyPins(
     const HashValueVector& hashes) const {
   // Validate that hashes is not empty. By the time this code is called (in
   // production), that should never happen, but it's good to be defensive.
   // And, hashes *can* be empty in some test scenarios.
   if (hashes.empty()) {
     LOG(ERROR) << "Rejecting empty public key chain for public-key-pinned "
                   "domain " << domain;
     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 {
   return true;
 }
 
 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