* [REVIEW PATCH v2] ptrace: reintroduce usage of subjective credentials in ptrace_has_cap() @ 2020-01-16 22:45 Christian Brauner 2020-01-17 2:29 ` Kees Cook 0 siblings, 1 reply; 5+ messages in thread From: Christian Brauner @ 2020-01-16 22:45 UTC (permalink / raw) To: linux-kernel, Kees Cook, Jann Horn, Oleg Nesterov Cc: stable, Serge Hallyn, Christian Brauner, Eric Paris Commit 69f594a38967 ("ptrace: do not audit capability check when outputing /proc/pid/stat") introduced the ability to opt out of audit messages for accesses to various proc files since they are not violations of policy. While doing so it somehow switched the check from ns_capable() to has_ns_capability{_noaudit}(). That means it switched from checking the subjective credentials of the task to using the objective credentials. I couldn't find the original lkml thread and so I don't know why this switch was done. But it seems wrong since ptrace_has_cap() is currently only used in ptrace_may_access(). And it's used to check whether the calling task (subject) has the CAP_SYS_PTRACE capability in the provided user namespace to operate on the target task (object). According to the cred.h comments this would mean the subjective credentials of the calling task need to be used. This switches it to use security_capable() because we only call ptrace_has_cap() in ptrace_may_access() and in there we already have a stable reference to the calling tasks creds under cred_guard_mutex so there's no need to go through another series of dereferences and rcu locking done in ns_capable{_noaudit}(). As one example where this might be particularly problematic, Jann pointed out that in combination with the upcoming IORING_OP_OPENAT feature, this bug might allow unprivileged users to bypass the capability checks while asynchronously opening files like /proc/*/mem, because the capability checks for this would be performed against kernel credentials. Cc: Oleg Nesterov <oleg@redhat.com> Cc: Eric Paris <eparis@redhat.com> Cc: stable@vger.kernel.org Reviewed-by: Serge Hallyn <serge@hallyn.com> Reviewed-by: Jann Horn <jannh@google.com> Fixes: 69f594a38967 ("ptrace: do not audit capability check when outputing /proc/pid/stat") Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> --- kernel/ptrace.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/kernel/ptrace.c b/kernel/ptrace.c index cb9ddcc08119..d146133e97f1 100644 --- a/kernel/ptrace.c +++ b/kernel/ptrace.c @@ -264,12 +264,13 @@ static int ptrace_check_attach(struct task_struct *child, bool ignore_state) return ret; } -static int ptrace_has_cap(struct user_namespace *ns, unsigned int mode) +static int ptrace_has_cap(const struct cred *cred, struct user_namespace *ns, + unsigned int mode) { if (mode & PTRACE_MODE_NOAUDIT) - return has_ns_capability_noaudit(current, ns, CAP_SYS_PTRACE); + return security_capable(cred, ns, CAP_SYS_PTRACE, CAP_OPT_NOAUDIT); else - return has_ns_capability(current, ns, CAP_SYS_PTRACE); + return security_capable(cred, ns, CAP_SYS_PTRACE, CAP_OPT_NONE); } /* Returns 0 on success, -errno on denial. */ @@ -321,7 +322,7 @@ static int __ptrace_may_access(struct task_struct *task, unsigned int mode) gid_eq(caller_gid, tcred->sgid) && gid_eq(caller_gid, tcred->gid)) goto ok; - if (ptrace_has_cap(tcred->user_ns, mode)) + if (ptrace_has_cap(cred, tcred->user_ns, mode)) goto ok; rcu_read_unlock(); return -EPERM; @@ -340,7 +341,7 @@ static int __ptrace_may_access(struct task_struct *task, unsigned int mode) mm = task->mm; if (mm && ((get_dumpable(mm) != SUID_DUMP_USER) && - !ptrace_has_cap(mm->user_ns, mode))) + !ptrace_has_cap(cred, mm->user_ns, mode))) return -EPERM; return security_ptrace_access_check(task, mode); base-commit: b3a987b0264d3ddbb24293ebff10eddfc472f653 -- 2.25.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [REVIEW PATCH v2] ptrace: reintroduce usage of subjective credentials in ptrace_has_cap() 2020-01-16 22:45 [REVIEW PATCH v2] ptrace: reintroduce usage of subjective credentials in ptrace_has_cap() Christian Brauner @ 2020-01-17 2:29 ` Kees Cook 2020-01-17 5:16 ` Christian Brauner 2020-01-17 11:08 ` Christian Brauner 0 siblings, 2 replies; 5+ messages in thread From: Kees Cook @ 2020-01-17 2:29 UTC (permalink / raw) To: Christian Brauner Cc: linux-kernel, Jann Horn, Oleg Nesterov, stable, Serge Hallyn, Eric Paris On Thu, Jan 16, 2020 at 11:45:18PM +0100, Christian Brauner wrote: > Commit 69f594a38967 ("ptrace: do not audit capability check when outputing /proc/pid/stat") > introduced the ability to opt out of audit messages for accesses to > various proc files since they are not violations of policy. > While doing so it somehow switched the check from ns_capable() to > has_ns_capability{_noaudit}(). That means it switched from checking the > subjective credentials of the task to using the objective credentials. I > couldn't find the original lkml thread and so I don't know why this switch > was done. But it seems wrong since ptrace_has_cap() is currently only used > in ptrace_may_access(). And it's used to check whether the calling task > (subject) has the CAP_SYS_PTRACE capability in the provided user namespace > to operate on the target task (object). According to the cred.h comments > this would mean the subjective credentials of the calling task need to be > used. I don't follow this description. As far as I can see, both the current code and your patch end up using current's cred, yes? I'm not following the subjective/objective change mentioned here. Before: bool has_ns_capability(struct task_struct *t, struct user_namespace *ns, int cap) { int ret; rcu_read_lock(); ret = security_capable(__task_cred(t), ns, cap, CAP_OPT_NONE); rcu_read_unlock(); return (ret == 0); } ... return has_ns_capability(current, ns, CAP_SYS_PTRACE) After: const struct cred *cred = current_cred(), ... ... return security_capable(cred, ns, CAP_SYS_PTRACE, CAP_OPT_NOAUDIT); The cred passed to security_capable() is the subject before and after. > This switches it to use security_capable() because we only call > ptrace_has_cap() in ptrace_may_access() and in there we already have a > stable reference to the calling tasks creds under cred_guard_mutex so > there's no need to go through another series of dereferences and rcu > locking done in ns_capable{_noaudit}(). This makes sense to me -- now there's no possible race on the cred changing between the two ptrace_has_cap() checks, yes? However, I'm still trying to see where cred_guard_mutex() comes into play for callers of ptrace_may_access(). I see it for the object ("task" arg in ptrace_may_access()), but if this is dealing with the cred on current, it's just the RCU read lock protecting it (which I think is fine here), but seems confusing in the commit log. > As one example where this might be particularly problematic, Jann pointed > out that in combination with the upcoming IORING_OP_OPENAT feature, this > bug might allow unprivileged users to bypass the capability checks while > asynchronously opening files like /proc/*/mem, because the capability > checks for this would be performed against kernel credentials. As in, winning a race between the two ptrace_has_cap() calls across a cred transition? > Cc: Oleg Nesterov <oleg@redhat.com> > Cc: Eric Paris <eparis@redhat.com> > Cc: stable@vger.kernel.org > Reviewed-by: Serge Hallyn <serge@hallyn.com> > Reviewed-by: Jann Horn <jannh@google.com> > Fixes: 69f594a38967 ("ptrace: do not audit capability check when outputing /proc/pid/stat") > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> > --- > kernel/ptrace.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/kernel/ptrace.c b/kernel/ptrace.c > index cb9ddcc08119..d146133e97f1 100644 > --- a/kernel/ptrace.c > +++ b/kernel/ptrace.c > @@ -264,12 +264,13 @@ static int ptrace_check_attach(struct task_struct *child, bool ignore_state) > return ret; > } > > -static int ptrace_has_cap(struct user_namespace *ns, unsigned int mode) > +static int ptrace_has_cap(const struct cred *cred, struct user_namespace *ns, > + unsigned int mode) > { > if (mode & PTRACE_MODE_NOAUDIT) > - return has_ns_capability_noaudit(current, ns, CAP_SYS_PTRACE); > + return security_capable(cred, ns, CAP_SYS_PTRACE, CAP_OPT_NOAUDIT); > else > - return has_ns_capability(current, ns, CAP_SYS_PTRACE); > + return security_capable(cred, ns, CAP_SYS_PTRACE, CAP_OPT_NONE); > } Style nit -- can we just make this a single invocation of security_capable(), something like: return security_capable(cred, ns, CAP_SYS_PTRACE, mode & PTRACE_MODE_NOAUDIT ? CAP_OPT_NOAUDIT, : CAP_OPT_NONE) == 0; Obviously not required, but the longer if hurts my eyes. ;) > > /* Returns 0 on success, -errno on denial. */ > @@ -321,7 +322,7 @@ static int __ptrace_may_access(struct task_struct *task, unsigned int mode) > gid_eq(caller_gid, tcred->sgid) && > gid_eq(caller_gid, tcred->gid)) > goto ok; > - if (ptrace_has_cap(tcred->user_ns, mode)) > + if (ptrace_has_cap(cred, tcred->user_ns, mode)) > goto ok; > rcu_read_unlock(); > return -EPERM; > @@ -340,7 +341,7 @@ static int __ptrace_may_access(struct task_struct *task, unsigned int mode) > mm = task->mm; > if (mm && > ((get_dumpable(mm) != SUID_DUMP_USER) && > - !ptrace_has_cap(mm->user_ns, mode))) > + !ptrace_has_cap(cred, mm->user_ns, mode))) > return -EPERM; > > return security_ptrace_access_check(task, mode); > > base-commit: b3a987b0264d3ddbb24293ebff10eddfc472f653 > -- > 2.25.0 > So, I think this change looks correct, but I find the commit subject and log confusing (perhaps because I am dense) and misleading (again, perhaps because I am dense). -- Kees Cook ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [REVIEW PATCH v2] ptrace: reintroduce usage of subjective credentials in ptrace_has_cap() 2020-01-17 2:29 ` Kees Cook @ 2020-01-17 5:16 ` Christian Brauner 2020-01-17 21:07 ` Kees Cook 2020-01-17 11:08 ` Christian Brauner 1 sibling, 1 reply; 5+ messages in thread From: Christian Brauner @ 2020-01-17 5:16 UTC (permalink / raw) To: Kees Cook Cc: linux-kernel, Jann Horn, Oleg Nesterov, stable, Serge Hallyn, Eric Paris On Thu, Jan 16, 2020 at 06:29:26PM -0800, Kees Cook wrote: > On Thu, Jan 16, 2020 at 11:45:18PM +0100, Christian Brauner wrote: > > Commit 69f594a38967 ("ptrace: do not audit capability check when outputing /proc/pid/stat") > > introduced the ability to opt out of audit messages for accesses to > > various proc files since they are not violations of policy. > > While doing so it somehow switched the check from ns_capable() to > > has_ns_capability{_noaudit}(). That means it switched from checking the > > subjective credentials of the task to using the objective credentials. I > > couldn't find the original lkml thread and so I don't know why this switch > > was done. But it seems wrong since ptrace_has_cap() is currently only used > > in ptrace_may_access(). And it's used to check whether the calling task > > (subject) has the CAP_SYS_PTRACE capability in the provided user namespace > > to operate on the target task (object). According to the cred.h comments > > this would mean the subjective credentials of the calling task need to be > > used. > > I don't follow this description. As far as I can see, both the current > code and your patch end up using current's cred, yes? I'm not following > the subjective/objective change mentioned here. > > Before: > bool has_ns_capability(struct task_struct *t, > struct user_namespace *ns, int cap) > { > int ret; > > rcu_read_lock(); > ret = security_capable(__task_cred(t), ns, cap, CAP_OPT_NONE); If I'm not mistaken, you're looking at the cuplrit: "__task_cred()": /** * __task_cred - Access a task's objective credentials * @task: The task to query * * Access the objective credentials of a task. The caller must hold the RCU * readlock. * * The result of this function should not be passed directly to get_cred(); * rather get_task_cred() should be used instead. */ #define __task_cred(task) \ rcu_dereference((task)->real_cred) > rcu_read_unlock(); > > return (ret == 0); > } > ... > return has_ns_capability(current, ns, CAP_SYS_PTRACE) > > After: > const struct cred *cred = current_cred(), ... > ... > return security_capable(cred, ns, CAP_SYS_PTRACE, CAP_OPT_NOAUDIT); > > The cred passed to security_capable() is the subject before and after. > > > This switches it to use security_capable() because we only call > > ptrace_has_cap() in ptrace_may_access() and in there we already have a > > stable reference to the calling tasks creds under cred_guard_mutex so > > there's no need to go through another series of dereferences and rcu > > locking done in ns_capable{_noaudit}(). > > This makes sense to me -- now there's no possible race on the cred > changing between the two ptrace_has_cap() checks, yes? > > However, I'm still trying to see where cred_guard_mutex() comes into > play for callers of ptrace_may_access(). I see it for the object > ("task" arg in ptrace_may_access()), but if this is dealing with the cred > on current, it's just the RCU read lock protecting it (which I think is > fine here), but seems confusing in the commit log. Ah, right. I'll drop that from the commit message and place in the rcu lock. Christian ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [REVIEW PATCH v2] ptrace: reintroduce usage of subjective credentials in ptrace_has_cap() 2020-01-17 5:16 ` Christian Brauner @ 2020-01-17 21:07 ` Kees Cook 0 siblings, 0 replies; 5+ messages in thread From: Kees Cook @ 2020-01-17 21:07 UTC (permalink / raw) To: Christian Brauner Cc: linux-kernel, Jann Horn, Oleg Nesterov, stable, Serge Hallyn, Eric Paris On Fri, Jan 17, 2020 at 06:16:23AM +0100, Christian Brauner wrote: > On Thu, Jan 16, 2020 at 06:29:26PM -0800, Kees Cook wrote: > > On Thu, Jan 16, 2020 at 11:45:18PM +0100, Christian Brauner wrote: > > > Commit 69f594a38967 ("ptrace: do not audit capability check when outputing /proc/pid/stat") > > > introduced the ability to opt out of audit messages for accesses to > > > various proc files since they are not violations of policy. > > > While doing so it somehow switched the check from ns_capable() to > > > has_ns_capability{_noaudit}(). That means it switched from checking the > > > subjective credentials of the task to using the objective credentials. I > > > couldn't find the original lkml thread and so I don't know why this switch > > > was done. But it seems wrong since ptrace_has_cap() is currently only used > > > in ptrace_may_access(). And it's used to check whether the calling task > > > (subject) has the CAP_SYS_PTRACE capability in the provided user namespace > > > to operate on the target task (object). According to the cred.h comments > > > this would mean the subjective credentials of the calling task need to be > > > used. > > > > I don't follow this description. As far as I can see, both the current > > code and your patch end up using current's cred, yes? I'm not following > > the subjective/objective change mentioned here. > > > > Before: > > bool has_ns_capability(struct task_struct *t, > > struct user_namespace *ns, int cap) > > { > > int ret; > > > > rcu_read_lock(); > > ret = security_capable(__task_cred(t), ns, cap, CAP_OPT_NONE); > > If I'm not mistaken, you're looking at the cuplrit: "__task_cred()": > [...] > #define __task_cred(task) \ > rcu_dereference((task)->real_cred) Ah! Yes, thank you. cred vs real_cred. That's what I missed! > > However, I'm still trying to see where cred_guard_mutex() comes into > > play for callers of ptrace_may_access(). I see it for the object > > ("task" arg in ptrace_may_access()), but if this is dealing with the cred > > on current, it's just the RCU read lock protecting it (which I think is > > fine here), but seems confusing in the commit log. > > Ah, right. I'll drop that from the commit message and place in the rcu > lock. Thanks for the clarification. With that adjusted, please consider it: Reviewed-by: Kees Cook <keescook@chromium.org> (I wonder how hard it might be to build some self-tests for this to catch future glitches...) -- Kees Cook ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [REVIEW PATCH v2] ptrace: reintroduce usage of subjective credentials in ptrace_has_cap() 2020-01-17 2:29 ` Kees Cook 2020-01-17 5:16 ` Christian Brauner @ 2020-01-17 11:08 ` Christian Brauner 1 sibling, 0 replies; 5+ messages in thread From: Christian Brauner @ 2020-01-17 11:08 UTC (permalink / raw) To: Kees Cook Cc: linux-kernel, Jann Horn, Oleg Nesterov, stable, Serge Hallyn, Eric Paris On Thu, Jan 16, 2020 at 06:29:26PM -0800, Kees Cook wrote: > On Thu, Jan 16, 2020 at 11:45:18PM +0100, Christian Brauner wrote: > > As one example where this might be particularly problematic, Jann pointed > > out that in combination with the upcoming IORING_OP_OPENAT feature, this > > bug might allow unprivileged users to bypass the capability checks while > > asynchronously opening files like /proc/*/mem, because the capability > > checks for this would be performed against kernel credentials. To follow up on this part of your mail. No, afaict, it's not aboutwinning a race. It's way simpler... When io uring creates a new kernel context it records the subjective credentials of the caller: ctx = io_ring_ctx_alloc(p); if (!ctx) { if (account_mem) io_unaccount_mem(user, ring_pages(p->sq_entries, p->cq_entries)); free_uid(user); return -ENOMEM; } ctx->compat = in_compat_syscall(); ctx->account_mem = account_mem; ctx->user = user; ------> ctx->creds = get_current_cred(); <------ Later on, when it starts to do work it creates a kernel thread: ctx->sqo_thread = kthread_create_on_cpu(io_sq_thread, ctx, cpu, "io_uring-sq"); } else { ctx->sqo_thread = kthread_create(io_sq_thread, ctx, "io_uring-sq"); } and registers io_sq_thread as "callback". The callback io_sq_thread() runs __with kernel creds__. To prevent this from becoming an issue io_sq_thread() will override the __subjective credentials__ with the callers credentials: old_cred = override_creds(ctx->creds); But ptrace_has_cap() currently looks at __task_cred(current) aka __real_cred__. This means once IORING_OP_OPENAT and IORING_OP_OPENAT2 lands in v5.5-rc6 it is more or less trivial for an unprivileged user to bypass ptrace_may_access(). Christian ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-01-17 21:07 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-16 22:45 [REVIEW PATCH v2] ptrace: reintroduce usage of subjective credentials in ptrace_has_cap() Christian Brauner 2020-01-17 2:29 ` Kees Cook 2020-01-17 5:16 ` Christian Brauner 2020-01-17 21:07 ` Kees Cook 2020-01-17 11:08 ` Christian Brauner
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).