netdev - [PATCH] inet_diag: fix inet_diag_bc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1308341990.3539.27.camel@edumazet-laptop>
Date:	Fri, 17 Jun 2011 22:19:50 +0200
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Eugene Teo <eugeneteo@...nel.sg>
Cc:	Dan Rosenberg <drosenberg@...curity.com>, davem@...emloft.net,
	kuznet@....inr.ac.ru, netdev@...r.kernel.org, security@...nel.org,
	Arnaldo Carvalho de Melo <acme@...hat.com>,
	Stephen Hemminger <shemminger@...tta.com>
Subject: [PATCH] inet_diag: fix inet_diag_bc_audit()

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@...curity.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@...curity.com>
Signed-off-by: Eric Dumazet <eric.dumazet@...il.com>
CC: Stephen Hemminger <shemminger@...tta.com>
CC: Arnaldo Carvalho de Melo <acme@...radead.org>
CC: Eugene Teo <eugeneteo@...nel.sg>
CC: Alexey Kuznetsov <kuznet@....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;
 	}


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