Received: by 2002:a05:6902:102b:0:0:0:0 with SMTP id x11csp1799312ybt; Thu, 2 Jul 2020 14:17:23 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxHyTz2nGB08cNG2nONF1q86l+rLIN4lp8rdMMEcn6fvpN2IjfJJjJqKPFyIcWQB7AbyRwV X-Received: by 2002:a17:906:1455:: with SMTP id q21mr12778897ejc.139.1593724643235; Thu, 02 Jul 2020 14:17:23 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1593724643; cv=none; d=google.com; s=arc-20160816; b=dpoCRDyOPTySW7WldfDixPATkbhXhVSEpFTk4AY+JFF11W6o20y/zqnuu/Ud0BELBG AiALeqWYEZt/Ti8urHykS7TIFN0x4ZsL35rm763LTFJX/L+QLLLi47bW1hznlGLIzEtq J17K2K5Aizr5YBkrr93lbDqZA3UYCSdxCTyJoxTg19mYQEwaqvCXbIGycSqL6vcbmqDa vrtagigli6NDQAE9WdlYWI4nnyVIACeU1wbYyZg9wCnYfiZR12gfOWvgFvpNSALiQq5p uT2x9kasNj7ppqFS4HHPJYATimpVf/51kkqGNoz2x0jHV1NwxCkyuG1uLCRRfpCE3HSX 1uww== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date; bh=oWdjzfK3NE7SJ0gM3QxdrLN735NwDHYPSOOg8N/ZxTU=; b=ST1Gyh2+5xHi5dNvaatgv0VIvQCs4BYZvY0rZ5BFF968qVQDVL59/S2u58OXb138Q0 fFTHmBSk0YrBlOotjI8aEJLwPkOnMVtWF1tQ47KAlkicyix54gYg7PTFCT9wYBiS+J0D olj45MDifMg3+wwmQjBgPGwzoWRsEP74R5SZgZC4/+wuAG9wG6FrCXfkHYcTiyOpjnaK L7j8ej5Lu2qBNDgZz8lWoEW+oU8hUNwYT+JuSvcu+8x9JIWXTw8AHe6SzNWrQ7X884pt QRtKqTiqDvxKeLUmf13a1Doi3LFrB1IsLLF+dC5M5OT8eh0a3kpV8t5M20abElwK7wM3 OXpw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id h32si6913896edd.601.2020.07.02.14.17.00; Thu, 02 Jul 2020 14:17:23 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726117AbgGBVQw (ORCPT + 99 others); Thu, 2 Jul 2020 17:16:52 -0400 Received: from mail.hallyn.com ([178.63.66.53]:58434 "EHLO mail.hallyn.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726015AbgGBVQv (ORCPT ); Thu, 2 Jul 2020 17:16:51 -0400 Received: by mail.hallyn.com (Postfix, from userid 1001) id 40BA3C77; Thu, 2 Jul 2020 16:16:47 -0500 (CDT) Date: Thu, 2 Jul 2020 16:16:47 -0500 From: "Serge E. Hallyn" To: Adrian Reber , Paul Moore Cc: Christian Brauner , Eric Biederman , Pavel Emelyanov , Oleg Nesterov , Dmitry Safonov <0x7f454c46@gmail.com>, Andrei Vagin , Nicolas Viennot , =?utf-8?B?TWljaGHFgiBDxYJhcGnFhHNraQ==?= , Kamil Yurtsever , Dirk Petersen , Christine Flood , Casey Schaufler , Mike Rapoport , Radostin Stoyanov , Cyrill Gorcunov , Serge Hallyn , Stephen Smalley , Sargun Dhillon , Arnd Bergmann , linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org, selinux@vger.kernel.org, Eric Paris , Jann Horn , linux-fsdevel@vger.kernel.org Subject: Re: [PATCH v4 3/3] prctl: Allow ptrace capable processes to change /proc/self/exe Message-ID: <20200702211647.GB3283@mail.hallyn.com> References: <20200701064906.323185-1-areber@redhat.com> <20200701064906.323185-4-areber@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200701064906.323185-4-areber@redhat.com> User-Agent: Mutt/1.9.4 (2018-02-28) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Jul 01, 2020 at 08:49:06AM +0200, Adrian Reber wrote: > From: Nicolas Viennot > > Previously, the current process could only change the /proc/self/exe > link with local CAP_SYS_ADMIN. > This commit relaxes this restriction by permitting such change with > CAP_CHECKPOINT_RESTORE, and the ability to use ptrace. > > With access to ptrace facilities, a process can do the following: fork a > child, execve() the target executable, and have the child use ptrace() > to replace the memory content of the current process. This technique > makes it possible to masquerade an arbitrary program as any executable, > even setuid ones. > > Signed-off-by: Nicolas Viennot > Signed-off-by: Adrian Reber This is scary. But I believe it is safe. Reviewed-by: Serge Hallyn I am a bit curious about the implications of the selinux patch. IIUC you are using the permission of the tracing process to execute the file without transition, so this is a way to work around the policy which might prevent the tracee from doing so. Given that SELinux wants to be MAC, I'm not *quite* sure that's considered kosher. You also are skipping the PROCESS__PTRACE to SECCLASS_PROCESS check which selinux_bprm_set_creds does later on. Again I'm just not quite sure what's considered normal there these days. Paul, do you have input there? > --- > include/linux/lsm_hook_defs.h | 1 + > include/linux/security.h | 6 ++++++ > kernel/sys.c | 12 ++++-------- > security/commoncap.c | 26 ++++++++++++++++++++++++++ > security/security.c | 5 +++++ > security/selinux/hooks.c | 14 ++++++++++++++ > 6 files changed, 56 insertions(+), 8 deletions(-) > > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h > index 0098852bb56a..90e51d5e093b 100644 > --- a/include/linux/lsm_hook_defs.h > +++ b/include/linux/lsm_hook_defs.h > @@ -211,6 +211,7 @@ LSM_HOOK(int, 0, task_kill, struct task_struct *p, struct kernel_siginfo *info, > int sig, const struct cred *cred) > LSM_HOOK(int, -ENOSYS, task_prctl, int option, unsigned long arg2, > unsigned long arg3, unsigned long arg4, unsigned long arg5) > +LSM_HOOK(int, 0, prctl_set_mm_exe_file, struct file *exe_file) > LSM_HOOK(void, LSM_RET_VOID, task_to_inode, struct task_struct *p, > struct inode *inode) > LSM_HOOK(int, 0, ipc_permission, struct kern_ipc_perm *ipcp, short flag) > diff --git a/include/linux/security.h b/include/linux/security.h > index 2797e7f6418e..0f594eb7e766 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -412,6 +412,7 @@ int security_task_kill(struct task_struct *p, struct kernel_siginfo *info, > int sig, const struct cred *cred); > int security_task_prctl(int option, unsigned long arg2, unsigned long arg3, > unsigned long arg4, unsigned long arg5); > +int security_prctl_set_mm_exe_file(struct file *exe_file); > void security_task_to_inode(struct task_struct *p, struct inode *inode); > int security_ipc_permission(struct kern_ipc_perm *ipcp, short flag); > void security_ipc_getsecid(struct kern_ipc_perm *ipcp, u32 *secid); > @@ -1124,6 +1125,11 @@ static inline int security_task_prctl(int option, unsigned long arg2, > return cap_task_prctl(option, arg2, arg3, arg4, arg5); > } > > +static inline int security_prctl_set_mm_exe_file(struct file *exe_file) > +{ > + return cap_prctl_set_mm_exe_file(exe_file); > +} > + > static inline void security_task_to_inode(struct task_struct *p, struct inode *inode) > { } > > diff --git a/kernel/sys.c b/kernel/sys.c > index 00a96746e28a..bb53e8408c63 100644 > --- a/kernel/sys.c > +++ b/kernel/sys.c > @@ -1851,6 +1851,10 @@ static int prctl_set_mm_exe_file(struct mm_struct *mm, unsigned int fd) > if (err) > goto exit; > > + err = security_prctl_set_mm_exe_file(exe.file); > + if (err) > + goto exit; > + > /* > * Forbid mm->exe_file change if old file still mapped. > */ > @@ -2006,14 +2010,6 @@ static int prctl_set_mm_map(int opt, const void __user *addr, unsigned long data > } > > if (prctl_map.exe_fd != (u32)-1) { > - /* > - * Make sure the caller has the rights to > - * change /proc/pid/exe link: only local sys admin should > - * be allowed to. > - */ > - if (!ns_capable(current_user_ns(), CAP_SYS_ADMIN)) > - return -EINVAL; > - > error = prctl_set_mm_exe_file(mm, prctl_map.exe_fd); > if (error) > return error; > diff --git a/security/commoncap.c b/security/commoncap.c > index 59bf3c1674c8..663d00fe2ecc 100644 > --- a/security/commoncap.c > +++ b/security/commoncap.c > @@ -1291,6 +1291,31 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3, > } > } > > +/** > + * cap_prctl_set_mm_exe_file - Determine whether /proc/self/exe can be changed > + * by the current process. > + * @exe_file: The new exe file > + * Returns 0 if permission is granted, -ve if denied. > + * > + * The current process is permitted to change its /proc/self/exe link via two policies: > + * 1) The current user can do checkpoint/restore. At the time of this writing, > + * this means CAP_SYS_ADMIN or CAP_CHECKPOINT_RESTORE capable. > + * 2) The current user can use ptrace. > + * > + * With access to ptrace facilities, a process can do the following: > + * fork a child, execve() the target executable, and have the child use > + * ptrace() to replace the memory content of the current process. > + * This technique makes it possible to masquerade an arbitrary program as the > + * target executable, even if it is setuid. > + */ > +int cap_prctl_set_mm_exe_file(struct file *exe_file) > +{ > + if (checkpoint_restore_ns_capable(current_user_ns())) > + return 0; > + > + return security_ptrace_access_check(current, PTRACE_MODE_ATTACH_REALCREDS); > +} > + > /** > * cap_vm_enough_memory - Determine whether a new virtual mapping is permitted > * @mm: The VM space in which the new mapping is to be made > @@ -1356,6 +1381,7 @@ static struct security_hook_list capability_hooks[] __lsm_ro_after_init = { > LSM_HOOK_INIT(mmap_file, cap_mmap_file), > LSM_HOOK_INIT(task_fix_setuid, cap_task_fix_setuid), > LSM_HOOK_INIT(task_prctl, cap_task_prctl), > + LSM_HOOK_INIT(prctl_set_mm_exe_file, cap_prctl_set_mm_exe_file), > LSM_HOOK_INIT(task_setscheduler, cap_task_setscheduler), > LSM_HOOK_INIT(task_setioprio, cap_task_setioprio), > LSM_HOOK_INIT(task_setnice, cap_task_setnice), > diff --git a/security/security.c b/security/security.c > index 2bb912496232..13a1ed32f9e3 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -1790,6 +1790,11 @@ int security_task_prctl(int option, unsigned long arg2, unsigned long arg3, > return rc; > } > > +int security_prctl_set_mm_exe_file(struct file *exe_file) > +{ > + return call_int_hook(prctl_set_mm_exe_file, 0, exe_file); > +} > + > void security_task_to_inode(struct task_struct *p, struct inode *inode) > { > call_void_hook(task_to_inode, p, inode); > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index ca901025802a..fca5581392b8 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -4156,6 +4156,19 @@ static int selinux_task_kill(struct task_struct *p, struct kernel_siginfo *info, > secid, task_sid(p), SECCLASS_PROCESS, perm, NULL); > } > > +static int selinux_prctl_set_mm_exe_file(struct file *exe_file) > +{ > + u32 sid = current_sid(); > + > + struct common_audit_data ad = { > + .type = LSM_AUDIT_DATA_FILE, > + .u.file = exe_file, > + }; > + > + return avc_has_perm(&selinux_state, sid, sid, > + SECCLASS_FILE, FILE__EXECUTE_NO_TRANS, &ad); > +} > + > static void selinux_task_to_inode(struct task_struct *p, > struct inode *inode) > { > @@ -7057,6 +7070,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = { > LSM_HOOK_INIT(task_getscheduler, selinux_task_getscheduler), > LSM_HOOK_INIT(task_movememory, selinux_task_movememory), > LSM_HOOK_INIT(task_kill, selinux_task_kill), > + LSM_HOOK_INIT(prctl_set_mm_exe_file, selinux_prctl_set_mm_exe_file), > LSM_HOOK_INIT(task_to_inode, selinux_task_to_inode), > > LSM_HOOK_INIT(ipc_permission, selinux_ipc_permission), > -- > 2.26.2