[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #20761 [Applications/Tor Launcher]: Tor Browser 6.5a4 is ignoring additional SocksPorts
#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
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs