[systemd-devel] [PATCH 1/2] test: update policy tests to handle user namespaces

Djalal Harouni tixxdz at opendz.org
Mon Sep 8 06:18:25 PDT 2014


Upstream kernels allow unprivileged users to create user namespaces
and change their uid/gid.

These patches update kdbus policy logic to handle this case and
improve our current checks across user namespaces.

So this patch adds:

* kdbus_test_waitpid() to get exit code of childs.
* kdbus_clone_userns_test() that performs the test inside a new
  user namespace.
* Converts all the other tests to return CHECK_OK, CHECK_SKIP or
  CHECK_ERR so we are consistent.

Currently we fail at kdbus_clone_userns_test() test #8. The next patch
will fix this issue.

Signed-off-by: Djalal Harouni <tixxdz at opendz.org>
---
 test/test-kdbus-policy.c | 503 +++++++++++++++++++++++++++++++++++------------
 1 file changed, 375 insertions(+), 128 deletions(-)

diff --git a/test/test-kdbus-policy.c b/test/test-kdbus-policy.c
index 8d7bcb5..a354290 100644
--- a/test/test-kdbus-policy.c
+++ b/test/test-kdbus-policy.c
@@ -12,6 +12,7 @@
 #include <fcntl.h>
 #include <pthread.h>
 #include <poll.h>
+#include <sched.h>
 #include <stdlib.h>
 #include <stddef.h>
 #include <stdint.h>
@@ -22,6 +23,8 @@
 #include <sys/wait.h>
 #include <sys/prctl.h>
 #include <sys/ioctl.h>
+#include <sys/eventfd.h>
+#include <sys/syscall.h>
 
 #include "kdbus-util.h"
 #include "kdbus-enum.h"
@@ -29,6 +32,30 @@
 #define MAX_CONN	64
 #define POLICY_NAME	"foo.test.policy-test"
 
+static int ok_cnt;
+static int skip_cnt;
+static int fail_cnt;
+
+static void print_test_status(int test_status)
+{
+	switch (test_status) {
+	case CHECK_OK:
+		printf("OK");
+		ok_cnt++;
+		break;
+	case CHECK_SKIP:
+		printf("SKIPPED");
+		skip_cnt++;
+		break;
+	case CHECK_ERR:
+	default:
+		printf("ERROR");
+		fail_cnt++;
+		break;
+	}
+
+	printf("\n");
+}
 
 /**
  * The purpose of these tests:
@@ -53,35 +80,59 @@ void kdbus_free_conn(struct conn *conn)
 	}
 }
 
+static void *kdbus_recv_echo(void *ptr)
+{
+	int ret;
+	struct conn *conn = ptr;
+
+	ret = conn_recv(conn);
+
+	return (void *)(long)ret;
+}
+
 /* Trigger kdbus_policy_set() */
 static int kdbus_set_policy_talk(struct conn *conn,
 				 const char *name,
 				 uid_t id, unsigned int type)
 {
+	int ret;
 	struct kdbus_policy_access access = {
 		.type = type,
 		.id = id,
 		.access = KDBUS_POLICY_TALK,
 	};
 
-	return conn_update_policy(conn, name, &access, 1);
+	ret = conn_update_policy(conn, name, &access, 1);
+	if (ret < 0)
+		return CHECK_ERR;
+
+	return CHECK_OK;
 }
 
-/* The policy access will be stored in a policy holder connection */
-static int kdbus_register_activator(char *bus, const char *name,
-				    struct conn **c)
+/* return CHECK_OK or CHECK_ERR on failure */
+static int kdbus_register_same_activator(char *bus, const char *name,
+					 struct conn **c)
 {
+	int ret;
 	struct conn *activator;
 
 	activator = kdbus_hello_activator(bus, name, NULL, 0);
-	if (!activator)
-		return -errno;
+	if (activator) {
+		*c = activator;
+		fprintf(stderr, "--- error was able to register name twice '%s'.\n",
+			name);
+		return CHECK_ERR;
+	}
 
-	*c = activator;
+	ret = -errno;
+	/* -EEXIST means test succeeded */
+	if (ret == -EEXIST)
+		return CHECK_OK;
 
-	return 0;
+	return CHECK_ERR;
 }
 
+/* return CHECK_OK or CHECK_ERR on failure */
 static int kdbus_register_policy_holder(char *bus, const char *name,
 					struct conn **conn)
 {
@@ -99,54 +150,88 @@ static int kdbus_register_policy_holder(char *bus, const char *name,
 	c = kdbus_hello_registrar(bus, name, access, 2,
 				  KDBUS_HELLO_POLICY_HOLDER);
 	if (!c)
-		return -errno;
+		return CHECK_ERR;
 
 	*conn = c;
 
-	return 0;
+	return CHECK_OK;
 }
 
-static void *kdbus_recv_echo(void *ptr)
+/* return CHECK_OK or CHECK_ERR on failure */
+static int kdbus_receiver_acquire_name(char *bus, const char *name,
+					struct conn **conn)
 {
 	int ret;
-	int cnt = 3;
-	struct pollfd fd;
-	struct conn *conn = ptr;
+	struct conn *c;
 
-	fd.fd = conn->fd;
-	fd.events = POLLIN | POLLPRI | POLLHUP;
-	fd.revents = 0;
+	c = kdbus_hello(bus, 0, NULL, 0);
+	if (!c)
+		return CHECK_ERR;
 
-	while (cnt) {
-		cnt--;
-		ret = poll(&fd, 1, 2000);
-		if (ret == 0) {
-			ret = -ETIMEDOUT;
-			break;
-		}
+	add_match_empty(c->fd);
 
-		if (ret > 0 && fd.revents & POLLIN) {
-			printf("-- Connection id: %llu received new message:\n",
-				(unsigned long long)conn->id);
-			ret = msg_recv(conn);
-		}
+	ret = name_acquire(c, name, 0);
+	if (ret < 0)
+		return CHECK_ERR;
 
-		if (ret >= 0 || ret != -EAGAIN)
-			break;
+	*conn = c;
+
+	return CHECK_OK;
+}
+
+/**
+ * Return the exit code of the child process and set the test_done
+ * variable if the test was performed:
+ * In case of WIFEXITED():
+ *   1) If exit code != EXIT_FAILURE assume test reached and set the
+ *      'test_done' variable if provided to 1.
+ *   2) If exit code != EXIT_FAILURE and exit code != EXIT_SUCCESS:
+ *      Convert the exit code to a proper -errno and return it.
+ * In all other cases:
+ *   set the exit code to EXIT_FAILURE and return it.
+ */
+static int kdbus_test_waitpid(pid_t pid, int *test_done)
+{
+	int ret;
+	int status;
+
+	int test_performed = 0;
+
+	ret = waitpid(pid, &status, 0);
+	if (ret < 0) {
+		ret = -errno;
+		fprintf(stderr, "error waitpid: %d (%m)\n", ret);
+		return ret;
 	}
 
-	return (void *)(long)ret;
+	if (WIFEXITED(status)) {
+		ret = WEXITSTATUS(status);
+		if (ret != EXIT_FAILURE) {
+			if (ret != EXIT_SUCCESS)
+				ret |= -1 << 8; /* get -errno */
+
+			/* test reached */
+			test_performed = 1;
+		}
+	} else {
+		ret = EXIT_FAILURE;
+	}
+
+	if (test_done)
+		*test_done = test_performed;
+
+	return ret;
 }
 
 /**
- * Just run a normal test, the 'conn_db' will be populated by
- * newly created connections. Caller should free all allocated
- * connections.
+ * Create new threads for receiving from multiple senders,
+ * The 'conn_db' will be populated by newly created connections.
+ * Caller should free all allocated connections.
  *
- * return 0 on success, a non-zero on failure.
+ * return 0 on success, negative errno on failure.
  */
-static int kdbus_normal_test(const char *bus, const char *name,
-			     struct conn **conn_db)
+static int kdbus_recv_in_threads(const char *bus, const char *name,
+				 struct conn **conn_db)
 {
 	int ret;
 	unsigned int i, tid;
@@ -198,18 +283,38 @@ static int kdbus_normal_test(const char *bus, const char *name,
 	return ret;
 }
 
-static int kdbus_fork_test(const char *bus, const char *name,
+/* Return: CHECK_OK or CHECK_ERR on failure */
+static int kdbus_normal_test(const char *bus, const char *name,
 			   struct conn **conn_db)
 {
-	int ret = 0;
-	int status;
+	int ret;
+
+	ret = kdbus_recv_in_threads(bus, name, conn_db);
+	if (ret < 0)
+		return CHECK_ERR;
+
+	return CHECK_OK;
+}
+
+/*
+ * Return: CHECK_OK, CHECK_ERR or CHECK_SKIP
+ * we return CHECK_OK only if the childs return with the expected
+ * 'exit_code' that is specified as an argument.
+ */
+static int kdbus_fork_test(const char *bus, const char *name,
+			   struct conn **conn_db, int exit_code)
+{
 	pid_t pid;
+	int ret = 0;
 	int test_done = 0;
+	int test_status = CHECK_ERR;
 
+	setbuf(stdout, NULL);
+	printf("STARTING the (FORK+DROP) test...............\n");
 	if (geteuid() > 0) {
 		fprintf(stderr, "error geteuid() != 0, %s() needs root\n",
 			__func__);
-		goto out;
+		return CHECK_SKIP;
 	}
 
 	pid = fork();
@@ -228,7 +333,7 @@ static int kdbus_fork_test(const char *bus, const char *name,
 		if (ret < 0)
 			goto child_fail;
 
-		ret = kdbus_normal_test(bus, POLICY_NAME, conn_db);
+		ret = kdbus_recv_in_threads(bus, name, conn_db);
 
 		/*
 		 * Here cached connections belong to child, they will
@@ -240,31 +345,203 @@ child_fail:
 		_exit(EXIT_FAILURE);
 	}
 
-	ret = waitpid(pid, &status, 0);
+	ret = kdbus_test_waitpid(pid, &test_done);
+
+out:
+	/* test reached */
+	if (test_done) {
+		if (ret == exit_code)
+			test_status = CHECK_OK;
+		else
+			fprintf(stderr,
+				"error TEST exit code: %d  was expecting code: %d\n",
+				ret, exit_code);
+	}
+
+	return test_status;
+}
+
+/* Return EXIT_SUCCESS, EXIT_FAILURE or negative errno */
+static int __kdbus_clone_userns_test(const char *bus,
+				     const char *name,
+				     struct conn **conn_db)
+{
+	int efd;
+	pid_t pid;
+	int ret = 0;
+	unsigned int uid = 65534;
+
+	ret = drop_privileges(uid, uid);
+	if (ret < 0)
+		return ret;
+
+	/**
+	 * Since we just dropped privileges, the dumpable flag was just
+	 * cleared which makes the /proc/$clone_child/uid_map to be
+	 * owned by root, hence any userns uid mapping will fail with
+	 * -EPERM since the mapping will be done by uid 65534.
+	 *
+	 * To avoid this set the dumpable flag again which makes procfs
+	 * update the /proc/$clone_child/ inodes owner to 65534.
+	 *
+	 * Using this we will be able write to /proc/$clone_child/uid_map
+	 * as uid 65534 and map the uid 65534 to 0 inside the user
+	 * namespace.
+	 */
+	ret = prctl(PR_SET_DUMPABLE, SUID_DUMP_USER);
 	if (ret < 0) {
 		ret = -errno;
-		fprintf(stderr, "error waitpid: %d (%m)\n", ret);
+		fprintf(stderr, "error prctl: %d (%m)\n", ret);
+		return ret;
+	}
+
+	/* sync parent/child */
+	efd = eventfd(0, EFD_CLOEXEC);
+	if (efd < 0) {
+		ret = -errno;
+		fprintf(stderr, "error eventfd: %d (%m)\n", ret);
+		return ret;
+	}
+
+	pid = syscall(__NR_clone, SIGCHLD|CLONE_NEWUSER, NULL);
+	if (pid < 0) {
+		ret = -errno;
+		fprintf(stderr, "error clone: %d (%m)\n", ret);
+		/*
+		 * Normal user not allowed to create userns,
+		 * so nothing to worry about ?
+		 */
+		if (ret == -EPERM) {
+			printf("-- CLONE_NEWUSER TEST Failed for uid: %u\n"
+				"-- Make sure that your kernel do not allow CLONE_NEWUSER for unprivileged users\n"
+				"-- Upstream Commit: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=5eaf563e53294d6696e651466697eb9d491f3946\n",
+				uid);
+			ret = 0;
+		}
+
 		goto out;
 	}
 
-	if (WIFEXITED(status)) {
-		ret = WEXITSTATUS(status);
-		if (ret != EXIT_FAILURE) {
-			if (ret != EXIT_SUCCESS)
-				ret |= -1 << 8; /* get -errno */
+	if (pid == 0) {
+		struct conn *conn_src;
+		eventfd_t event_status = 0;
 
-			test_done = 1;	/* assume test reached */
+		setbuf(stdout, NULL);
+		ret = prctl(PR_SET_PDEATHSIG, SIGKILL);
+		if (ret < 0) {
+			ret = -errno;
+			fprintf(stderr, "error prctl: %d (%m)\n", ret);
+			goto child_fail;
 		}
+
+		ret = eventfd_read(efd, &event_status);
+		if (ret < 0 || event_status != 1)
+			/* event_stats == 1 to continue */
+			goto child_fail;
+
+		/* ping connection from the new user namespace */
+		conn_src = kdbus_hello(bus, 0, NULL, 0);
+		if (!conn_src)
+			goto child_fail;
+
+		add_match_empty(conn_src->fd);
+		ret = msg_send(conn_src, name, 0xabcd1234,
+			       0, 0, 0, KDBUS_DST_ID_NAME);
+
+		close(conn_src->fd);
+		free(conn_src);
+		_exit(ret);
+
+child_fail:
+		_exit(EXIT_FAILURE);
+	}
+
+	ret = userns_map_uid_gid(pid, "0 65534 1", "0 65534 1");
+	if (ret < 0) {
+		/* send error to child */
+		eventfd_write(efd, 2);
+		fprintf(stderr, "error mapping uid/gid in new user namespace\n");
+		goto out;
 	}
 
+	/* Tell child we are ready */
+	ret = eventfd_write(efd, 1);
+	if (ret < 0) {
+		ret = -errno;
+		fprintf(stderr, "error eventfd_write: %d (%m)\n", ret);
+		goto out;
+	}
+
+	ret = kdbus_test_waitpid(pid, NULL);
+
 out:
-	/* test not reached, return -EIO and avoid EXIT_FAILURE */
-	if (!test_done)
-		ret = -EIO;
+	close(efd);
 
 	return ret;
 }
 
+static int kdbus_clone_userns_test(const char *bus,
+				   const char *name,
+				   struct conn **conn_db,
+				   int exit_code)
+{
+	pid_t pid;
+	int ret = 0;
+	int test_done = 0;
+	int test_status = CHECK_ERR;
+
+	setbuf(stdout, NULL);
+	printf("STARTING TEST connections in a new user namespace..\n");
+	if (geteuid() > 0) {
+		fprintf(stderr, "error geteuid() != 0, %s() needs root\n",
+			__func__);
+		return CHECK_SKIP;
+	}
+
+	pid = fork();
+	if (pid < 0) {
+		ret = -errno;
+		fprintf(stderr, "error fork(): %d (%m)\n", ret);
+		goto out;
+	}
+
+	if (pid == 0) {
+		ret = prctl(PR_SET_PDEATHSIG, SIGKILL);
+		if (ret < 0)
+			goto child_fail;
+
+		ret = __kdbus_clone_userns_test(bus, name, conn_db);
+		_exit(ret);
+child_fail:
+		_exit(EXIT_FAILURE);
+	}
+
+	/* Receive in the original (root privileged) user namespace */
+	ret = conn_recv(conn_db[0]);
+	if (!ret) {
+		fprintf(stderr,
+			"--- error received packet from unprivileged user namespace\n");
+		goto out;
+	}
+
+	ret = kdbus_test_waitpid(pid, &test_done);
+
+out:
+	/* test reached */
+	if (test_done) {
+		/* Set to CHECK_OK if we did get the right exit_code */
+		if (ret == exit_code)
+			test_status = CHECK_OK;
+		else
+			fprintf(stderr,
+				"error TEST exit code: %d  was expecting code: %d\n",
+				ret, exit_code);
+	}
+
+	return test_status;
+}
+
+/* Return CHECK_OK, CHECK_ERR or CHECK_SKIP */
 static int kdbus_check_policy(char *bus)
 {
 	int i;
@@ -282,107 +559,74 @@ static int kdbus_check_policy(char *bus)
 					   &policy_holder);
 	printf("-- TEST 1) register a policy holder for '%s' ",
 		POLICY_NAME);
-	if (ret < 0) {
-		printf("FAILED\n");
+	print_test_status(ret);
+	if (ret == CHECK_ERR)
 		goto out_free_connections;
-	}
-
-	printf("OK\n");
 
 	/* Try to register the same name with an activator */
-	ret = kdbus_register_activator(bus, POLICY_NAME, &activator);
+	ret = kdbus_register_same_activator(bus, POLICY_NAME,
+					    &activator);
 	printf("-- TEST 2) register again '%s' as an activator ",
 		POLICY_NAME);
-	if (ret == 0) {
-		printf("succeeded: TEST FAILED\n");
-		fprintf(stderr, "--- error was able to register twice '%s'.\n",
-			POLICY_NAME);
-		ret = -1;
-		goto out_free_connections;
-	} else if (ret < 0) {
-		/* -EEXIST means test succeeded */
-		if (ret == -EEXIST) {
-			ret = 0;
-			printf("failed: TEST OK\n");
-		} else {
-			printf("FAILED\n");
-			goto out_free_connections;
-		}
-	}
-
-	/* The name receiver */
-	conn_db[0] = kdbus_hello(bus, 0, NULL, 0);
-	if (!conn_db[0]) {
-		ret = -1;
+	print_test_status(ret);
+	if (ret == CHECK_ERR)
 		goto out_free_connections;
-	}
-
-	add_match_empty(conn_db[0]->fd);
 
-	ret = name_acquire(conn_db[0], POLICY_NAME, 0);
+	ret = kdbus_receiver_acquire_name(bus, POLICY_NAME, &conn_db[0]);
 	printf("-- TEST 3) acquire '%s' name..... ", POLICY_NAME);
-	if (ret < 0) {
-		printf("FAILED\n");
+	print_test_status(ret);
+	if (ret == CHECK_ERR)
 		goto out_free_connections;
-	}
-
-	printf("OK\n");
 
 	ret = kdbus_normal_test(bus, POLICY_NAME, conn_db);
 	printf("-- TEST 4) testing connections (NORMAL TEST).... ");
-	if (ret != 0) {
-		printf("FAILED\n");
+	print_test_status(ret);
+	if (ret == CHECK_ERR)
 		goto out_free_connections;
-	}
-
-	printf("OK\n");
 
 	name_list(conn_db[0], KDBUS_NAME_LIST_NAMES |
 			      KDBUS_NAME_LIST_UNIQUE |
 			      KDBUS_NAME_LIST_ACTIVATORS |
 			      KDBUS_NAME_LIST_QUEUED);
 
-	ret = kdbus_fork_test(bus, POLICY_NAME, conn_db);
+	ret = kdbus_fork_test(bus, POLICY_NAME, conn_db, EXIT_SUCCESS);
 	printf("-- TEST 5) testing connections (FORK+DROP)...... ");
-	if (ret != 0) {
-		printf("FAILED\n");
+	print_test_status(ret);
+	if (ret == CHECK_ERR)
 		goto out_free_connections;
-	}
-
-	printf("OK\n");
 
 	/*
 	 * Connections that can talk are perhaps being destroyed now.
 	 * Restrict the policy and purge cache entries where the
 	 * conn_db[0] is the destination.
+	 *
+	 * Now only connections with uid == 0 are allowed to talk.
 	 */
 	ret = kdbus_set_policy_talk(policy_holder, POLICY_NAME,
 				    geteuid(), KDBUS_POLICY_ACCESS_USER);
 	printf("-- TEST 6) restricting '%s' policy TALK access ",
 		POLICY_NAME);
-	if (ret < 0) {
-		printf("FAILED\n");
+	print_test_status(ret);
+	if (ret == CHECK_ERR)
 		goto out_free_connections;
-	}
 
-	printf("OK\n");
-
-	/* After setting the policy re-check connections */
-	ret = kdbus_fork_test(bus, POLICY_NAME, conn_db);
+	printf("-- TEST 7) testing connections (FORK+DROP) again\n");
+	/*
+	 * After setting the policy re-check connections
+	 * we expect the childs to fail with -EPERM
+	 */
+	ret = kdbus_fork_test(bus, POLICY_NAME, conn_db, -EPERM);
 	printf("-- TEST 7) testing connections (FORK+DROP) again ");
-	if (ret == 0) {
-		printf("FAILED\n");
-		fprintf(stderr, "--- error policy rules: send to all succeeded.\n");
-		ret = -1;
-	} else if (ret < 0) {
-		/* -EPERM means tests succeeded */
-		if (ret == -EPERM) {
-			ret = 0;
-			printf("OK\n");
-		} else {
-			printf("FAILED\n");
-		}
-	}
+	print_test_status(ret);
+	if (ret == CHECK_ERR)
+		goto out_free_connections;
+
+	printf("-- TEST 8) testing connections in a new user namespace\n");
+	/* Check if the name can be reached in a new userns */
+	ret = kdbus_clone_userns_test(bus, POLICY_NAME,
+				      conn_db, -EPERM);
+	printf("-- TEST 8) testing connections in a new user namespace ");
+	print_test_status(ret);
 
 out_free_connections:
 	kdbus_free_conn(activator);
@@ -413,7 +657,7 @@ int main(int argc, char *argv[])
 		uint64_t n_type;
 		char name[64];
 	} bus_make;
-	int fdc, ret, i;
+	int fdc, ret;
 	char *bus;
 
 	printf("-- opening /dev/" KBUILD_MODNAME "/control\n");
@@ -451,13 +695,16 @@ int main(int argc, char *argv[])
 
 	ret = kdbus_check_policy(bus);
 
-	printf("RUNNING TEST 'policy db check' ");
-	for (i = 0; i < 17; i++)
-		printf(".");
-	printf(" ");
+	printf("\nSUMMARY: %d tests passed, %d skipped%s, %d failed\n",
+		ok_cnt, skip_cnt,
+		skip_cnt ? " (need privileges)" : "", fail_cnt);
 
-	if (ret < 0) {
-		printf("FAILED\n");
+	if (skip_cnt > 0)
+		printf("For security reasons make sure to re-run skipped tests.\n");
+
+	printf("RUNNING TEST 'policy db check'................ ");
+	if (fail_cnt > 0) {
+		printf("Failed\n");
 		return EXIT_FAILURE;
 	}
 
-- 
1.9.3



More information about the systemd-devel mailing list