[PATCH resend - v2 0/2] procfs: make /proc/*/{stack,syscall,pagemap} 0400

[PATCH resend - v2 0/2] procfs: make /proc/*/{stack,syscall,pagemap} 0400

[Djalal Harouni]

(Please note: this is a resend of version 2, I got two Acked-by, but no
one replied on why it should not be applied...)


The following patches make /proc/*/{stack,syscall,personality,pagemap}
0400.

These files contain sensitive information that can be used by an
unprivileged process to leak address space and bypass ASLR. This will
make the VFS able to bloc unprivileged processes from getting file
descriptors on *already* *running* processes (privileged processes).

This does not protect all the /proc and exec-suid cases. It just reduces
the scope of ASLR leaks by protecting *already running* processes. The
leak is still possible on these files *only* if an attacker opens its
/proc/*/file and can *spawn* a target setuid process, then read from it.

So, only already running processes are protected.

Patches were Acked by Kees Cook and Andy Lutomirski. Thank you!


This is a resend, first send:
https://lkml.org/lkml/2013/12/15/114

Of the already version 2, the original discussion:
https://lkml.org/lkml/2013/8/26/354
(date: Aug 2013, and it can be used to leak ASLR).


Kees Cook also confirmed the security exposure here:
https://lkml.org/lkml/2013/8/28/564

At least we have a VFS protection for now.


Reminder:
I've discussed the technique to use the 'file->f_cred' to protect proc
entries here:
https://lkml.org/lkml/2013/10/1/371

Eric suggest it, I did the implementation and it was rejected.

Good I've took _all_ the comments in consideration, and came up with
another scheme. It will protect *already running* processes, but first
I need to get this simple series accepted!


Thanks!


Djalal Harouni (2):
 procfs: make /proc/*/{stack,syscall,personality} 0400
 procfs: make /proc/*/pagemap 0400

 fs/proc/base.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

[PATCH v2 1/2] procfs: make /proc/*/{stack,syscall,personality} 0400

[Djalal Harouni]

These procfs files contain sensitive information and currently their
mode is 0444. Change this to 0400, so the VFS will be able to block
unprivileged processes from getting file descriptors on arbitrary
privileged /proc/*/{stack,syscall,personality} files.

This reduces the scope of ASLR leaking and bypasses by protecting
already running processes.

Cc: Eric W. Biederman <ebiederm@xmission.com>
Acked-by: Kees Cook <keescook@chromium.org>
Acked-by: Andy Lutomirski <luto@amacapital.net>
Signed-off-by: Djalal Harouni <tixxdz@opendz.org>
---
 fs/proc/base.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 5150706..e69df4b 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2587,7 +2587,7 @@ static const struct pid_entry tgid_base_stuff[] = {
 	REG("environ",    S_IRUSR, proc_environ_operations),
 	INF("auxv",       S_IRUSR, proc_pid_auxv),
 	ONE("status",     S_IRUGO, proc_pid_status),
-	ONE("personality", S_IRUGO, proc_pid_personality),
+	ONE("personality", S_IRUSR, proc_pid_personality),
 	INF("limits",	  S_IRUGO, proc_pid_limits),
 #ifdef CONFIG_SCHED_DEBUG
 	REG("sched",      S_IRUGO|S_IWUSR, proc_pid_sched_operations),
@@ -2597,7 +2597,7 @@ static const struct pid_entry tgid_base_stuff[] = {
 #endif
 	REG("comm",      S_IRUGO|S_IWUSR, proc_pid_set_comm_operations),
 #ifdef CONFIG_HAVE_ARCH_TRACEHOOK
-	INF("syscall",    S_IRUGO, proc_pid_syscall),
+	INF("syscall",    S_IRUSR, proc_pid_syscall),
 #endif
 	INF("cmdline",    S_IRUGO, proc_pid_cmdline),
 	ONE("stat",       S_IRUGO, proc_tgid_stat),
@@ -2625,7 +2625,7 @@ static const struct pid_entry tgid_base_stuff[] = {
 	INF("wchan",      S_IRUGO, proc_pid_wchan),
 #endif
 #ifdef CONFIG_STACKTRACE
-	ONE("stack",      S_IRUGO, proc_pid_stack),
+	ONE("stack",      S_IRUSR, proc_pid_stack),
 #endif
 #ifdef CONFIG_SCHEDSTATS
 	INF("schedstat",  S_IRUGO, proc_pid_schedstat),
@@ -2926,14 +2926,14 @@ static const struct pid_entry tid_base_stuff[] = {
 	REG("environ",   S_IRUSR, proc_environ_operations),
 	INF("auxv",      S_IRUSR, proc_pid_auxv),
 	ONE("status",    S_IRUGO, proc_pid_status),
-	ONE("personality", S_IRUGO, proc_pid_personality),
+	ONE("personality", S_IRUSR, proc_pid_personality),
 	INF("limits",	 S_IRUGO, proc_pid_limits),
 #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),
 #ifdef CONFIG_HAVE_ARCH_TRACEHOOK
-	INF("syscall",   S_IRUGO, proc_pid_syscall),
+	INF("syscall",   S_IRUSR, proc_pid_syscall),
 #endif
 	INF("cmdline",   S_IRUGO, proc_pid_cmdline),
 	ONE("stat",      S_IRUGO, proc_tid_stat),
@@ -2963,7 +2963,7 @@ static const struct pid_entry tid_base_stuff[] = {
 	INF("wchan",     S_IRUGO, proc_pid_wchan),
 #endif
 #ifdef CONFIG_STACKTRACE
-	ONE("stack",      S_IRUGO, proc_pid_stack),
+	ONE("stack",      S_IRUSR, proc_pid_stack),
 #endif
 #ifdef CONFIG_SCHEDSTATS
 	INF("schedstat", S_IRUGO, proc_pid_schedstat),
-- 
1.7.11.7

Re: [PATCH v2 1/2] procfs: make /proc/*/{stack,syscall,personality} 0400

[Andrew Morton]

On Sat, 22 Mar 2014 14:37:39 +0100 Djalal Harouni <tixxdz@opendz.org> wrote:

> These procfs files contain sensitive information and currently their
> mode is 0444. Change this to 0400, so the VFS will be able to block
> unprivileged processes from getting file descriptors on arbitrary
> privileged /proc/*/{stack,syscall,personality} files.
> 
> This reduces the scope of ASLR leaking and bypasses by protecting
> already running processes.

Sigh.  Why on earth did we make these 444 in the first place.

There is risk of breakage here.  Probably anyone who is using these
files and who observes such breakage is a sophisticated user who
understands the reasons and knows how to make the tools work again. 
There's only one way to find out :(

I can't remember why we even added /proc/pid/syscall - the changelog
doesn't give a rationale (this is regrettably common).

Now for a six-year-late code review:

- How the heck can target==current in task_current_syscall()?

- Less talk, more action:

From: Andrew Morton <akpm@linux-foundation.org>
Subject: lib/syscall.c: unexport task_current_syscall()

It is only used by procfs and procfs cannot be a module.

Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 lib/syscall.c |    1 -
 1 file changed, 1 deletion(-)

diff -puN lib/syscall.c~a lib/syscall.c
--- a/lib/syscall.c~a
+++ a/lib/syscall.c
@@ -72,4 +72,3 @@ int task_current_syscall(struct task_str
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(task_current_syscall);
_

Re: [PATCH v2 1/2] procfs: make /proc/*/{stack,syscall,personality} 0400

[Oleg Nesterov]

On 03/28, Andrew Morton wrote:
>
> Now for a six-year-late code review:
>
> - How the heck can target==current in task_current_syscall()?
>
> - Less talk, more action:
>
> From: Andrew Morton <akpm@linux-foundation.org>
> Subject: lib/syscall.c: unexport task_current_syscall()
>
> It is only used by procfs and procfs cannot be a module.
>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
>
>  lib/syscall.c |    1 -
>  1 file changed, 1 deletion(-)
>
> diff -puN lib/syscall.c~a lib/syscall.c
> --- a/lib/syscall.c~a
> +++ a/lib/syscall.c
> @@ -72,4 +72,3 @@ int task_current_syscall(struct task_str
>
>  	return 0;
>  }
> -EXPORT_SYMBOL_GPL(task_current_syscall);

Well, I guess it was added for external tracing modules like systemtap.
And in this case target==current is very likely. And for utrace modules,
but it is dead.

I do not see any usage of task_current_syscall() in
git://sourceware.org/git/systemtap.git, so this change should not upset
systemtap at least.

And if we unexport task_current_syscall(), perhaps we should actually
lib/syscall.c. proc_pid_syscall() can use syscall_get_*() instead.
Or at least we can simplify it, I don't think /proc/ actually needs
wait_task_inactive().

Oleg.

Re: [PATCH v2 1/2] procfs: make /proc/*/{stack,syscall,personality} 0400

[Djalal Harouni]

On Fri, Mar 28, 2014 at 03:32:31PM -0700, Andrew Morton wrote:
> On Sat, 22 Mar 2014 14:37:39 +0100 Djalal Harouni <tixxdz@opendz.org> wrote:
> 
> > These procfs files contain sensitive information and currently their
> > mode is 0444. Change this to 0400, so the VFS will be able to block
> > unprivileged processes from getting file descriptors on arbitrary
> > privileged /proc/*/{stack,syscall,personality} files.
> > 
> > This reduces the scope of ASLR leaking and bypasses by protecting
> > already running processes.
> 
> Sigh.  Why on earth did we make these 444 in the first place.
> 
> There is risk of breakage here.  Probably anyone who is using these
> files and who observes such breakage is a sophisticated user who
> understands the reasons and knows how to make the tools work again. 
> There's only one way to find out :(
> 
> I can't remember why we even added /proc/pid/syscall - the changelog
> doesn't give a rationale (this is regrettably common).
Yep :-/

Ok, thank you for taking the patches!

Just to update the tread, Alexey Dobriyan suggested that we also improve
/proc/*/wchan

I'll do it by caching the ptrace check during open time, and re-check it
in proc_pid_wchan() only in case we return an address, this way we do not
break tools. Will add it to the patches I've and send it soon (didn't
want to disturb you during the merge window).

Thanks!

-- 
Djalal Harouni
http://opendz.org

[PATCH v2 1/2] procfs: make /proc/*/pagemap 0400

[Djalal Harouni]

The /proc/*/pagemap contain sensitive information and currently its
mode is 0444. Change this to 0400, so the VFS will prevent unprivileged
processes from getting file descriptors on arbitrary privileged
/proc/*/pagemap files.

This reduces the scope of address space leaking and bypasses by
protecting already running processes.

Cc: Eric W. Biederman <ebiederm@xmission.com>
Acked-by: Kees Cook <keescook@chromium.org>
Acked-by: Andy Lutomirski <luto@amacapital.net>
Signed-off-by: Djalal Harouni <tixxdz@opendz.org>
---
 fs/proc/base.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index e69df4b..081d055 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2616,7 +2616,7 @@ static const struct pid_entry tgid_base_stuff[] = {
 #ifdef CONFIG_PROC_PAGE_MONITOR
 	REG("clear_refs", S_IWUSR, proc_clear_refs_operations),
 	REG("smaps",      S_IRUGO, proc_pid_smaps_operations),
-	REG("pagemap",    S_IRUGO, proc_pagemap_operations),
+	REG("pagemap",    S_IRUSR, proc_pagemap_operations),
 #endif
 #ifdef CONFIG_SECURITY
 	DIR("attr",       S_IRUGO|S_IXUGO, proc_attr_dir_inode_operations, proc_attr_dir_operations),
@@ -2954,7 +2954,7 @@ static const struct pid_entry tid_base_stuff[] = {
 #ifdef CONFIG_PROC_PAGE_MONITOR
 	REG("clear_refs", S_IWUSR, proc_clear_refs_operations),
 	REG("smaps",     S_IRUGO, proc_tid_smaps_operations),
-	REG("pagemap",    S_IRUGO, proc_pagemap_operations),
+	REG("pagemap",    S_IRUSR, proc_pagemap_operations),
 #endif
 #ifdef CONFIG_SECURITY
 	DIR("attr",      S_IRUGO|S_IXUGO, proc_attr_dir_inode_operations, proc_attr_dir_operations),
-- 
1.7.11.7

Re: [PATCH resend - v2 0/2] procfs: make /proc/*/{stack,syscall,pagemap} 0400

[Kees Cook]

On Sat, Mar 22, 2014 at 7:37 AM, Djalal Harouni <tixxdz@opendz.org> wrote:
> (Please note: this is a resend of version 2, I got two Acked-by, but no
> one replied on why it should not be applied...)
>
>
> The following patches make /proc/*/{stack,syscall,personality,pagemap}
> 0400.
>
> These files contain sensitive information that can be used by an
> unprivileged process to leak address space and bypass ASLR. This will
> make the VFS able to bloc unprivileged processes from getting file
> descriptors on *already* *running* processes (privileged processes).
>
> This does not protect all the /proc and exec-suid cases. It just reduces
> the scope of ASLR leaks by protecting *already running* processes. The
> leak is still possible on these files *only* if an attacker opens its
> /proc/*/file and can *spawn* a target setuid process, then read from it.
>
> So, only already running processes are protected.
>
> Patches were Acked by Kees Cook and Andy Lutomirski. Thank you!
>
>
> This is a resend, first send:
> https://lkml.org/lkml/2013/12/15/114
>
> Of the already version 2, the original discussion:
> https://lkml.org/lkml/2013/8/26/354
> (date: Aug 2013, and it can be used to leak ASLR).
>
>
> Kees Cook also confirmed the security exposure here:
> https://lkml.org/lkml/2013/8/28/564
>
> At least we have a VFS protection for now.

Yes please. :)

Thanks for resending this!

-Kees

>
>
> Reminder:
> I've discussed the technique to use the 'file->f_cred' to protect proc
> entries here:
> https://lkml.org/lkml/2013/10/1/371
>
> Eric suggest it, I did the implementation and it was rejected.
>
> Good I've took _all_ the comments in consideration, and came up with
> another scheme. It will protect *already running* processes, but first
> I need to get this simple series accepted!
>
>
> Thanks!
>
>
> Djalal Harouni (2):
>  procfs: make /proc/*/{stack,syscall,personality} 0400
>  procfs: make /proc/*/pagemap 0400
>
>  fs/proc/base.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)



-- 
Kees Cook
Chrome OS Security

Re: [PATCH v2 1/2] procfs: make /proc/*/{stack,syscall,personality} 0400

[Alexey Dobriyan]

> - ONE("stack",      S_IRUGO, proc_pid_stack),
> + ONE("stack",      S_IRUSR, proc_pid_stack),

no love for /proc/*/wchan?

Re: [PATCH v2 1/2] procfs: make /proc/*/{stack,syscall,personality} 0400

[Djalal Harouni]

Hi Alexey,

On Mon, Mar 24, 2014 at 04:41:43PM +0300, Alexey Dobriyan wrote:
> > - ONE("stack",      S_IRUGO, proc_pid_stack),
> > + ONE("stack",      S_IRUSR, proc_pid_stack),
> 
> no love for /proc/*/wchan?

Yes you are right, didn't want to modify lot of things, and I missed that one.

So there is the ptrace check that will just block cases where users do
_not_ play tricks with the fd. I'll send another v3 with that included.

Thanks!

-- 
Djalal Harouni
http://opendz.org