From: Mickaël Salaün <[email protected]>
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 <[email protected]>
Cc: Eric Paris <[email protected]>
Cc: Jann Horn <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: Serge E. Hallyn <[email protected]>
Cc: Tyler Hicks <[email protected]>
Cc: [email protected]
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 <[email protected]>
---
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
On Fri, Oct 30, 2020 at 1:39 PM Mickaël Salaün <[email protected]> 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 <[email protected]>
> Cc: Eric Paris <[email protected]>
> Cc: Jann Horn <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: Oleg Nesterov <[email protected]>
> Cc: Serge E. Hallyn <[email protected]>
> Cc: Tyler Hicks <[email protected]>
> Cc: [email protected]
> 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 <[email protected]>
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 <[email protected]>
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;
On 30/10/2020 16:47, Jann Horn wrote:
> On Fri, Oct 30, 2020 at 1:39 PM Mickaël Salaün <[email protected]> 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 <[email protected]>
>> Cc: Eric Paris <[email protected]>
>> Cc: Jann Horn <[email protected]>
>> Cc: Kees Cook <[email protected]>
>> Cc: Oleg Nesterov <[email protected]>
>> Cc: Serge E. Hallyn <[email protected]>
>> Cc: Tyler Hicks <[email protected]>
>> Cc: [email protected]
>> 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 <[email protected]>
>
> 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 <[email protected]>
>
> 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;
On Fri, Oct 30, 2020 at 5:06 PM Mickaël Salaün <[email protected]> wrote:
> On 30/10/2020 16:47, Jann Horn wrote:
> > On Fri, Oct 30, 2020 at 1:39 PM Mickaël Salaün <[email protected]> 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 <[email protected]>
> >> Cc: Eric Paris <[email protected]>
> >> Cc: Jann Horn <[email protected]>
> >> Cc: Kees Cook <[email protected]>
> >> Cc: Oleg Nesterov <[email protected]>
> >> Cc: Serge E. Hallyn <[email protected]>
> >> Cc: Tyler Hicks <[email protected]>
> >> Cc: [email protected]
> >> 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 <[email protected]>
> >
> > 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 <[email protected]>
> >
> > 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.