* [PATCH] procfs: fixes pthread cross-thread naming if !PR_DUMPABLE
@ 2016-04-26 17:20 Janis Danisevskis
2016-04-26 20:14 ` Kees Cook
0 siblings, 1 reply; 6+ messages in thread
From: Janis Danisevskis @ 2016-04-26 17:20 UTC (permalink / raw)
To: linux-kernel
Cc: Janis Danisevskis, Andrew Morton, Kees Cook, Al Viro,
Cyrill Gorcunov, Alexey Dobriyan, Colin Ian King, David Rientjes,
Minfei Huang, John Stultz, Calvin Owens, Jann Horn
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>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] procfs: fixes pthread cross-thread naming if !PR_DUMPABLE
2016-04-26 17:20 [PATCH] procfs: fixes pthread cross-thread naming if !PR_DUMPABLE Janis Danisevskis
@ 2016-04-26 20:14 ` Kees Cook
2016-05-03 17:25 ` Janis Danisevskis
0 siblings, 1 reply; 6+ messages in thread
From: Kees Cook @ 2016-04-26 20:14 UTC (permalink / raw)
To: Janis Danisevskis
Cc: LKML, Andrew Morton, Al Viro, Cyrill Gorcunov, Alexey Dobriyan,
Colin Ian King, David Rientjes, Minfei Huang, John Stultz,
Calvin Owens, Jann Horn
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.
-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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] procfs: fixes pthread cross-thread naming if !PR_DUMPABLE
2016-04-26 20:14 ` Kees Cook
@ 2016-05-03 17:25 ` Janis Danisevskis
2016-05-03 17:42 ` Kees Cook
0 siblings, 1 reply; 6+ messages in thread
From: Janis Danisevskis @ 2016-05-03 17:25 UTC (permalink / raw)
To: Kees Cook
Cc: LKML, Andrew Morton, Al Viro, Cyrill Gorcunov, Alexey Dobriyan,
Colin Ian King, David Rientjes, Minfei Huang, John Stultz,
Calvin Owens, Jann Horn
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?
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
>
> -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
>>
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] procfs: fixes pthread cross-thread naming if !PR_DUMPABLE
2016-05-03 17:25 ` Janis Danisevskis
@ 2016-05-03 17:42 ` Kees Cook
2016-05-03 18:16 ` Janis Danisevskis
0 siblings, 1 reply; 6+ messages in thread
From: Kees Cook @ 2016-05-03 17:42 UTC (permalink / raw)
To: Janis Danisevskis
Cc: LKML, Andrew Morton, Al Viro, Cyrill Gorcunov, Alexey Dobriyan,
Colin Ian King, David Rientjes, Minfei Huang, John Stultz,
Calvin Owens, Jann Horn
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] procfs: fixes pthread cross-thread naming if !PR_DUMPABLE
2016-05-03 17:42 ` Kees Cook
@ 2016-05-03 18:16 ` Janis Danisevskis
2016-05-03 19:02 ` Kees Cook
0 siblings, 1 reply; 6+ messages in thread
From: Janis Danisevskis @ 2016-05-03 18:16 UTC (permalink / raw)
To: Kees Cook
Cc: LKML, Andrew Morton, Al Viro, Cyrill Gorcunov, Alexey Dobriyan,
Colin Ian King, David Rientjes, Minfei Huang, John Stultz,
Calvin Owens, Jann Horn
On 03/05/16 18:42, Kees Cook wrote:
> 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?
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
>>>>
>>>
>>>
>>>
>>
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] procfs: fixes pthread cross-thread naming if !PR_DUMPABLE
2016-05-03 18:16 ` Janis Danisevskis
@ 2016-05-03 19:02 ` Kees Cook
0 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2016-05-03 19:02 UTC (permalink / raw)
To: Janis Danisevskis
Cc: LKML, Andrew Morton, Al Viro, Cyrill Gorcunov, Alexey Dobriyan,
Colin Ian King, David Rientjes, Minfei Huang, John Stultz,
Calvin Owens, Jann Horn
On Tue, May 3, 2016 at 11:16 AM, Janis Danisevskis <jdanis@google.com> wrote:
>
>
> On 03/05/16 18:42, Kees Cook wrote:
>>
>> 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?
>
>
> 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@chromium.org>
-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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-05-03 19:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-26 17:20 [PATCH] procfs: fixes pthread cross-thread naming if !PR_DUMPABLE Janis Danisevskis
2016-04-26 20:14 ` Kees Cook
2016-05-03 17:25 ` Janis Danisevskis
2016-05-03 17:42 ` Kees Cook
2016-05-03 18:16 ` Janis Danisevskis
2016-05-03 19:02 ` Kees Cook
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.