procfs: fixes pthread cross-thread naming if !PR_DUMPABLE

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 <[email protected]>

---
 fs/proc/base.c | 42 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

fs/proc/base.c +41 -2

index b1755b2..c8ceb3c8 100644
@@ -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

Quoted 12 lines · Janis Danisevskis

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.

Quoted 1 line

Signed-off-by Janis Danisevskis <[email protected]>

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

Quoted 67 lines

---
  fs/proc/base.c | 42 +++++++++++++++++++++++++++++++++++++++++-
  1 file changed, 41 insertions(+), 1 deletion(-)

fs/proc/base.c +41 -2

index b1755b2..c8ceb3c8 100644
@@ -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

Quoted 16 lines · Kees Cook

Quoted 13 lines · Janis Danisevskis

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.

Quoted 8 lines

Quoted 1 line

Signed-off-by Janis Danisevskis <[email protected]>

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?

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.

Janis

Quoted 78 lines

-Kees

Quoted 72 lines

---
  fs/proc/base.c | 42 +++++++++++++++++++++++++++++++++++++++++-
  1 file changed, 41 insertions(+), 1 deletion(-)

fs/proc/base.c +3 -0

index b1755b2..c8ceb3c8 100644
@@ -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.
+ */

fs/proc/base.c +30 -2

+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

Quoted 29 lines · Janis Danisevskis

Quoted 16 lines · Kees Cook

Quoted 13 lines · Janis Danisevskis

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.

Quoted 8 lines

Quoted 1 line

Signed-off-by Janis Danisevskis <[email protected]>

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.

Quoted 2 lines

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.

Quoted 4 lines

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

Quoted 83 lines

Janis

Quoted 78 lines

-Kees

Quoted 72 lines

---
   fs/proc/base.c | 42 +++++++++++++++++++++++++++++++++++++++++-
   1 file changed, 41 insertions(+), 1 deletion(-)

fs/proc/base.c +3 -0

index b1755b2..c8ceb3c8 100644
@@ -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.
+ */

fs/proc/base.c +30 -2

+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

Quoted 56 lines · Kees Cook

Quoted 34 lines · Janis Danisevskis

Quoted 22 lines · Kees Cook

Quoted 8 lines · Janis Danisevskis

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.

Quoted 7 lines

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 <[email protected]>

Quoted 5 lines

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.

Quoted 2 lines

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.

Quoted 4 lines

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. :)

Quoted 4 lines

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

Quoted 95 lines

-Kees

Quoted 89 lines

Janis

Quoted 84 lines

-Kees

Quoted 78 lines

---
   fs/proc/base.c | 42 +++++++++++++++++++++++++++++++++++++++++-
   1 file changed, 41 insertions(+), 1 deletion(-)

fs/proc/base.c +1 -0

index b1755b2..c8ceb3c8 100644
@@ -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.
+ */

fs/proc/base.c +13 -0

+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

fs/proc/base.c +10 -0

+                * 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 =

{

fs/proc/base.c +7 -1

+               .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