[v1,1/2] ptrace: Set PF_SUPERPRIV when checking capability
diff mbox series

Message ID 20201030123849.770769-2-mic@digikod.net
State New, archived
Headers show
Series
  • Fix misuse of security_capable()
Related show

Commit Message

Mickaël Salaün Oct. 30, 2020, 12:38 p.m. UTC
From: Mickaël Salaün <mic@linux.microsoft.com>

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 <christian.brauner@ubuntu.com>
Cc: Eric Paris <eparis@redhat.com>
Cc: Jann Horn <jannh@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Serge E. Hallyn <serge@hallyn.com>
Cc: Tyler Hicks <tyhicks@linux.microsoft.com>
Cc: stable@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 <mic@linux.microsoft.com>
---
 kernel/ptrace.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

Comments

Jann Horn Oct. 30, 2020, 3:47 p.m. UTC | #1
On Fri, Oct 30, 2020 at 1:39 PM Mickaël Salaün <mic@digikod.net> 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 <christian.brauner@ubuntu.com>
> Cc: Eric Paris <eparis@redhat.com>
> Cc: Jann Horn <jannh@google.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Serge E. Hallyn <serge@hallyn.com>
> Cc: Tyler Hicks <tyhicks@linux.microsoft.com>
> Cc: stable@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 <mic@linux.microsoft.com>

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 <jannh@google.com>

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;
Mickaël Salaün Oct. 30, 2020, 4:06 p.m. UTC | #2
On 30/10/2020 16:47, Jann Horn wrote:
> On Fri, Oct 30, 2020 at 1:39 PM Mickaël Salaün <mic@digikod.net> 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 <christian.brauner@ubuntu.com>
>> Cc: Eric Paris <eparis@redhat.com>
>> Cc: Jann Horn <jannh@google.com>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Oleg Nesterov <oleg@redhat.com>
>> Cc: Serge E. Hallyn <serge@hallyn.com>
>> Cc: Tyler Hicks <tyhicks@linux.microsoft.com>
>> Cc: stable@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 <mic@linux.microsoft.com>
> 
> 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 <jannh@google.com>
> 
> 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;
Jann Horn Oct. 30, 2020, 6 p.m. UTC | #3
On Fri, Oct 30, 2020 at 5:06 PM Mickaël Salaün <mic@digikod.net> wrote:
> On 30/10/2020 16:47, Jann Horn wrote:
> > On Fri, Oct 30, 2020 at 1:39 PM Mickaël Salaün <mic@digikod.net> 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 <christian.brauner@ubuntu.com>
> >> Cc: Eric Paris <eparis@redhat.com>
> >> Cc: Jann Horn <jannh@google.com>
> >> Cc: Kees Cook <keescook@chromium.org>
> >> Cc: Oleg Nesterov <oleg@redhat.com>
> >> Cc: Serge E. Hallyn <serge@hallyn.com>
> >> Cc: Tyler Hicks <tyhicks@linux.microsoft.com>
> >> Cc: stable@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 <mic@linux.microsoft.com>
> >
> > 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 <jannh@google.com>
> >
> > 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.

Patch
diff mbox series

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);