[v2,09/10] LSM: SafeSetID: verify transitive constrainedness
diff mbox series

Message ID 20190411201243.167800-1-mortonm@chromium.org
State New
Headers show
Series
  • Untitled series #104475
Related show

Commit Message

Micah Morton April 11, 2019, 8:12 p.m. UTC
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>
---
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(-)

Comments

Kees Cook April 11, 2019, 8:38 p.m. UTC | #1
On Thu, Apr 11, 2019 at 1:12 PM Micah Morton <mortonm@chromium.org> wrote:
>
> 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 <keescook@chromium.org>

-Kees

> ---
> 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
Micah Morton May 7, 2019, 3:03 p.m. UTC | #2
Ready for merge.

On Thu, Apr 11, 2019 at 1:38 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Apr 11, 2019 at 1:12 PM Micah Morton <mortonm@chromium.org> wrote:
> >
> > 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 <keescook@chromium.org>
>
> -Kees
>
> > ---
> > 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

Patch
diff mbox series

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;