anonym wrote:
> 11/02/14 19:14, winterfairy at riseup.net wrote:
>> I believe I have fixed the regression described in ticket #6383. When
>> access to Tor's control port was restricted (to prevent GETINFO
>> address), Torbutton could no longer do "New Identity". I have created
>> a filtering proxy for the control port, that only allows SIGNAL NEWNYM.
>> This is enough to make "New Identity" work again as expected.
>>
>> Branch "winterfairy:bugfix/torbutton-new-identity" in my Tails repo
>> (based on devel).
>
> Thanks for you contribution!
>
>> Please review and test it, and merge it is it looks fine. If you believe
>> something could be improved, or should be done differently, please say.
>
> My test looked good. When it comes to reviewing I don't have much to add
> as intrigeri caught most blockers:
>
>> +TOR_CONTROL_PASSWD='passwd'
>
> Why do we set this?
Iceweasel (as "amnesia") does not have access to the authentication
cookie, and shouldn't. If Torbutton cannot read the authentication cookie,
it refuses to talk to the control port, even when substituted with this
filter. So I need to tell it what the cookie is using TOR_CONTROL_PASSWD,
and it will now talk to the filter trying to authenticate with this value
instead. The filter accepts all authentication regardless of value, as
noted in the code.
>> + # Read in a newline terminated line (max 128 chars)
>> + line = readh.readline(128)
>
> Why do we read 128 characters specifically (same thing done for all
> other socket reads)? Tor's control language always terminate responses
> with CRLF so why can't we use `readline()` without argument, which reads
> until a LF? I suppose that'll leave a trailing CR, but that could be
> dropped if necessary, which I don't think we need to care about because
> of how we verify responses with `startswith`.
By limiting line length, we gain some protection against DoS attacks on
the filter. The attacker cannot trigger an OOM by sending a 10 GB long
row, for example, thereas that maybe would be possible if reading until
LF.
I feel slightly more comfortable knowing you cannot OOM/crash the
application, even if there is other ways to prevent the user from reaching
the control port filter (all this assuming the attacker is running
software as amnesia).
> While I'm pretty sure this will be ok in the subset of the control
> language we deal with (*currently*), I'd at least want to hear how you
> thought about this with a code comment. While you're at it, it'd be
> great if you made the reads use a nicely named constant (and put the
> comment where it's defined) instead of magic 128:s everywhere.
Done, same branch.
> After that's fixed I'm ready to merge this.
>
> As a side note, I suppose it'd be nice to use stem [1] to greatly
> simplify the script and in particular avoid the low-level socket stuff
> but it's not packaged in Debian. :/
>
> [1] https://stem.torproject.org/
I am not familiar with Stem, so I cannot tell. But I think it might only
help with the talking to a control port stuff, not simulating our own
control port stuff.
