lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 28 Nov 2022 11:49:07 -0600 From: "Eric W. Biederman" <ebiederm@...ssion.com> To: David Laight <David.Laight@...LAB.COM> Cc: 'Andy Lutomirski' <luto@...nel.org>, Jann Horn <jannh@...gle.com>, Christian Brauner <brauner@...nel.org>, Kees Cook <keescook@...omium.org>, Jorge Merlino <jorge.merlino@...onical.com>, Al Viro <viro@...iv.linux.org.uk>, Thomas Gleixner <tglx@...utronix.de>, "Sebastian Andrzej Siewior" <bigeasy@...utronix.de>, Andrew Morton <akpm@...ux-foundation.org>, "linux-mm@...ck.org" <linux-mm@...ck.org>, "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>, "John Johansen" <john.johansen@...onical.com>, Paul Moore <paul@...l-moore.com>, James Morris <jmorris@...ei.org>, "Serge E. Hallyn" <serge@...lyn.com>, Stephen Smalley <stephen.smalley.work@...il.com>, Eric Paris <eparis@...isplace.org>, Richard Haines <richard_c_haines@...nternet.com>, Casey Schaufler <casey@...aufler-ca.com>, Xin Long <lucien.xin@...il.com>, "David S. Miller" <davem@...emloft.net>, Todd Kjos <tkjos@...gle.com>, "Ondrej Mosnacek" <omosnace@...hat.com>, Prashanth Prahlad <pprahlad@...hat.com>, Micah Morton <mortonm@...omium.org>, Fenghua Yu <fenghua.yu@...el.com>, Andrei Vagin <avagin@...il.com>, Linux Kernel Mailing List <linux-kernel@...r.kernel.org>, "apparmor@...ts.ubuntu.com" <apparmor@...ts.ubuntu.com>, "linux-security-module@...r.kernel.org" <linux-security-module@...r.kernel.org>, "selinux@...r.kernel.org" <selinux@...r.kernel.org>, "linux-hardening@...r.kernel.org" <linux-hardening@...r.kernel.org> Subject: Re: [PATCH 1/2] fs/exec: Explicitly unshare fs_struct on exec David Laight <David.Laight@...LAB.COM> writes: > From: Andy Lutomirski >> Sent: 14 October 2022 04:18 > ... >> But seriously, this makes no sense at all. It should not be possible to exec a program and then, >> without ptrace, change its cwd out from under it. Do we really need to preserve this behavior? > > it maybe ok if the exec'ed program also 'bought-in' to the > fact that its cwd and open files might get changed. > But imagine someone doing it to a login shell! I am slowly catching up on my email and I saw this conversation. When I initially saw this thread I was confused and thought this might run into an issue with fs/locks.c. I was close but wrong. fs/locks.c uses current->files as a sort of process identifier and so is very sensitive to when it is unshared. Making unsharing current->files unconditionally a bug. Not relevant to this conversation. There are several clone options that were only relevant for the old LinuxThreads implementation including CLONE_FS and CLONE_SIGHAND. The LinuxThreads implementation has not been needed since the introduction of CLONE_THREAD in linux-2.6.0 in 17 Dec 2003. Almost 20 years ago. I suggest we introduce CONFIG_CLONE_FS and CONFIG_SIGHAND to allow disabling support of these clone options. No known user space will care. The are both getting in the way of kernel maintenance so there is a reason to start pushing them out. Further simply not worrying about UNSHARE_FS during exec fixes the race so it essentially a bug fix by code removal. I believe something like the patch below should get the job done. diff --git a/fs/exec.c b/fs/exec.c index a0b1f0337a62..7ff13c77ad04 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1186,7 +1186,8 @@ static int unshare_sighand(struct task_struct *me) { struct sighand_struct *oldsighand = me->sighand; - if (refcount_read(&oldsighand->count) != 1) { + if (IS_ENABLED(CONFIG_CLONE_SIGHAND) && + refcount_read(&oldsighand->count) != 1) { struct sighand_struct *newsighand; /* * This ->sighand is shared with the CLONE_SIGHAND @@ -1568,6 +1569,9 @@ static void check_unsafe_exec(struct linux_binprm *bprm) if (task_no_new_privs(current)) bprm->unsafe |= LSM_UNSAFE_NO_NEW_PRIVS; + if (!IS_ENABLED(CONFIG_CLONE_FS)) + return; + t = p; n_fs = 1; spin_lock(&p->fs->lock); diff --git a/init/Kconfig b/init/Kconfig index 94125d3b6893..8660a6bcc1cf 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -1764,6 +1764,23 @@ config KALLSYMS_BASE_RELATIVE time constants, and no relocation pass is required at runtime to fix up the entries based on the runtime load address of the kernel. +config CLONE_FS + bool + default y + help + Support CLONE_FS being passed to clone. The only known user + is the old LinuxThreads package so it should be safe to disable + this option. + +config CLONE_SIGHAND + bool + default y + help + Support CLONE_SIGHAND being passed to clone. The only known user + is the old LinuxThreads package so it should be safe to disable + this option. + + # end of the "standard kernel features (expert users)" menu # syscall, maps, verifier diff --git a/kernel/fork.c b/kernel/fork.c index 08969f5aa38d..da9017b51da4 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -2023,6 +2023,16 @@ static __latent_entropy struct task_struct *copy_process( if ((clone_flags & CLONE_SIGHAND) && !(clone_flags & CLONE_VM)) return ERR_PTR(-EINVAL); + /* Don't allow CLONE_FS if not enabled */ + if (!IS_ENABLED(CONFIG_CLONE_FS) && + ((clone_flags & (CLONE_THREAD | CLONE_FS)) == CLONE_FS)) + return ERR_PTR(-EINVAL); + + /* Don't allow CLONE_SIGHAND if not enabled */ + if (!IS_ENABLED(CONFIG_CLONE_SIGHAND) && + ((clone_flags & (CLONE_THREAD | CLONE_SIGHAND)) == CLONE_SIGHAND)) + return ERR_PTR(-EINVAL); + /* * Siblings of global init remain as zombies on exit since they are * not reaped by their parent (swapper). To solve this and to avoid @@ -3101,6 +3111,9 @@ static int unshare_fs(unsigned long unshare_flags, struct fs_struct **new_fsp) if (fs->users == 1) return 0; + if (!IS_ENABLED(CONFIG_CLONE_FS)) + return -EINVAL; + *new_fsp = copy_fs_struct(fs); if (!*new_fsp) return -ENOMEM; Eric
Powered by blists - more mailing lists