Captive portal certificate list should be checked when name mismatch is the only error

chrome/browser/ssl/ssl_error_handler_unittest.cc

 // Copyright 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.
 
 #include "chrome/browser/ssl/ssl_error_handler.h"
 
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/field_trial.h"
@@ skipping 173 matching lines @@
   bool captive_portal_interstitial_shown_;
   bool redirected_to_suggested_url_;
   bool is_overridable_error_;
   CommonNameMismatchHandler::CheckUrlCallback suggested_url_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(TestSSLErrorHandlerDelegate);
 };
 
 }  // namespace
 
-template <net::CertStatus cert_status>
-class SSLErrorHandlerCertStatusTestBase
-    : public ChromeRenderViewHostTestHarness {
+// A class to test name mismatch errors. Creates an error handler with a name
+// mismatch error.
+class SSLErrorHandlerNameMismatchTest : public ChromeRenderViewHostTestHarness {
  public:
-  SSLErrorHandlerCertStatusTestBase() : field_trial_list_(nullptr) {}
+  SSLErrorHandlerNameMismatchTest() : field_trial_list_(nullptr) {}
 
   void SetUp() override {
     ChromeRenderViewHostTestHarness::SetUp();
     SSLErrorHandler::ResetConfigForTesting();
     SSLErrorHandler::SetInterstitialDelayForTesting(base::TimeDelta());
+    ssl_info_.Reset();
     ssl_info_.cert =
         net::ImportCertFromFile(net::GetTestCertsDirectory(), "ok_cert.pem");
-    ssl_info_.cert_status = cert_status;
+    ssl_info_.cert_status = net::CERT_STATUS_COMMON_NAME_INVALID;
     ssl_info_.public_key_hashes.push_back(
         net::HashValue(kCertPublicKeyHashValue));
 
     delegate_ =
         new TestSSLErrorHandlerDelegate(profile(), web_contents(), ssl_info_);
     error_handler_.reset(new TestSSLErrorHandler(
         std::unique_ptr<SSLErrorHandler::Delegate>(delegate_), web_contents(),
         profile(), net::MapCertStatusToNetError(ssl_info_.cert_status),
         ssl_info_,
         GURL(),  // request_url
         base::Callback<void(content::CertificateRequestResultType)>()));
-
-    // Enable finch experiment for captive portal interstitials.
-    ASSERT_TRUE(base::FieldTrialList::CreateFieldTrial(
-                    "CaptivePortalInterstitial", "Enabled"));
-    // Enable finch experiment for SSL common name mismatch handling.
-    ASSERT_TRUE(base::FieldTrialList::CreateFieldTrial(
-                    "SSLCommonNameMismatchHandling", "Enabled"));
   }
 
   void TearDown() override {
+    EXPECT_FALSE(error_handler()->IsTimerRunningForTesting());
+    error_handler_.reset(nullptr);
+    SSLErrorHandler::ResetConfigForTesting();
+    ChromeRenderViewHostTestHarness::TearDown();
+  }
+
+  TestSSLErrorHandler* error_handler() { return error_handler_.get(); }
+  TestSSLErrorHandlerDelegate* delegate() { return delegate_; }
+
+  const net::SSLInfo& ssl_info() { return ssl_info_; }
+
+ private:
+  net::SSLInfo ssl_info_;
+  std::unique_ptr<TestSSLErrorHandler> error_handler_;
+  TestSSLErrorHandlerDelegate* delegate_;
+  base::FieldTrialList field_trial_list_;
+
+  DISALLOW_COPY_AND_ASSIGN(SSLErrorHandlerNameMismatchTest);
+};
+
+// A class to test captive portal certificate list. Creates an error handler
+// with a name mismatch error by default. The error handler can be recreated by
+// calling ResetErrorHandler() with an appropriate cert status.
+class SSLErrorHandlerCaptivePortalCertListTest
+    : public ChromeRenderViewHostTestHarness {
+ public:
+  SSLErrorHandlerCaptivePortalCertListTest() : field_trial_list_(nullptr) {}
+
+  void SetUp() override {
+    ChromeRenderViewHostTestHarness::SetUp();
+    SSLErrorHandler::ResetConfigForTesting();
+    SSLErrorHandler::SetInterstitialDelayForTesting(base::TimeDelta());
+    ResetErrorHandler(net::CERT_STATUS_COMMON_NAME_INVALID);
+  }
+
+  void TearDown() override {
     EXPECT_FALSE(error_handler()->IsTimerRunningForTesting());
     error_handler_.reset(nullptr);
     SSLErrorHandler::ResetConfigForTesting();
     ChromeRenderViewHostTestHarness::TearDown();
   }
 
   TestSSLErrorHandler* error_handler() { return error_handler_.get(); }
   TestSSLErrorHandlerDelegate* delegate() { return delegate_; }
 
   const net::SSLInfo& ssl_info() { return ssl_info_; }
 
+ protected:
+  void SetFeatureEnabled(bool enabled) {
+    if (enabled) {
+      scoped_feature_list_.InitFromCommandLine(
+          "CaptivePortalCertificateList" /* enabled */,
+          std::string() /* disabled */);
+    } else {
+      scoped_feature_list_.InitFromCommandLine(
+          std::string(), "CaptivePortalCertificateList" /* disabled */);
+    }
+  }
+
+  // Deletes the current error handler and creates a new one with the given
+  // |cert_status|.
+  void ResetErrorHandler(net::CertStatus cert_status) {
+    ssl_info_.Reset();
+    ssl_info_.cert =
+        net::ImportCertFromFile(net::GetTestCertsDirectory(), "ok_cert.pem");
+    ssl_info_.cert_status = cert_status;
+    ssl_info_.public_key_hashes.push_back(
+        net::HashValue(kCertPublicKeyHashValue));
+
+    delegate_ =
+        new TestSSLErrorHandlerDelegate(profile(), web_contents(), ssl_info_);
+    error_handler_.reset(new TestSSLErrorHandler(
+        std::unique_ptr<SSLErrorHandler::Delegate>(delegate_), web_contents(),
+        profile(), net::MapCertStatusToNetError(ssl_info_.cert_status),
+        ssl_info_,
+        GURL(),  // request_url
+        base::Callback<void(content::CertificateRequestResultType)>()));
+
+    // Enable finch experiment for captive portal interstitials.
+    ASSERT_TRUE(base::FieldTrialList::CreateFieldTrial(
+        "CaptivePortalInterstitial", "Enabled"));
+    // Enable finch experiment for SSL common name mismatch handling.
+    ASSERT_TRUE(base::FieldTrialList::CreateFieldTrial(
+        "SSLCommonNameMismatchHandling", "Enabled"));
+  }
+
+  void TestNoCaptivePortalInterstitial() {
+    base::HistogramTester histograms;
+
+    EXPECT_FALSE(error_handler()->IsTimerRunningForTesting());
+    EXPECT_EQ(1u, ssl_info().public_key_hashes.size());
+
+    auto config_proto =
+        base::MakeUnique<chrome_browser_ssl::SSLErrorAssistantConfig>();
+    config_proto->add_captive_portal_cert()->set_sha256_hash(
+        "sha256/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa");
+    config_proto->add_captive_portal_cert()->set_sha256_hash(
+        ssl_info().public_key_hashes[0].ToString());
+    config_proto->add_captive_portal_cert()->set_sha256_hash(
+        "sha256/bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb");
+    SSLErrorHandler::SetErrorAssistantProto(std::move(config_proto));
+
+    error_handler()->StartHandlingError();
+
+    // Timer should start for captive portal detection.
+    EXPECT_TRUE(error_handler()->IsTimerRunningForTesting());
+    EXPECT_TRUE(delegate()->captive_portal_checked());
+    EXPECT_FALSE(delegate()->ssl_interstitial_shown());
+    EXPECT_FALSE(delegate()->captive_portal_interstitial_shown());
+    EXPECT_FALSE(delegate()->suggested_url_checked());
+
+    base::RunLoop().RunUntilIdle();
+
+    EXPECT_FALSE(error_handler()->IsTimerRunningForTesting());
+    EXPECT_TRUE(delegate()->captive_portal_checked());
+    EXPECT_TRUE(delegate()->ssl_interstitial_shown());
+    EXPECT_FALSE(delegate()->captive_portal_interstitial_shown());
+    EXPECT_FALSE(delegate()->suggested_url_checked());
+
+    // 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);
+  }
+
  private:
   net::SSLInfo ssl_info_;
   std::unique_ptr<TestSSLErrorHandler> error_handler_;
   TestSSLErrorHandlerDelegate* delegate_;
   base::FieldTrialList field_trial_list_;
+  base::test::ScopedFeatureList scoped_feature_list_;
 
-  DISALLOW_COPY_AND_ASSIGN(SSLErrorHandlerCertStatusTestBase);
+  DISALLOW_COPY_AND_ASSIGN(SSLErrorHandlerCaptivePortalCertListTest);
 };
 
-using SSLErrorHandlerNameMismatchTest =
-    SSLErrorHandlerCertStatusTestBase<net::CERT_STATUS_COMMON_NAME_INVALID>;
-using SSLErrorHandlerAuthorityInvalidTest =
-    SSLErrorHandlerCertStatusTestBase<net::CERT_STATUS_AUTHORITY_INVALID>;
 
 class SSLErrorHandlerDateInvalidTest : public ChromeRenderViewHostTestHarness {
  public:
   SSLErrorHandlerDateInvalidTest()
       : field_trial_test_(new network_time::FieldTrialTest()),
         clock_(new base::SimpleTestClock),
         tick_clock_(new base::SimpleTestTickClock),
         test_server_(new net::EmbeddedTestServer) {
     SetThreadBundleOptions(content::TestBrowserThreadBundle::REAL_IO_THREAD);
     network_time::NetworkTimeTracker::RegisterPrefs(pref_service_.registry());
@@ skipping 434 matching lines @@
   ClearErrorHandler();
 
   // Shut down the server to cancel the pending request.
   ASSERT_TRUE(test_server()->ShutdownAndWaitUntilComplete());
 }
 
 #if BUILDFLAG(ENABLE_CAPTIVE_PORTAL_DETECTION)
 
 // Tests that a certificate marked as a known captive portal certificate causes
 // the captive portal interstitial to be shown.
-TEST_F(SSLErrorHandlerNameMismatchTest, CaptivePortalCertificateList_Enabled) {
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitFromCommandLine(
-      "CaptivePortalCertificateList" /* enabled */,
-      std::string() /* disabled */);
+TEST_F(SSLErrorHandlerCaptivePortalCertListTest, Enabled) {
+  SetFeatureEnabled(true);
 
   EXPECT_FALSE(error_handler()->IsTimerRunningForTesting());
   EXPECT_EQ(1u, ssl_info().public_key_hashes.size());
 
   auto config_proto =
       base::MakeUnique<chrome_browser_ssl::SSLErrorAssistantConfig>();
   config_proto->add_captive_portal_cert()->set_sha256_hash(
       "sha256/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa");
   config_proto->add_captive_portal_cert()->set_sha256_hash(
       ssl_info().public_key_hashes[0].ToString());
@@ skipping 28 matching lines @@
   histograms.ExpectBucketCount(
       SSLErrorHandler::GetHistogramNameForTesting(),
       SSLErrorHandler::SHOW_CAPTIVE_PORTAL_INTERSTITIAL_OVERRIDABLE, 1);
   histograms.ExpectBucketCount(SSLErrorHandler::GetHistogramNameForTesting(),
                                SSLErrorHandler::CAPTIVE_PORTAL_CERT_FOUND, 1);
 }
 
 // Tests that a certificate marked as a known captive portal certificate does
 // not cause the captive portal interstitial to be shown, if the feature is
 // disabled.
-TEST_F(SSLErrorHandlerNameMismatchTest, CaptivePortalCertificateList_Disabled) {
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitFromCommandLine(
-      std::string() /* enabled */,
-      "CaptivePortalCertificateList" /* disabled */);
+TEST_F(SSLErrorHandlerCaptivePortalCertListTest, Disabled) {
+  SetFeatureEnabled(false);
 
-  base::HistogramTester histograms;
-
-  EXPECT_FALSE(error_handler()->IsTimerRunningForTesting());
-  EXPECT_EQ(1u, ssl_info().public_key_hashes.size());
-
-  auto config_proto =
-      base::MakeUnique<chrome_browser_ssl::SSLErrorAssistantConfig>();
-  config_proto->add_captive_portal_cert()->set_sha256_hash(
-      "sha256/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa");
-  config_proto->add_captive_portal_cert()->set_sha256_hash(
-      ssl_info().public_key_hashes[0].ToString());
-  config_proto->add_captive_portal_cert()->set_sha256_hash(
-      "sha256/bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb");
-  SSLErrorHandler::SetErrorAssistantProto(std::move(config_proto));
-
-  error_handler()->StartHandlingError();
-
-  // Timer should start since captive portal certificate list feature is
-  // disabled.
-  EXPECT_TRUE(error_handler()->IsTimerRunningForTesting());
-  EXPECT_TRUE(delegate()->captive_portal_checked());
-  EXPECT_FALSE(delegate()->ssl_interstitial_shown());
-  EXPECT_FALSE(delegate()->captive_portal_interstitial_shown());
-  EXPECT_FALSE(delegate()->suggested_url_checked());
-
-  // A buggy SSL error handler might have incorrectly started the timer. Run to
-  // completion to ensure the timer is expired.
-  base::RunLoop().RunUntilIdle();
-
-  EXPECT_FALSE(error_handler()->IsTimerRunningForTesting());
-  EXPECT_TRUE(delegate()->captive_portal_checked());
-  EXPECT_TRUE(delegate()->ssl_interstitial_shown());
-  EXPECT_FALSE(delegate()->captive_portal_interstitial_shown());
-  EXPECT_FALSE(delegate()->suggested_url_checked());
-
-  // 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);
+  // Default error for SSLErrorHandlerNameMismatchTest tests is name mismatch.
+  TestNoCaptivePortalInterstitial();
 }
 
 // Tests that an error other than name mismatch does not cause a captive portal
 // interstitial to be shown, even if the certificate is marked as a known
 // captive portal certificate.
-TEST_F(SSLErrorHandlerAuthorityInvalidTest,
-       CaptivePortalCertificateList_ShouldShowGenericInterstitial) {
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitFromCommandLine(
-      "CaptivePortalCertificateList" /* enabled */,
-      std::string() /* disabled */);
+TEST_F(SSLErrorHandlerCaptivePortalCertListTest, AuthorityInvalid) {
+  SetFeatureEnabled(true);
 
-  base::HistogramTester histograms;
+  ResetErrorHandler(net::ERR_CERT_AUTHORITY_INVALID);
+  TestNoCaptivePortalInterstitial();
+}
 
-  EXPECT_FALSE(error_handler()->IsTimerRunningForTesting());
-  EXPECT_EQ(1u, ssl_info().public_key_hashes.size());
+// Tests that an authority invalid error in addition to name mismatch error does
+// not cause a captive portal interstitial to be shown, even if the certificate
+// is marked as a known captive portal certificate. The resulting error is
+// authority-invalid.
+TEST_F(SSLErrorHandlerCaptivePortalCertListTest,
+       NameMismatchAndAuthorityInvalid) {
+  SetFeatureEnabled(true);
 
-  auto config_proto =
-      base::MakeUnique<chrome_browser_ssl::SSLErrorAssistantConfig>();
-  config_proto->add_captive_portal_cert()->set_sha256_hash(
-      "sha256/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa");
-  config_proto->add_captive_portal_cert()->set_sha256_hash(
-      ssl_info().public_key_hashes[0].ToString());
-  config_proto->add_captive_portal_cert()->set_sha256_hash(
-      "sha256/bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb");
-  SSLErrorHandler::SetErrorAssistantProto(std::move(config_proto));
+  // Sanity check that AUTHORITY_INVALID is seen as the net error.

[estark, 2017/02/28 21:12:06]

same optional nit

[meacer, 2017/02/28 23:57:00]

Done.

+  const net::CertStatus cert_status =
+      net::CERT_STATUS_COMMON_NAME_INVALID | net::CERT_STATUS_AUTHORITY_INVALID;
+  ASSERT_EQ(net::ERR_CERT_AUTHORITY_INVALID,
+            net::MapCertStatusToNetError(cert_status));
+  ResetErrorHandler(cert_status);
+  TestNoCaptivePortalInterstitial();
+}
 
-  error_handler()->StartHandlingError();
+// Tests that another error in addition to name mismatch error does not cause a
+// captive portal interstitial to be shown, even if the certificate is marked as
+// a known captive portal certificate. Similar to
+// NameMismatchAndAuthorityInvalid, except the resulting error is name mismatch.
+TEST_F(SSLErrorHandlerCaptivePortalCertListTest, NameMismatchAndWeakKey) {
+  SetFeatureEnabled(true);
 
-  // Timer should start for captive portal detection.
-  EXPECT_TRUE(error_handler()->IsTimerRunningForTesting());
-  EXPECT_TRUE(delegate()->captive_portal_checked());
-  EXPECT_FALSE(delegate()->ssl_interstitial_shown());
-  EXPECT_FALSE(delegate()->captive_portal_interstitial_shown());
-  EXPECT_FALSE(delegate()->suggested_url_checked());
+  // Sanity check that COMMON_NAME_INVALID is seen as the net error, since the

[estark, 2017/02/28 21:12:06]

ditto

[meacer, 2017/02/28 23:57:01]

Done.

+  // test is designed to verify that SSLErrorHandler notices other errors in the
+  // CertStatus even when COMMON_NAME_INVALID is the net error.
+  const net::CertStatus cert_status =
+      net::CERT_STATUS_COMMON_NAME_INVALID | net::CERT_STATUS_WEAK_KEY;
+  ASSERT_EQ(net::ERR_CERT_COMMON_NAME_INVALID,
+            net::MapCertStatusToNetError(cert_status));
+  ResetErrorHandler(cert_status);
+  TestNoCaptivePortalInterstitial();
+}
 
-  base::RunLoop().RunUntilIdle();
+#else
 
-  EXPECT_FALSE(error_handler()->IsTimerRunningForTesting());
-  EXPECT_TRUE(delegate()->captive_portal_checked());
-  EXPECT_TRUE(delegate()->ssl_interstitial_shown());
-  EXPECT_FALSE(delegate()->captive_portal_interstitial_shown());
-  EXPECT_FALSE(delegate()->suggested_url_checked());
+TEST_F(SSLErrorHandlerCaptivePortalCertListTest, DisabledByBuild) {
+  SetFeatureEnabled(true);
 
-  // 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);
+  // Default error for SSLErrorHandlerNameMismatchTest tests is name mismatch,
+  // but the feature is disabled by build so no captive portal interstitial will
+  // be displayed.
+  TestNoCaptivePortalInterstitial();
 }
 
 #endif  // BUILDFLAG(ENABLE_CAPTIVE_PORTAL_DETECTION)