Re: [PATCH] procfs: fixes pthread cross-thread naming if !PR_DUMPABLE

[Kees Cook]

On Tue, May 3, 2016 at 11:16 AM, Janis Danisevskis <jdanis@xxxxxxxxxx> wrote:
>
>
> On 03/05/16 18:42, Kees Cook wrote:
>>
>> On Tue, May 3, 2016 at 10:25 AM, Janis Danisevskis <jdanis@xxxxxxxxxx>
>> wrote:
>>>
>>>
>>>
>>> On 26/04/16 21:14, Kees Cook wrote:
>>>>
>>>>
>>>> On Tue, Apr 26, 2016 at 10:20 AM, Janis Danisevskis <jdanis@xxxxxxxxxx>
>>>> 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@xxxxxxxxxx>
>>>>
>>>>
>>>>
>>>> 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@xxxxxxxxxxxx>

-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