From 8e87f4ca2b1ebb022057bd4daadb90bfe628307e Mon Sep 17 00:00:00 2001 From: Anton Lazarev Date: Thu, 27 May 2021 17:38:48 -0700 Subject: [PATCH 1/5] avoid CNAME uncloaking if a proxy is set --- .../brave_ad_block_tp_network_delegate_helper.cc | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/browser/net/brave_ad_block_tp_network_delegate_helper.cc b/browser/net/brave_ad_block_tp_network_delegate_helper.cc index 76d2253a7b06..ee8342cbc245 100644 --- a/browser/net/brave_ad_block_tp_network_delegate_helper.cc +++ b/browser/net/brave_ad_block_tp_network_delegate_helper.cc @@ -24,6 +24,11 @@ #include "brave/grit/brave_generated_resources.h" #include "chrome/browser/net/secure_dns_config.h" #include "chrome/browser/net/system_network_context_manager.h" +#include "chrome/browser/profiles/profile.h" +#include "components/prefs/pref_service.h" +#include "components/proxy_config/proxy_config_dictionary.h" +#include "components/proxy_config/proxy_config_pref_names.h" +#include "components/proxy_config/proxy_prefs.h" #include "content/public/browser/browser_context.h" #include "content/public/browser/browser_thread.h" #include "content/public/browser/render_frame_host.h" @@ -245,6 +250,17 @@ void OnBeforeURLRequestAdBlockTP(const ResponseCallback& next_callback, brave_shields::features::kBraveAdblockCnameUncloaking) && ctx->browser_context && !ctx->browser_context->IsTor(); + // Also, skip CNAME uncloaking if there is currently a configured proxy. + Profile* profile = Profile::FromBrowserContext(ctx->browser_context); + ProxyConfigDictionary dict( + profile->GetPrefs()->GetDictionary(proxy_config::prefs::kProxy)->Clone()); + ProxyPrefs::ProxyMode mode; + + if (!dict.GetMode(&mode) || !(mode == ProxyPrefs::ProxyMode::MODE_DIRECT || + mode == ProxyPrefs::ProxyMode::MODE_SYSTEM)) { + should_check_uncloaked = false; + } + task_runner->PostTaskAndReplyWithResult( FROM_HERE, base::BindOnce(&ShouldBlockRequestOnTaskRunner, ctx, EngineFlags(), From 78036601285ae04d778b45741c001334fdecea1b Mon Sep 17 00:00:00 2001 From: Anton Lazarev Date: Thu, 3 Jun 2021 10:39:18 -0700 Subject: [PATCH 2/5] second attempt, using ProxyConfigService approach --- ...ave_ad_block_tp_network_delegate_helper.cc | 48 ++++++++++++++----- 1 file changed, 37 insertions(+), 11 deletions(-) diff --git a/browser/net/brave_ad_block_tp_network_delegate_helper.cc b/browser/net/brave_ad_block_tp_network_delegate_helper.cc index ee8342cbc245..8fcef05f75ee 100644 --- a/browser/net/brave_ad_block_tp_network_delegate_helper.cc +++ b/browser/net/brave_ad_block_tp_network_delegate_helper.cc @@ -22,13 +22,12 @@ #include "brave/components/brave_shields/common/brave_shield_constants.h" #include "brave/components/brave_shields/common/features.h" #include "brave/grit/brave_generated_resources.h" +#include "chrome/browser/net/proxy_service_factory.h" #include "chrome/browser/net/secure_dns_config.h" #include "chrome/browser/net/system_network_context_manager.h" #include "chrome/browser/profiles/profile.h" #include "components/prefs/pref_service.h" -#include "components/proxy_config/proxy_config_dictionary.h" -#include "components/proxy_config/proxy_config_pref_names.h" -#include "components/proxy_config/proxy_prefs.h" +#include "components/proxy_config/pref_proxy_config_tracker.h" #include "content/public/browser/browser_context.h" #include "content/public/browser/browser_thread.h" #include "content/public/browser/render_frame_host.h" @@ -37,6 +36,9 @@ #include "content/public/common/url_constants.h" #include "extensions/common/url_pattern.h" #include "mojo/public/cpp/bindings/remote.h" +#include "net/proxy_resolution/proxy_config.h" +#include "net/proxy_resolution/proxy_config_service.h" +#include "net/proxy_resolution/proxy_config_with_annotation.h" #include "services/network/host_resolver.h" #include "services/network/network_context.h" #include "ui/base/resource/resource_bundle.h" @@ -251,14 +253,38 @@ void OnBeforeURLRequestAdBlockTP(const ResponseCallback& next_callback, ctx->browser_context && !ctx->browser_context->IsTor(); // Also, skip CNAME uncloaking if there is currently a configured proxy. - Profile* profile = Profile::FromBrowserContext(ctx->browser_context); - ProxyConfigDictionary dict( - profile->GetPrefs()->GetDictionary(proxy_config::prefs::kProxy)->Clone()); - ProxyPrefs::ProxyMode mode; - - if (!dict.GetMode(&mode) || !(mode == ProxyPrefs::ProxyMode::MODE_DIRECT || - mode == ProxyPrefs::ProxyMode::MODE_SYSTEM)) { - should_check_uncloaked = false; + if (ctx->browser_context) { + Profile* profile = Profile::FromBrowserContext(ctx->browser_context); + + std::unique_ptr config_tracker = + ProxyServiceFactory::CreatePrefProxyConfigTrackerOfProfile( + profile->GetPrefs(), nullptr); + std::unique_ptr proxy_config_service = + ProxyServiceFactory::CreateProxyConfigService(config_tracker.get()); + + net::ProxyConfigWithAnnotation config; + net::ProxyConfigService::ConfigAvailability availability = + proxy_config_service->GetLatestProxyConfig(&config); + + if (availability == + net::ProxyConfigService::ConfigAvailability::CONFIG_VALID) { + // If only particular types of network traffic are being proxied, or if no + // proxy is configured, it should be safe to continue making unproxied DNS + // queries. However, in SingleProxy mode all types of network traffic + // should go through the proxy, so additional DNS queries should be + // avoided. + if (config.value().proxy_rules().type == + net::ProxyConfig::ProxyRules::Type::PROXY_LIST) { + should_check_uncloaked = false; + } + } else if (availability == + net::ProxyConfigService::ConfigAvailability::CONFIG_PENDING) { + // Fallback to not CNAME uncloaking if the proxy configuration cannot be + // determined. + should_check_uncloaked = false; + } + + config_tracker->DetachFromPrefService(); } task_runner->PostTaskAndReplyWithResult( From 40d19a22b3141bb70ec4fab7e66545e693aeffa0 Mon Sep 17 00:00:00 2001 From: Anton Lazarev Date: Fri, 4 Jun 2021 12:44:49 -0700 Subject: [PATCH 3/5] add GN dep and extract block to separate function --- browser/net/BUILD.gn | 1 + ...ave_ad_block_tp_network_delegate_helper.cc | 79 ++++++++++--------- 2 files changed, 44 insertions(+), 36 deletions(-) diff --git a/browser/net/BUILD.gn b/browser/net/BUILD.gn index 23fd8c5be7ce..5e2772fee93e 100644 --- a/browser/net/BUILD.gn +++ b/browser/net/BUILD.gn @@ -63,6 +63,7 @@ source_set("net") { "//brave/extensions:common", "//components/content_settings/core/browser", "//components/prefs", + "//components/proxy_config", "//components/user_prefs", "//content/public/browser", "//content/public/common", diff --git a/browser/net/brave_ad_block_tp_network_delegate_helper.cc b/browser/net/brave_ad_block_tp_network_delegate_helper.cc index 8fcef05f75ee..53933af66348 100644 --- a/browser/net/brave_ad_block_tp_network_delegate_helper.cc +++ b/browser/net/brave_ad_block_tp_network_delegate_helper.cc @@ -235,6 +235,46 @@ void UseCnameResult(scoped_refptr task_runner, } } +// If only particular types of network traffic are being proxied, or if no +// proxy is configured, it should be safe to continue making unproxied DNS +// queries. However, in SingleProxy mode all types of network traffic should go +// through the proxy, so additional DNS queries should be avoided. +bool ProxySettingsAllowUncloaking(content::BrowserContext* browser_context) { + DCHECK(browser_context); + + bool can_uncloak = true; + + Profile* profile = Profile::FromBrowserContext(browser_context); + + std::unique_ptr config_tracker = + ProxyServiceFactory::CreatePrefProxyConfigTrackerOfProfile( + profile->GetPrefs(), nullptr); + std::unique_ptr proxy_config_service = + ProxyServiceFactory::CreateProxyConfigService(config_tracker.get()); + + net::ProxyConfigWithAnnotation config; + net::ProxyConfigService::ConfigAvailability availability = + proxy_config_service->GetLatestProxyConfig(&config); + + if (availability == + net::ProxyConfigService::ConfigAvailability::CONFIG_VALID) { + // PROXY_LIST corresponds to SingleProxy mode. + if (config.value().proxy_rules().type == + net::ProxyConfig::ProxyRules::Type::PROXY_LIST) { + can_uncloak = false; + } + } else if (availability == + net::ProxyConfigService::ConfigAvailability::CONFIG_PENDING) { + // Fallback to not CNAME uncloaking if the proxy configuration cannot be + // determined. + can_uncloak = false; + } + + config_tracker->DetachFromPrefService(); + + return can_uncloak; +} + void OnBeforeURLRequestAdBlockTP(const ResponseCallback& next_callback, std::shared_ptr ctx) { DCHECK_CURRENTLY_ON(content::BrowserThread::UI); @@ -247,45 +287,12 @@ void OnBeforeURLRequestAdBlockTP(const ResponseCallback& next_callback, // DoH or standard DNS queries won't be routed through Tor, so we need to // skip it. + // Also, skip CNAME uncloaking if there is currently a configured proxy. bool should_check_uncloaked = base::FeatureList::IsEnabled( brave_shields::features::kBraveAdblockCnameUncloaking) && - ctx->browser_context && !ctx->browser_context->IsTor(); - - // Also, skip CNAME uncloaking if there is currently a configured proxy. - if (ctx->browser_context) { - Profile* profile = Profile::FromBrowserContext(ctx->browser_context); - - std::unique_ptr config_tracker = - ProxyServiceFactory::CreatePrefProxyConfigTrackerOfProfile( - profile->GetPrefs(), nullptr); - std::unique_ptr proxy_config_service = - ProxyServiceFactory::CreateProxyConfigService(config_tracker.get()); - - net::ProxyConfigWithAnnotation config; - net::ProxyConfigService::ConfigAvailability availability = - proxy_config_service->GetLatestProxyConfig(&config); - - if (availability == - net::ProxyConfigService::ConfigAvailability::CONFIG_VALID) { - // If only particular types of network traffic are being proxied, or if no - // proxy is configured, it should be safe to continue making unproxied DNS - // queries. However, in SingleProxy mode all types of network traffic - // should go through the proxy, so additional DNS queries should be - // avoided. - if (config.value().proxy_rules().type == - net::ProxyConfig::ProxyRules::Type::PROXY_LIST) { - should_check_uncloaked = false; - } - } else if (availability == - net::ProxyConfigService::ConfigAvailability::CONFIG_PENDING) { - // Fallback to not CNAME uncloaking if the proxy configuration cannot be - // determined. - should_check_uncloaked = false; - } - - config_tracker->DetachFromPrefService(); - } + ctx->browser_context && !ctx->browser_context->IsTor() && + ProxySettingsAllowUncloaking(ctx->browser_context); task_runner->PostTaskAndReplyWithResult( FROM_HERE, From 0a1d47930bd4f300aab7638446bc44a100cdc478 Mon Sep 17 00:00:00 2001 From: Anton Lazarev Date: Fri, 4 Jun 2021 16:32:28 -0700 Subject: [PATCH 4/5] extra logging to debug windows issues --- browser/net/brave_ad_block_tp_network_delegate_helper.cc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/browser/net/brave_ad_block_tp_network_delegate_helper.cc b/browser/net/brave_ad_block_tp_network_delegate_helper.cc index 53933af66348..6ea1bcb6c5a8 100644 --- a/browser/net/brave_ad_block_tp_network_delegate_helper.cc +++ b/browser/net/brave_ad_block_tp_network_delegate_helper.cc @@ -258,20 +258,24 @@ bool ProxySettingsAllowUncloaking(content::BrowserContext* browser_context) { if (availability == net::ProxyConfigService::ConfigAvailability::CONFIG_VALID) { + LOG(ERROR) << "CONFIG_VALID"; // PROXY_LIST corresponds to SingleProxy mode. if (config.value().proxy_rules().type == net::ProxyConfig::ProxyRules::Type::PROXY_LIST) { + LOG(ERROR) << "PROXY_LIST"; can_uncloak = false; } } else if (availability == net::ProxyConfigService::ConfigAvailability::CONFIG_PENDING) { // Fallback to not CNAME uncloaking if the proxy configuration cannot be // determined. + LOG(ERROR) << "CONFIG_PENDING"; can_uncloak = false; } config_tracker->DetachFromPrefService(); + LOG(ERROR) << can_uncloak; return can_uncloak; } @@ -294,6 +298,8 @@ void OnBeforeURLRequestAdBlockTP(const ResponseCallback& next_callback, ctx->browser_context && !ctx->browser_context->IsTor() && ProxySettingsAllowUncloaking(ctx->browser_context); + LOG(ERROR) << "should_check_uncloaked: " << should_check_uncloaked; + task_runner->PostTaskAndReplyWithResult( FROM_HERE, base::BindOnce(&ShouldBlockRequestOnTaskRunner, ctx, EngineFlags(), From 8832f8f0dc38e6266f345b3a639f8795a3167789 Mon Sep 17 00:00:00 2001 From: Anton Lazarev Date: Thu, 10 Jun 2021 20:15:29 -0700 Subject: [PATCH 5/5] remove logging and CONFIG_PENDING fallback --- .../net/brave_ad_block_tp_network_delegate_helper.cc | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/browser/net/brave_ad_block_tp_network_delegate_helper.cc b/browser/net/brave_ad_block_tp_network_delegate_helper.cc index 6ea1bcb6c5a8..33464a966b77 100644 --- a/browser/net/brave_ad_block_tp_network_delegate_helper.cc +++ b/browser/net/brave_ad_block_tp_network_delegate_helper.cc @@ -258,24 +258,15 @@ bool ProxySettingsAllowUncloaking(content::BrowserContext* browser_context) { if (availability == net::ProxyConfigService::ConfigAvailability::CONFIG_VALID) { - LOG(ERROR) << "CONFIG_VALID"; // PROXY_LIST corresponds to SingleProxy mode. if (config.value().proxy_rules().type == net::ProxyConfig::ProxyRules::Type::PROXY_LIST) { - LOG(ERROR) << "PROXY_LIST"; can_uncloak = false; } - } else if (availability == - net::ProxyConfigService::ConfigAvailability::CONFIG_PENDING) { - // Fallback to not CNAME uncloaking if the proxy configuration cannot be - // determined. - LOG(ERROR) << "CONFIG_PENDING"; - can_uncloak = false; } config_tracker->DetachFromPrefService(); - LOG(ERROR) << can_uncloak; return can_uncloak; } @@ -298,8 +289,6 @@ void OnBeforeURLRequestAdBlockTP(const ResponseCallback& next_callback, ctx->browser_context && !ctx->browser_context->IsTor() && ProxySettingsAllowUncloaking(ctx->browser_context); - LOG(ERROR) << "should_check_uncloaked: " << should_check_uncloaked; - task_runner->PostTaskAndReplyWithResult( FROM_HERE, base::BindOnce(&ShouldBlockRequestOnTaskRunner, ctx, EngineFlags(),