Merge tag 'safesetid-5.3' of git://github.com/micah-morton/linux

Pull safesetid updates from Micah Morton:
 "These changes from Jann Horn fix a couple issues in the recently added
  SafeSetID LSM:

   - There was a simple logic bug in one of the hooks for the LSM where
     the code was incorrectly returning early in some cases before all
     security checks had been passed.

   - There was a more high level issue with how this LSM gets configured
     that could allow for a program to bypass the security restrictions
     by switching to an allowed UID and then again to any other UID on
     the system if the target UID of the first transition is
     unconstrained on the system. Luckily this is an easy fix that we
     now enforce at the time the LSM gets configured.

  There are also some changes from Jann that make policy updates for
  this LSM atomic. Kees Cook, Jann and myself have reviewed these
  changes and they look good from our point of view"

* tag 'safesetid-5.3' of git://github.com/micah-morton/linux:
  LSM: SafeSetID: fix use of literal -1 in capable hook
  LSM: SafeSetID: verify transitive constrainedness
  LSM: SafeSetID: add read handler
  LSM: SafeSetID: rewrite userspace API to atomic updates
  LSM: SafeSetID: fix userns handling in securityfs
  LSM: SafeSetID: refactor policy parsing
  LSM: SafeSetID: refactor safesetid_security_capable()
  LSM: SafeSetID: refactor policy hash table
  LSM: SafeSetID: fix check for setresuid(new1, new2, new3)
  LSM: SafeSetID: fix pr_warn() to include newline

security/safesetid/lsm.h

@@ -15,19 +15,39 @@
 #define _SAFESETID_H
 
 #include <linux/types.h>
+#include <linux/uidgid.h>
+#include <linux/hashtable.h>
 
 /* Flag indicating whether initialization completed */
 extern int safesetid_initialized;
 
-/* Function type. */
-enum safesetid_whitelist_file_write_type {
-	SAFESETID_WHITELIST_ADD, /* Add whitelist policy. */
-	SAFESETID_WHITELIST_FLUSH, /* Flush whitelist policies. */
+enum sid_policy_type {
+	SIDPOL_DEFAULT, /* source ID is unaffected by policy */
+	SIDPOL_CONSTRAINED, /* source ID is affected by policy */
+	SIDPOL_ALLOWED /* target ID explicitly allowed */
 };
 
-/* Add entry to safesetid whitelist to allow 'parent' to setid to 'child'. */
-int add_safesetid_whitelist_entry(kuid_t parent, kuid_t child);
+/*
+ * Hash table entry to store safesetid policy signifying that 'src_uid'
+ * can setuid to 'dst_uid'.
+ */
+struct setuid_rule {
+	struct hlist_node next;
+	kuid_t src_uid;
+	kuid_t dst_uid;
+};
+
+#define SETID_HASH_BITS 8 /* 256 buckets in hash table */
+
+struct setuid_ruleset {
+	DECLARE_HASHTABLE(rules, SETID_HASH_BITS);
+	char *policy_str;
+	struct rcu_head rcu;
+};
+
+enum sid_policy_type _setuid_policy_lookup(struct setuid_ruleset *policy,
+		kuid_t src, kuid_t dst);
 
-void flush_safesetid_whitelist_entries(void);
+extern struct setuid_ruleset __rcu *safesetid_setuid_rules;
 
 #endif /* _SAFESETID_H */

security/safesetid/securityfs.c

@@ -11,92 +11,184 @@
  * published by the Free Software Foundation.
  *
  */
+
+#define pr_fmt(fmt) "SafeSetID: " fmt
+
 #include <linux/security.h>
 #include <linux/cred.h>
 
 #include "lsm.h"
 
-static struct dentry *safesetid_policy_dir;
-
-struct safesetid_file_entry {
-	const char *name;
-	enum safesetid_whitelist_file_write_type type;
-	struct dentry *dentry;
-};
-
-static struct safesetid_file_entry safesetid_files[] = {
-	{.name = "add_whitelist_policy",
-	 .type = SAFESETID_WHITELIST_ADD},
-	{.name = "flush_whitelist_policies",
-	 .type = SAFESETID_WHITELIST_FLUSH},
-};
+static DEFINE_MUTEX(policy_update_lock);
 
 /*
  * In the case the input buffer contains one or more invalid UIDs, the kuid_t
- * variables pointed to by 'parent' and 'child' will get updated but this
+ * variables pointed to by @parent and @child will get updated but this
  * function will return an error.
+ * Contents of @buf may be modified.
  */
-static int parse_safesetid_whitelist_policy(const char __user *buf,
-					    size_t len,
-					    kuid_t *parent,
-					    kuid_t *child)
+static int parse_policy_line(struct file *file, char *buf,
+	struct setuid_rule *rule)
 {
-	char *kern_buf;
-	char *parent_buf;
-	char *child_buf;
-	const char separator[] = ":";
+	char *child_str;
 	int ret;
-	size_t first_substring_length;
-	long parsed_parent;
-	long parsed_child;
+	u32 parsed_parent, parsed_child;
 
-	/* Duplicate string from user memory and NULL-terminate */
-	kern_buf = memdup_user_nul(buf, len);
-	if (IS_ERR(kern_buf))
-		return PTR_ERR(kern_buf);
+	/* Format of |buf| string should be <UID>:<UID>. */
+	child_str = strchr(buf, ':');
+	if (child_str == NULL)
+		return -EINVAL;
+	*child_str = '\0';
+	child_str++;
 
-	/*
-	 * Format of |buf| string should be <UID>:<UID>.
-	 * Find location of ":" in kern_buf (copied from |buf|).
-	 */
-	first_substring_length = strcspn(kern_buf, separator);
-	if (first_substring_length == 0 || first_substring_length == len) {
-		ret = -EINVAL;
-		goto free_kern;
+	ret = kstrtou32(buf, 0, &parsed_parent);
+	if (ret)
+		return ret;
+
+	ret = kstrtou32(child_str, 0, &parsed_child);
+	if (ret)
+		return ret;
+
+	rule->src_uid = make_kuid(file->f_cred->user_ns, parsed_parent);
+	rule->dst_uid = make_kuid(file->f_cred->user_ns, parsed_child);
+	if (!uid_valid(rule->src_uid) || !uid_valid(rule->dst_uid))
+		return -EINVAL;
+
+	return 0;
+}
+
+static void __release_ruleset(struct rcu_head *rcu)
+{
+	struct setuid_ruleset *pol =
+		container_of(rcu, struct setuid_ruleset, rcu);
+	int bucket;
+	struct setuid_rule *rule;
+	struct hlist_node *tmp;
+
+	hash_for_each_safe(pol->rules, bucket, tmp, rule, next)
+		kfree(rule);
+	kfree(pol->policy_str);
+	kfree(pol);
+}
+
+static void release_ruleset(struct setuid_ruleset *pol)
+{
+	call_rcu(&pol->rcu, __release_ruleset);
+}
+
+static void insert_rule(struct setuid_ruleset *pol, struct setuid_rule *rule)
+{
+	hash_add(pol->rules, &rule->next, __kuid_val(rule->src_uid));
+}
+
+static int verify_ruleset(struct setuid_ruleset *pol)
+{
+	int bucket;
+	struct setuid_rule *rule, *nrule;
+	int res = 0;
+
+	hash_for_each(pol->rules, bucket, rule, next) {
+		if (_setuid_policy_lookup(pol, rule->dst_uid, INVALID_UID) ==
+		    SIDPOL_DEFAULT) {
+			pr_warn("insecure policy detected: uid %d is constrained but transitively unconstrained through uid %d\n",
+				__kuid_val(rule->src_uid),
+				__kuid_val(rule->dst_uid));
+			res = -EINVAL;
+
+			/* fix it up */
+			nrule = kmalloc(sizeof(struct setuid_rule), GFP_KERNEL);
+			if (!nrule)
+				return -ENOMEM;
+			nrule->src_uid = rule->dst_uid;
+			nrule->dst_uid = rule->dst_uid;
+			insert_rule(pol, nrule);
+		}
 	}
+	return res;
+}
+
+static ssize_t handle_policy_update(struct file *file,
+				    const char __user *ubuf, size_t len)
+{
+	struct setuid_ruleset *pol;
+	char *buf, *p, *end;
+	int err;
 
-	parent_buf = kmemdup_nul(kern_buf, first_substring_length, GFP_KERNEL);
-	if (!parent_buf) {
-		ret = -ENOMEM;
-		goto free_kern;
+	pol = kmalloc(sizeof(struct setuid_ruleset), GFP_KERNEL);
+	if (!pol)
+		return -ENOMEM;
+	pol->policy_str = NULL;
+	hash_init(pol->rules);
+
+	p = buf = memdup_user_nul(ubuf, len);
+	if (IS_ERR(buf)) {
+		err = PTR_ERR(buf);
+		goto out_free_pol;
+	}
+	pol->policy_str = kstrdup(buf, GFP_KERNEL);
+	if (pol->policy_str == NULL) {
+		err = -ENOMEM;
+		goto out_free_buf;
 	}
 
-	ret = kstrtol(parent_buf, 0, &parsed_parent);
-	if (ret)
-		goto free_both;
+	/* policy lines, including the last one, end with \n */
+	while (*p != '\0') {
+		struct setuid_rule *rule;
 
-	child_buf = kern_buf + first_substring_length + 1;
-	ret = kstrtol(child_buf, 0, &parsed_child);
-	if (ret)
-		goto free_both;
+		end = strchr(p, '\n');
+		if (end == NULL) {
+			err = -EINVAL;
+			goto out_free_buf;
+		}
+		*end = '\0';
 
-	*parent = make_kuid(current_user_ns(), parsed_parent);
-	if (!uid_valid(*parent)) {
-		ret = -EINVAL;
-		goto free_both;
+		rule = kmalloc(sizeof(struct setuid_rule), GFP_KERNEL);
+		if (!rule) {
+			err = -ENOMEM;
+			goto out_free_buf;
 		}
 
-	*child = make_kuid(current_user_ns(), parsed_child);
-	if (!uid_valid(*child)) {
-		ret = -EINVAL;
-		goto free_both;
+		err = parse_policy_line(file, p, rule);
+		if (err)
+			goto out_free_rule;
+
+		if (_setuid_policy_lookup(pol, rule->src_uid, rule->dst_uid) ==
+		    SIDPOL_ALLOWED) {
+			pr_warn("bad policy: duplicate entry\n");
+			err = -EEXIST;
+			goto out_free_rule;
 		}
 
-free_both:
-	kfree(parent_buf);
-free_kern:
-	kfree(kern_buf);
-	return ret;
+		insert_rule(pol, rule);
+		p = end + 1;
+		continue;
+
+out_free_rule:
+		kfree(rule);
+		goto out_free_buf;
+	}
+
+	err = verify_ruleset(pol);
+	/* bogus policy falls through after fixing it up */
+	if (err && err != -EINVAL)
+		goto out_free_buf;
+
+	/*
+	 * Everything looks good, apply the policy and release the old one.
+	 * What we really want here is an xchg() wrapper for RCU, but since that
+	 * doesn't currently exist, just use a spinlock for now.
+	 */
+	mutex_lock(&policy_update_lock);
+	rcu_swap_protected(safesetid_setuid_rules, pol,
+			   lockdep_is_held(&policy_update_lock));
+	mutex_unlock(&policy_update_lock);
+	err = len;
+
+out_free_buf:
+	kfree(buf);
+out_free_pol:
+	release_ruleset(pol);
+	return err;
 }
 
 static ssize_t safesetid_file_write(struct file *file,
@@ -104,90 +196,65 @@ static ssize_t safesetid_file_write(struct file *file,
 				    size_t len,
 				    loff_t *ppos)
 {
-	struct safesetid_file_entry *file_entry =
-		file->f_inode->i_private;
-	kuid_t parent;
-	kuid_t child;
-	int ret;
-
-	if (!ns_capable(current_user_ns(), CAP_MAC_ADMIN))
+	if (!file_ns_capable(file, &init_user_ns, CAP_MAC_ADMIN))
 		return -EPERM;
 
 	if (*ppos != 0)
 		return -EINVAL;
 
-	switch (file_entry->type) {
-	case SAFESETID_WHITELIST_FLUSH:
-		flush_safesetid_whitelist_entries();
-		break;
-	case SAFESETID_WHITELIST_ADD:
-		ret = parse_safesetid_whitelist_policy(buf, len, &parent,
-								 &child);
-		if (ret)
-			return ret;
+	return handle_policy_update(file, buf, len);
+}
 
-		ret = add_safesetid_whitelist_entry(parent, child);
-		if (ret)
-			return ret;
-		break;
-	default:
-		pr_warn("Unknown securityfs file %d\n", file_entry->type);
-		break;
-	}
+static ssize_t safesetid_file_read(struct file *file, char __user *buf,
+				   size_t len, loff_t *ppos)
+{
+	ssize_t res = 0;
+	struct setuid_ruleset *pol;
+	const char *kbuf;
 
-	/* Return len on success so caller won't keep trying to write */
-	return len;
+	mutex_lock(&policy_update_lock);
+	pol = rcu_dereference_protected(safesetid_setuid_rules,
+					lockdep_is_held(&policy_update_lock));
+	if (pol) {
+		kbuf = pol->policy_str;
+		res = simple_read_from_buffer(buf, len, ppos,
+					      kbuf, strlen(kbuf));
+	}
+	mutex_unlock(&policy_update_lock);
+	return res;
 }
 
 static const struct file_operations safesetid_file_fops = {
+	.read = safesetid_file_read,
 	.write = safesetid_file_write,
 };
 
-static void safesetid_shutdown_securityfs(void)
-{
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(safesetid_files); ++i) {
-		struct safesetid_file_entry *entry =
-			&safesetid_files[i];
-		securityfs_remove(entry->dentry);
-		entry->dentry = NULL;
-	}
-
-	securityfs_remove(safesetid_policy_dir);
-	safesetid_policy_dir = NULL;
-}
-
 static int __init safesetid_init_securityfs(void)
 {
-	int i;
 	int ret;
+	struct dentry *policy_dir;
+	struct dentry *policy_file;
 
 	if (!safesetid_initialized)
 		return 0;
 
-	safesetid_policy_dir = securityfs_create_dir("safesetid", NULL);
-	if (IS_ERR(safesetid_policy_dir)) {
-		ret = PTR_ERR(safesetid_policy_dir);
+	policy_dir = securityfs_create_dir("safesetid", NULL);
+	if (IS_ERR(policy_dir)) {
+		ret = PTR_ERR(policy_dir);
 		goto error;
 	}
 
-	for (i = 0; i < ARRAY_SIZE(safesetid_files); ++i) {
-		struct safesetid_file_entry *entry =
-			&safesetid_files[i];
-		entry->dentry = securityfs_create_file(
-			entry->name, 0200, safesetid_policy_dir,
-			entry, &safesetid_file_fops);
-		if (IS_ERR(entry->dentry)) {
-			ret = PTR_ERR(entry->dentry);
+	policy_file = securityfs_create_file("whitelist_policy", 0600,
+			policy_dir, NULL, &safesetid_file_fops);
+	if (IS_ERR(policy_file)) {
+		ret = PTR_ERR(policy_file);
 		goto error;
 	}
-	}
 
 	return 0;
 
 error:
-	safesetid_shutdown_securityfs();
+	securityfs_remove(policy_dir);
 	return ret;
 }
 fs_initcall(safesetid_init_securityfs);

tools/testing/selftests/safesetid/safesetid-test.c

@@ -142,23 +142,19 @@ static void ensure_securityfs_mounted(void)
 
 static void write_policies(void)
 {
+	static char *policy_str =
+		"1:2\n"
+		"1:3\n"
+		"2:2\n"
+		"3:3\n";
 	ssize_t written;
 	int fd;
 
 	fd = open(add_whitelist_policy_file, O_WRONLY);
 	if (fd < 0)
 		die("cant open add_whitelist_policy file\n");
-	written = write(fd, "1:2", strlen("1:2"));
-	if (written != strlen("1:2")) {
-		if (written >= 0) {
-			die("short write to %s\n", add_whitelist_policy_file);
-		} else {
-			die("write to %s failed: %s\n",
-				add_whitelist_policy_file, strerror(errno));
-		}
-	}
-	written = write(fd, "1:3", strlen("1:3"));
-	if (written != strlen("1:3")) {
+	written = write(fd, policy_str, strlen(policy_str));
+	if (written != strlen(policy_str)) {
 		if (written >= 0) {
 			die("short write to %s\n", add_whitelist_policy_file);
 		} else {