Merge remote-tracking branch 'mvo5-private/release-2.54.3'

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

cmd/libsnap-confine-private/apparmor-support.c

@@ -20,6 +20,8 @@
 #endif
 
 #include "apparmor-support.h"
+#include "string-utils.h"
+#include "utils.h"
 
 #include <string.h>
 #include <errno.h>
@@ -53,18 +55,24 @@ void sc_init_apparmor_support(struct sc_apparmor *apparmor)
 			debug
 			    ("apparmor is available on the system but has been disabled at boot");
 			break;
-		case ENOENT:
-			debug
-			    ("apparmor is available but the interface but the interface is not available");
-			break;
 		case EPERM:
 			// NOTE: fall-through
 		case EACCES:
 			debug
 			    ("insufficient permissions to determine if apparmor is enabled");
-			break;
+			// since snap-confine is setuid root this should
+			// never happen so likely someone is trying to
+			// manipulate our execution environment - fail hard
+
+			// fall-through
+		case ENOENT:
+		case ENOMEM:
 		default:
-			debug("apparmor is not enabled: %s", strerror(errno));
+			// this shouldn't happen under normal usage so it
+			// is possible someone is trying to manipulate our
+			// execution environment - fail hard
+			die("aa_is_enabled() failed unexpectedly (%s)",
+			    strerror(errno));
 			break;
 		}
 		apparmor->is_confined = false;
@@ -81,13 +89,13 @@ void sc_init_apparmor_support(struct sc_apparmor *apparmor)
 	}
 	debug("apparmor label on snap-confine is: %s", label);
 	debug("apparmor mode is: %s", mode);
-	// The label has a special value "unconfined" that is applied to all
-	// processes without a dedicated profile. If that label is used then the
-	// current process is not confined. All other labels imply confinement.
-	if (label != NULL && strcmp(label, SC_AA_UNCONFINED_STR) == 0) {
-		apparmor->is_confined = false;
-	} else {
+	// expect to be confined by a profile with the name of a valid
+	// snap-confine binary since if not we may be executed under a
+	// profile with more permissions than expected
+	if (label != NULL && sc_streq(mode, SC_AA_ENFORCE_STR) && sc_is_expected_path(label)) {
 		apparmor->is_confined = true;
+	} else {
+		apparmor->is_confined = false;
 	}
 	// There are several possible results for the confinement type (mode) that
 	// are checked for below.

cmd/libsnap-confine-private/tool.c

@@ -110,7 +110,7 @@ void sc_call_snap_update_ns_as_user(int snap_update_ns_fd,
 			 snap_name);
 
 	const char *xdg_runtime_dir = getenv("XDG_RUNTIME_DIR");
-	char xdg_runtime_dir_env[PATH_MAX + strlen("XDG_RUNTIME_DIR=")];
+	char xdg_runtime_dir_env[PATH_MAX + sizeof("XDG_RUNTIME_DIR=")] = { 0 };
 	if (xdg_runtime_dir != NULL) {
 		sc_must_snprintf(xdg_runtime_dir_env,
 				 sizeof(xdg_runtime_dir_env),
@@ -163,14 +163,21 @@ static int sc_open_snapd_tool(const char *tool_name)
 	// want to store the terminating '\0'. The readlink system call doesn't add
 	// terminating null, but our initialization of buf handles this for us.
 	char buf[PATH_MAX + 1] = { 0 };
-	if (readlink("/proc/self/exe", buf, sizeof buf) < 0) {
+	if (readlink("/proc/self/exe", buf, sizeof(buf) - 1) < 0) {
 		die("cannot readlink /proc/self/exe");
 	}
 	if (buf[0] != '/') {	// this shouldn't happen, but make sure have absolute path
 		die("readlink /proc/self/exe returned relative path");
 	}
+	// as we are looking up other tools relative to our own path, check
+	// we are located where we think we should be - otherwise we
+	// may have been hardlink'd elsewhere and then may execute the
+	// wrong tool as a result
+	if (!sc_is_expected_path(buf)) {
+		die("running from unexpected location: %s", buf);
+	}
 	char *dir_name = dirname(buf);
-	int dir_fd SC_CLEANUP(sc_cleanup_close) = 1;
+	int dir_fd SC_CLEANUP(sc_cleanup_close) = -1;
 	dir_fd = open(dir_name, O_PATH | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC);
 	if (dir_fd < 0) {
 		die("cannot open path %s", dir_name);

cmd/libsnap-confine-private/utils-test.c

@@ -71,6 +71,37 @@ static void test_parse_bool(void)
 	g_assert_cmpint(errno, ==, EFAULT);
 }
 
+static void test_sc_is_expected_path(void)
+{
+	struct {
+		const char *path;
+		bool expected;
+	} test_cases[] = {
+		{"/tmp/snap-confine", false},
+		{"/tmp/foo", false},
+		{"/home/ ", false},
+		{"/usr/lib/snapd/snap-confine1", false},
+		{"/usr/lib/snapd/snap—confine", false},
+		{"/snap/core/usr/lib/snapd/snap-confine", false},
+		{"/snap/core/x1x/usr/lib/snapd/snap-confine", false},
+		{"/snap/core/z1/usr/lib/snapd/snap-confine", false},
+		{"/snap/cꓳre/1/usr/lib/snapd/snap-confine", false},
+		{"/snap/snapd1/1/usr/lib/snapd/snap-confine", false},
+		{"/snap/core/current/usr/lib/snapd/snap-confine", false},
+		{"/usr/lib/snapd/snap-confine", true},
+		{"/usr/libexec/snapd/snap-confine", true},
+		{"/snap/core/1/usr/lib/snapd/snap-confine", true},
+		{"/snap/core/x1/usr/lib/snapd/snap-confine", true},
+		{"/snap/snapd/1/usr/lib/snapd/snap-confine", true},
+		{"/snap/snapd/1/usr/libexec/snapd/snap-confine", false},
+	};
+	size_t i;
+	for (i = 0; i < sizeof(test_cases) / sizeof(test_cases[0]); i++) {
+		bool result = sc_is_expected_path(test_cases[i].path);
+		g_assert_cmpint(result, ==, test_cases[i].expected);
+	}
+}
+
 static void test_die(void)
 {
 	if (g_test_subprocess()) {
@@ -194,6 +225,7 @@ static void test_sc_nonfatal_mkpath__absolute(void)
 static void __attribute__((constructor)) init(void)
 {
 	g_test_add_func("/utils/parse_bool", test_parse_bool);
+	g_test_add_func("/utils/sc_is_expected_path", test_sc_is_expected_path);
 	g_test_add_func("/utils/die", test_die);
 	g_test_add_func("/utils/die_with_errno", test_die_with_errno);
 	g_test_add_func("/utils/sc_nonfatal_mkpath/relative",

cmd/libsnap-confine-private/utils.c

@@ -16,6 +16,7 @@
  */
 #include <errno.h>
 #include <fcntl.h>
+#include <regex.h>
 #include <stdarg.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -237,3 +238,15 @@ int sc_nonfatal_mkpath(const char *const path, mode_t mode)
 	}
 	return 0;
 }
+
+bool sc_is_expected_path(const char *path)
+{
+	const char *expected_path_re =
+	    "^(/snap/(snapd|core)/x?[0-9]+/usr/lib|/usr/lib(exec)?)/snapd/snap-confine$";
+	regex_t re;
+	if (regcomp(&re, expected_path_re, REG_EXTENDED | REG_NOSUB) != 0)
+		die("can not compile regex %s", expected_path_re);
+	int status = regexec(&re, path, 0, NULL, 0);
+	regfree(&re);
+	return status == 0;
+}

cmd/libsnap-confine-private/utils.h

@@ -111,4 +111,10 @@ void write_string_to_file(const char *filepath, const char *buf);
  **/
 __attribute__((warn_unused_result))
 int sc_nonfatal_mkpath(const char *const path, mode_t mode);
+
+/**
+ * Return true if path is a valid path for the snap-confine binary
+ **/
+__attribute__((warn_unused_result))
+bool sc_is_expected_path(const char *path);
 #endif

cmd/snap-confine/mount-support-test.c

@@ -21,6 +21,7 @@
 #include "mount-support-nvidia.c"
 
 #include <glib.h>
+#include <glib/gstdio.h>
 
 static void replace_slashes_with_NUL(char *path, size_t len)
 {
@@ -91,10 +92,50 @@ static void test_is_subdir(void)
 	g_assert_false(is_subdir("/", ""));
 }
 
+static void test_must_mkdir_and_open_with_perms(void)
+{
+	// make a directory with some contents and check we can
+	// must_mkdir_and_open_with_perms() to get control of it
+	GError *error = NULL;
+	GStatBuf st;
+	gchar *test_dir = g_dir_make_tmp("test-mkdir-XXXXXX", &error);
+	g_assert_no_error(error);
+	g_assert_nonnull(test_dir);
+	g_assert_cmpint(chmod(test_dir, 0755), ==, 0);
+	g_assert_true(g_file_test
+		      (test_dir, G_FILE_TEST_EXISTS | G_FILE_TEST_IS_DIR));
+	g_assert_cmpint(g_stat(test_dir, &st), ==, 0);
+	g_assert_true(st.st_uid == getuid());
+	g_assert_true(st.st_gid == getgid());
+	g_assert_true(st.st_mode == (S_IFDIR | 0755));
+
+	gchar *test_subdir = g_build_filename(test_dir, "foo", NULL);
+	g_assert_cmpint(g_mkdir_with_parents(test_dir, 0755), ==, 0);
+	g_file_test(test_subdir, G_FILE_TEST_EXISTS | G_FILE_TEST_IS_DIR);
+
+	// take over dir
+	int fd =
+	    must_mkdir_and_open_with_perms(test_dir, getuid(), getgid(), 0700);
+	// check can unlink dir itself with no contents successfully and it
+	// still exists
+	g_assert_cmpint(fd, >=, 0);
+	g_assert_false(g_file_test
+		       (test_subdir, G_FILE_TEST_EXISTS | G_FILE_TEST_IS_DIR));
+	g_assert_true(g_file_test
+		      (test_dir, G_FILE_TEST_EXISTS | G_FILE_TEST_IS_DIR));
+	g_assert_cmpint(g_stat(test_dir, &st), ==, 0);
+	g_assert_true(st.st_uid == getuid());
+	g_assert_true(st.st_gid == getgid());
+	g_assert_true(st.st_mode == (S_IFDIR | 0700));
+	close(fd);
+}
+
 static void __attribute__((constructor)) init(void)
 {
 	g_test_add_func("/mount/get_nextpath/typical",
 			test_get_nextpath__typical);
 	g_test_add_func("/mount/get_nextpath/weird", test_get_nextpath__weird);
 	g_test_add_func("/mount/is_subdir", test_is_subdir);
+	g_test_add_func("/mount/must_mkdir_and_open_with_perms",
+			test_must_mkdir_and_open_with_perms);
 }

cmd/snap-confine/mount-support.c

@@ -14,6 +14,7 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  *
  */
+
 #ifdef HAVE_CONFIG_H
 #include "config.h"
 #endif
@@ -51,6 +52,91 @@
 
 static void sc_detach_views_of_writable(sc_distro distro, bool normal_mode);
 
+static int must_mkdir_and_open_with_perms(const char *dir, uid_t uid, gid_t gid,
+					  mode_t mode)
+{
+	int retries = 10;
+	int fd;
+
+ mkdir:
+	if (--retries == 0) {
+		die("lost race to create dir %s too many times", dir);
+	}
+	// Ignore EEXIST since we want to reuse and we will open with
+	// O_NOFOLLOW, below.
+	if (mkdir(dir, 0700) < 0 && errno != EEXIST) {
+		die("cannot create directory %s", dir);
+	}
+	fd = open(dir, O_RDONLY | O_DIRECTORY | O_CLOEXEC | O_NOFOLLOW);
+	if (fd < 0) {
+		// if is not a directory then remove it and try again
+		if (errno == ENOTDIR && unlink(dir) == 0) {
+			goto mkdir;
+		}
+		die("cannot open directory %s", dir);
+	}
+	// ensure base_dir has the expected permissions since it may have
+	// already existed
+	struct stat st;
+	if (fstat(fd, &st) < 0) {
+		die("cannot stat base directory %s", dir);
+	}
+	if (st.st_uid != uid || st.st_gid != gid
+	    || st.st_mode != (S_IFDIR | mode)) {
+		unsigned char random[10] = { 0 };
+		char random_dir[MAX_BUF] = { 0 };
+		int offset;
+		size_t i;
+
+		// base_dir isn't what we expect - create a random
+		// directory name and rename the existing erroneous
+		// base_dir to this then try recreating it again - NOTE we
+		// don't use mkdtemp() here since we don't want to actually
+		// create the directory yet as we want rename() to do that
+		// for us
+#ifdef SYS_getrandom
+		// use syscall(SYS_getrandom) since getrandom() is
+		// not available on older glibc
+		if (syscall(SYS_getrandom, random, sizeof(random), 0) !=
+		    sizeof(random)) {
+			die("cannot get random bytes");
+		}
+#else
+		// use /dev/urandom on older systems which don't support
+		// SYS_getrandom
+		int rfd = open("/dev/urandom", O_RDONLY);
+		if (rfd < 0) {
+			die("cannot open /dev/urandom");
+		}
+		if (read(rfd, random, sizeof(random)) != sizeof(random)) {
+			die("cannot get random bytes");
+		}
+		close(rfd);
+#endif
+		offset =
+		    sc_must_snprintf(random_dir, sizeof(random_dir), "%s.",
+				     dir);
+		for (i = 0; i < sizeof(random); i++) {
+			offset +=
+			    sc_must_snprintf(random_dir + offset,
+					     sizeof(random_dir) - offset,
+					     "%02x", (unsigned int)random[i]);
+		}
+		// try and get dir which we own by renaming it to something
+		// else then creating it again
+
+		// TODO - change this to use renameat2(RENAME_EXCHANGE)
+		// once we can use a newer version of glibc for snapd
+		if (rename(dir, random_dir) < 0) {
+			die("cannot rename base_dir to random_dir '%s'",
+			    random_dir);
+		}
+		close(fd);
+		goto mkdir;
+	}
+	return fd;
+}
+
 // TODO: simplify this, after all it is just a tmpfs
 // TODO: fold this into bootstrap
 static void setup_private_mount(const char *snap_name)
@@ -86,29 +172,8 @@ static void setup_private_mount(const char *snap_name)
 	/* Switch to root group so that mkdir and open calls below create filesystem
 	 * elements that are not owned by the user calling into snap-confine. */
 	sc_identity old = sc_set_effective_identity(sc_root_group_identity());
-	// Create /tmp/snap.$SNAP_NAME/ 0700 root.root. Ignore EEXIST since we want
-	// to reuse and we will open with O_NOFOLLOW, below.
-	if (mkdir(base_dir, 0700) < 0 && errno != EEXIST) {
-		die("cannot create base directory %s", base_dir);
-	}
-	base_dir_fd = open(base_dir,
-			   O_RDONLY | O_DIRECTORY | O_CLOEXEC | O_NOFOLLOW);
-	if (base_dir_fd < 0) {
-		die("cannot open base directory %s", base_dir);
-	}
-	/* This seems redundant on first read but it has the non-obvious
-	 * property of changing existing directories  that have already existed
-	 * but had incorrect ownership or permission. This is possible due to
-	 * earlier bugs in snap-confine and due to the fact that some systems
-	 * use persistent /tmp directory and may not clean up leftover files
-	 * for arbitrarily long. This comment applies the following two pairs
-	 * of fchmod and fchown. */
-	if (fchmod(base_dir_fd, 0700) < 0) {
-		die("cannot chmod base directory %s to 0700", base_dir);
-	}
-	if (fchown(base_dir_fd, 0, 0) < 0) {
-		die("cannot chown base directory %s to root.root", base_dir);
-	}
+	// Create /tmp/snap.$SNAP_NAME/ 0700 root.root.
+	base_dir_fd = must_mkdir_and_open_with_perms(base_dir, 0, 0, 0700);
 	// Create /tmp/snap.$SNAP_NAME/tmp 01777 root.root Ignore EEXIST since we
 	// want to reuse and we will open with O_NOFOLLOW, below.
 	if (mkdirat(base_dir_fd, "tmp", 01777) < 0 && errno != EEXIST) {
@@ -120,14 +185,14 @@ static void setup_private_mount(const char *snap_name)
 	if (tmp_dir_fd < 0) {
 		die("cannot open private tmp directory %s/tmp", base_dir);
 	}
-	if (fchmod(tmp_dir_fd, 01777) < 0) {
-		die("cannot chmod private tmp directory %s/tmp to 01777",
-		    base_dir);
-	}
 	if (fchown(tmp_dir_fd, 0, 0) < 0) {
 		die("cannot chown private tmp directory %s/tmp to root.root",
 		    base_dir);
 	}
+	if (fchmod(tmp_dir_fd, 01777) < 0) {
+		die("cannot chmod private tmp directory %s/tmp to 01777",
+		    base_dir);
+	}
 	sc_do_mount(tmp_dir, "/tmp", NULL, MS_BIND, NULL);
 	sc_do_mount("none", "/tmp", NULL, MS_PRIVATE, NULL);
 }
@@ -464,7 +529,8 @@ static void sc_bootstrap_mount_namespace(const struct sc_mount_config *config)
 	sc_identity old = sc_set_effective_identity(sc_root_group_identity());
 	if (mkdir(SC_HOSTFS_DIR, 0755) < 0) {
 		if (errno != EEXIST) {
-			die("cannot perform operation: mkdir %s", SC_HOSTFS_DIR);
+			die("cannot perform operation: mkdir %s",
+			    SC_HOSTFS_DIR);
 		}
 	}
 	(void)sc_set_effective_identity(old);