linux-kernel - Re: [PATCH] procfs: fixes pthread cross-thread naming if !PR

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 26 Apr 2016 13:14:19 -0700
From:	Kees Cook <keescook@...omium.org>
To:	Janis Danisevskis <jdanis@...gle.com>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Al Viro <viro@...iv.linux.org.uk>,
	Cyrill Gorcunov <gorcunov@...nvz.org>,
	Alexey Dobriyan <adobriyan@...il.com>,
	Colin Ian King <colin.king@...onical.com>,
	David Rientjes <rientjes@...gle.com>,
	Minfei Huang <mnfhuang@...il.com>,
	John Stultz <john.stultz@...aro.org>,
	Calvin Owens <calvinowens@...com>, Jann Horn <jann@...jh.net>
Subject: Re: [PATCH] procfs: fixes pthread cross-thread naming if !PR_DUMPABLE

On Tue, Apr 26, 2016 at 10:20 AM, Janis Danisevskis <jdanis@...gle.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@...gle.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.

-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