Patchwork Prevent reading uninitialized memory with socket filters

login
register
mail settings
Submitter Dan Rosenberg
Date Nov. 9, 2010, 10:28 p.m.
Message ID <1289341724.7380.13.camel@dan>
Download mbox | patch
Permalink /patch/70584/
State Superseded
Delegated to: David Miller
Headers show

Comments

Dan Rosenberg - Nov. 9, 2010, 10:28 p.m.
The "mem" array used as scratch space for socket filters is not
initialized, allowing unprivileged users to leak kernel stack bytes.

Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com>

---
 net/core/filter.c               |    2 ++
 1 file changed, 2 insertions(+), 0 deletions(-)



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joe Perches - Nov. 9, 2010, 11:03 p.m.
On Tue, 2010-11-09 at 17:28 -0500, Dan Rosenberg wrote:
> The "mem" array used as scratch space for socket filters is not
> initialized, allowing unprivileged users to leak kernel stack bytes.

Hi Dan.

Using
	type var[count] = {};
instead of
	type var[count];
	...
	memset(var, 0, sizeof(var));

at least for gcc 4.4 and 4.5 generally results in smaller code.

$ size net/core/filter.o*
   text	   data	    bss	    dec	    hex	filename
   6751	     56	   1736	   8543	   215f	net/core/filter.o.memset
   6749	     56	   1736	   8541	   215d	net/core/filter.o.init


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Nov. 10, 2010, 5:28 a.m.
From: Dan Rosenberg <drosenberg@vsecurity.com>
Date: Tue, 09 Nov 2010 17:28:44 -0500

> The "mem" array used as scratch space for socket filters is not
> initialized, allowing unprivileged users to leak kernel stack bytes.
> 
> Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com>

Prove it.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet - Nov. 10, 2010, 5:53 a.m.
Le mardi 09 novembre 2010 à 21:28 -0800, David Miller a écrit :
> From: Dan Rosenberg <drosenberg@vsecurity.com>
> Date: Tue, 09 Nov 2010 17:28:44 -0500
> 
> > The "mem" array used as scratch space for socket filters is not
> > initialized, allowing unprivileged users to leak kernel stack bytes.
> > 
> > Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com>
> 
> Prove it.

And once done, add the checks in sk_chk_filter() ?

Allow a load of mem[X] only if a prior store of mem[X] is proven.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Rosenberg - Nov. 10, 2010, 11:12 a.m.
> 
> Prove it.

I hope this was a joke.  In either case, my reply is a joke:

http://lists.grok.org.uk/pipermail/full-disclosure/2010-November/077321.html

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Rosenberg - Nov. 10, 2010, 1:19 p.m.
For reasons I don't understand, this code seems to cause some people's
machines to lock up:

BUG: soft lockup - CPU#0 stuck for 10s! [dz:11844]

I haven't been able to reproduce this myself.  Very strange.  I'll send
a stack trace if I can reproduce.

-Dan

On Wed, 2010-11-10 at 06:12 -0500, Dan Rosenberg wrote:
> > 
> > Prove it.
> 
> I hope this was a joke.  In either case, my reply is a joke:
> 
> http://lists.grok.org.uk/pipermail/full-disclosure/2010-November/077321.html


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Nov. 10, 2010, 6:07 p.m.
From: Dan Rosenberg <drosenberg@vsecurity.com>
Date: Wed, 10 Nov 2010 06:12:47 -0500

> 
>> 
>> Prove it.
> 
> I hope this was a joke.

It absolutely is not.

You are very much not the first person ever to try and add an
expensive memset() here.

So the onus is really on you to prove this assertion and show the
exact code path by which the user can actually see any uninitialized
kernel stack memory (he can't, he can peek at certain values in a
certain extremely contrived range, making the leak useless), rather
than point us at some web external site archive of a list posting
which we cannot easily quote and reply to here.

I think you cannot do it, really.  Except in the AF_PACKET case, the
sockets can only see "0" or a negative error code, not the actual
sk_run_filter() return value.

In the one exception, AF_PACKET, the range of values the user can
see are in the range of MTU of the device being accessed, which
realistically is 1500 bytes.  This means the user cannot see any
kernel stack value outside of the range 0 to 1500, which isn't
worth using this expensive memset to guard against at all.

I don't even think it's worth adding all of the extra cpu cycles
incurred by Eric Dumazet's scheme of using a bitmap test on every
single memory buffer access.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/net/core/filter.c b/net/core/filter.c
index 7beaec3..2749ba0 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -121,6 +121,8 @@  unsigned int sk_run_filter(struct sk_buff *skb, struct sock_filter *filter, int
 	int k;
 	int pc;
 
+	memset(mem, 0, sizeof(mem));
+
 	/*
 	 * Process array of filter instructions.
 	 */