Opened 6 years ago

Closed 3 years ago

#3455 closed enhancement (fixed)

Tor Browser should set SOCKS username for a request based on first party domain

Reported by: mikeperry Owned by: arthuredelstein
Priority: High Milestone: TorBrowserBundle 2.3.x-stable
Component: Applications/Tor Browser Version:
Severity: Keywords: tbb-linkability, tbb-usability, TorBrowserTeam201410, tbb-firefox-patch, MikePerry201410R, tbb-4.5-alpha
Cc: gk, proper@…, lunar@…, isis@…, intrigeri, arthuredelstein@…, mcs Actual Points:
Parent ID: #5752 Points:
Reviewer: Sponsor:

Description (last modified by mikeperry)

Once Proposal 171 is implemented (#1865), Tor Browser should set the Proposal 171 SOCKS username to a function of the hostname in the referer header (possibly caching the first referer for subsequent link navigation).

If the referer is blank, we should use a function of the request URL hostname. This policy should effectively give us the same top-level origin isolation for circuit use that we want for other identifiers.

Lunar also points out that if this function introduces a hashed nonce that is changed on "New Identity" invocations, we can then do without the control port and control auth/password inside torbutton but still provide New Identity. This would simplify a lot of setups, and potentially allow us to remove more code from Torbutton.

Child Tickets

TicketTypeStatusOwnerSummary
#6733enhancementclosedbenPatch Firefox to allow addons to set SOCKS username+password per request

Change History (79)

comment:1 in reply to:  description Changed 6 years ago by rransom

Replying to mikeperry:

Once Proposal 171 is implemented (#1865), Tor Browser should set the Proposal 171 SOCKS username to a function of the hostname in the referer header (possibly caching the first referer for subsequent link navigation).

The SOCKS password would be a better field for intra-application identity compartments, so that when Tor decides to shove two compartments into the same circuit, it can merge two compartments run by the same application together before it merges different applications' compartments (which could have different SOCKS usernames) together.

comment:2 Changed 6 years ago by mikeperry

There may be a wrinkle here. Instead of actual referer, we may want to use the nsIContentPolicy's RequestOrigin parameter. The actual referer will be blank for cases like encrypted google search and other https->http transitions.

comment:3 Changed 6 years ago by mikeperry

Milestone: TorBrowserBundle 2.3.x-stable

comment:4 Changed 6 years ago by gk

Cc: g.koppen@… added

comment:5 Changed 6 years ago by proper

Cc: proper@… added

comment:6 Changed 6 years ago by arma

Parent ID: #5752

comment:7 Changed 6 years ago by mikeperry

Keywords: tbb-linkability added

comment:8 Changed 6 years ago by nickm

Can we write this up a little more verbosely, and explain what isolation policy it's meant to provide?

If I understand you right, it would mean for every page I visit, everything embedded in that page, and all pages on the same site, and everything embedded in _them_, would be linkable, but if I visit a page on a different site, it would get a different circuit?

comment:9 in reply to:  8 Changed 6 years ago by arma

Replying to nickm:

Can we write this up a little more verbosely, and explain what isolation policy it's meant to provide?

You may like https://www.torproject.org/projects/torbrowser/design/#identifier-linkability

If I understand you right, it would mean for every page I visit, everything embedded in that page, and all pages on the same site, and everything embedded in _them_, would be linkable, but if I visit a page on a different site, it would get a different circuit?

I believe that's right. It's quite a jump from the old "isolate by time interval" design. Clearly an improvement -- *if* we can afford to do it.

comment:11 Changed 6 years ago by mikeperry

Note that we'll also need to find some way to extract the referer even for users who disable referers/have arbitrary 3rd party addons that spoof them, especially if we want to stop those users from creating tons of circuits (#5708).

comment:12 Changed 6 years ago by arma

Is "referer" really the best stand-in we have for "was launched from that tab"?

comment:13 in reply to:  12 ; Changed 6 years ago by mikeperry

Replying to arma:

Is "referer" really the best stand-in we have for "was launched from that tab"?

"Launched from that tab" is not the model we're going for. It's the model used by JonDos, but what we want is "Launched from the same url bar origin/navigation session" (aka privacy requirement 1 in the design doc). Referer actually does map to what we want for that.

Now, this model does get a little sticky with redirects. See https://trac.torproject.org/projects/tor/ticket/3600#comment:16 for my thoughts on that case.

comment:14 in reply to:  13 ; Changed 6 years ago by rransom

Replying to mikeperry:

Replying to arma:

Is "referer" really the best stand-in we have for "was launched from that tab"?

"Launched from that tab" is not the model we're going for. It's the model used by JonDos, but what we want is "Launched from the same url bar origin/navigation session" (aka privacy requirement 1 in the design doc). Referer actually does map to what we want for that.

I think “launched from that tab/window” is a better model. It's easier to control, it doesn't require me to launch a second TBB if I want two separate sessions with the same website, ...

Now, this model does get a little sticky with redirects. See https://trac.torproject.org/projects/tor/ticket/3600#comment:16 for my thoughts on that case.

and it doesn't require me to know where those shortened links will take me, or remember which sites I have ‘visited’ or been redirected through since clicking ‘New Identity’ and flushing all of my currently-open tabs.

(Also, note that 3600#comment:16 is a more concise way to link to a comment on another ticket.

comment:15 in reply to:  14 Changed 6 years ago by mikeperry

Replying to rransom:

Replying to mikeperry:

Replying to arma:

Is "referer" really the best stand-in we have for "was launched from that tab"?

"Launched from that tab" is not the model we're going for. It's the model used by JonDos, but what we want is "Launched from the same url bar origin/navigation session" (aka privacy requirement 1 in the design doc). Referer actually does map to what we want for that.

I think “launched from that tab/window” is a better model. It's easier to control, it doesn't require me to launch a second TBB if I want two separate sessions with the same website, ...

Try to virtualize the rest of humanity (or just your densest cousin) using a per-tab/window model after having been trained to use their browsers as single identity bags. Here, let me help you understand what it would be like:

"What do you mean, log in to google again? I just did that in this other tab."
"OOps, I thought this tab was the OTHER gmail account..."
"Oh, I though the fourth tab was twitter, not gmail. I guess I reordered them."

Express route to Fail City, imo. See also point 1 in https://www.torproject.org/projects/torbrowser/design/#philosophy

Now, this model does get a little sticky with redirects. See https://trac.torproject.org/projects/tor/ticket/3600#comment:16 for my thoughts on that case.

and it doesn't require me to know where those shortened links will take me, or remember which sites I have ‘visited’ or been redirected through since clicking ‘New Identity’ and flushing all of my currently-open tabs.

Except for other activity on that tab... There's no way humans can remember what each tab/window was used for what and be expected not to mess up, forget, and link themselves.

We need to design this stuff for normal humans, not mentats.

comment:16 Changed 6 years ago by rransom

Normal humans can't even find their own files on a filesystem. And you think they would be better able to understand a barrage of ‘click OK to link your identity on one website to your identity on another website’ popups?

comment:17 in reply to:  16 Changed 6 years ago by mikeperry

Replying to rransom:

And you think they would be better able to understand a barrage of ‘click OK to link your identity on one website to your identity on another website’ popups?

No. As is quite clear in 3600#comment:16, I am still conflicted on this being a good idea. But you're commenting in the wrong bug now, because redirects are an orthogonal problem to choosing tab isolation vs url origin isolation. They happen in both. If you have an opinion/better idea for handling redirects, please comment in #3600, not here.

comment:18 Changed 5 years ago by lunar

Cc: lunar@… added

Instead of going to the ControlPort to send a NEWNYM command, Torbutton could simply create a new SOCKS username each time the 'new identity button is pressed'. Stream isolation will make both have the same outcome.

This would also reduce some complexity in the TBB launching script, because they would be no need to pass environment variable around to get access to the ControlPort.

comment:19 Changed 5 years ago by mikeperry

Description: modified (diff)

comment:20 in reply to:  18 Changed 5 years ago by proper

Replying to lunar:

Instead of going to the ControlPort to send a NEWNYM command, Torbutton could simply create a new SOCKS username each time the 'new identity button is pressed'. Stream isolation will make both have the same outcome.

Great suggestion!

This would also reduce some complexity in the TBB launching script, because they would be no need to pass environment variable around to get access to the ControlPort.

Bonus point: TBB gets (less) dependent on ControlPort. That helps when Tor Browser can not access ControlPort on purpose, i.e. when Tor Browser is being used behind a transparent or lan (Tor) proxy.

comment:21 Changed 5 years ago by cypherpunks

While this feature sounds great I have some concerns.

From Practical Vulnerabilities of the Tor Anonymity Network...

Still if an adversary is aware of one or more websites of interest that are
relatively unique, he can use website fingerprinting as an indicator of those
users that he might want to scrutinize further. And, if he is sophisticated, he
can also use machine learning techniques to improve his accuracy. Fortunately
for the aware user, it is easy to defeat website fingerprinting. By simultaneously
visiting multiple sites over the same Tor circuit (which Tor allows automatically)
any individual fingerprint becomes hard to recognize [19].

If we have a separate circuit for every website doesn't this make website fingerprinting even easier since we can no longer visit multiple sites through the same circuit?

comment:22 Changed 5 years ago by mikeperry

Website fingerprinting at guard nodes is an orthogonal issue to linkability at exit nodes. If you let the solution to one block the other, you end up with deadlock and nothing ever gets done.

comment:23 Changed 5 years ago by isis

Cc: isis@… added

comment:24 Changed 5 years ago by T(A)ILS developers

Cc: tails@… added

comment:25 Changed 5 years ago by mikeperry

Keywords: tbb-usability added
Summary: Tor Browser should set SOCKS username for a request based on refererTor Browser should set SOCKS username for a request based on first party domain

Calling this usability because it would also fix a common issue where Facebook and other webapps log you out if your IP changes.

Also I think we should probably just start with first party domain, to keep things simpler and perhaps more user intuitive. I think we should only bother with using referer if the network explodes under the onionskin load (which thanks to ntor, it shouldn't).

comment:26 Changed 4 years ago by arthuredelstein

Cc: arthuredelstein@… added

comment:27 Changed 4 years ago by gk

Cc: gk added; g.koppen@… removed

comment:28 Changed 3 years ago by arthuredelstein

Status: newneeds_information

Working on #3455. I have written a proxy filter patch like the one called for in https://bugzilla.mozilla.org/show_bug.cgi?id=436344. The patch will allow us to set SOCKS proxy settings (either the port number or the username/password) by first party domain. Two options I can see are (1) writing username/password support for Mozilla's SOCKS code (as in #6733), or (2) dynamically creating additional SOCKSPorts through tor's ControlPort whenever a new first party domain is encountered. I'm leaning toward (2) because (1) requires another significant patch to Firefox that may or may not be accepted upstream, while (2) requires only JavaScript code in torbutton to set new SOCKSPorts. Is there a reason I should do (1) instead of (2)?

comment:29 in reply to:  28 ; Changed 3 years ago by proper

Replying to arthuredelstein:

(2) dynamically creating additional SOCKSPorts through tor's ControlPort whenever a new first party domain is encountered. [...]
(2) requires only JavaScript code in torbutton to set new SOCKSPorts.

Will this work with multiple browser tabs as well? What is when traffic is created in two different tabs with different first party domains at the same time? My concern is, that these circuits would get mixed. But perhaps it's unjustified.

comment:30 in reply to:  29 Changed 3 years ago by arthuredelstein

Replying to proper:

Replying to arthuredelstein:

(2) dynamically creating additional SOCKSPorts through tor's ControlPort whenever a new first party domain is encountered. [...]
(2) requires only JavaScript code in torbutton to set new SOCKSPorts.

Will this work with multiple browser tabs as well? What is when traffic is created in two different tabs with different first party domains at the same time? My concern is, that these circuits would get mixed. But perhaps it's unjustified.

My understanding is that circuits for two tabs would not get mixed even for simultaneous loading. According to the tor manual: "By default, streams received on different SOCKSPorts, TransPorts, etc are always isolated from one another." In option (2), each of two tabs would have a different first party domain, so all requests from one tab would be sent over one SOCKSPort, and all requests from the other tab would be sent over a second SOCKSPort. Option (1) uses a very similar mechanism, in that we would rely on username/password instead of port number to separate the circuits for the two tabs.

comment:31 Changed 3 years ago by arthuredelstein

Mike Perry pointed out that users with local/app firewalls may have difficulty with multiple ports. Of course, even with option (1), it's necessary for any local firewall to be configured such that the default SOCKSPort and ControlPorts are unblocked to TorBrowser. But requiring users to have a large number of ports available locally is indeed more complicated. I guess this disadvantage needs to be weighed against the danger that Mozilla won't accept a patch for SOCKS username/password. Does anyone have a view on this?

comment:32 Changed 3 years ago by anon

As mentioned users with a local firewall have to punch holes for specific reasons already. Rather than assuming that a handful is acceptable but a few more is not, consider a auto port range setting.

Thus users would specify a local ephemeral or dynamic range of ports for Tor (ex. 9000 to 9999) or by process (Tor.exe) to whitelist in their local network policy authority.

Where this becomes a problem is with Unix domain sockets like SocksSocket; creating these dynamically may need a parent directory or other tmpfile like collision resistant method to support multiple unix domain socket paths. See https://trac.torproject.org/projects/tor/ticket/12585

comment:33 in reply to:  28 Changed 3 years ago by proper

Replying to arthuredelstein:

Is there a reason I should do (1) instead of (2)?

Would (2) work on systems, where Tor is listening on a different computer than Tor Browser (Isolating Proxy)?

comment:34 Changed 3 years ago by anon

shit coderman says:
"i don't like username+password for usability/interop reasons, and also the firefox upstream reason, and also the self evident auditability reason (i can easily see from lsof / netstat what socks ports in use, but i can't tell from established socks what username/auth was over it)"
"if it is auto, you can skip the occupied attempts. control port user knows where it opened"
" as for usability/interop, there are command line and basic socket tools which can do basic SOCKS5 or 4a but not authentication to SOCKS svr"
[QUESTION] 'suppose you open a port, and now another app needs that port?'
"the other app either fails gracefully like above (picking another avail port) or it errors out and you change your range to not collide. ports, the new IRQ numbers... ;)"

possible solution:
"if you wanted to be friendly, you would use named pipes for those who didn't want to use socks ports on auto. this would avoid any problems with port conflicts, and it would also provide (through sys internals) the same kind of visibility into who the sockspipe consumer was. so perhaps if socks ports are to be used, go the username/pass route. and if isolation via pipes/unixdomainsockets is desired, you can do that without username hack"

Last edited 3 years ago by anon (previous) (diff)

comment:35 Changed 3 years ago by arthuredelstein

As I said on IRC: I think I'm going to go for Option 1 (u+p) now, because of the complexity of blocked or conflicted ports and the current lack of support for auto port settings, unix domain sockets and Windows pipes. But if we run into big problems patching to upstream Firefox then we can revisit the multiple ports/sockets/pipes option. Thanks to everyone for the helpful comments!

comment:36 Changed 3 years ago by arthuredelstein

I'm posting three patch files for review. The first two are for tor-browser.git, and the last is for torbutton.git.

For now, the SOCKS username is set to the URL bar domain, and the password is set to '0'. If we decide to create a "New Identity" button for the URL bar, then that button can increment the password (i.e.: '1','2','3'...) for each identity change.

In TorBrowser, to see the SOCKS username/password assigned to each request, set extensions.torbutton.loglevel to 3 and open the Browser Console.

To confirm that tor is isolating circuits by URL bar domain, you can include the following settings in torrc:

AvoidDiskWrites 0
Log info file /path/to/your/logfile.txt
SafeLogging 0

Then call tail -f /path/to/your/logfile.txt

comment:37 Changed 3 years ago by arthuredelstein

Status: needs_informationneeds_review

comment:38 Changed 3 years ago by gk

Keywords: MikePerry201407R added

Re part1/3: What is the reasoning behind renaming ApplyFilter()? Can't we just leave it? That would break less things e.g. in the accompanying unittest: https://mxr.mozilla.org/mozilla-esr24/source/netwerk/test/unit/test_protocolproxyservice.js

comment:39 in reply to:  38 ; Changed 3 years ago by arthuredelstein

Replying to gk:

Re part1/3: What is the reasoning behind renaming ApplyFilter()? Can't we just leave it? That would break less things e.g. in the accompanying unittest: https://mxr.mozilla.org/mozilla-esr24/source/netwerk/test/unit/test_protocolproxyservice.js

It's true that the method

  applyFilter: function(pps, uri, pi) {
     return pps.newProxyInfo("http", "localhost", 8080, 0, 10,
                             pps.newProxyInfo("direct", "", -1, 0, 0, null));
  }

doesn't break if we keep the name ApplyFilter, because the second argument is ignored. But still, pedantically speaking, the second argument's name should be changed to channel. So I thought that by changing the method name we could ensure that anyone implementing nsIProtocolProxy is forced to re-examine their code. But maybe this is too paternalistic. ;)

comment:40 in reply to:  39 ; Changed 3 years ago by gk

Replying to arthuredelstein:

Replying to gk:

Re part1/3: What is the reasoning behind renaming ApplyFilter()? Can't we just leave it? That would break less things e.g. in the accompanying unittest: https://mxr.mozilla.org/mozilla-esr24/source/netwerk/test/unit/test_protocolproxyservice.js

It's true that the method

  applyFilter: function(pps, uri, pi) {
     return pps.newProxyInfo("http", "localhost", 8080, 0, 10,
                             pps.newProxyInfo("direct", "", -1, 0, 0, null));
  }

doesn't break if we keep the name ApplyFilter, because the second argument is ignored. But still, pedantically speaking, the second argument's name should be changed to channel.

Yeah, I definitely think this unittest needs to get fixed as well (I am okay doing it not in this bug but when we prepare things for Mozilla). Another reason for keeping the original method's name is that it is more specific than just "apply" giving a better context on what is going on...

One thing I forgot: we probably need new IIDs as we are changing interfaces.

comment:41 in reply to:  40 ; Changed 3 years ago by arthuredelstein

Replying to gk:

Yeah, I definitely think this unittest needs to get fixed as well (I am okay doing it not in this bug but when we prepare things for Mozilla). Another reason for keeping the original method's name is that it is more specific than just "apply" giving a better context on what is going on...

OK, I've changed the method's name back to ApplyFilter. And I updated the unit tests you found, since we might forget otherwise...

One thing I forgot: we probably need new IIDs as we are changing interfaces.

Good point -- I've updated them. New versions of all three patches have been uploaded.

comment:42 in reply to:  41 ; Changed 3 years ago by gk

Replying to arthuredelstein:

Replying to gk:

Yeah, I definitely think this unittest needs to get fixed as well (I am okay doing it not in this bug but when we prepare things for Mozilla). Another reason for keeping the original method's name is that it is more specific than just "apply" giving a better context on what is going on...

OK, I've changed the method's name back to ApplyFilter. And I updated the unit tests you found, since we might forget otherwise...

Nice. But there remains quite some stuff to be done here (you know, just that we don't forget it :) )
https://mxr.mozilla.org/mozilla-esr24/source/netwerk/test/unit/test_protocolproxyservice.js#151
https://mxr.mozilla.org/mozilla-esr24/source/netwerk/test/unit/test_protocolproxyservice.js#156
...

One thing I forgot: we probably need new IIDs as we are changing interfaces.

Good point -- I've updated them. New versions of all three patches have been uploaded.

Thanks! One more question: You did not update https://mxr.mozilla.org/mozilla-esr24/source/dom/mobilemessage/src/ril/MmsService.js because Tor Browser is not using that one (yet), right?

comment:43 in reply to:  42 Changed 3 years ago by arthuredelstein

Replying to gk:

Nice. But there remains quite some stuff to be done here (you know, just that we don't forget it :) )
https://mxr.mozilla.org/mozilla-esr24/source/netwerk/test/unit/test_protocolproxyservice.js#151
https://mxr.mozilla.org/mozilla-esr24/source/netwerk/test/unit/test_protocolproxyservice.js#156

You're absolutely right. I believe I've fixed these.

Thanks! One more question: You did not update https://mxr.mozilla.org/mozilla-esr24/source/dom/mobilemessage/src/ril/MmsService.js because Tor Browser is not using that one (yet), right?

Oops -- I've now fixed that. Patches 1 and 2 have been updated again.

comment:44 Changed 3 years ago by erinn

Keywords: tbb-firefox-patch added

comment:45 Changed 3 years ago by erinn

Component: Firefox Patch IssuesTor Browser

comment:46 Changed 3 years ago by gk

Some comments on Part 3 (which looks good to me):

1) "The filterFunction should expect two arguments: filterFunction(aChannel, aProxy)": you probably just meant aChannel and aProxy being those arguments.
2) "Returns a zero-argument function that will unregister the filter.": I wonder where this is going to happen? I think you can omit that as according to MDN:

All filters will be automatically unregistered at XPCOM shutdown.

That said, what I like to see getting unregistered/removed on shutdown are observers although I realize that the common style in Torbutton seems to be ignoring this thing... :) So, feel free to keep the observer for profile-after-change as-is.

comment:47 in reply to:  46 Changed 3 years ago by arthuredelstein

Replying to gk:

Thanks for looking it over!

1) "The filterFunction should expect two arguments: filterFunction(aChannel, aProxy)": you probably just meant aChannel and aProxy being those arguments.

Fixed that.

2) "Returns a zero-argument function that will unregister the filter.": I wonder where this is going to happen? I think you can omit that as according to MDN:

All filters will be automatically unregistered at XPCOM shutdown.

Good point. I've removed that code.

That said, what I like to see getting unregistered/removed on shutdown are observers although I realize that the common style in Torbutton seems to be ignoring this thing... :) So, feel free to keep the observer for profile-after-change as-is.

OK, I'll leave it for now. Maybe at some point we can revisit shutdown handling for all observers.

I've made a couple of additional minor changes to comments, and changed some vars to lets.

comment:48 Changed 3 years ago by intrigeri

Cc: intrigeri added; tails@… removed

comment:49 Changed 3 years ago by gk

comment:50 in reply to:  49 Changed 3 years ago by arthuredelstein

Replying to gk:

Seems we need something for HTTPS-E as well https://gitweb.torproject.org/https-everywhere.git/blob/HEAD:/src/components/ssl-observatory.js#l1042... Whatever it is.

SSL-Observatory is a plugin that tracks what sites you visit and sends that data to EFF! Maybe we should not be including this part of HTTPS-E in Tor Browser at all.

comment:51 Changed 3 years ago by mcs

Cc: mcs added

comment:52 Changed 3 years ago by mikeperry

Keywords: MikePerry201408R TorBrowserTeam201408 added; MikePerry201407R removed

comment:53 Changed 3 years ago by mikeperry

Owner: changed from mikeperry to arthuredelstein
Status: needs_reviewassigned

comment:54 Changed 3 years ago by mikeperry

Status: assignedneeds_review

Trac cannot change assignment without resetting ticket state...

comment:55 Changed 3 years ago by arthuredelstein

I forgot to explain my last updates to Patches 1/3 and 3/3 on 2014-08-07.

The commit message for Part 1/3 is:

This patch adds an alternative to the nsIProtocolProxyFilter
mechanism, called nsIProtocolProxyChannelFilter, in which
a protocol proxy filter receives a channel object rather than
a URI. Access to the channel object allows the proxy filter to
provide proxy settings according to the URL bar domain.

In this version, I'm retaining the original nsIProtocolProxyFilter interface, to avoid breaking SSL Observatory and any other existing extensions. I hope this means it will be easier to get Mozilla to accept this patch.

The new version of Patch 3/3 (DomainIsolator) implements the new nsIProtocolProxyChannelFilter to get a reference to the channel and thereby obtain the URL bar domain for tor circuit isolation.

comment:56 Changed 3 years ago by mikeperry

Keywords: TorBrowserTeam201409 added; MikePerry201408R TorBrowserTeam201408 removed

I think this new proxy filter API is a decent approach, and would be useful to many other projects. We should rebase it to Firefox31 and mozilla-central and try to get Mozilla to review it, especially for e10s support/performance.

Note: I have not given it a thorough review for correctness/safety yet. I figure we should see what Mozilla thinks about the approach first.

comment:57 in reply to:  56 Changed 3 years ago by arthuredelstein

Replying to mikeperry:

I think this new proxy filter API is a decent approach, and would be useful to many other projects. We should rebase it to Firefox31 and mozilla-central and try to get Mozilla to review it, especially for e10s support/performance.

Note: I have not given it a thorough review for correctness/safety yet. I figure we should see what Mozilla thinks about the approach first.

I've submitted the two C++ patches to Mozilla and requested reviews:

comment:58 Changed 3 years ago by mikeperry

Keywords: TorBrowserTeam201410 MikePerry201410R added; TorBrowserTeam201409 removed

comment:59 Changed 3 years ago by mikeperry

Keywords: tbb-4.5-alpha added

Changed 3 years ago by arthuredelstein

ESR31.1.1-based 3455.2

comment:60 Changed 3 years ago by arthuredelstein

I've brought the patches up to date. See the most recent 3 patches named 000*-Bug-3455...

comment:61 Changed 3 years ago by arthuredelstein

For the domain isolator component (3455.3) to work with tbb-4.5-alpha only, we'll need a tbb-4.5-alpha branch for torbutton.

Changed 3 years ago by arthuredelstein

ESR31.1.1-based 3455.1

comment:62 Changed 3 years ago by arthuredelstein

(Applied minor fix to 3455.1)

comment:63 Changed 3 years ago by mikeperry

After a quick look at the Torbutton side of the code -- I am wondering what happens with favicons. Historically, Mozilla's filters often miss those. I am also wondering how this interacts with requests from the addons pane. Neither of these are blockers, but we should be aware of these issues if they exist.

I am also a little concerned with your initialization. DomainIsolator.observe() is actually going to be called for several different observer topics. It may be causing multiple proxy filters to get registered. You should probably be checking for topic == "profile-after-change".

I still need to review the C++ side.

comment:64 in reply to:  63 Changed 3 years ago by arthuredelstein

Replying to mikeperry:

After a quick look at the Torbutton side of the code -- I am wondering what happens with favicons. Historically, Mozilla's filters often miss those.

I made a small test at https://arthuredelstein.github.io/tbtests/faviconDemo.html, which consists of a tiny html page with a link to a favicon. Annoyingly, the Firefox network tab does not show the favicon.ico request. So I examined the tor-SOCKS conncetion with tcpdump. After clearing the browser cache and browsing to the test, I could see two HTTP GET request/response pairs through the same socket connection, one for faviconDemo.html and one for favicon.ico. So favicon.ico is passing through tor. The first request was preceded by a SOCKS username:password request being set to arthuredelstein.github.io:0. There wasn't a second username:password request.

But at the same time, watching the tor Control Port, I could see only one STREAM event:

650 STREAM 265 NEW 0 arthuredelstein.github.io:80 SOURCE_ADDR=127.0.0.1:57287 PURPOSE=USER
650 STREAM 265 SENTCONNECT 63 arthuredelstein.github.io:80
650 STREAM 265 REMAP 63 185.31.17.133:80 SOURCE=EXIT
650 STREAM 265 SUCCEEDED 63 185.31.17.133:80
650 STREAM 265 CLOSED 63 185.31.17.133:80 REASON=DONE

where getinfo circuit-status shows the following info for circuit 63:

63 BUILT $9CD77810A49B52A333666689B334447DCAD40591~Unnamed,$43021D98AC4B2EC960233E5B4D7032BE450E6241~thebigpenistorrelay,$AF657ED27ADF08EDB9E4C7EE90DDA5D2C1D46DFC~torpoint BUILD_FLAGS=NEED_CAPACITY PURPOSE=GENERAL TIME_CREATED=2014-10-29T18:38:13.556569 SOCKS_USERNAME="arthuredelstein.github.io" SOCKS_PASSWORD="0"

So my interpretation is that Firefox is pipelining the favicon.ico request after the initial page request. I tried to turn off pipelining by various browser prefs, but without success.

Looking at the Firefox source code, however, I get the impression that if pipelining weren't happening, the favicon would not be sent over the same username+password. So it would still be better to try to fix this.

I am also wondering how this interacts with requests from the addons pane. Neither of these are blockers, but we should be aware of these issues if they exist.

At the moment, requests from the addons pane appear to be sent through the "default" tor circuit -- i.e, one with no SOCKS u+p. I figure this is not so serious, but it would be nice to fix in a future iteration.

One more serious issue that I just discovered, is that OCSP requests are being sent on the default circuit. Obviously this needs to be fixed. Somehow we need mozilla.thirdPartyUtil.getFirstPartyURIFromChannel to return the URL bar domain for the requesting page.

(This OCSP issue seems possibly tangentially related to #3666, but I couldn't find the relevant patch in tor-browser.git. Has the patch been upstreamed to Mozilla already?)

I am also a little concerned with your initialization. DomainIsolator.observe() is actually going to be called for several different observer topics. It may be causing multiple proxy filters to get registered. You should probably be checking for topic == "profile-after-change".

Good point. I've fixed that in the latest version of the patch.

Last edited 3 years ago by arthuredelstein (previous) (diff)

comment:65 Changed 3 years ago by arthuredelstein

I've posted a new version of 3455.2, that should have better protection against buffer overflows. I accidentally forgot to mark the previous version as obsolete, so please use the most recent version only.

comment:66 Changed 3 years ago by arthuredelstein

Last edited 3 years ago by arthuredelstein (previous) (diff)

comment:67 Changed 3 years ago by gk

3455.1 needs revision. The mingw-w64 compiler at least is not amused:

/home/ubuntu/build/tor-browser/netwerk/base/src/nsProtocolProxyService.cpp:1026:72: error: ambiguating new declaration of 'nsresult nsProtocolProxyService::DeprecatedBlockingResolve(nsIChannel*, uint32_t, nsIProxyInfo**)'
                                                   nsIProxyInfo **retval)
                                                                        ^

comment:68 Changed 3 years ago by gk

Status: needs_reviewneeds_revision

comment:69 Changed 3 years ago by arthuredelstein

I've added a fixup patch that will hopefully address the "ambiguating new declaration" compile error seen in mingw.

comment:70 in reply to:  67 Changed 3 years ago by gk

Replying to gk:

3455.1 needs revision. The mingw-w64 compiler at least is not amused:

/home/ubuntu/build/tor-browser/netwerk/base/src/nsProtocolProxyService.cpp:1026:72: error: ambiguating new declaration of 'nsresult nsProtocolProxyService::DeprecatedBlockingResolve(nsIChannel*, uint32_t, nsIProxyInfo**)'
                                                   nsIProxyInfo **retval)
                                                                        ^

An here is the missing part:

In file included from /home/ubuntu/build/tor-browser/netwerk/base/src/nsProtocolProxyService.cpp:10:0:
/home/ubuntu/build/tor-browser/netwerk/base/src/nsProtocolProxyService.h:50:14: note: old declaration 'nsresult nsProtocolProxyService::DeprecatedBlockingResolve(nsIChannel*, uint32_t, nsIProxyInfo**)'
     nsresult DeprecatedBlockingResolve(nsIChannel *aChannel,
              ^

comment:71 Changed 3 years ago by mikeperry

Resolution: fixed
Status: needs_revisionclosed

This is all merged for 4.5-alpha-1, including the #8405 patch.

Note: See TracTickets for help on using tickets.