Merge lp:~kyrofa/snap-confine/create_user_data_directory into lp:~snappy-dev/snap-confine/trunk

[Kyle Fazzari]

Add creation of user data directory.

Both Snappy binaries and services are provided the $SNAP_USER_DATA environment variable. However, the job of actually creating that directory has until now been left up to the Snappy binary wrapper. Which means that the $SNAP_USER_DATA directory is created for binaries, but actually points to a non-existing directory for services, since nothing creates it (bug #1527612).

This MP attempts to bring the creation of the user data directory into the one thing that both binaries and services have in common: the launcher.

[lp:~kyrofa/snap-confine/create_user_data_directory]

90. By Kyle Fazzari on 2016-02-08

Add creation of user data directory.

Previously this was only handled within Snappy's binary wrappers, which
meant that it wasn't created for services. This is an attempt to bring
that functionality into the thing binaries and services have in common:
Ubuntu Core Launcher.

[Seth Arnold (seth-arnold)]

Does this code run with more privileges than the services that it is working for?

If so, does something prevent the services from running at the same time?

If so, does something fix the directories' owner and group later?

Thanks

[Kyle Fazzari (kyrofa)]

Thanks for taking a look, Seth.

> Does this code run with more privileges than the services that it is working for?

Yes, the launcher runs with setuid root.

> If so, does something prevent the services from running at the same time?

No, nothing prevents services (or binaries) from running at the same time, but from what I've read mkdir() is atomic. Perhaps I misunderstood your concern here?

> If so, does something fix the directories' owner and group later?

Ah, great catch, I need to do that. Services would have been fine, but as-is this will create directories owned by root for binaries, which isn't what we want here. There are two ways I see to do that: the chown() syscall after the fact, or fork and drop privs before creating the directories. Normally I'd say the latter would be more secure so as to mitigate race condition attacks, but since we'd only ever be dropping privilege levels with the chown() (from root to user, never the other way around), I wonder if it's really a concern. Thoughts?

[lp:~kyrofa/snap-confine/create_user_data_directory]

91. By Kyle Fazzari on 2016-02-08

Make sure the directory is owned by the runner, not root.

[Kyle Fazzari (kyrofa)]

I've fixed that with a call to chown() as you can see in the most recent push. Let me know if there's a concern with it.

[Seth Arnold (seth-arnold)]

The trouble is that while one mkdir() call is atomic, a series of them is not; and mkdir() will happily follow symbolic links in the path.

I think there's a few approaches that would work:

- perform the chown() operations in reverse order, so the snap owner never owns a directory that's being operated on
- perform all the mkdir operations with the effective uid of the snap owner, so they only ever have their permissions anyway
- use mkdirat(2) instead of mkdir(2) to ensure that the directories are being created in the proper parent directory regardless of rename() or symlink() or mount(.., MS_BIND) tricks

There's pros and cons to each of these; the mkdirat() version may still require the reverse-order chown() calls too.

Thanks

[Tyler Hicks (tyhicks)]

Hi Kyle - The current MP isn't acceptable because it would allow an unprivileged user to create a path, owned by that user, anywhere in the filesystem by doing something like `SNAP_USER_DATA=/create/path/to/anywhere ubuntu-core-launcher ...`, where ... are valid arguments to ubuntu-core-launcher.

Does setup_user_data() have to be called before dropping user privileges?

review: Needs Fixing

[lp:~kyrofa/snap-confine/create_user_data_directory]

92. By Kyle Fazzari on 2016-02-09

Make the user data directory creation run after privileges are dropped.

[Kyle Fazzari (kyrofa)]

Oh my, what a glaring error that was!

Of course you're right Tyler-- the directory creation needs to happen before the apparmor/seccomp confinement, but should definitely happen after the privileges are dropped. My latest push reflects that.

Seth, I believe that actually addresses your concern as well, yes? Since now all mkdir operations are performed with the effective uid of whoever launched it?

[Kyle Fazzari (kyrofa)]

Note on the previous push I also updated the indentation to be consistent in the main() function. I'm sorry it makes for more to review, but it made it really difficult to conform when there was no conformity.

[Tyler Hicks (tyhicks)]

Thanks Kyle - This looks pretty good now but there's still one issue. Jamie tells me that snaps were granted control of the paths specified in SNAP_USER_DATA and SNAP_APP_USER_DATA_PATH, as well as SNAP_USER_DATA/../ and SNAP_APP_USER_DATA_PATH/../, because of the bug that you're trying to fix here. This change would happily follow symlinks that snaps had created in those locations since there's nothing to prevent the following of symlinks.

I'd suggest modifying the mkdir() loop to use openat() w/ O_NOFOLLOW and mkdirat(), as Seth originally suggested.

We'll need to update ubuntu-core-security, at the same time or after this change lands in the image, to remove access to those directories.

Yes, those white space changes sure make it more difficult to review! :)

review: Needs Fixing

[lp:~kyrofa/snap-confine/create_user_data_directory]

93. By Kyle Fazzari on 2016-02-09

Refactor to use open/openat/mkdirat instead of just mkdir.

[Kyle Fazzari (kyrofa)]

Alright Tyler, I've refactored the mkpath function to use open/openat/mkdirat with O_NOFOLLOW instead of mkdir. It complicated it a bit, but I think it's still readable. I also refactored the tests to be more readable.

[lp:~kyrofa/snap-confine/create_user_data_directory]

94. By Kyle Fazzari on 2016-02-09

Use strtok_r for mkpath tokenization.

[Tyler Hicks (tyhicks)]

Thanks for making those changes, Kyle. It all looks good to me now.

review: Approve

[lp:~kyrofa/snap-confine/create_user_data_directory]

95. By Kyle Fazzari on 2016-02-10

Include O_CLOEXEC in the open flags.

[Jamie Strandboge (jdstrand)]

This MP contains whitespace changes that make it difficult to review and is inconsistent with the current codebase. For example, it seems you went from 4 spaces to 3 spaces in main(). Can you update the MP to remove these whitespace changes?

Once that is done, I will sponsor the upload.

review: Needs Fixing

[Jamie Strandboge (jdstrand)]

Ok, on closer inspection I see that these whitespace changes are sorely needed. Looking at diff -Naur -wb, I see the non-whitespace changes and they are clear and fine. Thank you for the patch and for adding O_CLOEXEC.

review: Approve

[Jamie Strandboge (jdstrand)]

And for completeness, I've queued the ubuntu-core-security changes in http://bazaar.launchpad.net/~ubuntu-security/ubuntu-core-security/trunk/revision/193

=== modified file 'src/main.c'
--- src/main.c	2016-01-26 15:11:20 +0000
+++ src/main.c	2016-02-10 15:44:24 +0000
@@ -212,7 +212,7 @@
 
     // Create a 0700 base directory, this is the base dir that is
     // protected from other users.
-    // 
+    //
     // Under that basedir, we put a 1777 /tmp dir that is then bind
     // mounted for the applications to use
     must_snprintf(tmpdir, sizeof(tmpdir), "/tmp/snap.%d_%s_XXXXXX", uid, appname);
@@ -233,7 +233,7 @@
        die("unable to create /tmp inside private dir");
     }
     umask(old_mask);
-    
+
     // MS_BIND is there from linux 2.4
     if (mount(tmpdir, "/tmp", NULL, MS_BIND, NULL) != 0) {
         die("unable to bind private /tmp");
@@ -304,20 +304,108 @@
    if (unshare(CLONE_NEWNS) < 0) {
       die("unable to set up mount namespace");
    }
-   
+
    // make our "/" a rslave of the real "/". this means that
    // mounts from the host "/" get propagated to our namespace
    // (i.e. we see new media mounts)
    if (mount("none", "/", NULL, MS_REC|MS_SLAVE, NULL) != 0) {
       die("can not make make / rslave");
    }
-}   
+}
+
+void mkpath(const char *const path) {
+   // If asked to create an empty path, return immediately.
+   if (strlen(path) == 0) {
+      return;
+   }
+
+   // We're going to use strtok_r, which needs to modify the path, so we'll make
+   // a copy of it.
+   char *path_copy = strdup(path);
+   if (path_copy == NULL) {
+      die("failed to create user data directory");
+   }
+
+   // Open flags to use while we walk the user data path:
+   // - Don't follow symlinks
+   // - Don't allow child access to file descriptor
+   // - Only open a directory (fail otherwise)
+   int open_flags = O_NOFOLLOW | O_CLOEXEC | O_DIRECTORY;
+
+   // We're going to create each path segment via openat/mkdirat calls instead
+   // of mkdir calls, to avoid following symlinks and placing the user data
+   // directory somewhere we never intended for it to go. The first step is to
+   // get an initial file descriptor.
+   int fd = AT_FDCWD;
+   if (path_copy[0] == '/') {
+      fd = open("/", open_flags);
+      if (fd < 0) {
+         free(path_copy);
+         die("failed to create user data directory");
+      }
+   }
+
+   // strtok_r needs a pointer to keep track of where it is in the string.
+   char *path_walker;
+
+   // Initialize tokenizer and obtain first path segment.
+   char *path_segment = strtok_r(path_copy, "/", &path_walker);
+   while (path_segment) {
+      // Try to create the directory. It's okay if it already existed, but any
+      // other error is fatal.
+      if (mkdirat(fd, path_segment, 0755) < 0 && errno != EEXIST) {
+         close(fd);
+         free(path_copy);
+         die("failed to create user data directory");
+      }
+
+      // Open the parent directory we just made (and close the previous one) so
+      // we can continue down the path.
+      int previous_fd = fd;
+      fd = openat(fd, path_segment, open_flags);
+      close(previous_fd);
+      if (fd < 0) {
+         free(path_copy);
+         die("failed to create user data directory");
+      }
+
+      // Obtain the next path segment.
+      path_segment = strtok_r(NULL, "/", &path_walker);
+   }
+
+   // Close the descriptor for the final directory in the path.
+   close(fd);
+
+   free(path_copy);
+}
+
+void setup_user_data() {
+   const char *user_data = getenv("SNAP_USER_DATA");
+
+   // If $SNAP_USER_DATA wasn't defined, check the deprecated
+   // $SNAP_APP_USER_DATA_PATH.
+   if (user_data == NULL) {
+      user_data = getenv("SNAP_APP_USER_DATA_PATH");
+      // If it's still not defined, there's nothing to do. No need to die,
+      // there's simply no directory to create.
+      if (user_data == NULL) {
+         return;
+      }
+   }
+
+   // Only support absolute paths.
+   if (user_data[0] != '/') {
+      die("user data directory must be an absolute path");
+   }
+
+   mkpath(user_data);
+}
 
 int main(int argc, char **argv)
 {
    const int NR_ARGS = 3;
    if(argc < NR_ARGS+1)
-       die("Usage: %s <appname> <apparmor> <binary>", argv[0]);
+      die("Usage: %s <appname> <apparmor> <binary>", argv[0]);
 
    const char *appname = argv[1];
    const char *aa_profile = argv[2];
@@ -329,78 +417,80 @@
    // this code always needs to run as root for the cgroup/udev setup,
    // however for the tests we allow it to run as non-root
    if(geteuid() != 0 && getenv("UBUNTU_CORE_LAUNCHER_NO_ROOT") == NULL) {
-       die("need to run as root or suid");
+      die("need to run as root or suid");
    }
 
    if(geteuid() == 0) {
-       // verify binary path
-       char snaps_prefix[128];
-       must_snprintf(snaps_prefix, sizeof(snaps_prefix), "/snaps/%s/", appname);
-       if (strstr(binary, snaps_prefix) != binary)
-          die("binary must be inside /snaps/%s/", appname);
-
-       // ensure we run in our own slave mount namespace, this will
-       // create a new mount namespace and make it a slave of "/"
-       //
-       // Note that this means that no mount actions inside our
-       // namespace are propagated to the main "/". We need this
-       // both for the private /tmp we create and for the bind
-       // mounts we do on a classic ubuntu system
-       //
-       // This also means you can't run an automount daemon unter
-       // this launcher
-       setup_slave_mount_namespace();
-       
-       // do the mounting if run on a non-native snappy system
-       if(is_running_on_classic_ubuntu()) {
-          setup_snappy_os_mounts();
-       }
-      
-       // set up private mounts
-       setup_private_mount(appname);
-
-       // this needs to happen as root
-       if(snappy_udev_setup_required(appname)) {
-          setup_devices_cgroup(appname);
-          setup_udev_snappy_assign(appname);
-       }
-
-       // the rest does not so drop privs back to calling user
-       unsigned real_uid = getuid();
-       unsigned real_gid = getgid();
-
-       // Note that we do not call setgroups() here because its ok
-       // that the user keeps the groups he already belongs to
-       if (setgid(real_gid) != 0)
-          die("setgid failed");
-       if (setuid(real_uid) != 0)
-          die("setuid failed");
-
-       if(real_gid != 0 && (getuid() == 0 || geteuid() == 0))
-          die("dropping privs did not work");
-       if(real_uid != 0 && (getgid() == 0 || getegid() == 0))
-          die("dropping privs did not work");
-    }
-
-    //https://wiki.ubuntu.com/SecurityTeam/Specifications/SnappyConfinement#ubuntu-snapp-launch
-
-    int rc = 0;
-    // set apparmor rules
-    rc = aa_change_onexec(aa_profile);
-    if (rc != 0) {
-       if (getenv("SNAPPY_LAUNCHER_INSIDE_TESTS") == NULL)
-          die("aa_change_onexec failed with %i", rc);
-    }
-
-    // set seccomp
-    rc = seccomp_load_filters(aa_profile);
-    if (rc != 0)
-       die("seccomp_load_filters failed with %i", rc);
-
-    // and exec the new binary
-    argv[NR_ARGS] = (char*)binary,
-    execv(binary, (char *const*)&argv[NR_ARGS]);
-    perror("execv failed");
-    return 1;
+      // verify binary path
+      char snaps_prefix[128];
+      must_snprintf(snaps_prefix, sizeof(snaps_prefix), "/snaps/%s/", appname);
+      if (strstr(binary, snaps_prefix) != binary)
+         die("binary must be inside /snaps/%s/", appname);
+
+      // ensure we run in our own slave mount namespace, this will
+      // create a new mount namespace and make it a slave of "/"
+      //
+      // Note that this means that no mount actions inside our
+      // namespace are propagated to the main "/". We need this
+      // both for the private /tmp we create and for the bind
+      // mounts we do on a classic ubuntu system
+      //
+      // This also means you can't run an automount daemon unter
+      // this launcher
+      setup_slave_mount_namespace();
+
+      // do the mounting if run on a non-native snappy system
+      if(is_running_on_classic_ubuntu()) {
+         setup_snappy_os_mounts();
+      }
+
+      // set up private mounts
+      setup_private_mount(appname);
+
+      // this needs to happen as root
+      if(snappy_udev_setup_required(appname)) {
+         setup_devices_cgroup(appname);
+         setup_udev_snappy_assign(appname);
+      }
+
+      // the rest does not so drop privs back to calling user
+      unsigned real_uid = getuid();
+      unsigned real_gid = getgid();
+
+      // Note that we do not call setgroups() here because its ok
+      // that the user keeps the groups he already belongs to
+      if (setgid(real_gid) != 0)
+         die("setgid failed");
+      if (setuid(real_uid) != 0)
+         die("setuid failed");
+
+      if(real_gid != 0 && (getuid() == 0 || geteuid() == 0))
+         die("dropping privs did not work");
+      if(real_uid != 0 && (getgid() == 0 || getegid() == 0))
+         die("dropping privs did not work");
+   }
+
+   // Ensure that the user data path exists.
+   setup_user_data();
+
+   //https://wiki.ubuntu.com/SecurityTeam/Specifications/SnappyConfinement#ubuntu-snapp-launch
+
+   int rc = 0;
+   // set apparmor rules
+   rc = aa_change_onexec(aa_profile);
+   if (rc != 0) {
+   if (getenv("SNAPPY_LAUNCHER_INSIDE_TESTS") == NULL)
+      die("aa_change_onexec failed with %i", rc);
+   }
+
+   // set seccomp
+   rc = seccomp_load_filters(aa_profile);
+   if (rc != 0)
+      die("seccomp_load_filters failed with %i", rc);
+
+   // and exec the new binary
+   argv[NR_ARGS] = (char*)binary,
+   execv(binary, (char *const*)&argv[NR_ARGS]);
+   perror("execv failed");
+   return 1;
 }
-

=== added file 'src/ubuntu-core-launcher'
Binary files src/ubuntu-core-launcher	1970-01-01 00:00:00 +0000 and src/ubuntu-core-launcher	2016-02-10 15:44:24 +0000 differ
=== added file 'tests/test_create_user_data'
--- tests/test_create_user_data	1970-01-01 00:00:00 +0000
+++ tests/test_create_user_data	2016-02-10 15:44:24 +0000
@@ -0,0 +1,162 @@
+#!/bin/sh
+
+set -e
+
+. $(pwd)/common.sh
+
+cat >$TMP/unrestricted <<EOF
+# some comment
+@unrestricted
+EOF
+
+cat >$TMP/restricted <<EOF
+# filter that works ok for true
+open
+close
+
+mmap
+munmap
+mprotect
+
+fstat
+access
+read
+
+brk
+execve
+
+arch_prctl
+exit_group
+
+# unknown syscalls are ignore
+i-dont-exit
+EOF
+
+# $1: Path to check existence for and potentially remove at the end
+# $2: Profile name
+# $3: True if success is expected, false otherwise
+run_launcher() {
+   pass=false
+   if $L appid $2 /bin/true 2>/dev/null; then
+      if $3; then
+         if [ ! -d "$1" ]; then
+            pass=false
+         fi
+
+         pass=true
+      else
+         pass=false
+      fi
+   else
+      if $3; then
+         pass=false
+      else
+         pass=true
+      fi
+   fi
+
+   if [ -d "$1" ]; then
+      rmdir $1
+   fi
+
+   if $pass; then
+      return 0
+   else
+      return 1
+   fi
+}
+
+# $1 = $SNAP_USER_DATA definition
+# $2 = Profile name
+# $3 = True if success is expected, false otherwise
+run_current() {
+   export SNAP_USER_DATA=$1
+   run_launcher $1 $2 $3
+   pass=$?
+   unset SNAP_USER_DATA
+
+   return $pass
+}
+
+# $1 = $SNAP_APP_USER_DATA_PATH definition
+# $2 = Profile name
+# $3 = True if success is expected, false otherwise
+run_deprecated() {
+   export SNAP_APP_USER_DATA_PATH=$1
+   run_launcher $1 $2 $3
+   pass=$?
+   unset SNAP_APP_USER_DATA_PATH
+
+   return $pass
+}
+
+# $1 = User data path
+# $2 = Profile name
+# $3 = True if success is expected, false otherwise
+run_both() {
+   run_current $1 $2 $3
+   current_pass=$?
+   run_deprecated $1 $2 $3
+   deprecated_pass=$?
+
+   if [ $current_pass -a $deprecated_pass ]; then
+      PASS
+   else
+      FAIL
+   fi
+}
+
+printf "Test that an unrestricted launcher creates user data"
+run_both $TMP/user_data unrestricted true
+
+printf "Test that a restricted launcher creates user data"
+run_both $TMP/user_data restricted true
+
+printf "Test that an unrestricted launcher creates user data with parent directory"
+run_both $TMP/parent/user_data unrestricted true
+
+printf "Test that a restricted launcher creates user data with parent directory"
+run_both $TMP/parent/user_data restricted true
+
+printf "Test that user data can contain multiple path separators"
+run_both $TMP//user_data unrestricted true
+
+printf "Test that user data must be absolute"
+run_both "../foo" unrestricted false
+
+printf "Testing that user data cannot be contained within a symlink"
+mkdir $TMP/nefarious_parent
+ln -s $TMP/nefarious_parent $TMP/parent
+run_both $TMP/parent/user_data unrestricted, false
+
+printf "Test that an unrestricted launcher works when user data exists (current)"
+mkdir $TMP/user_data
+if run_current $TMP/user_data unrestricted true; then
+   PASS
+else
+   FAIL
+fi
+
+printf "Test that an restricted launcher works when user data exists (current)"
+mkdir $TMP/user_data
+if run_current $TMP/user_data restricted true; then
+   PASS
+else
+   FAIL
+fi
+
+printf "Test that an unrestricted launcher works when user data exists (deprecated)"
+mkdir $TMP/user_data
+if run_deprecated $TMP/user_data unrestricted true; then
+   PASS
+else
+   FAIL
+fi
+
+printf "Test that an restricted launcher works when user data exists (deprecated)"
+mkdir $TMP/user_data
+if run_deprecated $TMP/user_data restricted true; then
+   PASS
+else
+   FAIL
+fi