Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#19206 closed enhancement (fixed)

SOCKS isolation should include a process identifier.

Reported by: yawning Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: TorBrowserTeam201606R
Cc: arthuredelstein, brade, mcs, gk Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

This isn't an issue when using Tor Browser with "tor-launcher forks/execs the tor process" model, but is relevant to all other use cases, particularly with a system tor instance.

The SOCKS username/password isolation should include a instance identifier such that each invocation of Tor Browser ends up using difference circuits (Currently, the isolation tags will get reused).

The current format is domain:counter. The naive implementation of this would be something like domain:pid:counter. pid could be expanded to include process launch time information or similar to handle the PID reuse case, but I'm not sure how likely that is (the entire PID space needs to be consumed before PIDs are reused on Linux).

I filed #18125 a while ago with similar rationale in mind, but doing it this way is better, so feel free to kill off the older ticket in favor of this one.

Child Tickets

Change History (14)

comment:1 Changed 3 years ago by mcs

Cc: brade mcs added

comment:2 Changed 3 years ago by arthuredelstein

Another possible option would be to use a random string per first-party domain (such as a random UUID for the password). That would mean we don't have to obtain the PID.

comment:3 Changed 3 years ago by gk

Cc: gk added

comment:4 in reply to:  2 Changed 3 years ago by yawning

Replying to arthuredelstein:

Another possible option would be to use a random string per first-party domain (such as a random UUID for the password). That would mean we don't have to obtain the PID.

This would work as well, obtaining enough randomness to ensure collisions are unlikely is dirt cheap, and I assume changing the new circuit for site behavior to simply re-randomize is easy enough.

Is there any advantage to this behavior being configurable (beyond https://xkcd.com/1172/)?

comment:5 Changed 3 years ago by yawning

Keywords: TorBrowserTeam201606R added
Status: newneeds_review

https://git.schwanenlied.me/yawning/torbutton/src/feature19206

Implemented as 128 bits of entropy (as a hex string, per domain) | counter. The counter was retained for "being useful for debugging/troubleshooting".

Version 0, edited 3 years ago by yawning (next)

comment:6 Changed 3 years ago by yawning

Status: needs_reviewneeds_revision

It occurs to me that, we probably *should* clear out the domain isolator noncesForDomains cache on new identity so that it's possible to suppress the NEWNYM for the system tor use case. This appears trivial to add, so I'll do that on this branch as well, and re-submit it for review.

comment:7 Changed 3 years ago by yawning

Status: needs_revisionneeds_review

And pushed. Someone that cares more than I do can add a pref for actually skipping the NEWNYM, since it should no longer be necessary as long as our isolation code is actually working as intended.

My usecase never gives Tor Browser direct access to the control port anyway, so I'm just suppressing the signal there...

comment:8 Changed 3 years ago by arthuredelstein

Status: needs_reviewneeds_revision
function Nonce() {

My inclination for the sake of simplicity would be to drop the increment part and just make nonce() a simple function that returns a hex string.

  // Hexlify the tag.
  for (var i = 0; i < tag.length; i++)
    hexTag += tag[i].toString(16);

This code drops the leading zero for octets with value < 16. Doesn't affect entropy much, but would probably be nice to behave as expected. Also I generally prefer let to var.

// Per-domain noncces are stored in a map, so simply regenerate.

Typo.

Generally, we would precede the subject line for these two patches with "Bug 19206: ", because it makes sorting/searching easy.

Otherwise these patches look good to me.

comment:9 Changed 3 years ago by yawning

Status: needs_revisionneeds_review

Thanks for the review. I pushed a new branch that should address everything.

https://git.schwanenlied.me/yawning/torbutton/src/feature19206_take2

comment:10 Changed 3 years ago by arthuredelstein

Looks good to me.

comment:11 Changed 3 years ago by gk

Could you fix:

+  // unlikely it for there to be a collision.

and two typos in your 2278bb7aeab7e992f65ba90865e1fd62e33c767a commit message ("isolaton" and "annonymity")?

comment:13 Changed 3 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Thanks, yawning! This is commit 0fbd44f0305a01a14bc7439f5ad66b59d9278a2d and 36d849291ec0b20a58cccc2cd846fcd2540c9bbe (I fixed a small typo in the commit message of the former which I overlooked the last time).

comment:14 Changed 3 years ago by bugzilla

#13706 is a duplicate.

Note: See TracTickets for help on using tickets.