kern/56052: NPF: block return... processes the return packet again

[kardel%netbsd.org@localhost]

>Number:         56052
>Category:       kern
>Synopsis:       NPF: block return... processes the return packet again
>Confidential:   no
>Severity:       serious
>Priority:       high
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Fri Mar 12 09:00:00 +0000 2021
>Originator:     Frank Kardel
>Release:        NetBSD 9.99.81
>Organization:
	
>Environment:
	
	
System: NetBSD gateway 9.99.81 NetBSD 9.99.81 (GATEWAY) #9: Fri Mar 12 07:41:46 UTC 2021 kardel@gateway:/src/NetBSD/cur/src/obj.amd64/sys/arch/amd64/compile/GATEWAY amd64
Architecture: x86_64
Machine: amd64
>Description:
	A ruleset like
group "name" on <interface> {
	pass in final ...
	pass in final ...
        block return in final all apply "log"
}

	will not send out any TCP-RST or ICMP(,6} unreachable pakets as the
	block-return packets pass again through the NPF filter engine on
	their way out. block-return packets are sent via ip{,6}_output() and
	icmp{,6}_error().
	This can be observed via the npflogX interface.

	block-return packets must skip outgoing packet rules to avoid
	to add workaround rules like

# allow for block return packets
pass out final proto tcp all apply "log"
pass out final proto icmp all apply "log"

	before the block-return rule. this workaround allows also unwanted
	communication. more precise rules require pcap-filter
	rules and may still be incomplete.

	as the workarounds are hard to manage and hard to explain to
	users NPF needs to be fixed to work like other packet filters
	so that is does not examine its own block-return packets.

	see Fix: below

	I will commit the fix if no objections arise.

>How-To-Repeat:
	Have a ruleset like
group "name" on <interface> {
        pass in final ...
        pass in final ...
        block return in final all apply "log"
}

	send packets that are not matched by the pass in rules. observe
	via npflogX that the block-return packet pass the filter rules
	in out direct and they get finally dropped.

>Fix:
	to fix the iss the block-return packets need to be marked
	with the PACKET_TAG_NPF tag so the NPF filter engine will
	automatically pass these packets without rule processing.
	there is an issue in ip_icmp.c (IPv4 ICMP) in icmp_error.
	as it create a new packet the tags must be transferred. the
	the current code only knows about PACKAGE_TAG_PF. This needs to
	be extended.

	see the following patch for fixing the NPF block-return
	functionality.

Index: sys/net/npf/npf.h
===================================================================
RCS file: /cvsroot/src/sys/net/npf/npf.h,v
retrieving revision 1.63
diff -u -r1.63 npf.h
--- sys/net/npf/npf.h	30 May 2020 14:16:56 -0000	1.63
+++ sys/net/npf/npf.h	12 Mar 2021 08:00:18 -0000
@@ -121,7 +121,9 @@
 void *		nbuf_ensure_writable(nbuf_t *, size_t);
 
 bool		nbuf_cksum_barrier(nbuf_t *, int);
+int		mbuf_add_tag(nbuf_t *, struct mbuf *, uint32_t);
 int		nbuf_add_tag(nbuf_t *, uint32_t);
+int		mbuf_find_tag(nbuf_t *, struct mbuf *, uint32_t *);
 int		nbuf_find_tag(nbuf_t *, uint32_t *);
 
 /*
Index: sys/net/npf/npf_mbuf.c
===================================================================
RCS file: /cvsroot/src/sys/net/npf/npf_mbuf.c,v
retrieving revision 1.24
diff -u -r1.24 npf_mbuf.c
--- sys/net/npf/npf_mbuf.c	30 May 2020 14:16:56 -0000	1.24
+++ sys/net/npf/npf_mbuf.c	12 Mar 2021 08:00:18 -0000
@@ -297,14 +297,13 @@
 }
 
 /*
- * nbuf_add_tag: associate a tag with the network buffer.
+ * mbuf_add_tag: associate a tag with the network buffer.
  *
  * => Returns 0 on success or error number on failure.
  */
 int
-nbuf_add_tag(nbuf_t *nbuf, uint32_t val)
+mbuf_add_tag(nbuf_t *nbuf, struct mbuf *m, uint32_t val)
 {
-	struct mbuf *m = nbuf->nb_mbuf0;
 #ifdef _KERNEL
 	struct m_tag *mt;
 	uint32_t *dat;
@@ -328,14 +327,25 @@
 }
 
 /*
- * nbuf_find_tag: find a tag associated with a network buffer.
+ * nbuf_add_tag: associate a tag with the network buffer.
  *
  * => Returns 0 on success or error number on failure.
  */
 int
-nbuf_find_tag(nbuf_t *nbuf, uint32_t *val)
+nbuf_add_tag(nbuf_t *nbuf, uint32_t val)
 {
 	struct mbuf *m = nbuf->nb_mbuf0;
+	return mbuf_add_tag(nbuf, m, val);
+}
+
+/*
+ * mbuf_find_tag: find a tag associated with a network buffer.
+ *
+ * => Returns 0 on success or error number on failure.
+ */
+int
+mbuf_find_tag(nbuf_t *nbuf, struct mbuf *m, uint32_t *val)
+{
 #ifdef _KERNEL
 	struct m_tag *mt;
 
@@ -354,3 +364,15 @@
 	return nbuf->nb_mops->get_tag(m, val);
 #endif
 }
+
+/*
+ * nbuf_find_tag: find a tag associated with a network buffer.
+ *
+ * => Returns 0 on success or error number on failure.
+ */
+int
+nbuf_find_tag(nbuf_t *nbuf, uint32_t *val)
+{
+	struct mbuf *m = nbuf->nb_mbuf0;
+	return mbuf_find_tag(nbuf, m, val);
+}
Index: sys/net/npf/npf_sendpkt.c
===================================================================
RCS file: /cvsroot/src/sys/net/npf/npf_sendpkt.c,v
retrieving revision 1.22
diff -u -r1.22 npf_sendpkt.c
--- sys/net/npf/npf_sendpkt.c	30 May 2020 14:16:56 -0000	1.22
+++ sys/net/npf/npf_sendpkt.c	12 Mar 2021 08:00:18 -0000
@@ -197,6 +197,9 @@
 		}
 	}
 
+	/* don't look at our generated reject packets going out */
+	(void)mbuf_add_tag(npc->npc_nbuf, m, NPF_NTAG_PASS);
+
 	/* Pass to IP layer. */
 	if (npf_iscached(npc, NPC_IP4)) {
 		return ip_output(m, NULL, NULL, IP_FORWARDING, NULL, NULL);
@@ -215,6 +218,9 @@
 {
 	struct mbuf *m = nbuf_head_mbuf(npc->npc_nbuf);
 
+	/* don't look at our generated reject packets going out */
+	(void)mbuf_add_tag(npc->npc_nbuf, m, NPF_NTAG_PASS);
+
 	if (npf_iscached(npc, NPC_IP4)) {
 		icmp_error(m, ICMP_UNREACH, ICMP_UNREACH_ADMIN_PROHIBIT, 0, 0);
 		return 0;
Index: sys/netinet/ip_icmp.c
===================================================================
RCS file: /cvsroot/src/sys/netinet/ip_icmp.c,v
retrieving revision 1.177
diff -u -r1.177 ip_icmp.c
--- sys/netinet/ip_icmp.c	22 Dec 2018 14:28:57 -0000	1.177
+++ sys/netinet/ip_icmp.c	12 Mar 2021 08:00:19 -0000
@@ -165,6 +165,10 @@
 static int icmp_redirtimeout = 600;
 static struct rttimer_queue *icmp_redirect_timeout_q = NULL;
 
+/* packet filter m_tag helpers */
+static void move_mtag(int, struct mbuf*, struct mbuf *);
+static void clear_non_pf_mtags(struct mbuf *);
+
 /* Protect mtudisc and redirect stuff */
 static kmutex_t icmp_mtx __cacheline_aligned;
 
@@ -237,6 +241,22 @@
 }
 
 /*
+ * Move a specified m_tag from soure mbuf to target mbuf
+ */
+static void
+move_mtag(int type, struct mbuf *s, struct mbuf *t)
+{
+	struct m_tag *mtag;
+
+	mtag = m_tag_find(s, type);
+
+	if (mtag != NULL) {
+		m_tag_unlink(s, mtag);
+		m_tag_prepend(t, mtag);
+	}
+}
+
+/*
  * Generate an error packet of type error in response to a bad IP packet. 'n'
  * contains this packet. We create 'm' and send it.
  * 
@@ -254,7 +274,6 @@
 	const unsigned oiphlen = oip->ip_hl << 2;
 	struct icmp *icp;
 	struct mbuf *m;
-	struct m_tag *mtag;
 	unsigned datalen, mblen;
 	int totlen;
 
@@ -381,12 +400,10 @@
 	nip->ip_p = IPPROTO_ICMP;
 	nip->ip_src = oip->ip_src;
 	nip->ip_dst = oip->ip_dst;
-	/* move PF m_tag to new packet, if it exists */
-	mtag = m_tag_find(n, PACKET_TAG_PF);
-	if (mtag != NULL) {
-		m_tag_unlink(n, mtag);
-		m_tag_prepend(m, mtag);
-	}
+
+	/* move packet filter tags */
+	move_mtag(PACKET_TAG_NPF, n, m);
+	move_mtag(PACKET_TAG_PF, n, m);
 
 	icmp_reflect(m);
 
@@ -699,6 +716,39 @@
 }
 
 /*
+ * clear all non packet filter mtags
+ */
+static void
+clear_non_pf_mtags(struct mbuf *m)
+{
+	struct m_tag * mtag_npf = NULL;
+	struct m_tag * mtag_pf = NULL;
+
+	mtag_npf = m_tag_find(m, PACKET_TAG_NPF);
+	if (mtag_npf != NULL) {
+		m_tag_unlink(m, mtag_npf);
+	}
+
+	mtag_pf = m_tag_find(m, PACKET_TAG_PF);
+	if (mtag_pf != NULL) {
+		m_tag_unlink(m, mtag_pf);
+	}
+
+	/* remove all other tags */
+	m_tag_delete_chain(m);
+
+	/* recover tag for PF */
+	if (mtag_pf != NULL) {
+		m_tag_prepend(m, mtag_pf);
+	}
+
+	/* recover tag for NPF */
+	if (mtag_npf != NULL) {
+		m_tag_prepend(m, mtag_npf);
+	}
+}
+
+/*
  * Reflect the ip packet back to the source
  */
 void
@@ -919,7 +969,10 @@
 		memmove(ip + 1, (char *)ip + optlen,
 		    (unsigned)(m->m_len - sizeof(struct ip)));
 	}
-	m_tag_delete_chain(m);
+
+	/* remove all non packet filter mtags */
+	clear_non_pf_mtags(m);
+
 	m->m_flags &= ~(M_BCAST|M_MCAST);
 
 	/*

>Unformatted: