Add Expect CT policy that gets checked on all certs

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 d9c92421bf86e0ba4d7d101e07268769ad1dd137..ec253ab3abcce768f7d2b4884d935b134e659b91 100644
--- a/net/cert/ct_policy_enforcer.cc
+++ b/net/cert/ct_policy_enforcer.cc
@@ -134,29 +134,25 @@ bool HasEnoughDiverseSCTs(const ct::SCTList& verified_scts) {
          (verified_scts.size() != num_google_issued_scts);
 }
 
-enum CTComplianceStatus {
-  CT_NOT_COMPLIANT = 0,
-  CT_IN_WHITELIST = 1,
-  CT_ENOUGH_SCTS = 2,
-  CT_NOT_ENOUGH_DIVERSE_SCTS = 3,
-  CT_COMPLIANCE_MAX,
+enum EVPolicyStatus {
+  EV_POLICY_STATUS_NOT_COMPLIANT = 0,
+  EV_POLICY_STATUS_IN_WHITELIST = 1,
+  EV_POLICY_STATUS_COMPLIANT = 2,
+  EV_POLICY_STATUS_MAX,
 };
 
-const char* ComplianceStatusToString(CTComplianceStatus status) {
+const char* EVPolicyStatusToString(EVPolicyStatus status) {
   switch (status) {
-    case CT_NOT_COMPLIANT:
+    case EV_POLICY_STATUS_NOT_COMPLIANT:
       return "NOT_COMPLIANT";
       break;
-    case CT_IN_WHITELIST:
+    case EV_POLICY_STATUS_IN_WHITELIST:
       return "WHITELISTED";
       break;
-    case CT_ENOUGH_SCTS:
-      return "ENOUGH_SCTS";
+    case EV_POLICY_STATUS_COMPLIANT:
+      return "COMPLIANT";
       break;
-    case CT_NOT_ENOUGH_DIVERSE_SCTS:
-      return "NOT_ENOUGH_DIVERSE_SCTS";
-      break;
-    case CT_COMPLIANCE_MAX:
+    case EV_POLICY_STATUS_MAX:
       break;
   }
 
@@ -170,11 +166,11 @@ enum EVWhitelistStatus {
   EV_WHITELIST_MAX,
 };
 
-void LogCTComplianceStatusToUMA(CTComplianceStatus status,
-                                const ct::EVCertsWhitelist* ev_whitelist) {
-  UMA_HISTOGRAM_ENUMERATION("Net.SSL_EVCertificateCTCompliance", status,
-                            CT_COMPLIANCE_MAX);
-  if (status == CT_NOT_COMPLIANT) {
+void LogEVPolicyStatusToUMA(EVPolicyStatus status,
+                            const ct::EVCertsWhitelist* ev_whitelist) {
+  UMA_HISTOGRAM_ENUMERATION("Net.SSL_EVCTPolicyStatus", status,
+                            EV_POLICY_STATUS_MAX);
+  if (status == EV_POLICY_STATUS_NOT_COMPLIANT) {
     EVWhitelistStatus ev_whitelist_status = EV_WHITELIST_NOT_PRESENT;
     if (ev_whitelist != NULL) {
       if (ev_whitelist->IsValid())
@@ -188,41 +184,32 @@ void LogCTComplianceStatusToUMA(CTComplianceStatus status,
   }
 }
 
-struct ComplianceDetails {
-  ComplianceDetails()
-      : ct_presence_required(false),
-        build_timely(false),
-        status(CT_NOT_COMPLIANT) {}
+struct EVComplianceDetails {
+  EVComplianceDetails()
+      : build_timely(false), status(EV_POLICY_STATUS_NOT_COMPLIANT) {}
 
-  // Whether enforcement of the policy was required or not.
-  bool ct_presence_required;
-  // Whether the build is not older than 10 weeks. The value is meaningful only
-  // if |ct_presence_required| is true.
+  // True if the build is not older than 10 weeks.
   bool build_timely;
-  // Compliance status - meaningful only if |ct_presence_required| and
-  // |build_timely| are true.
-  CTComplianceStatus status;
-  // EV whitelist version.
+  // Compliance status. Cannot be EV_POLICY_STATUS_IS_WHITELISTED if
+  // |build_timely| is false.
+  EVPolicyStatus status;
+  // EV whitelist version. Only set if |build_timely| is true.
   base::Version whitelist_version;
 };
 
 scoped_ptr<base::Value> NetLogComplianceCheckResultCallback(
     X509Certificate* cert,
-    ComplianceDetails* details,
+    EVComplianceDetails* details,
     NetLogCaptureMode capture_mode) {
   scoped_ptr<base::DictionaryValue> dict(new base::DictionaryValue());
   dict->Set("certificate", NetLogX509CertificateCallback(cert, capture_mode));
-  dict->SetBoolean("policy_enforcement_required",
-                   details->ct_presence_required);
-  if (details->ct_presence_required) {
-    dict->SetBoolean("build_timely", details->build_timely);
-    if (details->build_timely) {
-      dict->SetString("ct_compliance_status",
-                      ComplianceStatusToString(details->status));
-      if (details->whitelist_version.IsValid())
-        dict->SetString("ev_whitelist_version",
-                        details->whitelist_version.GetString());
-    }
+  dict->SetBoolean("policy_enforcement_required", true);
+  dict->SetBoolean("build_timely", details->build_timely);
+  dict->SetString("ct_compliance_status",
+                  EVPolicyStatusToString(details->status));
+  if (details->whitelist_version.IsValid()) {
+    dict->SetString("ev_whitelist_version",
+                    details->whitelist_version.GetString());
   }
   return std::move(dict);
 }
@@ -261,49 +248,56 @@ bool IsCertificateInWhitelist(const X509Certificate& cert,
   return cert_in_ev_whitelist;
 }
 
-void CheckCTEVPolicyCompliance(X509Certificate* cert,
-                               const ct::EVCertsWhitelist* ev_whitelist,
-                               const ct::CTVerifyResult& ct_result,
-                               ComplianceDetails* result) {
-  result->ct_presence_required = true;
+bool CheckCertPolicyCompliance(X509Certificate* cert,
+                               const ct::CTVerifyResult& ct_result) {
+  if (!HasRequiredNumberOfSCTs(*cert, ct_result))
+    return false;
+
+  if (AllSCTsPastDistinctSCTRequirementEnforcementDate(
+          ct_result.verified_scts) &&
+      !HasEnoughDiverseSCTs(ct_result.verified_scts)) {
+    return false;
+  }
+
+  return true;
+}
 
+void CheckEVPolicyCompliance(X509Certificate* cert,
+                             CertStatus cert_status,
+                             const ct::EVCertsWhitelist* ev_whitelist,
+                             EVComplianceDetails* result) {
+  result->status = EV_POLICY_STATUS_NOT_COMPLIANT;
+  if ((cert_status & CERT_STATUS_CT_COMPLIANCE_FAILED) == 0)
+    result->status = EV_POLICY_STATUS_COMPLIANT;
   if (!IsBuildTimely())

[Ryan Sleevi, 2016/01/22 23:49:41]

See remarks above about why this is the wrong place.

[estark, 2016/01/23 01:38:41]

Acknowledged.

     return;

[Ryan Sleevi, 2016/01/22 23:49:41]

So the explanation for the timeliness check is not related to the whitelist, but
with respect to recognized logs.

The goal being is that if a client is so old that it doesn't have fresh log
information (e.g. in the event we've pulled a log for getting compromised and
misissuing SCTs), that clients don't trust it.

We could still use the whitelist, at this point, but instead we just disable CT
entirely.

I believe this check SHOULD be included in CheckCertPolicyCompliance, NOT
CheckEVPolicyCompliance, at least based on that logic.

If we want to change that policy, or adjust, I think that's a
reasonable-but-separate conversation we should have, but for now, we should keep
the present behaviour for all CT enforcement.

[estark, 2016/01/23 01:38:41]

Ah, I see. Though, to keep the logic exactly the same as it is now (untimely
build => not CT compliant, no whitelist check), I think we actually need the
build-timelinesss check in both CheckCertPolicyCompliance() and in
CheckEVPolicyCompliance(). Let me know if that looks correct to you. (If we only
have the build-timely check in CheckCertPolicyCompliance(), then by the time we
get to CheckEVPolicyCompliance(), the cert looks non-compliant and we go to the
whitelist same as if the server hadn't provided enough SCTs.)

I do have some questions/thoughts about how this build-timeliness logic should
work with expect CT, but we can discuss on a future CL.

   result->build_timely = true;
-
   if (ev_whitelist && ev_whitelist->IsValid())
     result->whitelist_version = ev_whitelist->Version();
 
-  if (IsCertificateInWhitelist(*cert, ev_whitelist)) {
-    result->status = CT_IN_WHITELIST;
-    return;
-  }
-
-  if (!HasRequiredNumberOfSCTs(*cert, ct_result)) {
-    result->status = CT_NOT_COMPLIANT;
-    return;
-  }
-
-  if (AllSCTsPastDistinctSCTRequirementEnforcementDate(
-          ct_result.verified_scts) &&
-      !HasEnoughDiverseSCTs(ct_result.verified_scts)) {
-    result->status = CT_NOT_ENOUGH_DIVERSE_SCTS;
-    return;
+  if (result->status != EV_POLICY_STATUS_COMPLIANT &&
+      IsCertificateInWhitelist(*cert, ev_whitelist)) {
+    result->status = EV_POLICY_STATUS_IN_WHITELIST;
   }
-
-  result->status = CT_ENOUGH_SCTS;
 }
 
 }  // namespace
 
-bool CTPolicyEnforcer::DoesConformToCTEVPolicy(
+bool CTPolicyEnforcer::DoesConformToCertPolicy(
     X509Certificate* cert,
+    const ct::CTVerifyResult& ct_result) {
+  return CheckCertPolicyCompliance(cert, ct_result);
+}
+
+bool CTPolicyEnforcer::DoesConformToEVPolicy(
+    X509Certificate* cert,
+    CertStatus cert_status,
     const ct::EVCertsWhitelist* ev_whitelist,
-    const ct::CTVerifyResult& ct_result,
     const BoundNetLog& net_log) {
-  ComplianceDetails details;
+  DCHECK_NE((cert_status & CERT_STATUS_IS_EV), 0u);
 
-  CheckCTEVPolicyCompliance(cert, ev_whitelist, ct_result, &details);
+  EVComplianceDetails details;
+  CheckEVPolicyCompliance(cert, cert_status, ev_whitelist, &details);
 
   NetLog::ParametersCallback net_log_callback =
       base::Bind(&NetLogComplianceCheckResultCallback, base::Unretained(cert),
@@ -312,18 +306,12 @@ bool CTPolicyEnforcer::DoesConformToCTEVPolicy(
   net_log.AddEvent(NetLog::TYPE_EV_CERT_CT_COMPLIANCE_CHECKED,
                    net_log_callback);
 
-  if (!details.ct_presence_required)
-    return true;
-
-  if (!details.build_timely)
-    return false;
-
-  LogCTComplianceStatusToUMA(details.status, ev_whitelist);
+  LogEVPolicyStatusToUMA(details.status, ev_whitelist);
 
-  if (details.status == CT_IN_WHITELIST || details.status == CT_ENOUGH_SCTS)
-    return true;
+  if (details.status == EV_POLICY_STATUS_IN_WHITELIST)
+    return details.build_timely;
 
-  return false;
+  return (details.status == EV_POLICY_STATUS_COMPLIANT);
 }
 
 }  // namespace net