[BUG] Security Announcement: ip6tables bug - level: very high

[BUG] Security Announcement: ip6tables bug - level: very high

[Andras Kis-Szabo]

[-- Attachment #1: Type: text/plain, Size: 3854 bytes --]

Hi,

My worst nightmare came true: a very serious bug in the ip6tables code.
I can not beleive it, so please test it!

On 31 May 2002, Kraus, Darko sent a letter to Jozsef Kadlecsik:
> I have notices two not functional things in ip6tables:
> - ip6tables -A FORWARD -j ACCEPT -p all   <--- the protocol type "all" does
> not match anything. I have to specify individually tcp, udp and icmpv6 in
> order to get forwarded.
> - ip6tables -A FORWARD -j REJECT causes segmentation fault.
> I am using kernel V2.4.18 with latest IPtables 1.2.6a. Also I have applied
> Rusty's patch-o-matic that came with iptables-1.2.6a.tar.gz to that kernel.
> My host is running as a router, and I have no problems setting up iptables
> for IPv4. IPv6 is the one who causes these problems. I am using a tunnel
> broker to connect to the internet via IPv6. If you need some additional info
> please let me know.
Yes, it's true (I will fix this ASAP).
After this I started to play with the test packets.

The exapmle scenario:
# ip6tables -L
# tcpreplay6 <pure-icmpv6
# tcpreplay6 <ah-len24-spi4096-clean-dst-len8-sub2-len4-pad1-pad1
// or any other packet with extension headers
# ip6tables -A OUTPUT -p ipv6-icmp
# tcpreplay6 <pure-icmpv6
# ip6tables -L OUTPUT -n -v
Chain OUTPUT (policy ACCEPT 1 packets, 57 bytes)
 pkts bytes target     prot opt in     out    source destination        
    1    57            icmpv6    *      *      ::/0   ::/0       
# tcpreplay6 <ah-len24-spi4096-clean-dst-len8-sub2-len4-pad1-pad1
// or any other packet with extension headers


*** The kernel crashed immediately. ***

(The test packets can be found in the pool:
http://www.securityaudit.hu/Netfilter/addons/TestPackets/ )

The POSSIBLE location of the problem:
linux/net/ipv6/netfilter/ip6_tables.c
ip6_packet_match() function,
line 231:
-                hdrptr = (u_int8_t *)(ipv6 + 1);
+                hdrptr = (u_int8_t *)((void *)ipv6 + 1);

In patch form:
[..............]
--- ip6_tables-p0bug.c	Wed Jun  5 23:57:31 2002
+++ ip6_tables.c	Thu Jun  6 00:45:54 2002
@@ -228,7 +228,7 @@
 	if((ip6info->flags & IP6T_F_PROTO)) {
 		u_int8_t currenthdr = ipv6->nexthdr;
 		u_int8_t *hdrptr;
-		hdrptr = (u_int8_t *)(ipv6 + 1);
+		hdrptr = (u_int8_t *)((void *)ipv6 + 1);
 		do {
 			if (ip6info->proto == currenthdr) {
 				if(ip6info->invflags & IP6T_INV_PROTO)
[..............]

Other problem:
ip6_nexthdr() function:
[..............]
--- ip6_tables-p0bug.c	Wed Jun  5 23:57:31 2002
+++ ip6_tables.c	Thu Jun  6 00:50:11 2002
@@ -138,7 +138,6 @@
    and returns the new header value, else returns 0 */
 static u_int8_t ip6_nexthdr(u_int8_t currenthdr, u_int8_t *hdrptr)
 {
-	int i;
 	u_int8_t hdrlen, nexthdr = 0;
 	switch(currenthdr){
 		case IPPROTO_AH:
@@ -147,7 +146,7 @@
 		repeatedly...with a large stick...no, an even LARGER
 		stick...no, you're still not thinking big enough */
 			nexthdr = *hdrptr;
-			hdrlen = hdrptr[i] * 4 + 8;
+			hdrlen = hdrptr[1] * 4 + 8;
 			hdrptr = hdrptr + hdrlen;
 			break;
 		/*stupid rfc2402 */
[..............]
The variable 'i' used uninitzialized - a copied buggy code?

I don't know which one is the correct solution, I will test it (maybe in
an hour).

*** Affected systems ***
Any Linux system, which has a rule with protocol match (-p)!

*** Preparedness ***
Any script-kiddie, or a plain user with instructions.
Automated tool: possible

*** Result ***
Kernel crash

*** Required packet ***
- any IPv6 packet with options (fix: near the 1st patch)
- IPv6 packet with AH option (fix: near the 2nd patch)

Regards,

	kisza

-- 
    Andras Kis-Szabo       Security Development, Design and Audit
-------------------------/        Zorp, NetFilter and IPv6
 kisza@SecurityAudit.hu /-----Member of the BUTE-MIS-SEARCHlab------>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[PATCH] Re: [BUG] Security Announcement: ip6tables bug - level: very high

[Andras Kis-Szabo]

[-- Attachment #1: Type: text/plain, Size: 2576 bytes --]

Hi,


*** Description / example ***

The exapmle scenario:
# ip6tables -L
# tcpreplay6 <pure-icmpv6
# tcpreplay6 <ah-len24-spi4096-clean-dst-len8-sub2-len4-pad1-pad1
// or any other packet with extension headers
# ip6tables -A OUTPUT -p ipv6-icmp
# tcpreplay6 <pure-icmpv6
# ip6tables -L OUTPUT -n -v
Chain OUTPUT (policy ACCEPT 1 packets, 57 bytes)
 pkts bytes target     prot opt in     out    source destination        
    1    57            icmpv6    *      *      ::/0   ::/0       
# tcpreplay6 <ah-len24-spi4096-clean-dst-len8-sub2-len4-pad1-pad1
// or any other packet with extension headers
 
	*** The kernel crashed immediately. ***

(The test packets can be found in the pool:
http://www.securityaudit.hu/Netfilter/addons/TestPackets/ )
 
*** Background ***
The original code contains 
- 2 errors in its source code and 
	When calculates the length of the AH header it used an index 	from the
'air'.
> -		      hdrlen = hdrptr[i] * 4 + 8;
> +			hdrlen = hdrptr[1] * 4 + 8;
	When set the pointer for the first header
> -                hdrptr = (u_int8_t *)(ipv6 + 1);
> +                hdrptr = (u_int8_t *)((void *)ipv6 + 1);
	(the kernel crashed here...)
- 1 error in its design.
	The code checked the extension headers - without the headers.
	It set the index after the ipv6 header and started iterate the
	headers until the protocol header to find the type of the 
	protocol. In the skbuff the ipv6 header is only the pure header,
	nothing more, so it walked somewhere in the kernel memory.


*** Affected systems ***
Any Linux system, which has a rule with protocol match (-p)!
 
*** Preparedness ***
Any script-kiddie, or a plain user with instructions.
Automated tool: possible
 
*** Result ***
Kernel crash
 
*** Required packet ***
- any IPv6 packet with options

*** Modifications in the code ***
I got the header parser code from my extension header patches, and put
it into the ip6_tables.c.
It can be buggy - I checked with the testpackets and worked properly.
It ahndles the '-p all' counter quiestion/ticket, too.
I left the original code (with the debug messages) in the file.

For the '-p all' question:
- patkets with NONE or ESP ext. headers won't be counted (it's only a
return, so it can be changed)
- the ip6tables never adds the size of the extensions to the counters!
	(It counts only the ipv6-header and the payload.)

Regards,
 
 	kisza

-- 
    Andras Kis-Szabo       Security Development, Design and Audit
-------------------------/        Zorp, NetFilter and IPv6
 kisza@SecurityAudit.hu /-----Member of the BUTE-MIS-SEARCHlab------>

[-- Attachment #2: exthdr-bug-20020606 --]
[-- Type: text/plain, Size: 5696 bytes --]

--- linux-p0bug/net/ipv6/netfilter/ip6_tables.c	Wed Jun  5 23:57:31 2002
+++ linux/net/ipv6/netfilter/ip6_tables.c	Thu Jun  6 03:30:59 2002
@@ -7,6 +7,8 @@
  * 19 Jan 2002 Harald Welte <laforge@gnumonks.org>
  * 	- increase module usage count as soon as we have rules inside
  * 	  a table
+ * 06 Jun 2002 Andras Kis-Szabo <kisza@sch.bme.hu>
+ *      - new (and real) extension header parser code
  */
 #include <linux/config.h>
 #include <linux/skbuff.h>
@@ -25,6 +27,7 @@
 #include <linux/netfilter_ipv6/ip6_tables.h>
 
 #define IPV6_HDR_LEN	(sizeof(struct ipv6hdr))
+#define IPV6_OPTHDR_LEN	(sizeof(struct ipv6_opt_hdr))
 
 /*#define DEBUG_IP_FIREWALL*/
 /*#define DEBUG_ALLOW_ALL*/ /* Useful for remote debugging */
@@ -133,12 +136,26 @@
 	return 0;
 }
 
+/* Check for an extension */
+static int ipv6_ext_hdr(u8 nexthdr)
+{
+        return ( (nexthdr == IPPROTO_HOPOPTS)   ||
+                 (nexthdr == IPPROTO_ROUTING)   ||
+                 (nexthdr == IPPROTO_FRAGMENT)  ||
+                 (nexthdr == IPPROTO_ESP)       ||
+                 (nexthdr == IPPROTO_AH)        ||
+                 (nexthdr == IPPROTO_NONE)      ||
+                 (nexthdr == IPPROTO_DSTOPTS) );
+}
+
+#if 0
+/* The old extension-header parser. Sorry for the #if 0!
+ * 		-- kisza */
 /* takes in current header and pointer to the header */
 /* if another header exists, sets hdrptr to the next header
    and returns the new header value, else returns 0 */
 static u_int8_t ip6_nexthdr(u_int8_t currenthdr, u_int8_t *hdrptr)
 {
-	int i;
 	u_int8_t hdrlen, nexthdr = 0;
 	switch(currenthdr){
 		case IPPROTO_AH:
@@ -147,13 +164,14 @@
 		repeatedly...with a large stick...no, an even LARGER
 		stick...no, you're still not thinking big enough */
 			nexthdr = *hdrptr;
-			hdrlen = hdrptr[i] * 4 + 8;
+			hdrlen = hdrptr[1] * 4 + 8;
 			hdrptr = hdrptr + hdrlen;
 			break;
 		/*stupid rfc2402 */
 		case IPPROTO_DSTOPTS:
 		case IPPROTO_ROUTING:
 		case IPPROTO_HOPOPTS:
+			dprintf(" Found a general header.\n");
 			nexthdr = *hdrptr;
 			hdrlen = hdrptr[1] * 8 + 8;
 			hdrptr = hdrptr + hdrlen;
@@ -163,13 +181,18 @@
 			hdrptr = hdrptr + 8;
 			break;
 	}	
+
+	dprintf(" Found header: %hi\n", nexthdr);
+
 	return nexthdr;
 
 }
+#endif
 
 /* Returns whether matches rule or not. */
 static inline int
-ip6_packet_match(const struct ipv6hdr *ipv6,
+ip6_packet_match(const struct sk_buff *skb,
+		 const struct ipv6hdr *ipv6,
 		 const char *indev,
 		 const char *outdev,
 		 const struct ip6t_ip6 *ip6info,
@@ -227,17 +250,86 @@
 	/* look for the desired protocol header */
 	if((ip6info->flags & IP6T_F_PROTO)) {
 		u_int8_t currenthdr = ipv6->nexthdr;
-		u_int8_t *hdrptr;
-		hdrptr = (u_int8_t *)(ipv6 + 1);
+		struct ipv6_opt_hdr *hdrptr;
+		u_int16_t ptr;		/* Header offset in skb */
+		u_int16_t hdrlen;	/* Header */
+
+		ptr = IPV6_HDR_LEN;
+
+		while (ipv6_ext_hdr(currenthdr)) {
+	                /* Is there enough space for the next ext header? */
+	                if (skb->len - ptr < IPV6_OPTHDR_LEN)
+	                        return 0;
+
+			/* NONE or ESP: there isn't protocol part */
+			/* If we want to count these packets in '-p all',
+			 * we will change the return 0 to 1*/
+			if ((currenthdr == IPPROTO_NONE) || 
+				(currenthdr == IPPROTO_ESP))
+				return 0;
+
+	                hdrptr = (struct ipv6_opt_hdr *)(skb->data + ptr);
+
+			/* Size calculation */
+	                if (currenthdr == IPPROTO_FRAGMENT) {
+	                        hdrlen = 8;
+	                } else if (currenthdr == IPPROTO_AH)
+	                        hdrlen = (hdrptr->hdrlen+2)<<2;
+	                else
+	                        hdrlen = ipv6_optlen(hdrptr);
+
+			currenthdr = hdrptr->nexthdr;
+	                ptr += hdrlen;
+			/* ptr is too large */
+	                if ( ptr > skb->len ) 
+				return 0;
+		}
+
+		/* currenthdr contains the protocol header */
+
+		dprintf("Packet protocol %hi ?= %s%hi.\n",
+				currenthdr, 
+				ip6info->invflags & IP6T_INV_PROTO ? "!":"",
+				ip6info->proto);
+
+		if (ip6info->proto == currenthdr) {
+			if(ip6info->invflags & IP6T_INV_PROTO) {
+				return 0;
+			}
+			return 1;
+		}
+
+		/*
+		 * Old extension header parser - WRONG IDEA! It always 
+		 * causes crash with an extension!
+ 		 * 	-- kisza
+		hdrptr = (u_int8_t *)((void *)ipv6 + 1);
 		do {
+			dprintf("Packet protocol currenthdr: %hi ?= %hi.\n",
+					currenthdr, ip6info->proto);
 			if (ip6info->proto == currenthdr) {
-				if(ip6info->invflags & IP6T_INV_PROTO)
+				if(ip6info->invflags & IP6T_INV_PROTO) {
+					dprintf("Packet protocol %hi does not "
+						"match %hi.%s\n",
+						currenthdr, ip6info->proto,
+						ip6info->invflags &
+						IP6T_INV_PROTO ? " (INV)":"");
 					return 0;
+				}
+				dprintf("Packet protocol %hi "
+					"matches %hi.%s\n",
+					currenthdr, ip6info->proto,
+					ip6info->invflags &
+					IP6T_INV_PROTO ? " (INV)":"");
 				return 1;
 			}
 			currenthdr = ip6_nexthdr(currenthdr, hdrptr);
 		} while(currenthdr);
-		if (!(ip6info->invflags & IP6T_INV_PROTO))
+		*/
+
+		/* We need match for the '-p all', too! */
+		if ((ip6info->proto != 0) &&
+			!(ip6info->invflags & IP6T_INV_PROTO))
 			return 0;
 	}
 	return 1;
@@ -359,7 +451,8 @@
 		IP_NF_ASSERT(e);
 		IP_NF_ASSERT(back);
 		(*pskb)->nfcache |= e->nfcache;
-		if (ip6_packet_match(ipv6, indev, outdev, &e->ipv6, offset)) {
+		if (ip6_packet_match(*pskb, ipv6, indev, outdev, 
+			&e->ipv6, offset)) {
 			struct ip6t_entry_target *t;
 
 			if (IP6T_MATCH_ITERATE(e, do_match,