kern/128260: [ PATCH ] ipfw_divert damages IPv6 packets

[Dan Lukes]

>Number:         128260
>Category:       kern
>Synopsis:       [ PATCH ] ipfw_divert damages IPv6 packets
>Confidential:   no
>Severity:       critical
>Priority:       medium
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Tue Oct 21 04:50:00 UTC 2008
>Closed-Date:
>Last-Modified:
>Originator:     Dan Lukes
>Release:        FreeBSD 6.4-PRERELEASE i386
>Organization:
Obludarium
>Environment:
FOUDED and PATCHED on

System: FreeBSD 6.4-PRERELEASE: Thu Sep 11 23:32:18 CEST 2008 i386
sys/netinet/ip_fw_pfil.c,v 1.19.2.4 2008/04/25 10:29:26

BUT THE SAME CODE (thus problem) seems to be part of RELENG-7 and HEAD

>Description:

In the ip_input() the code swap ip_len and ip_off fields to host order.
The ipfw_divert will revert the order back, but, unfortunatelly,
not only for affected IPv4 packet, but also for IPv6 packet which has
another header structure which can;t be interpreted using struct ip.

Resulting IPv6 packet has damaged header because swapping of
ramdom (from the view of IPv6 header structure) bytes.

Also, test for IPv4 fragmentation may have produce false positives when
applied to IPv6 packed with further undesired effects.

As the bug cause divert/tee of IPv6 to be unusable I'm marking it as critical. 

>How-To-Repeat:
	Tee a IPv6 traffic to divert port, read packet from them, 
compare original headers with headers after diverting
>Fix:

We need to apply IPv4 specific processing to IPv4 packets only
which is exactly what patch do - only "if (ip->ip_v == IPVERSION)"
check added around the original code, nothing more.

It would be very nice to have IPv6 tee/divert working
in forthcomming 6.4/7.1 releases, if possible. It seems to be clear
problem and simple patch (I hope) ...

Dan


--- sys/netinet/ip_fw_pfil.c.orig	2008-10-21 05:33:37.000000000 +0200
+++ sys/netinet/ip_fw_pfil.c	2008-10-21 05:33:37.000000000 +0200
@@ -365,32 +365,34 @@
 	 * in its original form.
 	 */
 	ip = mtod(clone, struct ip *);
-	if (!tee && ip->ip_off & (IP_MF | IP_OFFMASK)) {
+	if (ip->ip_v == IPVERSION) {
+		if (!tee && ip->ip_off & (IP_MF | IP_OFFMASK)) {
 
-		/* Reassemble packet. */
-		reass = ip_reass(clone);
+			/* Reassemble packet. */
+			reass = ip_reass(clone);
 
-		/*
-		 * IP header checksum fixup after reassembly and leave header
-		 * in network byte order.
-		 */
-		if (reass != NULL) {
-			ip = mtod(reass, struct ip *);
-			hlen = ip->ip_hl << 2;
+			/*
+			 * IP header checksum fixup after reassembly and leave header
+			 * in network byte order.
+			 */
+			if (reass != NULL) {
+				ip = mtod(reass, struct ip *);
+				hlen = ip->ip_hl << 2;
+				ip->ip_len = htons(ip->ip_len);
+				ip->ip_off = htons(ip->ip_off);
+				ip->ip_sum = 0;
+				if (hlen == sizeof(struct ip))
+					ip->ip_sum = in_cksum_hdr(ip);
+				else
+					ip->ip_sum = in_cksum(reass, hlen);
+				clone = reass;
+			} else
+				clone = NULL;
+		} else {
+			/* Convert header to network byte order. */
 			ip->ip_len = htons(ip->ip_len);
 			ip->ip_off = htons(ip->ip_off);
-			ip->ip_sum = 0;
-			if (hlen == sizeof(struct ip))
-				ip->ip_sum = in_cksum_hdr(ip);
-			else
-				ip->ip_sum = in_cksum(reass, hlen);
-			clone = reass;
-		} else
-			clone = NULL;
-	} else {
-		/* Convert header to network byte order. */
-		ip->ip_len = htons(ip->ip_len);
-		ip->ip_off = htons(ip->ip_off);
+		}
 	}
 
 	/* Do the dirty job... */


>Release-Note:
>Audit-Trail:
>Unformatted: