inet_diag insufficient validation?

inet_diag insufficient validation?

[Dan Rosenberg]

It seems to me that the auditing performed by inet_diag_bc_audit() is
insufficient to prevent pathological INET_DIAG bytecode from doing bad
things.

Firstly, it's possible to cause an infinite loop in inet_diag_bc_audit()
with a INET_DIAG_BC_JMP opcode with a "yes" value of 0.  The valid_cc()
function, also called from here, seems suspicious as well.

Once the bytecode is actually run in inet_diag_bc_run(), it looks like
more infinite loops are possible, if appropriate "yes" or "no" values
are set to zero and weren't validated by the audit.

Finally, I can't seem to find any validation that the reported length of
the netlink message header doesn't exceed the skb length, as checked in
some other netlink receive functions, which could result in reading
beyond the bounds of the socket data.  I could just be missing something
here though.

Regards,
Dan

Re: inet_diag insufficient validation?

[Dan Rosenberg]

On Wed, 2011-06-01 at 11:40 -0400, Dan Rosenberg wrote:

> Once the bytecode is actually run in inet_diag_bc_run(), it looks like
> more infinite loops are possible, if appropriate "yes" or "no" values
> are set to zero and weren't validated by the audit.

On second glance, there don't appear to be any infinite loops at runtime
(but the audit loop seems real).  Thanks to Nelson Elhage for setting me
straight.

-Dan

Re: inet_diag insufficient validation?

[Dan Rosenberg]

On Wed, 2011-06-01 at 11:40 -0400, Dan Rosenberg wrote:

> Finally, I can't seem to find any validation that the reported length of
> the netlink message header doesn't exceed the skb length, as checked in
> some other netlink receive functions, which could result in reading
> beyond the bounds of the socket data.  I could just be missing something
> here though.
> 

And for the second time, I was missing something - this validation
happens in netlink_rcv_skb().

That leaves the infinite loop in bytecode auditing, which I've confirmed
via reproducer.

-Dan

Re: [Security] inet_diag insufficient validation?

[Eugene Teo]

Cc'ed acme.

On Wed, Jun 1, 2011 at 11:40 PM, Dan Rosenberg <drosenberg@vsecurity.com> wrote:
> It seems to me that the auditing performed by inet_diag_bc_audit() is
> insufficient to prevent pathological INET_DIAG bytecode from doing bad
> things.
>
> Firstly, it's possible to cause an infinite loop in inet_diag_bc_audit()
> with a INET_DIAG_BC_JMP opcode with a "yes" value of 0.  The valid_cc()
> function, also called from here, seems suspicious as well.
>
> Once the bytecode is actually run in inet_diag_bc_run(), it looks like
> more infinite loops are possible, if appropriate "yes" or "no" values
> are set to zero and weren't validated by the audit.
>
> Finally, I can't seem to find any validation that the reported length of
> the netlink message header doesn't exceed the skb length, as checked in
> some other netlink receive functions, which could result in reading
> beyond the bounds of the socket data.  I could just be missing something
> here though.
>
> Regards,
> Dan
>
> _______________________________________________
> Security mailing list
> Security@linux.kernel.org
> http://linux.kernel.org/mailman/listinfo/security
>

Re: [Security] inet_diag insufficient validation?

[Dan Rosenberg]

On Fri, 2011-06-03 at 14:55 +0800, Eugene Teo wrote:
> Cc'ed acme.
> 
> On Wed, Jun 1, 2011 at 11:40 PM, Dan Rosenberg <drosenberg@vsecurity.com> wrote:
> > It seems to me that the auditing performed by inet_diag_bc_audit() is
> > insufficient to prevent pathological INET_DIAG bytecode from doing bad
> > things.
> >
> > Firstly, it's possible to cause an infinite loop in inet_diag_bc_audit()
> > with a INET_DIAG_BC_JMP opcode with a "yes" value of 0.  The valid_cc()
> > function, also called from here, seems suspicious as well.
> >

Any chance of getting this fixed?  I have a reproducer available if
necessary.

-Dan

[PATCH] inet_diag: fix inet_diag_bc_audit()

[Eric Dumazet]

Le vendredi 03 juin 2011 à 14:55 +0800, Eugene Teo a écrit :
> Cc'ed acme.
> 
> On Wed, Jun 1, 2011 at 11:40 PM, Dan Rosenberg <drosenberg@vsecurity.com> wrote:
> > It seems to me that the auditing performed by inet_diag_bc_audit() is
> > insufficient to prevent pathological INET_DIAG bytecode from doing bad
> > things.
> >
> > Firstly, it's possible to cause an infinite loop in inet_diag_bc_audit()
> > with a INET_DIAG_BC_JMP opcode with a "yes" value of 0.  The valid_cc()
> > function, also called from here, seems suspicious as well.
> >
> > Once the bytecode is actually run in inet_diag_bc_run(), it looks like
> > more infinite loops are possible, if appropriate "yes" or "no" values
> > are set to zero and weren't validated by the audit.
> >
> > Finally, I can't seem to find any validation that the reported length of
> > the netlink message header doesn't exceed the skb length, as checked in
> > some other netlink receive functions, which could result in reading
> > beyond the bounds of the socket data.  I could just be missing something
> > here though.
> >
> > Regards,
> > Dan
> >

Thanks guys, here is the patch I cooked to address this problem, sorry
for the long delay again.

[PATCH] inet_diag: fix inet_diag_bc_audit()

A malicious user or buggy application can inject code and trigger an
infinite loop in inet_diag_bc_audit()

Also make sure each instruction is aligned on 4 bytes boundary, to avoid
unaligned accesses.

Reported-by: Dan Rosenberg <drosenberg@vsecurity.com>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
CC: Stephen Hemminger <shemminger@vyatta.com>
CC: Arnaldo Carvalho de Melo <acme@infradead.org>
CC: Eugene Teo <eugeneteo@kernel.sg>
CC: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
---
 net/ipv4/inet_diag.c |   14 ++++++--------
 1 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
index 6ffe94c..3267d38 100644
--- a/net/ipv4/inet_diag.c
+++ b/net/ipv4/inet_diag.c
@@ -437,7 +437,7 @@ static int valid_cc(const void *bc, int len, int cc)
 			return 0;
 		if (cc == len)
 			return 1;
-		if (op->yes < 4)
+		if (op->yes < 4 || op->yes & 3)
 			return 0;
 		len -= op->yes;
 		bc  += op->yes;
@@ -447,11 +447,11 @@ static int valid_cc(const void *bc, int len, int cc)
 
 static int inet_diag_bc_audit(const void *bytecode, int bytecode_len)
 {
-	const unsigned char *bc = bytecode;
+	const void *bc = bytecode;
 	int  len = bytecode_len;
 
 	while (len > 0) {
-		struct inet_diag_bc_op *op = (struct inet_diag_bc_op *)bc;
+		const struct inet_diag_bc_op *op = bc;
 
 //printk("BC: %d %d %d {%d} / %d\n", op->code, op->yes, op->no, op[1].no, len);
 		switch (op->code) {
@@ -462,22 +462,20 @@ static int inet_diag_bc_audit(const void *bytecode, int bytecode_len)
 		case INET_DIAG_BC_S_LE:
 		case INET_DIAG_BC_D_GE:
 		case INET_DIAG_BC_D_LE:
-			if (op->yes < 4 || op->yes > len + 4)
-				return -EINVAL;
 		case INET_DIAG_BC_JMP:
-			if (op->no < 4 || op->no > len + 4)
+			if (op->no < 4 || op->no > len + 4 || op->no & 3)
 				return -EINVAL;
 			if (op->no < len &&
 			    !valid_cc(bytecode, bytecode_len, len - op->no))
 				return -EINVAL;
 			break;
 		case INET_DIAG_BC_NOP:
-			if (op->yes < 4 || op->yes > len + 4)
-				return -EINVAL;
 			break;
 		default:
 			return -EINVAL;
 		}
+		if (op->yes < 4 || op->yes > len + 4 || op->yes & 3)
+			return -EINVAL;
 		bc  += op->yes;
 		len -= op->yes;
 	}

Re: [PATCH] inet_diag: fix inet_diag_bc_audit()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 17 Jun 2011 22:19:50 +0200

> [PATCH] inet_diag: fix inet_diag_bc_audit()
> 
> A malicious user or buggy application can inject code and trigger an
> infinite loop in inet_diag_bc_audit()
> 
> Also make sure each instruction is aligned on 4 bytes boundary, to avoid
> unaligned accesses.
> 
> Reported-by: Dan Rosenberg <drosenberg@vsecurity.com>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Applied and queued up for -stable, thanks!