Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 4f72123d authored by Jann Horn's avatar Jann Horn Committed by Micah Morton
Browse files

LSM: SafeSetID: verify transitive constrainedness 



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: default avatarJann Horn <jannh@google.com>
Signed-off-by: default avatarMicah Morton <mortonm@chromium.org>
parent fbd9acb2

security/safesetid/securityfs.c

+37 −1
Original line number Diff line number Diff line
@@ -76,6 +76,37 @@ 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)

{
@@ -128,7 +159,7 @@ static ssize_t handle_policy_update(struct file *file, 
			goto out_free_rule;

		}



		hash_add(pol->rules, &rule->next, __kuid_val(rule->src_uid));

		insert_rule(pol, rule);

		p = end + 1;

		continue;
@@ -137,6 +168,11 @@ static ssize_t handle_policy_update(struct file *file, 
		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

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

+3 −1
Original line number Diff line number Diff line
@@ -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;