Opened 3 years ago

Closed 2 years ago

#20761 closed defect (fixed)

Tor Browser 6.5a4 is ignoring additional SocksPorts

Reported by: gk Owned by: mcs
Priority: Medium Milestone:
Component: Applications/Tor Launcher Version:
Severity: Normal Keywords: TorBrowserTeam201705R
Cc: mcs, arthuredelstein, brade Actual Points:
Parent ID: Points:
Reviewer: Sponsor: Sponsor4

Description

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.

Child Tickets

Change History (50)

comment:1 Changed 3 years ago by gk

Keywords: TorBrowserTeam201611 added; TorBrowserTeam2011 removed

comment:2 Changed 3 years ago by mcs

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

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.

comment:3 Changed 3 years ago by mcs

Cc: arthuredelstein added
Keywords: TorBrowserTeam201612 added; TorBrowserTeam201611 removed
Status: newneeds_information

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.

comment:4 in reply to:  3 ; Changed 3 years ago by 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.

comment:5 in reply to:  4 Changed 3 years ago by gk

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.

comment:6 in reply to:  4 ; Changed 3 years ago by 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?

comment:7 in reply to:  6 ; Changed 3 years ago by 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.

comment:8 in reply to:  7 ; Changed 3 years ago by mcs

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.

comment:9 in reply to:  7 Changed 3 years ago by gk

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)?

comment:10 in reply to:  8 Changed 3 years ago by arthuredelstein

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.

comment:11 Changed 3 years ago by arthuredelstein

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.

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

comment:12 Changed 3 years ago by mcs

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):

  1. Switch the default values of the use_ipc prefs to false so that TCP ports are used by default.
  2. Remove the SocksPort and ControlPort lines from torrc-defaults because they are not used.
  3. 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):

  1. Switch the default values of the use_ipc prefs to false so that TCP ports are used by default (as in Proposal 1).
  2. 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.
  3. 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.

comment:13 Changed 3 years ago by gk

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)

comment:14 Changed 3 years ago by mcs

Owner: changed from brade to mcs
Status: needs_informationassigned

comment:15 Changed 3 years ago by mcs

Keywords: TorBrowserTeam201612R added; TorBrowserTeam201612 removed
Status: assignedneeds_review

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.

comment:16 Changed 3 years ago by mcs

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.

comment:17 in reply to:  15 ; Changed 3 years ago by gk

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.

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).

comment:18 in reply to:  17 Changed 3 years ago by mcs

Status: needs_reviewneeds_information

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.

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.

comment:19 Changed 3 years ago by mcs

Keywords: TorBrowserTeam201612 added; TorBrowserTeam201612R removed

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.

comment:20 Changed 3 years ago by mcs

Should we create a ticket for the back out task? There are several Tor Launcher patches that we will need to back out:

  • 8871259c966755233b134a5ddb2b4926539d25c6 (#14272)
  • fa4e114a20067810af0c5cc6a57aa6e849386418 (#14272 fixup)
  • 8ca52414916c3d8bc2a2974017d759901ddc1736 (#20111)
  • 4dd8f6130f931616cf014e0ded444c30e04c8bad (#20185)

There are also Torbutton patches for #14272 and #20111, although it may be okay to leave them code in place (the code implements fallbacks).

comment:21 in reply to:  20 Changed 3 years ago by gk

Replying to mcs:

Should we create a ticket for the back out task? There are several Tor Launcher patches that we will need to back out:

  • 8871259c966755233b134a5ddb2b4926539d25c6 (#14272)
  • fa4e114a20067810af0c5cc6a57aa6e849386418 (#14272 fixup)
  • 8ca52414916c3d8bc2a2974017d759901ddc1736 (#20111)
  • 4dd8f6130f931616cf014e0ded444c30e04c8bad (#20185)

There are also Torbutton patches for #14272 and #20111, although it may be okay to leave them code in place (the code implements fallbacks).

I think we should back out everything that is related (although I guess we can leave the tor-browser patches in place). A ticket would be good as I think having someone reviewing the backout is a good idea in this case.

comment:22 Changed 3 years ago by gk

I've created #20951 for the backout task.

comment:23 Changed 3 years ago by mcs

I created #20956 "optionally do not write command line config to torrc" (a Core Tor enhancement that would help us fix this bug in a cleaner way).

comment:24 Changed 3 years ago by gk

Keywords: TorBrowserTeam201701 added; TorBrowserTeam201612 removed

Moving our tickets to January 2017

comment:25 Changed 3 years ago by gk

Status: needs_informationassigned

comment:26 Changed 2 years ago by arma

I saw a #20956 get merged to Tor master today. It's queued for a backport to 0.2.9.

Does today's Tor Browser 6.5 release suffer from this bug (#20761), meaning now it's not just an issue in alpha, but an issue in stable too?

comment:27 in reply to:  26 Changed 2 years ago by gk

Replying to arma:

I saw a #20956 get merged to Tor master today. It's queued for a backport to 0.2.9.

Does today's Tor Browser 6.5 release suffer from this bug (#20761), meaning now it's not just an issue in alpha, but an issue in stable too?

It should not as we backed out all unix domain socket code in the stable due do this bug (alas).

comment:28 Changed 2 years ago by mcs

Keywords: TorBrowserTeam201701R added; TorBrowserTeam201701 removed

Now that the fix for #20956 has landed, we can proceed with Tor Launcher changes that take advantage of it. Here are two patches for review (the tor-browser-bundle patch removes the SocksPort and ControlPort lines from torrc-defaults to avoid conflcts with what Tor Launcher does):

https://gitweb.torproject.org/user/brade/tor-browser-bundle.git/commit/?h=bug20761-02&id=5c44ae3568b9f42fcebb8426d567c0311c4c9b19

https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug20761-02&id=5060eb48b7fd401264888ccef1876f647daeb494

There is still one open issue: some current alpha users have SocksPort and ControlPort lines in their torrc that are there because tor wrote them (due to a SAVECONF). After an upgrade, lines will likely cause conflicts with the ports that the new Tor Launcher will add. Do we need to clean these up for people or is it acceptable to cause pain for alpha users? (manual edit of torrc required)

It seems like we may want to try to clean them up, but to do that we need to add code to Tor Launcher that runs once and tries to delete the offending lines from torrc.

comment:29 Changed 2 years ago by mcs

Status: assignedneeds_review

comment:30 in reply to:  28 Changed 2 years ago by arthuredelstein

Replying to mcs:

Now that the fix for #20956 has landed, we can proceed with Tor Launcher changes that take advantage of it. Here are two patches for review (the tor-browser-bundle patch removes the SocksPort and ControlPort lines from torrc-defaults to avoid conflcts with what Tor Launcher does):

https://gitweb.torproject.org/user/brade/tor-browser-bundle.git/commit/?h=bug20761-02&id=5c44ae3568b9f42fcebb8426d567c0311c4c9b19

https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug20761-02&id=5060eb48b7fd401264888ccef1876f647daeb494

These patches look good to me.

There is still one open issue: some current alpha users have SocksPort and ControlPort lines in their torrc that are there because tor wrote them (due to a SAVECONF). After an upgrade, lines will likely cause conflicts with the ports that the new Tor Launcher will add. Do we need to clean these up for people or is it acceptable to cause pain for alpha users? (manual edit of torrc required)

This problem would also be avoided with a fix for #20906. :)

comment:31 Changed 2 years ago by gk

Keywords: TorBrowserTeam201702R added; TorBrowserTeam201701R removed

Moving review tickets.

comment:32 Changed 2 years ago by gk

Sponsor: Sponsor4

comment:33 in reply to:  28 ; Changed 2 years ago by gk

Replying to mcs:

There is still one open issue: some current alpha users have SocksPort and ControlPort lines in their torrc that are there because tor wrote them (due to a SAVECONF). After an upgrade, lines will likely cause conflicts with the ports that the new Tor Launcher will add. Do we need to clean these up for people or is it acceptable to cause pain for alpha users? (manual edit of torrc required)

It seems like we may want to try to clean them up, but to do that we need to add code to Tor Launcher that runs once and tries to delete the offending lines from torrc.

Hmm. What about stable users once they get the fix in this bug? Aren't they potentially affected as well?

comment:34 in reply to:  33 Changed 2 years ago by mcs

Keywords: TorBrowserTeam201702 added; TorBrowserTeam201702R removed
Status: needs_reviewneeds_revision

Replying to gk:

Hmm. What about stable users once they get the fix in this bug? Aren't they potentially affected as well?

The versions of Tor Launcher that have shipped on our stable channel have never set SocksPort or ControlPort from code, so the likelihood that there will be a problem is much lower than for alpha channel users. But you are right: if someone has added additional SocksPort or ControlPort lines to their torrc, a conflict will occur.

With some reluctance, Kathy and I believe the best thing to do is for Tor Launcher to perform a one-time "fixup" of the user's torrc file. We will write code to remove any ControlPort and SocksPort lines that match what Tor Launcher will automatically configure via args when it starts tor. We will prepare a revised patch soon so we can think about including it in the next alpha.

comment:35 Changed 2 years ago by gk

Keywords: TorBrowserTeam201703 added; TorBrowserTeam201702 removed

Moving tickets to March

comment:36 Changed 2 years ago by mcs

Keywords: TorBrowserTeam201703R added; TorBrowserTeam201703 removed
Status: needs_revisionneeds_review

Here is a revised Tor Launcher patch that adds code to perform a one-time clean up of the user's torrc:
https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug20761-03&id=0b1b668f93926f28e167e597bef4543393d55ddb

The following patch is still needed as before:
https://gitweb.torproject.org/user/brade/tor-browser-bundle.git/commit/?h=bug20761-02&id=5c44ae3568b9f42fcebb8426d567c0311c4c9b19

Parsing torrc files is not trivial due to line continuation rules, so more testing with "real world" torrc files would be very welcome.

comment:37 Changed 2 years ago by gk

Keywords: TorBrowserTeam201704R added; TorBrowserTeam201703R removed

Moving review tickets to April.

comment:38 Changed 2 years ago by gk

Keywords: TorBrowserTeam201704 added; TorBrowserTeam201704R removed
Status: needs_reviewneeds_revision

Let's suppose you have something like

ControlPort unix:/run/user/1000/Tor/control.socket \
# Foo
DataDirectory /home/thomas/Arbeit/Tor/tor-browser-bundle/tor-browser/Browser/TorBrowser/Data/Tor

in your torrc. Then you'll end up with

[04-13 13:11:17] TorLauncher INFO: _fixupTorrc: removing ControlPort unix:/run/user/1000/Tor/control.socket DataDirectory /home/thomas/Arbeit/Tor/tor-browser-bundle/tor-browser/Browser/TorBrowser/Data/Tor

which seems wrong to me. Note first the comment is gone. That's due to

            // Remove trailing comment from continued line.
            aLine = aLine.substr(0, idx);

. Not sure if that's intended or not but it is surprising to me. It seems to me we want to end up with

ControlPort unix:/run/user/1000/Tor/control.socket # Foo

instead.

DataDirectory /home/thomas/Arbeit/Tor/tor-browser-bundle/tor-browser/Browser/TorBrowser/Data/Tor

seems to get added because continuedLine is still true and we have tmpLine. But continuedLine should not be true anymore in this case.

Please take things like

ControlPort 127.0.0.1:9050\
#Foo

into account as well. I guess this is perfectly valid (I have not checked tor code) but we could easily end up comparinf 9050#Foo or 9050Foo to 9050 and the line would not get removed although it should.

Nits:

if (0 == len)

should be

if (len == 0)

and one whitespace too much after the = in

let idx = aLine.indexOf("#");

.

comment:39 Changed 2 years ago by mcs

Keywords: TorBrowserTeam201704R added; TorBrowserTeam201704 removed
Status: needs_revisionneeds_review

Thanks for your review! Here is an updated patch:
https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug20761-04&id=1781f041f00f8f18a05b49d848d50fad0a6be40d

We added comments to clarify the handling of comments within continued lines. It is counterintuitive, but tor essentially ignores comments within continued line sequences and does not interrupt the continued line. See the definition of ContinuedVal within https://gitweb.torproject.org/tor.git/tree/doc/torrc_format.txt. Also, we did not attempt to preserve the comment in that case because it is difficult to do so. Of course tor does not preserve comments when it rewrites torrc in response to a SAVECONF command anyway, which will happen if Tor Launcher's Network Settings dialog is ever used.

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

Keywords: TorBrowserTeam201704 added; TorBrowserTeam201704R removed
Status: needs_reviewneeds_revision

Replying to mcs:

Thanks for your review! Here is an updated patch:
https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug20761-04&id=1781f041f00f8f18a05b49d848d50fad0a6be40d

We added comments to clarify the handling of comments within continued lines. It is counterintuitive, but tor essentially ignores comments within continued line sequences and does not interrupt the continued line. See the definition of ContinuedVal within https://gitweb.torproject.org/tor.git/tree/doc/torrc_format.txt. Also, we did not attempt to preserve the comment in that case because it is difficult to do so. Of course tor does not preserve comments when it rewrites torrc in response to a SAVECONF command anyway, which will happen if Tor Launcher's Network Settings dialog is ever used.

Thanks for the comments. It seems I underestimated the fanciness of this format while looking over it the first time. Here are my additional comments (basically just nits):

+    // Handle several cases:
+    //  SocksPort "unix:/path options"
+    //  SocksPort unix:"/path" options
+    //  SocksPort unix:/path options

I think removing "SocksPort" here would be good as this is method used for ControlPort as well.

+      if (removeLine)
+      {
+        ++removedLinesCount;
+        TorLauncherLogger.log(3, "_fixupTorrc: removing " + aLine);
+      }
+      else
+        revisedLines.push(aLine);

Please don't mix bracing style within if/else clause.

+      let ioFlags = 0x02 | 0x08 | 0x20; // WRONLY CREATE TRUNCATE
+      stream.init(aFile, ioFlags, 0600, 0);

I think there is no need to have a let ioFlags here. ioFlags is only used this one time.

What worries me more is how we would deploy the patch and related ones in the stable series. The thing is that probably a lot of users on macOS and Linux who are using e.g. TorBirdy have it configured to point to Tor Browser. And just switching to unix domain sockets in it is breaking things. I am actually not sure if doing that is worth it as we are not really enhancing security by ripping AF_INET related code out or enforcing it by sandboxing means in the stable series. Breaking a bunch of setups for no real enhancement seems not to be the way to go IMO.

comment:41 Changed 2 years ago by gk

I think I've found another, more serious issue: the \s+ in your matching regexes is wrong (e.g. in

+      let matchResult = aLine.match(/\s*\+*controlport\s+(.*)/i);

).
It is not guaranteed that you have at least one whitespace between key and value. Or are we optimizing for the SAVECONF case and there it is guaranteed?

comment:42 Changed 2 years ago by gk

To be more explicit:

ControlPort\
127.0.0.1:\
9050

is perfectly fine but would give you, after resolving the multiline, ControlPort127.0.0.1:9050. tor can handle that case, though.

Last edited 2 years ago by gk (previous) (diff)

comment:43 Changed 2 years ago by mcs

Cc: brade added

comment:44 Changed 2 years ago by gk

Keywords: TorBrowserTeam201705 added; TorBrowserTeam201704 removed

Moving our tickets to May 2017.

comment:45 in reply to:  40 ; Changed 2 years ago by mcs

Replying to gk:

What worries me more is how we would deploy the patch and related ones in the stable series. The thing is that probably a lot of users on macOS and Linux who are using e.g. TorBirdy have it configured to point to Tor Browser. And just switching to unix domain sockets in it is breaking things. I am actually not sure if doing that is worth it as we are not really enhancing security by ripping AF_INET related code out or enforcing it by sandboxing means in the stable series. Breaking a bunch of setups for no real enhancement seems not to be the way to go IMO.

You make a good point that, given the lack of sandboxing in the stable series, it does not make sense to break things that previously worked (such as TorBirdy finding a SOCKS listener on TCP port 9150). Maybe we should ship our stable releases with extensions.torlauncher.control_port_use_ipc = false and extensions.torlauncher.socks_port_use_ipc = false.

comment:46 in reply to:  41 Changed 2 years ago by mcs

Keywords: TorBrowserTeam201705R added; TorBrowserTeam201705 removed
Status: needs_revisionneeds_review

Replying to gk:

It is not guaranteed that you have at least one whitespace between key and value. Or are we optimizing for the SAVECONF case and there it is guaranteed?

This was an oversight due to our incomplete understanding of tor's config file parser. Kathy and I made a fix and also addressed the other small issues you raised in comment:40. Here is our new patch:
https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug20761-05&id=5154173e0facfff26bede4c37d2c47669350d0ff

comment:47 in reply to:  45 Changed 2 years ago by gk

Replying to mcs:

Replying to gk:

What worries me more is how we would deploy the patch and related ones in the stable series. The thing is that probably a lot of users on macOS and Linux who are using e.g. TorBirdy have it configured to point to Tor Browser. And just switching to unix domain sockets in it is breaking things. I am actually not sure if doing that is worth it as we are not really enhancing security by ripping AF_INET related code out or enforcing it by sandboxing means in the stable series. Breaking a bunch of setups for no real enhancement seems not to be the way to go IMO.

You make a good point that, given the lack of sandboxing in the stable series, it does not make sense to break things that previously worked (such as TorBirdy finding a SOCKS listener on TCP port 9150). Maybe we should ship our stable releases with extensions.torlauncher.control_port_use_ipc = false and extensions.torlauncher.socks_port_use_ipc = false.

I think so. Could you make an additional patch that does this so we can test the whole piece in the next alpha?

comment:48 Changed 2 years ago by gk

Keywords: TorBrowserTeam201705 added; TorBrowserTeam201705R removed
Status: needs_reviewneeds_revision

comment:50 Changed 2 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Let's ship it! commit 3f32f7a6dd4970936cc49d988369a61efa87c031 on tor-browser-bundle master and commit 5154173e0facfff26bede4c37d2c47669350d0ff + 485ba9456724e38b661c90dccc90322c74fa405f on tor-launcher master have the code.

Note: See TracTickets for help on using tickets.