Merge lp:~jdstrand/snap-confine/fix-udev-for-1604 into lp:~snappy-dev/snap-confine/trunk

[Jamie Strandboge]

  * update cgroup handling for 16.04 (LP: #1564401):
    - debian/usr.bin.ubuntu-core-launcher:
      + allow creating cgroups with snap.*
      + allow ixr of 'tr'
      + remove access to /var/lib/apparmor/clicks/
    - update README to more fully explain the cgroups implementation
    - src/80-snappy-assign.rules: append an app-specific tag instead of
      adding a generic tag and snap-specific property
    - src/snappy-app-dev: convert the new tag to the directory name
    - src/main.c:
      + refactor and simplify control flow to query udev for device assignment
        instead of searching apparmor policy for a specific string
      + adjust udev query for app-specific tag
      + raise real_uid after fork() before calling /lib/udev/snappy-app-dev
        so non-root app launches work with the device cgroup

[lp:~jdstrand/snap-confine/fix-udev-for-1604]

123. By Jamie Strandboge on 2016-04-06

address typo pointed out be Leo

[Jamie Strandboge (jdstrand)]

Thanks Leo, pushed in r123.

[Tyler Hicks (tyhicks)]

See inline comments.

review: Needs Fixing

[Tyler Hicks (tyhicks)]

I'll also note that, due to release deadlines, I did not have sufficient time to review the greater design changes that are occurring due to this MP. I really only had a chance to review the code changes.

[lp:~jdstrand/snap-confine/fix-udev-for-1604]

124. By Jamie Strandboge on 2016-04-14

merge from trunk

[Jamie Strandboge (jdstrand)]

>> +const char *static_devices[] = {
>
> I may be missing something but I don't see why this array was lifted from the function that uses it and turned into a global. If it is to remain as global, it should be declared as "static const char *static_devices[]" since it isn't used outside of main.c.

Ah, yes, this should have been moved back down after I made the code simpler.

[Jamie Strandboge (jdstrand)]

> This string test is incomplete for a couple reasons:
>
> 1) tagname_len could be greater than the size of the tagname buffer
> 2) the tagname buffer could be unterminated

True. I was thinking about the must_snprintf() that was called to set it would have taken care of this, but if we are doing all these checks for future-proof coding, we should do all the checks.

[Jamie Strandboge (jdstrand)]

>> Is it intentional that we don't attempt to setuid(0) (or fail?) if the first conditional is not met? It seems correct to continue on if real_uid is 0 but what about when effective_uid is not 0? I guess snappy-app-dev will simply fail due to insufficient perms and everything will be ok.

Yes, it will fail due to insufficient perms and yes this was intentional. The idea here is to raise if we can but not if we can't. The testsuite will have euid as non-zero, under sudo real_uid and euid are 0, and under suid launcher euid is 0 but real_uid is not.

[lp:~jdstrand/snap-confine/fix-udev-for-1604]

125. By Jamie Strandboge on 2016-04-14

move static_devices into setup_devices_cgroup(), where it should be

126. By Jamie Strandboge on 2016-04-14

src/main.c: further futreproof our tagname checks. Patch thanks to Tyler Hicks

127. By Jamie Strandboge on 2016-04-14

src/main.c: simplify execl() failure

128. By Jamie Strandboge on 2016-04-14

use __func__ in debug() statements

[Jamie Strandboge (jdstrand)]

Thanks for the review!

=== modified file 'README'
--- README	2016-04-13 21:10:41 +0000
+++ README	2016-04-14 23:03:26 +0000
@@ -46,9 +46,32 @@
 == devices cgroup ==
 
 It works like this:
-- on install of snaps with a special hardware:\n assign yaml udev rules are generated that add tags and properties to the matching hardware
-- this launcher creates a device cgroup that is deny-all by default, then adds itself to the group and also adds all devices that match the given snappy-assign tag and the appname property
-- the app is executed and now the normal device permissions/apparmor rules apply
+- on install of snaps with a special hardware: assign yaml udev rules are
+  generated that add tags to matching hardware. These assign rules are added to
+  udev via /etc/udev/rules.d/70-snap.... for each app within a snap. The tags
+  are of the form 'snap_<snap name>_<app>'.
+- when an application is launched, the launcher queries udev to detect if any
+  devices are tagged for this application. If no devices are tagged for this
+  application, a device cgroup is not setup
+- if there are devices tagged for this application, the launcher creates a
+  device cgroup in /sys/fs/cgroups/devices/snap.<snap name>.<app> and adds
+  itself to this cgroup. It then sets the cgroup as deny-all by default, adds
+  some common devices (eg, /dev/null, /dev/zero, etc) and any devices tagged
+  for use by this application using /lib/udev/snappy-app-dev
+- the app is executed and now the normal device permissions/apparmor rules
+  apply
+- udev match rules in /lib/udev/rules.d/80-snapy-assign.rules are in place to
+  run /lib/udev/snappy-app-dev to handle device events for devices tagged with
+  snap_*.
+
+Note, /sys/fs/cgroups/devices/snap.<snap name>.<app> is not (currently) removed
+on unassignment and the contents of the cgroup for the app are managed entirely
+by the launcher. When an application is started, the cgroup is reset by
+removing all previously added devices and then the list of assigned devices is
+built back up before launch. In this manner, devices can be assigned, changed,
+and unassigned and the app will always get the correct device added to the
+cgroup, but what is in /sys/fs/cgroups/devices/snap.<snap name>.<app> will not
+reflect assignment/unassignment until after the application is started.
 
 
 == private /tmp ==

=== modified file 'debian/changelog'
--- debian/changelog	2016-04-14 22:18:12 +0000
+++ debian/changelog	2016-04-14 23:03:26 +0000
@@ -1,3 +1,24 @@
+ubuntu-core-launcher (1.0.25) UNRELEASED; urgency=medium
+
+  * update cgroup handling for 16.04 (LP: #1564401):
+    - debian/usr.bin.ubuntu-core-launcher:
+      + allow creating cgroups with snap.*
+      + allow ixr of 'tr'
+      + remove access to /var/lib/apparmor/clicks/
+    - update README to more fully explain the cgroups implementation
+    - src/80-snappy-assign.rules: append an app-specific tag instead of
+      adding a generic tag and snap-specific property
+    - src/snappy-app-dev: convert the new tag to the directory name
+    - src/main.c:
+      + refactor and simplify control flow to query udev for device assignment
+        instead of searching apparmor policy for a specific string
+      + adjust udev query for app-specific tag
+      + raise real_uid after fork() before calling /lib/udev/snappy-app-dev
+        so non-root app launches work with the device cgroup
+
+
+ -- Jamie Strandboge <[e-mail]>  Thu, 14 Apr 2016 17:18:54 -0500
+
 ubuntu-core-launcher (1.0.24) xenial; urgency=medium
 
   [ Michael Vogt ]

=== modified file 'debian/usr.bin.ubuntu-core-launcher'
--- debian/usr.bin.ubuntu-core-launcher	2016-04-11 23:10:01 +0000
+++ debian/usr.bin.ubuntu-core-launcher	2016-04-14 23:03:26 +0000
@@ -26,15 +26,16 @@
     # cgroups
     capability sys_admin,
     capability dac_override,
-    /sys/fs/cgroup/devices/snappy.*/ w,
-    /sys/fs/cgroup/devices/snappy.*/tasks w,
-    /sys/fs/cgroup/devices/snappy.*/devices.{allow,deny} w,
+    /sys/fs/cgroup/devices/snap{,py}.*/ w,
+    /sys/fs/cgroup/devices/snap{,py}.*/tasks w,
+    /sys/fs/cgroup/devices/snap{,py}.*/devices.{allow,deny} w,
 
     # querying udev
     /etc/udev/udev.conf r,
     /sys/devices/**/uevent r,
     /lib/udev/snappy-app-dev ixr, # drop
     /run/udev/** rw,
+    /{,usr/}bin/tr ixr,
 
     # priv dropping
     capability setuid,
@@ -66,9 +67,6 @@
     # reading seccomp filters
     /var/lib/snapd/seccomp/profiles/* r,
 
-    # 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,

=== modified file 'src/80-snappy-assign.rules'
--- src/80-snappy-assign.rules	2015-04-18 14:54:33 +0000
+++ src/80-snappy-assign.rules	2016-04-14 23:03:26 +0000
@@ -1,2 +1,2 @@
 # add/remove snap package access to assigned devices
-TAG=="snappy-assign", RUN+="/lib/udev/snappy-app-dev $env{ACTION} $env{SNAPPY_APP} $devpath $major:$minor"
+TAG=="snap_*", RUN+="/lib/udev/snappy-app-dev $env{ACTION} $env{TAG} $devpath $major:$minor"

=== modified file 'src/main.c'
--- src/main.c	2016-04-14 20:47:41 +0000
+++ src/main.c	2016-04-14 23:03:26 +0000
@@ -38,6 +38,8 @@
 #include <fcntl.h>
 #include <glob.h>
 
+#include <ctype.h>
+
 #include "libudev.h"
 
 #include "utils.h"
@@ -45,6 +47,14 @@
 
 #define MAX_BUF 1000
 
+struct snappy_udev {
+	struct udev *udev;
+	struct udev_enumerate *devices;
+	struct udev_list_entry *assigned;
+	char tagname[MAX_BUF];
+	size_t tagname_len;
+};
+
 bool verify_appname(const char *appname)
 {
 	// these chars are allowed in a appname
@@ -59,11 +69,22 @@
 	return (status == 0);
 }
 
-void run_snappy_app_dev_add(struct udev *u, const char *path,
-			    const char *appname)
+void run_snappy_app_dev_add(struct snappy_udev *udev_s, const char *path)
 {
-	debug("run_snappy_app_dev_add: %s %s", path, appname);
-	struct udev_device *d = udev_device_new_from_syspath(u, path);
+	if (udev_s == NULL)
+		die("snappy_udev is NULL");
+	if (udev_s->udev == NULL)
+		die("snappy_udev->udev is NULL");
+	if (udev_s->tagname_len == 0
+	    || udev_s->tagname_len >= MAX_BUF
+	    || strnlen(udev_s->tagname, MAX_BUF) != udev_s->tagname_len
+	    || udev_s->tagname[udev_s->tagname_len] != '\0')
+		die("snappy_udev->tagname has invalid length");
+
+	debug("%s: %s %s", __func__, path, udev_s->tagname);
+
+	struct udev_device *d =
+	    udev_device_new_from_syspath(udev_s->udev, path);
 	if (d == NULL)
 		die("can not find %s", path);
 	dev_t devnum = udev_device_get_devnum(d);
@@ -75,14 +96,21 @@
 		die("could not fork");
 	}
 	if (pid == 0) {
+		uid_t real_uid, effective_uid, saved_uid;
+		if (getresuid(&real_uid, &effective_uid, &saved_uid) != 0)
+			die("could not find user IDs");
+		// can't update the cgroup unless the real_uid is 0, euid as
+		// 0 is not enough
+		if (real_uid != 0 && effective_uid == 0)
+			if (setuid(0) != 0)
+				die("setuid failed");
 		char buf[64];
 		unsigned major = MAJOR(devnum);
 		unsigned minor = MINOR(devnum);
 		must_snprintf(buf, sizeof(buf), "%u:%u", major, minor);
-		if (execl
-		    ("/lib/udev/snappy-app-dev", "/lib/udev/snappy-app-dev",
-		     "add", appname, path, buf, NULL) != 0)
-			die("execlp failed");
+		execl("/lib/udev/snappy-app-dev", "/lib/udev/snappy-app-dev",
+		      "add", udev_s->tagname, path, buf, NULL);
+		die("execl failed");
 	}
 	if (waitpid(pid, &status, 0) < 0)
 		die("waitpid failed");
@@ -92,14 +120,64 @@
 		die("child died with signal %i", WTERMSIG(status));
 }
 
-void setup_udev_snappy_assign(const char *appname)
+/*
+ * snappy_udev_init() - setup the snappy_udev structure. Return 0 if devices
+ * are assigned, else return -1. Callers should use snappy_udev_cleanup() to
+ * cleanup.
+ */
+int snappy_udev_init(const char *appname, struct snappy_udev *udev_s)
 {
-	debug("setup_udev_snappy_assign");
-
-	struct udev *u = udev_new();
-	if (u == NULL)
+	debug("%s", __func__);
+	int rc = 0;
+
+	// extra paranoia
+	if (!verify_appname(appname))
+		die("appname %s not allowed", appname);
+
+	udev_s->tagname[0] = '\0';
+	udev_s->tagname_len = 0;
+	// TAG+="snap_<appname>" (udev doesn't like '.' in the tag name)
+	udev_s->tagname_len = must_snprintf(udev_s->tagname, MAX_BUF,
+					    "snap_%s", appname);
+	for (int i = 0; i < udev_s->tagname_len; i++)
+		if (udev_s->tagname[i] == '.')
+			udev_s->tagname[i] = '_';
+
+	udev_s->udev = udev_new();
+	if (udev_s->udev == NULL)
 		die("udev_new failed");
 
+	udev_s->devices = udev_enumerate_new(udev_s->udev);
+	if (udev_s->devices == NULL)
+		die("udev_enumerate_new failed");
+
+	if (udev_enumerate_add_match_tag(udev_s->devices, udev_s->tagname) != 0)
+		die("udev_enumerate_add_match_tag");
+
+	if (udev_enumerate_scan_devices(udev_s->devices) != 0)
+		die("udev_enumerate_scan failed");
+
+	udev_s->assigned = udev_enumerate_get_list_entry(udev_s->devices);
+	if (udev_s->assigned == NULL)
+		rc = -1;
+
+	return rc;
+}
+
+void snappy_udev_cleanup(struct snappy_udev *udev_s)
+{
+	// udev_s->assigned does not need to be unreferenced since it is a
+	// pointer into udev_s->devices
+	if (udev_s->devices != NULL)
+		udev_enumerate_unref(udev_s->devices);
+	if (udev_s->udev != NULL)
+		udev_unref(udev_s->udev);
+}
+
+void setup_devices_cgroup(const char *appname, struct snappy_udev *udev_s)
+{
+	debug("%s", __func__);
+	// Devices that must always be present
 	const char *static_devices[] = {
 		"/sys/class/mem/null",
 		"/sys/class/mem/full",
@@ -111,50 +189,29 @@
 		"/sys/class/tty/ptmx",
 		NULL,
 	};
-	int i;
-	for (i = 0; static_devices[i] != NULL; i++) {
-		run_snappy_app_dev_add(u, static_devices[i], appname);
-	}
-
-	struct udev_enumerate *devices = udev_enumerate_new(u);
-	if (devices == NULL)
-		die("udev_enumerate_new failed");
-
-	if (udev_enumerate_add_match_tag(devices, "snappy-assign") != 0)
-		die("udev_enumerate_add_match_tag");
-
-	if (udev_enumerate_add_match_property(devices, "SNAPPY_APP", appname) !=
-	    0)
-		die("udev_enumerate_add_match_property");
-
-	if (udev_enumerate_scan_devices(devices) != 0)
-		die("udev_enumerate_scan failed");
-
-	struct udev_list_entry *l = udev_enumerate_get_list_entry(devices);
-	while (l != NULL) {
-		const char *path = udev_list_entry_get_name(l);
-		if (path == NULL)
-			die("udev_list_entry_get_name failed");
-		run_snappy_app_dev_add(u, path, appname);
-		l = udev_list_entry_get_next(l);
-	}
-
-	udev_enumerate_unref(devices);
-	udev_unref(u);
-}
-
-void setup_devices_cgroup(const char *appname)
-{
-	debug("setup_devices_cgroup");
 
 	// extra paranoia
 	if (!verify_appname(appname))
 		die("appname %s not allowed", appname);
+	if (udev_s == NULL)
+		die("snappy_udev is NULL");
+	if (udev_s->udev == NULL)
+		die("snappy_udev->udev is NULL");
+	if (udev_s->devices == NULL)
+		die("snappy_udev->devices is NULL");
+	if (udev_s->assigned == NULL)
+		die("snappy_udev->assigned is NULL");
+	if (udev_s->tagname_len == 0
+	    || udev_s->tagname_len >= MAX_BUF
+	    || strnlen(udev_s->tagname, MAX_BUF) != udev_s->tagname_len
+	    || udev_s->tagname[udev_s->tagname_len] != '\0')
+		die("snappy_udev->tagname has invalid length");
 
 	// create devices cgroup controller
 	char cgroup_dir[PATH_MAX];
+
 	must_snprintf(cgroup_dir, sizeof(cgroup_dir),
-		      "/sys/fs/cgroup/devices/snappy.%s/", appname);
+		      "/sys/fs/cgroup/devices/snap.%s/", appname);
 
 	if (mkdir(cgroup_dir, 0755) < 0 && errno != EEXIST)
 		die("mkdir failed");
@@ -168,55 +225,26 @@
 	must_snprintf(buf, sizeof(buf), "%i", getpid());
 	write_string_to_file(cgroup_file, buf);
 
-	// deny by default
+	// deny by default. Write 'a' to devices.deny to remove all existing
+	// devices that were added in previous launcher invocations, then add
+	// the static and assigned devices. This ensures that at application
+	// launch the cgroup only has what is currently assigned.
 	must_snprintf(cgroup_file, sizeof(cgroup_file), "%s%s", cgroup_dir,
 		      "devices.deny");
 	write_string_to_file(cgroup_file, "a");
 
-}
-
-bool snappy_udev_setup_required(const char *appname)
-{
-	debug("snappy_udev_setup_required");
-
-	// extra paranoia
-	if (!verify_appname(appname))
-		die("appname %s not allowed", appname);
-
-	char override_file[PATH_MAX];
-	must_snprintf(override_file, sizeof(override_file),
-		      "/var/lib/apparmor/clicks/%s.json.additional", appname);
-
-	// if a snap package gets unrestricted apparmor access we need to setup
-	// a device cgroup.
-	//
-	// the "needle" string is what gives this access so we search for that
-	// here
-	const char *needle =
-	    "{" "\n"
-	    " \"write_path\": [" "\n"
-	    "   \"/dev/**\"" "\n"
-	    " ]," "\n"
-	    " \"read_path\": [" "\n" "   \"/run/udev/data/*\"" "\n" " ]\n" "}";
-	debug("looking for: '%s'", needle);
-	char content[strlen(needle)];
-
-	int fd = open(override_file, O_CLOEXEC | O_NOFOLLOW | O_RDONLY);
-	if (fd < 0)
-		return false;
-	int n = read(fd, content, sizeof(content));
-	if (close(fd) != 0)
-		die("could not close override");
-	if (n < sizeof(content))
-		return false;
-
-	// memcpy so that we don't have to deal with \0 in the input
-	if (memcmp(content, needle, strlen(needle)) == 0) {
-		debug("found needle, need to apply udev setup");
-		return true;
+	// add the common devices
+	for (int i = 0; static_devices[i] != NULL; i++)
+		run_snappy_app_dev_add(udev_s, static_devices[i]);
+
+	// add the assigned devices
+	while (udev_s->assigned != NULL) {
+		const char *path = udev_list_entry_get_name(udev_s->assigned);
+		if (path == NULL)
+			die("udev_list_entry_get_name failed");
+		run_snappy_app_dev_add(udev_s, path);
+		udev_s->assigned = udev_list_entry_get_next(udev_s->assigned);
 	}
-
-	return false;
 }
 
 bool is_running_on_classic_ubuntu()
@@ -313,7 +341,7 @@
 
 void setup_snappy_os_mounts()
 {
-	debug("setup_snappy_os_mounts()\n");
+	debug("%s", __func__);
 
 	// FIXME: hardcoded "ubuntu-core.*"
 	glob_t glob_res;
@@ -503,10 +531,11 @@
 		setup_private_pts();
 
 		// this needs to happen as root
-		if (snappy_udev_setup_required(appname)) {
-			setup_devices_cgroup(appname);
-			setup_udev_snappy_assign(appname);
-		}
+		struct snappy_udev udev_s;
+		if (snappy_udev_init(appname, &udev_s) == 0)
+			setup_devices_cgroup(appname, &udev_s);
+		snappy_udev_cleanup(&udev_s);
+
 		// the rest does not so temporarily drop privs back to calling
 		// user (we'll permanently drop after loading seccomp)
 		if (setegid(real_gid) != 0)

=== modified file 'src/snappy-app-dev'
--- src/snappy-app-dev	2015-04-17 22:23:37 +0000
+++ src/snappy-app-dev	2016-04-14 23:03:26 +0000
@@ -15,7 +15,8 @@
 [ -n "$DEVPATH" ] || { echo "no devpath given" >&2; exit 1; }
 [ -n "$MAJMIN" ] || { echo "no major/minor given" >&2; exit 0; }
 
-app_dev_cgroup="/sys/fs/cgroup/devices/snappy.$APPNAME"
+APPNAME=`echo $APPNAME | tr '_' '.'`
+app_dev_cgroup="/sys/fs/cgroup/devices/$APPNAME"
 
 # check if it's a block or char dev
 if [ "${DEVPATH#*/block/}" != "$DEVPATH" ]; then