Use network time for bad clock interstitial.

components/ssl_errors/error_classification.cc

 // Copyright 2015 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 "components/ssl_errors/error_classification.h"
 
 #include <limits.h>
 #include <stddef.h>
 
 #include <vector>
 
 #include "base/build_time.h"
 #include "base/lazy_instance.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_split.h"
 #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"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "net/base/url_util.h"
 #include "net/cert/x509_cert_types.h"
 #include "net/cert/x509_certificate.h"
 #include "url/gurl.h"
 
 #if defined(OS_WIN)
@@ skipping 83 matching lines @@
   return GetWWWSubDomainMatch(request_url, dns_names, &www_host);
 }
 
 // The time to use when doing build time operations in browser tests.
 base::LazyInstance<base::Time> g_testing_build_time = LAZY_INSTANCE_INITIALIZER;
 
 }  // namespace
 
 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) {
   ssl_errors::ErrorInfo::ErrorType type =
       ssl_errors::ErrorInfo::NetErrorToErrorType(cert_error);
   UMA_HISTOGRAM_ENUMERATION("interstitial.ssl_error_type", type,
                             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();
       if (IsHostNameKnownTLD(host_name)) {
         HostnameTokens host_name_tokens = Tokenize(host_name);
         if (IsWWWSubDomainMatch(request_url, cert))
           RecordSSLInterstitialCause(overridable, WWW_SUBDOMAIN_MATCH);
         if (IsSubDomainOutsideWildcard(request_url, cert))
           RecordSSLInterstitialCause(overridable, SUBDOMAIN_OUTSIDE_WILDCARD);
         std::vector<std::string> dns_names;
@@ skipping 24 matching lines @@
       break;
     }
     default:
       break;
   }
   UMA_HISTOGRAM_ENUMERATION("interstitial.ssl.connection_type",
                             net::NetworkChangeNotifier::GetConnectionType(),
                             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;
-}
+  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;
 
-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();
-  }
-
-  if (time_now > build_time + base::TimeDelta::FromDays(365))
-    return true;
-  return false;
+  return CLOCK_STATE_UNKNOWN;
 }
 
 void SetBuildTimeForTesting(const base::Time& testing_time) {
   g_testing_build_time.Get() = testing_time;
 }
 
 bool IsHostNameKnownTLD(const std::string& host_name) {
   size_t tld_length = net::registry_controlled_domains::GetRegistryLength(
       host_name, net::registry_controlled_domains::EXCLUDE_UNKNOWN_REGISTRIES,
       net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES);
@@ skipping 194 matching lines @@
   return std::find(dns_names_domain.begin(), dns_names_domain.end() - 1,
                    host_name_domain) != dns_names_domain.end() - 1;
 }
 
 bool IsHostnameNonUniqueOrDotless(const std::string& hostname) {
   return net::IsHostnameNonUnique(hostname) ||
          hostname.find('.') == std::string::npos;
 }
 
 }  // namespace ssl_errors