linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] proc: restrict kernel stack dumps to root
@ 2018-09-11 18:39 Jann Horn
  2018-09-12 15:29 ` Jann Horn
  0 siblings, 1 reply; 8+ messages in thread
From: Jann Horn @ 2018-09-11 18:39 UTC (permalink / raw)
  To: Alexey Dobriyan, jannh
  Cc: Ken Chen, linux-kernel, linux-fsdevel, Will Deacon, Laura Abbott,
	Andy Lutomirski, security, Catalin Marinas, Josh Poimboeuf,
	Thomas Gleixner, Ingo Molnar, H. Peter Anvin

Restrict the ability to inspect kernel stacks of arbitrary tasks to root
in order to prevent a local attacker from exploiting racy stack unwinding
to leak kernel task stack contents.
See the added comment for a longer rationale.

There don't seem to be any users of this userspace API that can't
gracefully bail out if reading from the file fails. Therefore, I believe
that this change is unlikely to break things.
In the case that this patch does end up needing a revert, the next-best
solution might be to fake a single-entry stack based on wchan.

Fixes: 2ec220e27f50 ("proc: add /proc/*/stack")
Cc: stable@vger.kernel.org
Signed-off-by: Jann Horn <jannh@google.com>
---
 fs/proc/base.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index ccf86f16d9f0..7e9f07bf260d 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -407,6 +407,20 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
 	unsigned long *entries;
 	int err;
 
+	/*
+	 * The ability to racily run the kernel stack unwinder on a running task
+	 * and then observe the unwinder output is scary; while it is useful for
+	 * debugging kernel issues, it can also allow an attacker to leak kernel
+	 * stack contents.
+	 * Doing this in a manner that is at least safe from races would require
+	 * some work to ensure that the remote task can not be scheduled; and
+	 * even then, this would still expose the unwinder as local attack
+	 * surface.
+	 * Therefore, this interface is restricted to root.
+	 */
+	if (!file_ns_capable(m->file, &init_user_ns, CAP_SYS_ADMIN))
+		return -EACCES;
+
 	entries = kmalloc_array(MAX_STACK_TRACE_DEPTH, sizeof(*entries),
 				GFP_KERNEL);
 	if (!entries)
-- 
2.19.0.rc2.392.g5ba43deb5a-goog

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] proc: restrict kernel stack dumps to root
  2018-09-11 18:39 [PATCH] proc: restrict kernel stack dumps to root Jann Horn
@ 2018-09-12 15:29 ` Jann Horn
  2018-09-12 22:27   ` Kees Cook
  0 siblings, 1 reply; 8+ messages in thread
From: Jann Horn @ 2018-09-12 15:29 UTC (permalink / raw)
  To: Alexey Dobriyan
  Cc: Ken Chen, kernel list, linux-fsdevel, Will Deacon, Laura Abbott,
	Andy Lutomirski, security, Catalin Marinas, Josh Poimboeuf,
	Thomas Gleixner, Ingo Molnar, H . Peter Anvin, Linux API

+linux-api, I guess

On Tue, Sep 11, 2018 at 8:39 PM Jann Horn <jannh@google.com> wrote:
>
> Restrict the ability to inspect kernel stacks of arbitrary tasks to root
> in order to prevent a local attacker from exploiting racy stack unwinding
> to leak kernel task stack contents.
> See the added comment for a longer rationale.
>
> There don't seem to be any users of this userspace API that can't
> gracefully bail out if reading from the file fails. Therefore, I believe
> that this change is unlikely to break things.
> In the case that this patch does end up needing a revert, the next-best
> solution might be to fake a single-entry stack based on wchan.
>
> Fixes: 2ec220e27f50 ("proc: add /proc/*/stack")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jann Horn <jannh@google.com>
> ---
>  fs/proc/base.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index ccf86f16d9f0..7e9f07bf260d 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -407,6 +407,20 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
>         unsigned long *entries;
>         int err;
>
> +       /*
> +        * The ability to racily run the kernel stack unwinder on a running task
> +        * and then observe the unwinder output is scary; while it is useful for
> +        * debugging kernel issues, it can also allow an attacker to leak kernel
> +        * stack contents.
> +        * Doing this in a manner that is at least safe from races would require
> +        * some work to ensure that the remote task can not be scheduled; and
> +        * even then, this would still expose the unwinder as local attack
> +        * surface.
> +        * Therefore, this interface is restricted to root.
> +        */
> +       if (!file_ns_capable(m->file, &init_user_ns, CAP_SYS_ADMIN))
> +               return -EACCES;
> +
>         entries = kmalloc_array(MAX_STACK_TRACE_DEPTH, sizeof(*entries),
>                                 GFP_KERNEL);
>         if (!entries)
> --
> 2.19.0.rc2.392.g5ba43deb5a-goog
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] proc: restrict kernel stack dumps to root
  2018-09-12 15:29 ` Jann Horn
@ 2018-09-12 22:27   ` Kees Cook
  2018-09-12 22:47     ` Laura Abbott
  2018-09-13 11:55     ` Jann Horn
  0 siblings, 2 replies; 8+ messages in thread
From: Kees Cook @ 2018-09-12 22:27 UTC (permalink / raw)
  To: Jann Horn
  Cc: Alexey Dobriyan, Ken Chen, kernel list, linux-fsdevel,
	Will Deacon, Laura Abbott, Andy Lutomirski, Security Officers,
	Catalin Marinas, Josh Poimboeuf, Thomas Gleixner, Ingo Molnar,
	H . Peter Anvin, Linux API

On Wed, Sep 12, 2018 at 8:29 AM, Jann Horn <jannh@google.com> wrote:
> +linux-api, I guess
>
> On Tue, Sep 11, 2018 at 8:39 PM Jann Horn <jannh@google.com> wrote:
>>
>> Restrict the ability to inspect kernel stacks of arbitrary tasks to root
>> in order to prevent a local attacker from exploiting racy stack unwinding
>> to leak kernel task stack contents.
>> See the added comment for a longer rationale.
>>
>> There don't seem to be any users of this userspace API that can't
>> gracefully bail out if reading from the file fails. Therefore, I believe
>> that this change is unlikely to break things.
>> In the case that this patch does end up needing a revert, the next-best
>> solution might be to fake a single-entry stack based on wchan.
>>
>> Fixes: 2ec220e27f50 ("proc: add /proc/*/stack")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Jann Horn <jannh@google.com>
>> ---
>>  fs/proc/base.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>> index ccf86f16d9f0..7e9f07bf260d 100644
>> --- a/fs/proc/base.c
>> +++ b/fs/proc/base.c
>> @@ -407,6 +407,20 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
>>         unsigned long *entries;
>>         int err;
>>
>> +       /*
>> +        * The ability to racily run the kernel stack unwinder on a running task
>> +        * and then observe the unwinder output is scary; while it is useful for
>> +        * debugging kernel issues, it can also allow an attacker to leak kernel
>> +        * stack contents.
>> +        * Doing this in a manner that is at least safe from races would require
>> +        * some work to ensure that the remote task can not be scheduled; and
>> +        * even then, this would still expose the unwinder as local attack
>> +        * surface.
>> +        * Therefore, this interface is restricted to root.
>> +        */
>> +       if (!file_ns_capable(m->file, &init_user_ns, CAP_SYS_ADMIN))
>> +               return -EACCES;

In the past, we've avoided hard errors like this in favor of just
censoring the output. Do we want to be more cautious here? (i.e.
return 0 or a fuller seq_printf(m, "[<0>] privileged\n"); return 0;)

>> +
>>         entries = kmalloc_array(MAX_STACK_TRACE_DEPTH, sizeof(*entries),
>>                                 GFP_KERNEL);
>>         if (!entries)
>> --
>> 2.19.0.rc2.392.g5ba43deb5a-goog
>>

-Kees

-- 
Kees Cook
Pixel Security

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] proc: restrict kernel stack dumps to root
  2018-09-12 22:27   ` Kees Cook
@ 2018-09-12 22:47     ` Laura Abbott
  2018-09-13 11:55     ` Jann Horn
  1 sibling, 0 replies; 8+ messages in thread
From: Laura Abbott @ 2018-09-12 22:47 UTC (permalink / raw)
  To: Kees Cook, Jann Horn
  Cc: Alexey Dobriyan, Ken Chen, kernel list, linux-fsdevel,
	Will Deacon, Andy Lutomirski, Security Officers, Catalin Marinas,
	Josh Poimboeuf, Thomas Gleixner, Ingo Molnar, H . Peter Anvin,
	Linux API

On 09/12/2018 03:27 PM, Kees Cook wrote:
> On Wed, Sep 12, 2018 at 8:29 AM, Jann Horn <jannh@google.com> wrote:
>> +linux-api, I guess
>>
>> On Tue, Sep 11, 2018 at 8:39 PM Jann Horn <jannh@google.com> wrote:
>>>
>>> Restrict the ability to inspect kernel stacks of arbitrary tasks to root
>>> in order to prevent a local attacker from exploiting racy stack unwinding
>>> to leak kernel task stack contents.
>>> See the added comment for a longer rationale.
>>>
>>> There don't seem to be any users of this userspace API that can't
>>> gracefully bail out if reading from the file fails. Therefore, I believe
>>> that this change is unlikely to break things.
>>> In the case that this patch does end up needing a revert, the next-best
>>> solution might be to fake a single-entry stack based on wchan.
>>>
>>> Fixes: 2ec220e27f50 ("proc: add /proc/*/stack")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Jann Horn <jannh@google.com>
>>> ---
>>>   fs/proc/base.c | 14 ++++++++++++++
>>>   1 file changed, 14 insertions(+)
>>>
>>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>>> index ccf86f16d9f0..7e9f07bf260d 100644
>>> --- a/fs/proc/base.c
>>> +++ b/fs/proc/base.c
>>> @@ -407,6 +407,20 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
>>>          unsigned long *entries;
>>>          int err;
>>>
>>> +       /*
>>> +        * The ability to racily run the kernel stack unwinder on a running task
>>> +        * and then observe the unwinder output is scary; while it is useful for
>>> +        * debugging kernel issues, it can also allow an attacker to leak kernel
>>> +        * stack contents.
>>> +        * Doing this in a manner that is at least safe from races would require
>>> +        * some work to ensure that the remote task can not be scheduled; and
>>> +        * even then, this would still expose the unwinder as local attack
>>> +        * surface.
>>> +        * Therefore, this interface is restricted to root.
>>> +        */
>>> +       if (!file_ns_capable(m->file, &init_user_ns, CAP_SYS_ADMIN))
>>> +               return -EACCES;
> 
> In the past, we've avoided hard errors like this in favor of just
> censoring the output. Do we want to be more cautious here? (i.e.
> return 0 or a fuller seq_printf(m, "[<0>] privileged\n"); return 0;)
> 

The -EACCES is a strong hint to run with root privileges which is
nice from an end user perspective. If we don't want to return an
actual error, I think the "privileged" message would be okay.

Laura

>>> +
>>>          entries = kmalloc_array(MAX_STACK_TRACE_DEPTH, sizeof(*entries),
>>>                                  GFP_KERNEL);
>>>          if (!entries)
>>> --
>>> 2.19.0.rc2.392.g5ba43deb5a-goog
>>>
> 
> -Kees
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] proc: restrict kernel stack dumps to root
  2018-09-12 22:27   ` Kees Cook
  2018-09-12 22:47     ` Laura Abbott
@ 2018-09-13 11:55     ` Jann Horn
  2018-09-13 14:39       ` Kees Cook
  1 sibling, 1 reply; 8+ messages in thread
From: Jann Horn @ 2018-09-13 11:55 UTC (permalink / raw)
  To: Kees Cook
  Cc: Alexey Dobriyan, Ken Chen, kernel list, linux-fsdevel,
	Will Deacon, Laura Abbott, Andy Lutomirski, security,
	Catalin Marinas, Josh Poimboeuf, Thomas Gleixner, Ingo Molnar,
	H . Peter Anvin, Linux API

On Thu, Sep 13, 2018 at 12:28 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Sep 12, 2018 at 8:29 AM, Jann Horn <jannh@google.com> wrote:
> > +linux-api, I guess
> >
> > On Tue, Sep 11, 2018 at 8:39 PM Jann Horn <jannh@google.com> wrote:
> >>
> >> Restrict the ability to inspect kernel stacks of arbitrary tasks to root
> >> in order to prevent a local attacker from exploiting racy stack unwinding
> >> to leak kernel task stack contents.
> >> See the added comment for a longer rationale.
> >>
> >> There don't seem to be any users of this userspace API that can't
> >> gracefully bail out if reading from the file fails. Therefore, I believe
> >> that this change is unlikely to break things.
> >> In the case that this patch does end up needing a revert, the next-best
> >> solution might be to fake a single-entry stack based on wchan.
> >>
> >> Fixes: 2ec220e27f50 ("proc: add /proc/*/stack")
> >> Cc: stable@vger.kernel.org
> >> Signed-off-by: Jann Horn <jannh@google.com>
> >> ---
> >>  fs/proc/base.c | 14 ++++++++++++++
> >>  1 file changed, 14 insertions(+)
> >>
> >> diff --git a/fs/proc/base.c b/fs/proc/base.c
> >> index ccf86f16d9f0..7e9f07bf260d 100644
> >> --- a/fs/proc/base.c
> >> +++ b/fs/proc/base.c
> >> @@ -407,6 +407,20 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
> >>         unsigned long *entries;
> >>         int err;
> >>
> >> +       /*
> >> +        * The ability to racily run the kernel stack unwinder on a running task
> >> +        * and then observe the unwinder output is scary; while it is useful for
> >> +        * debugging kernel issues, it can also allow an attacker to leak kernel
> >> +        * stack contents.
> >> +        * Doing this in a manner that is at least safe from races would require
> >> +        * some work to ensure that the remote task can not be scheduled; and
> >> +        * even then, this would still expose the unwinder as local attack
> >> +        * surface.
> >> +        * Therefore, this interface is restricted to root.
> >> +        */
> >> +       if (!file_ns_capable(m->file, &init_user_ns, CAP_SYS_ADMIN))
> >> +               return -EACCES;
>
> In the past, we've avoided hard errors like this in favor of just
> censoring the output. Do we want to be more cautious here? (i.e.
> return 0 or a fuller seq_printf(m, "[<0>] privileged\n"); return 0;)

In my mind, this is different because it's a place where we don't have
to selectively censor output while preserving parts of it, and it's a
place where, as Laura said, it's useful to make lack of privileges
clearly visible because that informs users that they may have to retry
with more privileges.

Of course, if you have an example of software that actually breaks due
to this, I'll change it. But I looked at the three things in Debian
codesearch that seem to use it, and from what I can tell, they all
bail out cleanly when the read fails.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] proc: restrict kernel stack dumps to root
  2018-09-13 11:55     ` Jann Horn
@ 2018-09-13 14:39       ` Kees Cook
  2018-09-27  1:19         ` Jann Horn
  0 siblings, 1 reply; 8+ messages in thread
From: Kees Cook @ 2018-09-13 14:39 UTC (permalink / raw)
  To: Jann Horn
  Cc: Alexey Dobriyan, Ken Chen, kernel list, linux-fsdevel,
	Will Deacon, Laura Abbott, Andy Lutomirski, Security Officers,
	Catalin Marinas, Josh Poimboeuf, Thomas Gleixner, Ingo Molnar,
	H . Peter Anvin, Linux API

On Thu, Sep 13, 2018 at 4:55 AM, Jann Horn <jannh@google.com> wrote:
> On Thu, Sep 13, 2018 at 12:28 AM Kees Cook <keescook@chromium.org> wrote:
>>
>> On Wed, Sep 12, 2018 at 8:29 AM, Jann Horn <jannh@google.com> wrote:
>> > +linux-api, I guess
>> >
>> > On Tue, Sep 11, 2018 at 8:39 PM Jann Horn <jannh@google.com> wrote:
>> >>
>> >> Restrict the ability to inspect kernel stacks of arbitrary tasks to root
>> >> in order to prevent a local attacker from exploiting racy stack unwinding
>> >> to leak kernel task stack contents.
>> >> See the added comment for a longer rationale.
>> >>
>> >> There don't seem to be any users of this userspace API that can't
>> >> gracefully bail out if reading from the file fails. Therefore, I believe
>> >> that this change is unlikely to break things.
>> >> In the case that this patch does end up needing a revert, the next-best
>> >> solution might be to fake a single-entry stack based on wchan.
>> >>
>> >> Fixes: 2ec220e27f50 ("proc: add /proc/*/stack")
>> >> Cc: stable@vger.kernel.org
>> >> Signed-off-by: Jann Horn <jannh@google.com>
>> >> ---
>> >>  fs/proc/base.c | 14 ++++++++++++++
>> >>  1 file changed, 14 insertions(+)
>> >>
>> >> diff --git a/fs/proc/base.c b/fs/proc/base.c
>> >> index ccf86f16d9f0..7e9f07bf260d 100644
>> >> --- a/fs/proc/base.c
>> >> +++ b/fs/proc/base.c
>> >> @@ -407,6 +407,20 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
>> >>         unsigned long *entries;
>> >>         int err;
>> >>
>> >> +       /*
>> >> +        * The ability to racily run the kernel stack unwinder on a running task
>> >> +        * and then observe the unwinder output is scary; while it is useful for
>> >> +        * debugging kernel issues, it can also allow an attacker to leak kernel
>> >> +        * stack contents.
>> >> +        * Doing this in a manner that is at least safe from races would require
>> >> +        * some work to ensure that the remote task can not be scheduled; and
>> >> +        * even then, this would still expose the unwinder as local attack
>> >> +        * surface.
>> >> +        * Therefore, this interface is restricted to root.
>> >> +        */
>> >> +       if (!file_ns_capable(m->file, &init_user_ns, CAP_SYS_ADMIN))
>> >> +               return -EACCES;
>>
>> In the past, we've avoided hard errors like this in favor of just
>> censoring the output. Do we want to be more cautious here? (i.e.
>> return 0 or a fuller seq_printf(m, "[<0>] privileged\n"); return 0;)
>
> In my mind, this is different because it's a place where we don't have
> to selectively censor output while preserving parts of it, and it's a
> place where, as Laura said, it's useful to make lack of privileges
> clearly visible because that informs users that they may have to retry
> with more privileges.
>
> Of course, if you have an example of software that actually breaks due
> to this, I'll change it. But I looked at the three things in Debian
> codesearch that seem to use it, and from what I can tell, they all
> bail out cleanly when the read fails.

I prefer -EACCESS too, but I thought I'd mention the alternative. So, I guess:

Reviewed-by: Kees Cook <keescook@chromium.org>

:)

-Kees

-- 
Kees Cook
Pixel Security

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] proc: restrict kernel stack dumps to root
  2018-09-13 14:39       ` Kees Cook
@ 2018-09-27  1:19         ` Jann Horn
  2018-09-27  2:03           ` Kees Cook
  0 siblings, 1 reply; 8+ messages in thread
From: Jann Horn @ 2018-09-27  1:19 UTC (permalink / raw)
  To: Kees Cook, Andrew Morton
  Cc: Alexey Dobriyan, Ken Chen, kernel list, linux-fsdevel,
	Will Deacon, Laura Abbott, Andy Lutomirski, security,
	Catalin Marinas, Josh Poimboeuf, Thomas Gleixner, Ingo Molnar,
	H . Peter Anvin, Linux API

On Thu, Sep 13, 2018 at 4:39 PM Kees Cook <keescook@chromium.org> wrote:
> On Thu, Sep 13, 2018 at 4:55 AM, Jann Horn <jannh@google.com> wrote:
> > On Thu, Sep 13, 2018 at 12:28 AM Kees Cook <keescook@chromium.org> wrote:
> >>
> >> On Wed, Sep 12, 2018 at 8:29 AM, Jann Horn <jannh@google.com> wrote:
> >> > +linux-api, I guess
> >> >
> >> > On Tue, Sep 11, 2018 at 8:39 PM Jann Horn <jannh@google.com> wrote:
> >> >>
> >> >> Restrict the ability to inspect kernel stacks of arbitrary tasks to root
> >> >> in order to prevent a local attacker from exploiting racy stack unwinding
> >> >> to leak kernel task stack contents.
> >> >> See the added comment for a longer rationale.
> >> >>
> >> >> There don't seem to be any users of this userspace API that can't
> >> >> gracefully bail out if reading from the file fails. Therefore, I believe
> >> >> that this change is unlikely to break things.
> >> >> In the case that this patch does end up needing a revert, the next-best
> >> >> solution might be to fake a single-entry stack based on wchan.
> >> >>
> >> >> Fixes: 2ec220e27f50 ("proc: add /proc/*/stack")
> >> >> Cc: stable@vger.kernel.org
> >> >> Signed-off-by: Jann Horn <jannh@google.com>
> >> >> ---
> >> >>  fs/proc/base.c | 14 ++++++++++++++
> >> >>  1 file changed, 14 insertions(+)
> >> >>
> >> >> diff --git a/fs/proc/base.c b/fs/proc/base.c
> >> >> index ccf86f16d9f0..7e9f07bf260d 100644
> >> >> --- a/fs/proc/base.c
> >> >> +++ b/fs/proc/base.c
> >> >> @@ -407,6 +407,20 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
> >> >>         unsigned long *entries;
> >> >>         int err;
> >> >>
> >> >> +       /*
> >> >> +        * The ability to racily run the kernel stack unwinder on a running task
> >> >> +        * and then observe the unwinder output is scary; while it is useful for
> >> >> +        * debugging kernel issues, it can also allow an attacker to leak kernel
> >> >> +        * stack contents.
> >> >> +        * Doing this in a manner that is at least safe from races would require
> >> >> +        * some work to ensure that the remote task can not be scheduled; and
> >> >> +        * even then, this would still expose the unwinder as local attack
> >> >> +        * surface.
> >> >> +        * Therefore, this interface is restricted to root.
> >> >> +        */
> >> >> +       if (!file_ns_capable(m->file, &init_user_ns, CAP_SYS_ADMIN))
> >> >> +               return -EACCES;
> >>
> >> In the past, we've avoided hard errors like this in favor of just
> >> censoring the output. Do we want to be more cautious here? (i.e.
> >> return 0 or a fuller seq_printf(m, "[<0>] privileged\n"); return 0;)
> >
> > In my mind, this is different because it's a place where we don't have
> > to selectively censor output while preserving parts of it, and it's a
> > place where, as Laura said, it's useful to make lack of privileges
> > clearly visible because that informs users that they may have to retry
> > with more privileges.
> >
> > Of course, if you have an example of software that actually breaks due
> > to this, I'll change it. But I looked at the three things in Debian
> > codesearch that seem to use it, and from what I can tell, they all
> > bail out cleanly when the read fails.
>
> I prefer -EACCESS too, but I thought I'd mention the alternative. So, I guess:
>
> Reviewed-by: Kees Cook <keescook@chromium.org>

What do I need to do to get this merged? Oh, I think I misread
MAINTAINERS - Alexey Dobriyan is not a maintainer, just a reviewer. So
I guess this has to go via Andrew Morton? Should I resend the patch
with Andrew in the recipient list?

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] proc: restrict kernel stack dumps to root
  2018-09-27  1:19         ` Jann Horn
@ 2018-09-27  2:03           ` Kees Cook
  0 siblings, 0 replies; 8+ messages in thread
From: Kees Cook @ 2018-09-27  2:03 UTC (permalink / raw)
  To: Jann Horn
  Cc: Andrew Morton, Alexey Dobriyan, Ken Chen, kernel list,
	linux-fsdevel, Will Deacon, Laura Abbott, Andy Lutomirski,
	Security Officers, Catalin Marinas, Josh Poimboeuf,
	Thomas Gleixner, Ingo Molnar, H . Peter Anvin, Linux API

On Wed, Sep 26, 2018 at 6:19 PM, Jann Horn <jannh@google.com> wrote:
> On Thu, Sep 13, 2018 at 4:39 PM Kees Cook <keescook@chromium.org> wrote:
>> On Thu, Sep 13, 2018 at 4:55 AM, Jann Horn <jannh@google.com> wrote:
>> > On Thu, Sep 13, 2018 at 12:28 AM Kees Cook <keescook@chromium.org> wrote:
>> >>
>> >> On Wed, Sep 12, 2018 at 8:29 AM, Jann Horn <jannh@google.com> wrote:
>> >> > +linux-api, I guess
>> >> >
>> >> > On Tue, Sep 11, 2018 at 8:39 PM Jann Horn <jannh@google.com> wrote:
>> >> >>
>> >> >> Restrict the ability to inspect kernel stacks of arbitrary tasks to root
>> >> >> in order to prevent a local attacker from exploiting racy stack unwinding
>> >> >> to leak kernel task stack contents.
>> >> >> See the added comment for a longer rationale.
>> >> >>
>> >> >> There don't seem to be any users of this userspace API that can't
>> >> >> gracefully bail out if reading from the file fails. Therefore, I believe
>> >> >> that this change is unlikely to break things.
>> >> >> In the case that this patch does end up needing a revert, the next-best
>> >> >> solution might be to fake a single-entry stack based on wchan.
>> >> >>
>> >> >> Fixes: 2ec220e27f50 ("proc: add /proc/*/stack")
>> >> >> Cc: stable@vger.kernel.org
>> >> >> Signed-off-by: Jann Horn <jannh@google.com>
>> >> >> ---
>> >> >>  fs/proc/base.c | 14 ++++++++++++++
>> >> >>  1 file changed, 14 insertions(+)
>> >> >>
>> >> >> diff --git a/fs/proc/base.c b/fs/proc/base.c
>> >> >> index ccf86f16d9f0..7e9f07bf260d 100644
>> >> >> --- a/fs/proc/base.c
>> >> >> +++ b/fs/proc/base.c
>> >> >> @@ -407,6 +407,20 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
>> >> >>         unsigned long *entries;
>> >> >>         int err;
>> >> >>
>> >> >> +       /*
>> >> >> +        * The ability to racily run the kernel stack unwinder on a running task
>> >> >> +        * and then observe the unwinder output is scary; while it is useful for
>> >> >> +        * debugging kernel issues, it can also allow an attacker to leak kernel
>> >> >> +        * stack contents.
>> >> >> +        * Doing this in a manner that is at least safe from races would require
>> >> >> +        * some work to ensure that the remote task can not be scheduled; and
>> >> >> +        * even then, this would still expose the unwinder as local attack
>> >> >> +        * surface.
>> >> >> +        * Therefore, this interface is restricted to root.
>> >> >> +        */
>> >> >> +       if (!file_ns_capable(m->file, &init_user_ns, CAP_SYS_ADMIN))
>> >> >> +               return -EACCES;
>> >>
>> >> In the past, we've avoided hard errors like this in favor of just
>> >> censoring the output. Do we want to be more cautious here? (i.e.
>> >> return 0 or a fuller seq_printf(m, "[<0>] privileged\n"); return 0;)
>> >
>> > In my mind, this is different because it's a place where we don't have
>> > to selectively censor output while preserving parts of it, and it's a
>> > place where, as Laura said, it's useful to make lack of privileges
>> > clearly visible because that informs users that they may have to retry
>> > with more privileges.
>> >
>> > Of course, if you have an example of software that actually breaks due
>> > to this, I'll change it. But I looked at the three things in Debian
>> > codesearch that seem to use it, and from what I can tell, they all
>> > bail out cleanly when the read fails.
>>
>> I prefer -EACCESS too, but I thought I'd mention the alternative. So, I guess:
>>
>> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> What do I need to do to get this merged? Oh, I think I misread
> MAINTAINERS - Alexey Dobriyan is not a maintainer, just a reviewer. So
> I guess this has to go via Andrew Morton? Should I resend the patch
> with Andrew in the recipient list?

Yeah, traditionally Andrew has taken /proc patches.

-Kees

-- 
Kees Cook
Pixel Security

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-09-27  8:19 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-11 18:39 [PATCH] proc: restrict kernel stack dumps to root Jann Horn
2018-09-12 15:29 ` Jann Horn
2018-09-12 22:27   ` Kees Cook
2018-09-12 22:47     ` Laura Abbott
2018-09-13 11:55     ` Jann Horn
2018-09-13 14:39       ` Kees Cook
2018-09-27  1:19         ` Jann Horn
2018-09-27  2:03           ` Kees Cook

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).