Introduce the ability to require CT for specific hosts

net/socket/ssl_client_socket_impl.cc

Index: net/socket/ssl_client_socket_impl.cc
diff --git a/net/socket/ssl_client_socket_impl.cc b/net/socket/ssl_client_socket_impl.cc
index 60a8fae446bc38d88b85149bbbb7bdeb07299eef..b843d4eb8cee54b28c0213e34034dc29b5338a29 100644
--- a/net/socket/ssl_client_socket_impl.cc
+++ b/net/socket/ssl_client_socket_impl.cc
@@ -1327,27 +1327,31 @@ int SSLClientSocketImpl::DoVerifyCertComplete(int result) {
     }
   }
 
+  // If the connection was good, check HPKP and CT status simultaneously,
+  // but prefer to treat the HPKP error as more serious, if there was one.
   const CertStatus cert_status = server_cert_verify_result_.cert_status;
   if ((result == OK ||
-       (IsCertificateError(result) && IsCertStatusMinorError(cert_status))) &&
-      !transport_security_state_->CheckPublicKeyPins(
-          host_and_port_, server_cert_verify_result_.is_issued_by_known_root,
-          server_cert_verify_result_.public_key_hashes, server_cert_.get(),
-          server_cert_verify_result_.verified_cert.get(),
-          TransportSecurityState::ENABLE_PIN_REPORTS, &pinning_failure_log_)) {
-    if (server_cert_verify_result_.is_issued_by_known_root) {
-      server_cert_verify_result_.cert_status |= CERT_STATUS_PINNED_KEY_MISSING;
-      result = ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN;
-    } else {
-      pkp_bypassed_ = true;
+       (IsCertificateError(result) && IsCertStatusMinorError(cert_status)))) {
+    int ct_result = VerifyCT();
+    if (!transport_security_state_->CheckPublicKeyPins(
+            host_and_port_, server_cert_verify_result_.is_issued_by_known_root,
+            server_cert_verify_result_.public_key_hashes, server_cert_.get(),
+            server_cert_verify_result_.verified_cert.get(),
+            TransportSecurityState::ENABLE_PIN_REPORTS,
+            &pinning_failure_log_)) {
+      if (server_cert_verify_result_.is_issued_by_known_root) {
+        server_cert_verify_result_.cert_status |=
+            CERT_STATUS_PINNED_KEY_MISSING;
+        result = ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN;
+      } else {
+        pkp_bypassed_ = true;
+      }
     }
+    if (result != ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN && ct_result != OK)
+      result = ct_result;

[davidben, 2016/06/22 21:44:59]

SSLClientSocket-level test for all this? Particularly the part where HPKP
continues to be treated as more serious. (Maybe also on the QUIC side since the
logic is, sadly, currently duplicated. Though if we un-duplicate it...)

[Ryan Sleevi, 2016/06/22 22:07:39]

Sure

   }
 
   if (result == OK) {
-    // Only check Certificate Transparency if there were no other errors with
-    // the connection.
-    VerifyCT();
-
     DCHECK(!certificate_verified_);
     certificate_verified_ = true;
     MaybeCacheSession();
@@ -1377,58 +1381,6 @@ void SSLClientSocketImpl::UpdateServerCert() {
   }
 }
 
-void SSLClientSocketImpl::VerifyCT() {
-  const uint8_t* ocsp_response_raw;
-  size_t ocsp_response_len;
-  SSL_get0_ocsp_response(ssl_, &ocsp_response_raw, &ocsp_response_len);
-  std::string ocsp_response;
-  if (ocsp_response_len > 0) {
-    ocsp_response.assign(reinterpret_cast<const char*>(ocsp_response_raw),
-                         ocsp_response_len);
-  }
-
-  const uint8_t* sct_list_raw;
-  size_t sct_list_len;
-  SSL_get0_signed_cert_timestamp_list(ssl_, &sct_list_raw, &sct_list_len);
-  std::string sct_list;
-  if (sct_list_len > 0)
-    sct_list.assign(reinterpret_cast<const char*>(sct_list_raw), sct_list_len);
-
-  // Note that this is a completely synchronous operation: The CT Log Verifier
-  // gets all the data it needs for SCT verification and does not do any
-  // external communication.
-  cert_transparency_verifier_->Verify(
-      server_cert_verify_result_.verified_cert.get(), ocsp_response, sct_list,
-      &ct_verify_result_, net_log_);
-
-  ct_verify_result_.ct_policies_applied = true;
-  ct_verify_result_.ev_policy_compliance =
-      ct::EVPolicyCompliance::EV_POLICY_DOES_NOT_APPLY;
-  if (server_cert_verify_result_.cert_status & CERT_STATUS_IS_EV) {
-    scoped_refptr<ct::EVCertsWhitelist> ev_whitelist =
-        SSLConfigService::GetEVCertsWhitelist();
-    ct::EVPolicyCompliance ev_policy_compliance =
-        policy_enforcer_->DoesConformToCTEVPolicy(
-            server_cert_verify_result_.verified_cert.get(), ev_whitelist.get(),
-            ct_verify_result_.verified_scts, net_log_);
-    ct_verify_result_.ev_policy_compliance = ev_policy_compliance;
-    if (ev_policy_compliance !=
-            ct::EVPolicyCompliance::EV_POLICY_DOES_NOT_APPLY &&
-        ev_policy_compliance !=
-            ct::EVPolicyCompliance::EV_POLICY_COMPLIES_VIA_WHITELIST &&
-        ev_policy_compliance !=
-            ct::EVPolicyCompliance::EV_POLICY_COMPLIES_VIA_SCTS) {
-      server_cert_verify_result_.cert_status |=
-          CERT_STATUS_CT_COMPLIANCE_FAILED;
-      server_cert_verify_result_.cert_status &= ~CERT_STATUS_IS_EV;
-    }
-  }
-  ct_verify_result_.cert_policy_compliance =
-      policy_enforcer_->DoesConformToCertPolicy(
-          server_cert_verify_result_.verified_cert.get(),
-          ct_verify_result_.verified_scts, net_log_);
-}
-
 void SSLClientSocketImpl::OnHandshakeIOComplete(int result) {
   int rv = DoHandshakeLoop(result);
   if (rv != ERR_IO_PENDING) {
@@ -1817,6 +1769,70 @@ int SSLClientSocketImpl::TransportReadComplete(int result) {
   return result;
 }
 
+int SSLClientSocketImpl::VerifyCT() {
+  const uint8_t* ocsp_response_raw;
+  size_t ocsp_response_len;
+  SSL_get0_ocsp_response(ssl_, &ocsp_response_raw, &ocsp_response_len);
+  std::string ocsp_response;
+  if (ocsp_response_len > 0) {
+    ocsp_response.assign(reinterpret_cast<const char*>(ocsp_response_raw),
+                         ocsp_response_len);
+  }
+
+  const uint8_t* sct_list_raw;
+  size_t sct_list_len;
+  SSL_get0_signed_cert_timestamp_list(ssl_, &sct_list_raw, &sct_list_len);
+  std::string sct_list;
+  if (sct_list_len > 0)
+    sct_list.assign(reinterpret_cast<const char*>(sct_list_raw), sct_list_len);
+
+  // Note that this is a completely synchronous operation: The CT Log Verifier
+  // gets all the data it needs for SCT verification and does not do any
+  // external communication.
+  cert_transparency_verifier_->Verify(
+      server_cert_verify_result_.verified_cert.get(), ocsp_response, sct_list,
+      &ct_verify_result_, net_log_);
+
+  ct_verify_result_.ct_policies_applied = true;
+  ct_verify_result_.ev_policy_compliance =
+      ct::EVPolicyCompliance::EV_POLICY_DOES_NOT_APPLY;
+  if (server_cert_verify_result_.cert_status & CERT_STATUS_IS_EV) {
+    scoped_refptr<ct::EVCertsWhitelist> ev_whitelist =
+        SSLConfigService::GetEVCertsWhitelist();
+    ct::EVPolicyCompliance ev_policy_compliance =
+        policy_enforcer_->DoesConformToCTEVPolicy(
+            server_cert_verify_result_.verified_cert.get(), ev_whitelist.get(),
+            ct_verify_result_.verified_scts, net_log_);
+    ct_verify_result_.ev_policy_compliance = ev_policy_compliance;
+    if (ev_policy_compliance !=
+            ct::EVPolicyCompliance::EV_POLICY_DOES_NOT_APPLY &&
+        ev_policy_compliance !=
+            ct::EVPolicyCompliance::EV_POLICY_COMPLIES_VIA_WHITELIST &&
+        ev_policy_compliance !=
+            ct::EVPolicyCompliance::EV_POLICY_COMPLIES_VIA_SCTS) {
+      server_cert_verify_result_.cert_status |=
+          CERT_STATUS_CT_COMPLIANCE_FAILED;
+      server_cert_verify_result_.cert_status &= ~CERT_STATUS_IS_EV;
+    }
+  }
+  ct_verify_result_.cert_policy_compliance =
+      policy_enforcer_->DoesConformToCertPolicy(
+          server_cert_verify_result_.verified_cert.get(),
+          ct_verify_result_.verified_scts, net_log_);
+
+  if (ct_verify_result_.cert_policy_compliance !=
+          ct::CertPolicyCompliance::CERT_POLICY_COMPLIES_VIA_SCTS &&

[davidben, 2016/06/22 21:44:59]

To confirm: it is intentional that CERT_POLICY_COMPLIES_VIA_WHITELIST is not
sufficient here.

[Ryan Sleevi, 2016/06/22 22:07:39]

There is no CERT_POLICY_COMPLIES_VIA_WHITELIST - there's only a whitelist for
the EV policy, not the base cert policy. That's part of the CTPolicyEnforcer /
CTPolicyStatus documentation.

+      transport_security_state_->ShouldRequireCT(
+          host_and_port_.host(), server_cert_verify_result_.verified_cert.get(),
+          server_cert_verify_result_.public_key_hashes)) {
+    server_cert_verify_result_.cert_status |=
+        CERT_STATUS_CERTIFICATE_TRANSPARENCY_REQUIRED;
+    return ERR_CERTIFICATE_TRANSPARENCY_REQUIRED;
+  }
+
+  return OK;
+}
+
 int SSLClientSocketImpl::ClientCertRequestCallback(SSL* ssl) {
   DCHECK(ssl == ssl_);