[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 [off-list ref] wrote:

quoted

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 <redacted>
Signed-off-by: Eric Dumazet <redacted>
CC: Stephen Hemminger <redacted>
CC: Arnaldo Carvalho de Melo <redacted>
CC: Eugene Teo <redacted>
CC: Alexey Kuznetsov <redacted>
---
 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;
 	}