Find reasons for the SSL common name invalid error.

chrome/browser/ssl/ssl_error_classification.cc

Index: chrome/browser/ssl/ssl_error_classification.cc
diff --git a/chrome/browser/ssl/ssl_error_classification.cc b/chrome/browser/ssl/ssl_error_classification.cc
index ae9283c190f73594e2622102a2336d99b318f4c2..1fda37ec8e91579ca0fcbab9fe78b3c3ea76e4b7 100644
--- a/chrome/browser/ssl/ssl_error_classification.cc
+++ b/chrome/browser/ssl/ssl_error_classification.cc
@@ -1,4 +1,4 @@
-// Copyright 2014 The Chromium Authors. All rights reserved.
+// Copyright (c) 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.
 
@@ -9,8 +9,9 @@
 #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_cert_types.h"
 #include "net/cert/x509_certificate.h"
+#include "url/gurl.h"
 
 using base::Time;
 using base::TimeTicks;
@@ -25,6 +26,10 @@ enum SSLInterstitialCause {
   UNUSED_INTERSTITIAL_CAUSE_ENTRY,
 };
 
+// Scores/weights which will be constant through all the SSL error types.

[felt, 2014/07/15 00:52:44]

As we discussed earlier today, I'm not sure whether this is a good idea moving
forwards. It seems like some checks (e.g., the clock) ought to weigh a lot more
than a fraction of 0.5. You don't necessarily need to change it here, but please
be thinking about it.

[radhikabhar, 2014/07/15 17:34:09]

Done.

+static const float kServerWeight = 0.5f;
+static const float kClientWeight = 0.5f;
+
 void RecordSSLInterstitialCause(bool overridable, SSLInterstitialCause event) {
   if (overridable) {
     UMA_HISTOGRAM_ENUMERATION("interstitial.ssl.cause.overridable", event,
@@ -35,12 +40,29 @@ void RecordSSLInterstitialCause(bool overridable, SSLInterstitialCause event) {
   }
 }
 
+// Utility function - If |str2| is of the form "bc" and |str1| is of the form
+// "abc" then this function returns "a" otherwise it returns an empty string.

[felt, 2014/07/15 00:52:44]

Is this supposed to be checking whether str2 = prefix + str1, and then returning
prefix? It so, it might make sense to name it explain it like:

// Utility function: Check to see whether |longer_str| is equivalent to
|smaller_str| plus a prefix. If so,
// return the prefix. If not, returns an empty string. For example: if
|longer_str| is "abc" and |shorter_str|
// is "bc", it will return "a".
std::string FindStringPrefix(std::string longer_str, std::string smaller_str) {

You don't have to use this exact wording or anything -- but I think the current
explanation is confusing and could be improved by explaining that you are
looking for a string prefix.

[radhikabhar, 2014/07/15 17:34:09]

Done.

+std::string GetStringDifference(std::string str1, std::string str2) {
+  // Str1 is longer than str2.
+  std::size_t found  = str1.find(str2);

[felt, 2014/07/15 00:52:44]

nit: a stray space before the = sign

[radhikabhar, 2014/07/15 17:34:09]

Done.

+  if (found != std::string::npos) {
+    std::string result_last = str1.substr(found);
+    if (result_last.compare(str2) != 0 )
+      return std::string();

[felt, 2014/07/15 00:52:44]

to make sure I understand correctly: the reason for the
result_last.compare(str2) != 0 comparison is to make sure that there are no
trailing strings? e.g., this will reject "abcd" and "bc" as a pair.

[radhikabhar, 2014/07/15 17:34:09]

Yes, because we don't want o be accepting foo.google.com.foo when we are
presented with a certificate for google.com

+    std::string result_first = str1.substr(0, found);
+      return result_first;

[felt, 2014/07/15 00:52:44]

why is this indented?

[radhikabhar, 2014/07/15 17:34:09]

Done.

+  }
+  return std::string();
+}
+
 } // namespace
 
 SSLErrorClassification::SSLErrorClassification(
     base::Time current_time,
+    const GURL& url,
     const net::X509Certificate& cert)
   : current_time_(current_time),
+    request_url_(url),
     cert_(cert) { }
 
 SSLErrorClassification::~SSLErrorClassification() { }
@@ -50,12 +72,10 @@ float SSLErrorClassification::InvalidDateSeverityScore() const {
   // not and whether the user has encountered this error before or not.
   float severity_date_score = 0.0f;
 
-  static const float kClientWeight = 0.5f;
   static const float kSystemClockWeight = 0.75f;
   static const float kSystemClockWrongWeight = 0.1f;
   static const float kSystemClockRightWeight = 1.0f;
 
-  static const float kServerWeight = 0.5f;
   static const float kCertificateExpiredWeight = 0.3f;
   static const float kNotYetValidWeight = 0.2f;
 
@@ -69,6 +89,29 @@ float SSLErrorClassification::InvalidDateSeverityScore() const {
   }
   // TODO(radhikabhar): (crbug.com/393262) Check website settings.
 
+float SSLErrorClassification::InvalidDateSeverityScore() const{

[felt, 2014/07/15 00:52:44]

It looks like a rebase went wrong -- InvalidDateSeverityScore is repeated here
twice. I can't tell what is supposed to be new here

[radhikabhar, 2014/07/15 17:34:09]

Done.

+  // CLient-side characteristics. Check whether or not the system's clock is
+  // worng and whether or not the user has already encountered this error
+  // before.
+  float severity_date_score = 0.0f;
+
+  static const float kCertificateExpiredWeight = 0.3f;
+  static const float kNotYetValidWeight = 0.2f;
+
+  static const float kSystemClockWeight = 0.75f;
+  static const float kSystemClockWrongWeight = 0.1f;
+  static const float kSystemClockRightWeight = 1.0f;
+
+  if (IsUserClockInThePast(current_time_)  ||
+      IsUserClockInTheFuture(current_time_)) {
+    severity_date_score += kClientWeight * kSystemClockWeight *
+        kSystemClockWrongWeight;
+  } else {
+    severity_date_score += kClientWeight * kSystemClockWeight *
+        kSystemClockRightWeight;
+  }
+  // TODO(radhikabhar): (crbug.com/393262) Check website settings.
+
   // Server-side characteristics. Check whether the certificate has expired or
   // is not yet valid. If the certificate has expired then factor the time which
   // has passed since expiry.
@@ -81,6 +124,30 @@ float SSLErrorClassification::InvalidDateSeverityScore() const {
   return severity_date_score;
 }
 
+float SSLErrorClassification::InvalidCommonNameSeverityScore() const {
+  float severity_name_score = 0.0f;
+
+  static const float kWWWDifferenceWeight = 0.3f;
+  static const float kRegisteredDomainWeight = 0.2f;
+  static const float kRegisteredDomainInverseWeight = 1.0f;
+
+  if (IsWWWDifference())
+    severity_name_score += kServerWeight * kWWWDifferenceWeight;
+  if (IsRegisteredDomainMatch())
+    severity_name_score += kServerWeight * kRegisteredDomainWeight;
+  // Inverse case is more likely to be a MITM attack.
+  if (IsRegisteredDomainInverseMatch())
+    severity_name_score += kServerWeight * kRegisteredDomainInverseWeight;
+  return severity_name_score;
+}
+
+void SSLErrorClassification::RecordUMAStatistics(bool overridable) {
+  if (IsUserClockInThePast(base::Time::NowFromSystemTime()))
+    RecordSSLInterstitialCause(overridable, CLOCK_PAST);
+  if (IsUserClockInTheFuture(base::Time::NowFromSystemTime()))
+    RecordSSLInterstitialCause(overridable, CLOCK_FUTURE);
+}
+
 base::TimeDelta SSLErrorClassification::TimePassedSinceExpiry() const {
   base::TimeDelta delta = current_time_ - cert_.valid_expiry();
   return delta;
@@ -116,9 +183,132 @@ bool SSLErrorClassification::IsUserClockInTheFuture(base::Time time_now) {
   return false;
 }
 
-void SSLErrorClassification::RecordUMAStatistics(bool overridable) {
-  if (IsUserClockInThePast(base::Time::NowFromSystemTime()))
-    RecordSSLInterstitialCause(overridable, CLOCK_PAST);
-  if (IsUserClockInTheFuture(base::Time::NowFromSystemTime()))
-    RecordSSLInterstitialCause(overridable, CLOCK_FUTURE);
+bool SSLErrorClassification::IsWWWDifference() const {
+  std::string host_name = request_url_.host();
+  if (request_url_.HostIsIPAddress() || host_name.empty())
+    return false;
+
+  std::vector<std::string> dns_names;
+    cert_.GetDNSNames(&dns_names);
+  bool result = false;
+
+  // Need to account for all possible domains given in the SSL certificate.
+  for (size_t i = 0; i < dns_names.size(); ++i) {
+    std::string string_diff;
+    if (dns_names[i].empty() || dns_names[i].find('\0') != std::string::npos
+        || dns_names[i].length() == host_name.length())

[felt, 2014/07/15 00:52:44]

since this is multi-line, I'd recommend putting { }

[radhikabhar, 2014/07/15 17:34:08]

Done.

+      result = result || false;
+    else if (dns_names[i].length() > host_name.length())
+      string_diff = GetStringDifference(dns_names[i], host_name);
+    else
+      string_diff = GetStringDifference(host_name, dns_names[i]);
+    if (!string_diff.empty() && string_diff.compare("www.") == 0)
+      result = result || true;
+  }
+  return result;
+}

[felt, 2014/07/15 00:52:44]

It might be interesting to do another version of this that checks for subdomains
in general (not just www)

[radhikabhar, 2014/07/15 17:34:09]

The function IsRegisteredDomainMatch() checks whether the url is a subdomain of
the dns name given in the server. Is that what you meant or something else?

+
+bool SSLErrorClassification::IsRegisteredDomainMatch() const {
+  std::string host_name = request_url_.host();
+  if (request_url_.HostIsIPAddress() || host_name.empty())
+    return false;
+
+  std::vector<std::string> dns_names;
+  cert_.GetDNSNames(&dns_names);
+  bool result = false;
+
+  // Need to account for all the possible domains given in the SSL certificate.
+   for (size_t i = 0; i < dns_names.size(); ++i) {
+     if (dns_names[i].empty() || dns_names[i].find('\0') != std::string::npos
+         || dns_names[i].length() >= host_name.length())
+       result = result || false;
+     std::string string_diff = GetStringDifference(host_name, dns_names[i]);
+     if (string_diff.empty() || (string_diff.compare("www.") == 0)) {
+        result = result || false;
+     } else {
+       size_t total_count = std::count(string_diff.begin(),
+                                       string_diff.end(), '.');
+       if (total_count == 1)
+         result = result || true;
+     }
+  }
+  return result;
+}
+
+// The inverse case should be treated carefully as this is most likely a MITM
+// attack.
+bool SSLErrorClassification::IsRegisteredDomainInverseMatch() const {
+  std::string host_name = request_url_.host();
+  if (request_url_.HostIsIPAddress() || host_name.empty())
+    return false;
+
+  std::vector<std::string> dns_names;
+  cert_.GetDNSNames(&dns_names);
+  bool result = false;
+
+  // Need to account for all the possible domains given in the SSL certificate.
+  for (size_t i = 0; i < dns_names.size(); ++i) {
+    if (dns_names[i].empty() || dns_names[i].find('\0') != std::string::npos
+        || dns_names[i].length() <= host_name.length())
+      result = result || false;
+    std::string string_diff = GetStringDifference(dns_names[i], host_name);
+    if (string_diff.empty() || (string_diff.compare("www.") == 0)) {
+      result = result || false;
+    } else {
+      size_t total_count = std::count(string_diff.begin(),
+                                      string_diff.end(),'.');
+      if (total_count == 1)
+        result = result || true;
+    }
+  }
+  return result;
+}
+
+// This method is valid for wildcard certificates only.
+bool SSLErrorClassification::IsHostNameTooBroad() const {
+  std::string host_name = request_url_.host();
+  if (request_url_.HostIsIPAddress() || host_name.empty())
+    return false;
+
+  std::vector<std::string> dns_names;
+  cert_.GetDNSNames(&dns_names);
+  bool result = false;
+
+  // This method requires that the host name be longer than the dns name on
+  // the certificate.
+  for (size_t i = 0; i < dns_names.size(); ++i) {
+    if (dns_names[i].find("*") == std::string::npos) {
+      result = result || false;
+    } else {
+      if (dns_names[i].empty() || dns_names[i].find('\0') != std::string::npos
+          || dns_names[i].length() >= host_name.length()) {
+        result = result || false;
+      } else {
+        // Move past the '*' and '.'.
+        std::string extracted_common_name = dns_names[i].substr(2);
+        std::string string_diff = GetStringDifference(host_name,
+                                                      extracted_common_name);
+        size_t total_count = std::count(string_diff.begin(),
+                                        string_diff.end(), '.');
+        if (total_count == 2)
+          result = result || true;
+      }
+    }
+  }
+  return result;
+}
+
+bool SSLErrorClassification::IsSelfSigned() const {
+  // Check whether the issuer and the subject are the same.
+  const net::CertPrincipal& subject = cert_.subject();
+  const net::CertPrincipal& issuer = cert_.issuer();
+  bool result = subject.common_name == issuer.common_name &&
+      subject.locality_name == issuer.locality_name &&
+      subject.state_or_province_name == issuer.state_or_province_name &&
+      subject.country_name == issuer.country_name &&
+      subject.street_addresses == issuer.street_addresses &&
+      subject.organization_names == issuer.organization_names &&
+      subject.organization_unit_names == issuer.organization_names &&
+      subject.domain_components == issuer.domain_components;
+  return result;
 }