--sm4nu43k4a2Rpi4c
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Hello

I posted recently about the bug in NAT-T: The m_pullup call would change the
struct mbuf *m pointer in udp4_espinudp(), thus making the caller's value
invalid (and thus leading to a crash).

Here is an attempt to fix the problem, but it completely breaks NAT-T. 
It seems the m_pulldown() call corrupts the packet (UDP and ESP).

Could someone knowledgable with mbuf help? Why is my approach wrong?

-- 
Emmanuel Dreyfus
manu@netbsd.org

--sm4nu43k4a2Rpi4c
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="nat-t.diff"

--- udp_usrreq.c	2005-12-08 17:01:16.000000000 +0100
+++ udp_usrreq.c.pulldown	2005-12-08 15:37:19.000000000 +0100
@@ -402,5 +402,10 @@
 	dst.sin_port = uh->uh_dport;
 
-	n = udp4_realinput(&src, &dst, m, iphlen);
+	if ((n = udp4_realinput(&src, &dst, m, iphlen)) == -1) {
+		/* m was freed */
+		udpstat.udps_hdrops++;
+		return;
+	}
+		
 #ifdef INET6
 	if (IN_MULTICAST(ip->ip_dst.s_addr) || n == 0) {
@@ -803,9 +808,17 @@
 			struct sockaddr *sa = (struct sockaddr *)src;
 
-			if (udp4_espinudp(m, off, sa, inp->inp_socket) != 0) {
-				rcvcnt++;
+			switch (udp4_espinudp(m, off, sa, inp->inp_socket)) {
+			case -1:	/* error, m was freed */
+				rcvcnt = -1;
+				goto bad;
+				break;
+			case 1:		/* ESP over UDP, skip it */
+				rcvcnt++; /* XXX is that right? */
 				goto bad;
+				break;
+			case 0:		/* Plain UDP */
+			default:	/* unexpected */
+				break;
 			}
-
 			/* Normal UDP processing will take place */
 		}
@@ -1398,4 +1411,5 @@
  * 1 if the packet was processed
  * 0 if normal UDP processing should take place
+ * -1 an error occured and the mbuf was freeed
  */
 static int
@@ -1406,11 +1420,10 @@
 	struct socket *so;
 {
-	size_t len;
 	caddr_t data;
 	struct inpcb *inp;
 	size_t skip = 0;
-	size_t minlen;
 	size_t iphdrlen;
 	struct ip *ip;
+	struct mbuf *me;
 	struct mbuf *n;
 	struct m_tag *tag;
@@ -1419,24 +1432,18 @@
 
 	/*
-	 * Collapse the mbuf chain if the first mbuf is too short
-	 * The longest case is: UDP + non ESP marker + ESP
+	 * Make sure the non ESP marker and the ESP header 
+	 * are in contiguous memory. This operation avoids moving
+	 * mbuf m base and data up to off, so the calling function
+	 * will not use a garbled pointer.
 	 */
-	minlen = off + sizeof(u_int64_t) + sizeof(struct esp);
-	if (minlen > m->m_pkthdr.len)
-		minlen = m->m_pkthdr.len;
-
-	if (m->m_len < minlen) {
-		if ((m = m_pullup(m, minlen)) == NULL) {
-			printf("udp4_espinudp: m_pullup failed\n");
-			return 0;
-		}
+	me = m_pulldown(m, off, sizeof(u_int64_t) + sizeof(struct esp), NULL);
+	if (me == NULL) {
+		printf("udp4_espinudp: m_pulldown failed\n");
+		return -1;
 	}
-
-	len = m->m_len - off;
-	data = mtod(m, caddr_t) + off;
-	inp = sotoinpcb(so);
+	data = mtod(me, caddr_t);
 
 	/* Ignore keepalive packets */
-	if ((len == 1) && (data[0] == '\xff')) {
+	if ((me->m_len == 1) && (data[0] == '\xff')) {
 		return 1;
 	}
@@ -1447,8 +1454,9 @@
 	 * header to remove
 	 */
+	inp = sotoinpcb(so);
 	if (inp->inp_flags & INP_ESPINUDP) {
 		u_int32_t *st = (u_int32_t *)data;
 
-		if ((len <= sizeof(struct esp)) || (*st == 0))
+		if ((me->m_len <= sizeof(struct esp)) || (*st == 0))
 			return 0; /* Normal UDP processing */
 
@@ -1457,8 +1465,8 @@
 
 	if (inp->inp_flags & INP_ESPINUDP_NON_IKE) {
-		u_int32_t *st = (u_int32_t *)data;
+		u_int64_t *st = (u_int64_t *)data;
 
-		if ((len <= sizeof(u_int64_t) + sizeof(struct esp))
-		    || ((st[0] | st[1]) != 0))
+		if ((me->m_len <= sizeof(u_int64_t) + sizeof(struct esp))
+		    || (*st != 0))
 			return 0; /* Normal UDP processing */
 
@@ -1470,5 +1478,5 @@
 	 * order everywhere in IPSEC_NAT_T code.
 	 */
-	udphdr = (struct udphdr *)(data - skip);
+	udphdr = mtod(m, struct udphdr *);
 	sport = udphdr->uh_sport;
 	dport = udphdr->uh_dport;

--sm4nu43k4a2Rpi4c--