Flip default state when legacy TLS config not set

This will make all sites be treated as "control" (no warnings shown)
when the control site config has not yet been loaded. This will prevent
control sites from accidentally showing the warnings, at the expense of
occasionally not showing warnings on non-control sites near browser
startup time.

Bug: 1011089
Change-Id: I32ebb7affc0d0a4bc2a3f6b280c9e5943f9118eb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1845488
Commit-Queue: Christopher Thompson <cthomp@chromium.org>
Reviewed-by: Emily Stark <estark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#704904}

chrome/browser/ssl/security_state_tab_helper.cc

@@ -140,15 +140,16 @@ SecurityStateTabHelper::GetVisibleSecurityState() {
       // entry.
       int navigation_id =
           web_contents()->GetController().GetVisibleEntry()->GetUniqueID();
-      if (cached_is_legacy_tls_control_site_ &&
-          cached_is_legacy_tls_control_site_.value().first == navigation_id) {
-        state->is_legacy_tls_control_site =
-            cached_is_legacy_tls_control_site_.value().second;
+      if (cached_should_suppress_legacy_tls_warning_ &&
+          cached_should_suppress_legacy_tls_warning_.value().first ==
+              navigation_id) {
+        state->should_suppress_legacy_tls_warning =
+            cached_should_suppress_legacy_tls_warning_.value().second;
       } else {
-        state->is_legacy_tls_control_site =
-            IsTLSDeprecationConfigControlSite(state->url);
-        cached_is_legacy_tls_control_site_ = std::pair<int, bool>(
-            navigation_id, state->is_legacy_tls_control_site);
+        state->should_suppress_legacy_tls_warning =
+            ShouldSuppressLegacyTLSWarning(state->url);
+        cached_should_suppress_legacy_tls_warning_ = std::pair<int, bool>(
+            navigation_id, state->should_suppress_legacy_tls_warning);
       }
     }
   }

chrome/browser/ssl/security_state_tab_helper.h

@@ -45,15 +45,14 @@ class SecurityStateTabHelper
   bool UsedPolicyInstalledCertificate() const;
   security_state::MaliciousContentStatus GetMaliciousContentStatus() const;
 
-  // Caches the legacy TLS control site status for the duration of a page load
-  // (bound to a specific navigation ID) to ensure that we show consistent
+  // Caches the legacy TLS warning suppression status for the duration of a page
+  // load (bound to a specific navigation ID) to ensure that we show consistent
   // security UI (e.g., security indicator and page info). This is because the
-  // control site status depends on external state (a component loading from
-  // disk), which can cause inconsistent state across a page load if it isn't
-  // cached.
+  // status depends on external state (a component loading from disk), which can
+  // cause inconsistent state across a page load if it isn't cached.
   base::Optional<std::pair<int /* navigation entry ID */,
-                           bool /* is_legacy_tls_control_site */>>
-      cached_is_legacy_tls_control_site_;
+                           bool /* should_suppress_legacy_tls_warning */>>
+      cached_should_suppress_legacy_tls_warning_;
 
   WEB_CONTENTS_USER_DATA_KEY_DECL();
 

chrome/browser/ssl/security_state_tab_helper_browsertest.cc

@@ -1871,6 +1871,26 @@ IN_PROC_BROWSER_TEST_F(
   EXPECT_NE(std::string::npos, explanation.recommendations[1].find("GCM"));
 }
 
+// Tests that a connection with legacy TLS version (TLS 1.0/1.1) is not
+// downgraded to SecurityLevel WARNING if no config proto is set (i.e., so we
+// don't accidentally show the warning on control sites, see crbug.com/1011089).
+IN_PROC_BROWSER_TEST_F(BrowserTestNonsecureURLRequest, LegacyTLSNoProto) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(
+      security_state::features::kLegacyTLSWarnings);
+
+  auto* tab = browser()->tab_strip_model()->GetActiveWebContents();
+  auto* helper = SecurityStateTabHelper::FromWebContents(tab);
+
+  ui_test_utils::NavigateToURL(
+      browser(), GURL(std::string("https://") + kMockNonsecureHostname));
+
+  EXPECT_TRUE(helper->GetVisibleSecurityState()->connection_used_legacy_tls);
+  EXPECT_TRUE(
+      helper->GetVisibleSecurityState()->should_suppress_legacy_tls_warning);
+  EXPECT_EQ(security_state::SECURE, helper->GetSecurityLevel());
+}
+
 // Tests that a connection with legacy TLS versions (TLS 1.0/1.1) gets
 // downgraded to SecurityLevel WARNING and |connection_used_legacy_tls| is set.
 IN_PROC_BROWSER_TEST_F(BrowserTestNonsecureURLRequest,
@@ -1879,6 +1899,11 @@ IN_PROC_BROWSER_TEST_F(BrowserTestNonsecureURLRequest,
   scoped_feature_list.InitAndEnableFeature(
       security_state::features::kLegacyTLSWarnings);
 
+  // Set an empty config (otherwise all sites are treated as control).
+  auto config =
+      std::make_unique<chrome_browser_ssl::LegacyTLSExperimentConfig>();
+  SetRemoteTLSDeprecationConfigProto(std::move(config));
+
   auto* tab = browser()->tab_strip_model()->GetActiveWebContents();
   auto* helper = SecurityStateTabHelper::FromWebContents(tab);
 
@@ -1886,7 +1911,8 @@ IN_PROC_BROWSER_TEST_F(BrowserTestNonsecureURLRequest,
       browser(), GURL(std::string("https://") + kMockNonsecureHostname));
 
   EXPECT_TRUE(helper->GetVisibleSecurityState()->connection_used_legacy_tls);
-  EXPECT_FALSE(helper->GetVisibleSecurityState()->is_legacy_tls_control_site);
+  EXPECT_FALSE(
+      helper->GetVisibleSecurityState()->should_suppress_legacy_tls_warning);
   EXPECT_EQ(security_state::WARNING, helper->GetSecurityLevel());
 }
 
@@ -1911,14 +1937,15 @@ IN_PROC_BROWSER_TEST_F(BrowserTestNonsecureURLRequest,
   ui_test_utils::NavigateToURL(browser(), control_site);
 
   EXPECT_TRUE(helper->GetVisibleSecurityState()->connection_used_legacy_tls);
-  EXPECT_TRUE(helper->GetVisibleSecurityState()->is_legacy_tls_control_site);
+  EXPECT_TRUE(
+      helper->GetVisibleSecurityState()->should_suppress_legacy_tls_warning);
   EXPECT_EQ(helper->GetSecurityLevel(), security_state::SECURE);
 
   // Reset the config to be empty.
   auto empty_config =
       std::make_unique<chrome_browser_ssl::LegacyTLSExperimentConfig>();
   SetRemoteTLSDeprecationConfigProto(std::move(empty_config));
-  ASSERT_FALSE(IsTLSDeprecationConfigControlSite(control_site));
+  ASSERT_FALSE(ShouldSuppressLegacyTLSWarning(control_site));
 }
 
 // Tests that the SSLVersionMin policy can disable the Legacy TLS security
@@ -1942,7 +1969,8 @@ IN_PROC_BROWSER_TEST_F(BrowserTestNonsecureURLRequest,
       browser(), GURL(std::string("https://") + kMockNonsecureHostname));
 
   EXPECT_FALSE(helper->GetVisibleSecurityState()->connection_used_legacy_tls);
-  EXPECT_FALSE(helper->GetVisibleSecurityState()->is_legacy_tls_control_site);
+  EXPECT_FALSE(
+      helper->GetVisibleSecurityState()->should_suppress_legacy_tls_warning);
   EXPECT_EQ(helper->GetSecurityLevel(), security_state::SECURE);
 
   g_browser_process->local_state()->SetString(prefs::kSSLVersionMin,
@@ -1965,26 +1993,28 @@ IN_PROC_BROWSER_TEST_F(BrowserTestNonsecureURLRequest,
   ui_test_utils::NavigateToURL(browser(), control_site);
 
   EXPECT_TRUE(helper->GetVisibleSecurityState()->connection_used_legacy_tls);
-  EXPECT_FALSE(helper->GetVisibleSecurityState()->is_legacy_tls_control_site);
-  EXPECT_EQ(helper->GetSecurityLevel(), security_state::WARNING);
+  EXPECT_TRUE(
+      helper->GetVisibleSecurityState()->should_suppress_legacy_tls_warning);
+  EXPECT_EQ(helper->GetSecurityLevel(), security_state::SECURE);
 
   // Set the config proto.
   auto config =
       std::make_unique<chrome_browser_ssl::LegacyTLSExperimentConfig>();
-  config->add_control_site_hashes(kMockControlSiteHash);
   SetRemoteTLSDeprecationConfigProto(std::move(config));
 
   // Security state for the current page should not change.
   EXPECT_TRUE(helper->GetVisibleSecurityState()->connection_used_legacy_tls);
-  EXPECT_FALSE(helper->GetVisibleSecurityState()->is_legacy_tls_control_site);
-  EXPECT_EQ(helper->GetSecurityLevel(), security_state::WARNING);
+  EXPECT_TRUE(
+      helper->GetVisibleSecurityState()->should_suppress_legacy_tls_warning);
+  EXPECT_EQ(helper->GetSecurityLevel(), security_state::SECURE);
 
-  // Refreshing the page should update the page's security state to now be
-  // treated as control group.
+  // Refreshing the page should update the page's security state to now show a
+  // warning.
   ui_test_utils::NavigateToURL(browser(), control_site);
   EXPECT_TRUE(helper->GetVisibleSecurityState()->connection_used_legacy_tls);
-  EXPECT_TRUE(helper->GetVisibleSecurityState()->is_legacy_tls_control_site);
-  EXPECT_EQ(helper->GetSecurityLevel(), security_state::SECURE);
+  EXPECT_FALSE(
+      helper->GetVisibleSecurityState()->should_suppress_legacy_tls_warning);
+  EXPECT_EQ(helper->GetSecurityLevel(), security_state::WARNING);
 }
 
 // Tests that the Not Secure chip does not show for error pages on http:// URLs.

chrome/browser/ssl/tls_deprecation_config.cc

@@ -45,13 +45,15 @@ void SetRemoteTLSDeprecationConfigProto(
   TLSDeprecationConfigSingleton::GetInstance().SetProto(std::move(proto));
 }
 
-bool IsTLSDeprecationConfigControlSite(const GURL& url) {
+bool ShouldSuppressLegacyTLSWarning(const GURL& url) {
   if (!url.has_host() || !url.SchemeIsCryptographic())
     return false;
 
   auto* proto = TLSDeprecationConfigSingleton::GetInstance().GetProto();
+  // If the config is not yet loaded, we err on the side of not showing warnings
+  // for any sites.
   if (!proto)
-    return false;
+    return true;
 
   // Convert bytes from crypto::SHA256 so we can compare to the proto contents.
   std::string host_hash_bytes = crypto::SHA256HashString(url.host_piece());

chrome/browser/ssl/tls_deprecation_config.h

@@ -16,6 +16,6 @@ class LegacyTLSExperimentConfig;
 void SetRemoteTLSDeprecationConfigProto(
     std::unique_ptr<chrome_browser_ssl::LegacyTLSExperimentConfig> proto);
 
-bool IsTLSDeprecationConfigControlSite(const GURL& url);
+bool ShouldSuppressLegacyTLSWarning(const GURL& url);
 
 #endif  // CHROME_BROWSER_SSL_TLS_DEPRECATION_CONFIG_H_

chrome/browser/ssl/tls_deprecation_config_unittest.cc

@@ -15,7 +15,7 @@ using chrome_browser_ssl::LegacyTLSExperimentConfig;
 
 // Tests the case where no proto has been set by the component installer.
 TEST(TLSDeprecationConfigTest, NoProto) {
-  EXPECT_FALSE(IsTLSDeprecationConfigControlSite(GURL("https://example.test")));
+  EXPECT_TRUE(ShouldSuppressLegacyTLSWarning(GURL("https://example.test")));
 }
 
 // This tests that when no sites are in the control set,
@@ -30,7 +30,7 @@ TEST(TLSDeprecationConfigTest, NoControlSites) {
   auto config = std::make_unique<LegacyTLSExperimentConfig>();
   SetRemoteTLSDeprecationConfigProto(std::move(config));
 
-  EXPECT_FALSE(IsTLSDeprecationConfigControlSite(control_site));
+  EXPECT_FALSE(ShouldSuppressLegacyTLSWarning(control_site));
 }
 
 // This tests that when only a single control site is in the control set,
@@ -53,11 +53,11 @@ TEST(TLSDeprecationConfigTest, SingleControlSite) {
 
   SetRemoteTLSDeprecationConfigProto(std::move(config));
 
-  EXPECT_TRUE(IsTLSDeprecationConfigControlSite(control_site));
-  EXPECT_FALSE(IsTLSDeprecationConfigControlSite(non_control_site));
+  EXPECT_TRUE(ShouldSuppressLegacyTLSWarning(control_site));
+  EXPECT_FALSE(ShouldSuppressLegacyTLSWarning(non_control_site));
 
   // And HTTP sites should not count either.
-  EXPECT_FALSE(IsTLSDeprecationConfigControlSite(http_site));
+  EXPECT_FALSE(ShouldSuppressLegacyTLSWarning(http_site));
 }
 
 // This tests that the binary search in IsTLSDeprecationControlSite() works for
@@ -98,8 +98,8 @@ TEST(TLSDeprecationConfigTest, ManyControlSites) {
   SetRemoteTLSDeprecationConfigProto(std::move(config));
 
   for (auto& site : kControlSites) {
-    EXPECT_TRUE(IsTLSDeprecationConfigControlSite(site.url));
+    EXPECT_TRUE(ShouldSuppressLegacyTLSWarning(site.url));
   }
 
-  EXPECT_FALSE(IsTLSDeprecationConfigControlSite(non_control_site));
+  EXPECT_FALSE(ShouldSuppressLegacyTLSWarning(non_control_site));
 }

chrome/browser/ui/page_info/page_info.cc

@@ -830,7 +830,7 @@ void PageInfo::ComputeUIInputs(
     if (base::FeatureList::IsEnabled(
             security_state::features::kLegacyTLSWarnings) &&
         visible_security_state.connection_used_legacy_tls &&
-        !visible_security_state.is_legacy_tls_control_site) {
+        !visible_security_state.should_suppress_legacy_tls_warning) {
       site_connection_status_ = SITE_CONNECTION_STATUS_LEGACY_TLS;
     }
 

chrome/browser/ui/page_info/page_info_unittest.cc

@@ -872,7 +872,7 @@ TEST_F(PageInfoTest, LegacyTLS) {
   visible_security_state_.connection_status = status;
   visible_security_state_.connection_info_initialized = true;
   visible_security_state_.connection_used_legacy_tls = true;
-  visible_security_state_.is_legacy_tls_control_site = false;
+  visible_security_state_.should_suppress_legacy_tls_warning = false;
 
   SetDefaultUIExpectations(mock_ui());
 
@@ -900,7 +900,7 @@ TEST_F(PageInfoTest, LegacyTLSControlSite) {
   visible_security_state_.connection_status = status;
   visible_security_state_.connection_info_initialized = true;
   visible_security_state_.connection_used_legacy_tls = true;
-  visible_security_state_.is_legacy_tls_control_site = true;
+  visible_security_state_.should_suppress_legacy_tls_warning = true;
 
   SetDefaultUIExpectations(mock_ui());
 

components/security_state/core/security_state.cc

@@ -159,7 +159,7 @@ SecurityLevel GetSecurityLevel(
   if (base::FeatureList::IsEnabled(
           security_state::features::kLegacyTLSWarnings) &&
       visible_security_state.connection_used_legacy_tls &&
-      !visible_security_state.is_legacy_tls_control_site) {
+      !visible_security_state.should_suppress_legacy_tls_warning) {
     return WARNING;
   }
 
@@ -231,7 +231,7 @@ VisibleSecurityState::VisibleSecurityState()
       is_view_source(false),
       is_devtools(false),
       connection_used_legacy_tls(false),
-      is_legacy_tls_control_site(false) {}
+      should_suppress_legacy_tls_warning(false) {}
 
 VisibleSecurityState::~VisibleSecurityState() {}
 

components/security_state/core/security_state.h

@@ -181,7 +181,7 @@ struct VisibleSecurityState {
   bool connection_used_legacy_tls;
   // True if the page should be excluded from a UI treatment for legacy TLS
   // (used for control group in an experimental UI rollout).
-  bool is_legacy_tls_control_site;
+  bool should_suppress_legacy_tls_warning;
   // Contains information about input events that may impact the security
   // level of the page.
   InsecureInputEventData insecure_input_events;