kern/47886: IPSEC_NAT_T enabled kernels may access outdated pointers and pass ESP data to UPD-sockets

[Wolfgang.Stukenbrock%nagler-company.com@localhost]

>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:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue Jun 04 14:05:00 +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

>Unformatted: