| Message ID | 20190411201243.167800-1-mortonm@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
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
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
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;