[tor-bugs] #20761 [Applications/Tor Launcher]: Tor Browser 6.5a4 is ignoring additional SocksPorts

[Tor Bug Tracker & Wiki]

#20761: Tor Browser 6.5a4 is ignoring additional SocksPorts
---------------------------------------+--------------------------------
 Reporter:  gk                         |          Owner:  mcs
     Type:  defect                     |         Status:  needs_revision
 Priority:  Medium                     |      Milestone:
Component:  Applications/Tor Launcher  |        Version:
 Severity:  Normal                     |     Resolution:
 Keywords:  TorBrowserTeam201704       |  Actual Points:
Parent ID:                             |         Points:
 Reviewer:                             |        Sponsor:  Sponsor4
---------------------------------------+--------------------------------
Changes (by gk):

 * keywords:  TorBrowserTeam201704R => TorBrowserTeam201704
 * status:  needs_review => needs_revision


Comment:

 Replying to [comment:39 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.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/20761#comment:40>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online