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

[Kees Cook]

On Wed, Apr 10, 2019 at 9:56 AM Micah Morton <mortonm at chromium.org> wrote:
>
> From: Jann Horn <jannh at 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 at google.com>
> Signed-off-by: Micah Morton <mortonm at 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;
> +}

Why fail open? How about having the policy automatically add the
constraints (since the entire policy is known at verification time)?

-Kees

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


-- 
Kees Cook