Certificate Transparency: Correct month calculation.

net/cert/cert_policy_enforcer.cc

Index: net/cert/cert_policy_enforcer.cc
diff --git a/net/cert/cert_policy_enforcer.cc b/net/cert/cert_policy_enforcer.cc
index 680c2c6d1d792cdf4ef9bf6d9177222cb240e3b5..68a9d1024e4a6e0675b4d19af5ebf3c90e95ec86 100644
--- a/net/cert/cert_policy_enforcer.cc
+++ b/net/cert/cert_policy_enforcer.cc
@@ -43,20 +43,38 @@ bool IsBuildTimely() {
 #endif
 }
 
-uint32_t ApproximateMonthDifference(const base::Time& start,
-                                    const base::Time& end) {
+struct MonthsDiff {
+  MonthsDiff() : num_months(0), last_month_is_full(false) {}

[davidben, 2015/03/25 19:10:11]

Per https://chromium-cpp.appspot.com/, these can be inline initializers. So just

struct MonthsDiff {
  size_t num_months = 0;
  bool last_month_is_full = false;
};

[Ryan Sleevi, 2015/03/25 19:16:07]

That said, please just use two out variables; don't encapsulate this in a
struct.

(Yes, I'm generally anti-tuple)

[Eran Messeri, 2015/03/26 10:47:17]

Done - as Ryan suggested.

+  size_t num_months;
+  bool last_month_is_full;

[davidben, 2015/03/25 19:10:11]

last_month_is_full to me reads like you're rounding up and then reporting
whether the last month in num_months was full or partial.

Whereas you're rounding down and then reporting whether or not there was
*additional* time after adding num_months. Perhaps has_extra or
has_partial_month? (Note: that would invert the meaning of the boolean.)

[Eran Messeri, 2015/03/26 10:47:17]

Renamed to has_partial_month as suggested.

+};
+
+/*

[davidben, 2015/03/25 19:10:11]

Use // C++-style comments rather than /* C-style */.

[Eran Messeri, 2015/03/26 10:47:17]

Done.

+ * 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.
+ */
+MonthsDiff RoundedDownMonthDifference(const base::Time& start,
+                                      const base::Time& end) {
   base::Time::Exploded exploded_start;
   base::Time::Exploded exploded_expiry;
   start.UTCExplode(&exploded_start);
   end.UTCExplode(&exploded_expiry);
+  if (end < start)
+    return MonthsDiff();

[davidben, 2015/03/25 19:10:11]

Not that it matters, but I think this actually wants to return (0, true) by your
current meaning, not (0, false). Though see comment above; I think inverting the
meaning of the boolean makes it clearer anyway.

[Eran Messeri, 2015/03/26 10:47:17]

Done.

+
+  MonthsDiff ret;
+  ret.last_month_is_full = false;
   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)
+    ret.last_month_is_full = true;

[davidben, 2015/03/25 19:10:11]

It occurs to me this also doesn't account for hours and seconds and such. I
don't know how much we care since that's kind of tedious. (Both the < and the ==
comparisons would need to be modified. That or you unexplode the time, but then
you run into problems of what the unexplode function does if you pass in
February 31st.)

[Eran Messeri, 2015/03/26 10:47:17]

No benefit to being that tedious, it'd be extra code for no good reason. The
whole point behind this policy is to guarantee that long-lived certificates have
enough SCTs, nitpicking on the time of day will not matter much.

 
-  // Add any remainder as a full month.
-  if (exploded_expiry.day_of_month > exploded_start.day_of_month)
-    ++month_diff;
-
-  return month_diff;
+  ret.num_months = month_diff;
+  return ret;
 }
 
 bool HasRequiredNumberOfSCTs(const X509Certificate& cert,
@@ -81,18 +99,20 @@ bool HasRequiredNumberOfSCTs(const X509Certificate& cert,
     return false;
   }
 
-  uint32_t expiry_in_months_approx =
-      ApproximateMonthDifference(cert.valid_start(), cert.valid_expiry());
+  MonthsDiff exp =

[davidben, 2015/03/25 19:10:11]

exp -> lifetime? I first read that as exponentiation and was very confused. :-)
It's also not really the expiration, right? It's the total lifetime of the
certificate.

[Eran Messeri, 2015/03/26 10:47:17]

Done - rename to lifetime. You're right, it's not the expiration.

+      RoundedDownMonthDifference(cert.valid_start(), cert.valid_expiry());
 
   // 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 ((exp.num_months > 39) ||
+      (exp.num_months == 39 && !exp.last_month_is_full)) {
     num_required_embedded_scts = 5;
-  } else if (expiry_in_months_approx > 27) {
+  } else if ((exp.num_months > 27) ||
+             (exp.num_months == 27 && !exp.last_month_is_full)) {
     num_required_embedded_scts = 4;
-  } else if (expiry_in_months_approx >= 15) {
+  } else if (exp.num_months >= 15) {
     num_required_embedded_scts = 3;
   } else {
     num_required_embedded_scts = 2;