Calculate severity score for date_invalid error

chrome/browser/ssl/ssl_error_classification.cc

+// Copyright (c) 2012 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 "chrome/browser/ssl/ssl_error_classification.h"
+
+#include "base/build_time.h"
+#include "base/metrics/field_trial.h"
+#include "base/metrics/histogram.h"
+#include "base/time/time.h"
+#include "chrome/browser/browser_process.h"
+#include "components/network_time/network_time_tracker.h"
+#include "net/cert/x509_certificate.h"
+
+using base::Time;
+using base::TimeTicks;
+using base::TimeDelta;
+
+namespace {
+
+// Events for UMA. Do not reorder or change!
+enum SSLInterstitialCause {
+   CLOCK_PAST,
+     CLOCK_FUTURE,
+       UNUSED_INTERSTITIAL_CAUSE_ENTRY,
+       };

[felt, 2014/07/09 19:00:45]

^ woah crazy formatting here :)

[radhikabhar, 2014/07/10 17:14:47]

Done.

+
+void RecordSSLInterstitialCause(bool overridable, SSLInterstitialCause event) {
+  if (overridable) {
+    UMA_HISTOGRAM_ENUMERATION("interstitial.ssl.cause.overridable", event,
+                              UNUSED_INTERSTITIAL_CAUSE_ENTRY);
+  } else {
+    UMA_HISTOGRAM_ENUMERATION("interstitial.ssl.cause.nonoverridable", event,
+                              UNUSED_INTERSTITIAL_CAUSE_ENTRY);
+  }
+}
+
+} // namespace
+
+SSLErrorClassification::SSLErrorClassification(
+    base::Time current_time,
+    net::X509Certificate* cert)
+  : current_time_(current_time),
+  cert_(cert) { }

[felt, 2014/07/09 19:00:44]

formatting

[radhikabhar, 2014/07/10 17:14:47]

Done.

+
+SSLErrorClassification::~SSLErrorClassification() { }
+
+float SSLErrorClassification::ServerCharacteristics(){

[felt, 2014/07/09 19:00:45]

missing space:
ServerCharacteristics() {

[radhikabhar, 2014/07/10 17:14:47]

Done.

+  if (cert_->HasExpired()) {

[felt, 2014/07/09 19:00:44]

you don't need to use { } for single-line if-statements

[radhikabhar, 2014/07/10 17:14:47]

Done.

+    return 0.3 * TimePassedSinceExpiry();
+  }
+  if (current_time_ < cert_->valid_start())
+    return 0.2;
+  return 0.0;
+}
+
+float SSLErrorClassification::ClientCharacteristics() {
+  float severity_client_score = 0.0;
+  if (IsUserClockInThePast(true, current_time_)  ||
+      IsUserClockInTheFuture(true, current_time_)) {
+    severity_client_score = 0.75 * 0.1;
+  } else {
+    severity_client_score = 0.75 * 1;
+  }
+  //TODO(radhikabhar): Check website settings.

[felt, 2014/07/09 19:00:45]

need a space:
// TODO

[radhikabhar, 2014/07/10 17:14:47]

Done.

+  return severity_client_score;
+}
+
+float SSLErrorClassification::TimePassedSinceExpiry() {

[felt, 2014/07/09 19:00:45]

It doesn't make sense for a method named TimePassedSinceExpiry() to return a
float that is a score (and not an amount of time).

A) Create a new method named
SSLErrorClassification::CalculateScoreTimePassedSinceExpiry, which will be used
only for scoring in a single way.
B) Keep the method name ::TimePassedSinceExpiry() and have it return a
base::TimeDelta, so it can be reused in multiple places. And then the caller is
responsible for doing the scoring. This would allow the time calculation to be
reused in several places.

[radhikabhar, 2014/07/10 17:14:47]

Done.

+  base::TimeDelta delta = current_time_ - cert_->valid_expiry();
+  int64 time_passed = delta.InDays();
+  const int kHighThreshold = 7;
+  const int kLowThreshold = 4;
+  if (time_passed >= kHighThreshold)
+    return 0.4;
+  else if (time_passed >= kLowThreshold)
+    return 0.3;
+  else
+    return 0.2;
+}
+
+bool SSLErrorClassification::IsUserClockInThePast(bool overridable,
+                                                  base::Time time_now) {
+  base::Time network_time;
+  base::TimeDelta uncertainty;
+  base::Time build_time = base::GetBuildTime();
+  if (!g_browser_process->network_time_tracker()->GetNetworkTime(
+      base::TimeTicks(), &network_time, &uncertainty)) {
+    // When network time has not been initialized yet, simply rely on the
+    // machine's build time.

[felt, 2014/07/09 19:00:44]

you ought to check both network time and build time. network time could be
wrong, even if it's initialized.

if (time_now < network_time || time_now < build_time) return true;

then you wouldn't need the (network_time < build_time) check, I don't think? nor
would you need a network_time > build_time check (which I don't see here)

[radhikabhar, 2014/07/10 17:14:47]

I have deleted this because according to rsleevi@ comment, you cannot use the
network_time_tracker for security purposes. 

Details are here -
https://code.google.com/p/chromium/issues/detail?id=392662

On 2014/07/09 19:00:44, felt wrote:

+    network_time = build_time;
+  }
+  if (network_time < build_time)
+    network_time = build_time;
+  if (time_now < network_time - base::TimeDelta::FromDays(2) ||
+      time_now < build_time - base::TimeDelta::FromDays(2)) {
+    RecordSSLInterstitialCause(overridable, CLOCK_PAST);
+    return true;
+  }
+  return false;
+}
+
+bool SSLErrorClassification::IsUserClockInTheFuture(bool overridable,
+                                                    base::Time time_now) {
+  base::Time network_time;
+  base::TimeDelta uncertainty;
+  base::Time build_time = base::GetBuildTime();
+  if (!g_browser_process->network_time_tracker()->GetNetworkTime(
+      base::TimeTicks(), &network_time, &uncertainty)) {
+    // When network time has not been initialized yet, simply rely on the
+    // machine's build time.
+    network_time = build_time;
+  }

[felt, 2014/07/09 19:00:44]

same comment as above for checking both build_time and network_time

[radhikabhar, 2014/07/10 17:14:47]

Same as above.
On 2014/07/09 19:00:44, felt wrote:

+  if (network_time < build_time)
+    network_time = build_time;
+  if (network_time == build_time) {
+    // The build time will probably be weeks before the current date.
+    if (time_now > network_time - base::TimeDelta::FromDays(365)) {
+      RecordSSLInterstitialCause(overridable, CLOCK_FUTURE);
+      return true;
+    }
+  } else if (time_now > network_time) {
+    // Network time is more accurate.
+    RecordSSLInterstitialCause(overridable, CLOCK_FUTURE);
+    return true;
+  }
+  return false;
+}
+