From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934485AbcECSQt (ORCPT ); Tue, 3 May 2016 14:16:49 -0400 Received: from mail-wm0-f47.google.com ([74.125.82.47]:36771 "EHLO mail-wm0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932543AbcECSQr (ORCPT ); Tue, 3 May 2016 14:16:47 -0400 Subject: Re: [PATCH] procfs: fixes pthread cross-thread naming if !PR_DUMPABLE To: Kees Cook References: <1461691640-35817-1-git-send-email-jdanis@google.com> <5728DF05.70906@google.com> Cc: LKML , Andrew Morton , Al Viro , Cyrill Gorcunov , Alexey Dobriyan , Colin Ian King , David Rientjes , Minfei Huang , John Stultz , Calvin Owens , Jann Horn From: Janis Danisevskis Message-ID: <5728EB0B.7070509@google.com> Date: Tue, 3 May 2016 19:16:43 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 03/05/16 18:42, Kees Cook wrote: > On Tue, May 3, 2016 at 10:25 AM, Janis Danisevskis wrote: >> >> >> On 26/04/16 21:14, Kees Cook wrote: >>> >>> On Tue, Apr 26, 2016 at 10:20 AM, Janis Danisevskis >>> 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//task//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 >>> >>> >>> 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//task//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//task//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 >>>> >>> >>> >>> >> > > >