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..4fa70bdfd1ca4820bf26bb009f271cc7d2217209 100644
--- a/chrome/browser/ssl/ssl_error_classification.cc
+++ b/chrome/browser/ssl/ssl_error_classification.cc
@@ -2,15 +2,22 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <vector>
+
 #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/strings/string16.h"
+#include "base/strings/string_split.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
-#include "chrome/browser/browser_process.h"
-#include "components/network_time/network_time_tracker.h"
+#include "net/base/net_util.h"
+#include "net/base/registry_controlled_domains/registry_controlled_domain.h"
+#include "net/cert/x509_cert_types.h"
 #include "net/cert/x509_certificate.h"
+#include "url/gurl.h"
 
 using base::Time;
 using base::TimeTicks;
@@ -22,9 +29,18 @@ namespace {
 enum SSLInterstitialCause {
   CLOCK_PAST,
   CLOCK_FUTURE,
+  WWW_SUBDOMAIN_MATCH,
+  SUBDOMAIN_MATCH,
+  SUBDOMAIN_INVERSE_MATCH,
+  SUBDOMAIN_OUTSIDE_WILDCARD,
+  SELF_SIGNED,
   UNUSED_INTERSTITIAL_CAUSE_ENTRY,
 };
 
+// Scores/weights which will be constant through all the SSL error types.
+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,36 +51,62 @@ void RecordSSLInterstitialCause(bool overridable, SSLInterstitialCause event) {
   }
 }
 
+// Utility function - For two unequal strings which have been tokenized, this
+// method checks to see whether |tokenized_potential_sundomain| is a subdomain

[felt, 2014/07/16 23:31:47]

typo: sundomain -> subdomain

[radhikabhar, 2014/07/17 01:02:12]

Done.

+// of |tokenized_parent| and if it is then it returns the difference in the
+// number of tokens between both the vectors, i.e. the difference in the vector
+// size.
+int IsSubDomainDifference(

[felt, 2014/07/16 23:31:47]

since this returns an int now, you should rename it FindSubDomainDifference

[radhikabhar, 2014/07/17 01:02:11]

Done.

+    const std::vector<base::string16>& tokenized_potential_subdomian,
+    const std::vector<base::string16>& tokenized_parent) {
+  // A check to ensure that the number of tokens in the tokenized parent is
+  // greater than the tokenized_potential_subdomain.
+  if (tokenized_parent.size() > tokenized_potential_subdomian.size())
+    return 0;

[felt, 2014/07/16 23:31:47]

what's the anticipated behavior if they are equivalent? should it be >= ?

(I know, that can't happen right now because this should only be invoked if
there's a mismatch. But you should still keep it in mind, because the calling
logic for this method might change.)

[radhikabhar, 2014/07/17 01:02:12]

Done.

+
+  size_t tokens_match = 0;
+  size_t diff_size = tokenized_potential_subdomian.size() -
+      tokenized_parent.size();
+  for (size_t i = 0; i < tokenized_parent.size(); ++i) {
+    if (tokenized_parent[i] == tokenized_potential_subdomian[i + diff_size])
+      tokens_match++;
+  }
+  if (tokens_match == tokenized_parent.size())
+    return diff_size;
+  return 0;
+}
+
 } // 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() { }
 
-float SSLErrorClassification::InvalidDateSeverityScore() const {
-  // Client-side characterisitics. Check whether the system's clock is wrong or
-  // not and whether the user has encountered this error before or not.
+float SSLErrorClassification::InvalidDateSeverityScore() const{
+  // Client-side characteristics. Check whether or not the system's clock is
+  // wrong and whether or not the user has already encountered this error
+  // before.
   float severity_date_score = 0.0f;
 
-  static const float kClientWeight = 0.5f;
+  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;
 
-  static const float kServerWeight = 0.5f;
-  static const float kCertificateExpiredWeight = 0.3f;
-  static const float kNotYetValidWeight = 0.2f;
-
   if (IsUserClockInThePast(current_time_)  ||
       IsUserClockInTheFuture(current_time_)) {
-    severity_date_score = kClientWeight * kSystemClockWeight *
+    severity_date_score += kClientWeight * kSystemClockWeight *
         kSystemClockWrongWeight;
   } else {
-    severity_date_score = kClientWeight * kSystemClockWeight *
+    severity_date_score += kClientWeight * kSystemClockWeight *
         kSystemClockRightWeight;
   }
   // TODO(radhikabhar): (crbug.com/393262) Check website settings.
@@ -81,6 +123,47 @@ 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 kSubDomainWeight = 0.2f;
+  static const float kSubDomainInverseWeight = 1.0f;
+
+  if (IsWWWSubDomainMatch())
+    severity_name_score += kServerWeight * kWWWDifferenceWeight;
+  if (IsSubDomainMatch())
+    severity_name_score += kServerWeight * kSubDomainWeight;
+  // Inverse case is more likely to be a MITM attack.
+  if (IsSubDomainInverseMatch())
+    severity_name_score += kServerWeight * kSubDomainInverseWeight;
+  return severity_name_score;
+}
+
+void SSLErrorClassification::RecordUMAStatisticsDateInvalid(bool overridable) {
+  if (IsUserClockInThePast(base::Time::NowFromSystemTime()))
+    RecordSSLInterstitialCause(overridable, CLOCK_PAST);
+  if (IsUserClockInTheFuture(base::Time::NowFromSystemTime()))
+    RecordSSLInterstitialCause(overridable, CLOCK_FUTURE);
+}
+
+void SSLErrorClassification::RecordUMAStatisticsNameInvalid(
+    bool overridable) {
+

[felt, 2014/07/16 23:31:47]

nit: no empty line

[radhikabhar, 2014/07/17 01:02:12]

Done.

+  if (IsWWWSubDomainMatch())
+    RecordSSLInterstitialCause(overridable, WWW_SUBDOMAIN_MATCH);
+  if (IsSubDomainMatch())
+    RecordSSLInterstitialCause(overridable, SUBDOMAIN_MATCH);
+  if (IsSubDomainInverseMatch())
+    RecordSSLInterstitialCause(overridable, SUBDOMAIN_INVERSE_MATCH);
+}
+
+void SSLErrorClassification::RecordUMAStatisticsAuthorityInvalid(
+    bool overridable) {
+  if (IsSelfSigned())
+    RecordSSLInterstitialCause(overridable, SELF_SIGNED);
+}
+
 base::TimeDelta SSLErrorClassification::TimePassedSinceExpiry() const {
   base::TimeDelta delta = current_time_ - cert_.valid_expiry();
   return delta;
@@ -116,9 +199,166 @@ 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::IsNotValidURL(const GURL& url) {
+  size_t tld_length =
+      net::registry_controlled_domains::GetRegistryLength(
+          url,
+          net::registry_controlled_domains::EXCLUDE_UNKNOWN_REGISTRIES,
+          net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES);
+  if (tld_length == 0 || tld_length == std::string::npos)
+    return true;
+  return false;
+}
+
+
+bool SSLErrorClassification::IsWWWSubDomainMatch() const {

[felt, 2014/07/16 23:31:47]

can you document that you *do* accept the "inverse" in this case, and explain
why?

[radhikabhar, 2014/07/17 01:02:11]

Done.

+  std::string host_name = request_url_.host();
+  if (request_url_.HostIsIPAddress() || host_name.empty() ||
+      IsNotValidURL(request_url_)) {
+    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) {
+    if (dns_names[i].empty() || dns_names[i].find('\0') != std::string::npos
+        || dns_names[i].length() == host_name.length()) {

[felt, 2014/07/16 23:31:47]

do you want to check IsNotValidURL for the dns names too? (same question for the
others)

[radhikabhar, 2014/07/17 01:02:12]

Added a check.

+      result = result || false;
+    } else if (dns_names[i].length() > host_name.length()) {
+      result = result ||
+          net::StripWWW(base::ASCIIToUTF16(dns_names[i])) ==
+          base::ASCIIToUTF16(host_name);
+    } else {
+      result = result ||
+          net::StripWWW(base::ASCIIToUTF16(host_name)) ==
+          base::ASCIIToUTF16(dns_names[i]);
+    }
+  }
+  return result;
+}
+
+bool SSLErrorClassification::IsSubDomainMatch() const {
+  std::string host_name = request_url_.host();
+  if (request_url_.HostIsIPAddress() || host_name.empty() ||
+      IsNotValidURL(request_url_)) {

[felt, 2014/07/16 23:31:47]

do you need to do the HostIsIPAddress and host_name.empty() checks, or are they
redundant with what GetRegistryLength already checks? if you do need them, can
you move the checks into IsNotValidUrl?

[radhikabhar, 2014/07/17 01:02:11]

Yes they are redundant. Removed them

+    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;
+    } else {
+      std::vector<base::string16> dns_name_tokens;
+      std::vector<base::string16> host_name_tokens;
+      base::SplitStringDontTrim(base::ASCIIToUTF16(dns_names[i]),
+                                '.',
+                                &dns_name_tokens);
+      base::SplitStringDontTrim(base::ASCIIToUTF16(host_name),
+                                '.',
+                                &host_name_tokens);
+      int domain_diff = IsSubDomainDifference(host_name_tokens,
+                                              dns_name_tokens);
+      if (domain_diff == 1 &&  host_name_tokens[0] != base::ASCIIToUTF16("www"))
+        result = result || true;
+    }
+  }
+  return result;
+}
+
+// The inverse case should be treated carefully as this is most likely a MITM
+// attack. We don't want foo.appspot.com to be able to MITM for appspot.com.
+bool SSLErrorClassification::IsSubDomainInverseMatch() const {
+  std::string host_name = request_url_.host();
+  if (request_url_.HostIsIPAddress() || host_name.empty() ||
+      IsNotValidURL(request_url_)) {
+    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;
+    } else {
+      std::vector<base::string16> dns_name_tokens;
+      std::vector<base::string16> host_name_tokens;
+      base::SplitStringDontTrim(base::ASCIIToUTF16(dns_names[i]),
+                                '.',
+                                &dns_name_tokens);
+      base::SplitStringDontTrim(base::ASCIIToUTF16(host_name),
+                                '.',
+                                &host_name_tokens);
+      int domain_diff = IsSubDomainDifference(dns_name_tokens,
+                                              host_name_tokens);
+      if (domain_diff == 1 && dns_name_tokens[0] != base::ASCIIToUTF16("www"))
+        result = result || true;
+    }
+  }
+  return result;
+}
+
+// This method is valid for wildcard certificates only.
+bool SSLErrorClassification::IsSubDomainOutsideWildcard() const {
+  std::string host_name = request_url_.host();
+  if (request_url_.HostIsIPAddress() || host_name.empty() ||
+      IsNotValidURL(request_url_))
+    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][0] == '*' && dns_names[i][1] == '.')) {
+      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 '*.'.
+        std::string extracted_dns_name = dns_names[i].substr(2);
+        std::vector<base::string16> extracted_dns_name_tokens;
+        std::vector<base::string16> host_name_tokens;
+        base::SplitStringDontTrim(base::ASCIIToUTF16(extracted_dns_name),
+                                  '.',
+                                  &extracted_dns_name_tokens);
+        base::SplitStringDontTrim(base::ASCIIToUTF16(host_name),
+                                  '.',
+                                  &host_name_tokens);
+        int domain_diff = IsSubDomainDifference(host_name_tokens,
+                                                extracted_dns_name_tokens);
+        if (domain_diff == 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;
 }