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|?

[estark, 2016/03/11 22:00:05]

Can you please file a bug at include the URL (https://crbug.com/XXXXXX) here?

[mab, 2016/03/11 23:12:50]

Before I do that, let's ask felt, since she's on this review now.  (Or did you
specifically want to not change this behavior as part of this CL?)

[estark, 2016/03/11 23:15:23]

Yeah, that's fine with me.

+      switch (GetClockState(base::Time::NowFromSystemTime(), network_time)) {
+        case CLOCK_STATE_NETWORK_PAST:
+        case CLOCK_STATE_BUILD_PAST:
+          RecordSSLInterstitialCause(overridable, CLOCK_PAST);
+          break;
+        case CLOCK_STATE_NETWORK_FUTURE:
+        case CLOCK_STATE_BUILD_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_NETWORK_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) {
+  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) {

[estark, 2016/03/11 22:00:05]

nit: other code is this file doesn't use curly braces for single-line bodies so
I'd remove these.

[mab, 2016/03/11 23:12:50]

I kinda think this is how "goto fail" happened, but done.

[estark, 2016/03/11 23:15:23]

I know, I hate it too. But, when in Rome...

+      return CLOCK_STATE_NETWORK_PAST;
+    }
+    if (now_system > now_network + uncertainty + kNetworkTimeFudge) {

[estark, 2016/03/11 22:00:05]

same nit about the curly braces

[mab, 2016/03/11 23:12:50]

Done.

+      return CLOCK_STATE_NETWORK_FUTURE;
+    }
+    return CLOCK_STATE_NETWORK_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)) {

[estark, 2016/03/11 22:00:05]

ditto

[mab, 2016/03/11 23:12:50]

Done.

+    return CLOCK_STATE_BUILD_PAST;
+  }
+  if (now_system > build_time + base::TimeDelta::FromDays(365)) {

[estark, 2016/03/11 22:00:06]

ditto

[mab, 2016/03/11 23:12:50]

Done.

+    return CLOCK_STATE_BUILD_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) {
   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