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

Geolocation prompt is not blocked on Tor Windows #1353

Closed
srirambv opened this issue Sep 29, 2018 · 10 comments · Fixed by brave/brave-core#571 or brave/brave-core#582
Closed

Geolocation prompt is not blocked on Tor Windows #1353

srirambv opened this issue Sep 29, 2018 · 10 comments · Fixed by brave/brave-core#571 or brave/brave-core#582

Comments

@srirambv
Copy link
Contributor

Description

Geolocation prompt is not blocked on Tor Windows

Steps to Reproduce

  1. Open a Tor Window
  2. Visit https://browserleaks.com/geo
  3. Location prompt bubble is shown

Actual result:

image

Expected result:

Location prompt should be blocked by default on Tor tabs/Windows

Reproduces how often:

Easy

Brave version (chrome://version info)

Brave 0.55.10 Chromium: 70.0.3538.22 (Official Build) beta (64-bit)
Revision ac9418ba9c3bd7f6baaffa0b055dfe147e0f8364-refs/branch-heads/3538@{#468}
OS All

Reproducible on current release:

B-l blocks the request by default

Website problems only:

  • Does the issue resolve itself when disabling Brave Shields?
  • Is the issue reproducible on the latest version of Chrome?

Additional Information

cc: @diracdeltas @tomlowenthal to change blocking label if required and change milestone

@srirambv srirambv added feature/tor release/blocking feature/tor/leakproofing Eliminating unexpected ways that someone using Tor might be unmasked. labels Sep 29, 2018
@srirambv srirambv added this to the Releasable builds 0.55.x milestone Sep 29, 2018
@bbondy
Copy link
Member

bbondy commented Sep 29, 2018

For me a prompt is expected.
I will move to 1.x but I'd like a discussion for why it should not allow it at all.

@bbondy bbondy modified the milestones: Releasable builds 0.55.x, 1.x Backlog Sep 29, 2018
@diracdeltas
Copy link
Member

diracdeltas commented Sep 29, 2018

@bbondy IIRC the issue was that that the geolocation code in Chromium leaks the true IP of the user, which goes against Tor's purpose. so in b-l we auto-denied geolocation requests. cc @bridiver

@diracdeltas diracdeltas modified the milestones: 1.x Backlog, 1.0 (0.56.x) Sep 29, 2018
@bbondy
Copy link
Member

bbondy commented Sep 29, 2018

I'm aware that getCurrentPosition can use the Tor exit node IP to determine a fuzzier location (which isn't hidden anyway) but I don't think it exposes the user's true IP or anything like that.

@bbondy
Copy link
Member

bbondy commented Sep 29, 2018

I'd like for a real threat to be determined precisely, or at least explained before we pull it up from the backlog.

@bbondy bbondy modified the milestones: 1.0 (0.56.x), 1.x Backlog Sep 29, 2018
@bbondy
Copy link
Member

bbondy commented Sep 29, 2018

Talked about it on Slack, BrianJ mentioned that it uses the system request context for the request, so it doesn't use Tor currently. Moved to 1.0

@bbondy bbondy modified the milestones: 1.x Backlog, 1.0 (0.56.x) Sep 29, 2018
@diracdeltas diracdeltas modified the milestones: 1.0 (0.56.x), Releasable builds 0.55.x Sep 29, 2018
@tildelowengrimm tildelowengrimm removed the feature/tor/leakproofing Eliminating unexpected ways that someone using Tor might be unmasked. label Oct 1, 2018
@jumde jumde added the feature/tor/leakproofing Eliminating unexpected ways that someone using Tor might be unmasked. label Oct 1, 2018
@yrliou yrliou self-assigned this Oct 2, 2018
@riastradh-brave
Copy link
Contributor

For parity with browser-laptop it would be prudent to err on the side of blocking this, but as long as the geolocation query doesn't go through without explicit permission I don't think we need to make this a release blocker. If this hasn't been addressed by then, I will take a closer look at this next week when I get back from the Tor meeting to make sure that a mere geolocation request from the site (navigator.geolocation.getCurrentPosition -- any other vectors I'm missing?) can't trigger leaks in the browser.

@riastradh-brave riastradh-brave self-assigned this Oct 3, 2018
@riastradh-brave
Copy link
Contributor

Correction: on discussion with yan, I realize it is imprudent to allow the browser to perform geolocation queries without a clearer message that the geolocation is determined by a non-Tor connection, if such a message can be usefully conveyed at all. So this should remain a release blocker.

@yrliou yrliou added the QA/Yes label Oct 4, 2018
@yrliou
Copy link
Member

yrliou commented Oct 4, 2018

QA note: test plan specified in brave/brave-core#571

@emerick
Copy link
Contributor

emerick commented Oct 8, 2018

@yrliou Reverted and reopened due to build problem on Windows:

FAILED: vr_ui.dll vr_ui.dll.lib vr_ui.dll.pdb
ninja -t msvc -e environment.x64 -- ../../third_party/llvm-build/Release+Asserts/bin/lld-link.exe /nologo /IMPLIB:./vr_ui.dll.lib /DLL /OUT:./vr_ui.dll /PDB:./vr_ui.dll.pdb @./vr_ui.dll.rsp
lld-link: error: undefined symbol: ?kProfileUsingTor@prefs@tor@@3QBDB
>>> referenced by C:\brave-browser\src\brave\components\content_settings\core\browser\brave_host_content_settings_map.cc:26
>>>               obj/brave/components/content_settings/core/browser/browser/brave_host_content_settings_map.obj:(??0BraveHostContentSettingsMap@@QEAA@PEAVPrefService@@_N111@Z)
>>> referenced by C:\brave-browser\src\brave\components\content_settings\core\browser\brave_host_content_settings_map.cc:26
>>>               obj/brave/components/content_settings/core/browser/browser/brave_host_content_settings_map.obj:(??0BraveHostContentSettingsMap@@QEAA@PEAVPrefService@@_N111@Z)

@srirambv
Copy link
Contributor Author

srirambv commented Oct 10, 2018

Verification Passed on

Brave 0.55.12 Chromium: 70.0.3538.45 (Official Build) beta (64-bit)
Revision cbdc32e4334458954e9def214d7e5fa1ca1960eb-refs/branch-heads/3538@{#830}
OS Linux
  • Verified no location prompt shown on Tor Window with shields up/down

Verified passed with

Brave 0.55.12 Chromium: 70.0.3538.45 (Official Build) beta(64-bit)
Revision cbdc32e4334458954e9def214d7e5fa1ca1960eb-refs/branch-heads/3538@{#830}
OS Mac OS X

Verification Passed on

Brave 0.55.12 Chromium: 70.0.3538.45 (Official Build) beta (64-bit)
Revision cbdc32e4334458954e9def214d7e5fa1ca1960eb-refs/branch-heads/3538@{#830}
OS Windows

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment