Merge lp://qastaging/~chipaca/snap-confine/unshare into lp://qastaging/~snappy-dev/snap-confine/trunk

one small comment

One thing this branch does, without mentioning it (until now), is that if something fails you now get to see the error string and not just the error code.

You're welcome.

The DENIEDs you get without the changes suggested by jdstrand are:

without the first (rw, private) one:

audit: type=1400 audit(1431102661.774:53): apparmor="DENIED" operation="mount" info="failed mntpnt match" error=-13 profile="/usr/bin/ubuntu-core-launcher" name="/tmp/" pid=1928 comm="ubuntu-core-lau" flags="rw, private"

without the second (rw, bind) one:

audit: type=1400 audit(1431103430.829:55): apparmor="DENIED" operation="mount" info="failed flags match" error=-13 profile="/usr/bin/ubuntu-core-launcher" name="/tmp/" pid=1942 comm="ubuntu-core-lau" srcname="/tmp/snaps.1000/hello-world.canonical/1.0.15/tmp/" flags="rw, bind"

Yay, nice! Code looks good!

I have an (inline) question about the use of stat(2).

The changes in this MP can be used by an unprivileged user to grant access to files/directories that are not intended to be reachable by that user.

For example, assume that mode on /root is set to 700, meaning that an unprivileged user cannot enter the /root directory. If a user running ubuntu-core-launcher sets SNAP_APP_TMPDIR to /root/foo, ubuntu-core-launcher will fail if /root/foo does not exist. However, it will succeed when /root/foo does exist.

Also, if /root/foo/bar was (lazily) set to be world-readable, then it could opened for reading at /tmp/bar.

Good catch. I'll change it so that the launcher deduces the tmpdir from the app name.

Just pushed a thing that deduces the tmpdir from appname+version and creates it.

This means we need to change the wrapper scripts, but also that the wrapper script can do less.

In a future branch, we should change the wrapper scripts one (last?) more time, to pass all the information in explicitly and have the launcher create the right environ from scratch.

Also, we should probably make the tests work again.

I didn't get to finish my review before my EOD but I have found an issue where an unprivileged user can still traverse paths that they shouldn't have access to and, at least, have arbitrary directories created:

tyhicks$ mkdir /tmp/snaps.1000
tyhicks$ ln -s /root /tmp/snaps.1000/cat
tyhicks$ sudo ls /root
tyhicks$ ./ubuntu-core-launcher cat BAD apparmor /apps/cat/cat
unable to make /tmp/ private. errmsg: Invalid argument
tyhicks$ sudo ls /root
BAD

I'll look some more tomorrow.

Note that the above issue is due to two things:

1) Predictable directory names used in /tmp
2) Continuing if mkdir(2) returns an error with errno set to EEXIST

This allows attackers to create symlinks in /tmp that the launcher follows.

Hi John - This code is looking much more simple now. Thanks for all of the changes.

I have two small changes that need to be made (mentioned inline) and one remaining question.

How are all of the /tmp/snap.UID_appname_XXXXXX directories supposed to be cleaned up after the snap exits?

Here are the changes need to make the apparmor profile work (requires the two inline changes that I mentioned):

=== modified file 'debian/usr.bin.ubuntu-core-launcher'
--- debian/usr.bin.ubuntu-core-launcher 2015-05-08 16:50:56 +0000
+++ debian/usr.bin.ubuntu-core-launcher 2015-05-20 20:12:09 +0000
@@ -50,7 +50,10 @@
     # read apparmor to figure out if we need cgroups
     /var/lib/apparmor/clicks/* r,

- # make /tmp/ private, and bind-mount a private /tmp
+ # set up snap-specific private /tmp dir
+ capability chown,
+ /tmp/ w,
+ /tmp/snap.*/ w,
     mount options=(rw private) -> /tmp/,
- mount options=(rw bind) /tmp/snaps.[0-9]*/**/tmp/ -> /tmp/,
+ mount options=(rw bind) /tmp/snap.*/ -> /tmp/,
 }

Thank you for the review!
Fixed the missing /tmp/ (d'oh), moved the chown (good idea! added a comment so we remember why), done the changes to the apparmor profile.

I suspect the cleanest way to go about clenaing up /tmp/ is to have something like tmpreaper, but we haven't really discussed it yet.

This looks good to me. As you mentioned, there is the outstanding issue of /tmp/snap.* directories being left behind. This could result in a denial of service if mkdtemp() can no longer create a unique directory but I'm not too worried about that in terms of security. It is more of a usability issue, IMO. This gets my ack.