Tails

#2 Updated by Anonymous 2014-09-03 13:07:21

• Type of work changed from Discuss to Wait

#3 Updated by Anonymous 2014-09-03 13:16:41

• blocked by Feature #7873: Audit OnionShare added

#5 Updated by sajolida 2014-09-04 04:16:53

• Subject changed from Include OnionShare in Tails? to Include OnionShare in Tails
• Status changed from New to Confirmed

#6 Updated by sajolida 2014-09-04 05:02:52

• Subject changed from Include OnionShare in Tails to Include OnionShare

#7 Updated by sajolida 2014-09-09 12:42:22

• related to Feature #7879: Document how to serve files over HTTP behind a Tor Hidden Service added

#13 Updated by intrigeri 2016-02-28 13:05:59

• related to Feature #6742: Make tor-controlport-filter reusable added

#17 Updated by sajolida 2016-03-07 14:03:56

• related to Feature #6788: Use stem in the filtering proxy for the Tor control port added

#18 Updated by sajolida 2016-03-07 14:08:27

• Assignee set to anonym
• Target version set to Tails_2.4

Thanks for moving this forward. I marked this has related to Feature #6788 (I would have pointed you to it in Valencia if I had known it existed).

Maybe we should try to have OnionShare in Tails for 2.4 (2016-06-07 - the next major version)?

anonym: what do you think of Micah’s branch? If you like the work he did on tor-controlport-filter we should get moving on the rest of the integration: state of the Debian package, documentation, etc.

#19 Updated by sajolida 2016-03-07 14:14:58

• Status changed from Confirmed to In Progress

See related efforts going in similar directions: Feature #6742#note-11 and Feature #6742#note-13.

#21 Updated by anonym 2016-06-08 01:34:54

• Target version changed from Tails_2.4 to Tails_2.5

#22 Updated by anonym 2016-06-10 07:58:08

• Target version changed from Tails_2.5 to Tails_2.6

Let’s not install it in a point release (besides, I won’t have time until 2.5 any way). Sorry! :(

#23 Updated by sajolida 2016-06-23 03:40:37

• related to Feature #11542: Evaluate using roflcoptor as Tor control port filter added

#24 Updated by intrigeri 2016-08-31 04:18:16

• QA Check set to Ready for QA
• Type of work changed from Wait to Code

#25 Updated by anonym 2016-09-01 08:35:56

• Assignee changed from anonym to micahflee
• QA Check changed from Ready for QA to Dev Needed

Part of what has been blocking me here is that every time I’ve looked into this I get really upset about our tor-controlport-filter. As can be seen in your changes to it, you have to go quite deep into the Control Port language, partially implementing various parts of it; really, we should just be able to make a list of patterns sent by clients that will be proxied transparently between the client and the ControlPort, without the filter knowing anything about the protocol semantics. This is what Whonix’ control-port-filter-python (Feature #6742#note-11) and Subgraph’s roflcoptor (Feature #11542) do, and we should migrate to one of them (IIRC there are even other alternatives).

Any way, to not block this further, now that the hard part is done by you, I guess we can extend our own crappy filter for the time being. However, while I was testing onionshare 0.9 in a Tails built from your branch, I noticed that we don’t let requests like these through:

GETCONF HiddenServiceDir
SETCONF HiddenServiceDir="/tmp/tmpaefwc7hoonionshare" HiddenServicePort="80 127.0.0.1:17600"

which breaks onionshare. Micah, was this not a problem for you? If not, I wondered what has changed since then (stem update?).

Forthermore, onionshare doesn’t provide a facility to use another ControlPort than one of [9151, 9153, 9051] (see onionshare/hs.py in the sources) so I had to patch it to use Tails’ (filtered) port 9052. It’d be great if TOR_CONTROL_PORT from the environment would be added to this list (probably first), if set.

So, to sum up, what remains for us to proceed is:

• Add support for {GET,SET}CONF HiddenServiceDir in Tails’ tor-controlport-filter (or move to something else if that’s easier even on the short term)
• Make it possible to specify the Tor ControlPort to onioshare, preferably with TOR_CONTROL_PORT in the environment.
• Backport onionshare >=0.9 to Debian Jessie. It would need to contain the fix for the previous point, possibly as a patch in the Debian packaging.

Any way, Micah, if I ever meet you IRL I’ll buy you your preferred beverage as a way to ask for forgiveness for handling this pull request so poorly… :/

#39 Updated by anonym 2016-09-20 16:53:42

• Target version changed from Tails_2.6 to Tails_2.7

#40 Updated by intrigeri 2016-09-23 03:08:45

• related to Feature #11826: Evaluate using Whonix' control-port-filter-python as Tor control port filter added

#41 Updated by anonym 2016-09-23 05:31:54

• Assignee changed from micahflee to anonym
• % Done changed from 0 to 30
• Feature Branch set to feature/7870-include_onionshare

anonym wrote:
> * Add support for {GET,SET}CONF HiddenServiceDir in Tails’ tor-controlport-filter (or move to something else if that’s easier even on the short term)

Whoops! This was just because I tested with a too old version of stem (without ADD_ONION support). Nevermind, we can just install the version from jessie-backports.

However, tor-controlport-filter will need support for SETEVENT HS_DESC (and SETEVENTS to unsubscribe), which I’ve added support for.

> * Make it possible to specify the Tor ControlPort to onioshare, preferably with TOR_CONTROL_PORT in the environment.

I’ve written a patch which I’ll try to upstream.

> * Backport onionshare >=0.9 to Debian Jessie. It would need to contain the fix for the previous point, possibly as a patch in the Debian packaging.

For now we can install from Debian Stretch.

#42 Updated by anonym 2016-09-23 13:22:56

• Assignee deleted (anonym)
• Target version changed from Tails_2.7 to Tails_2.9.1
• % Done changed from 30 to 50
• QA Check changed from Dev Needed to Ready for QA

I’ve sent a pull request to onionshare for the patch I mentioned above, but since I apply it to this branch we already have it. Try it yourself: http://nightly.tails.boum.org/build_Tails_ISO_feature-7870-include-onionshare/lastSuccessful/archive/build-artifacts/

So, now tor-controlport-filter can handle multiple concurrent sessions. I know we weren’t gonna waste time on that (and instead look at roflcoptor or similar) but it was literally a five minute work. I spent a lot more time trying (WIP branch: feature/7870-include_onionshare-oblivious) to simplify the filter by making the it oblivious to the control port language, e.g. it would just proxy the raw commands between clients and the real control port. I’m not at all happy with the current stem-integration since it parses (regex + split style) the subset of the language we allow, and digs pretty deep into internals, finding the equivalent stem functionality etc. Extending it is a pain, while it should only be to say “allow this pattern”. If some one can find out in my WIP branch why e.g. onionshare doesn’t like the response the oblivious filter gives to e.g. GETINFO version I’d be really happy (hint: I think the problem should be around the only use of controller.msg).

Any way, I think we could include the current state in the next Tails major release.

#44 Updated by intrigeri 2016-09-24 01:15:41

• Assignee deleted (None)

u, do you want to have a look at the branch and test it? I’m happy to do another code review pass afterwards.

#49 Updated by intrigeri 2016-09-30 01:34:17

• Assignee set to anonym
• QA Check changed from Ready for QA to Info Needed

> * Separate filter data from the script into per-“process” (the /proc/$PID/cmdline, really) filters, for more fine-grained control. This radically limits how the filtered control port can be used by users with access to it, which makes Bug #9365 less of an issue.

Wait: are we trusting /proc/$PID/cmdline’s content to make security (control port filtering) decisions? IIRC any process can change its own cmdline as it wishes (and it might be that other processes run by the same user can as well).

> Sorry for the NIH syndrome, roflcoptor, but once you are properly packaged in Debian I think we should reconsider.

Nitpicking: Whonix’ control port filter seems closer to something we could ship soonish than roflcoptor.

> intrigeri, could you look at commit:2b76c549634a79b34229951a58837dc8b6fb6407 and tell me if you know some way we can continue running the filter as a non-root user with limited capabilities given the new requirement?

I don’t understand how your question relates to that commit. Can you please clarify?

#50 Updated by anonym 2016-09-30 04:41:29

• Assignee changed from anonym to intrigeri

intrigeri wrote:
> > * Separate filter data from the script into per-“process” (the /proc/$PID/cmdline, really) filters, for more fine-grained control. This radically limits how the filtered control port can be used by users with access to it, which makes Bug #9365 less of an issue.
>
> Wait: are we trusting /proc/$PID/cmdline’s content to make security (control port filtering) decisions? IIRC any process can change its own cmdline as it wishes (and it might be that other processes run by the same user can as well).

Oh shit, this is news to me (just verified with exec -a "pwned" sh -c 'cat /proc/$$/cmdline') and a reminder to no trust random comments on the Internet. Bleh.

Since /proc/$PID/exe is not very helpful for interpreted programs, unless there’s some other way to achieve what I want there I guess I’ll just revert all that so only a single filter is exposed (to any process for users which are allowed to connect to the filter). Then, of course, Bug #9365 is an issue again, and I’m not so sure if we want the per-tab circuit view enabled in Tor Browser, and probably we’d want to run a second instance of the filter only accessible to the onioncitcuits user (which we’d reintroduce). Dammit!

So, any idea how to salvage this? Some inspiration from AppArmor, which must have had to tackle similar issues.

> > Sorry for the NIH syndrome, roflcoptor, but once you are properly packaged in Debian I think we should reconsider.
>
> Nitpicking: Whonix’ control port filter seems closer to something we could ship soonish than roflcoptor.

Not really; control-port-filter-python doesn’t support subscribing to events (SETEVENTS).

> > intrigeri, could you look at commit:2b76c549634a79b34229951a58837dc8b6fb6407 and tell me if you know some way we can continue running the filter as a non-root user with limited capabilities given the new requirement?
>
> I don’t understand how your question relates to that commit. Can you please clarify?

Sorry, I copied the commit id of the commit above (in the branche’s log). I meant commit:c78a6e7c741fee043e0c2e3a54f718bc47193062. This is no longer an issue, though, if we will revert the /proc/$PID/cmdline thing, since that’s what makes it need root privileges.

#52 Updated by intrigeri 2016-09-30 05:13:43

• Assignee changed from intrigeri to anonym

> So, any idea how to salvage this? Some inspiration from AppArmor, which must have had to tackle similar issues.

AppArmor is cheating a bit: it deals with that kind of problems using code in the Linux kernel.

#54 Updated by anonym 2016-10-03 18:35:07

• QA Check changed from Info Needed to Dev Needed

intrigeri wrote:
> > So, any idea how to salvage this? Some inspiration from AppArmor, which must have had to tackle similar issues.
>
> AppArmor is cheating a bit: it deals with that kind of problems using code in the Linux kernel.

Right, and I guess it does so in a manner that is not reusable for this purpose.

So, without per-application filtering it makes a bigger difference again of whether we enable Tor Browser’s per-tab circuit view, or whether onioncircuits is run by a separate user than amnesia. So we have these options:

• Status quo: no per-tab circuit view, onioncircuits is run by a separate user. We run two instances of tor-controlport-filter, for these two users:
  • amnesia: only allows signal newnym and the new stuff needed by onionshare, i.e. nothing about Tor’s stream/circuit state.
  • onioncircuits: full access to Tor’s stream/circuit state via getconf {circuit,stream}-status.
• Disregarding Bug #9365: we run one instance of tor-controlport-filter exposed to the amnesia user, which runs onioncircuits itself. So the filter exposes the full Tor stream/circuit state to the amnesia user.

I guess I am leaning towards the status quo, mostly because Bug #9365 isn’t resolved and I don’t want to make things worse. Hence that’s also the approach that won’t be blocked by anything, so we can get onionshare, finally. :)

#58 Updated by anonym 2016-10-04 16:35:37

• % Done changed from 50 to 60

intrigeri wrote:
> > Ooh, a apparmor outgrowth (read: hacky extension :)), I like it! I think you are on to something here, but I’ll first try to get the “status quo” merged, and then possibly deal with this separately.

In the end, I went with intrigeri’s excellent idea (which saved my ass here!) since I worry about that part being dropped if I don’t do this now, when I have the problem space in my head. So, the current state of the branch has per-application filters that is determined at the time a process connects to the server, using AppArmor’s way of identifying the executable (so it works for scripts). Yay! Thanks, intrigeri!

> > It might be a good exercise for me to write some apparmor profiles, e.g. for onioncircuits and onionshare.
>
> Yes, we definitely need more people on board who have more experience & confidence with editing our AppArmor policy.

Indeed. For now I added empty stub profiles in complain mode (which is good enough for tor-controlport-filter) although this will spam the journal significantly. So unless there is a way to make a specific profile in complain mode silent, we’re now blocked by the lack of profiles for onionshare and… onioncircuits! :)

> > What I thought of was that tor-controlport-filter could run aa-status (although I’ll look into how to get the eq. info from the python3-apparmor module) look for the PID, and see which (enforced!) profile the PID is using. Maybe that is good enough?
>
> That would be good enough, but indeed it would be nicer and cheaper to use the Python library: aa-status output is not meant to be stable and machine-readable, so we would need tests to ensure all that works fine, and writing these tests may take more time than implementing a more robust solution.

It turns out the Python library doesn’t have facilities for this, and that aa-status uses AppArmor’s sysfs interface. So in the end we parse aa-status output. I guess it’d be very simple to write an automated test that checks that at least onioncircuits is working (e.g. use Dogtail to check that the circuit list is not empty, and that the “You are not connected to Tor yet” status is not shown).

#59 Updated by anonym 2016-10-06 20:25:26

• Assignee deleted (anonym)
• % Done changed from 60 to 70
• QA Check changed from Dev Needed to Info Needed

intrigeri, on tails-dev I said that “aa-status parses /sys/kernel/security/apparmor/profiles”, to which you asked

(13:51:48) intrigeri: anonym: parses that, and then? does it compare to
/proc/$PID/exe? if yes, then this has exactly the same problem as the
previous approach, no?

While it doesn’t do what you asked, it any way turned out I was mistaken — /sys/kernel/security/apparmor/profiles is parsed only when determining which profiles are loaded, but to find out which processes have which profiles loaded, aa-status parses all PIDs /proc/$PID/attr/current (example: for onionshare it contains something like /usr/bin/onionshare (complain)). Getting a PID → AppArmor profile (if any) mapping is safe with that approach. However, parsing aa-status’ output is far more complex than just reading that file, so I’ve switched to that approach.

Furthermore, I have:

• Fixed #6472
• Made the tor-controlport-filter listen on 9051, and the “real” one (which is rarely used) on 9052, which simplifies client configuration
• Make tor-launcher use tor-controlport-filter — now I believe tor.sh from Tails’ shell library might be the only client to the real ControlPort (except the filtered one)
• Extended the filter matching to also match based on the client’s user; the rationale is that e.g. tor-launcher uses the unconfined firefox executable, so locking it down further to fit the exact usecase we have in mind seems like a good idea. Now we could (in our firewall configuration) actually allow all users to connect to the filtered control port if we so wish.

I guess it seems like I’m overdoing this, but I hope brushing up my Python will help when reviewing Feature #11753, and segfault’s USB installation verifier and Tails Server.

Also, I spent yesterday evening learning a bit about handwriting AppArmor profiles (which I think is a beneficial approach for being able to review AppArmor stuff with more insight) and just pushed this commit:

commit cfb29d6f52f37d21e7e64caa5c7b4f36172060e4
Author: anonym <anonym@riseup.net>
Date:   Wed Oct 5 20:27:41 2016 +0200

    WIP: AppArmor profiles for onioncircuits and onionshare{,-gui}.

    They're still in complain mode, cause enforcing them breaks the
    applications (when they read from tor-controlport-filter's socket, they
    get empty data and fail). And there are still some audit log entries
    generated for onionshare-gui, for reasons I do not yet understand. There
    are a few other questions in comments.

    However, this is actually solving the blocking issue we have with the
    stub AppArmor profiles flooding the journal. Fixing these AppArmor
    profiles so we then can enable them is more like a bonus.

So, this branch could be merged now, technically, so I’m at least gonna ask for a review. But I am in need of some AppArmor expertise to get to a point where these profiles can be enforced. u, do you have the possibility to look at my AppArmor profiles before the end of October (so 2016-10-31)? Otherwise, someone else? If not, I guess we just ship them in complain mode.

#64 Updated by Anonymous 2016-10-27 08:30:28

• Assignee set to anonym
• QA Check deleted (Info Needed)

I’m unsure why this ticket is marked as “info needed” and assigned to me. I’m tentatively reassigning to anonym -> please tell me if something is expected from me on this ticket.

And as a further question, do you think we should / could upstream these AppArmor profiles? Should be quite easy and I can take care of it if you think it’s a cool thing. (reclaimer: I did not try the branch nor the profiles yet.)

#66 Updated by anonym 2016-10-27 09:57:11

• QA Check set to Dev Needed

u wrote:
> Ah now I see: " u, do you have the possibility to look at my AppArmor profiles before the end of October (so 2016-10-31)? Otherwise, someone else? If not, I guess we just ship them in complain mode." -> I can’t manage this no, sorry. November or ask somebody else?

Ack. Then let’s leave the enforcement of these profiles for later (i.e. not blockers).

I’ve noticed an issue: onionshare depends on torbrowser-launcher, so the latter is installed, which we absolutely do not want. So yet another equivs workaround, I suppose.

#68 Updated by anonym 2016-10-27 14:14:24

• QA Check deleted (Dev Needed)

u wrote:
> anonym wrote:
> > I’ve noticed an issue: onionshare depends on torbrowser-launcher, so the latter is installed, which we absolutely do not want. So yet another equivs workaround, I suppose.

Fixed now! I’m not sure, but I guess this branch might be ready for a review’n’merge now.

> Correct, because in Debian, as onionshare is not usable over system tor, in order to make it work, one absolutely needs torbrowser-launcher. I need to check if this dependency can already be dropped or not in the package.

It will work with a system tor instance as long as the user running onionshare has access to Tor’s control port (e.g. is a member of the debian-tor group). So a tor | torbrowser-launcher seems appropriate. I think it will even work with the old version of tor that’s in Jessie, but I’m not sure.

#75 Updated by anonym 2016-11-12 16:17:19

• Assignee changed from anonym to intrigeri
• QA Check set to Ready for QA

Let me summarize all comments above to make the review of this branch easier:

• Don’t bother looking at the git log — this branch has over a hundred commits, and in the end we have essentially a rewrite of the filter. I suggest looking at the git diff, resorting to git log only if a clarification is needed.
• This branch also fixes: Feature #6742, Feature #6788, Feature #9001. Those tickets are updated to reflect this, but let’s keep all discussion on this ticket.
• I also just closed Feature #11542 and Feature #11826, which are related.
• The filtered control port now listens on 9051, and the unfiltered one on 9052. This way we don’t have to reconfigure every application, since there’s extremely few cases where the unfiltered port is used.
• Now we separate filter data from the script into per-“executable” filters, for more fine-grained control. This radically limits how the filtered control port can be used by users with access to it, which makes Bug #9365 less of an issue — the user is limited to the control port commands that the program can generate.
• To allow the “listen port -> binary/script path” lookup (thanks to AppArmor) we ship profiles for onionshare and onioncircuits that are in complain mode.
• We now enable the per-tab circuit view in the Tor Browser. While the discussion/conclusions of Bug #9365, Bug #10339 and Bug #9366 are not very decisive, it seems we agree that there are more serious things to worry about than stream/circuit state. Also, the per-process filtering makes it harder to exploit. However, our filter makes sure that Tor Browser only receives STREAM events for its own connections (see the restrict-stream-events filter option) so I think this change is completely safe now.

GLHF!

#76 Updated by intrigeri 2016-11-16 07:38:45

• Assignee changed from intrigeri to anonym
• QA Check changed from Ready for QA to Dev Needed

> Let me summarize all comments above to make the review of this branch easier:

Thanks!

• Installing onionshare from Stretch is risky: it may stop being installable on Jessie at any time. IMO we should upload/fetch whatever version we want to/from jessie-backports. If you need sponsorship for the upload, please file a subtask and assign it to me, I’ll gladly do it.
• I like install_fake_package, thanks for the refactoring!
• The AppArmor policies introduced by this branch (OnionShare, OnionCircuits) should be upstreamed. It might not be a blocker, but before I merge this branch I would like to see a ticket for upstreaming, with a clear timeline and responsible person(s).
• We should not need things like /{,lib/live/mount/rootfs/filesystem.squashfs/} since we use aliases.
• “exceåt” feels like a typo (it’s cute though :)
• The “Allow all user data except exceåt .gnupg, .ssh and ” rules would be better replaced by something like what I’ve proposed upstream for Totem, that fits in two lines instead of 5. Also, how about using abstractions/private-files-strict?
• Any reason why the usr.bin.onionshare, usr.bin.onioncircuits and usr.bin.onionshare-gui policies are not enforced? It feels weird to have non-empty policies (hinting that they at least have seen some work, and are not there merely for the filter to identify apps) in complain mode. BTW you need to be aware that complain mode doesn’t do exactly what one generally guesses it does (some stuff still gets blocked even in complain mode).
• /usr/share/icons/Adwaita/index.theme rwk feels strange: abstractions/freedesktop.org (which is included from abstractions/gnome should be enough to give read access, and that app shouldn’t need write access.
• At least “The client listens on a control port 9051” needs to be updated in the design doc.
• I’m not sure what good the match- prefix does in match-exe-paths and match-users: it made me wonder if the corresponding value should be a regexp. Maybe just drop the prefix?
• Is Port=80,176([0-4][0-]|50) really what you mean, i.e. is Port=80,1761- valid and Port=80,17622 invalid?
• CapabilityBoundingSet= makes me sad, and “no matter how I tried to set CapabilityBoundingSet, it didn’t work” doesn’t help me help you. This script certainly doesn’t need all capabilities. It would be nice to know what you’ve tried and seen failing.
• local status should probable be local package_status instead, and then you may want to localize package_name as well.
• s/who/that/ in “who are pretty self-explanatory”
• “The format is based on YAML”: well, it is YAML, no?
• Why do “we fallback to /proc/pid/exe in case there is no AppArmor profile”? What kind of security issues does this operation mode have?
• The else corresponding to if len(matched_filters) > 0 is just one line, and way below the if (doesn’t fit on my screen). This makes the control flow hard to follow. Maybe handle the current else condition first, return, and then proceed without the extra indentation for what seems to be the default case? More generally, match_and_parse_filter feels too long to me.
• “Prase” typo
• Is it on purpose that we only process the first filter in matched_filters?
• handle_controlport_session is way too long. Its many nested functions raise the “shouldn’t it be a class instead?” flag in my head.
• s/unsubscribed to/unsubscribed from/
• Maybe use lambda instead of generating temporary named functions merely to pass them to another function?
• Can line = str(binary_line, 'ascii') be used by a client to make the control port filter crash, e.g. by passing data that can’t be decoded to ascii?
• cmd, cmd_arg_sep, arg_str = match.groups(): using named capture groups in the regexps might make this nicer.
• s/Stem call/Stem calls/
• A dispatch table might help make the set of if cmd == "PROTOCOLINFO" / elif nicer to follow.
• In except (yaml.parser.ParserError, yaml.scanner.ScannerError) it would be nice to log the actual error.
• In format strings like '{} (pid: {}, user: {}, port: {}, filter: {})'.format( that have many placeholders, it would be nice to use named placeholders instead of positional ones.
• The design doc pretends that the tor-launcher user has access to the control port, which seems to be obsolete.
• The design doc says that members of the debian-tor group are allowed access to the control port. How so?

#78 Updated by anonym 2016-11-24 21:07:37

• File onionshare_0.9.1-1-bpo8+1.gitbundle added
• Assignee changed from anonym to intrigeri
• QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:
> > Let me summarize all comments above to make the review of this branch easier:
>
> Thanks!

And thank you for the review!

> * Installing onionshare from Stretch is risky: it may stop being installable on Jessie at any time. IMO we should upload/fetch whatever version we want to/from jessie-backports. If you need sponsorship for the upload, please file a subtask and assign it to me, I’ll gladly do it.

I’ve built and uploaded a package to the branch’s APT suite. I’ve attached a Git bundle against onionshare Debian package Git repo’s jessie-backports branch. Is that all you need?

I’ve enabled the APT suite and fixed the APT pinnings too, of course.

> * I like install_fake_package, thanks for the refactoring!

:)

> * The AppArmor policies introduced by this branch (OnionShare, OnionCircuits) should be upstreamed. It might not be a blocker, but before I merge this branch I would like to see a ticket for upstreaming, with a clear timeline and responsible person(s).

I don’t think it should be blocking, and u takes care of this, so I’m considering this concern of yours dealt with.

> * We should not need things like /{,lib/live/mount/rootfs/filesystem.squashfs/} since we use aliases.

Fixed.

> * “exceåt” feels like a typo (it’s cute though :)

… :S

> * The “Allow all user data except exceåt .gnupg, .ssh and ” rules would be better replaced by something like what I’ve proposed upstream for Totem, that fits in two lines instead of 5. Also, how about using abstractions/private-files-strict?

Fixed.

> * Any reason why the usr.bin.onionshare, usr.bin.onioncircuits and usr.bin.onionshare-gui policies are not enforced? It feels weird to have non-empty policies (hinting that they at least have seen some work, and are not there merely for the filter to identify apps) in complain mode.

They’re not enforced because they break functionality, and they are non-empty so that the journal isn’t flooded with audit entries.

> BTW you need to be aware that complain mode doesn’t do exactly what one generally guesses it does (some stuff still gets blocked even in complain mode).

Wow, ok. Everything I’ve tried seems to work, though. How would that work with an “empty” profile in complain mode? Or a “permit everything” (how would that look?) profile in enforce mode?

IMHO, our requirement for this branch to be merged is to have an AppArmor profile for each of these scripts, and it must be quiet enough to not flood the journal. I don’t really care how we achieve that, but I’m unsure how, but I’d happily accept a suggestion. What about an enforced profile that explicitly accepts everything?

Beyond that, let’s not revist these profiles until when we choose to enforce them. Let’s iterate!

> * /usr/share/icons/Adwaita/index.theme rwk feels strange: abstractions/freedesktop.org (which is included from abstractions/gnome should be enough to give read access, and that app shouldn’t need write access.

It doesn’t set the `k` (lock) permission:

Nov 24 15:23:44 amnesia kernel: audit: type=1400 audit(1480001024.060:37): apparmor="DENIED" operation="file_lock" profile="/usr/bin/onionshare-gui" name="/usr/share/icons/Adwaita/index.theme" pid=3468 comm="onionshare-gui" requested_mask="k" denied_mask="k" fsuid=1000 ouid=0

So we need something if we want to suppress that message from the journal.

> * At least “The client listens on a control port 9051” needs to be updated in the design doc.

Fixed.

> * I’m not sure what good the match- prefix does in match-exe-paths and match-users: it made me wonder if the corresponding value should be a regexp. Maybe just drop the prefix?

I still want the key to make it clear that it’s a qualifier for the rules, and I think it’s clearer of all of them share some prefix. What about allowed-, then?

> * Is Port=80,176([0-4][0-]|50) really what you mean, i.e. is Port=80,1761- valid and Port=80,17622 invalid?

That’s a typo. Fixed.

> * CapabilityBoundingSet= makes me sad, and “no matter how I tried to set CapabilityBoundingSet, it didn’t work” doesn’t help me help you. This script certainly doesn’t need all capabilities. It would be nice to know what you’ve tried and seen failing.

Ok, after brute-forcing (again, not sure what went wrong last time) I got:

CapabilityBoundingSet=CAP_DAC_OVERRIDE CAP_NET_BIND_SERVICE CAP_SYS_PTRACE

Yay! It looks quite bad, though (e.g. “Bypass file read, write, and execute permission checks” and “transfer data to or from the memory of arbitrary processes”).

> * local status should probable be local package_status instead, and then you may want to localize package_name as well.

Fixed.

> * s/who/that/ in “who are pretty self-explanatory”

Fixed.

> * “The format is based on YAML”: well, it is YAML, no?

Fixed.

> * Why do “we fallback to /proc/pid/exe in case there is no AppArmor profile”?

We want to support binaries that lack AppArmor profiles too. The AppArmor hack is only relevant for scripts vs their interpreter.

> What kind of security issues does this operation mode have?

None. First note that /proc/pid/exe is only different from what the AppArmor profile considers being the executable when running a script, so we only have to reason about that case. If we are falling back to /proc/pid/exe for a script, that means we’ll get the filter for the interpreter. Since the user can run arbitrary code in the interpreter, an interpreter with a less restricted filter would allow us to do more than the target script any way, so there’s no security issue.

> * The else corresponding to if len(matched_filters) > 0 is just one line, and way below the if (doesn’t fit on my screen). This makes the control flow hard to follow. Maybe handle the current else condition first, return, and then proceed without the extra indentation for what seems to be the default case? More generally, match_and_parse_filter feels too long to me.

Fixed. The two return:s are a bit anti-DRY now, though. Whatever.

> * “Prase” typo

Fixed.

> * Is it on purpose that we only process the first filter in matched_filters?

Yup. I tried allowing matching multiple filters (resulting in the least restrictive union of all the filters combined), but that was crazy complex so I dropped it. It’s in the Git log.

> * handle_controlport_session is way too long. Its many nested functions raise the “shouldn’t it be a class instead?” flag in my head.

Conceptually that will be the same thing, just code shuffled around a bit. I don’t think classes or OO has any worth in itself, and don’t see how it would make things any easier to conceptualize.

> * s/unsubscribed to/unsubscribed from/

Fixed.

> * Maybe use lambda instead of generating temporary named functions merely to pass them to another function?

I think local function definitions are much easier to read. What benefit do you actually see in moving to lambdas?

> * Can line = str(binary_line, 'ascii') be used by a client to make the control port filter crash, e.g. by passing data that can’t be decoded to ascii?

Perhaps sending such a char will make the client’s thread crash, but it won’t DoS any other client. So do we care about looking further into this?

> * cmd, cmd_arg_sep, arg_str = match.groups(): using named capture groups in the regexps might make this nicer.

Can you give an example?

> * s/Stem call/Stem calls/

Fixed.

> * A dispatch table might help make the set of if cmd == "PROTOCOLINFO" / elif nicer to follow.

Just to be clear: you mean that I make each if-case into a function, and then create a dict like dispatch = { 'PROTOCOLINFO': process_protocolinfo, ... } and then replace the whooe if with: dispatch(cmd)(<list of all needed local variables>? If so, well, I would consider that much less intuitive, and I’d rather not. If not, can you clarify?

> * In except (yaml.parser.ParserError, yaml.scanner.ScannerError) it would be nice to log the actual error.

Fixed.

> * In format strings like '{} (pid: {}, user: {}, port: {}, filter: {})'.format( that have many placeholders, it would be nice to use named placeholders instead of positional ones.

This is what I tried:

            client_desc = '{exe} (pid: {pid}, user: {user}, port: {port}, filter: {filter_name})'
            .format(
                exe = client_exe_path,
                pid = client_pid,
                user = client_user,
                port = self.client_address[1],
                filter_name = filter_name
            )

but it doesn’t really look like an improvement to me. The repetitions in the format string just confuses I guess. Simply putting a linebreak after each argument to .format() seems like the real improvement, if there is one. Or what do you think?

> * The design doc pretends that the tor-launcher user has access to the control port, which seems to be obsolete.

Fixed.

> * The design doc says that members of the debian-tor group are allowed access to the control port. How so?

Well, it has access to the control socket. Fixed.

Big thanks, again, for the comprehensive review!

#79 Updated by anonym 2016-11-25 13:18:43

• Target version changed from Tails_2.9.1 to Tails 2.10

#81 Updated by intrigeri 2016-12-07 09:21:00

• Affected tool set to OnionShare

#82 Updated by intrigeri 2016-12-07 10:27:06

• Assignee changed from intrigeri to anonym
• QA Check changed from Ready for QA to Dev Needed

Note that the branch had a conflict with current devel, so it didn’t build on Jenkins anymore. I’ve merged devel, resolved the conflict.

>> * Installing onionshare from Stretch is risky: it may stop being installable on Jessie at any time. IMO we should upload/fetch whatever version we want to/from jessie-backports. If you need sponsorship for the upload, please file a subtask and assign it to me, I’ll gladly do it.

> I’ve built and uploaded a package to the branch’s APT suite. I’ve attached a Git bundle against onionshare Debian package Git repo’s jessie-backports branch. Is that all you need?

Yes, thanks! I’ve created Feature #12021 to track this. I’ll assume that you’ve tested this backport on a regular Jessie system. If not, please comment on Feature #12021.

>> * The AppArmor policies introduced by this branch (OnionShare, OnionCircuits) should be upstreamed. It might not be a blocker, but before I merge this branch I would like to see a ticket for upstreaming, with a clear timeline and responsible person(s).

> I don’t think it should be blocking, and u takes care of this, so I’m considering this concern of yours dealt with.

OK. I’ve unparented Feature #11929 and Feature #11930 then.

>> * Any reason why the usr.bin.onionshare, usr.bin.onioncircuits and usr.bin.onionshare-gui policies are not enforced? It feels weird to have non-empty policies (hinting that they at least have seen some work, and are not there merely for the filter to identify apps) in complain mode.

> They’re not enforced because they break functionality, and they are non-empty so that the journal isn’t flooded with audit entries.

OK. Then how about adding a comment in the profiles themselves to make this clear? Bonus points if details about “they break functionality” are provided, this could save u some time on Feature #11929 (she’s already sent a PR with these profiles, not sure if she was aware of the breakage they can cause).

In passing I see you’ve set usr.bin.onionshare-gui to enforce mode since then. Cool! I hope it was on purpose :)

>> BTW you need to be aware that complain mode doesn’t do exactly what one generally guesses it does (some stuff still gets blocked even in complain mode).

> Wow, ok. Everything I’ve tried seems to work, though. How would that work with an “empty” profile in complain mode? Or a “permit everything” (how would that look?) profile in enforce mode?

I don’t remember the details, sorry. But if it works as-is, fine :)

> IMHO, our requirement for this branch to be merged is to have an AppArmor profile for each of these scripts, and it must be quiet enough to not flood the journal.

ACK

> I don’t really care how we achieve that, but I’m unsure how, but I’d happily accept a suggestion. What about an enforced profile that explicitly accepts everything?

We certainly could do that, but the current profiles already reach the goal you’ve set above, so there’s no need to do more work, right?

>> * /usr/share/icons/Adwaita/index.theme rwk feels strange: abstractions/freedesktop.org (which is included from abstractions/gnome should be enough to give read access, and that app shouldn’t need write access.

> It doesn’t set the `k` (lock) permission:
[…]
> So we need something if we want to suppress that message from the journal.

OK. I see you’ve replaced ‘rwk’ with ‘k’, which addresses my concern that was about write access (locking is fine).

>> * I’m not sure what good the match- prefix does in match-exe-paths and match-users: it made me wonder if the corresponding value should be a regexp. Maybe just drop the prefix?

> I still want the key to make it clear that it’s a qualifier for the rules, and I think it’s clearer of all of them share some prefix. What about allowed-, then?

I still feel that “it’s a qualifier for the rules” is already clear enough without any prefix (e.g. see how it’s done in iptables), but I won’t insist, so I’ll discuss the other possible prefixes.

By definition, everything in those YAML files as about allowing {user, executable} to do perform a set of actions, so using e.g. allowed-users would feel wrong to me. I mean, allowed-commands would look better than allowed-users (but let’s not do that either). Besides, it would suggest (given the reader thinks there’s an implicit “or”) that the specified user can do anything, and everyone else can only do what’s in commands and events. So let’s not do that.

Other prefixes that I could live with:

• apply-to-
• apply-rules-to-
• rules-for-
• target-

… or drop the prefix, and instead do something like:

- apply-to:
- exe-paths:
- '/usr/bin/onioncircuits'
- users:
- 'amnesia'
rules:
- commands:
[...]

… which clarifies the difference between what you call qualifiers, and what the rules are.

What do you think?

>> * CapabilityBoundingSet= makes me sad, and “no matter how I tried to set CapabilityBoundingSet, it didn’t work” doesn’t help me help you. This script certainly doesn’t need all capabilities. It would be nice to know what you’ve tried and seen failing.

> Ok, after brute-forcing (again, not sure what went wrong last time) I got:

 CapabilityBoundingSet=CAP_DAC_OVERRIDE CAP_NET_BIND_SERVICE CAP_SYS_PTRACE 

Yay!

That’s better. At least it doesn’t include CAP_SYS_ADMIN.

> It looks quite bad, though (e.g. “Bypass file read, write, and execute permission checks” and “transfer data to or from the memory of arbitrary processes”).

Yeah, something feels wrong. If you’re interested in going a bit further on this one:

• CAP_DAC_OVERRIDE is probably needed to access /var/run/tor/control.authcookie and /var/run/tor/control, and there’s little we can do about it without doing crazy stuff;
• I don’t understand why CAP_NET_BIND_SERVICE is needed, since we’re listening on an unprivileged port; what’s broken if you drop it?
• CAP_SYS_PTRACE feels wrong; what’s broken if you drop it?

>> * The else corresponding to if len(matched_filters) > 0 is just one line, and way below the if (doesn’t fit on my screen). This makes the control flow hard to follow. Maybe handle the current else condition first, return, and then proceed without the extra indentation for what seems to be the default case? More generally, match_and_parse_filter feels too long to me.

> Fixed.

Thanks, it’s much more readable now. “More generally, match_and_parse_filter feels too long to me” still holds, but well.

>> * Is it on purpose that we only process the first filter in matched_filters?

> Yup. I tried allowing matching multiple filters (resulting in the least restrictive union of all the filters combined), but that was crazy complex so I dropped it. It’s in the Git log.

OK. So perhaps let’s play it safe and abort if len(matched_filters) > 1, instead of merely issuing a log message?

>> * Maybe use lambda instead of generating temporary named functions merely to pass them to another function?

> I think local function definitions are much easier to read. What benefit do you actually see in moving to lambdas?

I think that’s our usual disagreement between “the disadvantages of maintaining state when not really needed” vs. “naming stuff for improved clarity” so let’s leave it at that :)

>> * Can line = str(binary_line, 'ascii') be used by a client to make the control port filter crash, e.g. by passing data that can’t be decoded to ascii?

> Perhaps sending such a char will make the client’s thread crash, but it won’t DoS any other client. So do we care about looking further into this?

To be clear, if the filter crashes, it’ll fail closed, so in practice the client will basically just have shut down its own access to the control port, right?

>> * cmd, cmd_arg_sep, arg_str = match.groups(): using named capture groups in the regexps might make this nicer.

> Can you give an example?

Sure.

Instead of:

match = re.match(r'(\S+)(\s*)([^\r\n]*)\r?\n$', line)
[… 5 lines that separate the parentheses from the naming of what they capture …]
cmd, cmd_arg_sep, arg_str = match.groups()

match = re.match(
r'(?P<cmd>\S+)(?P<cmd_arg_sep>\s*)(?P<arg_str>[^\r\n]*)\r?\n$',
line)
[…]
cmd, cmd_arg_sep, arg_str = match.group('cmd', 'cmd_arg_sep', 'arg_str')

The advantage is that when reading the regexp, one knows immediately what the captured sub-strings are, instead of having to look up further in the code (currently, 6 lines later but this has the potential to grow) to understand what the regexp is about.

Besides, positional capturing groups tend to suck just like positional args (everything depends on ordering, which can cause subtle breakage at a distance since the match object is kept, can be passed to other functions, that themselves can use e.g. match.groups in a totally different place in the code).

>> * A dispatch table might help make the set of if cmd == "PROTOCOLINFO" / elif nicer to follow.

> Just to be clear: you mean that I make each if-case into a function, and then create a dict like dispatch = { 'PROTOCOLINFO': process_protocolinfo, ... } and then replace the whooe if with: dispatch(cmd)(<list of all needed local variables>?

Yes.

> If so, well, I would consider that much less intuitive, and I’d rather not.

As you can guess I totally disagree, but I won’t insist. I can live with how it is right now. I bet I’ll raise this again in the future if that big pile of if/elif:s grows even bigger (it doesn’t fit on my screen already).

>> * In format strings like '{} (pid: {}, user: {}, port: {}, filter: {})'.format( that have many placeholders, it would be nice to use named placeholders instead of positional ones.

> This is what I tried:

>

>             client_desc = '{exe} (pid: {pid}, user: {user}, port: {port}, filter: {filter_name})'
>             .format(
>                 exe = client_exe_path,
>                 pid = client_pid,
>                 user = client_user,
>                 port = self.client_address[1],
>                 filter_name = filter_name
>             )
> 

> but it doesn’t really look like an improvement to me.

To me it definitely does, as reading the string is enough to understand what it is about, while currently I have to manually match each {} one after the other to understand what it will be replaced with.

> Simply putting a linebreak after each argument to .format() seems like the real improvement, if there is one.

It won’t address my concern at all, so only do it if it improves things for you.

Meta: I had a hard time wrapping my mind around this program, hence all my readability comments, be it about nested functions, function length, long if/elif structures, format string arguments naming, etc. I understand you dislike most of my improvement proposals in this field, which is fine: we definitely have differing tastes in this area. I don’t want to push for changes that would make the code harder for you to maintain & hack. But then let’s make it clear that we’re optimizing for you, since most of the readability issues I’ve been experiencing are left unsolved, which won’t encourage me to help with anything that must be done in the future on this codebase. Don’t get me wrong: I can totally live with that once it’s been clearly said! I mean, I’m writing Modern Perl that nobody else on the team is at ease with ;)

>> * The design doc pretends that the tor-launcher user has access to the control port, which seems to be obsolete.

> Fixed.

Thanks, but: s/to the control port run/to the control port runs/

#83 Updated by intrigeri 2016-12-08 08:39:22

• related to deleted (Feature #6742: Make tor-controlport-filter reusable)

#84 Updated by intrigeri 2016-12-08 08:39:30

• blocks Feature #6742: Make tor-controlport-filter reusable added

#85 Updated by intrigeri 2016-12-08 08:39:55

• blocks Feature #8173: Make Ricochet usable in Tails added

#86 Updated by intrigeri 2016-12-08 08:40:48

• related to deleted (Feature #6788: Use stem in the filtering proxy for the Tor control port)

#87 Updated by intrigeri 2016-12-08 08:40:54

• blocks Feature #6788: Use stem in the filtering proxy for the Tor control port added

#88 Updated by anonym 2016-12-08 20:48:30

• Assignee changed from anonym to intrigeri
• QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:
> Note that the branch had a conflict with current devel, so it didn’t build on Jenkins anymore. I’ve merged devel, resolved the conflict.

Thanks!

> >> * Installing onionshare from Stretch is risky: it may stop being installable on Jessie at any time. IMO we should upload/fetch whatever version we want to/from jessie-backports. If you need sponsorship for the upload, please file a subtask and assign it to me, I’ll gladly do it.
>
> > I’ve built and uploaded a package to the branch’s APT suite. I’ve attached a Git bundle against onionshare Debian package Git repo’s jessie-backports branch. Is that all you need?
>
> Yes, thanks! I’ve created Feature #12021 to track this. I’ll assume that you’ve tested this backport on a regular Jessie system. If not, please comment on Feature #12021.

So the backport is now uploaded, and I’ve adjusted the branch to install it from jessie-backports.

> [ … AppArmor profiles … ]

Apparently me and u fixed them enough so they do not break anything if enforced. So they are now all enforced. Yay! I think we can leave this topic for now — surely we will look at them again in detail when we upstream them.

> >> * I’m not sure what good the match- prefix does in match-exe-paths and match-users: it made me wonder if the corresponding value should be a regexp. Maybe just drop the prefix?
>
> > I still want the key to make it clear that it’s a qualifier for the rules, and I think it’s clearer of all of them share some prefix. What about allowed-, then?
>
> I still feel that “it’s a qualifier for the rules” is already clear enough without any prefix (e.g. see how it’s done in iptables), but I won’t insist, so I’ll discuss the other possible prefixes.
>
> By definition, everything in those YAML files as about allowing {user, executable} to do perform a set of actions, so using e.g. allowed-users would feel wrong to me. I mean, allowed-commands would look better than allowed-users (but let’s not do that either). Besides, it would suggest (given the reader thinks there’s an implicit “or”) that the specified user can do anything, and everyone else can only do what’s in commands and events. So let’s not do that.
>
> Other prefixes that I could live with:
>
> * apply-to-
> * apply-rules-to-
> * rules-for-
> * target-
>
> … or drop the prefix, and instead do something like:
>
> […]
>
> … which clarifies the difference between what you call qualifiers, and what the rules are.
>
> What do you think?

In the end I just dropped the prefix.

> * I don’t understand why CAP_NET_BIND_SERVICE is needed, since we’re listening on an unprivileged port; what’s broken if you drop it?

Interesting. It shouldn’t be there. Fixed!

> * CAP_SYS_PTRACE feels wrong; what’s broken if you drop it?

The thread silently dies when it calls exe_path_of_pid(), which tries to read /proc/{pid}/attr/current, which belongs to AppArmor, and given its dependency on ptrace, I guess it makes sense.

> Thanks, it’s much more readable now. “More generally, match_and_parse_filter feels too long to me” still holds, but well.

I refactored it some more.

> >> * Is it on purpose that we only process the first filter in matched_filters?
>
> > Yup. I tried allowing matching multiple filters (resulting in the least restrictive union of all the filters combined), but that was crazy complex so I dropped it. It’s in the Git log.
>
> OK. So perhaps let’s play it safe and abort if len(matched_filters) > 1, instead of merely issuing a log message?

True! Fail safe!

> >> * Can line = str(binary_line, 'ascii') be used by a client to make the control port filter crash, e.g. by passing data that can’t be decoded to ascii?
>
> > Perhaps sending such a char will make the client’s thread crash, but it won’t DoS any other client. So do we care about looking further into this?
>
> To be clear, if the filter crashes, it’ll fail closed, so in practice the client will basically just have shut down its own access to the control port, right?

Correct.

> >> * cmd, cmd_arg_sep, arg_str = match.groups(): using named capture groups in the regexps might make this nicer.
>
> > Can you give an example?
>
> Sure.
>
> Instead of:
>
> […]
>
> […]
>
> The advantage is that when reading the regexp, one knows immediately what the captured sub-strings are, instead of having to look up further in the code (currently, 6 lines later but this has the potential to grow) to understand what the regexp is about.
>
> Besides, positional capturing groups tend to suck just like positional args (everything depends on ordering, which can cause subtle breakage at a distance since the match object is kept, can be passed to other functions, that themselves can use e.g. match.groups in a totally different place in the code).

Fixed! Thanks for the example!

> >> * A dispatch table might help make the set of if cmd == "PROTOCOLINFO" / elif nicer to follow.
>
> > Just to be clear: you mean that I make each if-case into a function, and then create a dict like dispatch = { 'PROTOCOLINFO': process_protocolinfo, ... } and then replace the whooe if with: dispatch(cmd)(<list of all needed local variables>?
>
> Yes.
>
> > If so, well, I would consider that much less intuitive, and I’d rather not.
>
> As you can guess I totally disagree, but I won’t insist. I can live with how it is right now. I bet I’ll raise this again in the future if that big pile of if/elif:s grows even bigger (it doesn’t fit on my screen already).

Got it. If you really want to, I can refactor the if-blocks into functions.

> >> * In format strings like '{} (pid: {}, user: {}, port: {}, filter: {})'.format( that have many placeholders, it would be nice to use named placeholders instead of positional ones.
>
> > This is what I tried:
>
> > […]
>
> > but it doesn’t really look like an improvement to me.
>
> To me it definitely does, as reading the string is enough to understand what it is about, while currently I have to manually match each {} one after the other to understand what it will be replaced with.
>
> > Simply putting a linebreak after each argument to .format() seems like the real improvement, if there is one.
>
> It won’t address my concern at all, so only do it if it improves things for you.

In the end I did what you suggested. IMHO it makes things neither worse nor better, but if it is an improvement for you, then it’s worth it.

> Meta: I had a hard time wrapping my mind around this program, hence all my readability comments, be it about nested functions, function length, long if/elif structures, format string arguments naming, etc. I understand you dislike most of my improvement proposals in this field, which is fine: we definitely have differing tastes in this area. I don’t want to push for changes that would make the code harder for you to maintain & hack. But then let’s make it clear that we’re optimizing for you, since most of the readability issues I’ve been experiencing are left unsolved, which won’t encourage me to help with anything that must be done in the future on this codebase. Don’t get me wrong: I can totally live with that once it’s been clearly said! I mean, I’m writing Modern Perl that nobody else on the team is at ease with ;)

Got it. I did make an attempt at turning the handle_controlport_session (which is the messiest part, with all those nested functions) into a class, which I hope you’ll like. But, yeah, I hate OOP and have never spent any time trying to learn to do it properly, so I’m not overly optimistic. Input is welcome! I hope I got all those trillion self:s right. :)

> >> * The design doc pretends that the tor-launcher user has access to the control port, which seems to be obsolete.
>
> > Fixed.
>
> Thanks, but: s/to the control port run/to the control port runs/

Fixed.

#90 Updated by intrigeri 2016-12-11 10:12:37

• Assignee changed from intrigeri to anonym
• QA Check changed from Ready for QA to Dev Needed

> Apparently me and u fixed them enough so they do not break anything if enforced. So they are now all enforced. Yay!

Excellent :)

> I think we can leave this topic for now — surely we will look at them again in detail when we upstream them.

Sure.

>> Thanks, it’s much more readable now. “More generally, match_and_parse_filter feels too long to me” still holds, but well.

> I refactored it some more.

Thanks!

> In the end I did what you suggested. IMHO it makes things neither worse nor better, but if it is an improvement for you, then it’s worth it.

Thanks!

> Got it. I did make an attempt at turning the handle_controlport_session (which is the messiest part, with all those nested functions) into a class, which I hope you’ll like. But, yeah, I hate OOP and have never spent any time trying to learn to do it properly, so I’m not overly optimistic. Input is welcome! I hope I got all those trillion self:s right. :)

I like it, thanks! I wouldn’t mind a 1-3 lines docstring for each class, that would sum up the scope of its responsibilities.

This kind of stuff:

self.allowed_commands = handler.allowed_commands
self.allowed_events = handler.allowed_events
self.client_address = handler.client_address
self.client_pid = handler.client_pid
self.controller = handler.controller
self.debug_log = handler.debug_log
self.filter_name = handler.filter_name
[... etc ...]

… would be better handled by explicitly using a method delegation mechanism, if there’s any such thing available in the Python OO world. This would be more declarative, DRYer, and gives a better place to adjust things in case the delegated-to class changes its interface :) E.g. in Modern Perl I would use https://metacpan.org/pod/distribution/Moose/lib/Moose/Manual/Delegation.pod. If that’s not easily done in Python, forget it.

self.handler is defined but not used in FilteredControlPortProxySession.

I’ve fixed (untested, sorry!) most style issues raised by flake8, and left a few ones that I don’t see how to fix without making things even worse. But it tells me “F821 undefined name ‘line’” for self.filter_line(line) and that’s not about style so I’ll let you handle it.

I think those few small things are all that’s left to do before I actually try an ISO built from the branch :)

#91 Updated by micahflee 2016-12-29 15:45:57

• File onionshare-settings.png added

Hello Tails people. Sorry I’ve been so absent from this conversation, but I’m incredibly excited about all of this progress, and I can’t wait for OnionShare to be in Tails by default.

We’ve already made OnionShare choose the Tor control port based on the TOR_CONTROL_PORT environment variable, if it’s there. I’ve been working on adding support for system tor with different configurations, and I realized that the TOR_CONTROL_PORT environment variable on a normal system (like Debian or Ubuntu) interferes with launching Tor Browser. And using environment variables for other settings, like the control port authentication password, is clunky and not the most user-friendly. So I’m actively working on a new feature of OnionShare that will allow users to configure exactly how it should connect to Tor (see the attached screenshot). It will store these settings in a json file in ~/.config/onionshare/onionshare.json, and both onionshare and onionshare-gui will use it. So it should be trivial to just ship Tails with the proper json file, in the future.

But in the meantime, I know that Tails is relying on the TOR_CONTROL_PORT environment variable, so I’m wondering the best way to handle this with minimal issues. If the settings file doesn’t exist, OnionShare will choose the default “Attempt automatic configuration with Tor Browser” option, which is how it currently works. In automatic mode, maybe it should continue to look out for TOR_CONTROL_PORT. But if the settings file exists, then maybe it should ignore TOR_CONTROL_PORT and just use the values in the settings file. Does this seem reasonable?

Also, another new (advanced) feature that I’m excited about is support for stealth onion services. After you create the onion service you’ll be able to copy both the OnionShare URL and the HidServAuth line to send to someone. This feature requires tor >= 0.2.9.1-alpha and python3-stem >= 1.5.0. Will Tails ship with new enough tor and python3-stem packages to support it?

#97 Updated by anonym 2017-01-10 11:47:48

• Assignee changed from anonym to intrigeri
• QA Check changed from Dev Needed to Ready for QA

Everything has been fixed now, and my manual testing has been successful. I have no automated test suite results, but you’ll have it tomorrow. Please review’n’merge (also: Feature #6742, Feature #6788 and Feature #9001).

#100 Updated by intrigeri 2017-01-11 10:16:16

• Assignee changed from intrigeri to anonym
• QA Check changed from Ready for QA to Info Needed

Manual testing went fine (modulo one UX problem I’ve reported upstream). I’m almost ready to merge but I’ve seen a test suite failure with the onionshare branch merged: “Tor Browser should use SocksPort 9150 but was seen connecting to 127.0.0.1:9051”. It’s somewhat expected given how the test is implemented, but I don’t know why we’ve never seen that before (Tor Browser connecting to the filtered control port), so it feels suspicious. Maybe it’s simply that the old control port filter was faster so we never noticed connections to it with our netstat+sleep loop?

If you are confident that’s a regression that only affects the test suite and isn’t suggesting there’s a more serious problem, then please file a ticket to fix the test suite for 2.10 final, and I will merge this branch despite the test suite regression.

#101 Updated by anonym 2017-01-11 12:04:55

• blocks Bug #12130: Tor Browser stream isolation test fails to that the control port is allowed added

#102 Updated by anonym 2017-01-11 12:11:04

• Assignee changed from anonym to intrigeri
• QA Check changed from Info Needed to Ready for QA

intrigeri wrote:
> Manual testing went fine

Yay!

> (modulo one UX problem I’ve reported upstream).

Thanks! Link?

> I’m almost ready to merge but I’ve seen a test suite failure with the onionshare branch merged: “Tor Browser should use SocksPort 9150 but was seen connecting to 127.0.0.1:9051”. It’s somewhat expected given how the test is implemented, but I don’t know why we’ve never seen that before (Tor Browser connecting to the filtered control port), so it feels suspicious. Maybe it’s simply that the old control port filter was faster so we never noticed connections to it with our netstat+sleep loop?

Previously the control port filter would block the subscription to the stream events etc which Tor Browser initiates on start, so there would never be a connection to the control port. Except for a short while when issuing a NEWNYM, but we don’t do that during the stream isolation tests. Good catch, nonetheless! I admit to only have relied on Jenkins (no @fragile) results.

> If you are confident that’s a regression that only affects the test suite and isn’t suggesting there’s a more serious problem, then please file a ticket to fix the test suite for 2.10 final, and I will merge this branch despite the test suite regression.

I’m confident, but I still want this fixed ASAP, so => Bug #12130.

#103 Updated by intrigeri 2017-01-11 12:14:03

• Status changed from In Progress to Fix committed
• % Done changed from 50 to 100

Applied in changeset commit:1a3c62b96edd337cacd6e6b6d2c9eed1951784a8.

#104 Updated by intrigeri 2017-01-11 12:17:34

• Assignee deleted (intrigeri)
• QA Check changed from Ready for QA to Pass

#106 Updated by anonym 2017-01-24 20:45:58

• Status changed from Fix committed to Resolved