lkml.org 
[lkml]   [2016]   [May]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] procfs: fixes pthread cross-thread naming if !PR_DUMPABLE
From
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?

-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

\
 
 \ /
  Last update: 2016-05-03 20:01    [W:0.058 / U:0.340 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site