Centralize the logic for checking public key 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 648 matching lines @@
     if (max_age.InSeconds() == 0)
       domain_state.pkp.spki_hashes.clear();
     domain_state.pkp.last_observed = now;
     domain_state.pkp.expiry = now + max_age;
     EnableHost(host, domain_state);
     return true;
   }
   return false;
 }
 
+bool TransportSecurityState::VerifyPinning(
+    const HashValueVector& public_key_hashes,
+    bool is_issued_by_known_root,
+    bool sni_available,
+    const std::string& host,
+    std::string* pinning_failure_log) {
+#if defined(OFFICIAL_BUILD) && !defined(OS_ANDROID) && !defined(OS_IOS)

[agl, 2014/08/07 18:32:59]

Now that this is it's own function, I think this would be better as:

#if !defined(OFFICIAL_BUILD) || defined(OS_ANDROID) || defined(OS_IOS)
  return true;
#endif

[Ryan Hamilton, 2014/08/07 18:44:26]

Done. (Though I used "#else" instead of #endif because otherwise, I think we'll
get unreachable code errors, right?)

+  // Take care of any mandates for public key pinning.
+  //
+  // Pinning is only enabled for official builds to make sure that others don't
+  // end up with pins that cannot be easily updated.
+  //
+  // TODO(agl): We might have an issue here where a request for foo.example.com
+  // merges into a SPDY connection to www.example.com, and gets a different
+  // certificate.
+
+  // Perform pin validation if, and only if, all these conditions obtain:
+  //
+  // * a TransportSecurityState object is available;
+  // * the server's certificate chain is valid (or suffers from only a minor
+  //   error);
+  // * the server's certificate chain chains up to a known root (i.e. not a
+  //   user-installed trust anchor); and
+  // * the build is recent (very old builds should fail open so that users
+  //   have some chance to recover).
+  //
+  if (!is_issued_by_known_root || !TransportSecurityState::IsBuildTimely()) {
+    return true;
+  }

[agl, 2014/08/07 18:32:59]

if (!is_issued_by_known_root ||
    !TransportSecurityState::IsBuildTimely() ||
    !HasPublicKeyPins(host, sni_available)) {
  return true;
}

[Ryan Hamilton, 2014/08/07 18:44:26]

Done.

+
+  if (!HasPublicKeyPins(host, sni_available))
+    return true;
+
+  if (CheckPublicKeyPins(host,

[palmer, 2014/08/07 18:50:50]

Does it make sense to just move the logic in this function into
|CheckPublicKeyPins|? It seems odd to have both |VerifyPinning| (or
|ValidatePinning| and |CheckPublicKeyPins|.

[Ryan Hamilton, 2014/08/07 20:09:38]

Hm. I'm totally happy to do this and it seems like a sensible idea. However
there's one detail that I'm unclear how to best handle. As currently
implemented, my ValidatePinning method simply returns true if it's not an
official build. This complicates life for testing. 

Perhaps we could add an argument to the construct (or a setter) which controls
how this method behaves. Tests would enable the checks, but the code which
actually constructs the "real" TSS could disable it when not official?

[Ryan Sleevi, 2014/08/07 20:40:17]

How so / what tests? Our tests don't have OFFICIAL_BUILD set.
If this is a merge target, probably not.

I don't want you to be forced to fixing bugs, but as noted above, it's a BUG
that we #ifdef these out (really, we should only be #ifdef'ing out the static
pins)

However, that's a bigger (different) CL, and though it's totally where we need
to go, I'm not sure if it's worth blocking you on solving.

In case it's ambiguous what it'd be:
1) We'd drop the #ifdefs entirely
2) We'd drop the IsBuildTimely check from 695 (it's only applicable to static
pins, and GetStaticDomainState() already checks timeliness)
3) GetStaticDomainState() would have the #ifdef to disable it for OFFICIAL_BUILD

If we wanted to enable it for builds, it'd be something like
GetStaticDomainState(...)
  if (!StaticPinsEnabled())
    return false;
  logic that looks at kNumPreloaded, etc

bool StaticPinsEnabled(...)
  if (g_some_static_method_for_testing_was_called)
    return g_some_static_method_for_testing_was_called
#if defined(OFFICIAL_BUILD) && ...
  return false;
#else
  return true;
#endif

[Ryan Hamilton, 2014/08/07 22:07:11]

Discussed offline. I *think* I've done what you proposed.

+                         sni_available,
+                         public_key_hashes,
+                         pinning_failure_log)) {
+    UMA_HISTOGRAM_BOOLEAN("Net.PublicKeyPinSuccess", true);
+    return true;
+  }
+
+  LOG(ERROR) << *pinning_failure_log;
+  UMA_HISTOGRAM_BOOLEAN("Net.PublicKeyPinSuccess", false);
+  TransportSecurityState::ReportUMAOnPinFailure(host);
+#else
+  return true;
+#endif
+}
+
 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(
@@ skipping 222 matching lines @@
   return pkp.spki_hashes.size() > 0 || pkp.bad_spki_hashes.size() > 0;
 }
 
 TransportSecurityState::DomainState::PKPState::PKPState() {
 }
 
 TransportSecurityState::DomainState::PKPState::~PKPState() {
 }
 
 }  // namespace