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/CAG48ez1FQVkt78129WozBwFbVhAPyAr9oJAHFHAbbNxEBr...
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
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);
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;
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;
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.
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 */
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
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/CAG48ez1FQVkt78129WozBwFbVhAPyAr9oJAHFHAbbNxEBr...
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
linux-stable-mirror@lists.linaro.org
-
Jann Horn -
Kees Cook -
Mickaël Salaün