AppArmor policy for scope zmq access is too lenient

[Jamie Strandboge]

Currently in apparmor-easyprof-ubuntu 1.2.3 we have:
  owner /run/user/[0-9]*/zmq/Registry-s rw,
  owner /run/user/[0-9]*/zmq/Registry-p r,
  owner /run/user/[0-9]*/zmq/c-*-r rw,

Note that all scopes, regardless of whether they use the ubuntu-scope-network or ubuntu-scope-local-content templates have access to these overlapped accesses. While we discussed the apparmor policy at length at the recent sprint, in thinking about this more there are still a few issues:

 1. How will the scope-registry handle when either /run/user/[0-9]*/zmq/Registry-s or /run/user/[0-9]*/zmq/Registry-p already exists?

 2. In addition to dealing with /run/user/[0-9]*/zmq/c-*-r possibly already existing, there is an additional issue with this access-- because the ubuntu-scope-network and ubuntu-scope-local-content templates both allow this access, this allows a malicious scope author to create a scope using the ubuntu-scope-local-content template, then collect files off the filesystem and store them in /run/user/[0-9]*/zmq/c-I_can_leak_your_files.tar.gz-c, then upload a new version of the scope using the ubuntu-scope-network template, which can then ship /run/user/[0-9]*/zmq/c-I_can_leak_your_files.tar.gz-c off to a remote server when the user upgrades (the fact that it is in /run doesn't really help-- the malicious scope can save the file in its scope-specific directory then copy it in to place to make sure it is always there).

For '1', standard defensive programming should be sufficient and someone should verify that the scopes API is handling when these files already exist (as sockets, regular files, etc, etc).

For '2', standard defensive programming should also be used, but that isn't enough. I suggested at the sprint that these endpoints should be made application specific by their name like with the other endpoints, but was told this is problematic. I can (and will) update the apparmor policy to use this rule:

  owner /run/user/[0-9]*/zmq/c-[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]-r rw,

but this doesn't solve the problem since a malicious app writer will just pick something matching that apparmor regular expression (AARE). AIUI, it is difficult/impossible to make these endpoints application specific (eg, "/run/user/[0-9]*/zmq/c-@{APP_PKGNAME}-r" which would be the preferred fix). If that is the case, we can either namespace this endpoint in zmq/local-fs/c-*r and zmq/local-net/c-*r and adjust the policy templates accordingly. I have a feeling this will have the same problems (or worse) as making the endpoint application specific since you'd need to track the type of scope this is. Alternatively, you could have a garbage collector to unconditionally remove any non-unix domain socket files and unused unix domain socket files that match zmq/c-*-r. While making these endpoints application specific would be cleanest from a policy point of view, implementing good garbage collection (perhaps triggered on scope start/register) would be sufficient to close this bug.

Thanks!

[Pete Woods (pete-woods)]

Taking the liberty of changing the project

[Jamie Strandboge (jdstrand)]

This bug is less important for rtm now that only ubuntu-scope-network is available to click scopes, however, we expect ubuntu-scope-local-content to come back at some point, so this will need to be addressed sometime.

[Michi Henning (michihenning)]

> 1. How will the scope-registry handle when either /run/user/[0-9]*/zmq/Registry-s or /run/user/[0-9]*/zmq/Registry-p already exists?

The algorithm is as follows:

- Check if a connect() to the endpoint succeeds (that is, the other accepts the connection). If so, disconnect and report an error stating that another instance of the service is already running at that endpoint. Otherwise, attempt to bind to the endpoint.

If a file is in the way with the same name, the file is unlinked before binding to the endpoint, so the file will just disappear to make way for the socket.

That latter behavior is out of our control and implemented by Zmq.

If a *directory* is in the way, the attempt to bind to the endpoint fails with an "address already in use" error.

I guess we could modify the code to try and recursively remove any directory that is in the way. But I'm reluctant to do that:

- An innocent configuration error during development could potentially blow away most of the filesystem.

- Removal of the directory before binding suffers from race conditions. An unrelated process can put files or directories back there while the removal is in progress.

BTW, the endpoints for the registry are now called zmq/Registry-R, zmq/Registry-s, and zmq/Registry-p.

> 2. In addition to dealing with /run/user/[0-9]*/zmq/c-*-r possibly already existing

The same applies here: any file that exists there already will be blown away. Any *directory* with the same name will cause the bind to fail. But I'm not too worried about that scenario because the c-*-r name contains a UUID that includes 16 bits of pseudo-randomness from a Mersenne twister (the other 16 bits are a counter). It would be difficult to guess the correct name because each process seeds the random number generator differently, using std::random_device (which is implemented using /dev/random).

> For '2', standard defensive programming should also be used, but that isn't enough. I suggested at the sprint that these
> endpoints should be made application specific by their name like with the other endpoints, but was told this is problematic.

I considered using a directory <scope_id>. Something like /run/user/<uid>/zmq/<scope_id>/endpoint-here

It makes it a pain to test though. I guess we could still do this, creating the scope_id directory if it doesn't exist.

Jamie, let me know how strongly you feel about this. If you think we need it, I'll look at implementing it.

[Jamie Strandboge (jdstrand)]

I don't feel strongly about this based on your comments and my understanding of the defensive programming surrounding c-* files. Removing the rtm tag and marking wishlist as a security improvement for now.

[Jamie Strandboge (jdstrand)]

Actually, I removed the tag but could not adjust the priority. Feel free to mark as wishlist.