Certificate Transparency: Correct month calculation.

The number of SCTs required for certificates with a validity period of over 14 months (but under 15) was incorrect. Fixed by not counting partial months.

BUG=470169

Review URL: https://codereview.chromium.org/1032093002

Cr-Commit-Position: refs/heads/master@{#322501}

net/cert/cert_policy_enforcer.cc

@@ -43,20 +43,34 @@ bool IsBuildTimely() {
 #endif
 }
 
-uint32_t ApproximateMonthDifference(const base::Time& start,
-                                    const base::Time& end) {
+// 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
+// are not consistent in terms of including the range start value.
+void RoundedDownMonthDifference(const base::Time& start,
+                                const base::Time& end,
+                                size_t* rounded_months_difference,
+                                bool* has_partial_month) {
+  DCHECK(rounded_months_difference);
+  DCHECK(has_partial_month);
   base::Time::Exploded exploded_start;
   base::Time::Exploded exploded_expiry;
   start.UTCExplode(&exploded_start);
   end.UTCExplode(&exploded_expiry);
+  if (end < start) {
+    *rounded_months_difference = 0;
+    *has_partial_month = false;
+  }
+
+  *has_partial_month = true;
   uint32_t month_diff = (exploded_expiry.year - exploded_start.year) * 12 +
                         (exploded_expiry.month - exploded_start.month);
+  if (exploded_expiry.day_of_month < exploded_start.day_of_month)
+    --month_diff;
+  else if (exploded_expiry.day_of_month == exploded_start.day_of_month)
+    *has_partial_month = false;
 
-  // Add any remainder as a full month.
-  if (exploded_expiry.day_of_month > exploded_start.day_of_month)
-    ++month_diff;
-
-  return month_diff;
+  *rounded_months_difference = month_diff;
 }
 
 bool HasRequiredNumberOfSCTs(const X509Certificate& cert,
@@ -81,18 +95,20 @@ bool HasRequiredNumberOfSCTs(const X509Certificate& cert,
     return false;
   }
 
-  uint32_t expiry_in_months_approx =
-      ApproximateMonthDifference(cert.valid_start(), cert.valid_expiry());
+  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 (expiry_in_months_approx > 39) {
+  if (lifetime > 39 || (lifetime == 39 && has_partial_month)) {
     num_required_embedded_scts = 5;
-  } else if (expiry_in_months_approx > 27) {
+  } else if (lifetime > 27 || (lifetime == 27 && has_partial_month)) {
     num_required_embedded_scts = 4;
-  } else if (expiry_in_months_approx >= 15) {
+  } else if (lifetime >= 15) {
     num_required_embedded_scts = 3;
   } else {
     num_required_embedded_scts = 2;

net/cert/cert_policy_enforcer_unittest.cc

@@ -67,6 +67,29 @@ class CertPolicyEnforcerTest : public ::testing::Test {
     }
   }
 
+  void CheckCertificateCompliesWithExactNumberOfEmbeddedSCTs(
+      const base::Time& start,
+      const base::Time& end,
+      size_t required_scts) {
+    scoped_refptr<X509Certificate> cert(
+        new X509Certificate("subject", "issuer", start, end));
+    ct::CTVerifyResult result;
+    for (size_t i = 0; i < required_scts - 1; ++i) {
+      FillResultWithSCTsOfOrigin(ct::SignedCertificateTimestamp::SCT_EMBEDDED,
+                                 1, &result);
+      EXPECT_FALSE(policy_enforcer_->DoesConformToCTEVPolicy(
+          cert.get(), nullptr, result, BoundNetLog()))
+          << " for: " << (end - start).InDays() << " and " << required_scts
+          << " scts=" << result.verified_scts.size() << " i=" << i;
+    }
+    FillResultWithSCTsOfOrigin(ct::SignedCertificateTimestamp::SCT_EMBEDDED, 1,
+                               &result);
+    EXPECT_TRUE(policy_enforcer_->DoesConformToCTEVPolicy(
+        cert.get(), nullptr, result, BoundNetLog()))
+        << " for: " << (end - start).InDays() << " and " << required_scts
+        << " scts=" << result.verified_scts.size();
+  }
+
  protected:
   scoped_ptr<CertPolicyEnforcer> policy_enforcer_;
   scoped_refptr<X509Certificate> chain_;
@@ -140,31 +163,45 @@ TEST_F(CertPolicyEnforcerTest, DoesNotConformToPolicyInvalidDates) {
 
 TEST_F(CertPolicyEnforcerTest,
        ConformsToPolicyExactNumberOfSCTsForValidityPeriod) {
-  // Test multiple validity periods: Over 27 months, Over 15 months (but less
-  // than 27 months),
-  // Less than 15 months.
-  const size_t validity_period[] = {12, 19, 30, 50};
-  const size_t needed_scts[] = {2, 3, 4, 5};
-
-  for (int i = 0; i < 3; ++i) {
-    size_t curr_validity = validity_period[i];
-    scoped_refptr<X509Certificate> cert(new X509Certificate(
-        "subject", "issuer", base::Time::Now(),
-        base::Time::Now() + base::TimeDelta::FromDays(31 * curr_validity)));
-    size_t curr_required_scts = needed_scts[i];
-    ct::CTVerifyResult result;
-    for (size_t j = 0; j < curr_required_scts - 1; ++j) {
-      FillResultWithSCTsOfOrigin(ct::SignedCertificateTimestamp::SCT_EMBEDDED,
-                                 1, &result);
-      EXPECT_FALSE(policy_enforcer_->DoesConformToCTEVPolicy(
-          cert.get(), nullptr, result, BoundNetLog()))
-          << " for: " << curr_validity << " and " << curr_required_scts
-          << " scts=" << result.verified_scts.size() << " j=" << j;
-    }
-    FillResultWithSCTsOfOrigin(ct::SignedCertificateTimestamp::SCT_EMBEDDED, 1,
-                               &result);
-    EXPECT_TRUE(policy_enforcer_->DoesConformToCTEVPolicy(
-        cert.get(), nullptr, result, BoundNetLog()));
+  // Test multiple validity periods
+  const struct TestData {
+    base::Time validity_start;
+    base::Time validity_end;
+    size_t scts_required;
+  } kTestData[] = {{// Cert valid for 14 months, needs 2 SCTs.
+                    base::Time::FromUTCExploded({2015, 3, 0, 25, 11, 25, 0, 0}),
+                    base::Time::FromUTCExploded({2016, 6, 0, 6, 11, 25, 0, 0}),
+                    2},
+                   {// Cert valid for exactly 15 months, needs 3 SCTs.
+                    base::Time::FromUTCExploded({2015, 3, 0, 25, 11, 25, 0, 0}),
+                    base::Time::FromUTCExploded({2016, 6, 0, 25, 11, 25, 0, 0}),
+                    3},
+                   {// Cert valid for over 15 months, needs 3 SCTs.
+                    base::Time::FromUTCExploded({2015, 3, 0, 25, 11, 25, 0, 0}),
+                    base::Time::FromUTCExploded({2016, 6, 0, 27, 11, 25, 0, 0}),
+                    3},
+                   {// Cert valid for exactly 27 months, needs 3 SCTs.
+                    base::Time::FromUTCExploded({2015, 3, 0, 25, 11, 25, 0, 0}),
+                    base::Time::FromUTCExploded({2017, 6, 0, 25, 11, 25, 0, 0}),
+                    3},
+                   {// Cert valid for over 27 months, needs 4 SCTs.
+                    base::Time::FromUTCExploded({2015, 3, 0, 25, 11, 25, 0, 0}),
+                    base::Time::FromUTCExploded({2017, 6, 0, 28, 11, 25, 0, 0}),
+                    4},
+                   {// Cert valid for exactly 39 months, needs 4 SCTs.
+                    base::Time::FromUTCExploded({2015, 3, 0, 25, 11, 25, 0, 0}),
+                    base::Time::FromUTCExploded({2018, 6, 0, 25, 11, 25, 0, 0}),
+                    4},
+                   {// Cert valid for over 39 months, needs 5 SCTs.
+                    base::Time::FromUTCExploded({2015, 3, 0, 25, 11, 25, 0, 0}),
+                    base::Time::FromUTCExploded({2018, 6, 0, 27, 11, 25, 0, 0}),
+                    5}};
+
+  for (size_t i = 0; i < arraysize(kTestData); ++i) {
+    SCOPED_TRACE(i);
+    CheckCertificateCompliesWithExactNumberOfEmbeddedSCTs(
+        kTestData[i].validity_start, kTestData[i].validity_end,
+        kTestData[i].scts_required);
   }
 }