Remove remaining code for relaxing the PAC stripping policy.

The PacHttpsUrlStrippingEnabled policy was removed in M74. This CL also removes the equivalent --unsafe-pac-url command line flag and related plumbing.

Bug: 619087
Change-Id: Ic40d65662270b873e625ab108b6de82302b5143e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1567589
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Auto-Submit: Eric Roman <eroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#651517}

chrome/browser/net/network_context_configuration_browsertest.cc

@@ -135,11 +135,6 @@ struct TestCase {
   NetworkContextType network_context_type;
 };
 
-network::mojom::NetworkContextParamsPtr CreateDefaultNetworkContextParams() {
-  return g_browser_process->system_network_context_manager()
-      ->CreateDefaultNetworkContextParams();
-}
-
 // Tests the system, profile, and incognito profile NetworkContexts.
 class NetworkContextConfigurationBrowserTest
     : public InProcessBrowserTest,
@@ -1821,70 +1816,6 @@ class NetworkContextConfigurationHttpsStrippingPacBrowserTest
   }
 };
 
-// Start Chrome and check that PAC HTTPS path stripping is enabled.
-IN_PROC_BROWSER_TEST_P(NetworkContextConfigurationHttpsStrippingPacBrowserTest,
-                       PRE_PacHttpsUrlStripping) {
-  if (IsRestartStateWithInProcessNetworkService())
-    return;
-  ASSERT_FALSE(CreateDefaultNetworkContextParams()
-                   ->dangerously_allow_pac_access_to_secure_urls);
-
-  std::unique_ptr<network::ResourceRequest> request =
-      std::make_unique<network::ResourceRequest>();
-  // This URL should be directed to the proxy that fails with
-  // ERR_TUNNEL_CONNECTION_FAILED.
-  request->url = GURL("https://does.not.resolve.test:1872/foo");
-
-  content::SimpleURLLoaderTestHelper simple_loader_helper;
-  std::unique_ptr<network::SimpleURLLoader> simple_loader =
-      network::SimpleURLLoader::Create(std::move(request),
-                                       TRAFFIC_ANNOTATION_FOR_TESTS);
-
-  simple_loader->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
-      loader_factory(), simple_loader_helper.GetCallback());
-  simple_loader_helper.WaitForCallback();
-  EXPECT_FALSE(simple_loader_helper.response_body());
-  EXPECT_EQ(net::ERR_TUNNEL_CONNECTION_FAILED, simple_loader->NetError());
-
-  // Disable stripping paths from HTTPS PAC URLs for the next test.
-  g_browser_process->local_state()->SetBoolean(
-      prefs::kPacHttpsUrlStrippingEnabled, false);
-  // Check that the changed setting is reflected in the network context params.
-  // The changes aren't applied to existing URLRequestContexts, however, so have
-  // to restart to see the setting change respected.
-  EXPECT_TRUE(CreateDefaultNetworkContextParams()
-                  ->dangerously_allow_pac_access_to_secure_urls);
-}
-
-// Restart Chrome and check the case where PAC HTTPS path stripping is disabled.
-// Have to restart Chrome because the setting is only checked on NetworkContext
-// creation.
-// Flaky. See https://crbug.com/840127.
-IN_PROC_BROWSER_TEST_P(NetworkContextConfigurationHttpsStrippingPacBrowserTest,
-                       DISABLED_PacHttpsUrlStripping) {
-  if (IsRestartStateWithInProcessNetworkService())
-    return;
-  ASSERT_TRUE(CreateDefaultNetworkContextParams()
-                  ->dangerously_allow_pac_access_to_secure_urls);
-
-  std::unique_ptr<network::ResourceRequest> request =
-      std::make_unique<network::ResourceRequest>();
-  // This URL should be directed to the proxy that fails with
-  // ERR_PROXY_CONNECTION_FAILED.
-  request->url = GURL("https://does.not.resolve.test:1872/foo");
-
-  content::SimpleURLLoaderTestHelper simple_loader_helper;
-  std::unique_ptr<network::SimpleURLLoader> simple_loader =
-      network::SimpleURLLoader::Create(std::move(request),
-                                       TRAFFIC_ANNOTATION_FOR_TESTS);
-
-  simple_loader->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
-      loader_factory(), simple_loader_helper.GetCallback());
-  simple_loader_helper.WaitForCallback();
-  EXPECT_FALSE(simple_loader_helper.response_body());
-  EXPECT_EQ(net::ERR_PROXY_CONNECTION_FAILED, simple_loader->NetError());
-}
-
 // Instiates tests with a prefix indicating which NetworkContext is being
 // tested, and a suffix of "/0" if the network service is disabled, "/1" if it's
 // enabled, and "/2" if it's enabled and restarted.

chrome/browser/net/system_network_context_manager.cc

@@ -502,7 +502,6 @@ void SystemNetworkContextManager::RegisterPrefs(PrefRegistrySimple* registry) {
   registry->RegisterBooleanPref(prefs::kEnableReferrers, true);
 
   registry->RegisterBooleanPref(prefs::kQuickCheckEnabled, true);
-  registry->RegisterBooleanPref(prefs::kPacHttpsUrlStrippingEnabled, true);
 }
 
 void SystemNetworkContextManager::OnNetworkServiceCreated(
@@ -629,8 +628,6 @@ SystemNetworkContextManager::CreateDefaultNetworkContextParams() {
 
   network_context_params->pac_quick_check_enabled =
       local_state_->GetBoolean(prefs::kQuickCheckEnabled);
-  network_context_params->dangerously_allow_pac_access_to_secure_urls =
-      !local_state_->GetBoolean(prefs::kPacHttpsUrlStrippingEnabled);
 
   // Use the SystemNetworkContextManager to populate and update SSL
   // configuration. The SystemNetworkContextManager is owned by the

chrome/browser/prefs/chrome_command_line_pref_store.cc

@@ -86,7 +86,6 @@ const CommandLinePrefStore::BooleanSwitchToPreferenceMapEntry
         {chromeos::switches::kEnableCastReceiver, prefs::kCastReceiverEnabled,
          true},
 #endif
-        {switches::kUnsafePacUrl, prefs::kPacHttpsUrlStrippingEnabled, false},
         {switches::kEnableLocalSyncBackend,
          syncer::prefs::kEnableLocalSyncBackend, true},
 #if !defined(OS_CHROMEOS) && !defined(OS_ANDROID)

chrome/common/chrome_switches.cc

@@ -630,11 +630,6 @@ const char kTryChromeAgain[]                = "try-chrome-again";
 // apps/origins.  This should be used only for testing purpose.
 const char kUnlimitedStorage[]              = "unlimited-storage";
 
-// Pass the full https:// URL to PAC (Proxy Auto Config) scripts. As opposed to
-// the default behavior which strips path and query components before passing
-// to the PAC scripts.
-const char kUnsafePacUrl[] = "unsafe-pac-url";
-
 // A string used to override the default user agent with a custom one.
 const char kUserAgent[]                     = "user-agent";
 

chrome/common/chrome_switches.h

@@ -183,7 +183,6 @@ extern const char kTestName[];
 extern const char kTrustedDownloadSources[];
 extern const char kTryChromeAgain[];
 extern const char kUnlimitedStorage[];
-extern const char kUnsafePacUrl[];
 extern const char kUserAgent[];
 extern const char kUserDataDir[];
 extern const char kValidateCrx[];

chrome/common/pref_names.cc

@@ -2282,23 +2282,6 @@ const char kPartnerBookmarkMappings[] = "partnerbookmarks.mappings";
 // rules, or an implicit fallback to DIRECT connections).
 const char kQuickCheckEnabled[] = "proxy.quick_check_enabled";
 
-// Whether PAC scripts are given a stripped https:// URL (enabled), or
-// the full URL for https:// (disabled).
-//
-// This is a security feature which is on by default, and prevents PAC
-// scripts (which may have been sourced in an untrusted manner) from
-// having access to data that is ordinarily protected by a TLS channel
-// (i.e. the path and query components of an https:// URL).
-//
-// This preference is not exposed in the UI, but is overridable using
-// a commandline flag --unsafe-pac-url.
-//
-// The ability to turn off this security feature is not intended to be
-// a long-lived feature, but rather an escape-hatch for enterprises
-// while rolling out the change to PAC.
-const char kPacHttpsUrlStrippingEnabled[] =
-    "proxy.pac_https_url_stripping_enabled";
-
 // Whether Guest Mode is enabled within the browser.
 const char kBrowserGuestModeEnabled[] = "profile.browser_guest_enabled";
 

chrome/common/pref_names.h

@@ -788,7 +788,6 @@ extern const char kPartnerBookmarkMappings[];
 #endif  // defined(OS_ANDROID)
 
 extern const char kQuickCheckEnabled[];
-extern const char kPacHttpsUrlStrippingEnabled[];
 extern const char kBrowserGuestModeEnabled[];
 extern const char kBrowserAddPersonEnabled[];
 extern const char kForceBrowserSignin[];

net/proxy_resolution/proxy_resolution_service.cc

@@ -365,19 +365,30 @@ class UnsetProxyConfigService : public ProxyConfigService {
 #endif
 
 // Returns a sanitized copy of |url| which is safe to pass on to a PAC script.
-// The method for sanitizing is determined by |policy|. See the comments for
-// that enum for details.
-GURL SanitizeUrl(const GURL& url,
-                 ProxyResolutionService::SanitizeUrlPolicy policy) {
+//
+// PAC scripts are modelled as being controllable by a network-present
+// attacker (since such an attacker can influence the outcome of proxy
+// auto-discovery, or modify the contents of insecurely delivered PAC scripts).
+//
+// As such, it is important that the full path/query of https:// URLs not be
+// sent to PAC scripts, since that would give an attacker access to data that
+// is ordinarily protected by TLS.
+//
+// Obscuring the path for http:// URLs isn't being done since it doesn't matter
+// for security (attacker can already route traffic through their HTTP proxy
+// and see the full URL for http:// requests).
+//
+// TODO(https://crbug.com/882536): Use the same stripping for insecure URL
+// schemes.
+GURL SanitizeUrl(const GURL& url) {
   DCHECK(url.is_valid());
 
   GURL::Replacements replacements;
   replacements.ClearUsername();
   replacements.ClearPassword();
   replacements.ClearRef();
 
-  if (policy == ProxyResolutionService::SanitizeUrlPolicy::SAFE &&
-      url.SchemeIsCryptographic()) {
+  if (url.SchemeIsCryptographic()) {
     replacements.ClearPath();
     replacements.ClearQuery();
   }
@@ -1056,7 +1067,6 @@ ProxyResolutionService::ProxyResolutionService(
       stall_proxy_auto_config_delay_(
           TimeDelta::FromMilliseconds(kDelayAfterNetworkChangesMs)),
       quick_check_enabled_(true),
-      sanitize_url_policy_(SanitizeUrlPolicy::SAFE),
       weak_ptr_factory_(this) {
   NetworkChangeNotifier::AddIPAddressObserver(this);
   NetworkChangeNotifier::AddDNSObserver(this);
@@ -1175,7 +1185,7 @@ int ProxyResolutionService::ResolveProxy(const GURL& raw_url,
   // script). The goal is to remove sensitive data (like embedded user names
   // and password), and local data (i.e. reference fragment) which does not need
   // to be disclosed to the resolver.
-  GURL url = SanitizeUrl(raw_url, sanitize_url_policy_);
+  GURL url = SanitizeUrl(raw_url);
 
   // Check if the request can be completed right away. (This is the case when
   // using a direct connection for example).

net/proxy_resolution/proxy_resolution_service.h

@@ -54,25 +54,6 @@ class NET_EXPORT ProxyResolutionService
       public NetworkChangeNotifier::DNSObserver,
       public ProxyConfigService::Observer {
  public:
-  // Enumerates the policy to use when sanitizing URLs for proxy resolution
-  // (before passing them off to PAC scripts).
-  enum class SanitizeUrlPolicy {
-    // Do a basic level of sanitization for URLs:
-    //   - strip embedded identities (ex: "username:password@")
-    //   - strip the fragment (ex: "#blah")
-    //
-    // This is considered "unsafe" because it does not do any additional
-    // stripping for https:// URLs.
-    UNSAFE,
-
-    // SAFE does the same sanitization as UNSAFE, but additionally strips
-    // everything but the (scheme,host,port) from cryptographic URL schemes
-    // (https:// and wss://).
-    //
-    // In other words, it strips the path and query portion of https:// URLs.
-    SAFE,
-  };
-
   // This interface defines the set of policies for when to poll the PAC
   // script for changes.
   //
@@ -306,13 +287,6 @@ class NET_EXPORT ProxyResolutionService
   }
   bool quick_check_enabled_for_testing() const { return quick_check_enabled_; }
 
-  void set_sanitize_url_policy(SanitizeUrlPolicy policy) {
-    sanitize_url_policy_ = policy;
-  }
-  SanitizeUrlPolicy sanitize_url_policy_for_testing() const {
-    return sanitize_url_policy_;
-  }
-
  private:
   FRIEND_TEST_ALL_PREFIXES(ProxyResolutionServiceTest,
                            UpdateConfigAfterFailedAutodetect);
@@ -470,9 +444,6 @@ class NET_EXPORT ProxyResolutionService
   // Whether child PacFileDeciders should use QuickCheck
   bool quick_check_enabled_;
 
-  // The method to use for sanitizing URLs seen by the proxy resolver.
-  SanitizeUrlPolicy sanitize_url_policy_;
-
   THREAD_CHECKER(thread_checker_);
 
   ProxyDelegate* proxy_delegate_ = nullptr;