Add initial version of captive portal list checking.

chrome/browser/ssl/ssl_error_handler.cc

Index: chrome/browser/ssl/ssl_error_handler.cc
diff --git a/chrome/browser/ssl/ssl_error_handler.cc b/chrome/browser/ssl/ssl_error_handler.cc
index de91bccc4a633a05d462f1fa82afdce2495bd7af..f9a43f9110790181cebfb4b89a2ebe3e280d3700 100644
--- a/chrome/browser/ssl/ssl_error_handler.cc
+++ b/chrome/browser/ssl/ssl_error_handler.cc
@@ -5,10 +5,12 @@
 #include "chrome/browser/ssl/ssl_error_handler.h"
 
 #include <stdint.h>
+#include <unordered_set>
 #include <utility>
 
 #include "base/callback_helpers.h"
 #include "base/feature_list.h"
+#include "base/files/file_util.h"
 #include "base/lazy_instance.h"
 #include "base/macros.h"
 #include "base/metrics/histogram_macros.h"
@@ -21,7 +23,9 @@
 #include "chrome/browser/ssl/bad_clock_blocking_page.h"
 #include "chrome/browser/ssl/ssl_blocking_page.h"
 #include "chrome/browser/ssl/ssl_cert_reporter.h"
+#include "chrome/browser/ssl/tls_error_assistant.pb.h"
 #include "chrome/common/features.h"
+#include "chrome/grit/browser_resources.h"
 #include "components/network_time/network_time_tracker.h"
 #include "components/ssl_errors/error_classification.h"
 #include "components/ssl_errors/error_info.h"
@@ -30,6 +34,7 @@
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
 #include "net/base/net_errors.h"
+#include "ui/base/resource/resource_bundle.h"
 
 #if BUILDFLAG(ENABLE_CAPTIVE_PORTAL_DETECTION)
 #include "chrome/browser/captive_portal/captive_portal_service.h"
@@ -57,19 +62,7 @@ const base::Feature kSSLCommonNameMismatchHandling{
 // - Otherwise, an SSL interstitial is displayed.
 const int64_t kInterstitialDelayInMilliseconds = 3000;
 
-// Events for UMA.
-enum SSLErrorHandlerEvent {
-  HANDLE_ALL,
-  SHOW_CAPTIVE_PORTAL_INTERSTITIAL_NONOVERRIDABLE,
-  SHOW_CAPTIVE_PORTAL_INTERSTITIAL_OVERRIDABLE,
-  SHOW_SSL_INTERSTITIAL_NONOVERRIDABLE,
-  SHOW_SSL_INTERSTITIAL_OVERRIDABLE,
-  WWW_MISMATCH_FOUND,
-  WWW_MISMATCH_URL_AVAILABLE,
-  WWW_MISMATCH_URL_NOT_AVAILABLE,
-  SHOW_BAD_CLOCK,
-  SSL_ERROR_HANDLER_EVENT_COUNT
-};
+const char kHistogram[] = "interstitial.ssl_error_handler";
 
 // Adds a message to console after navigation commits and then, deletes itself.
 // Also deletes itself if the navigation is stopped.
@@ -128,9 +121,9 @@ class CommonNameMismatchRedirectObserver
   DISALLOW_COPY_AND_ASSIGN(CommonNameMismatchRedirectObserver);
 };
 
-void RecordUMA(SSLErrorHandlerEvent event) {
-  UMA_HISTOGRAM_ENUMERATION("interstitial.ssl_error_handler", event,
-                            SSL_ERROR_HANDLER_EVENT_COUNT);
+void RecordUMA(SSLErrorHandler::UMAEvent event) {
+  UMA_HISTOGRAM_ENUMERATION(kHistogram, event,
+                            SSLErrorHandler::SSL_ERROR_HANDLER_EVENT_COUNT);
 }
 
 #if BUILDFLAG(ENABLE_CAPTIVE_PORTAL_DETECTION)
@@ -143,6 +136,23 @@ bool IsSSLCommonNameMismatchHandlingEnabled() {
   return base::FeatureList::IsEnabled(kSSLCommonNameMismatchHandling);
 }
 
+// Reads the TLS error assistant configuration from the resource bundle.
+void ReadErrorAssistantProtoFromResourceBundle(std::string* binary_pb) {
+  ui::ResourceBundle& bundle = ui::ResourceBundle::GetSharedInstance();
+  bundle.GetRawDataResource(IDR_TLS_ERROR_ASSISTANT_PB).CopyToString(binary_pb);
+}
+
+std::unique_ptr<std::unordered_set<std::string>> LoadCaptivePortalCertHashes(
+    const chrome_browser_ssl::TLSErrorAssistantConfig& proto) {
+  std::unique_ptr<std::unordered_set<std::string>> hashes(
+      new std::unordered_set<std::string>());
+  for (const chrome_browser_ssl::CaptivePortalCert& cert :
+       proto.captive_portal_cert()) {
+    hashes.get()->insert(cert.sha256_hash());
+  }
+  return hashes;
+}
+
 // Configuration for SSLErrorHandler.
 class ConfigSingleton : public base::NonThreadSafe {
  public:
@@ -153,12 +163,19 @@ class ConfigSingleton : public base::NonThreadSafe {
   base::Clock* clock() const;
   network_time::NetworkTimeTracker* network_time_tracker() const;
 
+  // Returns true if one of the cert hashes in |ssl_info| is of a captive portal

[estark, 2017/01/12 18:54:43]

Haven't read the implementation yet, but this makes it sound like we check every
cert in the chain against the captive portal list.

Is that intentional? I'm wondering if it's safer and equally effective to just
check the leaf cert. (In case we somehow accidentally were to end up with an
intermediate or root on the captive portal list.)

....


or after reading a little below, maybe this means that every different hash
algorithm of the leaf cert is checked in the captive portal list. So, plz
clarify?

[meacer, 2017/01/12 23:54:03]

It's intentional for this draft in the sense that I wanted to make sure leaf is
the first or the last cert in the chain. If we can guarantee, we should just
check the leaf.

Though I wonder if there are any intermediates that we can also mark as captive
portal (haven't seen any so far)
Yes, it is checking both hashes. This is another thing I wanted to clarify, do
we always have both hashes? If so, we can always check the sha256 version.

Also note that it's checking the serialized hashes via ToString(), rather than
the actual hash. The reason is code simplicity: Since the hashes are put in a
set, we'll have to pull in comparison operators for net::HashValue if we use the
actual hashes. Let me know if you prefer using std::unorderd_set<net::HashValue>
instead.

[estark, 2017/01/13 23:40:23]

Hmmm. From my reading of ssl_info.h, I would guess we can't assume anything
about the order of the hashes or what algorithms are present. Nor do I see any
nicely exposed way to calculate an SPKI hash. So I guess what you're doing is
the best option, we should just make sure to only add leaf certs to the captive
portal list. (Is there somewhere we can document that?)

(I think comparing the strings is fine.)

[meacer, 2017/01/20 21:29:59]

Added a note to tls_error_assistant.proto. In the future, we can also do some
validation while generating the binary version of that proto, but that will be
complicated (e.g. will need to add the full PEM).

+  // certificate. The set of captive portal hashes is loaded on first use.
+  bool IsKnownCaptivePortalCert(const net::SSLInfo& ssl_info);
+
+  // Testing methods:
   void SetInterstitialDelayForTesting(const base::TimeDelta& delay);
   void SetTimerStartedCallbackForTesting(
       SSLErrorHandler::TimerStartedCallback* callback);
   void SetClockForTesting(base::Clock* clock);
   void SetNetworkTimeTrackerForTesting(
       network_time::NetworkTimeTracker* tracker);
+  void SetErrorAssistantProtoForTesting(
+      const chrome_browser_ssl::TLSErrorAssistantConfig& error_assistant_proto);
 
  private:
   base::TimeDelta interstitial_delay_;
@@ -172,6 +189,11 @@ class ConfigSingleton : public base::NonThreadSafe {
   base::Clock* testing_clock_ = nullptr;
 
   network_time::NetworkTimeTracker* network_time_tracker_ = nullptr;
+
+  // SPKI hashes belonging to certs treated as captive portal portals. Populated

[estark, 2017/01/12 18:54:43]

nit: extra "portal"

[meacer, 2017/01/20 21:29:59]

Done.

+  // the first time IsKnownCaptivePortalCert() or
+  // SetErrorAssistantProtoForTesting() is called.
+  std::unique_ptr<std::unordered_set<std::string>> captive_portal_spki_hashes_;
 };
 
 ConfigSingleton::ConfigSingleton()
@@ -218,6 +240,31 @@ void ConfigSingleton::SetNetworkTimeTrackerForTesting(
   network_time_tracker_ = tracker;
 }
 
+void ConfigSingleton::SetErrorAssistantProtoForTesting(
+    const chrome_browser_ssl::TLSErrorAssistantConfig& error_assistant_proto) {
+  DCHECK(!captive_portal_spki_hashes_);
+  captive_portal_spki_hashes_ =
+      LoadCaptivePortalCertHashes(error_assistant_proto);
+}
+
+bool ConfigSingleton::IsKnownCaptivePortalCert(const net::SSLInfo& ssl_info) {
+  if (!captive_portal_spki_hashes_) {
+    std::string binary_proto;
+    ReadErrorAssistantProtoFromResourceBundle(&binary_proto);
+    chrome_browser_ssl::TLSErrorAssistantConfig proto;
+    proto.ParseFromString(binary_proto);
+    captive_portal_spki_hashes_ = LoadCaptivePortalCertHashes(proto);
+  }
+
+  for (const net::HashValue& hash_value : ssl_info.public_key_hashes) {

[estark, 2017/01/12 18:54:43]

Ok, now that I got down here, it looks like we're comparing the hashes of
several algorithms for each cert in the chain. So I think the comment above
should be clarified (line 166), and also want to make sure you're intending to
check every cert in the chain. Is this mostly for convenience, so that we don't
have to calculate the SPKI hashes of the leaf cert here?

[meacer, 2017/01/12 23:54:03]

Can we not assume the first or the last two of the hashes belong to the leaf? If
yes I don't think we need to re-calculate the hashes, right? 

[meacer, 2017/01/20 21:29:59]

Added sha256 check and clarified comment in line 166.

+    if (captive_portal_spki_hashes_->find(hash_value.ToString()) !=
+        captive_portal_spki_hashes_->end()) {
+      return true;
+    }
+  }
+  return false;
+}
+
 static base::LazyInstance<ConfigSingleton>::Leaky g_config =
     LAZY_INSTANCE_INITIALIZER;
 
@@ -266,6 +313,17 @@ void SSLErrorHandler::SetNetworkTimeTrackerForTesting(
   g_config.Pointer()->SetNetworkTimeTrackerForTesting(tracker);
 }
 
+// static
+void SSLErrorHandler::SetErrorAssistantProtoForTesting(
+    const chrome_browser_ssl::TLSErrorAssistantConfig& config_proto) {
+  g_config.Pointer()->SetErrorAssistantProtoForTesting(config_proto);
+}
+
+// static
+std::string SSLErrorHandler::GetHistogramNameForTesting() {

[estark, 2017/01/12 18:54:43]

Ah, this is a good idea, I always end up repeating the histogram name literal in
the tests...

[meacer, 2017/01/12 23:54:03]

It always works until it doesn't.

+  return kHistogram;
+}
+
 SSLErrorHandler::SSLErrorHandler(
     content::WebContents* web_contents,
     int cert_error,
@@ -297,6 +355,13 @@ void SSLErrorHandler::StartHandlingError() {
     return;
   }
 
+  if (cert_error_ == net::ERR_CERT_COMMON_NAME_INVALID &&
+      g_config.Pointer()->IsKnownCaptivePortalCert(ssl_info_)) {

[estark, 2017/01/12 18:54:43]

Maybe this should be Finchable?

It would be good to have a Finch feature as a kill switch, plus if we roll it
out 50/50 we could see via UMA the difference in captive portal interstitials
shown for users who are checking the captive portal list and those who aren't.

[meacer, 2017/01/12 23:54:03]

I was hoping to avoid another finch for this file, but being able to experiment
sounds like a good idea. Will do this.

+    RecordUMA(CAPTIVE_PORTAL_CERT_FOUND);
+    ShowCaptivePortalInterstitial(GURL());

[estark, 2017/01/12 18:54:43]

How will this work without an empty landing URL? Does that mean the Connect
button (or whatever it's called) won't go anywhere?

[meacer, 2017/01/12 23:54:03]

Empty landing URL takes the user to the captive portal ping URL
(www.gstatic.com/generate_204). If the captive portal is a proper one, it should
then show its own login page there.

[meacer, 2017/01/20 21:29:59]

Actually, this was buggy (the text was broken). I'm now setting the URL
explicitly.

+    return;
+  }
+
   std::vector<std::string> dns_names;
   ssl_info_.cert->GetDNSNames(&dns_names);
   DCHECK(!dns_names.empty());