Project

General

Profile

Feature #7870

Include OnionShare

Added by u over 4 years ago. Updated over 2 years ago.

Status:
Resolved
Priority:
Normal
Assignee:
-
Category:
-
Target version:
Start date:
12/07/2016
Due date:
% Done:

100%

QA Check:
Pass
Feature Branch:
feature/7870-include_onionshare
Type of work:
Code
Blueprint:
Starter:
Affected tool:
OnionShare

Description

OnionShare is a tool that lets people very easily set up a temporary Tor Hidden service to share files of any size. They would host the file on their computer and share a .onion URL generated by OnionShare with one or several persons who can then, simply by using the Tor Browser on their machine, download this file. Traffic is end-to-end encrypted and never leaves the Tor network.

OnionShare is written in Python and sets up a local webserver for the time being (using Flask/Werkzeug). It provides CLI options and a GUI.

The software should soon be in Debian (currently in NEW queue).

I think it's a great tool that would give Tails users an easy way to share files without having to use leaky third party services.
What do you think?

https://onionshare.org/

onionshare_0.9.1-1-bpo8+1.gitbundle (1.09 MB) anonym, 11/24/2016 07:59 PM

onionshare-settings.png View (37.7 KB) micahflee, 12/29/2016 03:45 PM


Subtasks

Feature #12021: Have onionshare 0.9+ in jessie-backportsResolved


Related issues

Related to Tails - Feature #7879: Document how to serve files over HTTP behind a Tor Hidden Service Rejected 09/07/2014
Related to Tails - Feature #11542: Evaluate using roflcoptor as Tor control port filter Resolved 06/23/2016
Related to Tails - Feature #11826: Evaluate using Whonix' control-port-filter-python as Tor control port filter Resolved 09/22/2016
Blocked by Tails - Feature #7873: Audit OnionShare Resolved
Blocks Tails - Feature #6742: Make tor-controlport-filter reusable Resolved 02/21/2014
Blocks Tails - Feature #8173: Make Ricochet usable in Tails In Progress 10/25/2014
Blocks Tails - Feature #6788: Use stem in the filtering proxy for the Tor control port Resolved 05/27/2014
Blocks Tails - Bug #12130: Tor Browser stream isolation test fails to that the control port is allowed Resolved 01/11/2017

Associated revisions

Revision 046aa79b (diff)
Added by anonym over 2 years ago

Install onionshare from Debian Stretch.

We'll need version >= 0.9, which isn't in jessie-backports yet.

Will-fix: #7870

Revision 6e9c1bfb (diff)
Added by anonym over 2 years ago

Patch onionshare to use the filtered Tor ControlPort.

We should upstream the patch!

Refs: #7870

Revision 83746f79 (diff)
Added by anonym over 2 years ago

Install python3-stem from jessie-backports.

We need version >= 1.4 for create_ephemeral_hidden_service()
(ADD_ONION) support, used by onionshare.

Refs: #7870

Revision e8b452b9 (diff)
Added by anonym over 2 years ago

Add support for `SETEVENTS` and `SETEVENT HS_DESC` to tor-controlport-filter.

While onionshare tries more events, subscribing and unsubscribing to
HS_DESC events is the minimum it needs to work at all.

Refs: #7870

Revision 6b76bffd (diff)
Added by anonym over 2 years ago

Make ADD_ONION calls blocking in tor-controlport-filter.

This is the KISS approach since tor-controlport-filter isn't designed to
be asynchronous, and since onionshare calls
create_ephemeral_hidden_service() with `await_publication = True` we
need this to not reach a strange state of infinite waiting.

Refs: #7870

Revision 3bc18d48 (diff)
Added by anonym over 2 years ago

Make tor-controlport-filter handle multiple sessions simultaneously.

Since stem-using applications will keep the session going, they will
prevent any other application from using the filter. So, for instance,
onionshare would block e.g. Tor Browser from sending a NEWNYM signal.

Refs: #7870

Revision 24d5e162 (diff)
Added by anonym over 2 years ago

Enable the feature-7870-include-onionshare APT overlay.

Will-Fix: #7870

Revision 738f6491 (diff)
Added by anonym over 2 years ago

Install onionshare from our APT repo.

... instead of Debian Stretch, which could end up breaking some day.

Will-fix: #7870

Revision 78f55d2b (diff)
Added by intrigeri over 2 years ago

Delete trailing white space (refs: #7870).

Revision d6879a0a (diff)
Added by intrigeri over 2 years ago

Fix most style issues raised by flake8 (refs: #7870).

Revision 1a3c62b9
Added by intrigeri over 2 years ago

Merge remote-tracking branch 'origin/feature/7870-include_onionshare' into devel

Fix-committed: #7870, #6742, #6788, #9001

Revision 530bdcfb (diff)
Added by intrigeri over 2 years ago

Drop useless feature-7870-include-onionshare APT overlay.

We now fetch onionshare from jessie-backports so we don't need
the custom backport we have in our own repo.

Refs: #7870

History

#1 Updated by BitingBird over 4 years ago

Seems possibly useful for users, but it's not good for the Tor network. So I don't know...

#2 Updated by u over 4 years ago

  • Type of work changed from Discuss to Wait

#3 Updated by u over 4 years ago

#4 Updated by sajolida over 4 years ago

During the September meeting we said that this was blocked by two things:

  • Having OnionShare available in Debian stable, probably through backports (currently in NEW queue).
  • Having it audited, that's #7873.

#5 Updated by sajolida over 4 years ago

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

#6 Updated by sajolida over 4 years ago

  • Subject changed from Include OnionShare in Tails to Include OnionShare

#7 Updated by sajolida over 4 years ago

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

#8 Updated by u over 4 years ago

OnionShare is now in Debian unstable :)

#9 Updated by u over 4 years ago

Onionshare is also available through wheezy-backports.

= Version 0.6 has a nicer GUI and better translations and shall arrive there soon.

#10 Updated by micahflee over 3 years ago

I'm currently testing ephemeral hidden services with OnionShare, which is a feature in Tor 0.2.7.x. Using this type of hidden service will solve multiple Tails-specific problems. It should work in sandbox mode, which is what's currently preventing OnionShare from working in Tails [1]. And because ephemeral HS data is never written to disk (the other type of HS writes files to disk as the debian-tor user, which the amnesia user can't read), hopefully OnionShare can work without root.

I've made an ephemeral branch [2] if you want to try it. But there still are a few issues with getting this working in Tails without root.

1. In order to start a hidden service, OnionShare needs to talk to the Tor control port. This is denied by iptables though. OnionShare could accept it by running `sudo /sbin/iptables -I OUTPUT -o lo -p tcp --dport 9051 -j ACCEPT`, but it would require root. Should the amnesia user be allowed to connect to the Tor control port?

2. Even after OnionShare can communicate with the Tor control port, it's configured to require authentication. If the amnesia user should be allowed to use it, then the authentication would need to be removed.

3. OnionShare also works by running a web server on a local port (it just picks one that's available each time), and making that the hidden service. For that to work, Tor needs to be able to connect to that port, but iptables denies it. OnionShare could accept it by running an iptables command, but it would require root. This could be avoided by choosing a port, or range of ports, that OnionShare will always use in Tails and ship iptables rules that accept connections to these ports by default.

I guess this is a Tails policy decision. Should OnionShare work with just the amnesia user? Or should you have to set an administrator password when you login to Tails if you want to use OnionShare?

[1] https://github.com/micahflee/onionshare/issues/179
[2] https://github.com/micahflee/onionshare/tree/ephemeral

#11 Updated by intrigeri over 3 years ago

Should the amnesia user be allowed to connect to the Tor control port?

Unfiltered access to the control port == trivial deanonymization. Currently the desktop user has filtered access via https://git-tails.immerda.ch/tails/tree/config/chroot_local-includes/usr/local/sbin/tor-controlport-filter. It would be good to know what exact additional commands the filter should let through.

2. Even after OnionShare can communicate with the Tor control port, it's configured to require authentication. If the amnesia user should be allowed to use it, then the authentication would need to be removed.

I'm not sure what you mean here. I see no such authentication enabled in our torrc. Perhaps you're testing with a different tor or torrc?

This could be avoided by choosing a port, or range of ports, that OnionShare will always use in Tails and ship iptables rules that accept connections to these ports by default.

Sounds good. That range could be documented for all similar HS usage, by the way.

I guess this is a Tails policy decision. Should OnionShare work with just the amnesia user?

Yes, please. I suggest drafting some user stories with Gherkin (see *.feature on https://git-tails.immerda.ch/tails/tree/features/ for examples) and emailing about it, and once they're happy with it + there's a prototype, we can check the security/implementation details.

#12 Updated by intrigeri over 3 years ago

#8173#note-6 is relevant here too, wrt. control port filtering.

#13 Updated by intrigeri about 3 years ago

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

#14 Updated by intrigeri about 3 years ago

See #6742 for pointers wrt. control port filter implementations.

#15 Updated by micahflee about 3 years ago

During the winter 2016 Tor dev meeting I made a bunch of progress on this issue. Here's a branch with my commits: https://github.com/micahflee/tails/commits/feature/7870-include_onionshare

Most of the work involved rewriting tor-controlport-filter that was included in Tails. My new version follows the control port spec better in various ways (e.g. responses include "\r\n" instead of "\n"), and fully supports connecting with stem (the current version throws errors). I've also refactored it to connect to the real control port using stem, so python-stem 1.4.0+ is now a dependency for the new tor-controlport-filter (it's also a dependency for OnionShare), but I'm not sure how to include this dependency in the Tails project so I didn't add any commits for that.

In addition to responding to "SIGNAL NEWNYM" and "AUTHENTICATE" like the current version does, the new tor-controlport-filter now supports the following things:

  • Responds to "PROTOCOLINFO", which stem calls when connecting. It responds saying there's no authentication, even though the real control port requires authentication.
  • Responds to "GETINFO version" with the real tor version.
  • Responds to "ADD_ONION", "DEL_ONION", and "GETINFO onions/current", so now there is full ephemeral hidden service support.

Another issue was what port OnionShare listens on. I picked the port range 17600-17650 (because no one else is using that range), and have already committed this change to the OnionShare project, so it will be available in the next release. So I've also included a commit to ferm.conf to allow access to these ports on the loopback device, but I haven't actually tested that this works within Tails.

#16 Updated by u about 3 years ago

Hi!

micahflee wrote:

During the winter 2016 Tor dev meeting I made a bunch of progress on this issue. Here's a branch with my commits: https://github.com/micahflee/tails/commits/feature/7870-include_onionshare

Awesome !!! :)

Most of the work involved rewriting tor-controlport-filter that was included in Tails. My new version follows the control port spec better in various ways (e.g. responses include "\r\n" instead of "\n"), and fully supports connecting with stem (the current version throws errors). I've also refactored it to connect to the real control port using stem, so python-stem 1.4.0+ is now a dependency for the new tor-controlport-filter (it's also a dependency for OnionShare), but I'm not sure how to include this dependency in the Tails project so I didn't add any commits for that.

I think in order to have this dependency added, we simply need to update the onionshare package in Debian - and Tails will use it from there. I can take care of this.

Another issue was what port OnionShare listens on. I picked the port range 17600-17650 (because no one else is using that range), and have already committed this change to the OnionShare project, so it will be available in the next release. So I've also included a commit to ferm.conf to allow access to these ports on the loopback device, but I haven't actually tested that this works within Tails.

I think this would need to be part of dedicated integration work within Tails then.

#17 Updated by sajolida about 3 years ago

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

#18 Updated by sajolida about 3 years ago

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

Thanks for moving this forward. I marked this has related to #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 about 3 years ago

  • Status changed from Confirmed to In Progress

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

#20 Updated by u about 3 years ago

I've just proposed a new backport for Jessie to Ximin who sponsored my last version of the package, too.

As soon as Micah releases a new version, I can build it for Debian and ask somebody else from the pkg-privacy maintainers team to upload it.

#21 Updated by anonym almost 3 years ago

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

#22 Updated by anonym almost 3 years ago

  • 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 almost 3 years ago

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

#24 Updated by intrigeri over 2 years ago

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

#25 Updated by anonym over 2 years ago

  • 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 (#6742#note-11) and Subgraph's roflcoptor (#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... :/

#26 Updated by u over 2 years ago

  • 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.

There is already a packaging which I've worked on but there are failing tests and thus it has not been uploaded to Debian unstable. Thus there is no backport. I'm waiting either for Micah to work on these tests either I have to do it myself, but I'm not sure I have the time to dive that much into the code. See https://github.com/micahflee/onionshare/issues/284

But I can add those patches to the package indeed!

#27 Updated by u over 2 years ago

version 0.9.1 is on it's way into Debian unstable.
I'll try to patch and test that and will prepare another upload and backport when that test is conclusive.

#28 Updated by cypherpunks over 2 years ago

Please don't include this backdoor into tails!

ADD_ONION is a f*cking evil backdoor!

Here is an alternative:

#!/bin/bash
  1. single-socat.sh

gzip -f $
socat -vv UNIX-LISTEN:/home/amnesia/socat.sock,fork EXEC:"./single-bash.sh $
"

#!/bin/bash
  1. single-bash.sh

DATE=$(date +"%a, %d %b %Y %H:%M:%S %Z")
CONTENT_LENGTH=$(stat -c '%s' $@.gz)
SERVER="notonionshare"

echo -ne "HTTP/1.0 200 OK\r\n"
echo -ne "Date: $DATE\r\n"
echo -ne "Expires: $DATE\r\n"
echo -ne "Server: $SERVER\r\n"
echo -ne "Content-Type: application/octet-stream\r\n"
echo -ne "Content-Length: $CONTENT_LENGTH\r\n"
echo -ne "Content-Encoding: x-gzip\r\n"
echo -ne "\r\n"
cat $@.gz

#Add to torrc
HiddenServiceDir /var/lib/tor/private
HiddenServicePort 80 unix:/home/amnesia/socat.sock
HiddenServiceAuthorizeClient stealth private01

#29 Updated by cypherpunks over 2 years ago

cypherpunks wrote:

Please don't include this backdoor into tails!

ADD_ONION is a f*cking evil backdoor!

Here is an alternative:

Though I am a fan of very simple alternatives involving nothing more than socat, I think we'd like to know why you consider OnionShare a "backdoor", and what this has to do with ADD_ONION.

#30 Updated by sajolida over 2 years ago

Are several people using the cypherpunks account?

#31 Updated by intrigeri over 2 years ago

Are several people using the cypherpunks account?

Most likely it has cypherpunks as its password, and various people use it.

#32 Updated by cypherpunks over 2 years ago

ADD_ONION is a pandora box. You don't have control over it, it works silently, barely registers in the logs, it can bypass your firewall, and it's very hard to know for sure where it's connecting to.

ADD_ONION is dangerous because it's not you, but a 3rd party application has control over it. The typical response from the tor dev team on irc was something like, well you shouldn't give anyone access to your tor controller, even anonym on IRC said something similar! but the inclusion of onionshare in tails will screw all tails users royally.
Onionshare uses ADD_ONION and if you compromise onionshare you can compromise the tails user.

My main problem with ADD_ONION is that you cannot control the TARGET part for ADD_ONION. It can virtually be any destination, either local or remote. If that is fixed by upstream tor, maybe then the inclusion of onionshare would be ok.

refer to:
https://trac.torproject.org/projects/tor/ticket/18608
https://trac.torproject.org/projects/tor/ticket/18157
https://trac.torproject.org/projects/tor/ticket/18156

cypherpunks password is writecode, the same for trac.torproject.org

ps for the single-socat.sh script, it should have been $@ instead of $.
pps onionshare doesn't even use HidServAuth!

#33 Updated by cypherpunks over 2 years ago

cypherpunks wrote:

Onionshare uses ADD_ONION and if you compromise onionshare you can compromise the tails user.

My main problem with ADD_ONION is that you cannot control the TARGET part for ADD_ONION. It can virtually be any destination, either local or remote. If that is fixed by upstream tor, maybe then the inclusion of onionshare would be ok.

And how big is the attack surface area for OnionShare? Adding an application which has a dangerous amount of control over Tor is not something that I would like. I suppose it would also make it such that local applications running under the amnesia user could gain greater control of the control port by hijacking OnionShare via LD_PRELOAD, LD_LIBRARY_PATH, or various GTK modules, regardless of the attack surface.

You really should explain the issue more clearly rather than just saying "it's a fucking evil backdoor", which simply makes it easy to dismiss your objection as yet another crazy cypherpunks user, even if it is important to bring up.

pps onionshare doesn't even use HidServAuth!

If it used HidServAuth, then the person who wanted to download from it would need to modify their own torrc and reload Tor before they could access that HS. I don't think that would be particularly user-friendly. A better compromise would be to implement basic password authentication upon connect, in order to prevent HS harvesting from being used to find and download whatever is being served.

intrigeri wrote:

Are several people using the cypherpunks account?

Most likely it has cypherpunks as its password, and various people use it.

It's a generic community account with the password writecode (from the "cypherpunks manifesto"). It's also shared by the Tor Project bug tracker. Sometimes people say crazy things under that account. Hopefully it doesn't make valuable contributions from this handle seem less credible.

#34 Updated by sajolida over 2 years ago

Hopefully it doesn't make valuable contributions from this handle
seem less credible.

It's useful to me to know that I'm not talking to a single person :)

#35 Updated by cypherpunks over 2 years ago

Yawning said: it's probably a good idea to assume that anything that has access to the control port can de-anonymize you

atagar said: But you might be right about ADD_ONION being even more dangerous (can't say for sure).

dgoulet said: After discussion on #tor-dev, with an open control port, if an attacker is able to give it command, you are doomed by design.

The dangers outweigh the benefits on this one. Murphy's law.

#36 Updated by anonym over 2 years ago

ADD_ONION is dangerous [...]

Note that the whole point of Tails' control port filter is to prevent certain attacks possible if a user with full access to the control port is compromised -- we only allow some commands, and certain arguments to some commands. So...

My main problem with ADD_ONION is that you cannot control the TARGET part for ADD_ONION. It can virtually be any destination, either local or remote. If that is fixed by upstream tor, maybe then the inclusion of onionshare would be ok.

... we can limit (via regex) the parameters to ADD_ONION (and SETCONF HiddenServiceDir) so only sane targets can be used, e.g. loopback space (or just 127.0.0.1 for simplicity).

Would this fix all your concerns, cypherpunks (from comments 28, 32 and 35)?

#37 Updated by cypherpunks over 2 years ago

To anonym.
Ideally this should be fixed at the tor level.
Wouldn't it be safer if onionshare used unix sockets? And then have the tor-control-filter's ADD_ONION only use a hardcoded value, say /var/run/onionshare.sock?
I don't think this requires any modification of the ferm firewall rules.

#38 Updated by anonym over 2 years ago

cypherpunks wrote:

Ideally this should be fixed at the tor level.

Perhaps, but we're not there yet.

Wouldn't it be safer if onionshare used unix sockets?

Elaborate "safer" (w.r.t. what?). Is this about adding filesystem permissions as another security layer? If so, sure, but I doubt we'd prioritize this. Patches welcome!

And then have the tor-control-filter's ADD_ONION only use a hardcoded value, say /var/run/onionshare.sock?

Now I'm lost. ADD_ONION is a control port command you send when you already have established a control port session. The Unix socket is just a different method of accessing the control port (than the e.g. Tor's SOCKSPort which uses TCP) so I don't see how any of this is related.

I don't think this requires any modification of the ferm firewall rules.

Indeed, Netfilter has no filesystem concepts.

#39 Updated by anonym over 2 years ago

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

#40 Updated by intrigeri over 2 years ago

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

#41 Updated by anonym over 2 years ago

  • 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 over 2 years ago

  • 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.

#43 Updated by anonym over 2 years ago

anonym wrote:

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).

I had another look, and now I got it right. Now I'm much happier with the filter! :)

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

Even more so now!

#44 Updated by intrigeri over 2 years ago

  • Assignee set to u

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

#45 Updated by anonym over 2 years ago

Note that by now (after some joyful coding procrastination this weekend, yay) this branch does a few more things than just include onionshare:

  • #6788: Use stem in the filtering proxy for the Tor control port
  • 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 #9365 less of an issue.
  • Enable the per-tab circuit view in the Tor Browser. While the discussion/conclusions of #9365, #10339 and #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, I guess.
  • #9001: Onion Circuits should connect via the Tor control port filter. Because of this + the per-process filtering the onioncircuits user and wrapper script are not needed any more, so simplification++.

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

intrigeri, could you look at 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?

#46 Updated by micahflee over 2 years ago

Hello from Tor dev in Seattle! Just want to say that I'm very excited about this progress. I tried out the nightly build that includes OnionShare and it seems to work great. My main dev goals this week are to work on outstanding issues in OnionShare and Tor Browser Launcher, and to do whatever I can to help finish this issue.

#47 Updated by micahflee over 2 years ago

I've merged the related pull request upstream: https://github.com/micahflee/onionshare/pull/314

So it will be in the next onionshare release.

#48 Updated by anonym over 2 years ago

micahflee wrote:

Hello from Tor dev in Seattle!

Yay! Send my regards to everyone! :)

I've merged the related pull request upstream: https://github.com/micahflee/onionshare/pull/314

Excellent! I think that's all we strictly need from your side...

So it will be in the next onionshare release.

... except that it is eventually released, of course. :) It'd be great if this release was backported to Jessie, too, although installing from Stretch like we do now probably will work until Tails is based on Stretch and it's solved automatically.

#49 Updated by intrigeri over 2 years ago

  • Assignee changed from u 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 #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 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 over 2 years ago

  • 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 #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, #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 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 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.

#51 Updated by anonym over 2 years ago

anonym wrote:

intrigeri, could you look at 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?

For the record, roflcoptor deals with this problem by separating the cuntionality (only) into procsnitchd. Any way, I've looked at the capabilities list, and there's nothing that seems to do what I'd like, so I think you can forget about this.

#52 Updated by intrigeri over 2 years ago

  • 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.

#53 Updated by anonym over 2 years ago

anonym wrote:

anonym wrote:

intrigeri, could you look at 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?

For the record, roflcoptor deals with this problem by separating the cuntionality (only) into procsnitchd. Any way, I've looked at the capabilities list, and there's nothing that seems to do what I'd like, so I think you can forget about this.

I've noted that roflcoptor also does filter selection based on "who" is connecting, also by looking up the PID, but then looking at /proc/pid/exe. So they have the same issue with interpreted languages.

#54 Updated by anonym over 2 years ago

  • 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 #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 #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. :)

#55 Updated by intrigeri over 2 years ago

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

Crazy idea: confine each app we want to filter with AppArmor, and then use the profile assigned by AppArmor to identify the app in the control port filtering proxy.

#56 Updated by anonym over 2 years ago

intrigeri wrote:

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

Crazy idea: confine each app we want to filter with AppArmor, and then use the profile assigned by AppArmor to identify the app in the control port filtering proxy.

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. It might be a good exercise for me to write some apparmor profiles, e.g. for onioncircuits and onionshare.

Can you elaborate how you think that could be implemented? 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 cement that tor-controlport-filter would have to run as root, though, but I am not so worried about that.

#57 Updated by intrigeri over 2 years ago

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.

Great!

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.

Can you elaborate how you think that could be implemented?

I had no idea when suggesting this in passing :)

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.

#58 Updated by anonym over 2 years ago

  • % 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 over 2 years ago

  • Assignee changed from anonym to u
  • % 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 #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.

#60 Updated by intrigeri over 2 years ago

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! :)

Maybe set the profiles to enforce mode and add allow /** rwkux (untested, making that up) or something to not block anything?

#61 Updated by intrigeri over 2 years ago

[...] before the end of October (so 2016-10-31)?

Why this deadline? I don't think that branch can reasonably be a candidate for Tails 2.7.

#62 Updated by anonym over 2 years ago

intrigeri wrote:

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! :)

Maybe set the profiles to enforce mode and add allow /** rwkux (untested, making that up) or something to not block anything?

Note that the updated profiles (mentioned in #7870#note-59) are already good enough for not flooding the journal while in complain mode. I get that it's hard to follow this "ticket" now, sorry! I will do a summary of all that's been going on whenever I request a review.

[...] before the end of October (so 2016-10-31)?

Why this deadline? I don't think that branch can reasonably be a candidate for Tails 2.7.

I have bad experience setting deadlines close to my own deadline, because I expect I will have to put more effort into the profiles after I've got feedback, and then there could be need for more roundtrips. But I see how you might have misunderstood that I was somehow aligning it with the 2.7 release. :)

#63 Updated by anonym over 2 years ago

I've added an (optional) option I call restrict-stream-events (boolean, default: false) that, when set, restricts clients subscribing to STREAM events so they only see the events about streams they initiated themselves. For instance, if set for the Tor Browser this means that it will only receive its own STREAM events, and not those from other applications. That is good, because its per-tab circuit view just discards such streams, so there's no unnecessary information leak.

In fact, this exact use case is the rationale behind this feature. I am somewhat concerned about the browser's extensibility; now that we can fix the control port policy per application we pretty much can dictate exactly how the control port will be used, except for applications that are extensible or just have some functionality for running arbitrary code. I mean, if the amnesia user is compromised, it now cannot use Tor Browser's JavaScript console to learn about all streams. Of course, it can start OnionCircuits and "parse" its GUI with Dogtail, so perhaps this is a moot point any way. :)

#64 Updated by u over 2 years ago

  • Assignee changed from u 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.)

#65 Updated by u over 2 years ago

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?

#66 Updated by anonym over 2 years ago

  • 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.

#67 Updated by u over 2 years ago

Hi,

anonym wrote:

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.

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.

#68 Updated by anonym over 2 years ago

  • 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.

#69 Updated by intrigeri over 2 years ago

Correct, because in Debian, as onionshare is not usable over system tor,

Just curious: can't it be used that way by a user who has full access to the system Tor's control port?

#70 Updated by intrigeri over 2 years ago

So a tor | torbrowser-launcher seems appropriate.

torbrowser-launcher | tor would probably be nicer to get something that works out-of-the-box by default.

#71 Updated by cypherpunks over 2 years ago

anonym wrote:

cypherpunks wrote:

Ideally this should be fixed at the tor level.

Perhaps, but we're not there yet.

Wouldn't it be safer if onionshare used unix sockets?

Elaborate "safer" (w.r.t. what?). Is this about adding filesystem permissions as another security layer? If so, sure, but I doubt we'd prioritize this. Patches welcome!

And then have the tor-control-filter's ADD_ONION only use a hardcoded value, say /var/run/onionshare.sock?

Now I'm lost. ADD_ONION is a control port command you send when you already have established a control port session. The Unix socket is just a different method of accessing the control port (than the e.g. Tor's SOCKSPort which uses TCP) so I don't see how any of this is related.

I don't think this requires any modification of the ferm firewall rules.

Indeed, Netfilter has no filesystem concepts.

Sorry i disappeared. What I meant with the unix socket is have onion share use a unix socket instead of listening to 127.0.0.1:17600+. This requires a change in onionshare itself.

Have onionshare listening socket be /var/run/onionshare/onionshare-1.sock as an example, and the tor control filter only allows the add_onion target be /var/run/onionshare/onionshare-*.sock. This won't require any ferm modifications.

#72 Updated by anonym over 2 years ago

cypherpunks wrote:

Sorry i disappeared. What I meant with the unix socket is have onion share use a unix socket instead of listening to 127.0.0.1:17600+. This requires a change in onionshare itself.

Those ports are used for the web servers serving the shared files. I.e. they are the hidden services. Hidden services cannot use UNIX sockets.

Have onionshare listening socket be /var/run/onionshare/onionshare-1.sock as an example, and the tor control filter only allows the add_onion target be /var/run/onionshare/onionshare-*.sock. This won't require any ferm modifications.

This is in fact what we'd get if we used roflcoptor, but I don't see any advantage in it over the current solution. Am I missing something?

Also, note that the current solution doesn't require any ferm modifications either (we still have to allow the amnesia user to access the TCP control port for other reasons).

#73 Updated by anonym over 2 years ago

anonym wrote:

cypherpunks wrote:

Sorry i disappeared. What I meant with the unix socket is have onion share use a unix socket instead of listening to 127.0.0.1:17600+. This requires a change in onionshare itself.

Those ports are used for the web servers serving the shared files. I.e. they are the hidden services. Hidden services cannot use UNIX sockets.

Actually I'm wrong -- HiddenServicePort can accept UNIX sockets as unix:$PATH. So, onionshare would then need to run an http server that can serve over a UNIX socket. Flask, which is currently used, doesn't seem to support it, but perhaps it can be extended to do so?

Have onionshare listening socket be /var/run/onionshare/onionshare-1.sock as an example, and the tor control filter only allows the add_onion target be /var/run/onionshare/onionshare-*.sock. This won't require any ferm modifications.

Indeed, I quite like this!

However, the current state of things is good enough, so I won't spend any time on this. I'd love if you filed an upstream bug, however! :)

#74 Updated by anonym over 2 years ago

anonym wrote:

So, onionshare would then need to run an http server that can serve over a UNIX socket. Flask, which is currently used, doesn't seem to support it, but perhaps it can be extended to do so?

Flask uses wekzeug, and after a quick search it seems someone has already done this (although I didn't look at the details): https://github.com/tomekwojcik/werkzeug_plus_unix_sockets

Any way, this is the last I intend to say about this topic on this ticket.

#75 Updated by anonym over 2 years ago

  • 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: #6742, #6788, #9001. Those tickets are updated to reflect this, but let's keep all discussion on this ticket.
  • I also just closed #11542 and #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 #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 #9365, #10339 and #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 over 2 years ago

  • 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?

#77 Updated by u over 2 years ago

intrigeri wrote:

  • 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'll take care of it. Currently working on a review with anonym. Eventually it would be nice to have a more security related review by you or jvoisin. (I can ask him.)

#78 Updated by anonym over 2 years ago

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 over 2 years ago

  • Target version changed from Tails_2.9.1 to Tails 2.10

#80 Updated by anonym over 2 years ago

I bumped this feature to the new next major release (given 2.8 was cancelled).

#81 Updated by intrigeri over 2 years ago

  • Affected tool set to OnionShare

#82 Updated by intrigeri over 2 years ago

  • 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 #12021 to track this. I'll assume that you've tested this backport on a regular Jessie system. If not, please comment on #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 #11929 and #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 #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 over 2 years ago

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

#84 Updated by intrigeri over 2 years ago

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

#85 Updated by intrigeri over 2 years ago

#86 Updated by intrigeri over 2 years ago

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

#87 Updated by intrigeri over 2 years ago

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

#88 Updated by anonym over 2 years ago

  • 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 #12021 to track this. I'll assume that you've tested this backport on a regular Jessie system. If not, please comment on #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.

#89 Updated by sajolida over 2 years ago

This ticket is huge so I created a separate one for the documentation (#12026). Please tell me once the code is ready and merge into devel so I can try it out. Hints on how to get started and what to document would be warmly welcome.

#90 Updated by intrigeri over 2 years ago

  • 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 over 2 years ago

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?

#92 Updated by anonym over 2 years ago

micahflee wrote:

Hello Tails people. Sorry I've been so absent from this conversation

Don't worry, the discussion on this ticket went far beyond the initial subject, and that is my fault (but TBH I think it was good for avoiding communication overhead by splitting the discussion to their respective tickets).

but I'm incredibly excited about all of this progress, and I can't wait for OnionShare to be in Tails by default.

Me too! It should be less than a month away from now.

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.

Sounds good!

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.

Actually, we don't any more, so we do not really care about the "meantime". \o/

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?

Yes. To me it sounds like good practice if the options file has higher priority than environment variablew. Only command line options (or graphical controls for options) should override the options file.

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.

Cool! Off topic: perhaps we can make Tor Browser handle some custom URL scheme which includes both HidServAuth's onion-address and auth-cookie? Of course, this also introduces compatibility issues (unsupported clients) and maybe even extra attack surface (e.g. sending the onion-address and auth-cookie to tor via the control connection). I'm just throwing a crazy idea out there. Overall, I like if we can avoid options, and instead explore the possibility of making optional improvements the only mode of operation.

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?

tl;dr: yes, at some point. :)

Tails will move to tor >= 0.2.9.x as soon as it is the preferred stable tor branch, and python3-stem 1.5.3 is already in Debian Stretch (which Tails 3.0 will be based on). If needed, python3-stem 1.5.x can easily be backported to Debian Jessie, and hence Tails 2.x. Personally I'd like a simple way for Tails users to handle such URLs before I'd put any extra effort at making this feature available any earlier than when we get it for free (Tails 3.0 will hopefully be released by mid-June).

#93 Updated by intrigeri over 2 years ago

To me it sounds like good practice if the options file has higher priority than environment variablew. Only command line options (or graphical controls for options) should override the options file.

Nitpicking: I'm under the impression that most programs act the opposite way: more volatile ways to set the config (e.g. envvars) take precedence over more hard-code, long-term ways to do it (e.g. config file).

Not that I care much what onionshare specifically does though :)

Tails will move to tor >= 0.2.9.x as soon as it is the preferred stable tor branch,

i.e. now!

#94 Updated by intrigeri over 2 years ago

In AppArmor 2.11 (that should make its way into Stretch by the end of the month), aa-status has a --json option. Would be nice to have a ticket about using it in Tails 3.0 :)

#95 Updated by anonym over 2 years ago

intrigeri wrote:

To me it sounds like good practice if the options file has higher priority than environment variablew. Only command line options (or graphical controls for options) should override the options file.

Nitpicking: I'm under the impression that most programs act the opposite way: more volatile ways to set the config (e.g. envvars) take precedence over more hard-code, long-term ways to do it (e.g. config file).

Err. You are correct! :S Micah, please ignore what I said on this topic! :)

In AppArmor 2.11 (that should make its way into Stretch by the end of the month), aa-status has a --json option. Would be nice to have a ticket about using it in Tails 3.0 :)

Filed as #12123.

#96 Updated by anonym over 2 years ago

intrigeri wrote:

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.

Done in 59e073a509.

This kind of stuff:

[...]

… 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.

I didn't find any very pythonic way to do it. It seems the pythonic way is to not do self.x = handler.x in the constructor, but just self.handler = handler and then use it with self.handler.x. IIRC you do not like this, but I don't see why it's bad in this case -- do you? If not, I'll just do that instead.

self.handler is defined but not used in FilteredControlPortProxySession.

Fixed.

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.

They seemed innocent enough to me.

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.

Whoops, that could cause an actual thread crash, now fixed in 7b5683e212.

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

Wait until I say I have tested the recent changes (including your flake8 changes) myself before proceeding!

#97 Updated by anonym over 2 years ago

  • 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: #6742, #6788 and #9001).

#98 Updated by intrigeri over 2 years ago

Code review passes, will now test.

#99 Updated by intrigeri over 2 years ago

I'll add a commit on top to "Drop useless feature-7870-include-onionshare APT overlay" though.

#100 Updated by intrigeri over 2 years ago

  • 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 over 2 years ago

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

#102 Updated by anonym over 2 years ago

  • 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 => #12130.

#103 Updated by intrigeri over 2 years ago

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

#104 Updated by intrigeri over 2 years ago

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

#105 Updated by intrigeri over 2 years ago

(modulo one UX problem I've reported upstream).

Thanks! Link?

https://github.com/micahflee/onionshare/issues/345

Previously the control port filter would block the subscription to the stream events etc which Tor Browser initiates on start,

If not blocking this anymore has any consequence on related tickets (e.g. the one about view of the currently used circuit, or whatnot), then please update them :)

#106 Updated by anonym over 2 years ago

  • Status changed from Fix committed to Resolved

Also available in: Atom PDF