Add a policy for disabling the stripping of PAC URLs.

The default is to strip https:// URLs before submitting them to PAC scripts. This CL introduces the policy "PacHttpsUrlStrippingEnabled" for disabling this security feature. Setting the policy to "false" causes Chrome to no longer strip https:// URLs before sending them to PAC scripts. This applies to all profiles, and all PAC scripts (including those discovered through WPAD, and those delivered over an insecure transport). The intent of this policy is to help enterprises with a compatibility problem transition. BUG=616396 [email protected] Review-Url: https://codereview.chromium.org/2030193004 Cr-Commit-Position: refs/heads/master@{#397808} (cherry picked from commit 9f7ea64) Review URL: https://codereview.chromium.org/2059513003 . Cr-Commit-Position: refs/branch-heads/2743@{crosswalk-project#309} Cr-Branched-From: 2b3ae3b-refs/heads/master@{#394939}
huningxin · Jun 10, 2016 · 4874cac · 4874cac
1 parent 5640232
commit 4874cac
Show file tree

Hide file tree

Showing 13 changed files with 146 additions and 15 deletions.
diff --git a/chrome/browser/io_thread.cc b/chrome/browser/io_thread.cc
@@ -442,6 +442,10 @@ IOThread::IOThread(
                             local_state);
   quick_check_enabled_.MoveToThread(io_thread_proxy);
 
+  pac_https_url_stripping_enabled_.Init(prefs::kPacHttpsUrlStrippingEnabled,
+                                        local_state);
+  pac_https_url_stripping_enabled_.MoveToThread(io_thread_proxy);
+
   is_spdy_allowed_by_policy_ =
       policy_service
           ->GetPolicies(policy::PolicyNamespace(policy::POLICY_DOMAIN_CHROME,
@@ -1049,6 +1053,7 @@ void IOThread::RegisterPrefs(PrefRegistrySimple* registry) {
   data_reduction_proxy::RegisterPrefs(registry);
   registry->RegisterBooleanPref(prefs::kBuiltInDnsClientEnabled, true);
   registry->RegisterBooleanPref(prefs::kQuickCheckEnabled, true);
+  registry->RegisterBooleanPref(prefs::kPacHttpsUrlStrippingEnabled, true);
 }
 
 void IOThread::UpdateServerWhitelist() {
@@ -1159,7 +1164,7 @@ void IOThread::InitSystemRequestContextOnIOThread() {
       net_log_, globals_->proxy_script_fetcher_context.get(),
       globals_->system_network_delegate.get(),
       std::move(system_proxy_config_service_), command_line,
-      quick_check_enabled_.GetValue());
+      WpadQuickCheckEnabled(), PacHttpsUrlStrippingEnabled());
 
   globals_->system_request_context.reset(
       ConstructSystemRequestContext(globals_, params_, net_log_));
@@ -1558,6 +1563,14 @@ net::QuicVersion IOThread::NetworkSessionConfigurator::ParseQuicVersion(
   return net::QUIC_VERSION_UNSUPPORTED;
 }
 
+bool IOThread::WpadQuickCheckEnabled() const {
+  return quick_check_enabled_.GetValue();
+}
+
+bool IOThread::PacHttpsUrlStrippingEnabled() const {
+  return pac_https_url_stripping_enabled_.GetValue();
+}
+
 // static
 net::URLRequestContext* IOThread::ConstructSystemRequestContext(
     IOThread::Globals* globals,

diff --git a/chrome/browser/io_thread.h b/chrome/browser/io_thread.h
@@ -425,6 +425,17 @@ class IOThread : public content::BrowserThreadDelegate {
   // Returns the callback for updating data use prefs.
   const metrics::UpdateUsagePrefCallbackType& GetMetricsDataUseForwarder();
 
+  // Returns true if the indicated proxy resolution features are
+  // enabled. These features are controlled through
+  // preferences/policy/commandline.
+  //
+  // For a description of what these features are, and how they are
+  // configured, see the comments in pref_names.cc for
+  // |kQuickCheckEnabled| and |kPacHttpsUrlStrippingEnabled
+  // respectively.
+  bool WpadQuickCheckEnabled() const;
+  bool PacHttpsUrlStrippingEnabled() const;
+
  private:
   // Provide SystemURLRequestContextGetter with access to
   // InitSystemRequestContext().
@@ -516,6 +527,8 @@ class IOThread : public content::BrowserThreadDelegate {
 
   BooleanPrefMember quick_check_enabled_;
 
+  BooleanPrefMember pac_https_url_stripping_enabled_;
+
   // Store HTTP Auth-related policies in this thread.
   // TODO(aberent) Make the list of auth schemes a PrefMember, so that the
   // policy can change after startup (https://crbug/549273).

diff --git a/chrome/browser/net/proxy_service_factory.cc b/chrome/browser/net/proxy_service_factory.cc
@@ -117,7 +117,8 @@ std::unique_ptr<net::ProxyService> ProxyServiceFactory::CreateProxyService(
     net::NetworkDelegate* network_delegate,
     std::unique_ptr<net::ProxyConfigService> proxy_config_service,
     const base::CommandLine& command_line,
-    bool quick_check_enabled) {
+    bool quick_check_enabled,
+    bool pac_https_url_stripping_enabled) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   bool use_v8 = !command_line.HasSwitch(switches::kWinHttpProxyResolver);
   // TODO(eroman): Figure out why this doesn't work in single-process mode.
@@ -187,11 +188,10 @@ std::unique_ptr<net::ProxyService> ProxyServiceFactory::CreateProxyService(
   }
 
   proxy_service->set_quick_check_enabled(quick_check_enabled);
-
-  if (command_line.HasSwitch(switches::kUnsafePacUrl)) {
-    proxy_service->set_sanitize_url_policy(
-        net::ProxyService::SanitizeUrlPolicy::UNSAFE);
-  }
+  proxy_service->set_sanitize_url_policy(
+      pac_https_url_stripping_enabled
+          ? net::ProxyService::SanitizeUrlPolicy::SAFE
+          : net::ProxyService::SanitizeUrlPolicy::UNSAFE);
 
   return proxy_service;
 }
diff --git a/chrome/browser/net/proxy_service_factory.h b/chrome/browser/net/proxy_service_factory.h
@@ -52,7 +52,8 @@ class ProxyServiceFactory {
       net::NetworkDelegate* network_delegate,
       std::unique_ptr<net::ProxyConfigService> proxy_config_service,
       const base::CommandLine& command_line,
-      bool quick_check_enabled);
+      bool quick_check_enabled,
+      bool pac_https_url_stripping_enabled);
 
  private:
   DISALLOW_IMPLICIT_CONSTRUCTORS(ProxyServiceFactory);

diff --git a/chrome/browser/policy/configuration_policy_handler_list_factory.cc b/chrome/browser/policy/configuration_policy_handler_list_factory.cc
@@ -103,6 +103,9 @@ const PolicyToPreferenceMapEntry kSimplePolicyMap[] = {
   { key::kWPADQuickCheckEnabled,
     prefs::kQuickCheckEnabled,
     base::Value::TYPE_BOOLEAN },
+  { key::kPacHttpsUrlStrippingEnabled,
+    prefs::kPacHttpsUrlStrippingEnabled,
+    base::Value::TYPE_BOOLEAN },
   { key::kDisableSpdy,
     prefs::kDisableSpdy,
     base::Value::TYPE_BOOLEAN },

diff --git a/chrome/browser/policy/policy_browsertest.cc b/chrome/browser/policy/policy_browsertest.cc
@@ -54,6 +54,7 @@
 #include "chrome/browser/infobars/infobar_service.h"
 #include "chrome/browser/interstitials/security_interstitial_page.h"
 #include "chrome/browser/interstitials/security_interstitial_page_test_utils.h"
+#include "chrome/browser/io_thread.h"
 #include "chrome/browser/media/media_capture_devices_dispatcher.h"
 #include "chrome/browser/media/media_stream_devices_controller.h"
 #include "chrome/browser/media/router/media_router_feature.h"
@@ -1353,6 +1354,48 @@ IN_PROC_BROWSER_TEST_F(PolicyTest, DisableSpdy) {
   EXPECT_TRUE(net::HttpStreamFactory::spdy_enabled());
 }
 
+namespace {
+
+// The following helpers retrieve whether https:// URL stripping is
+// enabled for PAC scripts. It needs to run on the IO thread.
+void GetPacHttpsUrlStrippingEnabledOnIOThread(IOThread* io_thread,
+                                              bool* enabled) {
+  *enabled = io_thread->PacHttpsUrlStrippingEnabled();
+}
+
+bool GetPacHttpsUrlStrippingEnabled() {
+  bool enabled;
+  base::RunLoop loop;
+  BrowserThread::PostTaskAndReply(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&GetPacHttpsUrlStrippingEnabledOnIOThread,
+                 g_browser_process->io_thread(), base::Unretained(&enabled)),
+      loop.QuitClosure());
+  loop.Run();
+  return enabled;
+}
+
+}  // namespace
+
+// Verifies that stripping of https:// URLs before sending to PAC scripts can
+// be disabled via a policy. Also verifies that stripping is enabled by
+// default.
+IN_PROC_BROWSER_TEST_F(PolicyTest, DisablePacHttpsUrlStripping) {
+  // Stripping is enabled by default.
+  EXPECT_TRUE(GetPacHttpsUrlStrippingEnabled());
+
+  // Disable it via a policy.
+  PolicyMap policies;
+  policies.Set(key::kPacHttpsUrlStrippingEnabled, POLICY_LEVEL_MANDATORY,
+               POLICY_SCOPE_USER, POLICY_SOURCE_CLOUD,
+               base::WrapUnique(new base::FundamentalValue(false)), nullptr);
+  UpdateProviderPolicy(policies);
+  content::RunAllPendingInMessageLoop();
+
+  // It should now reflect as disabled.
+  EXPECT_FALSE(GetPacHttpsUrlStrippingEnabled());
+}
+
 IN_PROC_BROWSER_TEST_F(PolicyTest, DisabledPlugins) {
   // Verifies that plugins can be forced to be disabled by policy.
 

diff --git a/chrome/browser/prefs/command_line_pref_store.cc b/chrome/browser/prefs/command_line_pref_store.cc
@@ -79,6 +79,7 @@ const CommandLinePrefStore::BooleanSwitchToPreferenceMapEntry
           prefs::kUnifiedDesktopEnabledByDefault, true },
 #endif
       { switches::kDisableAsyncDns, prefs::kBuiltInDnsClientEnabled, false },
+      { switches::kUnsafePacUrl, prefs::kPacHttpsUrlStrippingEnabled, false },
 };
 
 const CommandLinePrefStore::IntegerSwitchToPreferenceMapEntry

diff --git a/chrome/browser/profiles/profile_io_data.cc b/chrome/browser/profiles/profile_io_data.cc
@@ -385,7 +385,6 @@ void NotifyContextGettersOfShutdownOnIO(
 void ProfileIOData::InitializeOnUIThread(Profile* profile) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   PrefService* pref_service = profile->GetPrefs();
-  PrefService* local_state_pref_service = g_browser_process->local_state();
 
   std::unique_ptr<ProfileParams> params(new ProfileParams);
   params->path = profile->GetPath();
@@ -497,10 +496,6 @@ void ProfileIOData::InitializeOnUIThread(Profile* profile) {
     signin_allowed_.MoveToThread(io_task_runner);
   }
 
-  quick_check_enabled_.Init(prefs::kQuickCheckEnabled,
-                            local_state_pref_service);
-  quick_check_enabled_.MoveToThread(io_task_runner);
-
   media_device_id_salt_ = new MediaDeviceIDSalt(pref_service, IsOffTheRecord());
 
   network_prediction_options_.Init(prefs::kNetworkPredictionOptions,
@@ -1061,7 +1056,8 @@ void ProfileIOData::Init(
       io_thread_globals->proxy_script_fetcher_context.get(),
       io_thread_globals->system_network_delegate.get(),
       std::move(profile_params_->proxy_config_service), command_line,
-      quick_check_enabled_.GetValue());
+      io_thread->WpadQuickCheckEnabled(),
+      io_thread->PacHttpsUrlStrippingEnabled());
   transport_security_state_.reset(new net::TransportSecurityState());
   base::SequencedWorkerPool* pool = BrowserThread::GetBlockingPool();
   transport_security_persister_.reset(
@@ -1261,7 +1257,6 @@ void ProfileIOData::ShutdownOnUIThread(
   sync_disabled_.Destroy();
   signin_allowed_.Destroy();
   network_prediction_options_.Destroy();
-  quick_check_enabled_.Destroy();
   if (media_device_id_salt_.get())
     media_device_id_salt_->ShutdownOnUIThread();
   session_startup_pref_.Destroy();

diff --git a/chrome/common/pref_names.cc b/chrome/common/pref_names.cc
@@ -2114,8 +2114,39 @@ const char kPartnerBookmarkMappings[] = "partnerbookmarks.mappings";
 #endif
 
 // Whether DNS Quick Check is disabled in proxy resolution.
+//
+// This is a performance optimization for WPAD (Web Proxy
+// Auto-Discovery) which places a 1 second timeout on resolving the
+// DNS for PAC script URLs.
+//
+// It is on by default, but can be disabled via the Policy option
+// "WPADQuickCheckEnbled". There is no other UI for changing this
+// preference.
+//
+// For instance, if the DNS resolution for 'wpad' takes longer than 1
+// second, auto-detection will give up and fallback to the next proxy
+// configuration (which could be manually configured proxy server
+// 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 Policy (PacHttpsUrlStrippingEnabled), or 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";
 

diff --git a/chrome/common/pref_names.h b/chrome/common/pref_names.h
@@ -771,6 +771,7 @@ extern const char kPartnerBookmarkMappings[];
 #endif
 
 extern const char kQuickCheckEnabled[];
+extern const char kPacHttpsUrlStrippingEnabled[];
 extern const char kBrowserGuestModeEnabled[];
 extern const char kBrowserAddPersonEnabled[];
 

diff --git a/chrome/test/data/policy/policy_test_cases.json b/chrome/test/data/policy/policy_test_cases.json
@@ -1767,6 +1767,16 @@
     ]
   },
 
+  "PacHttpsUrlStrippingEnabled": {
+    "os": ["win", "linux", "mac", "chromeos"],
+    "test_policy": { "PacHttpsUrlStrippingEnabled": true },
+    "pref_mappings": [
+      { "pref": "proxy.pac_https_url_stripping_enabled",
+        "local_state": true
+      }
+    ]
+  },
+
   "RegisteredProtocolHandlers": {
     "os": ["win", "linux", "mac", "chromeos"],
     "can_be_recommended": true,

diff --git a/components/policy/resources/policy_templates.json b/components/policy/resources/policy_templates.json
@@ -8566,6 +8566,25 @@
       'tags': [],
       'desc': '''If this is set to true or is not set, users will be able to cast tabs, sites or the desktop from the browser. If set to false, this option will be disabled.'''
     },
+    {
+      'name': 'PacHttpsUrlStrippingEnabled',
+      'type': 'main',
+      'schema': { 'type': 'boolean' },
+      'supported_on': [ 'chrome.*:52-', 'chrome_os:52-' ],
+      'features': {
+        'dynamic_refresh': False,
+        'per_profile': False,
+      },
+      'example_value': False,
+      'id': 332,
+      'caption': '''Enable PAC URL stripping (for https://)''',
+      'tags': ['system-security'],
+      'desc': '''Strips privacy and security sensitive parts of https:// URLs before passing them on to PAC scripts (Proxy Auto Config) used by <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> during proxy resolution.
+
+      When not set (or set to true) the default behavior is to strip https:// URLs before submitting them to a PAC script. In this manner the PAC script is not able to view data that is ordinarily protected by an encrypted channel (like the path and query).
+
+      When the policy is set to false, this security feature is disabled, and PAC scripts are granted the ability to view the full URL. This setting applies to all PAC scripts regardless of origin. For instance it applies to PAC scripts obtained through WPAD as well as those fetched over an insecure transport.''',
+    },
   ],
   'messages': {
     # Messages that are not associated to any policies.

diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml
@@ -69509,6 +69509,7 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
              SAML login pages"/>
   <int value="329" label="Permit locking the screen"/>
   <int value="330" label="Set certificate availability for ARC-apps"/>
+  <int value="332" label="Enable PAC https:// URL stripping"/>
   <int value="333" label="Enables cast"/>
 </enum>