From e599632d8180d03590f00732aea0b437425ad5c0 Mon Sep 17 00:00:00 2001 From: Anton Lazarev Date: Thu, 18 Nov 2021 18:31:29 -0800 Subject: [PATCH 1/2] disable CNAME uncloaking when DoH is enabled with an HTTPS proxy --- ...ave_ad_block_tp_network_delegate_helper.cc | 21 +++++++++++++++++-- 1 file changed, 19 insertions(+), 2 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 f5331eee7e08..e47bcc8d0b6a 100644 --- a/browser/net/brave_ad_block_tp_network_delegate_helper.cc +++ b/browser/net/brave_ad_block_tp_network_delegate_helper.cc @@ -289,7 +289,12 @@ void UseCnameResult(scoped_refptr task_runner, // case of per-scheme proxy configurations, a fallback for any non-matching // request can be configured, in which case additional DNS queries should be // avoided as well. -bool ProxySettingsAllowUncloaking(content::BrowserContext* browser_context) { +// +// For some reason, when DoH is enabled alongside a system HTTPS proxy, the +// CNAME queries here are also not proxied. So uncloaking is disabled in that +// case as well. +bool ProxySettingsAllowUncloaking(content::BrowserContext* browser_context, + bool doh_enabled) { DCHECK(browser_context); bool can_uncloak = true; @@ -317,6 +322,12 @@ bool ProxySettingsAllowUncloaking(content::BrowserContext* browser_context) { !config.value().proxy_rules().fallback_proxies.IsEmpty())) { can_uncloak = false; } + + if (config.value().proxy_rules().type == + net::ProxyConfig::ProxyRules::Type::PROXY_LIST_PER_SCHEME && + !config.value().proxy_rules().proxies_for_https.IsEmpty()) { + can_uncloak = false; + } } config_tracker->DetachFromPrefService(); @@ -334,6 +345,12 @@ void OnBeforeURLRequestAdBlockTP(const ResponseCallback& next_callback, scoped_refptr task_runner = g_brave_browser_process->ad_block_service()->GetTaskRunner(); + SecureDnsConfig secure_dns_config = + SystemNetworkContextManager::GetStubResolverConfigReader() + ->GetSecureDnsConfiguration(false); + + bool doh_enabled = (secure_dns_config.mode() == net::SecureDnsMode::kSecure); + // 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. @@ -341,7 +358,7 @@ void OnBeforeURLRequestAdBlockTP(const ResponseCallback& next_callback, base::FeatureList::IsEnabled( brave_shields::features::kBraveAdblockCnameUncloaking) && ctx->browser_context && !ctx->browser_context->IsTor() && - ProxySettingsAllowUncloaking(ctx->browser_context); + ProxySettingsAllowUncloaking(ctx->browser_context, doh_enabled); // When default 1p blocking is disabled, first-party requests should not be // CNAME uncloaked unless using aggressive blocking mode. From 892959923b011e5a978dfd3c14fb125d99bdd969 Mon Sep 17 00:00:00 2001 From: Anton Lazarev Date: Fri, 19 Nov 2021 13:40:55 -0800 Subject: [PATCH 2/2] fix tests --- ...ock_tp_network_delegate_helper_unittest.cc | 28 +++++++++++++++---- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/browser/net/brave_ad_block_tp_network_delegate_helper_unittest.cc b/browser/net/brave_ad_block_tp_network_delegate_helper_unittest.cc index 8145ad2f3a00..d37f8583c1ef 100644 --- a/browser/net/brave_ad_block_tp_network_delegate_helper_unittest.cc +++ b/browser/net/brave_ad_block_tp_network_delegate_helper_unittest.cc @@ -18,7 +18,10 @@ #include "brave/components/brave_shields/browser/ad_block_subscription_download_manager.h" #include "brave/components/brave_shields/browser/ad_block_subscription_service_manager.h" #include "brave/test/base/testing_brave_browser_process.h" +#include "chrome/browser/net/system_network_context_manager.h" #include "chrome/common/chrome_paths.h" +#include "chrome/test/base/scoped_testing_local_state.h" +#include "chrome/test/base/testing_browser_process.h" #include "content/public/test/browser_task_environment.h" #include "net/base/net_errors.h" #include "net/dns/mock_host_resolver.h" @@ -37,7 +40,8 @@ namespace { // "external" runner. class TestingBraveComponentUpdaterDelegate : public BraveComponent::Delegate { public: - TestingBraveComponentUpdaterDelegate() = default; + explicit TestingBraveComponentUpdaterDelegate(PrefService* local_state) + : local_state_(local_state) {} ~TestingBraveComponentUpdaterDelegate() override = default; TestingBraveComponentUpdaterDelegate(TestingBraveComponentUpdaterDelegate&) = @@ -63,9 +67,10 @@ class TestingBraveComponentUpdaterDelegate : public BraveComponent::Delegate { } const std::string locale() const override { return "en"; } - PrefService* local_state() override { - return nullptr; - } + PrefService* local_state() override { return local_state_; } + + private: + PrefService* local_state_; }; } // namespace @@ -79,8 +84,12 @@ void FakeAdBlockSubscriptionDownloadManagerGetter( class BraveAdBlockTPNetworkDelegateHelperTest : public testing::Test { protected: void SetUp() override { + local_state_ = std::make_unique( + TestingBrowserProcess::GetGlobal()); + brave_component_updater_delegate_ = - std::make_unique(); + std::make_unique( + local_state_->Get()); base::FilePath user_data_dir; DCHECK(base::PathService::Get(chrome::DIR_USER_DATA, &user_data_dir)); @@ -100,6 +109,11 @@ class BraveAdBlockTPNetworkDelegateHelperTest : public testing::Test { resolver_wrapper_ = std::make_unique( host_resolver_.get(), net::NetLog::Get()); brave::SetAdblockCnameHostResolverForTesting(resolver_wrapper_.get()); + + stub_resolver_config_reader_ = + std::make_unique(local_state_->Get()); + SystemNetworkContextManager::set_stub_resolver_config_reader_for_testing( + stub_resolver_config_reader_.get()); } void TearDown() override { @@ -129,6 +143,8 @@ class BraveAdBlockTPNetworkDelegateHelperTest : public testing::Test { return rc == net::ERR_IO_PENDING; } + std::unique_ptr local_state_; + std::unique_ptr brave_component_updater_delegate_; @@ -136,6 +152,8 @@ class BraveAdBlockTPNetworkDelegateHelperTest : public testing::Test { std::unique_ptr host_resolver_; + std::unique_ptr stub_resolver_config_reader_; + private: std::unique_ptr resolver_wrapper_; };