| Message ID | 20190410165619.212464-1-mortonm@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/10] LSM: SafeSetID: fix pr_warn() to include newline | expand |
On Wed, Apr 10, 2019 at 9:56 AM 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> > --- > 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 >
On Wed, Apr 10, 2019 at 7:28 PM Kees Cook <keescook@chromium.org> wrote: > On Wed, Apr 10, 2019 at 9:56 AM 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> > > --- > > 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)? Are you suggesting to print a warning, automatically add the constraint, apply the policy, and still return -EINVAL? I guess that would work. I think it is a good idea to require userspace to explicitly write these rules, since implicitly-added rules might make the rules harder to understand for someone reading a policy file.
On Wed, Apr 10, 2019 at 10:37 AM Jann Horn <jannh@google.com> wrote: > > On Wed, Apr 10, 2019 at 7:28 PM Kees Cook <keescook@chromium.org> wrote: > > On Wed, Apr 10, 2019 at 9:56 AM 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> > > > --- > > > 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)? > > Are you suggesting to print a warning, automatically add the > constraint, apply the policy, and still return -EINVAL? I guess that > would work. I meant not return EINVAL in this case but just fix it. > I think it is a good idea to require userspace to explicitly write > these rules, since implicitly-added rules might make the rules harder > to understand for someone reading a policy file. Mostly it's a question for the userspace design. If EINVAL is checked, then I think it's probably fine, but will that error propagate to whatever will eventually launch the processes that are supposed to be confined? Since it's a detectable case, why not just fix it (with a warning) and run safely?
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; +} + 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;