Ready for merge.

On Thu, Apr 11, 2019 at 1:38 PM Kees Cook [off-list ref] wrote:

On Thu, Apr 11, 2019 at 1:12 PM Micah Morton [off-list ref] wrote:

quoted

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>

Reviewed-by: Kees Cook <redacted>

-Kees

quoted

---
Changes since the last patch: Instead of failing open when userspace
configures an unconstrained (and vulnerable) policy, fix up the policy
to make sure it is safe by restricting the un-constrained UIDs. Return
EINVAL from the policy write in the case that userspace writes an
unconstrained policy. Also move hash_add() into a small helper function.
 security/safesetid/securityfs.c               | 38 ++++++++++++++++++-
 .../selftests/safesetid/safesetid-test.c      |  4 +-
 2 files changed, 40 insertions(+), 2 deletions(-)

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 @@ 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

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

--
Kees Cook