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

[Janis Danisevskis]

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.

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