| Message ID | 20180927153316.200286-1-jannh@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [resend] proc: restrict kernel stack dumps to root | expand |
On Thu, Sep 27, 2018 at 8:33 AM, 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> Acked-by: Kees Cook <keescook@chromium.org> -Kees > --- > Resending because I forgot to send this to akpm the first time. > > 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 >
On Thu, 27 Sep 2018 17:33:16 +0200 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> It's a bit worrisome cc'ing stable on a patch which might need a revert. > --- 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. > + */ The /proc file is 0400 so the user can only read owned-by-self stacks, yes? In what way could exposure of one's own kernel stack contents lead to plausible attacks? I guess maybe post-setuid, perhaps? I do think we're owed considerably more explanation of the present risk before considering a somewhat dangerous -stable backport, please.
On Fri, Sep 28, 2018 at 12:29 AM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Thu, 27 Sep 2018 17:33:16 +0200 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> > > It's a bit worrisome cc'ing stable on a patch which might need a revert. > > > --- 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. > > + */ > > The /proc file is 0400 so the user can only read owned-by-self stacks, > yes? In what way could exposure of one's own kernel stack contents > lead to plausible attacks? I guess maybe post-setuid, perhaps? > > I do think we're owed considerably more explanation of the present risk > before considering a somewhat dangerous -stable backport, please. I sent a bug report to security@. The short version: Currently, you can use /proc/self/task/*/stack to cause a stack walk on a task you control while it is running on another CPU. That means that the stack can change under the stack walker. The stack walker does have guards against going completely off the rails and into random kernel memory, but it can interpret random data from your kernel stack as instruction pointers and stack pointers. This can cause exposure of kernel stack contents to userspace.
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)
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> --- Resending because I forgot to send this to akpm the first time. fs/proc/base.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)