Skip to content
This repository has been archived by the owner on Jan 4, 2019. It is now read-only.

Tor browser context #473

Merged
merged 36 commits into from
Jul 18, 2018
Merged

Tor browser context #473

merged 36 commits into from
Jul 18, 2018

Conversation

darkdh
Copy link
Member

@darkdh darkdh commented Feb 1, 2018

  1. context creating options: "isolated_storage", "tor_proxy"
  2. setTorNewIdentity API
  3. automatically launch tor when tor browser context created and tear it
    down when tor browser context destroyed

fix #468
fix #464
fix #509

browser-laptop changes: https://github.com/brave/browser-laptop/tree/feature/tor

Auditors: @bridiver, @riastradh-brave, @diracdeltas

@darkdh darkdh self-assigned this Feb 1, 2018
darkdh added a commit to brave/browser-laptop that referenced this pull request Feb 1, 2018
@diracdeltas
Copy link
Member

lgtm but don't we still need to set ShouldUseProcessPerSite to true for the Tor browser context in order to ensure that different origins do not share a Site Instance?

@darkdh
Copy link
Member Author

darkdh commented Feb 2, 2018

yeah, you are right. I forgot to do that.

diracdeltas pushed a commit to brave/browser-laptop that referenced this pull request Feb 6, 2018
@@ -643,6 +751,23 @@ void BraveBrowserContext::SetExitType(ExitType exit_type) {
}
}

void BraveBrowserContext::SetTorNewIdentity(const GURL& url,
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is this used for?

Copy link
Member

Choose a reason for hiding this comment

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

it's tied to a button that the user clicks when they want a new Tor circuit. https://tb-manual.torproject.org/en-US/managing-identities.html

Copy link
Collaborator

Choose a reason for hiding this comment

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

this doesn't seem to match the described functionality - "This option is useful if you want to prevent your subsequent browser activity from being linkable to what you were doing before. Selecting it will close all your open tabs and windows, clear all private information such as cookies and browsing history, and use new Tor circuits for all connections"

Copy link
Collaborator

Choose a reason for hiding this comment

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

it looks like it just creates a new tor proxy, but based on the description I think each Tor identify would be similar to a session tab and create a new browser context

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, it should be this New Tor Circuit for this Site

Copy link
Collaborator

@bridiver bridiver Feb 8, 2018

Choose a reason for hiding this comment

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

if we're trying to make this friendly for non-technical users I don't think New Tor Circuit for this site is going to help. I even had to dig a little bit to understand exactly what that means. Why don't we just do this anytime a user reloads a url (explicit reload vs normal navigation)?

Copy link
Member Author

Choose a reason for hiding this comment

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

We've already used different tor circuits for different sites. This function enable users to change it.
cc @riastradh-brave

Copy link
Contributor

Choose a reason for hiding this comment

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

The user-visible operation was just copied from the Tor Browser. It has not undergone any UX review on the Brave side, which certainly ought to happen before this is released. However, I expect that whatever manifestation it has to the user, we will probably want an operation to require a new Tor circuit for an origin -- possibly never used on its own, always in conjunction with other things, but I can't see that far ahead right now.

*partition_domain = site.host();
*partition_name = site.host();
}
*in_memory = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be set to brave_browser_context->IsOffTheRecord() so we don't run into problems later if we add support for persistent tor tabs

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 for the case of IsIsolatedStorage and it is always in memory https://github.com/brave/muon/pull/473/files#diff-df5c427fd693f9ebb2e5952cf0e9c8c7R388

Copy link
Collaborator

Choose a reason for hiding this comment

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

IsIsolatedStorage does not imply in_memory so I still think this should use IsOffTheRecord

StoragePartitionDescriptor descriptor(partition_path, in_memory);
URLRequestContextGetterMap::iterator iter =
url_request_context_getter_map_.find(descriptor);
if (iter != url_request_context_getter_map_.end())
Copy link
Collaborator

Choose a reason for hiding this comment

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

lint?

@@ -363,6 +436,36 @@ void BraveBrowserContext::UpdateDefaultZoomLevel() {
->OnDefaultZoomLevelChanged();
}

void BraveBrowserContext::TorSetProxy(
Copy link
Collaborator

Choose a reason for hiding this comment

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

these methods have to be called on the IO thread

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed in 393465d

Copy link
Collaborator

@bridiver bridiver left a comment

Choose a reason for hiding this comment

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

IO/UI thread issues

@darkdh darkdh changed the title Adding tor browser context support "isolated_storage" and "tor_proxy" options Tor browser context Feb 16, 2018
@darkdh darkdh force-pushed the tor_browser_context branch from f2df25a to a7e7661 Compare February 28, 2018 23:33
diracdeltas pushed a commit to brave/browser-laptop that referenced this pull request Feb 28, 2018
if (options.GetString("tor_proxy", &tor_proxy)) {
tor_proxy_ = GURL(tor_proxy);
if (!tor_process_.IsValid()) {
base::FilePath tor("/usr/local/bin/tor");
Copy link
Member Author

Choose a reason for hiding this comment

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

@jumde this path need to be changed depends on the binary location we want to bundle

Copy link
Member

Choose a reason for hiding this comment

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

i would slightly prefer that this be ./app/extensions/bin/tor (relative to root of browser-laptop) similar to what we will do with geth: https://github.com/brave/browser-laptop/pull/13177/files#diff-a108ee634057ec3892dd3354639d2117R11.

see getExtensionsPath in browser-laptop for how to get this path

Copy link
Member Author

Choose a reason for hiding this comment

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

that's better. we can export a option tor_bin_path and passing it from browser-laptop

Copy link
Member Author

Choose a reason for hiding this comment

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

and we can have a unified path among different platforms

@darkdh
Copy link
Member Author

darkdh commented Mar 20, 2018

rebased to current master with C65

darkdh and others added 20 commits July 6, 2018 09:17
This is necessary because there is an unfortunate ordering issue with
tor startup: it writes the control port first, and then the auth
cookie, but we need both in order to connect to the control port.
And it doesn't delete the auth cookie, so it can get stale.  Hence we
need to monitor writes to the auth cookie too.
This is necessary because we use `persist:tor' since for hysterical
raisins there's only one normal `private' partition with in_memory_ =
true.  We use the virtual method IsOffTheRecord() to discriminate
instead.

Fixes #608.
Fixes brave/browser-laptop#14392.

Auditors: @darkdh
…ion.getTorPid()

NOTE:
Use setTorLauncherCallback right after Session.fromPartition for tor browser context
if you want to get pid after launch

Auditors: @riastradh-brave, @diracdeltas
because we don't want to spend extra cycles to keep track of expired
keys

fix #611

Auditors: @riastradh-brave
Don't just expire any old entries for the site we're browsing -- that
may leave lots of other ones around in memory.
The timer is scheduled to run ten minutes after the last circuit that
was created.  This way, the last ten minutes of circuits are not
guaranteed to stick around in memory indefinitely.

Caveat: This doesn't _zero_ the memory, so it may still appear in
`strings /proc/N/mem`.  But it does make the memory available to be
recycled, so it's not _guaranteed_ to still appear in `strings
/proc/N/mem`.

Also, timestamp the map entries.  If we explicitly create a new map
entry for a site by requesting a new identity, the old expiry queue
entry will not delete it, but a new expiry queue entry will delete
it.  This way, circuits created by requesting a new identity are not
shorter-lived than other circuits.

We leave the old entries in the priority queue because there's no
convenient way to delete them with std::priority_queue.  In
principle, this might leak space if you repeatedly request a new
identity, but it can only leak as much space as you use by repeatedly
requesting a new identity for a maximum of ten minutes.

fix #611 real good this time

Auditors: @darkdh

Test Plan:
1. Search DDG for `what is my ip address'.
2. Record the IP address it reports.
3. Reload.
4. Confirm it's the same IP address.
5. Full-reload.
6. Confirm it's a different IP address.  Record the new IP address.
7. Wait >10min.
8. Reload.
9. Confirm it's a different IP address again.
@darkdh darkdh force-pushed the tor_browser_context branch from cf215a0 to 845d425 Compare July 6, 2018 16:17
@darkdh darkdh force-pushed the tor_browser_context branch from 845d425 to 1ee6a7f Compare July 12, 2018 22:17
darkdh added 4 commits July 12, 2018 15:19
…lity process

fix brave/browser-laptop#14636

SuicideOnChannelErrorFilter calls exit in OnChannelError() this will
cause other endpoints listener can't finish their cleanup when pipe error
happens(browser process crashed or be killed) and
SuicideOnChannelErrorFilter::OnChannelError happens to be called before others.
This should be fine for most of the cases but not tor_launcher service.
`TorLauncher` requires StrongBinding::OnConnectionError to delete itself
so that `~TorLauncher` will get called and terminate tor process.

This should only happens on MacOS, SuicideOnChannelErrorFilter is
guarded by OS_POSIX and Linux has `prctl(PR_SET_PDEATHSIG, SIGKILL)`
so tor process will receive SIGKILL when tor_launcher utility process terminates
prematurely

Auditors: @riastradh-brave, @bridiver, @bbondy
…er_suppressed(noopener) specified

because WebContentsImpl::CreateNewWindow will use target_url as new site instance

The problem was cloning original site instance cause the inconsistency
between original partition and target partition because tor browser
context enforce isolation storage so every different site has its own storage partition

fix brave/browser-laptop#14392

Test Plan:
1. Open tor tab
2. Visit site contains rel="noopener" href (https://jsfiddle.net/dqokhmsg/)
3. Click the link
4. Brave shouldn't crash

Auditors: @bridiver, @bbondy
Prevent `SuicideOnChannelErrorFilter` to be added to tor_launcher utility process
Use new site instance for SessionStorageNamespaceImpl clone when opener_suppressed(noopener) specified
@darkdh darkdh merged commit 4ffc7f0 into master Jul 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants