Re: [Tails-dev] Made New Identity work again, please review
28/02/14 13:56, winterfa...@riseup.net wrote: +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? Done, same branch (winterfairy:bugfix/torbutton-new-identity). Excellent! It's now merged. Thanks again for you contribution! Cheers! ___ tails-dev mailing list tails-dev@boum.org https://mailman.boum.org/listinfo/tails-dev To unsubscribe from this list, send an empty email to tails-dev-unsubscr...@boum.org.
[Tails-dev] Made New Identity work again, please review
+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? Done, same branch (winterfairy:bugfix/torbutton-new-identity). ___ tails-dev mailing list tails-dev@boum.org https://mailman.boum.org/listinfo/tails-dev To unsubscribe from this list, send an empty email to tails-dev-unsubscr...@boum.org.
[Tails-dev] Made New Identity work again, please review
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. ___ tails-dev mailing list tails-dev@boum.org https://mailman.boum.org/listinfo/tails-dev To unsubscribe from this list, send an empty email to tails-dev-unsubscr...@boum.org.
Re: [Tails-dev] Made New Identity work again, please review
Hi, winterfa...@riseup.net wrote (27 Feb 2014 14:58:11 GMT) : anonym wrote: 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 Fully agreed. This refactoring would be worth a low-priority ticket, IMHO: if properly documented, this would be a good Easy coding task for new contributors :) but it's not packaged in Debian. :/ It is: python-stem | 1.1.0-1 | jessie | source, all python-stem | 1.1.0-1 | sid| source, all Want to request a backport for Wheezy, so that we can use it in 1.1+? (In any case, I doubt it's worth backporting it for Squeeze *now*, so the low-priority refactoring ticket, if any, should be blocked by #6015.) 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. Sure. It still would be an interesting improvement, though. Cheers, -- intrigeri | GnuPG key @ https://gaffer.ptitcanardnoir.org/intrigeri/intrigeri.asc | OTR fingerprint @ https://gaffer.ptitcanardnoir.org/intrigeri/otr.asc ___ tails-dev mailing list tails-dev@boum.org https://mailman.boum.org/listinfo/tails-dev To unsubscribe from this list, send an empty email to tails-dev-unsubscr...@boum.org.
Re: [Tails-dev] Made New Identity work again, please review
27/02/14 19:25, intrigeri wrote: Hi, winterfa...@riseup.net wrote (27 Feb 2014 14:58:11 GMT) : anonym wrote: 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 Fully agreed. This refactoring would be worth a low-priority ticket, IMHO: if properly documented, this would be a good Easy coding task for new contributors :) Tracked in #6788. but it's not packaged in Debian. :/ It is: [...] Sorry, I meant Debian squeeze, which means that it's not relevant at the moment since time is short. Want to request a backport for Wheezy, so that we can use it in 1.1+? (In any case, I doubt it's worth backporting it for Squeeze *now*, so the low-priority refactoring ticket, if any, should be blocked by #6015.) Actually I think we can wait with all of this until the time we want to extend the control port filter, if it ever comes. Just to not waste people's time unnecessarily. Cheers! ___ tails-dev mailing list tails-dev@boum.org https://mailman.boum.org/listinfo/tails-dev To unsubscribe from this list, send an empty email to tails-dev-unsubscr...@boum.org.
Re: [Tails-dev] Made New Identity work again, please review
27/02/14 15:58, winterfa...@riseup.net 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! ___ tails-dev mailing list tails-dev@boum.org https://mailman.boum.org/listinfo/tails-dev To unsubscribe from this list, send an empty email to tails-dev-unsubscr...@boum.org.
[Tails-dev] Made New Identity work again, please review
intrigeri wrote: winterfairy at riseup.net wrote (20 Feb 2014 11:52:17 GMT) : 3. It might be overkill, and surely adds some code, but I would pass the port to listen on, control port socket path and authentication cookie path as command-line arguments (preferably named, not positional). Just to make it easier for others to reuse this piece of code, and increase collaboration between similar projects. A quick research showed up that this can probably be done with only a few lines of code if using the modules optparse or argparse. Python 2.6 (which Tails uses) only has optparse. In python 2.7 optparse is deprecated and replaced by argparse. This would mean writing one version for squeeze and one for wheezy. I don't think it is worth it currently. Fair enough. Care to file a ticket, blocked by #6015, so that this is not forgotten at post-1.1 time? I guess I should wait until my branch is actually accepted/merged into devel? If you also mark the new ticket as blocked by the new identity one, I see little use in waiting :) Ticket filed. https://labs.riseup.net/code/issues/6742 ___ tails-dev mailing list tails-dev@boum.org https://mailman.boum.org/listinfo/tails-dev To unsubscribe from this list, send an empty email to tails-dev-unsubscr...@boum.org.
[Tails-dev] Made New Identity work again, please review
intrigeri wrote: Hi, (Note: it would be great not to break threading: anyone reading the old thread, but not this one, will miss your reply etc. IIRC you read the list via the archives, so I understand it's a bit of a pain, but hopefully your MUA allows you to insert arbitrary References and In-Reply-To headers.) Don't think so. Sorry. 3. It might be overkill, and surely adds some code, but I would pass the port to listen on, control port socket path and authentication cookie path as command-line arguments (preferably named, not positional). Just to make it easier for others to reuse this piece of code, and increase collaboration between similar projects. A quick research showed up that this can probably be done with only a few lines of code if using the modules optparse or argparse. Python 2.6 (which Tails uses) only has optparse. In python 2.7 optparse is deprecated and replaced by argparse. This would mean writing one version for squeeze and one for wheezy. I don't think it is worth it currently. Fair enough. Care to file a ticket, blocked by #6015, so that this is not forgotten at post-1.1 time? I guess I should wait until my branch is actually accepted/merged into devel? ___ tails-dev mailing list tails-dev@boum.org https://mailman.boum.org/listinfo/tails-dev To unsubscribe from this list, send an empty email to tails-dev-unsubscr...@boum.org.
Re: [Tails-dev] Made New Identity work again, please review
winterfa...@riseup.net wrote (20 Feb 2014 11:52:17 GMT) : 3. It might be overkill, and surely adds some code, but I would pass the port to listen on, control port socket path and authentication cookie path as command-line arguments (preferably named, not positional). Just to make it easier for others to reuse this piece of code, and increase collaboration between similar projects. A quick research showed up that this can probably be done with only a few lines of code if using the modules optparse or argparse. Python 2.6 (which Tails uses) only has optparse. In python 2.7 optparse is deprecated and replaced by argparse. This would mean writing one version for squeeze and one for wheezy. I don't think it is worth it currently. Fair enough. Care to file a ticket, blocked by #6015, so that this is not forgotten at post-1.1 time? I guess I should wait until my branch is actually accepted/merged into devel? If you also mark the new ticket as blocked by the new identity one, I see little use in waiting :) Cheers, -- intrigeri | GnuPG key @ https://gaffer.ptitcanardnoir.org/intrigeri/intrigeri.asc | OTR fingerprint @ https://gaffer.ptitcanardnoir.org/intrigeri/otr.asc ___ tails-dev mailing list tails-dev@boum.org https://mailman.boum.org/listinfo/tails-dev To unsubscribe from this list, send an empty email to tails-dev-unsubscr...@boum.org.
Re: [Tails-dev] Made New Identity work again, please review
Hi, (Note: it would be great not to break threading: anyone reading the old thread, but not this one, will miss your reply etc. IIRC you read the list via the archives, so I understand it's a bit of a pain, but hopefully your MUA allows you to insert arbitrary References and In-Reply-To headers.) winterfa...@riseup.net wrote (13 Feb 2014 12:20:36 GMT) : intrigeri wrote: 1. I'm not entirely convinced by the idea of running tor-controlport-filter as the vidalia user. Granted, it's the shortest path, but it feels weird, and having a bit more privilege separation would be good. Fixed. Same branch. Looks good to me. 2. The use of string.find(expected text) seems a bit fragile. Couldn't this matching be done in a bit stricter way? I am not sure what you mean by stricter. I changed it to startswith() anyway, which is equivalent to what I did, but clearer. find() reports position of first occurence (-1 on error). I was testing that the position was == 0. Oh, thanks for the explanation, and for making the code clearer. Indeed the semantics of `find' were not obvious to me without reading the doc. 3. It might be overkill, and surely adds some code, but I would pass the port to listen on, control port socket path and authentication cookie path as command-line arguments (preferably named, not positional). Just to make it easier for others to reuse this piece of code, and increase collaboration between similar projects. A quick research showed up that this can probably be done with only a few lines of code if using the modules optparse or argparse. Python 2.6 (which Tails uses) only has optparse. In python 2.7 optparse is deprecated and replaced by argparse. This would mean writing one version for squeeze and one for wheezy. I don't think it is worth it currently. Fair enough. Care to file a ticket, blocked by #6015, so that this is not forgotten at post-1.1 time? Merged again into experimental, pushed again to the main repo and to Jenkins. Cheers, -- intrigeri | GnuPG key @ https://gaffer.ptitcanardnoir.org/intrigeri/intrigeri.asc | OTR fingerprint @ https://gaffer.ptitcanardnoir.org/intrigeri/otr.asc ___ tails-dev mailing list tails-dev@boum.org https://mailman.boum.org/listinfo/tails-dev To unsubscribe from this list, send an empty email to tails-dev-unsubscr...@boum.org.
[Tails-dev] Made New Identity work again, please review
intrigeri wrote: winterfairy 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. Congrats! This usability regression was one of the top mentionned ones in my experience when doing user support in the last few months. I won't be doing the entire review'n'merge process, but I had a quick look, and here are a few quick notes: 1. I'm not entirely convinced by the idea of running tor-controlport-filter as the vidalia user. Granted, it's the shortest path, but it feels weird, and having a bit more privilege separation would be good. Fixed. Same branch. 2. The use of string.find(expected text) seems a bit fragile. Couldn't this matching be done in a bit stricter way? I am not sure what you mean by stricter. I changed it to startswith() anyway, which is equivalent to what I did, but clearer. find() reports position of first occurence (-1 on error). I was testing that the position was == 0. 3. It might be overkill, and surely adds some code, but I would pass the port to listen on, control port socket path and authentication cookie path as command-line arguments (preferably named, not positional). Just to make it easier for others to reuse this piece of code, and increase collaboration between similar projects. A quick research showed up that this can probably be done with only a few lines of code if using the modules optparse or argparse. Python 2.6 (which Tails uses) only has optparse. In python 2.7 optparse is deprecated and replaced by argparse. This would mean writing one version for squeeze and one for wheezy. I don't think it is worth it currently. Other than this, I was (once again!) impressed by the quality of this submission. I've merged it into our experimental branch, and pushed to our master repository + to our Jenkins auto-builder. Thanks. (I did not use Whonix existing implementation, because I felt very uneasy about running a potentially security critical application as a shell script. Fully agreed. I suggest you point the Whonix developers to your own implementation, once it's merged. Ok. ___ tails-dev mailing list tails-dev@boum.org https://mailman.boum.org/listinfo/tails-dev
[Tails-dev] Made New Identity work again, please review
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). The design documentation is also updated, and the issue removed from known issues. Well tested during development, both in normal operation and failure conditions (real and faked). Also tested by patching a Tails 0.22.1 installation, and ensuring it works live (circuits gets replaced as shown in Vidalia's network map). 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. (I did not use Whonix existing implementation, because I felt very uneasy about running a potentially security critical application as a shell script. Also, it required dependencies not currently in Tails, it seems, and I have not the knowledge to review shell scripts from a security point of view.) ___ tails-dev mailing list tails-dev@boum.org https://mailman.boum.org/listinfo/tails-dev
Re: [Tails-dev] Made New Identity work again, please review
Hi, winterfa...@riseup.net wrote (11 Feb 2014 18:14:18 GMT) : 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. Congrats! This usability regression was one of the top mentionned ones in my experience when doing user support in the last few months. I won't be doing the entire review'n'merge process, but I had a quick look, and here are a few quick notes: 1. I'm not entirely convinced by the idea of running tor-controlport-filter as the vidalia user. Granted, it's the shortest path, but it feels weird, and having a bit more privilege separation would be good. 2. The use of string.find(expected text) seems a bit fragile. Couldn't this matching be done in a bit stricter way? 3. It might be overkill, and surely adds some code, but I would pass the port to listen on, control port socket path and authentication cookie path as command-line arguments (preferably named, not positional). Just to make it easier for others to reuse this piece of code, and increase collaboration between similar projects. Other than this, I was (once again!) impressed by the quality of this submission. I've merged it into our experimental branch, and pushed to our master repository + to our Jenkins auto-builder. anonym: don't consider this as a full review, I only had a quick look, really, and did not test this at all. (I did not use Whonix existing implementation, because I felt very uneasy about running a potentially security critical application as a shell script. Fully agreed. I suggest you point the Whonix developers to your own implementation, once it's merged. Cheers, -- intrigeri | GnuPG key @ https://gaffer.ptitcanardnoir.org/intrigeri/intrigeri.asc | OTR fingerprint @ https://gaffer.ptitcanardnoir.org/intrigeri/otr.asc ___ tails-dev mailing list tails-dev@boum.org https://mailman.boum.org/listinfo/tails-dev
