From 4874cac8cee7b9735893e678758a53dcede20700 Mon Sep 17 00:00:00 2001 From: Eric Roman Date: Fri, 10 Jun 2016 00:57:42 -0700 Subject: [PATCH] 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 TBR=isherman@chromium.org Review-Url: https://codereview.chromium.org/2030193004 Cr-Commit-Position: refs/heads/master@{#397808} (cherry picked from commit 9f7ea64cf573101c01ddcf020b87c63089038834) Review URL: https://codereview.chromium.org/2059513003 . Cr-Commit-Position: refs/branch-heads/2743@{#309} Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939} --- chrome/browser/io_thread.cc | 15 ++++++- chrome/browser/io_thread.h | 13 ++++++ chrome/browser/net/proxy_service_factory.cc | 12 +++--- chrome/browser/net/proxy_service_factory.h | 3 +- ...nfiguration_policy_handler_list_factory.cc | 3 ++ chrome/browser/policy/policy_browsertest.cc | 43 +++++++++++++++++++ .../browser/prefs/command_line_pref_store.cc | 1 + chrome/browser/profiles/profile_io_data.cc | 9 +--- chrome/common/pref_names.cc | 31 +++++++++++++ chrome/common/pref_names.h | 1 + .../test/data/policy/policy_test_cases.json | 10 +++++ .../policy/resources/policy_templates.json | 19 ++++++++ tools/metrics/histograms/histograms.xml | 1 + 13 files changed, 146 insertions(+), 15 deletions(-) diff --git a/chrome/browser/io_thread.cc b/chrome/browser/io_thread.cc index 44a5755307f92..5e2080252100f 100644 --- 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 index 85210ba1abdb4..84503476f914d 100644 --- 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 index 63e1062fbfb68..c67427c247ef9 100644 --- a/chrome/browser/net/proxy_service_factory.cc +++ b/chrome/browser/net/proxy_service_factory.cc @@ -117,7 +117,8 @@ std::unique_ptr ProxyServiceFactory::CreateProxyService( net::NetworkDelegate* network_delegate, std::unique_ptr 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 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 index afbeda0d20225..aea984fa81360 100644 --- 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 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 index 97b222523df2e..2232e3c74c2e1 100644 --- 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 index 05339894a2d65..e0a7d4551c5c4 100644 --- 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 index 042430cfb3d02..459347cd23df5 100644 --- 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 index 934a43ec2c0e9..1e25a44807cb7 100644 --- 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 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 index 07609e6aa3c5d..77c8680cd178b 100644 --- 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 index 8c5c85efbeda7..d6f96fa8102a3 100644 --- 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 index 7f50fc3076abe..3f1e461946591 100644 --- 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 index d87c6f9ff8122..77147b4660e9b 100644 --- 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 $1Google Chrome 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 index 017696b050b24..d4d8f6bbe4567 100644 --- 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"/> +