diff mbox

inet_diag: fix inet_diag_bc_audit()

Message ID 1308341990.3539.27.camel@edumazet-laptop
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Eric Dumazet June 17, 2011, 8:19 p.m. UTC 
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(-)



--
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

Comments

David Miller June 17, 2011, 8:26 p.m. UTC | #1 
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!
--
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
diff mbox

Patch

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;
 	}