Align the CT Implementation with Policy

net/cert/ct_policy_enforcer.cc

Index: net/cert/ct_policy_enforcer.cc
diff --git a/net/cert/ct_policy_enforcer.cc b/net/cert/ct_policy_enforcer.cc
index 48ebbc2b2a29b9dc6f447d0c309f8af41d6588ab..5d76a0d7915abd984eba0594385006fa08ffaec9 100644
--- a/net/cert/ct_policy_enforcer.cc
+++ b/net/cert/ct_policy_enforcer.cc
@@ -31,10 +31,6 @@ namespace net {
 
 namespace {
 
-bool IsEmbeddedSCT(const scoped_refptr<ct::SignedCertificateTimestamp>& sct) {
-  return sct->origin == ct::SignedCertificateTimestamp::SCT_EMBEDDED;
-}
-
 // Returns true if the current build is recent enough to ensure that
 // built-in security information (e.g. CT Logs) is fresh enough.
 // TODO(eranm): Move to base or net/base
@@ -44,11 +40,6 @@ bool IsBuildTimely() {
   return (base::Time::Now() - build_time).InDays() < 70 /* 10 weeks */;
 }
 
-bool IsGoogleIssuedSCT(
-    const scoped_refptr<ct::SignedCertificateTimestamp>& sct) {
-  return ct::IsLogOperatedByGoogle(sct->log_id);
-}
-
 // Returns a rounded-down months difference of |start| and |end|,
 // together with an indication of whether the last month was
 // a full month, because the range starts specified in the policy
@@ -79,58 +70,6 @@ void RoundedDownMonthDifference(const base::Time& start,
   *rounded_months_difference = month_diff;
 }
 
-bool HasRequiredNumberOfSCTs(const X509Certificate& cert,
-                             const ct::SCTList& verified_scts) {
-  size_t num_valid_scts = verified_scts.size();
-  size_t num_embedded_scts = base::checked_cast<size_t>(
-      std::count_if(verified_scts.begin(), verified_scts.end(), IsEmbeddedSCT));
-
-  size_t num_non_embedded_scts = num_valid_scts - num_embedded_scts;
-  // If at least two valid SCTs were delivered by means other than embedding
-  // (i.e. in a TLS extension or OCSP), then the certificate conforms to bullet
-  // number 3 of the "Qualifying Certificate" section of the CT/EV policy.
-  if (num_non_embedded_scts >= 2)
-    return true;
-
-  if (cert.valid_start().is_null() || cert.valid_expiry().is_null() ||
-      cert.valid_start().is_max() || cert.valid_expiry().is_max()) {
-    // Will not be able to calculate the certificate's validity period.
-    return false;
-  }
-
-  size_t lifetime;
-  bool has_partial_month;
-  RoundedDownMonthDifference(cert.valid_start(), cert.valid_expiry(), &lifetime,
-                             &has_partial_month);
-
-  // For embedded SCTs, if the certificate has the number of SCTs specified in
-  // table 1 of the "Qualifying Certificate" section of the CT/EV policy, then
-  // it qualifies.
-  size_t num_required_embedded_scts;
-  if (lifetime > 39 || (lifetime == 39 && has_partial_month)) {
-    num_required_embedded_scts = 5;
-  } else if (lifetime > 27 || (lifetime == 27 && has_partial_month)) {
-    num_required_embedded_scts = 4;
-  } else if (lifetime >= 15) {
-    num_required_embedded_scts = 3;
-  } else {
-    num_required_embedded_scts = 2;
-  }
-
-  return num_embedded_scts >= num_required_embedded_scts;
-}
-
-// Returns true if |verified_scts| contains SCTs from at least one log that is
-// operated by Google and at least one log that is not operated by Google. This
-// is required for SCTs after July 1st, 2015, as documented at
-// http://dev.chromium.org/Home/chromium-security/root-ca-policy/EVCTPlanMay2015edition.pdf
-bool HasEnoughDiverseSCTs(const ct::SCTList& verified_scts) {
-  size_t num_google_issued_scts = base::checked_cast<size_t>(std::count_if(
-      verified_scts.begin(), verified_scts.end(), IsGoogleIssuedSCT));
-  return (num_google_issued_scts > 0) &&
-         (verified_scts.size() != num_google_issued_scts);
-}
-
 const char* EVPolicyComplianceToString(ct::EVPolicyCompliance status) {
   switch (status) {
     case ct::EVPolicyCompliance::EV_POLICY_DOES_NOT_APPLY:
@@ -238,52 +177,165 @@ std::unique_ptr<base::Value> NetLogCertComplianceCheckResultCallback(
   return std::move(dict);
 }
 
-// Returns true if all SCTs in |verified_scts| were issued on, or after, the
-// date specified in kDiverseSCTRequirementStartDate
-bool AllSCTsPastDistinctSCTRequirementEnforcementDate(
-    const ct::SCTList& verified_scts) {
-  // The date when diverse SCTs requirement is effective from.
-  // 2015-07-01 00:00:00 UTC.
-  base::Time kDiverseSCTRequirementStartDate =
-      base::Time::FromInternalValue(13080182400000000);
-
-  for (const auto& it : verified_scts) {
-    if (it->timestamp < kDiverseSCTRequirementStartDate)
-      return false;
-  }
-
-  return true;
-}
-
 bool IsCertificateInWhitelist(const X509Certificate& cert,
                               const ct::EVCertsWhitelist* ev_whitelist) {
-  bool cert_in_ev_whitelist = false;
-  if (ev_whitelist && ev_whitelist->IsValid()) {
-    const SHA256HashValue fingerprint(
-        X509Certificate::CalculateFingerprint256(cert.os_cert_handle()));
+  if (!ev_whitelist || !ev_whitelist->IsValid())
+    return false;

[Ryan Sleevi, 2016/05/02 23:41:23]

This was just a style cleanup for better short-circuits.

 
-    std::string truncated_fp =
-        std::string(reinterpret_cast<const char*>(fingerprint.data), 8);
-    cert_in_ev_whitelist = ev_whitelist->ContainsCertificateHash(truncated_fp);
+  const SHA256HashValue fingerprint(
+      X509Certificate::CalculateFingerprint256(cert.os_cert_handle()));
 
-    UMA_HISTOGRAM_BOOLEAN("Net.SSL_EVCertificateInWhitelist",
-                          cert_in_ev_whitelist);
-  }
+  std::string truncated_fp =
+      std::string(reinterpret_cast<const char*>(fingerprint.data), 8);
+  bool cert_in_ev_whitelist =
+      ev_whitelist->ContainsCertificateHash(truncated_fp);
+
+  UMA_HISTOGRAM_BOOLEAN("Net.SSL_EVCertificateInWhitelist",
+                        cert_in_ev_whitelist);
   return cert_in_ev_whitelist;
 }
 
 ct::CertPolicyCompliance CheckCertPolicyCompliance(
-    X509Certificate* cert,
-    const ct::SCTList& verified_scts,
-    const BoundNetLog& net_log) {
-  if (!HasRequiredNumberOfSCTs(*cert, verified_scts))
+    const X509Certificate& cert,
+    const ct::SCTList& verified_scts) {
+  base::Time issuance_date = base::Time::Max();
+
+  // Cert is outside the bounds of parsable; reject it.
+  if (cert.valid_start().is_null() || cert.valid_expiry().is_null() ||
+      cert.valid_start().is_max() || cert.valid_expiry().is_max()) {
     return ct::CertPolicyCompliance::CERT_POLICY_NOT_ENOUGH_SCTS;
-  if (AllSCTsPastDistinctSCTRequirementEnforcementDate(verified_scts) &&
-      !HasEnoughDiverseSCTs(verified_scts)) {
+  }

[Ryan Sleevi, 2016/05/02 23:41:23]

This is a functional change unspecified in policy, but considering such a cert
will break everything anyways, there's no harm in the early short-circuit.

[Eran Messeri, 2016/05/03 12:52:13]

Nit: in that case issuance_date can be defined and initialized later on.

+
+  // Scan for the earliest SCT. This is used to determine whether to enforce
+  // log diversity requirements, as well as whether to enforce whether or not
+  // a log was qualified or pending qualification at time of issuance (in the
+  // case of embedded SCTs). It's acceptable to ignore the origin of the SCT,
+  // because SCTs delivered via OCSP/TLS extension will cover the full
+  // certificate, which necessarily will exist only after the precertificate
+  // has been logged and the actual certificate issued.
+  // Note: Here, issuance date is defined as the earliest of all SCTs, rather
+  // than the latest of embedded SCTs, in order to give CAs the benefit of
+  // the doubt in the event a log is revoked in the midst of processing
+  // a precertificate and issuing the certificate.
+  for (const auto& sct : verified_scts)
+    issuance_date = std::min(sct->timestamp, issuance_date);

[Ryan Sleevi, 2016/05/02 23:41:23]

This is intentionally pre-computed, because it will be used to check the
qualified/non-qualified status in a follow-up CL.

+
+  bool has_valid_google_sct = false;
+  bool has_valid_nongoogle_sct = false;
+  bool has_valid_embedded_sct = false;
+  bool has_valid_nonembedded_sct = false;
+  bool has_embedded_google_sct = false;
+  bool has_embedded_nongoogle_sct = false;
+  std::vector<base::StringPiece> embedded_log_ids;
+  for (const auto& sct : verified_scts) {
+    if (ct::IsLogOperatedByGoogle(sct->log_id)) {
+      has_valid_google_sct = true;
+      if (sct->origin == ct::SignedCertificateTimestamp::SCT_EMBEDDED)
+        has_embedded_google_sct = true;
+    } else {
+      has_valid_nongoogle_sct = true;
+      if (sct->origin == ct::SignedCertificateTimestamp::SCT_EMBEDDED)
+        has_embedded_nongoogle_sct = true;
+    }
+    if (sct->origin != ct::SignedCertificateTimestamp::SCT_EMBEDDED) {
+      has_valid_nonembedded_sct = true;
+    } else {
+      has_valid_embedded_sct = true;
+      embedded_log_ids.push_back(sct->log_id);
+    }
+  }
+
+  // Option 1:
+  // An SCT presented via the TLS extension OR embedded within a stapled OCSP
+  //   response is from a log qualified at time of check;
+  // AND there is at least one SCT from a Google Log that is qualified at
+  //   time of check, presented via any method;
+  // AND there is at least one SCT from a non-Google Log that is qualified
+  //   at the time of check, presented via any method.
+  //
+  // Note: Because SCTs embedded via TLS or OCSP can be updated on the fly,
+  // the issuance date is irrelevant, as any policy changes can be
+  // accomodated.
+  if (has_valid_nonembedded_sct && has_valid_google_sct &&
+      has_valid_nongoogle_sct) {
+    return ct::CertPolicyCompliance::CERT_POLICY_COMPLIES_VIA_SCTS;
+  }
+  // Note: If has_valid_nonembedded_sct was true, but Option 2 isn't met,
+  // then the result will be that there weren't diverse enough SCTs, as that
+  // the only other way for the conditional above to fail). Because Option 1
+  // has the diversity requirement, it's implicitly a minimum number of SCTs
+  // (specifically, 2), but that's not explicitly specified in the policy.
+
+  // Option 2:
+  // There is at least one embedded SCT from a log qualified at the time of
+  //   check ...
+  if (!has_valid_embedded_sct) {
+    // Under Option 2, there weren't enough SCTs, and potentially under
+    // Option 1, there weren't diverse enough SCTs. Try to signal the error
+    // that is most easily fixed.
+    return has_valid_nonembedded_sct
+               ? ct::CertPolicyCompliance::CERT_POLICY_NOT_DIVERSE_SCTS
+               : ct::CertPolicyCompliance::CERT_POLICY_NOT_ENOUGH_SCTS;
+  }
+
+  // ... AND there is at least one embedded SCT from a Google Log once or
+  //   currently qualified;
+  // AND there is at least one embedded SCT from a non-Google Log once or
+  //   currently qualified;
+  // ...
+  //
+  // Note: This policy language is only enforced after the below issuance
+  // date, as that's when the diversity policy first came into effect for
+  // SCTs embedded in certificates.
+  // The date when diverse SCTs requirement is effective from.
+  // 2015-07-01 00:00:00 UTC.
+  const base::Time kDiverseSCTRequirementStartDate =
+      base::Time::FromInternalValue(13080182400000000);
+  if (issuance_date >= kDiverseSCTRequirementStartDate &&
+      !(has_embedded_google_sct && has_embedded_nongoogle_sct)) {
+    // Note: This also covers the case for embedded SCTs, as it's only

[estark, 2016/05/03 18:15:03]

This comment doesn't make sense to me. Is "embedded" supposed to be
"non-embedded" by any chance?

[Ryan Sleevi, 2016/05/03 18:31:03]

Yup, thanks.

+    // possible to reach here if both sets are not diverse enough.
     return ct::CertPolicyCompliance::CERT_POLICY_NOT_DIVERSE_SCTS;
   }
 
-  return ct::CertPolicyCompliance::CERT_POLICY_COMPLIES_VIA_SCTS;
+  size_t lifetime_in_months = 0;
+  bool has_partial_month = false;
+  RoundedDownMonthDifference(cert.valid_start(), cert.valid_expiry(),
+                             &lifetime_in_months, &has_partial_month);
+
+  // ... AND the certificate embeds SCTs from AT LEAST the number of logs
+  //   once or currently qualified shown in Table 1 of the CT Policy.
+  size_t num_required_embedded_scts = 5;
+  if (lifetime_in_months > 39 ||
+      (lifetime_in_months == 39 && has_partial_month)) {
+    num_required_embedded_scts = 5;
+  } else if (lifetime_in_months > 27 ||
+             (lifetime_in_months == 27 && has_partial_month)) {
+    num_required_embedded_scts = 4;
+  } else if (lifetime_in_months >= 15) {
+    num_required_embedded_scts = 3;
+  } else {
+    num_required_embedded_scts = 2;
+  }
+
+  // Sort the embedded log IDs and remove duplicates, so that only a single
+  // SCT from each log is accepted. This is to handle the case where a given
+  // log returns different SCTs for the same precertificate (which is
+  // permitted, but advised against).
+  std::sort(embedded_log_ids.begin(), embedded_log_ids.end());
+  size_t num_embedded_scts = std::distance(
+      embedded_log_ids.begin(),

[Eran Messeri, 2016/05/03 12:52:13]

Question: Is the begin() iterator always valid after the std::unique invocation?

Seems to me it would be if std::unique is always guaranteed not to move the
first item it a sequence of identical items.

[Ryan Sleevi, 2016/05/03 18:31:03]

Nope, that's a bug :)

[Ryan Sleevi, 2016/05/03 18:32:29]

I guess I should clarify - for std::vector it's defined because this just swaps
positions, but for safety/clarity of code, I opted to rely on the
non-invalidation of iterator behaviour.

+      std::unique(embedded_log_ids.begin(), embedded_log_ids.end()));
+
+  if (num_embedded_scts >= num_required_embedded_scts)
+    return ct::CertPolicyCompliance::CERT_POLICY_COMPLIES_VIA_SCTS;
+
+  // Under Option 2, there weren't enough SCTs, and potentially under Option
+  // 1, there weren't diverse enough SCTs. Try to signal the error that is
+  // most easily fixed.
+  return has_valid_nonembedded_sct
+             ? ct::CertPolicyCompliance::CERT_POLICY_NOT_DIVERSE_SCTS
+             : ct::CertPolicyCompliance::CERT_POLICY_NOT_ENOUGH_SCTS;
 }
 
 ct::EVPolicyCompliance CertPolicyComplianceToEVPolicyCompliance(
@@ -307,7 +359,7 @@ void CheckCTEVPolicyCompliance(X509Certificate* cert,
                                const BoundNetLog& net_log,
                                EVComplianceDetails* result) {
   result->status = CertPolicyComplianceToEVPolicyCompliance(
-      CheckCertPolicyCompliance(cert, verified_scts, net_log));
+      CheckCertPolicyCompliance(*cert, verified_scts));
   if (ev_whitelist && ev_whitelist->IsValid())
     result->whitelist_version = ev_whitelist->Version();
 
@@ -333,7 +385,7 @@ ct::CertPolicyCompliance CTPolicyEnforcer::DoesConformToCertPolicy(
   if (!build_timely) {
     compliance = ct::CertPolicyCompliance::CERT_POLICY_BUILD_NOT_TIMELY;
   } else {
-    compliance = CheckCertPolicyCompliance(cert, verified_scts, net_log);
+    compliance = CheckCertPolicyCompliance(*cert, verified_scts);
   }
 
   NetLog::ParametersCallback net_log_callback =