Use network time for bad clock interstitial.

components/ssl_errors/error_classification.cc

Index: components/ssl_errors/error_classification.cc
diff --git a/components/ssl_errors/error_classification.cc b/components/ssl_errors/error_classification.cc
index 7ab390bc62adc8d5e8918e574f77f7654a99c4bf..76c43ac870a22873355c7c65a41ec060bebd7069 100644
--- a/components/ssl_errors/error_classification.cc
+++ b/components/ssl_errors/error_classification.cc
@@ -16,6 +16,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
+#include "components/network_time/network_time_tracker.h"
 #include "components/ssl_errors/error_info.h"
 #include "components/url_formatter/url_formatter.h"
 #include "net/base/network_change_notifier.h"
@@ -119,6 +120,7 @@ base::LazyInstance<base::Time> g_testing_build_time = LAZY_INSTANCE_INITIALIZER;
 
 void RecordUMAStatistics(bool overridable,
                          const base::Time& current_time,
+                         const network_time::NetworkTimeTracker* network_time,
                          const GURL& request_url,
                          int cert_error,
                          const net::X509Certificate& cert) {
@@ -128,15 +130,27 @@ void RecordUMAStatistics(bool overridable,
                             ssl_errors::ErrorInfo::END_OF_ENUM);
   switch (type) {
     case ssl_errors::ErrorInfo::CERT_DATE_INVALID: {
-      if (IsUserClockInThePast(base::Time::NowFromSystemTime())) {
-        RecordSSLInterstitialCause(overridable, CLOCK_PAST);
-      } else if (IsUserClockInTheFuture(base::Time::NowFromSystemTime())) {
-        RecordSSLInterstitialCause(overridable, CLOCK_FUTURE);
-      } else if (cert.HasExpired() &&
-                 (current_time - cert.valid_expiry()).InDays() < 28) {
-        RecordSSLInterstitialCause(overridable, EXPIRED_RECENTLY);
+      // TODO(mab): Why doesn't this just use |current_time|?

[felt, 2016/03/11 23:23:45]

I'm pretty sure that was a refactoring accident. This should use |current_time|.

[mab, 2016/03/11 23:34:20]

Done.

+      switch (GetClockState(base::Time::NowFromSystemTime(), network_time)) {
+        case CLOCK_STATE_PAST:
+          RecordSSLInterstitialCause(overridable, CLOCK_PAST);
+          break;
+        case CLOCK_STATE_FUTURE:
+          RecordSSLInterstitialCause(overridable, CLOCK_FUTURE);
+          break;
+        case CLOCK_STATE_UNKNOWN:
+        // Fall through, but, would it be better to break here?  Not
+        // sure it makes sense to record |EXPIRED_RECENTLY| in this
+        // case.  UNKNOWN means that network time is unavailable and
+        // that the system clock is within a 367-day bound around
+        // the build time.  That's a lot of slop.
+        case CLOCK_STATE_OK:
+          if (cert.HasExpired() &&
+              (current_time - cert.valid_expiry()).InDays() < 28) {
+            RecordSSLInterstitialCause(overridable, EXPIRED_RECENTLY);
+          }
+          break;
       }
-      break;
     }
     case ssl_errors::ErrorInfo::CERT_COMMON_NAME_INVALID: {
       std::string host_name = request_url.host();
@@ -181,30 +195,29 @@ void RecordUMAStatistics(bool overridable,
                             net::NetworkChangeNotifier::CONNECTION_LAST);
 }
 
-bool IsUserClockInThePast(const base::Time& time_now) {
-  base::Time build_time;
-  if (!g_testing_build_time.Get().is_null()) {
-    build_time = g_testing_build_time.Get();
-  } else {
-    build_time = base::GetBuildTime();
+ClockState GetClockState(
+    const base::Time& now_system,
+    const network_time::NetworkTimeTracker* network_time_tracker) {

[felt, 2016/03/11 23:23:45]

I was wondering why you're passing around the NTT instead of just the time. It's
because you also need the uncertainty estimate? What is the range for the
uncertainty estimate?

[mab, 2016/03/11 23:34:20]

It might be O(minutes) in extreme cases.  I like being able to use it in the
calculation, but I don't think it's critical.  Also, if we have two base::Time
arguments (or 3, if we pass in the build time as well) that improves the odds of
getting them confused.

These aren't strong reasons.  Happy to pass in the time if you prefer it.

+  base::Time now_network;
+  base::TimeDelta uncertainty;
+  const base::TimeDelta kNetworkTimeFudge = base::TimeDelta::FromMinutes(5);
+  if (network_time_tracker->GetNetworkTime(&now_network, &uncertainty)) {
+    if (now_system < now_network - uncertainty - kNetworkTimeFudge)
+      return CLOCK_STATE_PAST;
+    if (now_system > now_network + uncertainty + kNetworkTimeFudge)
+      return CLOCK_STATE_FUTURE;
+    return CLOCK_STATE_OK;
   }
 
-  if (time_now < build_time - base::TimeDelta::FromDays(2))
-    return true;
-  return false;
-}
-
-bool IsUserClockInTheFuture(const base::Time& time_now) {
-  base::Time build_time;
-  if (!g_testing_build_time.Get().is_null()) {
-    build_time = g_testing_build_time.Get();
-  } else {
-    build_time = base::GetBuildTime();
-  }
+  base::Time build_time = g_testing_build_time.Get().is_null()
+                              ? base::GetBuildTime()
+                              : g_testing_build_time.Get();
+  if (now_system < build_time - base::TimeDelta::FromDays(2))
+    return CLOCK_STATE_PAST;
+  if (now_system > build_time + base::TimeDelta::FromDays(365))
+    return CLOCK_STATE_FUTURE;
 
-  if (time_now > build_time + base::TimeDelta::FromDays(365))
-    return true;
-  return false;
+  return CLOCK_STATE_UNKNOWN;
 }
 
 void SetBuildTimeForTesting(const base::Time& testing_time) {