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..e7231c6a42c8f9ab0c5f0efe03bc3e61efc7d8ab 100644
--- a/net/http/transport_security_state.cc
+++ b/net/http/transport_security_state.cc
@@ -614,6 +614,7 @@ bool TransportSecurityState::AddHSTSHeader(const std::string& host,
   base::Time now = base::Time::Now();
   base::TimeDelta max_age;
   TransportSecurityState::DomainState domain_state;
+  //GetDomainState(host, true /* SNI enabled */, &domain_state);

[Ryan Sleevi, 2013/12/27 00:21:06]

Remove

[palmer, 2014/01/03 22:43:44]

Done.

   GetDynamicDomainState(host, &domain_state);
   if (ParseHSTSHeader(value, &max_age, &domain_state.sts_include_subdomains)) {
     // Handle max-age == 0
@@ -637,6 +638,7 @@ bool TransportSecurityState::AddHPKPHeader(const std::string& host,
   base::Time now = base::Time::Now();
   base::TimeDelta max_age;
   TransportSecurityState::DomainState domain_state;
+  //GetDomainState(host, true /* SNI enabled */, &domain_state);

[Ryan Sleevi, 2013/12/27 00:21:06]

Remove

[palmer, 2014/01/03 22:43:44]

Done.

   GetDynamicDomainState(host, &domain_state);
   if (ParseHPKPHeader(value, ssl_info.public_key_hashes,
                       &max_age, &domain_state.pkp_include_subdomains,
@@ -853,27 +855,66 @@ bool TransportSecurityState::DomainState::CheckPublicKeyPins(
     return false;
   }
 
-  if (HashesIntersect(bad_static_spki_hashes, hashes)) {
+  // If there is a static entry, check its pins (if any). |this| may have
+  // been loaded via GetDynamicDomainState (only), and hence we would
+  // erroneously ignore static key pins if we did not do this check.
+  // See crbug.com/29386.

[Ryan Sleevi, 2013/12/27 00:21:06]

Avoid pronouns in comments.

// TODO(palmer): Static pins are unconditionally checked, preventing
// dynamic pins from being used to replace the static pins. This
// ensures that a dynamic header update for HSTS does not replace
// statically-loaded pins, but does prevent a dynamic header update
// via HPKP for statically pinned sites.
// See http://crbug.com/29386 and [bug for refactor] for more details.

[palmer, 2014/01/03 22:43:44]

Done.

+  //
+  // This is a terrible hack and should be resolved with a proper refactor
+  // in the very near future (as of Dec 2013). TODO(palmer).

[Ryan Sleevi, 2013/12/27 00:21:06]

note: remove this bit.

[palmer, 2014/01/03 22:43:44]

Done.

+  HashValueVector restored_static_hashes;
+  HashValueVector restored_bad_static_hashes;
+  DomainState restored_static_state;
+  bool did_restore_static_state = false;

[Ryan Sleevi, 2013/12/27 00:21:06]

The naming here is a bit confusing, because you're not actually "restoring" the
pins (which suggests mutating the DomainState).

HashValueVector preloaded_static_hashes;
HashValueVector preloaded_bad_static_hashes;
bool did_find_static_hashes = false;

+  if (IsBuildTimely()) {

[Ryan Sleevi, 2013/12/27 00:21:06]

if (TransportSecurityState::IsBuildTimely()) - you're in DomainState here, so
I'm surprised this code would compile?

[palmer, 2014/01/03 22:43:44]

<spiderman>...and I'm just sitting here, compiling it</spiderman>

Anyway, done (for improved clarity).

+    std::string canonicalized_host = CanonicalizeHost(domain);
+
+    for (size_t i = 0; canonicalized_host[i]; i += canonicalized_host[i] + 1) {
+      std::string host_sub_chunk(&canonicalized_host[i],

[Ryan Sleevi, 2013/12/27 00:21:06]

This duplicates significant portions of GetStaticDomainState(), which makes me
really uncomfortable.

Our underlying goal is this:
If receiving an HSTS header, *static* SPKI pins are not persisted, *dynamic*
SPKI pins (if any) are, and the *dynamic* HSTS state should replace the *static*
HSTS state.

If receiving an HPKP header, *static* SPKI pins are not persisted, *dynamic*
SPKI pins are updated and persisted, *static* HSTS state is not persisted and
*dynamic* HSTS state (if any) is.

Within DomainState, we already track static vs dynamic state information for
HPKP via
- pkp_observed
- static_spki_hashes vs dynamic_spki_hashes
- dynamic_spki_hashes_expiry
- bad_static_spki_hashes

We fail to distinguish between the two for pkp_include_subdomains

However, we do *not* track static vs dynamic state information for HSTS, namely
- sts_observed
- upgrade_mode
- upgrade_expiry
- sts_include_subdomains

A 'reasonable' quick-fix to DomainState would be to explicitly track the static
vs dynamic data members for PKP and STS, along with bools to track whether a
static or dynamic state exists.

Persistence would *only* persist dynamic state, if any, while evaluation would
always check dynamic state (if any) before static state.

DomainState {
  struct HSTSState {
    // sts_observed (new to track static as well)
    base::Time last_observed;

    // upgrade_expiry
    base::Time upgrade_expiry;

    // upgrade_mode (new to track static vs dynamic)
    UpgradeMode upgrade_mode;

    // sts_include_subdomains (new to track static vs dynamic)
    bool include_subdomains;
  };

  HPKPState {
    // pkp_observed (new to track static vs dynamic)
    base::Time last_observed;

    // dynamic_spki_hashes_expiry (new to track static vs dynamic)
    base::Time expiry;

    // formerly [static/dynamic]_spki_hashes
    HashValueVector spki_hashes;

    // formerly bad_static_spki_hashes;
    HashValueVector bad_spki_hashes;
  };

  bool has_static_hsts;
  HSTSState static_hsts_state;
  HSTSState dynamic_hsts_state;

  bool has_static_pkp;
  HPKPState static_hpkp_state;
  HPKPState dynamic_hpkp_state;
};


And then updating the functions as appropriate. Of course, this may require more
significant gutter-work, just due to the renaming.

This would also solve the related bug you're trying to solve here, where an HSTS
header on a sub-domain will mask the HPKP state from the parent. By explicitly
storing the static and dynamic state differently, you could continue walking
static state for PKP info even after you found HSTS info from dynamic state.

This is a much larger refactor though, as I write it out, but that's effectively
what you're trying to do here. Your current case handles HSTS-masking-HPKP, but
doesn't handle HPKP-masking-HSTS (although less likely).

[palmer, 2014/01/03 22:43:44]

Done.

+                                 canonicalized_host.size() - i);
+      restored_static_state.domain = DNSDomainToString(host_sub_chunk);
+      if (HasPreload(kPreloadedSTS, kNumPreloadedSTS, canonicalized_host, i,
+                     &restored_static_state, &did_restore_static_state)) {
+        break;
+      }
+      if (HasPreload(kPreloadedSNISTS, kNumPreloadedSNISTS, canonicalized_host,
+                     i, &restored_static_state, &did_restore_static_state)) {

[Ryan Sleevi, 2013/12/27 00:21:06]

You're unconditionally checking against SNI hosts. This seems incorrect, because
you've lost the sni_enabled flag.

[palmer, 2014/01/03 22:43:44]

Handled this by getting rid of that code.

+        break;
+      }
+    }
+  }
+  if (did_restore_static_state) {
+    restored_bad_static_hashes = restored_static_state.bad_static_spki_hashes;
+    restored_static_hashes = restored_static_state.static_spki_hashes;
+  }
+
+  if (HashesIntersect(restored_bad_static_hashes, hashes) ||
+      HashesIntersect(bad_static_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(bad_static_spki_hashes) <<
+               ", " << HashesToBase64String(restored_bad_static_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_spki_hashes.empty() && static_spki_hashes.empty() &&
+      restored_static_hashes.empty()) {
     return true;
+  }
 
   if (HashesIntersect(dynamic_spki_hashes, hashes) ||
-      HashesIntersect(static_spki_hashes, hashes)) {
+      HashesIntersect(static_spki_hashes, hashes) ||
+      HashesIntersect(restored_static_hashes, hashes)) {
     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);
+             << " or: " << HashesToBase64String(static_spki_hashes)
+             << " or: " << HashesToBase64String(restored_static_hashes);
   return false;
 }