* [PATCH v2 09/10] LSM: SafeSetID: verify transitive constrainedness
@ 2019-04-11 20:12 Micah Morton
2019-04-11 20:38 ` Kees Cook
0 siblings, 1 reply; 3+ messages in thread
From: Micah Morton @ 2019-04-11 20:12 UTC (permalink / raw)
To: jmorris, keescook, casey, linux-security-module; +Cc: Jann Horn, Micah Morton
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(-)
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2 09/10] LSM: SafeSetID: verify transitive constrainedness
2019-04-11 20:12 [PATCH v2 09/10] LSM: SafeSetID: verify transitive constrainedness Micah Morton
@ 2019-04-11 20:38 ` Kees Cook
2019-05-07 15:03 ` Micah Morton
0 siblings, 1 reply; 3+ messages in thread
From: Kees Cook @ 2019-04-11 20:38 UTC (permalink / raw)
To: Micah Morton
Cc: James Morris, Casey Schaufler, linux-security-module, Jann Horn
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2 09/10] LSM: SafeSetID: verify transitive constrainedness
2019-04-11 20:38 ` Kees Cook
@ 2019-05-07 15:03 ` Micah Morton
0 siblings, 0 replies; 3+ messages in thread
From: Micah Morton @ 2019-05-07 15:03 UTC (permalink / raw)
To: Kees Cook; +Cc: James Morris, Casey Schaufler, linux-security-module, Jann Horn
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-05-07 15:03 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-11 20:12 [PATCH v2 09/10] LSM: SafeSetID: verify transitive constrainedness Micah Morton
2019-04-11 20:38 ` Kees Cook
2019-05-07 15:03 ` Micah Morton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).