NetBSD Problem Report #47886

From Wolfgang.Stukenbrock@nagler-company.com  Tue Jun  4 14:00:25 2013
Return-Path: <Wolfgang.Stukenbrock@nagler-company.com>
Received: from mail.netbsd.org (mail.netbsd.org [149.20.53.66])
	(using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "Postmaster NetBSD.org" (verified OK))
	by mollari.NetBSD.org (Postfix) with ESMTPS id F3FDD71B05
	for <gnats-bugs@gnats.NetBSD.org>; Tue,  4 Jun 2013 14:00:24 +0000 (UTC)
Message-Id: <20130604140013.2CD274EAA37@s012.nagler-company.com>
Date: Tue,  4 Jun 2013 16:00:13 +0200 (CEST)
From: Wolfgang.Stukenbrock@nagler-company.com
Reply-To: Wolfgang.Stukenbrock@nagler-company.com
To: gnats-bugs@gnats.NetBSD.org
Subject: IPSEC_NAT_T enabled kernels may access outdated pointers and pass ESP data to UPD-sockets
X-Send-Pr-Version: 3.95

>Number:         47886
>Category:       kern
>Synopsis:       IPSEC_NAT_T enabled kernels may access outdated pointers and pass ESP data to UPD-sockets
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue Jun 04 14:05:00 +0000 2013
>Closed-Date:    Mon Oct 07 03:28:45 +0000 2013
>Last-Modified:  Mon Oct 07 03:28:45 +0000 2013
>Originator:     Dr. Wolfgang Stukenbrock
>Release:        NetBSD 6.1
>Organization:
Dr. Nagler & Company GmbH
>Environment:


System: NetBSD s012 4.0 NetBSD 4.0 (NSW-S012) #12: Tue Jun 19 11:15:19 CEST 2012 ncadmin@s012:/usr/src/sys/arch/amd64/compile/NSW-S012 amd64
Architecture: x86_64
Machine: amd64
>Description:
	If IPSEC_NAT_T is enabled, it may happen that the mbuf chain gets changed by m_pullup().
	So it is no good idea to use a pointer to the IP-header that is based on the old one.

	A second strange thing is, that we strip off the UPD header during processing and set the type to ESP, but still
	processing this packet as UPD packet, if we fail to duplicate the packet or to allocate a packet tag.
	I'm not shure what can happen in udp6_realinput() and/or udp4_sendup() when called with ESP-packets.
	As far as I can see, this may inject any data into the UPD-stack while bypassing ipfilter rules.

	Also returning the 1 byte keep-alive packets to the UPD processing may end up in passing it to IPv6
	UPD sockets, if it has been send to a multicast address.
	Same may happen with "normal" NAT-T packets send to Multicast addresses.
	(May be some kind of DOS attack when running without ipfilter setup ???)
>How-To-Repeat:
	Found while validating private patches agains 6.1 release while preparing upgrade to 6.1.
>Fix:
	The following patch to src/sys/netinet/udp_usrreq.c will fix the problem:
	- avoid any UDP processing of NAT-T-keep-alive packets
	- avoid any UDP processing after stripping of the UPD header
	  (this makes the duplication of the packet obsolete and speeds up processing.)
	- recalculate the pointer to the IP header if the UPD packets is returned to upd_input() for futher processing.

--- udp_usrreq.c	2013/06/04 12:07:05	1.1
+++ udp_usrreq.c	2013/06/04 13:18:57
@@ -412,6 +412,11 @@
 		UDP_STATINC(UDP_STAT_HDROPS);
 		return;
 	}
+#ifdef IPSEC_NAT_T
+	if (m == NULL) return; /* packet has been processed by ESP stuff - e.g. dropped NAT-T-keep-alive-packet ... */
+// need to refresch pointer to ip-header, mbuf may have changed after pullup ...
+	ip = mtod(m, struct ip *);
+#endif
 #ifdef INET6
 	if (IN_MULTICAST(ip->ip_dst.s_addr) || n == 0) {
 		struct sockaddr_in6 src6, dst6;
@@ -1493,7 +1498,6 @@
 	size_t minlen;
 	size_t iphdrlen;
 	struct ip *ip;
-	struct mbuf *n;
 	struct m_tag *tag;
 	struct udphdr *udphdr;
 	u_int16_t sport, dport;
@@ -1521,6 +1525,8 @@

 	/* Ignore keepalive packets */
 	if ((len == 1) && (*(unsigned char *)data == 0xff)) {
+		m_free(m);
+		*mp = NULL; /* avoid any further processiong by caller ... */
 		return 1;
 	}

@@ -1578,16 +1584,7 @@
 	ip = mtod(m, struct ip *);
 	ip->ip_len = htons(ntohs(ip->ip_len) - skip);
 	ip->ip_p = IPPROTO_ESP;
-
-	/*
-	 * Copy the mbuf to avoid multiple free, as both
-	 * esp4_input (which we call) and udp_input (which
-	 * called us) free the mbuf.
-	 */
-	if ((n = m_dup(m, 0, M_COPYALL, M_DONTWAIT)) == NULL) {
-		printf("udp4_espinudp: m_dup failed\n");
-		return 0;
-	}
+// remark: we have modified the packet - it is now ESP, so we should not return to UDP processing ... 

 	/*
 	 * Add a PACKET_TAG_IPSEC_NAT_T_PORT tag to remember
@@ -1598,20 +1595,21 @@
 	if ((tag = m_tag_get(PACKET_TAG_IPSEC_NAT_T_PORTS,
 	    sizeof(sport) + sizeof(dport), M_DONTWAIT)) == NULL) {
 		printf("udp4_espinudp: m_tag_get failed\n");
-		m_freem(n);
-		return 0;
+		m_freem(m); /* must free mbuf when returning -1 ... */
+		return -1;
 	}
 	((u_int16_t *)(tag + 1))[0] = sport;
 	((u_int16_t *)(tag + 1))[1] = dport;
-	m_tag_prepend(n, tag);
+	m_tag_prepend(m, tag);

 #ifdef FAST_IPSEC
-	ipsec4_common_input(n, iphdrlen, IPPROTO_ESP);
+	ipsec4_common_input(m, iphdrlen, IPPROTO_ESP);
 #else
-	esp4_input(n, iphdrlen);
+	esp4_input(m, iphdrlen);
 #endif

 	/* We handled it, it shouldn't be handled by UDP */
+	*mp = NULL; /* avoid free by caller ... */
 	return 1;
 }
 #endif

>Release-Note:

>Audit-Trail:
From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/47886 CVS commit: src/sys
Date: Tue, 4 Jun 2013 18:47:37 -0400

 Module Name:	src
 Committed By:	christos
 Date:		Tue Jun  4 22:47:37 UTC 2013

 Modified Files:
 	src/sys/netinet: ip_output.c udp_usrreq.c
 	src/sys/netipsec: files.netipsec ipsec.c ipsec.h ipsec_input.c
 	    ipsec_output.c key.c key.h keydb.h xform_ah.c xform_esp.c
 	    xform_ipcomp.c

 Log Message:
 PR/47886: Dr. Wolfgang Stukenbrock: IPSEC_NAT_T enabled kernels may access
 outdated pointers and pass ESP data to UPD-sockets.
 While here, simplify the code and remove the IPSEC_NAT_T option; always
 compile nat-traversal in so that it does not bitrot.


 To generate a diff of this commit:
 cvs rdiff -u -r1.218 -r1.219 src/sys/netinet/ip_output.c
 cvs rdiff -u -r1.187 -r1.188 src/sys/netinet/udp_usrreq.c
 cvs rdiff -u -r1.10 -r1.11 src/sys/netipsec/files.netipsec
 cvs rdiff -u -r1.57 -r1.58 src/sys/netipsec/ipsec.c
 cvs rdiff -u -r1.31 -r1.32 src/sys/netipsec/ipsec.h
 cvs rdiff -u -r1.29 -r1.30 src/sys/netipsec/ipsec_input.c \
     src/sys/netipsec/xform_ipcomp.c
 cvs rdiff -u -r1.38 -r1.39 src/sys/netipsec/ipsec_output.c \
     src/sys/netipsec/xform_ah.c
 cvs rdiff -u -r1.79 -r1.80 src/sys/netipsec/key.c
 cvs rdiff -u -r1.11 -r1.12 src/sys/netipsec/key.h
 cvs rdiff -u -r1.12 -r1.13 src/sys/netipsec/keydb.h
 cvs rdiff -u -r1.41 -r1.42 src/sys/netipsec/xform_esp.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->closed
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Mon, 07 Oct 2013 03:28:45 +0000
State-Changed-Why:
Christos committed it (and his commit looks too invasive for the
stable branches)


>Unformatted:

Home	PR Database Search