Fix Tor dns leak #7769

darkdh · 2021-02-02T00:57:51Z

This PR

Disable CNAME adblock for Tor to prevent it from sending DoH or standard DNS query which will not be routed through Tor

1. Fixes DoH requests should respect proxy config, matching the behavior as Firefox, so that DoH requests will route through Tor
2. Enforce DoH for DNS queries sent by CNAME adblock because users can turn DoH off and we currently don't have a way to route insecure DNS lookup through Tor(proxy config). We can consider implementing Proxy DNS when using SOCKS v5(network.proxy.socks_remote_dns) that Firefox has so we don't have to enforce DoH for this
3. Skip CNAME adblock DNS lookup if DoH mode is not secure or DoH server list is empty

Submitter Checklist:

There is a ticket for my issue.
Used Github auto-closing keywords in the commit message.
Wrote a good PR/commit description
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed).
Requested a security/privacy review as needed.

Reviewer Checklist:

New files have MPL-2.0 license header.
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

DoH off

Follow the section Using the (Pre)-Master-Secret to setup SSLKEYLOGFILE
https://wiki.wireshark.org/TLS
After Wireshark is capturing traffic, use the dns filter
Launch Brave
Go to brave://settings/security and turn off Use secure DNS
Open Tor window
Navigate to https://tools.ietf.org/
After site loaded, stop Wireshark capturing
There shouldn't be any DNS query or DoH query for tools.ietf.org

DoH enabled

Follow the section Using the (Pre)-Master-Secret to setup SSLKEYLOGFILE
https://wiki.wireshark.org/TLS
After Wireshark is capturing traffic, use the dns filter
Launch Brave
Go to brave://settings/security and make sure Use secure DNS is on and use custom Cloudflare resolver
Open Tor window
Navigate to https://tools.ietf.org/
After site loaded, stop Wireshark capturing
There shouldn't be any DNS query or DoH query for tools.ietf.org

darkdh · 2021-02-02T00:59:29Z

browser/net/brave_ad_block_tp_network_delegate_helper.cc

@@ -148,13 +161,35 @@ class AdblockCnameResolveHostClient : public network::mojom::ResolveHostClient {
    // See https://crbug.com/872665
    optional_parameters->source = net::HostResolverSource::DNS;


ResolveHostParameters::secure_dns_mode_override for SECURE mode is useless without specifying doh_server.
So we have to do it through DnsConfigOverrides

antonok-edm · 2021-02-02T03:28:11Z

browser/net/brave_ad_block_tp_network_delegate_helper.cc

+      // Enforce DoH for Tor
+      // TODO(darkdh): we can consider implementing
+      // "Proxy DNS when using SOCKS v5" (network.proxy.socks_remote_dns)
+      // like Firefox has so that we don't have to enforce DoH


Just a heads up about brave/brave-browser#13414 - we should be able to remove most of this machinery once we rebase onto CR89, so I'd recommend keeping the fixes here as minimal as possible!

It seems like we should be setting this stuff when the Tor connection is created because presumably we need to always use it? Not just for this?

Normally, when we are in Tor window, we don't need to send ANY dns queries. We route all the HTTP/HTTPS traffic to tor proxy without knowing the IP, that means we just send hostname to proxy and the Tor exit node will do the resolving.

diracdeltas

this is great but i don't think we should enable DoH for users who haven't picked a DoH provider, because IIUC they need to consent to the provider's terms of service. for users who haven't picked a DoH provider, can we just disable cname adblocking for them in Tor mode?

bridiver · 2021-02-03T00:26:46Z

patches/net-dns-dns_transaction.cc.patch

+     request_->SetExtraRequestHeaders(extra_request_headers);
+     // Disable secure DNS for any DoH server hostname lookups to avoid deadlock.
+     request_->SetDisableSecureDns(true);
+-    request_->SetLoadFlags(request_->load_flags() | LOAD_DISABLE_CACHE |


I think you can avoid this patch by defining LOAD_BYPASS_PROXY as LOAD_DISABLE_CACHE. If that doesn't work for some reason you should use a define/chromium_src override after the original call instead of changing it to minimize the patch

iefremov · 2021-02-03T12:56:22Z

browser/net/brave_ad_block_tp_network_delegate_helper_unittest.cc

  auto request_info = std::make_shared<brave::BraveRequestInfo>(GURL());
  int rc =
      OnBeforeURLRequest_AdBlockTPPreWork(ResponseCallback(), request_info);
  EXPECT_TRUE(request_info->new_url_spec.empty());
  EXPECT_EQ(rc, net::OK);
 }
+
+TEST_F(BraveAdBlockTPNetworkDelegateHelperTest, ByassTor) {


actually I think this is a bad name because it makes it sound like it should be bypassing Tor and that's exactly the opposite of what we are doing. Maybe something like DisableInsecureRequestsOverTor

iefremov · 2021-02-03T12:58:26Z

browser/net/brave_ad_block_tp_network_delegate_helper_unittest.cc

+};
+}  // namespace
+
+class BraveAdBlockTPNetworkDelegateHelperTest : public testing::Test {


this fixture is not related to existing tests, so you could leave them as is and give the test class more relevant name

iefremov · 2021-02-03T13:18:31Z

chromium_src/net/dns/dns_transaction.cc

+
+#include "net/base/load_flags.h"
+
+#define LOAD_BYPASS_PROXY LOAD_DISABLE_CACHE


I really don't think we should do this for all requests. I checked git history around this code (https://chromium-review.googlesource.com/c/chromium/src/+/1843334/10/net/dns/dns_transaction.cc), and the authors say LOAD_BYPASS_PROXY is here to prevent deadlocks.

So we should either try upstreaming this or limit to tor-related requests

That is for SetDisableSecureDns (https://chromium-review.googlesource.com/c/chromium/src/+/1843332/5/net/dns/dns_transaction.cc)
It's been there since the first DoH commit
https://chromium-review.googlesource.com/c/chromium/src/+/710554/40/net/dns/dns_transaction.cc#431

yea, I actually can't really imagine why we'd ever want to bypass the proxy for DoH requests. That seems like it would actually prevent requests from working at all if the proxy was required for access and non-proxy requests are blocked by firewall

There is a discussion with upstream net dev
https://groups.google.com/u/1/a/chromium.org/g/net-dev/c/GI4E9ERwPgw

darkdh · 2021-02-03T22:25:42Z

per discussion on slack with @bridiver and @iefremov, we came to a conclusion that disabling CNAME adblock for Tor would be best option now. Considering in order to make DoH route through Tor, we need to remove LOAD_BYPASS_PROXY for dns transaction but it might introduce dns and proxy code looping when we need to resolve proxy name. see (https://groups.google.com/u/1/a/chromium.org/g/net-dev/c/GI4E9ERwPgw)
So if we want to route DoH through Tor, we need fine grained solution for it and also need a bigger discussion about this with privacy and security

fmarier · 2021-02-03T22:59:25Z

we came to a conclusion that disabling CNAME adblock for Tor would be best option now. Considering in order to make DoH route through Tor, we need to remove LOAD_BYPASS_PROXY for dns transaction but it might introduce dns and proxy code looping when we need to resolve proxy name.

Sounds like a good compromise to me.

iefremov · 2021-02-04T09:30:22Z

browser/net/brave_ad_block_tp_network_delegate_helper.cc

+  // DoH or standard DNS quries won't be routed through Tor, so we need to skip
+  // it.
+  if (ctx->browser_context->IsTor()) {
+    return net::OK;


This is going to disable adblocker in tor windows. What we need to do here is an adblocking pass without CNAME resolving, e.g. we can directly call ShouldBlockAdWithOptionalCname with empty CNAME

Tested on Firefox: When DoH is enabled, the requests will route through proxy

because we currently have no better way to route insecure DNS query through proxy like "Proxy DNS when using SOCKS v5" (network.proxy.socks_remote_dns) that Firefox has

darkdh requested review from fmarier, diracdeltas, bbondy, iefremov and antonok-edm February 2, 2021 00:57

darkdh requested a review from a team as a code owner February 2, 2021 00:57

darkdh self-assigned this Feb 2, 2021

darkdh commented Feb 2, 2021

View reviewed changes

darkdh force-pushed the tor-dns-leak branch from 143b511 to 16a7c24 Compare February 2, 2021 01:08

antonok-edm approved these changes Feb 2, 2021

View reviewed changes

diracdeltas requested changes Feb 2, 2021

View reviewed changes

bridiver reviewed Feb 3, 2021

View reviewed changes

darkdh force-pushed the tor-dns-leak branch from 1f1759b to b5bf0de Compare February 3, 2021 01:42

iefremov reviewed Feb 3, 2021

View reviewed changes

diracdeltas self-requested a review February 3, 2021 18:55

darkdh force-pushed the tor-dns-leak branch 2 times, most recently from dbb13b2 to 689261a Compare February 3, 2021 22:16

darkdh requested review from iefremov and bridiver February 4, 2021 02:53

iefremov reviewed Feb 4, 2021

View reviewed changes

darkdh added 4 commits February 4, 2021 10:29

DoH requests should respect proxy config

5e099e8

Tested on Firefox: When DoH is enabled, the requests will route through proxy

Enforce DoH for the CNAME adblock DNS query for Tor context

7455704

because we currently have no better way to route insecure DNS query through proxy like "Proxy DNS when using SOCKS v5" (network.proxy.socks_remote_dns) that Firefox has

Bypass CNAME adblocking for Tor when DoH is not enabled by users

47118d9

Disable CNAME adblock for Tor

a148c43

darkdh force-pushed the tor-dns-leak branch from 689261a to a148c43 Compare February 4, 2021 18:30

iefremov approved these changes Feb 4, 2021

View reviewed changes

diracdeltas approved these changes Feb 4, 2021

View reviewed changes

darkdh merged commit 12fe321 into master Feb 4, 2021

darkdh deleted the tor-dns-leak branch February 4, 2021 21:32

darkdh added this to the 1.22.x - Nightly milestone Feb 4, 2021

brave-builds pushed a commit that referenced this pull request Feb 10, 2021

Uplift of #7769 (squashed) to beta

91e9db5

brave-builds mentioned this pull request Feb 10, 2021

Fix Tor dns leak (uplift to 1.21.x) #7909

Merged

7 tasks

bbondy mentioned this pull request Feb 19, 2021

Specify resolver source to avoid using system resolver for CNAME adblocking (uplift to 1.20.x) #8010

Merged

7 tasks

stephendonner mentioned this pull request Feb 19, 2021

[hackerone] Tor DNS issue brave/brave-browser#13527

Closed

antonok-edm mentioned this pull request May 4, 2021

Only check CNAME for unblocked requests #8704

Merged

24 tasks

antonok-edm mentioned this pull request Jul 28, 2022

Expand CNAME uncloaking protection #14392

Closed

25 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Tor dns leak #7769

Fix Tor dns leak #7769

darkdh commented Feb 2, 2021 •

edited

Loading

darkdh Feb 2, 2021

antonok-edm Feb 2, 2021

bridiver Feb 3, 2021

darkdh Feb 3, 2021

diracdeltas left a comment

bridiver Feb 3, 2021

darkdh Feb 3, 2021

iefremov Feb 3, 2021

bridiver Feb 3, 2021

darkdh Feb 3, 2021

iefremov Feb 3, 2021

darkdh Feb 3, 2021

iefremov Feb 3, 2021

darkdh Feb 3, 2021 •

edited

Loading

bridiver Feb 3, 2021

darkdh Feb 3, 2021

darkdh commented Feb 3, 2021

fmarier commented Feb 3, 2021

iefremov Feb 4, 2021

darkdh Feb 4, 2021

		@@ -148,13 +161,35 @@ class AdblockCnameResolveHostClient : public network::mojom::ResolveHostClient {
		// See https://crbug.com/872665
		optional_parameters->source = net::HostResolverSource::DNS;


		#include "net/base/load_flags.h"

		#define LOAD_BYPASS_PROXY LOAD_DISABLE_CACHE

Fix Tor dns leak #7769

Fix Tor dns leak #7769

Conversation

darkdh commented Feb 2, 2021 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

DoH off

DoH enabled

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diracdeltas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkdh Feb 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkdh commented Feb 3, 2021

fmarier commented Feb 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkdh commented Feb 2, 2021 •

edited

Loading

darkdh Feb 3, 2021 •

edited

Loading