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 linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 3 May 2016 19:16:43 +0100 From: Janis Danisevskis <jdanis@...gle.com> To: Kees Cook <keescook@...omium.org> Cc: LKML <linux-kernel@...r.kernel.org>, Andrew Morton <akpm@...ux-foundation.org>, Al Viro <viro@...iv.linux.org.uk>, Cyrill Gorcunov <gorcunov@...nvz.org>, Alexey Dobriyan <adobriyan@...il.com>, Colin Ian King <colin.king@...onical.com>, David Rientjes <rientjes@...gle.com>, Minfei Huang <mnfhuang@...il.com>, John Stultz <john.stultz@...aro.org>, Calvin Owens <calvinowens@...com>, Jann Horn <jann@...jh.net> Subject: Re: [PATCH] procfs: fixes pthread cross-thread naming if !PR_DUMPABLE On 03/05/16 18:42, Kees Cook wrote: > On Tue, May 3, 2016 at 10:25 AM, Janis Danisevskis <jdanis@...gle.com> wrote: >> >> >> On 26/04/16 21:14, Kees Cook wrote: >>> >>> On Tue, Apr 26, 2016 at 10:20 AM, Janis Danisevskis <jdanis@...gle.com> >>> wrote: >>>> >>>> The PR_DUMPABLE flag causes the pid related paths of the >>>> proc file system to be owned by ROOT. The implementation >>>> of pthread_set/getname_np however needs access to >>>> /proc/<pid>/task/<tid>/comm. >>>> If PR_DUMPABLE is false this implementation is locked out. >>>> >>>> This patch installs a special permission function for >>>> the file "comm" that grants read and write access to >>>> all threads of the same group regardless of the ownership >>>> of the inode. For all other threads the function falls back >>>> to the generic inode permission check. >>>> >>>> Signed-off-by: Janis Danisevskis <jdanis@...gle.com> >>> >>> >>> Instead of a permissions function, perhaps this should be handled in >>> the open() of proc_pid_set_comm_operations (and the REG permissions >>> loosened)? I'm concerned there's a race here between the perm check >>> and the resulting open. I'd rather have the open doing the check to >>> eliminate the race. >> >> >> I kind of thought that the permission check is on the open path >> could you elaborate on the race that you are expecting? > > In looking I see now that comm_write() still retains its > same_thread_group() check, so nevermind about the race. I was thinking > it was gone, so that the pid could change between the permissions > check and the write. > >> Also, in what way would you loosen the permissions on the REG? >> If the DUMPABLE flag is cleared this node is owned by ROOT. >> So the only way to make it writable to a user process would be >> to make it world writable. This cannot be your intention. > > I meant to do all the access control in the open() routine to make the > world-writable permissions irrelevant. But, I think, your solution is > easier to read. :) > > One thing I can't find, though, is where PR_SET_DUMPABLE makes these > uid changes. I only see uid changes happening when the cred changes > (which then triggers the dumpable change). What's the process flow > that gets a thread into this state? In fs/proc/base.c look for task_dumpable. It happens in the revalidate functions and also when the nodes are first instantiated. Janis > > -Kees > >> >> Janis >> >> >>> >>> -Kees >>> >>>> --- >>>> fs/proc/base.c | 42 +++++++++++++++++++++++++++++++++++++++++- >>>> 1 file changed, 41 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/fs/proc/base.c b/fs/proc/base.c >>>> index b1755b2..c8ceb3c8 100644 >>>> --- a/fs/proc/base.c >>>> +++ b/fs/proc/base.c >>>> @@ -3157,6 +3157,44 @@ int proc_pid_readdir(struct file *file, struct >>>> dir_context *ctx) >>>> } >>>> >>>> /* >>>> + * proc_tid_comm_permission is a special permission function exclusively >>>> + * used for the node /proc/<pid>/task/<tid>/comm. >>>> + * It bypasses generic permission checks in the case where a task of the >>>> same >>>> + * task group attempts to access the node. >>>> + * The rational behind this is that glibc and bionic access this node >>>> for >>>> + * cross thread naming (pthread_set/getname_np(!self)). However, if >>>> + * PR_SET_DUMPABLE gets set to 0 this node among others becomes uid=0 >>>> gid=0, >>>> + * which locks out the cross thread naming implementation. >>>> + * This function makes sure that the node is always accessible for >>>> members of >>>> + * same thread group. >>>> + */ >>>> +static int proc_tid_comm_permission(struct inode *inode, int mask) >>>> +{ >>>> + bool is_same_tgroup; >>>> + struct task_struct *task; >>>> + >>>> + task = get_proc_task(inode); >>>> + if (!task) >>>> + return -ESRCH; >>>> + is_same_tgroup = same_thread_group(current, task); >>>> + put_task_struct(task); >>>> + >>>> + if (likely(is_same_tgroup && !(mask & MAY_EXEC))) { >>>> + /* This file (/proc/<pid>/task/<tid>/comm) can always be >>>> + * read or written by the members of the corresponding >>>> + * thread group. >>>> + */ >>>> + return 0; >>>> + } >>>> + >>>> + return generic_permission(inode, mask); >>>> +} >>>> + >>>> +static const struct inode_operations proc_tid_comm_inode_operations = { >>>> + .permission = proc_tid_comm_permission, >>>> +}; >>>> + >>>> +/* >>>> * Tasks >>>> */ >>>> static const struct pid_entry tid_base_stuff[] = { >>>> @@ -3174,7 +3212,9 @@ static const struct pid_entry tid_base_stuff[] = { >>>> #ifdef CONFIG_SCHED_DEBUG >>>> REG("sched", S_IRUGO|S_IWUSR, proc_pid_sched_operations), >>>> #endif >>>> - REG("comm", S_IRUGO|S_IWUSR, proc_pid_set_comm_operations), >>>> + NOD("comm", S_IFREG|S_IRUGO|S_IWUSR, >>>> + &proc_tid_comm_inode_operations, >>>> + &proc_pid_set_comm_operations, {}), >>>> #ifdef CONFIG_HAVE_ARCH_TRACEHOOK >>>> ONE("syscall", S_IRUSR, proc_pid_syscall), >>>> #endif >>>> -- >>>> 2.8.0.rc3.226.g39d4020 >>>> >>> >>> >>> >> > > >
Powered by blists - more mailing lists