Re: [PATCH v1 1/2] ptrace: Set PF_SUPERPRIV when checking capability
On Fri, Oct 30, 2020 at 5:06 PM Mickaël Salaün wrote:
> On 30/10/2020 16:47, Jann Horn wrote:
> > On Fri, Oct 30, 2020 at 1:39 PM Mickaël Salaün wrote:
> >> Commit 69f594a38967 ("ptrace: do not audit capability check when outputing
> >> /proc/pid/stat") replaced the use of ns_capable() with
> >> has_ns_capability{,_noaudit}() which doesn't set PF_SUPERPRIV.
> >>
> >> Commit 6b3ad6649a4c ("ptrace: reintroduce usage of subjective credentials
> >> in
> >> ptrace_has_cap()") replaced has_ns_capability{,_noaudit}() with
> >> security_capable(), which doesn't set PF_SUPERPRIV neither.
> >>
> >> Since commit 98f368e9e263 ("kernel: Add noaudit variant of ns_capable()"),
> >> a
> >> new ns_capable_noaudit() helper is available. Let's use it!
> >>
> >> As a result, the signature of ptrace_has_cap() is restored to its original
> >> one.
> >>
> >> Cc: Christian Brauner
> >> Cc: Eric Paris
> >> Cc: Jann Horn
> >> Cc: Kees Cook
> >> Cc: Oleg Nesterov
> >> Cc: Serge E. Hallyn
> >> Cc: Tyler Hicks
> >> Cc: sta...@vger.kernel.org
> >> Fixes: 6b3ad6649a4c ("ptrace: reintroduce usage of subjective credentials
> >> in ptrace_has_cap()")
> >> Fixes: 69f594a38967 ("ptrace: do not audit capability check when outputing
> >> /proc/pid/stat")
> >> Signed-off-by: Mickaël Salaün
> >
> > Yeah... I guess this makes sense. (We'd have to undo or change it if
> > we ever end up needing to use a different set of credentials, e.g.
> > from ->f_cred, but I guess that's really something we should avoid
> > anyway.)
> >
> > Reviewed-by: Jann Horn
> >
> > with one nit:
> >
> >
> > [...]
> >> /* Returns 0 on success, -errno on denial. */
> >> static int __ptrace_may_access(struct task_struct *task, unsigned int
> >> mode)
> >> {
> >> - const struct cred *cred = current_cred(), *tcred;
> >> + const struct cred *const cred = current_cred(), *tcred;
> >
> > This is an unrelated change, and almost no kernel code marks local
> > pointer variables as "const". I would drop this change from the patch.
>
> This give guarantee that the cred variable will not be used for
> something else than current_cred(), which kinda prove that this patch
> doesn't change the behavior of __ptrace_may_access() by not using cred
> in ptrace_has_cap(). It doesn't hurt and I think it could be useful to
> spot issues when backporting.
And it might require an extra fixup while backporting because the next
line is different and that might cause the patch to not apply.
Re: [PATCH v1 1/2] ptrace: Set PF_SUPERPRIV when checking capability
On 30/10/2020 16:47, Jann Horn wrote:
> On Fri, Oct 30, 2020 at 1:39 PM Mickaël Salaün wrote:
>> Commit 69f594a38967 ("ptrace: do not audit capability check when outputing
>> /proc/pid/stat") replaced the use of ns_capable() with
>> has_ns_capability{,_noaudit}() which doesn't set PF_SUPERPRIV.
>>
>> Commit 6b3ad6649a4c ("ptrace: reintroduce usage of subjective credentials in
>> ptrace_has_cap()") replaced has_ns_capability{,_noaudit}() with
>> security_capable(), which doesn't set PF_SUPERPRIV neither.
>>
>> Since commit 98f368e9e263 ("kernel: Add noaudit variant of ns_capable()"), a
>> new ns_capable_noaudit() helper is available. Let's use it!
>>
>> As a result, the signature of ptrace_has_cap() is restored to its original
>> one.
>>
>> Cc: Christian Brauner
>> Cc: Eric Paris
>> Cc: Jann Horn
>> Cc: Kees Cook
>> Cc: Oleg Nesterov
>> Cc: Serge E. Hallyn
>> Cc: Tyler Hicks
>> Cc: sta...@vger.kernel.org
>> Fixes: 6b3ad6649a4c ("ptrace: reintroduce usage of subjective credentials in
>> ptrace_has_cap()")
>> Fixes: 69f594a38967 ("ptrace: do not audit capability check when outputing
>> /proc/pid/stat")
>> Signed-off-by: Mickaël Salaün
>
> Yeah... I guess this makes sense. (We'd have to undo or change it if
> we ever end up needing to use a different set of credentials, e.g.
> from ->f_cred, but I guess that's really something we should avoid
> anyway.)
>
> Reviewed-by: Jann Horn
>
> with one nit:
>
>
> [...]
>> /* Returns 0 on success, -errno on denial. */
>> static int __ptrace_may_access(struct task_struct *task, unsigned int mode)
>> {
>> - const struct cred *cred = current_cred(), *tcred;
>> + const struct cred *const cred = current_cred(), *tcred;
>
> This is an unrelated change, and almost no kernel code marks local
> pointer variables as "const". I would drop this change from the patch.
This give guarantee that the cred variable will not be used for
something else than current_cred(), which kinda prove that this patch
doesn't change the behavior of __ptrace_may_access() by not using cred
in ptrace_has_cap(). It doesn't hurt and I think it could be useful to
spot issues when backporting.
>
>> struct mm_struct *mm;
>> kuid_t caller_uid;
>> kgid_t caller_gid;
Re: [PATCH v1 1/2] ptrace: Set PF_SUPERPRIV when checking capability
On Fri, Oct 30, 2020 at 1:39 PM Mickaël Salaün wrote:
> Commit 69f594a38967 ("ptrace: do not audit capability check when outputing
> /proc/pid/stat") replaced the use of ns_capable() with
> has_ns_capability{,_noaudit}() which doesn't set PF_SUPERPRIV.
>
> Commit 6b3ad6649a4c ("ptrace: reintroduce usage of subjective credentials in
> ptrace_has_cap()") replaced has_ns_capability{,_noaudit}() with
> security_capable(), which doesn't set PF_SUPERPRIV neither.
>
> Since commit 98f368e9e263 ("kernel: Add noaudit variant of ns_capable()"), a
> new ns_capable_noaudit() helper is available. Let's use it!
>
> As a result, the signature of ptrace_has_cap() is restored to its original
> one.
>
> Cc: Christian Brauner
> Cc: Eric Paris
> Cc: Jann Horn
> Cc: Kees Cook
> Cc: Oleg Nesterov
> Cc: Serge E. Hallyn
> Cc: Tyler Hicks
> Cc: sta...@vger.kernel.org
> Fixes: 6b3ad6649a4c ("ptrace: reintroduce usage of subjective credentials in
> ptrace_has_cap()")
> Fixes: 69f594a38967 ("ptrace: do not audit capability check when outputing
> /proc/pid/stat")
> Signed-off-by: Mickaël Salaün
Yeah... I guess this makes sense. (We'd have to undo or change it if
we ever end up needing to use a different set of credentials, e.g.
from ->f_cred, but I guess that's really something we should avoid
anyway.)
Reviewed-by: Jann Horn
with one nit:
[...]
> /* Returns 0 on success, -errno on denial. */
> static int __ptrace_may_access(struct task_struct *task, unsigned int mode)
> {
> - const struct cred *cred = current_cred(), *tcred;
> + const struct cred *const cred = current_cred(), *tcred;
This is an unrelated change, and almost no kernel code marks local
pointer variables as "const". I would drop this change from the patch.
> struct mm_struct *mm;
> kuid_t caller_uid;
> kgid_t caller_gid;
[PATCH v1 1/2] ptrace: Set PF_SUPERPRIV when checking capability
From: Mickaël Salaün
Commit 69f594a38967 ("ptrace: do not audit capability check when outputing
/proc/pid/stat") replaced the use of ns_capable() with
has_ns_capability{,_noaudit}() which doesn't set PF_SUPERPRIV.
Commit 6b3ad6649a4c ("ptrace: reintroduce usage of subjective credentials in
ptrace_has_cap()") replaced has_ns_capability{,_noaudit}() with
security_capable(), which doesn't set PF_SUPERPRIV neither.
Since commit 98f368e9e263 ("kernel: Add noaudit variant of ns_capable()"), a
new ns_capable_noaudit() helper is available. Let's use it!
As a result, the signature of ptrace_has_cap() is restored to its original one.
Cc: Christian Brauner
Cc: Eric Paris
Cc: Jann Horn
Cc: Kees Cook
Cc: Oleg Nesterov
Cc: Serge E. Hallyn
Cc: Tyler Hicks
Cc: sta...@vger.kernel.org
Fixes: 6b3ad6649a4c ("ptrace: reintroduce usage of subjective credentials in
ptrace_has_cap()")
Fixes: 69f594a38967 ("ptrace: do not audit capability check when outputing
/proc/pid/stat")
Signed-off-by: Mickaël Salaün
---
kernel/ptrace.c | 18 ++
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 43d6179508d6..aa3c2fd6e41b 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -264,23 +264,17 @@ static int ptrace_check_attach(struct task_struct *child,
bool ignore_state)
return ret;
}
-static bool ptrace_has_cap(const struct cred *cred, struct user_namespace *ns,
- unsigned int mode)
+static bool ptrace_has_cap(struct user_namespace *ns, unsigned int mode)
{
- int ret;
-
if (mode & PTRACE_MODE_NOAUDIT)
- ret = security_capable(cred, ns, CAP_SYS_PTRACE,
CAP_OPT_NOAUDIT);
- else
- ret = security_capable(cred, ns, CAP_SYS_PTRACE, CAP_OPT_NONE);
-
- return ret == 0;
+ return ns_capable_noaudit(ns, CAP_SYS_PTRACE);
+ return ns_capable(ns, CAP_SYS_PTRACE);
}
/* Returns 0 on success, -errno on denial. */
static int __ptrace_may_access(struct task_struct *task, unsigned int mode)
{
- const struct cred *cred = current_cred(), *tcred;
+ const struct cred *const cred = current_cred(), *tcred;
struct mm_struct *mm;
kuid_t caller_uid;
kgid_t caller_gid;
@@ -326,7 +320,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(cred, tcred->user_ns, mode))
+ if (ptrace_has_cap(tcred->user_ns, mode))
goto ok;
rcu_read_unlock();
return -EPERM;
@@ -345,7 +339,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(cred, mm->user_ns, mode)))
+!ptrace_has_cap(mm->user_ns, mode)))
return -EPERM;
return security_ptrace_access_check(task, mode);
--
2.28.0
