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..6acd95283c79f3f6e7173ac7c5c63d53816b70c2 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.

[felt, 2014/07/15 20:44:34]

Pretty sure this was correct before, they removed the "(c)" in 2013 or 2014.

[radhikabhar, 2014/07/16 22:35:15]

Done.

 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -8,9 +8,9 @@
 #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_cert_types.h"
 #include "net/cert/x509_certificate.h"
+#include "url/gurl.h"
 
 using base::Time;
 using base::TimeTicks;
@@ -25,6 +25,10 @@ enum SSLInterstitialCause {
   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 +39,52 @@ void RecordSSLInterstitialCause(bool overridable, SSLInterstitialCause event) {
   }
 }
 
+// Utility function - For two unequal strings, this method checks to see whether
+// the |longer_str| = prefix + |smaller_str|. If so, then it returns the prefix

[palmer, 2014/07/15 21:23:23]

Nit: Antonyms are longer/shorter, larger/smaller.

[radhikabhar, 2014/07/16 22:35:14]

Done.

+// otherwise it returns an empty string.
+std::string GetStringPrefix(std::string longer_str, std::string smaller_str) {
+  std::size_t found = longer_str.find(smaller_str);
+  if (found != std::string::npos) {
+    std::string result_last = longer_str.substr(found);
+    if (result_last.compare(smaller_str) != 0 )
+      return std::string();
+    std::string result_first = longer_str.substr(0, found);
+    return result_first;
+  }
+  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() { }
 
-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

[felt, 2014/07/15 20:44:34]

nit: Client-

[radhikabhar, 2014/07/16 22:35:15]

Done.

+  // worng and whether or not the user has already encountered this error

[felt, 2014/07/15 20:44:34]

nit: wrong

[radhikabhar, 2014/07/16 22:35:15]

Done.

+  // 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 +101,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 kSubDomainWeight = 0.2f;
+  static const float kSubDomainInverseWeight = 1.0f;
+
+  if (IsWWWDifference())
+    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::RecordUMAStatistics(bool overridable) {

[felt, 2014/07/15 20:44:34]

Can you add UMA stats for all the other ones too? IsWWWDifference,
IsSubDomainMatch, etc.

[radhikabhar, 2014/07/16 22:35:14]

Done.

+  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 +160,133 @@ 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 {

[felt, 2014/07/15 20:44:33]

suggestion: rename this IsWWWSubDomainMatch to be consistent with the other
methods

[radhikabhar, 2014/07/16 22:35:14]

Done.

+  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);

[felt, 2014/07/15 20:44:34]

spurious indentation

[radhikabhar, 2014/07/16 22:35:15]

Done.

+  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_prefix;
+    if (dns_names[i].empty() || dns_names[i].find('\0') != std::string::npos
+        || dns_names[i].length() == host_name.length()) {
+      result = result || false;
+    } else if (dns_names[i].length() > host_name.length()) {
+      string_prefix = GetStringPrefix(dns_names[i], host_name);

[palmer, 2014/07/15 21:23:23]

BUG: You really do need to tokenize the hostnames. That is, split on "." into a
vector of DNS labels:

   "www.google.com" --> ("www", "google", "com")

Otherwise, you run the risk of treating "totallyfakegoogle.com" as a subdomain
of "google.com"!

I think what you want is something more like:

bool IsSubdomain(const vector<string>& tokenized_parent,
                 const vector<string>& tokenized_potential_subdomain) {
  // ...
}

But I suspect that some similar code — possibly code that takes the Public
Suffix List into account — already exists somewhere in Chromium.

And you probably will want to take the PSL into account. Probably even as simple
as, "never apply this heuristic to any hostname that ends in a domain name in
the PSL that is not a true TLD; require exact matches for such names."

[radhikabhar, 2014/07/16 22:35:15]

Done

+    } else {
+      string_prefix = GetStringPrefix(host_name, dns_names[i]);
+    }
+    if (!string_prefix.empty() && string_prefix.compare("www.") == 0)
+      result = result || true;
+  }
+  return result;
+}
+
+bool SSLErrorClassification::IsSubDomainMatch() 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) {

[felt, 2014/07/15 20:44:34]

indentation

+     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_prefix = GetStringPrefix(host_name, dns_names[i]);
+     if (string_prefix.empty() || (string_prefix.compare("www.") == 0)) {

[felt, 2014/07/15 20:44:34]

Why is this false if the string prefix is www.? Shouldn't IsSubDomainMatch be a
superset of IsWWWDifference?

[radhikabhar, 2014/07/16 22:35:14]

Yes but then I wanted to differentiate between the cases for www and the rest of
them because www is more likely to be benign as compared to any other case.
Also, if I return true out here for www then it would be recorded twice in the
histogram once in the bucket for IsWWWDifference and IsSubDomain or so we want
to record them in both the places? 
If I am calculating the score for Common_name_invalid, I will be calling both
the functions and this will again return twice which might mess with the
calculation.

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

ok sounds good

+        result = result || false;
+     } else {
+       size_t total_count = std::count(string_prefix.begin(),

[felt, 2014/07/15 20:44:34]

What about the pair: a.foogoogle.com, google.com? It should return false, but I
think it will return true.

[radhikabhar, 2014/07/16 22:35:15]

You are right. Fixed it by tokenizing the string and then checking whether it is
a sub domain or not. Added a test case too.

+                                       string_prefix.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.

[felt, 2014/07/15 20:44:33]

Can you explain why, for posterity?

[radhikabhar, 2014/07/16 22:35:14]

Done.

+bool SSLErrorClassification::IsSubDomainInverseMatch() 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_prefix = GetStringPrefix(dns_names[i], host_name);
+    if (string_prefix.empty() || (string_prefix.compare("www.") == 0)) {
+      result = result || false;
+    } else {
+      size_t total_count = std::count(string_prefix.begin(),
+                                      string_prefix.end(),'.');

[felt, 2014/07/15 20:44:34]

nit: space between arguments

[radhikabhar, 2014/07/16 22:35:15]

Done.

+      if (total_count == 1)
+        result = result || true;
+    }
+  }
+  return result;
+}
+
+// This method is valid for wildcard certificates only.

[felt, 2014/07/15 20:44:34]

you should make sure that it returns false if it runs on a non-wildcard
certificate (add a test, I don't see one)

[radhikabhar, 2014/07/16 22:35:14]

I actually do check it for the google cert on line 58, Should I add more test
cases?

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

missed that, OK

+bool SSLErrorClassification::IsHostNameTooBroad() const {

[felt, 2014/07/15 20:44:34]

You might want to consider renaming this something like:
IsSubDomainOutsideWildcard, or something like that. I didn't understand what
"too broad" meant here. Also, might make sense to document with the good example
you gave me: it should return true for cases like a.b.foo.com, when the
certificate is for *.foo.com.

[radhikabhar, 2014/07/16 22:35:15]

Done.

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

[felt, 2014/07/15 20:44:34]

More specifically, you are expecting the first two characters to be "*.". You
should probably check for that directly, instead of checking to see if there is
a * anywhere.

[radhikabhar, 2014/07/16 22:35:14]

Done.

+      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_prefix = GetStringPrefix(host_name,
+                                                      extracted_common_name);

[felt, 2014/07/15 20:44:33]

^ indentation

[radhikabhar, 2014/07/16 22:35:15]

Done.

+        size_t total_count = std::count(string_prefix.begin(),
+                                        string_prefix.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;
 }