Certificate Transparency: Correct month calculation.

net/cert/cert_policy_enforcer.cc

 // Copyright 2014 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/cert/cert_policy_enforcer.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/build_time.h"
@@ skipping 25 matching lines @@
 bool IsBuildTimely() {
 #if defined(DONT_EMBED_BUILD_METADATA) && !defined(OFFICIAL_BUILD)
   return true;
 #else
   const base::Time build_time = base::GetBuildTime();
   // We consider built-in information to be timely for 10 weeks.
   return (base::Time::Now() - build_time).InDays() < 70 /* 10 weeks */;
 #endif
 }
 
-uint32_t ApproximateMonthDifference(const base::Time& start,
-                                    const base::Time& end) {
+struct MonthsDiff {
+  MonthsDiff() : num_months(0), last_month_is_full(false) {}
+  size_t num_months;
+  bool last_month_is_full;
+};

[davidben, 2015/03/26 15:05:06]

This struct should go away now.

[Eran Messeri, 2015/03/26 22:58:52]

Oops, 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.
+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,
                              const ct::CTVerifyResult& ct_result) {
   // TODO(eranm): Count the number of *independent* SCTs once the information
   // about log operators is available, crbug.com/425174
   size_t num_valid_scts = ct_result.verified_scts.size();
   size_t num_embedded_scts =
       std::count_if(ct_result.verified_scts.begin(),
                     ct_result.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;
   }
 
-  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)) {

[davidben, 2015/03/26 15:05:06]

Nit: parens around (lifetime > 39) are unnecessary. Ditto for line 115.

[Eran Messeri, 2015/03/26 22:58:52]

Done.

     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;
   }
 
   return num_embedded_scts >= num_required_embedded_scts;
 }
 
 enum CTComplianceStatus {
   CT_NOT_COMPLIANT = 0,
@@ skipping 160 matching lines @@
 
   LogCTComplianceStatusToUMA(details.status, ev_whitelist);
 
   if (details.status == CT_IN_WHITELIST || details.status == CT_ENOUGH_SCTS)
     return true;
 
   return false;
 }
 
 }  // namespace net