From 7e18b89c1fe52bfc7868dc3ded772e81f404b3c4 Mon Sep 17 00:00:00 2001 From: sclittle Date: Mon, 11 May 2015 21:30:58 -0700 Subject: [PATCH] Temporary fix to prevent the DRP from being activated improperly. This change prevents any *.googlezip.net Data Reduction Proxy from being configured as part of the ProxyService's ProxyConfig via the proxy pref. Specifying a DRP in the proxy rules is not a supported means of activating the DRP, and could cause requests to be sent to the DRP without the appropriate authentication headers and without using any of the DRP bypass logic. This is a fix for http://crbug.com/476610, to be merged into M43. BUG=476610 Review URL: https://codereview.chromium.org/1126413006 Cr-Commit-Position: refs/heads/master@{#329349} --- .../browser/net/pref_proxy_config_tracker_impl.cc | 88 +++++++++++++++++++--- .../net/pref_proxy_config_tracker_impl_unittest.cc | 74 ++++++++++++++++++ 2 files changed, 153 insertions(+), 9 deletions(-) diff --git a/chrome/browser/net/pref_proxy_config_tracker_impl.cc b/chrome/browser/net/pref_proxy_config_tracker_impl.cc index b7d0d3aa20ee..49161ba7564b 100644 --- a/chrome/browser/net/pref_proxy_config_tracker_impl.cc +++ b/chrome/browser/net/pref_proxy_config_tracker_impl.cc @@ -7,6 +7,7 @@ #include "base/bind.h" #include "base/prefs/pref_registry_simple.h" #include "base/prefs/pref_service.h" +#include "base/strings/string_util.h" #include "base/values.h" #include "chrome/browser/chrome_notification_types.h" #include "chrome/browser/prefs/proxy_config_dictionary.h" @@ -15,9 +16,66 @@ #include "content/public/browser/browser_thread.h" #include "content/public/browser/notification_details.h" #include "content/public/browser/notification_source.h" +#include "net/proxy/proxy_list.h" +#include "net/proxy/proxy_server.h" using content::BrowserThread; +namespace { + +// Determine if |proxy| is of the form "*.googlezip.net". +bool IsGooglezipDataReductionProxy(const net::ProxyServer& proxy) { + return proxy.is_valid() && !proxy.is_direct() && + EndsWith(proxy.host_port_pair().host(), ".googlezip.net", true); +} + +// Removes any Data Reduction Proxies like *.googlezip.net from |proxy_list|. +void RemoveGooglezipDataReductionProxiesFromList(net::ProxyList* proxy_list) { + if (proxy_list->IsEmpty()) + return; + + bool found_googlezip_proxy = false; + for (const net::ProxyServer& proxy : proxy_list->GetAll()) { + if (IsGooglezipDataReductionProxy(proxy)) { + found_googlezip_proxy = true; + break; + } + } + if (!found_googlezip_proxy) + return; + + net::ProxyList replacement_list; + for (const net::ProxyServer& proxy : proxy_list->GetAll()) { + if (!IsGooglezipDataReductionProxy(proxy)) + replacement_list.AddProxyServer(proxy); + } + + if (replacement_list.IsEmpty()) + replacement_list.AddProxyServer(net::ProxyServer::Direct()); + *proxy_list = replacement_list; +} + +// Remove any Data Reduction Proxies like *.googlezip.net from |proxy_rules|. +// This is to prevent a Data Reduction Proxy from being activated in an +// unsupported way, such as from a proxy pref, which could cause Chrome to use +// the Data Reduction Proxy without adding any of the necessary authentication +// headers or applying the Data Reduction Proxy bypass logic. See +// http://crbug.com/476610. +// TODO(sclittle): Add UMA to record how often this method is called, and how +// often it actually removes a *.googlezip.net proxy. This method should be +// removed once it stops actually finding and removing *.googlezip.net proxies +// from the proxy rules. +void RemoveGooglezipDataReductionProxies( + net::ProxyConfig::ProxyRules* proxy_rules) { + RemoveGooglezipDataReductionProxiesFromList(&proxy_rules->fallback_proxies); + RemoveGooglezipDataReductionProxiesFromList(&proxy_rules->proxies_for_ftp); + RemoveGooglezipDataReductionProxiesFromList(&proxy_rules->proxies_for_http); + RemoveGooglezipDataReductionProxiesFromList(&proxy_rules->proxies_for_https); + RemoveGooglezipDataReductionProxiesFromList(&proxy_rules->single_proxies); +} + +} // namespace + //============================= ChromeProxyConfigService ======================= ChromeProxyConfigService::ChromeProxyConfigService( @@ -179,25 +237,37 @@ net::ProxyConfigService::ConfigAvailability bool ignore_fallback_config, ProxyPrefs::ConfigState* effective_config_state, net::ProxyConfig* effective_config) { + net::ProxyConfigService::ConfigAvailability rv; *effective_config_state = pref_state; if (PrefPrecedes(pref_state)) { *effective_config = pref_config; - return net::ProxyConfigService::CONFIG_VALID; - } - - // If there's no system proxy config, fall back to prefs or default. - if (system_availability == net::ProxyConfigService::CONFIG_UNSET) { + rv = net::ProxyConfigService::CONFIG_VALID; + } else if (system_availability == net::ProxyConfigService::CONFIG_UNSET) { + // If there's no system proxy config, fall back to prefs or default. if (pref_state == ProxyPrefs::CONFIG_FALLBACK && !ignore_fallback_config) *effective_config = pref_config; else *effective_config = net::ProxyConfig::CreateDirect(); - return net::ProxyConfigService::CONFIG_VALID; + rv = net::ProxyConfigService::CONFIG_VALID; + } else { + *effective_config_state = ProxyPrefs::CONFIG_SYSTEM; + *effective_config = system_config; + rv = system_availability; } - *effective_config_state = ProxyPrefs::CONFIG_SYSTEM; - *effective_config = system_config; - return system_availability; + // Remove any Data Reduction Proxies like *.googlezip.net from the proxy + // config rules, since specifying a DRP in the proxy rules is not a supported + // means of activating the DRP, and could cause requests to be sent to the DRP + // without the appropriate authentication headers and without using any of the + // DRP bypass logic. This prevents the Data Reduction Proxy from being + // improperly activated via the proxy pref. + // TODO(sclittle): This is a temporary fix for http://crbug.com/476610, and + // should be removed once that bug is fixed and verified. + if (rv == net::ProxyConfigService::CONFIG_VALID) + RemoveGooglezipDataReductionProxies(&effective_config->proxy_rules()); + + return rv; } // static diff --git a/chrome/browser/net/pref_proxy_config_tracker_impl_unittest.cc b/chrome/browser/net/pref_proxy_config_tracker_impl_unittest.cc index 63445b8ae174..44a3c005b5b9 100644 --- a/chrome/browser/net/pref_proxy_config_tracker_impl_unittest.cc +++ b/chrome/browser/net/pref_proxy_config_tracker_impl_unittest.cc @@ -4,6 +4,8 @@ #include "chrome/browser/net/pref_proxy_config_tracker_impl.h" +#include + #include "base/command_line.h" #include "base/files/file_path.h" #include "base/message_loop/message_loop.h" @@ -15,6 +17,8 @@ #include "chrome/common/pref_names.h" #include "content/public/test/test_browser_thread.h" #include "net/proxy/proxy_config_service_common_unittest.h" +#include "net/proxy/proxy_info.h" +#include "net/proxy/proxy_list.h" #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" @@ -287,6 +291,76 @@ TEST_F(PrefProxyConfigTrackerImplTest, ExplicitSystemSettings) { EXPECT_EQ(GURL(kFixedPacUrl), actual_config.pac_url()); } +void CheckResolvedProxyMatches(net::ProxyConfig* config, + const GURL& url, + const std::string& result_string) { + net::ProxyInfo expected_result; + expected_result.UseNamedProxy(result_string); + + net::ProxyInfo result; + config->proxy_rules().Apply(url, &result); + + EXPECT_TRUE(expected_result.proxy_list().Equals(result.proxy_list())) + << "expected: " << expected_result.proxy_list().ToPacString() + << "\nactual: " << result.proxy_list().ToPacString(); +} + +TEST_F(PrefProxyConfigTrackerImplTest, ExcludeGooglezipDataReductionProxies) { + const std::string kDataReductionProxies = + "https://proxy.googlezip.net:443,compress.googlezip.net," + "https://proxy-dev.googlezip.net:443,proxy-dev.googlezip.net," + "quic://proxy.googlezip.net"; + + struct { + std::string initial_proxy_rules; + const char* http_proxy_info; + const char* https_proxy_info; + const char* ftp_proxy_info; + } test_cases[] = { + {"http=foopyhttp," + kDataReductionProxies + + ",direct://;https=foopyhttps," + kDataReductionProxies + + ",direct://;ftp=foopyftp," + kDataReductionProxies + ",direct://", + "foopyhttp;direct://", + "foopyhttps;direct://", + "foopyftp;direct://"}, + + {"foopy," + kDataReductionProxies + ",direct://", + "foopy;direct://", + "foopy;direct://", + "foopy;direct://"}, + + {"http=" + kDataReductionProxies + ";https=" + kDataReductionProxies + + ";ftp=" + kDataReductionProxies, + "direct://", + "direct://", + "direct://"}, + + {"http=" + kDataReductionProxies + ",foopy,direct://", + "foopy;direct://", + "direct://", + "direct://"}, + }; + + // Test setting the proxy from a user pref. + for (const auto& test : test_cases) { + pref_service_->SetUserPref(prefs::kProxy, + ProxyConfigDictionary::CreateFixedServers( + test.initial_proxy_rules, std::string())); + loop_.RunUntilIdle(); + + net::ProxyConfig config; + EXPECT_EQ(net::ProxyConfigService::CONFIG_VALID, + proxy_config_service_->GetLatestProxyConfig(&config)); + + CheckResolvedProxyMatches(&config, GURL("http://google.com"), + test.http_proxy_info); + CheckResolvedProxyMatches(&config, GURL("https://google.com"), + test.https_proxy_info); + CheckResolvedProxyMatches(&config, GURL("ftp://google.com"), + test.ftp_proxy_info); + } +} + // Test parameter object for testing command line proxy configuration. struct CommandLineTestParams { // Explicit assignment operator, so testing::TestWithParam works with MSVC. -- 2.11.4.GIT