[PATCH 09/10] LSM: SafeSetID: verify transitive constrainedness

[Micah Morton <mortonm@chromium.org>]

From: Jann Horn <jannh@google.com>

Someone might write a ruleset like the following, expecting that it
securely constrains UID 1 to UIDs 1, 2 and 3:

    1:2
    1:3

However, because no constraints are applied to UIDs 2 and 3, an attacker
with UID 1 can simply first switch to UID 2, then switch to any UID from
there. The secure way to write this ruleset would be:

    1:2
    1:3
    2:2
    3:3

, which uses "transition to self" as a way to inhibit the default-allow
policy without allowing anything specific.

This is somewhat unintuitive. To make sure that policy authors don't
accidentally write insecure policies because of this, let the kernel verify
that a new ruleset does not contain any entries that are constrained, but
transitively unconstrained.

Signed-off-by: Jann Horn <jannh@google.com>
Signed-off-by: Micah Morton <mortonm@chromium.org>
---
 security/safesetid/securityfs.c               | 21 +++++++++++++++++++
 .../selftests/safesetid/safesetid-test.c      |  4 +++-
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
index 7a08fff2bc14..3ec64487f0e9 100644
--- a/security/safesetid/securityfs.c
+++ b/security/safesetid/securityfs.c
@@ -77,6 +77,23 @@ static void release_ruleset(struct setuid_ruleset *pol)
 		call_rcu(&pol->rcu, __release_ruleset);
 }
 
+static int verify_ruleset(struct setuid_ruleset *pol)
+{
+	int bucket;
+	struct setuid_rule *rule;
+
+	hash_for_each(pol->rules, bucket, rule, next) {
+		if (_setuid_policy_lookup(pol, rule->dst_uid, INVALID_UID) ==
+		    SIDPOL_DEFAULT) {
+			pr_warn("insecure policy rejected: uid %d is constrained but transitively unconstrained through uid %d\n",
+				__kuid_val(rule->src_uid),
+				__kuid_val(rule->dst_uid));
+			return -EINVAL;
+		}
+	}
+	return 0;
+}
+
 static ssize_t handle_policy_update(struct file *file,
 				    const char __user *ubuf, size_t len)
 {
@@ -139,6 +156,10 @@ static ssize_t handle_policy_update(struct file *file,
 		goto out_free_buf;
 	}
 
+	err = verify_ruleset(pol);
+	if (err)
+		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
diff --git a/tools/testing/selftests/safesetid/safesetid-test.c b/tools/testing/selftests/safesetid/safesetid-test.c
index 4f03813d1911..8f40c6ecdad1 100644
--- a/tools/testing/selftests/safesetid/safesetid-test.c
+++ b/tools/testing/selftests/safesetid/safesetid-test.c
@@ -144,7 +144,9 @@ static void write_policies(void)
 {
 	static char *policy_str =
 		"1:2\n"
-		"1:3\n";
+		"1:3\n"
+		"2:2\n"
+		"3:3\n";
 	ssize_t written;
 	int fd;
 
-- 
2.21.0.392.gf8f6787159e-goog