From 03638e62f55f27e7a96d6b1175e75b7a81e562b3 Mon Sep 17 00:00:00 2001 From: Jann Horn Date: Wed, 10 Apr 2019 09:56:05 -0700 Subject: 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 Signed-off-by: Micah Morton --- tools/testing/selftests/safesetid/safesetid-test.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) (limited to 'tools') diff --git a/tools/testing/selftests/safesetid/safesetid-test.c b/tools/testing/selftests/safesetid/safesetid-test.c index 892c8e8b1b8b..4f03813d1911 100644 --- a/tools/testing/selftests/safesetid/safesetid-test.c +++ b/tools/testing/selftests/safesetid/safesetid-test.c @@ -142,23 +142,17 @@ static void ensure_securityfs_mounted(void) static void write_policies(void) { + static char *policy_str = + "1:2\n" + "1: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 { -- cgit v1.2.3 From 4f72123da579655855301b591535a1415224f123 Mon Sep 17 00:00:00 2001 From: Jann Horn Date: Thu, 11 Apr 2019 13:12:43 -0700 Subject: 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: Jann Horn Signed-off-by: Micah Morton --- security/safesetid/securityfs.c | 38 +++++++++++++++++++++- tools/testing/selftests/safesetid/safesetid-test.c | 4 ++- 2 files changed, 40 insertions(+), 2 deletions(-) (limited to 'tools') diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c index 997b403c6255..d568e17dd773 100644 --- a/security/safesetid/securityfs.c +++ b/security/safesetid/securityfs.c @@ -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 @@ out_free_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 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; -- cgit v1.2.3