[PATCH v1 0/2] Fix misuse of security_capable()

[PATCH v1 0/2] Fix misuse of security_capable()

[Mickaël Salaün]

This series replaces all the use of security_capable(current_cred(),
...) with ns_capable{,_noaudit}() which set PF_SUPERPRIV.

This initially come from a review of Landlock by Jann Horn:
https://lore.kernel.org/lkml/CAG48ez1FQVkt78129WozBwFbVhAPyAr9oJAHFHAbbNxEBr9h1g@mail.gmail.com/

Mickaël Salaün (2):
  ptrace: Set PF_SUPERPRIV when checking capability
  seccomp: Set PF_SUPERPRIV when checking capability

 kernel/ptrace.c  | 18 ++++++------------
 kernel/seccomp.c |  5 ++---
 2 files changed, 8 insertions(+), 15 deletions(-)


base-commit: 3650b228f83adda7e5ee532e2b90429c03f7b9ec
-- 
2.28.0

[PATCH v1 1/2] ptrace: Set PF_SUPERPRIV when checking capability

[Mickaël Salaün]

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

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

[PATCH v1 2/2] seccomp: Set PF_SUPERPRIV when checking capability

[Mickaël Salaün]

From: Mickaël Salaün <mic@linux.microsoft.com>

Replace the use of security_capable(current_cred(), ...) with
ns_capable_noaudit() which set PF_SUPERPRIV.

Since commit 98f368e9e263 ("kernel: Add noaudit variant of
ns_capable()"), a new ns_capable_noaudit() helper is available.  Let's
use it!

Cc: Jann Horn <jannh@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Tyler Hicks <tyhicks@linux.microsoft.com>
Cc: Will Drewry <wad@chromium.org>
Cc: stable@vger.kernel.org
Fixes: e2cfabdfd075 ("seccomp: add system call filtering using BPF")
Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
---
 kernel/seccomp.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 8ad7a293255a..53a7d1512dd7 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -38,7 +38,7 @@
 #include <linux/filter.h>
 #include <linux/pid.h>
 #include <linux/ptrace.h>
-#include <linux/security.h>
+#include <linux/capability.h>
 #include <linux/tracehook.h>
 #include <linux/uaccess.h>
 #include <linux/anon_inodes.h>
@@ -558,8 +558,7 @@ static struct seccomp_filter *seccomp_prepare_filter(struct sock_fprog *fprog)
 	 * behavior of privileged children.
 	 */
 	if (!task_no_new_privs(current) &&
-	    security_capable(current_cred(), current_user_ns(),
-				     CAP_SYS_ADMIN, CAP_OPT_NOAUDIT) != 0)
+			!ns_capable_noaudit(current_user_ns(), CAP_SYS_ADMIN))
 		return ERR_PTR(-EACCES);
 
 	/* Allocate a new seccomp_filter */
-- 
2.28.0

Re: [PATCH v1 2/2] seccomp: Set PF_SUPERPRIV when checking capability

[Jann Horn]

On Fri, Oct 30, 2020 at 1:39 PM Mickaël Salaün <mic@digikod.net> wrote:
> Replace the use of security_capable(current_cred(), ...) with
> ns_capable_noaudit() which set PF_SUPERPRIV.
>
> Since commit 98f368e9e263 ("kernel: Add noaudit variant of
> ns_capable()"), a new ns_capable_noaudit() helper is available.  Let's
> use it!
>
> Cc: Jann Horn <jannh@google.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Tyler Hicks <tyhicks@linux.microsoft.com>
> Cc: Will Drewry <wad@chromium.org>
> Cc: stable@vger.kernel.org
> Fixes: e2cfabdfd075 ("seccomp: add system call filtering using BPF")
> Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>

Reviewed-by: Jann Horn <jannh@google.com>

Re: [PATCH v1 1/2] ptrace: Set PF_SUPERPRIV when checking capability

[Jann Horn]

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;

Re: [PATCH v1 1/2] ptrace: Set PF_SUPERPRIV when checking capability

[Mickaël Salaün]

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;

Re: [PATCH v1 1/2] ptrace: Set PF_SUPERPRIV when checking capability

[Jann Horn]

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.

Re: [PATCH v1 0/2] Fix misuse of security_capable()

[Kees Cook]

On Fri, 30 Oct 2020 13:38:47 +0100, Mickaël Salaün wrote:
> This series replaces all the use of security_capable(current_cred(),
> ...) with ns_capable{,_noaudit}() which set PF_SUPERPRIV.
> 
> This initially come from a review of Landlock by Jann Horn:
> https://lore.kernel.org/lkml/CAG48ez1FQVkt78129WozBwFbVhAPyAr9oJAHFHAbbNxEBr9h1g@mail.gmail.com/
> 
> Mickaël Salaün (2):
>   ptrace: Set PF_SUPERPRIV when checking capability
>   seccomp: Set PF_SUPERPRIV when checking capability
> 
> [...]

Applied to for-linus/seccomp, thanks!

[1/2] ptrace: Set PF_SUPERPRIV when checking capability
      https://git.kernel.org/kees/c/cf23705244c9
[2/2] seccomp: Set PF_SUPERPRIV when checking capability
      https://git.kernel.org/kees/c/fb14528e4436

-- 
Kees Cook