LSM: SafeSetID: rewrite userspace API to atomic updates

The current API of the SafeSetID LSM uses one write() per rule, and applies
each written rule instantly. This has several downsides:

 - While a policy is being loaded, once a single parent-child pair has been
   loaded, the parent is restricted to that specific child, even if
   subsequent rules would allow transitions to other child UIDs. This means
   that during policy loading, set*uid() can randomly fail.
 - To replace the policy without rebooting, it is necessary to first flush
   all old rules. This creates a time window in which no constraints are
   placed on the use of CAP_SETUID.
 - If we want to perform sanity checks on the final policy, this requires
   that the policy isn't constructed in a piecemeal fashion without telling
   the kernel when it's done.

Other kernel APIs - including things like the userns code and netfilter -
avoid this problem by performing updates atomically. Luckily, SafeSetID
hasn't landed in a stable (upstream) release yet, so maybe it's not too
late to completely change the API.

The new API for SafeSetID is: If you want to change the policy, open
"safesetid/whitelist_policy" and write the entire policy,
newline-delimited, in there.

Signed-off-by: Jann Horn <jannh@google.com>
Signed-off-by: Micah Morton <mortonm@chromium.org>

security/safesetid/lsm.c

@@ -24,28 +24,38 @@
 /* Flag indicating whether initialization completed */
 int safesetid_initialized;
 
-#define NUM_BITS 8 /* 256 buckets in hash table */
+struct setuid_ruleset __rcu *safesetid_setuid_rules;
 
-static DEFINE_HASHTABLE(safesetid_whitelist_hashtable, NUM_BITS);
-
-static DEFINE_SPINLOCK(safesetid_whitelist_hashtable_spinlock);
-
-static enum sid_policy_type setuid_policy_lookup(kuid_t src, kuid_t dst)
+/* Compute a decision for a transition from @src to @dst under @policy. */
+enum sid_policy_type _setuid_policy_lookup(struct setuid_ruleset *policy,
+		kuid_t src, kuid_t dst)
 {
-	struct entry *entry;
+	struct setuid_rule *rule;
 	enum sid_policy_type result = SIDPOL_DEFAULT;
 
-	rcu_read_lock();
-	hash_for_each_possible_rcu(safesetid_whitelist_hashtable,
-				   entry, next, __kuid_val(src)) {
-		if (!uid_eq(entry->src_uid, src))
+	hash_for_each_possible(policy->rules, rule, next, __kuid_val(src)) {
+		if (!uid_eq(rule->src_uid, src))
 			continue;
-		if (uid_eq(entry->dst_uid, dst)) {
-			rcu_read_unlock();
+		if (uid_eq(rule->dst_uid, dst))
 			return SIDPOL_ALLOWED;
-		}
 		result = SIDPOL_CONSTRAINED;
 	}
+	return result;
+}
+
+/*
+ * Compute a decision for a transition from @src to @dst under the active
+ * policy.
+ */
+static enum sid_policy_type setuid_policy_lookup(kuid_t src, kuid_t dst)
+{
+	enum sid_policy_type result = SIDPOL_DEFAULT;
+	struct setuid_ruleset *pol;
+
+	rcu_read_lock();
+	pol = rcu_dereference(safesetid_setuid_rules);
+	if (pol)
+		result = _setuid_policy_lookup(pol, src, dst);
 	rcu_read_unlock();
 	return result;
 }
@@ -139,52 +149,6 @@ static int safesetid_task_fix_setuid(struct cred *new,
 	return -EACCES;
 }
 
-int add_safesetid_whitelist_entry(kuid_t parent, kuid_t child)
-{
-	struct entry *new;
-
-	/* Return if entry already exists */
-	if (setuid_policy_lookup(parent, child) == SIDPOL_ALLOWED)
-		return 0;
-
-	new = kzalloc(sizeof(struct entry), GFP_KERNEL);
-	if (!new)
-		return -ENOMEM;
-	new->src_uid = parent;
-	new->dst_uid = child;
-	spin_lock(&safesetid_whitelist_hashtable_spinlock);
-	hash_add_rcu(safesetid_whitelist_hashtable,
-		     &new->next,
-		     __kuid_val(parent));
-	spin_unlock(&safesetid_whitelist_hashtable_spinlock);
-	return 0;
-}
-
-void flush_safesetid_whitelist_entries(void)
-{
-	struct entry *entry;
-	struct hlist_node *hlist_node;
-	unsigned int bkt_loop_cursor;
-	HLIST_HEAD(free_list);
-
-	/*
-	 * Could probably use hash_for_each_rcu here instead, but this should
-	 * be fine as well.
-	 */
-	spin_lock(&safesetid_whitelist_hashtable_spinlock);
-	hash_for_each_safe(safesetid_whitelist_hashtable, bkt_loop_cursor,
-			   hlist_node, entry, next) {
-		hash_del_rcu(&entry->next);
-		hlist_add_head(&entry->dlist, &free_list);
-	}
-	spin_unlock(&safesetid_whitelist_hashtable_spinlock);
-	synchronize_rcu();
-	hlist_for_each_entry_safe(entry, hlist_node, &free_list, dlist) {
-		hlist_del(&entry->dlist);
-		kfree(entry);
-	}
-}
-
 static struct security_hook_list safesetid_security_hooks[] = {
 	LSM_HOOK_INIT(task_fix_setuid, safesetid_task_fix_setuid),
 	LSM_HOOK_INIT(capable, safesetid_security_capable)

security/safesetid/lsm.h

@@ -21,12 +21,6 @@
 /* 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 */
@@ -35,18 +29,24 @@ enum sid_policy_type {
 
 /*
  * Hash table entry to store safesetid policy signifying that 'src_uid'
- * can setid to 'dst_uid'.
+ * can setuid to 'dst_uid'.
  */
-struct entry {
+struct setuid_rule {
 	struct hlist_node next;
-	struct hlist_node dlist; /* for deletion cleanup */
 	kuid_t src_uid;
 	kuid_t dst_uid;
 };
 
-/* Add entry to safesetid whitelist to allow 'parent' to setid to 'child'. */
-int add_safesetid_whitelist_entry(kuid_t parent, kuid_t child);
+#define SETID_HASH_BITS 8 /* 256 buckets in hash table */
+
+struct setuid_ruleset {
+	DECLARE_HASHTABLE(rules, SETID_HASH_BITS);
+	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 */