Tor Browser 6.5a4 is ignoring additional SocksPorts

Let's assume you have something like that in your torrc-defaults file in Tor Browser:

SocksPort 9150 IPv6Traffic PreferIPv6 KeepAliveIsolateSOCKSAuth
SocksPort 9160 IPv6Traffic PreferIPv6 KeepAliveIsolateSOCKSAuth
ControlPort 9151
CookieAuthentication 1

And let's say you point your Torbirdy to 9160. That setup works perfectly fine with Tor Browser 6.5a3 and breaks with 6.5a4. Setting extensions.torlauncher.socks_port_use_ipc to false (trying to simulate the old behavior) does not fix it. Nor does setting TOR_SOCKS_PORT=9150 help. In fact, looking at the code it seems this is not supported anymore which we should fix.

added TorBrowserTeam201705R component::applications/tor launcher owner::mcs priority::medium resolution::fixed severity::normal sponsor::4 status::closed type::defect labels

Trac:
Keywords: TorBrowserTeam2011 deleted, TorBrowserTeam201611 added

Yes, breaking this was an oversight on our part (mcs + brade) when we did the work for #14272 (moved) and #20111 (moved).

If I remember correctly, Tor Launcher now always passes SocksPort and ControlPort as arguments when starting tor. This works well if no one has edited torrc outside of Tor Launcher, and we wanted to ensure that toggling the use_ipc pref(s) would cause the right thing to happen. But we will have to think about how to support more scenarios.

Kathy and I are not sure how best to fix this bug. We cannot insist that all ports be configured via torrc-defaults because the path for the Unix domain sockets is not known until runtime (users can install their browser anywhere).

On the other hand, if we change Tor Launcher to always add a SocksPort (by changing Tor Launcher to pass +SocksPort ... on the tor command line) then we will run into two problems: (1) another SocksPort will be added each time the browser is started and (2) when the user toggles extensions.torlauncher.socks_port_use_ipc the previous SocksPort set by Tor Launcher will not be removed.

One solution would be to add a Boolean preference that causes all tor port configuration to be taken from torrc/torrc-defaults (thus restoring the Tor Browser 6.0.x behavior). Tor Launcher and Torbutton would still need to know how to connect to the ports, so it would be up to the user to ensure that the Tor Launcher prefs such as extensions.torlauncher.control_ipc_path match what they have in their torrc. We would have to come up with a good name for the new pref, e.g., extensions.torlauncher.tor_use_torrc_ports (suggestions for a less confusing name are welcome).

Another approach would be to add support for some new preferences that could be set by users who need additional SOCKS listeners, e.g., extensions.torlauncher.socks_port_extra.1, extensions.torlauncher.socks_port_extra.2, etc.

Georg and Arthur, which approach do you like best? Or maybe you have a better idea?

Also, do we need to allow users to have more than one ControlPort too? My guess is that we do.

Trac:
Status: new to needs_information
Keywords: TorBrowserTeam201611 deleted, TorBrowserTeam201612 added
Cc: mcs to mcs, arthuredelstein

Replying to mcs:

On the other hand, if we change Tor Launcher to always add a SocksPort (by changing Tor Launcher to pass +SocksPort ... on the tor command line) then we will run into two problems: (1) another SocksPort will be added each time the browser is started and (2) when the user toggles extensions.torlauncher.socks_port_use_ipc the previous SocksPort set by Tor Launcher will not be removed.

As far as I know, the +SocksPort ... command-line argument doesn't write anything permanent to the torrc file, but just temporarily adds a SocksPort for the lifetime of the tor process being launched. So I don't think you will be accumulating more than two SocksPorts at a time.

I wonder if you like the following approach:

• When socks_port_use_ipc = false, omit passing a SocksPort argument altogether and just use the 9150 port created by the defaults_torrc file. Both torbirdy and Tor Browser will use the same socket at port 9150.
• When socks_port_use_ipc = true, pass +SocksPort /path/to/domain/socket. In this case, you have two SocksPorts open (one domain socket for Tor Browser and one socket at port 9150 for torbirdy or other apps).

Hopefully then we don't have to add any additional prefs.

Also, do we need to allow users to have more than one ControlPort too? My guess is that we do.

If there are third-party apps that use the ControlPort, then it seems likely.

Replying to arthuredelstein:

Replying to mcs:

On the other hand, if we change Tor Launcher to always add a SocksPort (by changing Tor Launcher to pass +SocksPort ... on the tor command line) then we will run into two problems: (1) another SocksPort will be added each time the browser is started and (2) when the user toggles extensions.torlauncher.socks_port_use_ipc the previous SocksPort set by Tor Launcher will not be removed.

As far as I know, the +SocksPort ... command-line argument doesn't write anything permanent to the torrc file, but just temporarily adds a SocksPort for the lifetime of the tor process being launched. So I don't think you will be accumulating more than two SocksPorts at a time.

I wonder if you like the following approach:

• When socks_port_use_ipc = false, omit passing a SocksPort argument altogether and just use the 9150 port created by the defaults_torrc file. Both torbirdy and Tor Browser will use the same socket at port 9150.

Or whatever got specified in the defaults-torrc file (see ticket description)

• When socks_port_use_ipc = true, pass +SocksPort /path/to/domain/socket. In this case, you have two SocksPorts open (one domain socket for Tor Browser and one socket at port 9150 for torbirdy or other apps).

Hopefully then we don't have to add any additional prefs.

Sounds good to me.

Also, do we need to allow users to have more than one ControlPort too? My guess is that we do.

If there are third-party apps that use the ControlPort, then it seems likely.

Yes.

Replying to arthuredelstein:

As far as I know, the +SocksPort ... command-line argument doesn't write anything permanent to the torrc file, but just temporarily adds a SocksPort for the lifetime of the tor process being launched. So I don't think you will be accumulating more than two SocksPorts at a time.

Kathy and I saw that happen yesterday when we experimented with +SocksPort. It happens without the + as well, at least with Tor 0.2.9.5-alpha. If you start Tor Browser and then make a change via Tor Launcher's Network Settings window so that torrc gets written to disk (e.g., enable allowed ports), you will see that ControlPort and SocksPort lines are added. We only tested on OSX so far. I wonder what the intended behavior is?

Replying to mcs:

Replying to arthuredelstein:

As far as I know, the +SocksPort ... command-line argument doesn't write anything permanent to the torrc file, but just temporarily adds a SocksPort for the lifetime of the tor process being launched. So I don't think you will be accumulating more than two SocksPorts at a time.

Kathy and I saw that happen yesterday when we experimented with +SocksPort. It happens without the + as well, at least with Tor 0.2.9.5-alpha. If you start Tor Browser and then make a change via Tor Launcher's Network Settings window so that torrc gets written to disk (e.g., enable allowed ports), you will see that ControlPort and SocksPort lines are added. We only tested on OSX so far. I wonder what the intended behavior is?

Oh, that's interesting. I guess that behavior is caused by the SAVECONF command?

It seems clear we don't want to save an ephemeral SocksPort /one/time/path line to the torrc file. So I suppose one (somewhat awkward) option, instead of using SAVECONF, would be to load the settings into a string using GETINFO CONFIG-TEXT, remove the line with the unix-domain SocksPort (that was created for this session only), and then write the torrc file directly from JavaScript.

Replying to arthuredelstein:

Oh, that's interesting. I guess that behavior is caused by the SAVECONF command?

I assume so, yes.

It seems clear we don't want to save an ephemeral SocksPort /one/time/path line to the torrc file. So I suppose one (somewhat awkward) option, instead of using SAVECONF, would be to load the settings into a string using GETINFO CONFIG-TEXT, remove the line with the unix-domain SocksPort (that was created for this session only), and then write the torrc file directly from JavaScript.

That is an interesting idea but possibly fragile, since it might be tricky to figure out for sure which lines to filter out before rewriting torrc. Kathy and I will think about this a little more.

Replying to arthuredelstein:

Replying to mcs:

Replying to arthuredelstein:

As far as I know, the +SocksPort ... command-line argument doesn't write anything permanent to the torrc file, but just temporarily adds a SocksPort for the lifetime of the tor process being launched. So I don't think you will be accumulating more than two SocksPorts at a time.

Kathy and I saw that happen yesterday when we experimented with +SocksPort. It happens without the + as well, at least with Tor 0.2.9.5-alpha. If you start Tor Browser and then make a change via Tor Launcher's Network Settings window so that torrc gets written to disk (e.g., enable allowed ports), you will see that ControlPort and SocksPort lines are added. We only tested on OSX so far. I wonder what the intended behavior is?

Oh, that's interesting. I guess that behavior is caused by the SAVECONF command?

It seems clear we don't want to save an ephemeral SocksPort /one/time/path line to the torrc file. So I suppose one (somewhat awkward) option, instead of using SAVECONF, would be to load the settings into a string using GETINFO CONFIG-TEXT, remove the line with the unix-domain SocksPort (that was created for this session only), and then write the torrc file directly from JavaScript.

Hrm. This seems scary. I need to think a bit more about it. mcs/brade: What about the other part of the problem, that even disabling unix domain usage does not restore the old behavior? Is that a problem we could solve separately and which when thinking about might help to shed some light on the solution for the other one (which is while using domain sockets the other SocksPort etc. options in the torrc files should not be ignored)?

Replying to mcs:

It seems clear we don't want to save an ephemeral SocksPort /one/time/path line to the torrc file. So I suppose one (somewhat awkward) option, instead of using SAVECONF, would be to load the settings into a string using GETINFO CONFIG-TEXT, remove the line with the unix-domain SocksPort (that was created for this session only), and then write the torrc file directly from JavaScript.

That is an interesting idea but possibly fragile, since it might be tricky to figure out for sure which lines to filter out before rewriting torrc. Kathy and I will think about this a little more.

Because you know the domain socket path (in socksPortInfo.ipcFile), I think you can use an exact match to filter that line out.

One alternative solution would be to remove old unix-domain SocksPort settings from the config settings before you issue saveconfig. You could read them all in using getconf socksport and then set all but the obsolete ones using setconf socksport a socksport b again. There would need to be a way to distinguish obsolete Tor Launcher SocksPorts from user-added SocksPorts, however, which might be difficult.

I think another way to look at it is that perhaps tor could handle redundant SocksPorts better. I just tried the following:

~> tor +SocksPort 9999 +SocksPort 9998 +SocksPort 9999

and the result was:

Dec 02 11:43:32.724 [notice] Tor v0.2.7.6 (git-605ae665009853bd) running on Linux with Libevent 2.0.21-stable, OpenSSL 1.0.1t and Zlib 1.2.8.
Dec 02 11:43:32.725 [notice] Tor can't help you if you use it wrong! Learn how to be safe at https://www.torproject.org/download/download#warning
Dec 02 11:43:32.725 [notice] Read configuration file "/etc/tor/torrc".
Dec 02 11:43:32.727 [notice] Opening Socks listener on 127.0.0.1:9150
Dec 02 11:43:32.728 [notice] Opening Socks listener on 127.0.0.1:9999
Dec 02 11:43:32.728 [notice] Opening Socks listener on 127.0.0.1:9998
Dec 02 11:43:32.728 [notice] Opening Socks listener on 127.0.0.1:9999
Dec 02 11:43:32.728 [warn] Could not bind to 127.0.0.1:9999: Address already in use. Is Tor already running?

It seems to me a better behavior would be to simply ignore the redundant request for +SocksPort 9999. In other words, tor should store SocksPorts in a set rather than a list. Then if we were to issue saveconfig over the control port, it would only save a single line with SocksPort 9999. So if Tor Launcher keeps adding the same unix domain socket every time it launches (+ SocksPort /domain/socket/path), it won't accumulate redundant copies in the torrc file.

Given the way tor behaves today and the fact that we don’t have much time to change it, Kathy and I have two options to propose:

Proposal 1 (require users to set prefs to configure additional listeners): A. Switch the default values of the use_ipc prefs to false so that TCP ports are used by default. B. Remove the SocksPort and ControlPort lines from torrc-defaults because they are not used. C. When configuring listeners, Tor Launcher will use the same method as in TB 6.5a5 (command line arguments) except it will also recognize new prefs (e.g., extensions.torlauncher.socks_port_extra.1) and pass additional command line arguments to enable additional listeners.

Proposal 2 (allow users to transition from config file to preferences): A. Switch the default values of the use_ipc prefs to false so that TCP ports are used by default (as in Proposal 1). B. If use_ipc is false, Tor Launcher will not configure the listener via command line arguments; instead, it will rely on the torrc-default / torrc settings like TB 6.0.x does. C. If use_ipc is true, the behavior for the will be as in TB 6.5a5 except Tor Launcher will recognize new prefs (e.g., extensions.torlauncher.socks_port_extra.1) and pass additional command line arguments to enable the additional listeners (as in Proposal 1). As an exception to “B” above, Kathy and I think it makes sense to use the extra prefs even when use_ipc is false if any "port_extra" pref values have been added by the user (instead of relying on the config file).

Proposal 1 is simpler but forces advanced users to change how they configure things as soon as they start using TB 6.5. Proposal 2 will be a little more complex to implement and maintain, but it has the advantage that users who are content to stick with TCP can continue to configure all of their ports via the torrc file.

I think 2B is an important property (probably more important than we thought). Thus, I'd say let's try to get proposal 2 done and tested in 6.5a6. (if that's realistic time-wise)

Trac:
Status: needs_information to assigned
Owner: brade to mcs

There are two commits on the following branch that need review: https://gitweb.torproject.org/user/brade/tor-launcher.git/log/?h=bug20761-01

The older commit switches the default to TCP listeners. The second commit implements our Proposal 2 from comment:12.

Switching back to TCP listeners after running with use_ipc = true may still require some manual cleanup by the user (e.g., if the unix: paths were written to their torrc), but I don't think we can avoid that until we fix #20906 (moved).

Trac:
Keywords: TorBrowserTeam201612 deleted, TorBrowserTeam201612R added
Status: assigned to needs_review

Let me add that Kathy and I tested the new code using a wide variety of scenarios on OSX, but more testing would be appreciated.

Replying to mcs:

There are two commits on the following branch that need review: https://gitweb.torproject.org/user/brade/tor-launcher.git/log/?h=bug20761-01

The older commit switches the default to TCP listeners. The second commit implements our Proposal 2 from comment:12.

Looks good to me. Just one nit: if any, -> if any..

Switching back to TCP listeners after running with use_ipc = true may still require some manual cleanup by the user (e.g., if the unix: paths were written to their torrc), but I don't think we can avoid that until we fix #20906 (moved).

Hm. I hit that one as well when testing in the hardened build I am currently using. IIRC this is only an issue if one has clicked on "OK" on the network settings dialog once, right? After removing the problematic lines everything works fine. I tested in a clean alpha and even a clean stable and in both scenarios we are using TCP + the old behavior is visible. Good. Now, the question I am wrestling with is: "Could that be testing enough or do we really need to break a bunch of alpha users by disabling Unix domain sockets just to enable them in 7.0a1 again?"

I wonder how much testing we are actually getting for the scenarios we are caring about if we shipped that patch in the next alpha:

1. We don't get an idea about what is happening during an update from 6.0.x to 6.5 with IPC disabled as the current alpha has it enabled.
2. We would get an idea about what is happening if a user downloads a fresh Tor Browser and is using that one.
3. We would get an idea about whether Tor Browser is behaving well over a longer period of time with IPC being disabled.

Plus we get 4) We get an idea about scenarios that won't happen with 6.5 being shipped (i.e. the breakage we are talking about).

Replying to gk:

Looks good to me. Just one nit: if any, -> if any..

Thanks for catching that typo.

Switching back to TCP listeners after running with use_ipc = true may still require some manual cleanup by the user (e.g., if the unix: paths were written to their torrc), but I don't think we can avoid that until we fix #20906 (moved).

Hm. I hit that one as well when testing in the hardened build I am currently using. IIRC this is only an issue if one has clicked on "OK" on the network settings dialog once, right?

Actually, Tor Launcher also does a SAVECONF when transitioning from DisableNetwork=1 to DisableNetwork=0, which happens after the last step in the setup wizard and each time during startup if a default bridge is being used. The bottom line is that most alpha users will have ControlPort and SocksPort lines in their torrc, which means if we land these patches then things will break after the 6.5a6 update is applied.

I am sorry that we overlooked this; Kathy and I consider it to be a fatal flaw with Proposal 2. This transition problem is bad enough that Kathy feels strongly that we should just switch to Proposal 1 and retrain our users to use the "extra" prefs if they need additional listeners. We could even do something a little more interesting such as ship with use_ipc = true but add an extra pref that provides an additional SOCKS listener on TCP port 9150 (the decision to do that needs a separate discussion though).

I would like to stick with Proposal 2 but all of the ideas I have come up with handling a transition from use_ipc = true to use_ipc = false are too fragile (e.g., try to detect the situation and clean up the user's torrc using file I/O). Therefore, I have to side with Kathy and argue that we should implement Proposal 1.

Trac:
Status: needs_review to needs_information

gk and I had a discussion about this on #tor-dev. Because of the not-very-satisfying tradeoffs that we are wrestling with, we agreed on the following plan:

• Before we ship TB 6.5 stable, we will back out support for Unix domain sockets.
• We will leave things as-is on the alpha channel while we look for a better solution. In other words, alpha users will have to live with this bug for a while longer.

Trac:
Keywords: TorBrowserTeam201612R deleted, TorBrowserTeam201612 added