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

[John Lenton]

Set up a private mount namespace for /tmp.

Missing here:
 * tests. Not sure if we can test this; you need CAP_SYS_ADMIN to call unshare().
 * apparmor or seccomp or something is blocking this. Need jdstrand's eyes.

[lp:~chipaca/snap-confine/unshare]

64. By John Lenton on 2015-05-06

move setup_private_mount to the right place

[Sergio Schvezov (sergiusens)]

one small comment

[lp:~chipaca/snap-confine/unshare]

65. By John Lenton on 2015-05-08

made a comment about kernel and libc versions

66. By John Lenton on 2015-05-08

"are require" indeed

67. By John Lenton on 2015-05-08

aslo commented on MS_ vars

[John Lenton (chipaca)]

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.

[lp:~chipaca/snap-confine/unshare]

68. By John Lenton on 2015-05-08

fixes to apparmor profile as per jdstrand

[John Lenton (chipaca)]

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"

[Michael Vogt (mvo)]

Yay, nice! Code looks good!

review: Approve

[Tyler Hicks (tyhicks)]

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

review: Needs Information

[Tyler Hicks (tyhicks)]

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.

review: Needs Fixing

[John Lenton (chipaca)]

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

[lp:~chipaca/snap-confine/unshare]

69. By John Lenton on 2015-05-13

fixed issues found in review (and then some).

[John Lenton (chipaca)]

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.

[Tyler Hicks (tyhicks)]

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.

review: Needs Fixing

[Tyler Hicks (tyhicks)]

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.

[lp:~chipaca/snap-confine/unshare]

70. By John Lenton on 2015-05-20

moved back to not requiring the version; use mkdtemp to create the tempdir

71. By John Lenton on 2015-05-20

drop now spurious includes

[Tyler Hicks (tyhicks)]

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?

review: Needs Fixing

[Tyler Hicks (tyhicks)]

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/,
 }

[lp:~chipaca/snap-confine/unshare]

72. By John Lenton on 2015-05-20

address issues raised by tyhicks in review

[John Lenton (chipaca)]

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.

[Tyler Hicks (tyhicks)]

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.

review: Approve

=== modified file 'debian/usr.bin.ubuntu-core-launcher'
--- debian/usr.bin.ubuntu-core-launcher	2015-04-22 15:32:05 +0000
+++ debian/usr.bin.ubuntu-core-launcher	2015-05-20 21:14:43 +0000
@@ -49,4 +49,11 @@
 
     # read apparmor to figure out if we need cgroups
     /var/lib/apparmor/clicks/* r,
+
+    # set up snap-specific private /tmp dir
+    capability chown,
+    /tmp/ w,
+    /tmp/snap.*/ w,
+    mount options=(rw private) -> /tmp/,
+    mount options=(rw bind) /tmp/snap.*/ -> /tmp/,
 }
 
=== modified file 'src/Makefile'
--- src/Makefile	2015-04-18 14:53:25 +0000
+++ src/Makefile	2015-05-20 21:14:43 +0000
@@ -1,5 +1,5 @@
 
-CFLAGS= -O2 -Wall -Werror $(shell dpkg-buildflags --get CFLAGS)
+CFLAGS= -D_GNU_SOURCE -O2 -Wall -Werror $(shell dpkg-buildflags --get CFLAGS)
 LD_FLAGS = $(shell dpkg-buildflags --get LDFLAGS)
 LIBS = -lapparmor -lseccomp -ludev
 
 
=== modified file 'src/main.c'
--- src/main.c	2015-04-23 12:03:47 +0000
+++ src/main.c	2015-05-20 21:14:43 +0000
@@ -38,6 +38,8 @@
 #include "utils.h"
 #include "seccomp.h"
 
+#define MAX_BUF 1000
+
 bool verify_appname(const char *appname) {
    // these chars are allowed in a appname
    const char* whitelist_re = "^[a-z0-9][a-z0-9+._-]+$";
@@ -198,6 +200,38 @@
    return false;
 }
 
+void setup_private_mount(const char* appname) {
+    uid_t uid = getuid();
+    gid_t gid = getgid();
+    char tmpdir[MAX_BUF] = {0};
+
+    must_snprintf(tmpdir, MAX_BUF, "/tmp/snap.%d_%s_XXXXXX", uid, appname);
+    if (mkdtemp(tmpdir) == NULL) {
+        die("unable to create tmpdir");
+    }
+
+    // unshare() and CLONE_NEWNS require linux >= 2.6.16 and glibc >= 2.14
+    // if using an older glibc, you'd need -D_BSD_SOURCE or -D_SVID_SORUCE.
+    if (unshare(CLONE_NEWNS) < 0) {
+        die("unable to set up mount namespace");
+    }
+
+    // MS_PRIVATE needs linux > 2.6.11
+    if (mount("none", "/tmp", NULL, MS_PRIVATE, NULL) != 0) {
+        die("unable to make /tmp/ private");
+    }
+
+    // MS_BIND is there from linux 2.4
+    if (mount(tmpdir, "/tmp", NULL, MS_BIND, NULL) != 0) {
+        die("unable to bind private /tmp");
+    }
+
+    // do the chown after the bind mount to avoid potential shenanigans
+    if (chown("/tmp/", uid, gid) < 0) {
+        die("unable to chown tmpdir");
+    }
+}
+
 int main(int argc, char **argv)
 {
    const int NR_ARGS = 3;
@@ -226,6 +260,9 @@
    }
 
    if(geteuid() == 0) {
+       // set up private mounts
+       setup_private_mount(appname);
+
        // this needs to happen as root
        if(snappy_udev_setup_required(appname)) {
           setup_devices_cgroup(appname);
@@ -241,7 +278,7 @@
        if (setgid(real_gid) != 0)
           die("setgid failed");
        if (setuid(real_uid) != 0)
-          die("seteuid failed");
+          die("setuid failed");
 
        if(real_gid != 0 && (getuid() == 0 || geteuid() == 0))
           die("dropping privs did not work");
 
=== modified file 'src/utils.c'
--- src/utils.c	2015-04-23 10:20:17 +0000
+++ src/utils.c	2015-05-20 21:14:43 +0000
@@ -14,11 +14,11 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  *
  */
-#include<stdio.h>
-#include<stdlib.h>
-#include<stdarg.h>
-#include<string.h>
-#include<stdio.h>
+#include <errno.h>
+#include <stdarg.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
 
 #include "utils.h"
 
@@ -29,7 +29,11 @@
    vfprintf(stderr, msg, va);
    va_end(va);
 
-   fprintf(stderr, "\n");
+   if (errno != 0) {
+     perror(". errmsg");
+   } else {
+     fprintf(stderr, "\n");
+   }
    exit(1);
 }
 
@@ -68,7 +72,7 @@
    fclose(f);
 }
 
-void must_snprintf(char *str, size_t size, const char *format, ...) {
+int must_snprintf(char *str, size_t size, const char *format, ...) {
    int n = -1;
 
    va_list va;
@@ -78,4 +82,6 @@
 
    if(n < 0 || n >= size)
       die("failed to snprintf %s", str);
+
+   return n;
 }
 
=== modified file 'src/utils.h'
--- src/utils.h	2015-04-23 10:20:17 +0000
+++ src/utils.h	2015-05-20 21:14:43 +0000
@@ -19,12 +19,20 @@
 #ifndef CORE_LAUNCHER_UTILS_H
 #define CORE_LAUNCHER_UTILS_H
 
+__attribute__ ((noreturn))
+__attribute__ ((format (printf, 1, 2)))
 void die(const char *fmt, ...);
+
+__attribute__ ((format (printf, 1, 2)))
 bool error(const char *fmt, ...);
+
+__attribute__ ((format (printf, 1, 2)))
 void debug(const char *fmt, ...);
+
 void write_string_to_file(const char *filepath, const char *buf);
 
 // snprintf version that dies on any error condition
-void must_snprintf(char *str, size_t size, const char *format, ...);
+__attribute__ ((format (printf, 3, 4)))
+int must_snprintf(char *str, size_t size, const char *format, ...);
 
 #endif