inet_diag insufficient validation?

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

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

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

Cc'ed acme.

On Wed, Jun 1, 2011 at 11:40 PM, Dan Rosenberg [off-list ref] 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

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

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

-Dan

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

From: Eric Dumazet <redacted>
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 <redacted>
Signed-off-by: Eric Dumazet <redacted>

Applied and queued up for -stable, thanks!