27/02/14 15:58, winterfairy@??? wrote:
> 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.
Ah, I see. Could you please add a code comment at the place where
`TOR_CONTROL_PASSWD` is set, explaining this non-obvious (at least to me
:)) assignment?
>>> + # 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).
Fair enough.
>> 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.
Looks good, thanks!
>> 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.
Exactly, so we wouldn't have to maintain that code any more, which is
something. It's true, though, that the investment might not be worth it
now, but who knows? Perhaps we want to extend the filter in the future,
making it more worthwhile? Let's leave it until then.
Cheers!
