[PATCH] proc: avoid leaking eip, esp, or wchan to non-privileged processes (fwd)

[PATCH] proc: avoid leaking eip, esp, or wchan to non-privileged processes (fwd)

[James Morris]

This patch needs some review.

Note that stable@kernel.org typically backport already-reviewed and 
applied patches.  I think security@kernel.org is for reporting problems in 
a non-public way (whereas, this is already public knowledge).

Having someone specifically responsible for maintaining kernel memory 
protection features would be of immense value.  Any takers?

---------- Forwarded message ----------
Date: Thu, 30 Apr 2009 09:11:40 -0600
From: Jake Edge <jake@lwn.net>
To: linux-kernel@vger.kernel.org
Cc: security@kernel.org, stable@kernel.org, linux-fsdevel@vger.kernel.org
Subject: [PATCH] proc: avoid leaking eip, esp,
    or wchan to non-privileged processes


proc: avoid leaking eip, esp, or wchan to non-privileged processes

Using the same test as is used for /proc/pid/maps and smaps, only allow
processes that can ptrace() a given process to access its /proc/pid/wchan and
get eip and esp from /proc/pid/stat.

ASLR can be bypassed by sampling eip as shown by the proof-of-concept code
at http://code.google.com/p/fuzzyaslr/  As part of a presentation
(http://www.cr0.org/paper/to-jt-linux-alsr-leak.pdf) esp and wchan were also
noted as possibly usable information leaks as well.

Signed-off-by: Jake Edge <jake@lwn.net>
---
 fs/proc/array.c |   11 ++++++++---
 fs/proc/base.c  |    3 +++
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/fs/proc/array.c b/fs/proc/array.c
index 7e4877d..e96d508 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -80,6 +80,7 @@
 #include <linux/delayacct.h>
 #include <linux/seq_file.h>
 #include <linux/pid_namespace.h>
+#include <linux/ptrace.h>
 #include <linux/tracehook.h>

 #include <asm/pgtable.h>
@@ -352,6 +353,7 @@ static int do_task_stat(struct seq_file *m, struct
pid_namespace *ns,
 	char state;
 	pid_t ppid = 0, pgid = -1, sid = -1;
 	int num_threads = 0;
+	int permitted;
 	struct mm_struct *mm;
 	unsigned long long start_time;
 	unsigned long cmin_flt = 0, cmaj_flt = 0;
@@ -364,11 +366,14 @@ static int do_task_stat(struct seq_file *m, struct
pid_namespace *ns,

 	state = *get_task_state(task);
 	vsize = eip = esp = 0;
+	permitted = ptrace_may_access(task, PTRACE_MODE_READ);
 	mm = get_task_mm(task);
 	if (mm) {
 		vsize = task_vsize(mm);
-		eip = KSTK_EIP(task);
-		esp = KSTK_ESP(task);
+		if (permitted) {
+			eip = KSTK_EIP(task);
+			esp = KSTK_ESP(task);
+		}
 	}

 	get_task_comm(tcomm, task);
@@ -424,7 +429,7 @@ static int do_task_stat(struct seq_file *m, struct
pid_namespace *ns,
 		unlock_task_sighand(task, &flags);
 	}

-	if (!whole || num_threads < 2)
+	if (permitted && (!whole || num_threads < 2))
 		wchan = get_wchan(task);
 	if (!whole) {
 		min_flt = task->min_flt;
diff --git a/fs/proc/base.c b/fs/proc/base.c
index aa763ab..cd3c0d7 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -319,6 +319,9 @@ static int proc_pid_wchan(struct task_struct *task, char
*buffer)
 	unsigned long wchan;
 	char symname[KSYM_NAME_LEN];

+	if (!ptrace_may_access(task, PTRACE_MODE_READ))
+		return 0;
+
 	wchan = get_wchan(task);

 	if (lookup_symbol_name(wchan, symname) < 0)
-- 
1.6.2.2


-- 
Jake Edge - LWN - jake@lwn.net - http://lwn.net
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] proc: avoid leaking eip, esp, or wchan to non-privileged processes (fwd)

[Arjan van de Ven]

On Mon, 4 May 2009 09:27:29 +1000 (EST)
James Morris <jmorris@namei.org> wrote:

> This patch needs some review.

having just gotten back from another trip...

this patch looks relevant, but has a compatibility risk.... Personally
I think it'd be ok, but if things would break we do have some options
that are better than 'do nothing'.

One option to reduce the compatibility risk (as the expense of the
security angle) would be to mask the lower, say 4 to 8 bits of these
variables instead for non-ptrace-capable callers. Other obfuscation
options are obviously also possible.



> 
> Having someone specifically responsible for maintaining kernel memory 
> protection features would be of immense value.  Any takers?

I suppose I could do that.... but would be nice to have a few folks.



-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH] proc: avoid leaking eip, esp, or wchan to non-privileged processes (fwd)

[Jake Edge]

On Mon, 4 May 2009 09:27:29 +1000 (EST) James Morris wrote:
> This patch needs some review.

Indeed.

> Note that stable@kernel.org typically backport already-reviewed and 
> applied patches.  I think security@kernel.org is for reporting
> problems in a non-public way (whereas, this is already public
> knowledge).

I realize (now :) that I didn't get this out to all of the right folks,
thanks for doing that.  I didn't realize security@kernel.org was only
for non-public security problems, though.  Maybe there needs to be a
'security maintainer' separate from that list?  Or maybe there is one
and I just didn't find that person in MAINTAINERS?

It would seem that the 'start_stack' value output by /proc/pid/stat
should also only be available to processes that can ptrace ... that was
not part of my original patch, but I think should be added ...

jake

-- 
Jake Edge - LWN - jake@lwn.net - http://lwn.net

Re: [PATCH] proc: avoid leaking eip, esp, or wchan to non-privileged processes (fwd)

[Greg KH]

On Sun, May 03, 2009 at 07:11:24PM -0600, Jake Edge wrote:
> On Mon, 4 May 2009 09:27:29 +1000 (EST) James Morris wrote:
> > This patch needs some review.
> 
> Indeed.
> 
> > Note that stable@kernel.org typically backport already-reviewed and 
> > applied patches.  I think security@kernel.org is for reporting
> > problems in a non-public way (whereas, this is already public
> > knowledge).
> 
> I realize (now :) that I didn't get this out to all of the right folks,
> thanks for doing that.  I didn't realize security@kernel.org was only
> for non-public security problems, though.  Maybe there needs to be a
> 'security maintainer' separate from that list?  Or maybe there is one
> and I just didn't find that person in MAINTAINERS?

No, you did it correctly, as per Documentation/SecurityBugs.

And, from the MAINTAINERS file:
	SECURITY CONTACT
	P:      Security Officers
	M:      security@kernel.org
	S:      Supported

thanks,

greg k-h

Re: [PATCH] proc: avoid leaking eip, esp, or wchan to non-privileged processes (fwd)

[Eric W. Biederman]

James Morris <jmorris@namei.org> writes:

> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index aa763ab..cd3c0d7 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -319,6 +319,9 @@ static int proc_pid_wchan(struct task_struct *task, char
> *buffer)
>  	unsigned long wchan;
>  	char symname[KSYM_NAME_LEN];
>
> +	if (!ptrace_may_access(task, PTRACE_MODE_READ))
> +		return 0;
> +
>  	wchan = get_wchan(task);
>
>  	if (lookup_symbol_name(wchan, symname) < 0)

If the symbol is in the kernel it should be safe to return it's
name, all that is an information leak of a different sort.
Overall I expect we should return  -EPERM here and not simply
an empty file.

Have you tested these patches against ps top and similar common
tools?

Eric

Re: [PATCH] proc: avoid leaking eip, esp, or wchan to non-privileged processes (fwd)

[Jake Edge]

On Sun, 03 May 2009 23:09:24 -0700 Eric W. Biederman wrote:
> James Morris <jmorris@namei.org> writes:
> 
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index aa763ab..cd3c0d7 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -319,6 +319,9 @@ static int proc_pid_wchan(struct task_struct
> > *task, char *buffer)
> >  	unsigned long wchan;
> >  	char symname[KSYM_NAME_LEN];
> >
> > +	if (!ptrace_may_access(task, PTRACE_MODE_READ))
> > +		return 0;
> > +
> >  	wchan = get_wchan(task);
> >
> >  	if (lookup_symbol_name(wchan, symname) < 0)
> 
> If the symbol is in the kernel it should be safe to return it's
> name, all that is an information leak of a different sort.
> Overall I expect we should return  -EPERM here and not simply
> an empty file.

Ah, so if lookup_symbol_name() fails, it should return nothing or an
error?

This was "modeled" after the maps and smaps behavior, in that
they return an empty file if there is insufficient permission rather
than EPERM ... 

> Have you tested these patches against ps top and similar common
> tools?

both ps and top seem to operate just fine ... any of the default values
coming out of stat should be OK as they can be returned under other
circumstances ... the wchan file is a bit different, but it is not
clear to me what uses that rather than the wchan value in the stat file

thanks,

jake

-- 
Jake Edge - LWN - jake@lwn.net - http://lwn.net