Messages in this thread | | | Date | Tue, 3 May 2016 12:02:16 -0700 | Subject | Re: [PATCH] procfs: fixes pthread cross-thread naming if !PR_DUMPABLE | From | Kees Cook <> |
| |
On Tue, May 3, 2016 at 11:16 AM, Janis Danisevskis <jdanis@google.com> wrote: > > > On 03/05/16 18:42, Kees Cook wrote: >> >> On Tue, May 3, 2016 at 10:25 AM, Janis Danisevskis <jdanis@google.com> >> wrote: >>> >>> >>> >>> On 26/04/16 21:14, Kees Cook wrote: >>>> >>>> >>>> On Tue, Apr 26, 2016 at 10:20 AM, Janis Danisevskis <jdanis@google.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@google.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.
Thanks! I see it now.
Acked-by: Kees Cook <keescook@chromium.org>
-Kees
> > 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 >>>>> >>>> >>>> >>>> >>> >> >> >> >
-- Kees Cook Chrome OS & Brillo Security
| |