Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Block .onion requests in non-Tor window #8040

Merged
merged 1 commit into from
Feb 23, 2021
Merged

Block .onion requests in non-Tor window #8040

merged 1 commit into from
Feb 23, 2021

Conversation

darkdh
Copy link
Member

@darkdh darkdh commented Feb 22, 2021

and show "Open in Tor" when auto redirect setting is off
Click "Open in Tor" now will have same window disposition as auto redirect

Resolves brave/brave-browser#14261

Submitter Checklist:

  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • 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:

Auto redirect off

  1. Start Wireshark and filter for dns.
  2. disable DoH in brave://settings/security (makes it easier to see requests in Wireshark)
  3. Open a normal window and type https://brave5t5rjjg3s6k.onion/ in the URL bar.
  4. Confirm that you get an error page and that the .onion didn't result in a DNS lookup.
  5. There should be a "Open in Tor" button
  6. Click it and confirm that it opens https://brave5t5rjjg3s6k.onion/ in Tor window
  7. Go back to the tab of step 2 and type https://brave5t5rjjg3s6k.com/ in the URL bar.
  8. "Open in Tor" button should be clear

Auto redirect on

  1. Start Wireshark and filter for dns.
  2. disable DoH in brave://settings/security (makes it easier to see requests in Wireshark)
  3. Go to brave://settings/extensions and turn on Automatically redirect .onion sites
  4. Open a normal window and type https://brave5t5rjjg3s6k.onion/ in the URL bar.
  5. Confirm that you get an error page and that the .onion didn't result in a DNS lookup.
  6. Tor window should be opened automatically with https://brave5t5rjjg3s6k.onion/

@darkdh darkdh self-assigned this Feb 22, 2021
@@ -52,7 +52,7 @@ void OnTorProfileCreated(GURL onion_location,
if (!browser)
return;
content::OpenURLParams open_tor(onion_location, content::Referrer(),
WindowOpenDisposition::NEW_FOREGROUND_TAB,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the leftover of #7713, "Open in Tor" and auto redirect should have same window disposition

Copy link
Member

@fmarier fmarier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

I would suggest manually testing with DoH enabled once before merging, just to make sure there aren't any leaks due to that separate DNS resolver.

@darkdh
Copy link
Member Author

darkdh commented Feb 23, 2021

unrelated npm immer audit failure which is recorded in brave/brave-browser#14305
https://ci.brave.com/blue/rest/organizations/jenkins/pipelines/pr-brave-browser-block-onion-request-post-init/runs/1/nodes/119/steps/145/log/?start=0

And @fmarier I have also tested with DoH enabled with SSLKEYLOGFILE provided to Wireshark

@darkdh darkdh merged commit f3e532e into master Feb 23, 2021
@darkdh darkdh deleted the block-onion-request branch February 23, 2021 17:22
@darkdh darkdh added this to the 1.22.x - Nightly milestone Feb 23, 2021
brave-builds pushed a commit that referenced this pull request Feb 23, 2021
darkdh added a commit that referenced this pull request Feb 23, 2021
Block .onion requests in non-Tor window
@stephendonner
Copy link
Contributor

stephendonner commented Feb 23, 2021

Verified FIXED using the testplan #8040 (comment) on:

Brave 1.22.43 Chromium: 89.0.4389.58 (Official Build) nightly (x86_64)
Revision 1a139f28ecc27719439e37c6b1533cee999cb802-refs/branch-heads/4389@{#1134}
OS macOS Version 11.2.1 (Build 20D74)
Brave 1.22.43 Chromium: 89.0.4389.58 (Official Build) nightly (64-bit)
Revision 1a139f28ecc27719439e37c6b1533cee999cb802-refs/branch-heads/4389@{#1134}
OS Windows 10 OS Version 2009 (Build 21318.1000)

Auto redirect off

  • Started Wireshark and filtered for dns.
  • disabled DoH in brave://settings/security (makes it easier to see requests in Wireshark)
  • Opened a normal window and typed https://brave5t5rjjg3s6k.onion/ in the URL bar.
  • Confirmed I got an error page and that the .onion didn't result in a DNS lookup.

Screen Shot 2021-02-23 at 2 21 47 PM

  • Verified there was an "Open in Tor" button
  • Clicked it and confirmed that it opened https://brave5t5rjjg3s6k.onion/ in Tor window
  • Went back to the tab of step 2 and typed https://brave5t5rjjg3s6k.com/ in the URL bar.
    Verified no "Open in Tor" button.

Screen Shot 2021-02-23 at 2 23 20 PM

Auto redirect on

  • Started Wireshark and filtered for dns.
  • disable DoH in brave://settings/security (makes it easier to see requests in Wireshark)
  • Went to brave://settings/extensions and turned on Automatically redirect .onion sites
  • Opened a normal window and typed https://brave5t5rjjg3s6k.onion/ in the URL bar.
  • Confirmed that I got an error page and that the .onion didn't result in a DNS lookup.
  • Confirmed that Tor window opened automatically with https://brave5t5rjjg3s6k.onion/

Screen Shot 2021-02-23 at 2 19 49 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.onion request in regular window should also avoid DNS leakage
3 participants