Add initial version of captive portal list checking.

chrome/browser/ssl/ssl_browser_tests.cc

Index: chrome/browser/ssl/ssl_browser_tests.cc
diff --git a/chrome/browser/ssl/ssl_browser_tests.cc b/chrome/browser/ssl/ssl_browser_tests.cc
index 9bbf09a3a2d08454c0f0df78ea52cd223036300d..cc7f4b00d2cbab0ba7b0cd332f505320d4c71dac 100644
--- a/chrome/browser/ssl/ssl_browser_tests.cc
+++ b/chrome/browser/ssl/ssl_browser_tests.cc
@@ -18,6 +18,7 @@
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/histogram_tester.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/test/simple_test_clock.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/default_clock.h"
@@ -38,6 +39,7 @@
 #include "chrome/browser/ssl/common_name_mismatch_handler.h"
 #include "chrome/browser/ssl/security_state_tab_helper.h"
 #include "chrome/browser/ssl/ssl_blocking_page.h"
+#include "chrome/browser/ssl/ssl_error_assistant.pb.h"
 #include "chrome/browser/ssl/ssl_error_handler.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
@@ -84,9 +86,11 @@
 #include "content/public/test/test_navigation_observer.h"
 #include "content/public/test/test_renderer_host.h"
 #include "content/public/test/test_utils.h"
+#include "crypto/sha2.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
+#include "net/cert/asn1_util.h"
 #include "net/cert/cert_status_flags.h"
 #include "net/cert/mock_cert_verifier.h"
 #include "net/cert/x509_certificate.h"
@@ -110,6 +114,10 @@
 #include "net/cert/nss_cert_database.h"
 #endif  // defined(USE_NSS_CERTS)
 
+#if BUILDFLAG(ENABLE_CAPTIVE_PORTAL_DETECTION)
+#include "chrome/browser/ssl/captive_portal_blocking_page.h"
+#endif
+
 using base::ASCIIToUTF16;
 using chrome_browser_interstitials::SecurityInterstitialIDNTest;
 using content::InterstitialPage;
@@ -124,6 +132,12 @@ const base::FilePath::CharType kDocRoot[] =
 
 namespace {
 
+#if BUILDFLAG(ENABLE_CAPTIVE_PORTAL_DETECTION)
+// A hostname that will trigger a captive portal interstitial. The actual value
+// isn't important.
+const char kCaptivePortalHostname[] = "captive-portal.test";
+#endif
+
 enum ProceedDecision {
   SSL_INTERSTITIAL_PROCEED,
   SSL_INTERSTITIAL_DO_NOT_PROCEED
@@ -226,12 +240,17 @@ class SSLInterstitialTimerObserver {
   // Waits until the interstitial delay timer in SSLErrorHandler is started.
   void WaitForTimerStarted() { message_loop_runner_->Run(); }
 
+  // Returns true if the interstitial delay timer has been started.
+  bool timer_started() const { return timer_started_; }
+
  private:
   void OnTimerStarted(content::WebContents* web_contents) {
+    timer_started_ = true;
     if (web_contents_ == web_contents)
       message_loop_runner_->Quit();
   }
 
+  bool timer_started_ = false;
   const content::WebContents* web_contents_;
   SSLErrorHandler::TimerStartedCallback callback_;
 
@@ -273,6 +292,19 @@ std::string EncodeQuery(const std::string& query) {
   return std::string(buffer.data(), buffer.length());
 }
 
+// Returns the Sha256 hash of the SPKI of |cert|.
+net::HashValue GetSPKIHash(net::X509Certificate* cert) {
+  std::string der_data;
+  EXPECT_TRUE(
+      net::X509Certificate::GetDEREncoded(cert->os_cert_handle(), &der_data));
+  base::StringPiece der_bytes(der_data);
+  base::StringPiece spki_bytes;
+  EXPECT_TRUE(net::asn1::ExtractSPKIFromDERCert(der_bytes, &spki_bytes));
+  net::HashValue sha256(net::HASH_VALUE_SHA256);
+  crypto::SHA256HashString(spki_bytes, sha256.data(), crypto::kSHA256Length);
+  return sha256;
+}
+
 }  // namespace
 
 class SSLUITest : public InProcessBrowserTest {
@@ -308,6 +340,11 @@ class SSLUITest : public InProcessBrowserTest {
         "https", "localhost", std::move(interceptor));
   }
 
+  void SetUp() override {
+    InProcessBrowserTest::SetUp();
+    SSLErrorHandler::ResetConfigForTesting();

[Ryan Sleevi, 2017/02/02 02:24:36]

I believe you also should do this in TearDown(), although it's mostly pedantry
in a world of isolated tests (just clearer symmetry)

Alternatively, a scoped resetter would also work, but that's probably more
hassle :)

[meacer, 2017/02/06 23:39:17]

Done.

+  }
+
   void SetUpCommandLine(base::CommandLine* command_line) override {
     // Browser will both run and display insecure content.
     command_line->AppendSwitch(switches::kAllowRunningInsecureContent);
@@ -3885,6 +3922,262 @@ IN_PROC_BROWSER_TEST_F(SSLUITestIgnoreLocalhostCertErrors,
   ASSERT_TRUE(content::ExecuteScript(tab, "window.open()"));
 }
 
+#if BUILDFLAG(ENABLE_CAPTIVE_PORTAL_DETECTION)
+
+IN_PROC_BROWSER_TEST_F(SSLUITest, CaptivePortalCertificateList_Disabled) {

[estark, 2017/02/03 07:24:07]

optional nit: for this and the other tests, I like having a one-sentence
description of the test above this line, e.g. "Tests that the captive portal
certificate list is not used when the feature is disabled via Finch."
(especially in this file where there are gazillions of tests... maybe we should
look at splitting them up/organizing them at some point)

[meacer, 2017/02/06 23:39:17]

Done.

+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitFromCommandLine(

[estark, 2017/02/03 07:24:07]

optional nit: I'd find InitAndDisableFeature() a bit easier to read (here and
below)

[meacer, 2017/02/06 23:39:17]

I initially wanted to use it too, but it requires exposing the feature in
SSLErrorHandler. That would normally be okay, but the feature is protected by
BUILDFLAG(ENABLE_CAPTIVE_PORTAL_DETECTION) which will require pulling a bunch of
headers (feature list and buildflags) to SSLErrorHandler header. I kind of want
to avoid that, but don't feel strongly if you really want this :)

+      "" /* enabled */, "CaptivePortalCertificateList" /* disabled */);
+
+  ASSERT_TRUE(https_server_mismatched_.Start());
+  base::HistogramTester histograms;
+
+  // Mark the server's cert as a captive portal cert.
+  const net::HashValue server_spki_hash =
+      GetSPKIHash(https_server_mismatched_.GetCertificate().get());
+  chrome_browser_ssl::SSLErrorAssistantConfig config_proto;
+  config_proto.add_captive_portal_cert()->set_sha256_hash(
+      server_spki_hash.ToString());
+  SSLErrorHandler::SetErrorAssistantProtoForTesting(config_proto);
+
+  // Navigate to an unsafe page on the server. A normal SSL interstitial should
+  // be displayed since CaptivePortalCertificateList feature is disabled.
+  WebContents* tab = browser()->tab_strip_model()->GetActiveWebContents();
+  SSLInterstitialTimerObserver interstitial_timer_observer(tab);
+  ui_test_utils::NavigateToURL(
+      browser(), https_server_mismatched_.GetURL("/ssl/blank_page.html"));
+  content::WaitForInterstitialAttach(tab);
+
+  InterstitialPage* interstitial_page = tab->GetInterstitialPage();
+  ASSERT_EQ(SSLBlockingPage::kTypeForTesting,
+            interstitial_page->GetDelegateForTesting()->GetTypeForTesting());
+  EXPECT_TRUE(interstitial_timer_observer.timer_started());
+
+  // Check that the histogram for the SSL interstitial was recorded.
+  histograms.ExpectTotalCount(SSLErrorHandler::GetHistogramNameForTesting(), 2);
+  histograms.ExpectBucketCount(SSLErrorHandler::GetHistogramNameForTesting(),
+                               SSLErrorHandler::HANDLE_ALL, 1);
+  histograms.ExpectBucketCount(
+      SSLErrorHandler::GetHistogramNameForTesting(),
+      SSLErrorHandler::SHOW_SSL_INTERSTITIAL_OVERRIDABLE, 1);
+  histograms.ExpectBucketCount(SSLErrorHandler::GetHistogramNameForTesting(),
+                               SSLErrorHandler::CAPTIVE_PORTAL_CERT_FOUND, 0);
+}
+
+IN_PROC_BROWSER_TEST_F(SSLUITest,
+                       CaptivePortalCertificateList_Enabled_FromProto) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitFromCommandLine(
+      "CaptivePortalCertificateList" /* enabled */, "" /* disabled */);
+
+  ASSERT_TRUE(https_server_mismatched_.Start());
+  base::HistogramTester histograms;
+
+  // Mark the server's cert as a captive portal cert.
+  const net::HashValue server_spki_hash =
+      GetSPKIHash(https_server_mismatched_.GetCertificate().get());
+  chrome_browser_ssl::SSLErrorAssistantConfig config_proto;
+  config_proto.add_captive_portal_cert()->set_sha256_hash(
+      server_spki_hash.ToString());
+  SSLErrorHandler::SetErrorAssistantProtoForTesting(config_proto);
+
+  // Navigate to an unsafe page on the server. The captive portal interstitial
+  // should be displayed since CaptivePortalCertificateList feature is enabled.
+  WebContents* tab = browser()->tab_strip_model()->GetActiveWebContents();
+  SSLInterstitialTimerObserver interstitial_timer_observer(tab);
+  ui_test_utils::NavigateToURL(
+      browser(), https_server_mismatched_.GetURL("/ssl/blank_page.html"));
+  content::WaitForInterstitialAttach(tab);
+
+  InterstitialPage* interstitial_page = tab->GetInterstitialPage();
+  ASSERT_EQ(CaptivePortalBlockingPage::kTypeForTesting,
+            interstitial_page->GetDelegateForTesting()->GetTypeForTesting());
+  EXPECT_FALSE(interstitial_timer_observer.timer_started());
+
+  // Check that the histogram for the captive portal cert was recorded.
+  histograms.ExpectTotalCount(SSLErrorHandler::GetHistogramNameForTesting(), 3);
+  histograms.ExpectBucketCount(SSLErrorHandler::GetHistogramNameForTesting(),
+                               SSLErrorHandler::HANDLE_ALL, 1);
+  histograms.ExpectBucketCount(
+      SSLErrorHandler::GetHistogramNameForTesting(),
+      SSLErrorHandler::SHOW_CAPTIVE_PORTAL_INTERSTITIAL_OVERRIDABLE, 1);
+  histograms.ExpectBucketCount(SSLErrorHandler::GetHistogramNameForTesting(),
+                               SSLErrorHandler::CAPTIVE_PORTAL_CERT_FOUND, 1);
+}
+
+namespace {
+
+// Test class that mimics a URL request with a certificate whose SPKI hash is in
+// ssl_error_assistant.asciipb resource. A better way of testing the SPKI hashes
+// inside the resource bundle would be to serve the actual certificate from the
+// embeded test server, but the test server can only serve a limited number of

[estark, 2017/02/03 07:24:07]

typo: embeded -> embedded

[meacer, 2017/02/06 23:39:17]

Done.

+// predefined certificates.

[Ryan Sleevi, 2017/02/02 02:24:36]

You could use a MockCertVerifyProc to return any cert you want as a 'verified'
cert, but Emily may know more if that doesn't work :)

[estark, 2017/02/03 07:24:07]

I had originally suggested a MockCertVerifier to Mustafa, but I walked that back
because I had a vague memory that it was against the rules for a CertVerifier to
return a leaf cert that is different than the one passed in. (i.e. it would
violate the CertVerifyResult guarantee at [1])

But now that I'm looking in this file, I see pre-existing examples of
MockCertVerifiers returning certs different than what was passed in, so I guess
it's not a hard-and-fast rule.

[1]
https://cs.chromium.org/chromium/src/net/cert/cert_verify_result.h?q=CertVeri...

[meacer, 2017/02/06 23:39:17]

Converted the tests to use MockCertVerifier.

+template <int net_error>

[estark, 2017/02/03 07:24:07]

Just curious, why templatize the test fixture instead of making it a
parameterized test? Is it because the expectations are different depending on
the error so you don't want to choose different expectations based on the
parameter value?

[meacer, 2017/02/06 23:39:17]

Parametrizing this class added complexity: 
- If we keep this class as is and make the interceptor take a net_error
parameter, we'd need to hold a reference to the interceptor and post the desired
net_error parameter to the IO thread.
- The other option is to pass net_error to the test's constructor, which would
require two different test classes along with their constructors. The current
implementation avoids adding that extra code (it typedefs two classes instead,
saving a whopping 5 lines of code).

This is moot now that the test is using MockCertVerifier.

+class SSLUICaptivePortalTest : public InProcessBrowserTest {
+ public:
+  SSLUICaptivePortalTest() : InProcessBrowserTest(), cert_(nullptr) {}
+
+  void SetUpOnMainThread() override {
+    cert_ =
+        net::ImportCertFromFile(net::GetTestCertsDirectory(), "ok_cert.pem");
+    ASSERT_TRUE(cert_);
+    content::BrowserThread::PostTask(
+        content::BrowserThread::IO, FROM_HERE,
+        base::Bind(&SSLUICaptivePortalTest::AddUrlHandler, cert_));
+  }
+
+  static void AddUrlHandler(scoped_refptr<net::X509Certificate> cert) {
+    net::URLRequestFilter* filter = net::URLRequestFilter::GetInstance();
+    filter->AddHostnameInterceptor("https", kCaptivePortalHostname,
+                                   std::unique_ptr<net::URLRequestInterceptor>(
+                                       new CaptivePortalInterceptor(cert)));
+  }
+
+ private:
+  // A URLRequestJob that mocks a TLS connection with a certificate whose SPKI
+  // hash is marked as a known captive portal certificate SPKI.
+  class CaptivePortalURLRequestJob : public net::URLRequestJob {
+   public:
+    CaptivePortalURLRequestJob(net::URLRequest* request,
+                               net::NetworkDelegate* network_delegate,
+                               scoped_refptr<net::X509Certificate> cert)
+        : net::URLRequestJob(request, network_delegate),
+          cert_(std::move(cert)),
+          weak_factory_(this) {}
+
+    // net::URLRequestJob method:
+    void Start() override {
+      net::SSLInfo ssl_info;
+      ssl_info.SetCertError(net_error);
+      ssl_info.cert = cert_;
+
+      // Set the SPKI hash to captive-portal.badssl.com leaf certificate. This
+      // doesn't match the actual cert (ok_cert.pem) but is good enough for
+      // testing.
+      net::HashValue hash;
+      ASSERT_TRUE(hash.FromString(
+          "sha256/fjZPHewEHTrMDX3I1ecEIeoy3WFxHyGplOLv28kIbtI="));
+      ssl_info.public_key_hashes.push_back(hash);
+
+      base::ThreadTaskRunnerHandle::Get()->PostTask(
+          FROM_HERE,
+          base::Bind(&CaptivePortalURLRequestJob::NotifySSLCertificateError,
+                     weak_factory_.GetWeakPtr(), ssl_info, false));
+    }
+
+   protected:
+    ~CaptivePortalURLRequestJob() override {}
+
+   private:
+    const scoped_refptr<net::X509Certificate> cert_;
+    base::WeakPtrFactory<CaptivePortalURLRequestJob> weak_factory_;
+
+    DISALLOW_COPY_AND_ASSIGN(CaptivePortalURLRequestJob);
+  };
+
+  // A URLRequestInterceptor that handles requests with
+  // CaptivePortalURLRequestJob jobs.
+  class CaptivePortalInterceptor : public net::URLRequestInterceptor {
+   public:
+    CaptivePortalInterceptor(scoped_refptr<net::X509Certificate> cert)
+        : cert_(std::move(cert)) {}
+
+    ~CaptivePortalInterceptor() override {}
+
+    // net::URLRequestInterceptor method:
+    net::URLRequestJob* MaybeInterceptRequest(
+        net::URLRequest* request,
+        net::NetworkDelegate* network_delegate) const override {
+      return new CaptivePortalURLRequestJob(request, network_delegate, cert_);
+    }
+
+   private:
+    const scoped_refptr<net::X509Certificate> cert_;
+
+    DISALLOW_COPY_AND_ASSIGN(CaptivePortalInterceptor);
+  };
+
+  scoped_refptr<net::X509Certificate> cert_;
+};
+
+using SSLUICaptivePortalNameMismatchTest =
+    SSLUICaptivePortalTest<net::ERR_CERT_COMMON_NAME_INVALID>;
+using SSLUICaptivePortalAuthorityInvalidTest =
+    SSLUICaptivePortalTest<net::ERR_CERT_AUTHORITY_INVALID>;
+
+}  // namespace
+
+// Same as CaptivePortalCertificateList_Enabled_FromProto, but this time the
+// cert's SPKI hash is listed in ssl_error_assistant.asciipb.
+IN_PROC_BROWSER_TEST_F(SSLUICaptivePortalNameMismatchTest,
+                       CaptivePortalCertificateList_Enabled_FromResource) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitFromCommandLine(
+      "CaptivePortalCertificateList" /* enabled */, "" /* disabled */);
+
+  base::HistogramTester histograms;
+
+  // Navigate to an unsafe page on the server. The captive portal interstitial
+  // should be displayed since CaptivePortalCertificateList feature is enabled.
+  WebContents* tab = browser()->tab_strip_model()->GetActiveWebContents();
+  SSLInterstitialTimerObserver interstitial_timer_observer(tab);
+  ui_test_utils::NavigateToURL(
+      browser(), GURL(std::string("https://") + kCaptivePortalHostname));
+  content::WaitForInterstitialAttach(tab);
+
+  InterstitialPage* interstitial_page = tab->GetInterstitialPage();
+  ASSERT_EQ(CaptivePortalBlockingPage::kTypeForTesting,
+            interstitial_page->GetDelegateForTesting()->GetTypeForTesting());
+  EXPECT_FALSE(interstitial_timer_observer.timer_started());
+
+  // Check that the histogram for the captive portal cert was recorded.
+  histograms.ExpectTotalCount(SSLErrorHandler::GetHistogramNameForTesting(), 3);
+  histograms.ExpectBucketCount(SSLErrorHandler::GetHistogramNameForTesting(),
+                               SSLErrorHandler::HANDLE_ALL, 1);
+  histograms.ExpectBucketCount(
+      SSLErrorHandler::GetHistogramNameForTesting(),
+      SSLErrorHandler::SHOW_CAPTIVE_PORTAL_INTERSTITIAL_OVERRIDABLE, 1);
+  histograms.ExpectBucketCount(SSLErrorHandler::GetHistogramNameForTesting(),
+                               SSLErrorHandler::CAPTIVE_PORTAL_CERT_FOUND, 1);
+}
+
+// Same as SSLUICaptivePortalNameMismatchTest, but this time the error is
+// authority-invalid. Captive portal interstitial should not be shown.
+IN_PROC_BROWSER_TEST_F(SSLUICaptivePortalAuthorityInvalidTest,
+                       CaptivePortalCertificateList_Enabled_FromResource) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitFromCommandLine(
+      "CaptivePortalCertificateList" /* enabled */, "" /* disabled */);
+
+  // Set interstitial delay to zero.
+  SSLErrorHandler::SetInterstitialDelayForTesting(base::TimeDelta());
+
+  base::HistogramTester histograms;
+  // Navigate to an unsafe page on the server. CaptivePortalCertificateList
+  // feature is enabled but the error is not a name mismatch, so a generic SSL
+  // interstitial should be displayed.
+  WebContents* tab = browser()->tab_strip_model()->GetActiveWebContents();
+  SSLInterstitialTimerObserver interstitial_timer_observer(tab);
+  ui_test_utils::NavigateToURL(
+      browser(), GURL(std::string("https://") + kCaptivePortalHostname));
+  content::WaitForInterstitialAttach(tab);
+
+  InterstitialPage* interstitial_page = tab->GetInterstitialPage();
+  ASSERT_EQ(SSLBlockingPage::kTypeForTesting,
+            interstitial_page->GetDelegateForTesting()->GetTypeForTesting());
+  EXPECT_TRUE(interstitial_timer_observer.timer_started());
+
+  // Check that the histogram for the captive portal cert was recorded.
+  histograms.ExpectTotalCount(SSLErrorHandler::GetHistogramNameForTesting(), 2);
+  histograms.ExpectBucketCount(SSLErrorHandler::GetHistogramNameForTesting(),
+                               SSLErrorHandler::HANDLE_ALL, 1);
+  histograms.ExpectBucketCount(
+      SSLErrorHandler::GetHistogramNameForTesting(),
+      SSLErrorHandler::SHOW_SSL_INTERSTITIAL_OVERRIDABLE, 1);
+}
+
+#endif  // BUILDFLAG(ENABLE_CAPTIVE_PORTAL_DETECTION)
+
 // TODO(jcampan): more tests to do below.
 
 // Visit a page over https that contains a frame with a redirect.