Merge lp:~jdstrand/snap-confine/security-review-fixes into lp:~snappy-dev/snap-confine/trunk

[Jamie Strandboge]

Various fixes to address Seth's feedback:
- cleaned up 'out' handling and die instead
- fixed the typo
- verify snprint() >= 512 (I increased this length) and added tests
- add test for filter missing trailing newline (I didn't change the line-too-long handling, but did add tests and verify we must end with trailing newline
- use strcmp() with "@unrestricted". add @unrestricted near miss tests
- use getresuid() instead of UBUNTU_CORE_LAUNCHER_NO_ROOT
- replace getenv() with secure_getenv() everywhere we can
- check return code of fork()
- verify return code of fclose()
- simplified final execv()

In addition:
- don't support obsoleted SNAP_APP_TMPDIR and SNAP_APP_USER_DATA_PATH
- use uid_t and gid_t instead of unsigned
- check return codes of other (f)close()s to help futureproof

[lp:~jdstrand/snap-confine/security-review-fixes]

122. By Jamie Strandboge on 2016-03-28

merge from trunk

123. By Jamie Strandboge on 2016-03-28

rename recently add tests

124. By Jamie Strandboge on 2016-03-28

merge from trunk

125. By Jamie Strandboge on 2016-03-29

fix debian/changelog which had improper syntax

126. By Jamie Strandboge on 2016-03-29

merge from trunk

127. By Jamie Strandboge on 2016-03-30

merge from trunk

128. By Jamie Strandboge on 2016-03-30

merge from trunk

129. By Jamie Strandboge on 2016-03-30

merge from trunk

[Tyler Hicks (tyhicks)]

I've completed a partial review. I still haven't gotten through all of the seccomp and test related changes. I'll complete that later this afternoon.

review: Needs Fixing

[Tyler Hicks (tyhicks)]

The seccomp_load_filters() error handling is still very confusing since there are a mix of die()'s and non-zero return values which ultimately end in a die(). If seccomp_load_filters() is going to internally call die() upon error in some conditions, it should be changed to have a void return and simply call die() in all error conditions. I believe that was what Seth was suggesting, as well.

review: Needs Fixing

[lp:~jdstrand/snap-confine/security-review-fixes]

130. By Jamie Strandboge on 2016-03-31

update a few comments based on tyhicks feedback

[Tyler Hicks (tyhicks)]

I've completed my review now. I only had one additional comment regarding another fclose() error check.

[lp:~jdstrand/snap-confine/security-review-fixes]

131. By Jamie Strandboge on 2016-03-31

simplify seccomp_load_filters() and always die() on error instead of trying to
return non-zero sometimes

[Jamie Strandboge (jdstrand)]

Thanks for the review! Yes, the error checking on the (f)close()s are not needed since they are read only, but I added them to help futureproof the codebase. Leaving those in.

I cleaned up seccomp_load_filters() error handling in r131.

=== modified file 'debian/changelog'
--- debian/changelog	2016-03-30 12:46:25 +0000
+++ debian/changelog	2016-03-31 19:41:16 +0000
@@ -2,6 +2,9 @@
 
   * update README for recent commits
   * add README.syscalls
+  * src/seccomp.c: various cleanups from security team audit (also add
+    additional tests)
+  * don't support obsoleted SNAP_APP_TMPDIR and SNAP_APP_USER_DATA_PATH
 
  -- Jamie Strandboge <[e-mail]>  Tue, 29 Mar 2016 16:07:13 -0500
 

=== modified file 'src/main.c'
--- src/main.c	2016-03-22 19:42:30 +0000
+++ src/main.c	2016-03-31 19:41:16 +0000
@@ -67,6 +67,9 @@
 
 	int status = 0;
 	pid_t pid = fork();
+	if (pid < 0) {
+		die("could not fork");
+	}
 	if (pid == 0) {
 		char buf[64];
 		unsigned major = MAJOR(devnum);
@@ -198,7 +201,8 @@
 	if (fd < 0)
 		return false;
 	int n = read(fd, content, sizeof(content));
-	close(fd);
+	if (close(fd) != 0)
+		die("could not close override");
 	if (n < sizeof(content))
 		return false;
 
@@ -259,7 +263,7 @@
 		die("unable to chown tmpdir");
 	}
 	// ensure we set the various TMPDIRs to our newly created tmpdir
-	const char *tmpd[] = { "TMPDIR", "TEMPDIR", "SNAP_APP_TMPDIR", NULL };
+	const char *tmpd[] = { "TMPDIR", "TEMPDIR", NULL };
 	int i;
 	for (i = 0; tmpd[i] != NULL; i++) {
 		if (setenv(tmpd[i], "/tmp", 1) != 0) {
@@ -395,7 +399,7 @@
 		// 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);
+			close(fd);	// we die regardless of return code
 			free(path_copy);
 			die("failed to create user data directory");
 		}
@@ -403,7 +407,10 @@
 		// previous one) so we can continue down the path.
 		int previous_fd = fd;
 		fd = openat(fd, path_segment, open_flags);
-		close(previous_fd);
+		if (close(previous_fd) != 0) {
+			free(path_copy);
+			die("could not close path segment");
+		}
 		if (fd < 0) {
 			free(path_copy);
 			die("failed to create user data directory");
@@ -413,7 +420,10 @@
 	}
 
 	// Close the descriptor for the final directory in the path.
-	close(fd);
+	if (close(fd) != 0) {
+		free(path_copy);
+		die("could not close final directory");
+	}
 
 	free(path_copy);
 }
@@ -422,16 +432,8 @@
 {
 	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;
-		}
-	}
+	if (user_data == NULL)
+		return;
 	// Only support absolute paths.
 	if (user_data[0] != '/') {
 		die("user data directory must be an absolute path");
@@ -449,15 +451,16 @@
 	const char *appname = argv[1];
 	const char *aa_profile = argv[2];
 	const char *binary = argv[3];
-	unsigned real_uid = getuid();
-	unsigned real_gid = getgid();
+	uid_t real_uid = getuid();
+	gid_t real_gid = getgid();
 
 	if (!verify_appname(appname))
 		die("appname %s not allowed", appname);
 
 	// 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) {
+	if (geteuid() != 0
+	    && secure_getenv("UBUNTU_CORE_LAUNCHER_NO_ROOT") == NULL) {
 		die("need to run as root or suid");
 	}
 
@@ -511,13 +514,11 @@
 	// set apparmor rules
 	rc = aa_change_onexec(aa_profile);
 	if (rc != 0) {
-		if (getenv("SNAPPY_LAUNCHER_INSIDE_TESTS") == NULL)
+		if (secure_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);
+	// set seccomp (note: seccomp_load_filters die()s on all failures)
+	seccomp_load_filters(aa_profile);
 
 	// Permanently drop if not root
 	if (geteuid() == 0) {
@@ -534,8 +535,7 @@
 			die("permanently dropping privs did not work");
 	}
 	// and exec the new binary
-	argv[NR_ARGS] = (char *)binary,
-	    execv(binary, (char *const *)&argv[NR_ARGS]);
+	execv(binary, (char *const *)&argv[NR_ARGS]);
 	perror("execv failed");
 	return 1;
 }

=== modified file 'src/seccomp.c'
--- src/seccomp.c	2016-03-22 19:42:30 +0000
+++ src/seccomp.c	2016-03-31 19:41:16 +0000
@@ -14,6 +14,10 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  *
  */
+#ifndef _GNU_SOURCE
+#define _GNU_SOURCE
+#endif
+
 #include <errno.h>
 #include <stdio.h>
 #include <unistd.h>
@@ -37,7 +41,7 @@
 	return slen;
 }
 
-int seccomp_load_filters(const char *filter_profile)
+void seccomp_load_filters(const char *filter_profile)
 {
 	debug("seccomp_load_filters %s", filter_profile);
 	int rc = 0;
@@ -45,45 +49,51 @@
 	scmp_filter_ctx ctx = NULL;
 	FILE *f = NULL;
 	size_t lineno = 0;
+	uid_t real_uid, effective_uid, saved_uid;
 
 	ctx = seccomp_init(SCMP_ACT_KILL);
-	if (ctx == NULL)
-		return ENOMEM;
-
+	if (ctx == NULL) {
+		errno = ENOMEM;
+		die("seccomp_init() failed");
+	}
 	// Disable NO_NEW_PRIVS because it interferes with exec transitions in
 	// AppArmor. Unfortunately this means that security policies must be
 	// very careful to not allow the following otherwise apps can escape
-	// the snadbox:
+	// the sandbox:
 	//   - seccomp syscall
 	//   - prctl with PR_SET_SECCOMP
 	//   - ptrace (trace) in AppArmor
 	//   - capability sys_admin in AppArmor
 	// Note that with NO_NEW_PRIVS disabled, CAP_SYS_ADMIN is required to
 	// change the seccomp sandbox.
-	if (getenv("UBUNTU_CORE_LAUNCHER_NO_ROOT") == NULL) {
-		rc = seccomp_attr_set(ctx, SCMP_FLTATR_CTL_NNP, 0);
-		if (rc != 0) {
-			fprintf(stderr, "Cannot disable nnp\n");
-			return -1;
-		}
-	}
-
-	if (getenv("SNAPPY_LAUNCHER_SECCOMP_PROFILE_DIR") != NULL)
+
+	if (getresuid(&real_uid, &effective_uid, &saved_uid) != 0)
+		die("could not find user IDs");
+
+	// If running privileged or capable of raising, disable nnp
+	if (real_uid == 0 || effective_uid == 0 || saved_uid == 0)
+		if (seccomp_attr_set(ctx, SCMP_FLTATR_CTL_NNP, 0) != 0)
+			die("Cannot disable nnp");
+
+	// Note that secure_gettenv will always return NULL when suid, so
+	// SNAPPY_LAUNCHER_SECCOMP_PROFILE_DIR can't be (ab)used in that case.
+	if (secure_getenv("SNAPPY_LAUNCHER_SECCOMP_PROFILE_DIR") != NULL)
 		filter_profile_dir =
-		    getenv("SNAPPY_LAUNCHER_SECCOMP_PROFILE_DIR");
+		    secure_getenv("SNAPPY_LAUNCHER_SECCOMP_PROFILE_DIR");
 
-	char profile_path[128];
-	if (snprintf
-	    (profile_path, sizeof(profile_path), "%s/%s", filter_profile_dir,
-	     filter_profile) < 0) {
-		goto out;
+	char profile_path[512];	// arbitrary path name limit
+	int snprintf_rc = snprintf(profile_path, sizeof(profile_path), "%s/%s",
+				   filter_profile_dir, filter_profile);
+	if (snprintf_rc < 0 || snprintf_rc >= 512) {
+		errno = 0;
+		die("snprintf returned unexpected value");
 	}
 
 	f = fopen(profile_path, "r");
 	if (f == NULL) {
 		fprintf(stderr, "Can not open %s (%s)\n", profile_path,
 			strerror(errno));
-		return -1;
+		die("aborting");
 	}
 	// 80 characters + '\n' + '\0'
 	char buf[82];
@@ -104,16 +114,17 @@
 			fprintf(stderr,
 				"seccomp filter line %zu was too long (%zu characters max)\n",
 				lineno, sizeof(buf) - 2);
-			rc = -1;
-			goto out;
+			errno = 0;
+			die("aborting");
 		}
 		// kill final newline
 		len = trim_right(buf, len);
 		if (len == 0)
 			continue;
 
-		// check for special "@unrestricted" command
-		if (strncmp(buf, "@unrestricted", sizeof(buf)) == 0)
+		// check for special "@unrestricted" rule which short-circuits
+		// seccomp sandbox
+		if (strcmp(buf, "@unrestricted") == 0)
 			goto out;
 
 		// syscall not available on this arch/kernel
@@ -132,13 +143,15 @@
 				fprintf(stderr,
 					"seccomp_rule_add failed with %i for '%s'\n",
 					rc, buf);
-				goto out;
+				errno = 0;
+				die("aborting");
 			}
 		}
 	}
 
-	// raise privileges to load seccomp policy since we don't have nnp
-	if (getenv("UBUNTU_CORE_LAUNCHER_NO_ROOT") == NULL) {
+	// If not root but can raise, then raise privileges to load seccomp
+	// policy since we don't have nnp
+	if (effective_uid != 0 && saved_uid == 0) {
 		if (seteuid(0) != 0)
 			die("seteuid failed");
 		if (geteuid() != 0)
@@ -149,10 +162,8 @@
 
 	if (rc != 0) {
 		fprintf(stderr, "seccomp_load failed with %i\n", rc);
-		goto out;
+		die("aborting");
 	}
-
- out:
 	// drop privileges again
 	if (geteuid() == 0) {
 		unsigned real_uid = getuid();
@@ -162,9 +173,11 @@
 			die("dropping privs after seccomp_load did not work");
 	}
 
+ out:
 	if (f != NULL) {
-		fclose(f);
+		if (fclose(f) != 0)
+			die("could not close seccomp file");
 	}
 	seccomp_release(ctx);
-	return rc;
+	return;
 }

=== modified file 'src/utils.c'
--- src/utils.c	2016-03-22 12:17:03 +0000
+++ src/utils.c	2016-03-31 19:41:16 +0000
@@ -70,7 +70,8 @@
 		die("fwrite failed");
 	if (fflush(f) != 0)
 		die("fflush failed");
-	fclose(f);
+	if (fclose(f) != 0)
+		die("fclose failed");
 }
 
 int must_snprintf(char *str, size_t size, const char *format, ...)

=== added file 'tests/test_bad_seccomp_filter_length'
--- tests/test_bad_seccomp_filter_length	1970-01-01 00:00:00 +0000
+++ tests/test_bad_seccomp_filter_length	2016-03-31 19:41:16 +0000
@@ -0,0 +1,20 @@
+#!/bin/sh
+
+set -e
+
+. $(pwd)/common.sh
+
+printf "Test seccomp filter (bad - too long)"
+
+cat >$TMP/aaprofile <<EOF
+# some comment
+baddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddd
+EOF
+
+if $L appname aaprofile /bin/true 2>/dev/null; then
+    # true returned successfully, length test failed
+    FAIL
+fi
+
+# all good
+PASS

=== added file 'tests/test_bad_seccomp_filter_missing_trailing_newline'
--- tests/test_bad_seccomp_filter_missing_trailing_newline	1970-01-01 00:00:00 +0000
+++ tests/test_bad_seccomp_filter_missing_trailing_newline	2016-03-31 19:41:16 +0000
@@ -0,0 +1,17 @@
+#!/bin/sh
+
+set -e
+
+. $(pwd)/common.sh
+
+printf "Test seccomp filter (bad - no trailing newline)"
+
+echo -n "missingnewline" > $TMP/aaprofile
+
+if $L appname aaprofile /bin/true 2>/dev/null; then
+    # true returned successfully, length test failed
+    FAIL
+fi
+
+# all good
+PASS

=== modified file 'tests/test_create_user_data'
--- tests/test_create_user_data	2016-03-19 02:32:43 +0000
+++ tests/test_create_user_data	2016-03-31 19:41:16 +0000
@@ -82,56 +82,28 @@
    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
+run_current $TMP/user_data unrestricted true
 
 printf "Test that a restricted launcher creates user data"
-run_both $TMP/user_data restricted true
+run_current $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
+run_current $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
+run_current $TMP/parent/user_data restricted true
 
 printf "Test that user data can contain multiple path separators"
-run_both $TMP//user_data unrestricted true
+run_current $TMP//user_data unrestricted true
 
 printf "Test that user data must be absolute"
-run_both "../foo" unrestricted false
+run_current "../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
+run_current $TMP/parent/user_data unrestricted, false
 
 printf "Test that an unrestricted launcher works when user data exists (current)"
 mkdir $TMP/user_data
@@ -148,19 +120,3 @@
 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

=== added file 'tests/test_unrestricted_missed'
--- tests/test_unrestricted_missed	1970-01-01 00:00:00 +0000
+++ tests/test_unrestricted_missed	2016-03-31 19:41:16 +0000
@@ -0,0 +1,30 @@
+#!/bin/sh
+
+set -e
+
+. $(pwd)/common.sh
+
+cat >$TMP/myprofile <<EOF
+# super strict filter
+@unrestricte
+@unrestrictes
+@nrestricted
+@UNRESTRICTED
+unrestricted
+EOF
+
+# ensure that the command "true" can not run due to impossible
+# filtering
+
+printf "Test that near misses of unrestricted fail"
+if $L appid myprofile /bin/true 2>/dev/null; then
+    # true returned successfully, our filtering is broken!
+    FAIL
+else
+    # true returend a error code, check dmesg
+    if dmesg|tail -n1|grep -q "audit"; then
+        PASS
+    else
+        FAIL
+    fi
+fi