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

chrome/browser/ssl/ssl_browser_tests.cc

 // Copyright (c) 2012 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 <utility>
 
 #include "base/base_switches.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
@@ skipping 4024 matching lines @@
 // 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
 // embedded test server, but the test server can only serve a limited number of
 // predefined certificates.
 class SSLUICaptivePortalListResourceBundleTest
     : public CertVerifierBrowserTest {
  public:
   SSLUICaptivePortalListResourceBundleTest()
       : CertVerifierBrowserTest(),
-        https_server_(net::EmbeddedTestServer::TYPE_HTTPS),
-        https_server_mismatched_(net::EmbeddedTestServer::TYPE_HTTPS) {
+        https_server_(net::EmbeddedTestServer::TYPE_HTTPS) {
     https_server_.ServeFilesFromSourceDirectory(base::FilePath(kDocRoot));
-
-    https_server_mismatched_.SetSSLConfig(
-        net::EmbeddedTestServer::CERT_MISMATCHED_NAME);
-    https_server_mismatched_.AddDefaultHandlers(base::FilePath(kDocRoot));
   }
 
   void SetUp() override {
     CertVerifierBrowserTest::SetUp();
     SSLErrorHandler::ResetConfigForTesting();
     SetUpCertVerifier(0, net::OK, std::string());
   }
 
   void TearDown() override {
     SSLErrorHandler::ResetConfigForTesting();
     CertVerifierBrowserTest::TearDown();
   }
 
  protected:
+  // Checks that a captive portal interstitial isn't displayed, even though the
+  // server's certificate is marked as a captive portal certificate.
+  void TestNoCaptivePortalInterstitial(net::CertStatus cert_status,
+                                       int net_error) {
+    ASSERT_TRUE(https_server()->Start());
+    base::HistogramTester histograms;
+
+    // Mark the server's cert as a captive portal cert.
+    SetUpCertVerifier(cert_status, net_error, kCaptivePortalSPKI);
+
+    // Navigate to an unsafe page on the server. CaptivePortalCertificateList
+    // feature is enabled but either the error is not name-mismatch, or it's not
+    // the only error, 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(), https_server()->GetURL("/"));
+    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);
+  }
+
   void SetUpCertVerifier(net::CertStatus cert_status,
                          int net_result,
                          const std::string& spki_hash) {
     scoped_refptr<net::X509Certificate> cert(https_server_.GetCertificate());
     net::CertVerifyResult verify_result;
     verify_result.is_issued_by_known_root =
         (net_result != net::ERR_CERT_AUTHORITY_INVALID);
     verify_result.verified_cert = cert;
     verify_result.cert_status = cert_status;
 
     // Set the SPKI hash to captive-portal.badssl.com leaf certificate.
     if (!spki_hash.empty()) {
       net::HashValue hash;
       ASSERT_TRUE(hash.FromString(spki_hash));
       verify_result.public_key_hashes.push_back(hash);
     }
     mock_cert_verifier()->AddResultForCert(cert, verify_result, net_result);
   }
 
   net::EmbeddedTestServer* https_server() { return &https_server_; }
-  net::EmbeddedTestServer* https_server_mismatched() {
-    return &https_server_mismatched_;
-  }
 
  private:
   net::EmbeddedTestServer https_server_;
-  net::EmbeddedTestServer https_server_mismatched_;
 };
 
 }  // 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(SSLUICaptivePortalListResourceBundleTest, Enabled) {
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.InitFromCommandLine(
       "CaptivePortalCertificateList" /* enabled */,
@@ skipping 150 matching lines @@
 }
 
 // Same as SSLUICaptivePortalNameMismatchTest, but this time the error is
 // authority-invalid. Captive portal interstitial should not be shown.
 IN_PROC_BROWSER_TEST_F(SSLUICaptivePortalListResourceBundleTest,
                        Enabled_AuthorityInvalid) {
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.InitFromCommandLine(
       "CaptivePortalCertificateList" /* enabled */,
       std::string() /* disabled */);
-  ASSERT_TRUE(https_server()->Start());
-  base::HistogramTester histograms;
 
-  // Mark the server's cert as a captive portal cert, but with an
-  // authority-invalid error.
-  SetUpCertVerifier(net::CERT_STATUS_AUTHORITY_INVALID,
-                    net::ERR_CERT_AUTHORITY_INVALID, kCaptivePortalSPKI);
+  TestNoCaptivePortalInterstitial(net::CERT_STATUS_AUTHORITY_INVALID,
+                                  net::ERR_CERT_AUTHORITY_INVALID);
+}
 
-  // 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(), https_server()->GetURL("/"));
-  content::WaitForInterstitialAttach(tab);
+// Same as SSLUICaptivePortalListResourceBundleTest.Enabled_AuthorityInvalid,
+// but this time there are two errors (name mismatch + weak key). Captive portal
+// interstitial should not be shown when name mismatch isn't the only error.
+IN_PROC_BROWSER_TEST_F(SSLUICaptivePortalListResourceBundleTest,
+                       Enabled_NameMismatchAndWeakKey) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitFromCommandLine(
+      "CaptivePortalCertificateList" /* enabled */,
+      std::string() /* disabled */);
 
-  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);
+  TestNoCaptivePortalInterstitial(
+      net::CERT_STATUS_COMMON_NAME_INVALID | net::CERT_STATUS_WEAK_KEY,

[estark, 2017/02/15 00:31:41]

nit: it might be good to have a sanity check that MapCertStatusToNetError()
returns COMMON_NAME_INVALID. In other words make sure that SSLErrorHandler is
looking at all the errors, not just the most serious one.

[meacer, 2017/02/15 00:37:29]

Hmm, I thought about checking for name mismatch error through
SSLErrorHandler::cert_error but there is no way to access the SSLErrorHandler
instance.

Just to understand your suggestion, are you saying we should check
MapCertStatusToNetError output here?
I'm not sure I understand this part. How would MapCertStatusToNetError returning
COMMON_NAME_INVALID ensure that?

[meacer, 2017/02/23 23:28:19]

Done.

+      net::ERR_CERT_COMMON_NAME_INVALID);
 }
 
 #else
 
 // Tests that the captive portal certificate list is not used when captive
 // portal checks are disabled by build, even if the captive portal certificate
 // list feature is enabled via Finch. The list is passed to SSLErrorHandler via
 // a proto.
 IN_PROC_BROWSER_TEST_F(SSLUICaptivePortalListTest, PortalChecksDisabled) {
   base::test::ScopedFeatureList scoped_feature_list;
@@ skipping 41 matching lines @@
 
 // Visit a page over https that contains a frame with a redirect.
 
 // XMLHttpRequest insecure content in synchronous mode.
 
 // XMLHttpRequest insecure content in asynchronous mode.
 
 // XMLHttpRequest over bad ssl in synchronous mode.
 
 // XMLHttpRequest over OK ssl in synchronous mode.