Fix for UnsafelyTreatInsecureOriginAsSecure policy in browser UI

This CL updates secure_origin_whitelist::GetWhitelist() to delegate
parsing the set of whitelisted sites to a new function, ParseWhitelist,
which takes in a string and returns the parsed vector of strings. This
allows callers in the browser process to explicitly parse a whitelist
from either prefs or command-line switches. This allows
SecurityStateTabHelper to have its own custom IsOriginSecureWithWhitelist
function to which it can bind an explicitly passed whitelist of origins,
rather than just using content::IsOriginSecure as the callback to
security_state functions. content::IsOriginSecure uses GetWhitelist()
which only loads the whitelist from command-line flags, which are only
correctly set for renderer processes. The custom callback for
SecurityStateTabHelper allows it to also check prefs for the whitelist,
which is how the enterprise policy is accessible in the browser process
(where security indicator UI logic occurs).

This can cause the pref and the switch to be two different sources of
truth for the origin whitelist, however this simpler fix will be easier
to backport. This fix favors the switch over the pref if both are set,
allowing developers to still set temporary overrides while maintaining
the policy behavior for general users. More general fixes may involve
changing how the whitelist propagates between parts of Chrome
(including in content and blink).

Bug: 869422
Change-Id: I93b46d66844af8cee00d919537ce66fc2c56cd46
Reviewed-on: https://chromium-review.googlesource.com/1157029Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Commit-Queue: Christopher Thompson <cthomp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579835}

chrome/browser/secure_origin_whitelist_browsertest.cc

@@ -4,6 +4,7 @@
 
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
+#include "chrome/browser/ssl/security_state_tab_helper.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/chrome_switches.h"
@@ -163,3 +164,32 @@ IN_PROC_BROWSER_TEST_P(SecureOriginWhitelistBrowsertest, Simple) {
               ExpectSecureContext() ? secure : insecure);
   }
 }
+
+// Tests that whitelisted insecure origins are correctly set as security level
+// NONE instead of the default level HTTPS_SHOW_WARNING.
+IN_PROC_BROWSER_TEST_P(SecureOriginWhitelistBrowsertest, SecurityIndicators) {
+  ui_test_utils::NavigateToURL(
+      browser(),
+      embedded_test_server()->GetURL(
+          "example.com", "/secure_origin_whitelist_browsertest.html"));
+  auto* helper = SecurityStateTabHelper::FromWebContents(
+      browser()->tab_strip_model()->GetActiveWebContents());
+  ASSERT_TRUE(helper);
+  security_state::SecurityInfo security_info;
+  helper->GetSecurityInfo(&security_info);
+
+  if (GetParam() == TestVariant::kPolicyOldAndNew) {
+    // When both policies are set, the new policy overrides the old policy.
+    EXPECT_EQ(security_state::HTTP_SHOW_WARNING, security_info.security_level);
+    ui_test_utils::NavigateToURL(
+        browser(),
+        embedded_test_server()->GetURL(
+            "otherexample.com", "/secure_origin_whitelist_browsertest.html"));
+    helper->GetSecurityInfo(&security_info);
+    EXPECT_EQ(security_state::NONE, security_info.security_level);
+  } else {
+    EXPECT_EQ(ExpectSecureContext() ? security_state::NONE
+                                    : security_state::HTTP_SHOW_WARNING,
+              security_info.security_level);
+  }
+}

chrome/browser/ssl/security_state_tab_helper.cc

@@ -5,16 +5,21 @@
 #include "chrome/browser/ssl/security_state_tab_helper.h"
 
 #include "base/bind.h"
+#include "base/command_line.h"
 #include "base/feature_list.h"
 #include "base/metrics/field_trial_params.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/strings/pattern.h"
+#include "base/strings/string_util.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "chrome/browser/safe_browsing/ui_manager.h"
+#include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
+#include "chrome/common/secure_origin_whitelist.h"
 #include "components/prefs/pref_service.h"
 #include "components/safe_browsing/features.h"
 #include "components/security_state/content/content_utils.h"
@@ -31,6 +36,7 @@
 #include "net/ssl/ssl_cipher_suite_names.h"
 #include "net/ssl/ssl_connection_status_flags.h"
 #include "third_party/boringssl/src/include/openssl/ssl.h"
+#include "url/origin.h"
 
 #if defined(OS_CHROMEOS)
 #include "chrome/browser/chromeos/policy/policy_cert_service.h"
@@ -55,6 +61,22 @@ void RecordSecurityLevel(const security_state::SecurityInfo& security_info) {
   }
 }
 
+bool IsOriginSecureWithWhitelist(
+    const std::vector<std::string>& secure_origins_and_patterns,
+    const GURL& url) {
+  if (content::IsOriginSecure(url))
+    return true;
+
+  url::Origin origin = url::Origin::Create(url);
+  if (base::ContainsValue(secure_origins_and_patterns, origin.Serialize()))
+    return true;
+  for (const auto& origin_or_pattern : secure_origins_and_patterns) {
+    if (base::MatchPattern(origin.host(), origin_or_pattern))
+      return true;
+  }
+  return false;
+}
+
 }  // namespace
 
 using safe_browsing::SafeBrowsingUIManager;
@@ -75,9 +97,11 @@ SecurityStateTabHelper::~SecurityStateTabHelper() {}
 
 void SecurityStateTabHelper::GetSecurityInfo(
     security_state::SecurityInfo* result) const {
-  security_state::GetSecurityInfo(GetVisibleSecurityState(),
-                                  UsedPolicyInstalledCertificate(),
-                                  base::Bind(&content::IsOriginSecure), result);
+  security_state::GetSecurityInfo(
+      GetVisibleSecurityState(), UsedPolicyInstalledCertificate(),
+      base::BindRepeating(&IsOriginSecureWithWhitelist,
+                          GetSecureOriginsAndPatterns()),
+      result);
 }
 
 void SecurityStateTabHelper::DidStartNavigation(
@@ -332,3 +356,20 @@ SecurityStateTabHelper::GetVisibleSecurityState() const {
 
   return state;
 }
+
+std::vector<std::string> SecurityStateTabHelper::GetSecureOriginsAndPatterns()
+    const {
+  const base::CommandLine& command_line =
+      *base::CommandLine::ForCurrentProcess();
+  Profile* profile =
+      Profile::FromBrowserContext(web_contents()->GetBrowserContext());
+  PrefService* prefs = profile->GetPrefs();
+  std::string origins_str = "";
+  if (command_line.HasSwitch(switches::kUnsafelyTreatInsecureOriginAsSecure)) {
+    origins_str = command_line.GetSwitchValueASCII(
+        switches::kUnsafelyTreatInsecureOriginAsSecure);
+  } else if (prefs->HasPrefPath(prefs::kUnsafelyTreatInsecureOriginAsSecure)) {
+    origins_str = prefs->GetString(prefs::kUnsafelyTreatInsecureOriginAsSecure);
+  }
+  return secure_origin_whitelist::ParseWhitelist(origins_str);
+}

chrome/browser/ssl/security_state_tab_helper.h

@@ -46,6 +46,7 @@ class SecurityStateTabHelper
   security_state::MaliciousContentStatus GetMaliciousContentStatus() const;
   std::unique_ptr<security_state::VisibleSecurityState>
   GetVisibleSecurityState() const;
+  std::vector<std::string> GetSecureOriginsAndPatterns() const;
 
   // True if a console message has been logged about an omnibox warning shown
   // when sensitive input fields are shown on insecure HTTP pages. This message

chrome/common/secure_origin_whitelist.cc

@@ -106,15 +106,8 @@ std::string CanonicalizePatternComponents(const std::string& hostname_pattern) {
 
 namespace secure_origin_whitelist {
 
-std::vector<std::string> GetWhitelist() {
+std::vector<std::string> ParseWhitelist(const std::string& origins_str) {
   std::vector<std::string> origin_patterns;
-  // If kUnsafelyTreatInsecureOriginAsSecure option is given, then treat the
-  // value as a comma-separated list of origins or origin patterns:
-  const base::CommandLine& command_line =
-      *base::CommandLine::ForCurrentProcess();
-  if (command_line.HasSwitch(switches::kUnsafelyTreatInsecureOriginAsSecure)) {
-    std::string origins_str = command_line.GetSwitchValueASCII(
-        switches::kUnsafelyTreatInsecureOriginAsSecure);
   for (const std::string& origin_str : base::SplitString(
            origins_str, ",", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL)) {
     if (origin_str.find("*") != std::string::npos) {
@@ -136,7 +129,6 @@ std::vector<std::string> GetWhitelist() {
     if (!origin.unique())
       origin_patterns.push_back(origin.Serialize());
   }
-  }
 
   UMA_HISTOGRAM_COUNTS_100("Security.TreatInsecureOriginAsSecure",
                            origin_patterns.size());
@@ -152,6 +144,23 @@ std::vector<std::string> GetWhitelist() {
   return origin_patterns;
 }
 
+std::vector<std::string> GetWhitelist() {
+  // If kUnsafelyTreatInsecureOriginAsSecure option is given, then treat the
+  // value as a comma-separated list of origins or origin patterns. Callers that
+  // need to also check the kUnsafelyTreatInsecureOriginAsSecure pref value must
+  // instead use ParseWhitelist directly (as there is no way for GetWhitelist()
+  // to access prefs). For renderer processes the pref and the switch will
+  // match, but for non-renderer processes the switch may not be set.
+  const base::CommandLine& command_line =
+      *base::CommandLine::ForCurrentProcess();
+  std::string origins_str = "";
+  if (command_line.HasSwitch(switches::kUnsafelyTreatInsecureOriginAsSecure)) {
+    origins_str = command_line.GetSwitchValueASCII(
+        switches::kUnsafelyTreatInsecureOriginAsSecure);
+  }
+  return ParseWhitelist(origins_str);
+}
+
 std::set<std::string> GetSchemesBypassingSecureContextCheck() {
   std::set<std::string> schemes;
   schemes.insert(extensions::kExtensionScheme);

chrome/common/secure_origin_whitelist.h

@@ -31,6 +31,11 @@ namespace secure_origin_whitelist {
 // patterns, each component is individually canonicalized.
 std::vector<std::string> GetWhitelist();
 
+// Parses a comma-separated list of origins and wildcard hostname patterns.
+// This separate function allows callers other than GetWhitelist() to
+// explicitly pass a whitelist to be parsed.
+std::vector<std::string> ParseWhitelist(const std::string& origins_str);
+
 // Returns a whitelist of schemes that should bypass the Is Privileged Context
 // check. See http://www.w3.org/TR/powerful-features/#settings-privileged.
 std::set<std::string> GetSchemesBypassingSecureContextCheck();