Centralize the logic for checking public key 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 0b209b356e834dc7018bb0d9a743711b3e02cff5..b18087974a26672efa418cffdb643006376f526a 100644
--- a/net/http/transport_security_state.cc
+++ b/net/http/transport_security_state.cc
@@ -84,7 +84,13 @@ bool AddHash(const char* sha1_hash,
 }  // namespace
 
 TransportSecurityState::TransportSecurityState()
-  : delegate_(NULL) {
+    : delegate_(NULL),
+      enable_static_pinning_(true) {
+  // Static pinning is only enabled for official builds to make sure that
+  // others don't end up with pins that cannot be easily updated.
+#if !defined(OFFICIAL_BUILD) || defined(OS_ANDROID) || defined(OS_IOS)
+  enable_static_pinning_ = false;
+#endif
   DCHECK(CalledOnValidThread());
 }
 
@@ -118,20 +124,53 @@ bool TransportSecurityState::ShouldUpgradeToSSL(const std::string& host,
   return false;
 }
 
-bool TransportSecurityState::CheckPublicKeyPins(const std::string& host,
-                                                bool sni_enabled,
-                                                const HashValueVector& hashes,
-                                                std::string* failure_log) {
+bool TransportSecurityState::CheckPublicKeyPins(
+    const std::string& host,
+    bool sni_available,
+    bool is_issued_by_known_root,
+    const HashValueVector& public_key_hashes,
+    std::string* pinning_failure_log) {
+  // Perform pin validation if, and only if, all these conditions obtain:
+  //
+  // * 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 ||
+      !IsBuildTimely() ||
+      !HasPublicKeyPins(host, sni_available)) {
+    return true;

[Ryan Sleevi, 2014/08/07 23:31:19]

Re-git-cl-format this?

[Ryan Hamilton, 2014/08/08 00:54:00]

Done.

+  }
+
+  bool pins_are_valid = CheckPublicKeyPinsImpl(host,
+                                               sni_available,
+                                               public_key_hashes,
+                                               pinning_failure_log);
+  if (!pins_are_valid) {
+    LOG(ERROR) << *pinning_failure_log;
+    ReportUMAOnPinFailure(host);
+  }
+
+  UMA_HISTOGRAM_BOOLEAN("Net.PublicKeyPinSuccess", pins_are_valid);
+  return pins_are_valid;
+}
+
+bool TransportSecurityState::CheckPublicKeyPinsImpl(
+    const std::string& host,
+    bool sni_enabled,
+    const HashValueVector& hashes,
+    std::string* failure_log) {
   DomainState dynamic_state;
   if (GetDynamicDomainState(host, &dynamic_state))
     return dynamic_state.CheckPublicKeyPins(hashes, failure_log);
 
   DomainState static_state;
-  if (GetStaticDomainState(host, sni_enabled, &static_state) &&
-      static_state.CheckPublicKeyPins(hashes, failure_log)) {
-      return true;
-  }
+  if (GetStaticDomainState(host, sni_enabled, &static_state))
+    return static_state.CheckPublicKeyPins(hashes, failure_log);
 
+  // HasPublicKeyPins should have returned true in order for this method
+  // to have been called, so if we fall through to here, it's an error.
   return false;
 }
 
@@ -768,6 +807,9 @@ bool TransportSecurityState::GetStaticDomainState(const std::string& host,
                                                   DomainState* out) const {
   DCHECK(CalledOnValidThread());
 
+  if (!enable_static_pinning_)
+    return false;

[Ryan Sleevi, 2014/08/07 23:31:19]

Ugh and crap.

This breaks HSTS. And we didn't have any tests that caught it, presumably, which
is double-plus-ungood. Or it just worked because of flipping the bool.

I think it just means we move the enable_static_pinning_ check into HasPreload,
and don't set the required hashes / excluded hashes (lines 611 onward, which
modify out->pkp). With out->pkp empty, you should fall through to line 921,
which allows any chain to be valid.

[Ryan Hamilton, 2014/08/08 00:54:00]

Done. (I hope :>)

+
   const std::string canonicalized_host = CanonicalizeHost(host);
 
   out->sts.upgrade_mode = DomainState::MODE_FORCE_HTTPS;