Check Expect-CT at connection setup

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"
 
 #include <algorithm>
 #include <memory>
 #include <utility>
 #include <vector>
@@ skipping 32 matching lines @@
 #include "net/http/transport_security_state_ct_policies.inc"
 #include "net/http/transport_security_state_static.h"
 
 const size_t kMaxHPKPReportCacheEntries = 50;
 const int kTimeToRememberHPKPReportsMins = 60;
 const size_t kReportCacheKeyLength = 16;
 
 // Points to the active transport security state source.
 const TransportSecurityStateSource* g_hsts_source = &kHSTSSource;
 
-// Override for ShouldRequireCT() for unit tests. Possible values:
+// Override for CheckCTRequirements() for unit tests. Possible values:
 //  -1: Unless a delegate says otherwise, do not require CT.
 //   0: Use the default implementation (e.g. production)
 //   1: Unless a delegate says otherwise, require CT.
 int g_ct_required_for_testing = 0;
 
 bool IsDynamicExpectCTEnabled() {
   return base::FeatureList::IsEnabled(
       TransportSecurityState::kDynamicExpectCTFeature);
 }
 
@@ skipping 789 matching lines @@
   STSState unused;
   PKPState static_pkp_state;
   if (GetStaticDomainState(host, &unused, &static_pkp_state)) {
     if (static_pkp_state.HasPublicKeyPins())
       return true;
   }
 
   return false;
 }
 
-bool TransportSecurityState::ShouldRequireCT(
-    const std::string& hostname,
+bool TransportSecurityState::CheckCTRequirements(

[mattm, 2017/05/04 01:57:11]

do you think it's worth returning an enum value so there's no confusion over
what true / false mean?

[estark, 2017/05/04 04:03:25]

Yes, I think that makes sense. Done.

+    const net::HostPortPair& host_port_pair,
+    bool is_issued_by_known_root,
+    const HashValueVector& public_key_hashes,
     const X509Certificate* validated_certificate_chain,
-    const HashValueVector& public_key_hashes) {
+    const X509Certificate* served_certificate_chain,
+    const SignedCertificateTimestampAndStatusList&
+        signed_certificate_timestamps,
+    const ExpectCTReportStatus report_status,
+    ct::CertPolicyCompliance cert_policy_compliance) {
   using CTRequirementLevel = RequireCTDelegate::CTRequirementLevel;
+  std::string hostname = host_port_pair.host();
+
+  // If the connection complies with CT policy, then no further checks are
+  // necessary.
+  if (cert_policy_compliance ==
+          ct::CertPolicyCompliance::CERT_POLICY_COMPLIES_VIA_SCTS ||
+      cert_policy_compliance ==
+          ct::CertPolicyCompliance::CERT_POLICY_BUILD_NOT_TIMELY) {
+    return true;
+  }
+
+  // Check Expect-CT first so that other CT requirements do not prevent
+  // Expect-CT reports from being sent.
+  ExpectCTState state;
+  if (is_issued_by_known_root && IsDynamicExpectCTEnabled() &&
+      GetDynamicExpectCTState(hostname, &state)) {
+    if (expect_ct_reporter_ && !state.report_uri.is_empty() &&
+        report_status == ENABLE_EXPECT_CT_REPORTS) {
+      expect_ct_reporter_->OnExpectCTFailed(
+          host_port_pair, state.report_uri, validated_certificate_chain,
+          served_certificate_chain, signed_certificate_timestamps);
+    }
+    if (state.enforce)
+      return false;
+  }
 
   CTRequirementLevel ct_required = CTRequirementLevel::DEFAULT;
   if (require_ct_delegate_)
     ct_required = require_ct_delegate_->IsCTRequiredForHost(hostname);
   if (ct_required != CTRequirementLevel::DEFAULT)
-    return ct_required == CTRequirementLevel::REQUIRED;
+    return ct_required != CTRequirementLevel::REQUIRED;
 
   // Allow unittests to override the default result.
   if (g_ct_required_for_testing)
     return g_ct_required_for_testing == 1;

[mattm, 2017/05/04 01:57:11]

should that be !=  now?

[estark, 2017/05/04 04:03:25]

Fixed with the new enum return value.

 
   // Until CT is required for all secure hosts on the Internet, this should
-  // remain false. It is provided to simplify the various short-circuit
+  // remain true. It is provided to simplify the various short-circuit
   // returns below.
-  bool default_response = false;
+  bool default_response = true;

[mattm, 2017/05/04 01:57:11]

since you're touching this anyway, make this const?

[estark, 2017/05/04 04:03:24]

Done.

 
 // FieldTrials are not supported in Native Client apps.
 #if !defined(OS_NACL)
   // Emergency escape valve; not to be activated until there's an actual
   // emergency (e.g. a weird path-building bug due to a CA's failed
   // disclosure of cross-signed sub-CAs).
   std::string group_name =
       base::FieldTrialList::FindFullName("EnforceCTForProblematicRoots");
   if (base::StartsWith(group_name, "disabled",
                        base::CompareCase::INSENSITIVE_ASCII)) {
@@ skipping 30 matching lines @@
         if (std::binary_search(
                 restricted_ca.exceptions,
                 restricted_ca.exceptions + restricted_ca.exceptions_length,
                 sub_ca_hash, SHA256ToHashValueComparator())) {
           // Found an excluded sub-CA; CT is not required.
           return default_response;
         }
       }
 
       // No exception found. This certificate must conform to the CT policy.
       return true;

[mattm, 2017/05/04 01:57:11]

should that be false now? (Is there a test for this?)

[estark, 2017/05/04 04:03:25]

Oops. There was a test; I just forgot to run it. Sorry about that. Fixed,
trybots are green now.

     }
   }
 
   return default_response;
 }
 
 void TransportSecurityState::SetDelegate(
     TransportSecurityState::Delegate* delegate) {
   DCHECK(CalledOnValidThread());
   delegate_ = delegate;
@@ skipping 462 matching lines @@
       return;
     if (!ssl_info.is_issued_by_known_root)
       return;
     if (!ssl_info.ct_compliance_details_available)
       return;
     if (ssl_info.ct_cert_policy_compliance ==
         ct::CertPolicyCompliance::CERT_POLICY_COMPLIES_VIA_SCTS)
       return;
     ExpectCTState state;
     if (GetStaticExpectCTState(host_port_pair.host(), &state)) {
-      expect_ct_reporter_->OnExpectCTFailed(host_port_pair, state.report_uri,
-                                            ssl_info);
+      expect_ct_reporter_->OnExpectCTFailed(
+          host_port_pair, state.report_uri, ssl_info.cert.get(),
+          ssl_info.unverified_cert.get(),
+          ssl_info.signed_certificate_timestamps);
     }
     return;
   }
 
   // Otherwise, see if the site has sent a valid Expect-CT header to dynamically
   // turn on reporting and/or enforcement.
   if (!IsDynamicExpectCTEnabled())
     return;
   base::Time now = base::Time::Now();
   base::TimeDelta max_age;
@@ skipping 12 matching lines @@
     ExpectCTState state;
     // If an Expect-CT header is observed over a non-compliant connection, the
     // site owner should be notified about the misconfiguration. If the site was
     // already opted in to Expect-CT, this report would have been sent at
     // connection setup time. If the host is not already a noted Expect-CT host,
     // however, the lack of CT compliance would not have been evaluated/reported
     // at connection setup time, so it needs to be reported here while
     // processing the header.
     if (expect_ct_reporter_ && !report_uri.is_empty() &&
         !GetDynamicExpectCTState(host_port_pair.host(), &state)) {
-      expect_ct_reporter_->OnExpectCTFailed(host_port_pair, report_uri,
-                                            ssl_info);
+      expect_ct_reporter_->OnExpectCTFailed(
+          host_port_pair, report_uri, ssl_info.cert.get(),
+          ssl_info.unverified_cert.get(),
+          ssl_info.signed_certificate_timestamps);
     }
     return;
   }
   AddExpectCTInternal(host_port_pair.host(), now, now + max_age, enforce,
                       report_uri);
 }
 
 // static
 void TransportSecurityState::SetShouldRequireCTForTesting(bool* required) {
   if (!required) {
@@ skipping 328 matching lines @@
 TransportSecurityState::PKPStateIterator::PKPStateIterator(
     const TransportSecurityState& state)
     : iterator_(state.enabled_pkp_hosts_.begin()),
       end_(state.enabled_pkp_hosts_.end()) {
 }
 
 TransportSecurityState::PKPStateIterator::~PKPStateIterator() {
 }
 
 }  // namespace