From: bluhm <bluhm@openbsd.org>
Date: Thu, 14 Dec 2017 14:26:50 +0000 (+0000)
Subject: The pf code marks ICMP packets belonging to an TCP or UDP divert
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=17b8cf86171e182e0d461acd987663ede9d7d5e5;p=openbsd

The pf code marks ICMP packets belonging to an TCP or UDP divert
state as diverted.  This is necessary for IP input to accept the
packet as ours.  But it must not be used to match the ICMP packet
to a raw socket.  Clear the PF_TAG_DIVERTED mbuf pf flag for the
special ICMP and ICMP6 packets in icmp_input_if() and icmp6_input().

The m_tag_delete_chain() caused an inconsistent PF_TAG_DIVERTED
mbuf pf flag and PACKET_TAG_PF_DIVERT mbuf tag which triggered an
assert in rip_input().  Deleting all mbuf tags can have undesired
side effects and is not necessary anymore since icmp_reflect() calls
m_resethdr().  Do not touch the mbuf tags and adjust the mbuf pf
flags for the correct behavior of rip_input() and rip6_input().

reported by Chris Eidem, James Turner, vicviq, Scott Vanderbilt
OK mpi@
---

diff --git a/sys/netinet/ip_icmp.c b/sys/netinet/ip_icmp.c
index f218b6f428e..b71e8738da2 100644
--- a/sys/netinet/ip_icmp.c
+++ b/sys/netinet/ip_icmp.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ip_icmp.c,v 1.173 2017/10/18 17:01:14 bluhm Exp $	*/
+/*	$OpenBSD: ip_icmp.c,v 1.174 2017/12/14 14:26:50 bluhm Exp $	*/
 /*	$NetBSD: ip_icmp.c,v 1.19 1996/02/13 23:42:22 christos Exp $	*/
 
 /*
@@ -378,6 +378,12 @@ icmp_input_if(struct ifnet *ifp, struct mbuf **mp, int *offp, int proto, int af)
 #if NPF > 0
 	if (m->m_pkthdr.pf.flags & PF_TAG_DIVERTED) {
 		switch (icp->icmp_type) {
+		 /*
+		  * As pf_icmp_mapping() considers redirects belonging to a
+		  * diverted connection, we must include it here.
+		  */
+		case ICMP_REDIRECT:
+			/* FALLTHROUGH */
 		/*
 		 * These ICMP types map to other connections.  They must be
 		 * delivered to pr_ctlinput() also for diverted connections.
@@ -386,12 +392,11 @@ icmp_input_if(struct ifnet *ifp, struct mbuf **mp, int *offp, int proto, int af)
 		case ICMP_TIMXCEED:
 		case ICMP_PARAMPROB:
 		case ICMP_SOURCEQUENCH:
-			break;
-		 /*
-		  * Although pf_icmp_mapping() considers redirects belonging
-		  * to a diverted connection, we must process it here anyway.
-		  */
-		case ICMP_REDIRECT:
+			/*
+			 * Do not use the divert-to property of the TCP or UDP
+			 * rule when doing the PCB lookup for the raw socket.
+			 */
+			m->m_pkthdr.pf.flags &=~ PF_TAG_DIVERTED;
 			break;
 		default:
 			goto raw;
@@ -454,10 +459,6 @@ icmp_input_if(struct ifnet *ifp, struct mbuf **mp, int *offp, int proto, int af)
 			goto badcode;
 		code = PRC_QUENCH;
 	deliver:
-		/* Free packet atttributes */
-		if (m->m_flags & M_PKTHDR)
-			m_tag_delete_chain(m);
-
 		/*
 		 * Problem with datagram; advise higher level routines.
 		 */
@@ -585,10 +586,6 @@ reflect:
 		    &ip->ip_dst.s_addr, 1))
 			goto freeit;
 #endif
-		/* Free packet atttributes */
-		if (m->m_flags & M_PKTHDR)
-			m_tag_delete_chain(m);
-
 		icmpstat_inc(icps_reflect);
 		icmpstat_inc(icps_outhist + icp->icmp_type);
 		if (!icmp_reflect(m, &opts, NULL)) {
@@ -604,9 +601,6 @@ reflect:
 		struct sockaddr_in ssrc;
 		struct rtentry *newrt = NULL;
 
-		/* Free packet atttributes */
-		if (m->m_flags & M_PKTHDR)
-			m_tag_delete_chain(m);
 		if (icmp_rediraccept == 0 || ipforwarding == 1)
 			goto freeit;
 		if (code > 3)
diff --git a/sys/netinet6/icmp6.c b/sys/netinet6/icmp6.c
index 9567e5cc4fd..7db5b5c659a 100644
--- a/sys/netinet6/icmp6.c
+++ b/sys/netinet6/icmp6.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: icmp6.c,v 1.220 2017/11/03 14:28:57 florian Exp $	*/
+/*	$OpenBSD: icmp6.c,v 1.221 2017/12/14 14:26:50 bluhm Exp $	*/
 /*	$KAME: icmp6.c,v 1.217 2001/06/20 15:03:29 jinmei Exp $	*/
 
 /*
@@ -431,6 +431,11 @@ icmp6_input(struct mbuf **mp, int *offp, int proto, int af)
 		case ICMP6_PACKET_TOO_BIG:
 		case ICMP6_TIME_EXCEEDED:
 		case ICMP6_PARAM_PROB:
+			/*
+			 * Do not use the divert-to property of the TCP or UDP
+			 * rule when doing the PCB lookup for the raw socket.
+			 */
+			m->m_pkthdr.pf.flags &=~ PF_TAG_DIVERTED;
 			break;
 		default:
 			goto raw;