Pull up following revision(s) (requested by maxv in ticket #817): netbsd-8
authormartin <martin@NetBSD.org>
Wed, 09 May 2018 15:35:37 +0000
branchnetbsd-8
changeset 318925 03e26d54e1b1
parent 318924 acd23d109223
child 318926 3f37d373199a
Pull up following revision(s) (requested by maxv in ticket #817): sys/net/npf/npf_inet.c: revision 1.38-1.44 sys/net/npf/npf_handler.c: revision 1.38-1.39 sys/net/npf/npf_alg_icmp.c: revision 1.26 sys/net/npf/npf.h: revision 1.56 sys/net/npf/npf_sendpkt.c: revision 1.17-1.18 Declare NPC_FMTERR, and use it to kick malformed packets. Several sanity checks are added in IPv6; after we see the first IPPROTO_FRAGMENT header, we are allowed to fail to advance, otherwise we kick the packet. Sent on tech-net@ a few days ago, no response, but I'm committing it now anyway. Switch nptr to uint8_t, and use nbuf_ensure_contig. Makes us use fewer magic values. Remove dead branches, 'npc' can't be NULL (and it is dereferenced earlier). Fix two consecutive mistakes. The first mistake was npf_inet.c rev1.37: "Don't reassemble ipv6 fragments, instead treat the first fragment as a regular packet (subject to filtering rules), and pass subsequent fragments in the same group unconditionally." Doing this was entirely wrong, because then a packet just had to push the L4 payload in a secondary fragment, and NPF wouldn't apply rules on it - meaning any IPv6 packet could bypass >=L4 filtering. This mistake was supposed to be a fix for the second mistake. The second mistake was that ip6_reass_packet (in npf_reassembly) was getting called with npc->npc_hlen. But npc_hlen pointed to the last encountered header in the IPv6 chain, which was not necessarily the fragment header. So ip6_reass_packet was given garbage, and would fail, resulting in the packet getting kicked. So basically IPv6 was broken by NPF. The first mistake is reverted, and the second one is fixed by doing: - hlen = sizeof(struct ip6_frag); + hlen = 0; Now the iteration stops on the fragment header, and the call to ip6_reass_packet is valid. My npf_inet.c rev1.38 is partially reverted: we don't need to worry about failing properly to advance; once the packet is reassembled npf_cache_ip gets called again, and this time the whole chain should be there. Tested with a simple UDPv6 server - send a 3000-byte-sized buffer, the packet gets correctly reassembled by NPF now. Mmh, put back the RFC6946 check (about dummy fragments), otherwise NPF is not happy in npf_reassembly, because NPC_IPFRAG is again returned after the packet was reassembled. I'm wondering whether it would not be better to just remove the fragment header in frag6_input directly. Fix the "return-rst" rule on IPv6 packets. The scopes needed to be set on the addresses before invoking ip6_output, because ip6_output needs them. The reason they are not here already is because pfil_run_hooks (in ip6_input) is called _before_ the kernel initializes the scopes. Until now ip6_output was always failing, and the IPv6-TCP-RST packet was never actually sent. Perhaps it would be better to have the kernel initialize the scopes before invoking pfil_run_hooks, but several things will need to be fixed in several places. Tested with a simple TCPv6 server. Until now the client would block waiting for an answer that never came; now it receives an RST right away and closes the connection, as expected. I believe that the same problem exists in the "return-icmp" rules, but I can't investigate this right now (some problems with wireshark). Fix the IPv6 payload computation in npf_tcpsaw. It was incorrect, and this caused the "return-rst" rules to send back an RST with the wrong ACK when the received SYN had an IPv6 option. Set the scopes before calling icmp6_error(). This fixes a bug similar to the one I fixed in rev1.17: since the scopes were not set the packet was never actually sent. Tested with wireshark, now the ICMPv6 reply is correctly sent, as expected. Don't read the L4 payload after IPPROTO_AH when handling IPv6 packets. AH must be considered as the payload, otherwise a block all pass in proto ah from any pass out proto ah from any configuration will actually block everything, because NPF checks the protocol against the one found after AH, and not AH itself. In addition it may have been a problem for stateful connections; an AH packet sent by an attacker with an incorrect authentication and a correct TCP/UDP/whatever payload from an active connection could manage to change NPF's FSM state, which would perhaps have altered the legitimate connection with the authenticated remote IPsec host. Note that IPv4 already doesn't go beyond AH, which is the correct behavior. Add XXX (we don't handle IPv6 Jumbograms), and whitespace.
sys/net/npf/npf.h
sys/net/npf/npf_alg_icmp.c
sys/net/npf/npf_handler.c
sys/net/npf/npf_inet.c
sys/net/npf/npf_sendpkt.c
--- a/sys/net/npf/npf.h	Wed May 09 15:28:44 2018 +0000
+++ b/sys/net/npf/npf.h	Wed May 09 15:35:37 2018 +0000
@@ -1,4 +1,4 @@
-/*	$NetBSD: npf.h,v 1.54.6.1 2018/04/04 16:40:42 martin Exp $	*/
+/*	$NetBSD: npf.h,v 1.54.6.2 2018/05/09 15:35:37 martin Exp $	*/
 
 /*-
  * Copyright (c) 2009-2014 The NetBSD Foundation, Inc.
@@ -143,6 +143,8 @@
 
 #define	NPC_ALG_EXEC	0x100	/* ALG execution. */
 
+#define	NPC_FMTERR	0x200	/* Format error. */
+
 #define	NPC_IP46	(NPC_IP4|NPC_IP6)
 
 typedef struct {
--- a/sys/net/npf/npf_alg_icmp.c	Wed May 09 15:28:44 2018 +0000
+++ b/sys/net/npf/npf_alg_icmp.c	Wed May 09 15:35:37 2018 +0000
@@ -1,4 +1,4 @@
-/*	$NetBSD: npf_alg_icmp.c,v 1.24 2016/12/26 23:05:06 christos Exp $	*/
+/*	$NetBSD: npf_alg_icmp.c,v 1.24.8.1 2018/05/09 15:35:37 martin Exp $	*/
 
 /*-
  * Copyright (c) 2010 The NetBSD Foundation, Inc.
@@ -35,7 +35,7 @@
 
 #ifdef _KERNEL
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: npf_alg_icmp.c,v 1.24 2016/12/26 23:05:06 christos Exp $");
+__KERNEL_RCSID(0, "$NetBSD: npf_alg_icmp.c,v 1.24.8.1 2018/05/09 15:35:37 martin Exp $");
 
 #include <sys/param.h>
 #include <sys/module.h>
@@ -135,9 +135,6 @@
 	case ICMP_REDIRECT:
 	case ICMP_TIMXCEED:
 	case ICMP_PARAMPROB:
-		if (npc == NULL) {
-			return false;
-		}
 		/* Should contain original IP header. */
 		if (!nbuf_advance(nbuf, offsetof(struct icmp, icmp_ip), 0)) {
 			return false;
@@ -175,9 +172,6 @@
 	case ICMP6_PACKET_TOO_BIG:
 	case ICMP6_TIME_EXCEEDED:
 	case ICMP6_PARAM_PROB:
-		if (npc == NULL) {
-			return false;
-		}
 		/* Should contain original IP header. */
 		if (!nbuf_advance(nbuf, sizeof(struct icmp6_hdr), 0)) {
 			return false;
--- a/sys/net/npf/npf_handler.c	Wed May 09 15:28:44 2018 +0000
+++ b/sys/net/npf/npf_handler.c	Wed May 09 15:35:37 2018 +0000
@@ -1,4 +1,4 @@
-/*	$NetBSD: npf_handler.c,v 1.37 2017/02/19 20:27:22 christos Exp $	*/
+/*	$NetBSD: npf_handler.c,v 1.37.6.1 2018/05/09 15:35:37 martin Exp $	*/
 
 /*-
  * Copyright (c) 2009-2013 The NetBSD Foundation, Inc.
@@ -37,7 +37,7 @@
 
 #ifdef _KERNEL
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: npf_handler.c,v 1.37 2017/02/19 20:27:22 christos Exp $");
+__KERNEL_RCSID(0, "$NetBSD: npf_handler.c,v 1.37.6.1 2018/05/09 15:35:37 martin Exp $");
 
 #include <sys/types.h>
 #include <sys/param.h>
@@ -109,7 +109,7 @@
 	nbuf_init(npf, nbuf, *mp, nbuf->nb_ifp);
 	npc->npc_info = 0;
 
-	if (npf_cache_all(npc) & NPC_IPFRAG) {
+	if (npf_cache_all(npc) & (NPC_IPFRAG|NPC_FMTERR)) {
 		return EINVAL;
 	}
 	npf_stats_inc(npf, NPF_STAT_REASSEMBLY);
@@ -154,18 +154,19 @@
 	error = 0;
 	rp = NULL;
 
-	/* Cache everything.  Determine whether it is an IP fragment. */
+	/* Cache everything. */
 	flags = npf_cache_all(&npc);
+
+	/* If error on the format, leave quickly. */
+	if (flags & NPC_FMTERR) {
+		error = EINVAL;
+		goto fastout;
+	}
+
+	/* Determine whether it is an IP fragment. */
 	if (__predict_false(flags & NPC_IPFRAG)) {
 		/*
-		 * We pass IPv6 fragments unconditionally
-		 * The first IPv6 fragment is not marked as such
-		 * and passes through the filter
-		 */
-		if (flags & NPC_IP6)
-			return 0;
-		/*
-		 * Pass to IPv4 reassembly mechanism.
+		 * Pass to IPv4/IPv6 reassembly mechanism.
 		 */
 		error = npf_reassembly(npf, &npc, mp);
 		if (error) {
@@ -308,6 +309,7 @@
 		error = ENETUNREACH;
 	}
 
+fastout:
 	if (*mp) {
 		/* Free the mbuf chain. */
 		m_freem(*mp);
--- a/sys/net/npf/npf_inet.c	Wed May 09 15:28:44 2018 +0000
+++ b/sys/net/npf/npf_inet.c	Wed May 09 15:35:37 2018 +0000
@@ -1,4 +1,4 @@
-/*	$NetBSD: npf_inet.c,v 1.37 2017/02/19 20:27:22 christos Exp $	*/
+/*	$NetBSD: npf_inet.c,v 1.37.6.1 2018/05/09 15:35:37 martin Exp $	*/
 
 /*-
  * Copyright (c) 2009-2014 The NetBSD Foundation, Inc.
@@ -40,7 +40,7 @@
 
 #ifdef _KERNEL
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: npf_inet.c,v 1.37 2017/02/19 20:27:22 christos Exp $");
+__KERNEL_RCSID(0, "$NetBSD: npf_inet.c,v 1.37.6.1 2018/05/09 15:35:37 martin Exp $");
 
 #include <sys/param.h>
 #include <sys/types.h>
@@ -215,7 +215,8 @@
 		return ntohs(ip->ip_len) - npc->npc_hlen - thlen;
 	} else if (npf_iscached(npc, NPC_IP6)) {
 		const struct ip6_hdr *ip6 = npc->npc_ip.v6;
-		return ntohs(ip6->ip6_plen) - thlen;
+		return ntohs(ip6->ip6_plen) -
+		    (npc->npc_hlen - sizeof(*ip6)) - thlen;
 	}
 	return 0;
 }
@@ -229,7 +230,7 @@
 	nbuf_t *nbuf = npc->npc_nbuf;
 	const struct tcphdr *th = npc->npc_l4.tcp;
 	int topts_len, step;
-	void *nptr;
+	uint8_t *nptr;
 	uint8_t val;
 	bool ok;
 
@@ -252,7 +253,7 @@
 		ok = false;
 		goto done;
 	}
-	val = *(uint8_t *)nptr;
+	val = *nptr;
 
 	switch (val) {
 	case TCPOPT_EOL:
@@ -264,43 +265,43 @@
 		step = 1;
 		break;
 	case TCPOPT_MAXSEG:
-		if ((nptr = nbuf_advance(nbuf, 2, 2)) == NULL) {
+		if ((nptr = nbuf_ensure_contig(nbuf, TCPOLEN_MAXSEG)) == NULL) {
 			ok = false;
 			goto done;
 		}
 		if (mss) {
 			if (*mss) {
-				memcpy(nptr, mss, sizeof(uint16_t));
+				memcpy(nptr + 2, mss, sizeof(uint16_t));
 			} else {
-				memcpy(mss, nptr, sizeof(uint16_t));
+				memcpy(mss, nptr + 2, sizeof(uint16_t));
 			}
 		}
 		topts_len -= TCPOLEN_MAXSEG;
-		step = 2;
+		step = TCPOLEN_MAXSEG;
 		break;
 	case TCPOPT_WINDOW:
 		/* TCP Window Scaling (RFC 1323). */
-		if ((nptr = nbuf_advance(nbuf, 2, 1)) == NULL) {
+		if ((nptr = nbuf_ensure_contig(nbuf, TCPOLEN_WINDOW)) == NULL) {
 			ok = false;
 			goto done;
 		}
-		val = *(uint8_t *)nptr;
+		val = *(nptr + 2);
 		*wscale = (val > TCP_MAX_WINSHIFT) ? TCP_MAX_WINSHIFT : val;
 		topts_len -= TCPOLEN_WINDOW;
-		step = 1;
+		step = TCPOLEN_WINDOW;
 		break;
 	default:
-		if ((nptr = nbuf_advance(nbuf, 1, 1)) == NULL) {
+		if ((nptr = nbuf_ensure_contig(nbuf, 2)) == NULL) {
 			ok = false;
 			goto done;
 		}
-		val = *(uint8_t *)nptr;
+		val = *(nptr + 1);
 		if (val < 2 || val > topts_len) {
 			ok = false;
 			goto done;
 		}
 		topts_len -= val;
-		step = val - 1;
+		step = val;
 	}
 
 	/* Any options left? */
@@ -322,18 +323,22 @@
 	const uint8_t ver = *(const uint8_t *)nptr;
 	int flags = 0;
 
+	/*
+	 * We intentionally don't read the L4 payload after IPPROTO_AH.
+	 */
+
 	switch (ver >> 4) {
 	case IPVERSION: {
 		struct ip *ip;
 
 		ip = nbuf_ensure_contig(nbuf, sizeof(struct ip));
 		if (ip == NULL) {
-			return 0;
+			return NPC_FMTERR;
 		}
 
 		/* Check header length and fragment offset. */
 		if ((u_int)(ip->ip_hl << 2) < sizeof(struct ip)) {
-			return 0;
+			return NPC_FMTERR;
 		}
 		if (ip->ip_off & ~htons(IP_DF | IP_RF)) {
 			/* Note fragmentation. */
@@ -357,27 +362,29 @@
 		struct ip6_ext *ip6e;
 		struct ip6_frag *ip6f;
 		size_t off, hlen;
+		int frag_present;
 
 		ip6 = nbuf_ensure_contig(nbuf, sizeof(struct ip6_hdr));
 		if (ip6 == NULL) {
-			return 0;
+			return NPC_FMTERR;
 		}
 
+		/*
+		 * XXX: We don't handle IPv6 Jumbograms.
+		 */
+
 		/* Set initial next-protocol value. */
 		hlen = sizeof(struct ip6_hdr);
 		npc->npc_proto = ip6->ip6_nxt;
 		npc->npc_hlen = hlen;
 
+		frag_present = 0;
+
 		/*
 		 * Advance by the length of the current header.
 		 */
 		off = nbuf_offset(nbuf);
-		while (nbuf_advance(nbuf, hlen, 0) != NULL) {
-			ip6e = nbuf_ensure_contig(nbuf, sizeof(*ip6e));
-			if (ip6e == NULL) {
-				return 0;
-			}
-
+		while ((ip6e = nbuf_advance(nbuf, hlen, sizeof(*ip6e))) != NULL) {
 			/*
 			 * Determine whether we are going to continue.
 			 */
@@ -388,24 +395,22 @@
 				hlen = (ip6e->ip6e_len + 1) << 3;
 				break;
 			case IPPROTO_FRAGMENT:
+				if (frag_present++)
+					return NPC_FMTERR;
 				ip6f = nbuf_ensure_contig(nbuf, sizeof(*ip6f));
 				if (ip6f == NULL)
-					return 0;
-				/*
-				 * We treat the first fragment as a regular
-				 * packet and then we pass the rest of the
-				 * fragments unconditionally. This way if
-				 * the first packet passes the rest will
-				 * be able to reassembled, if not they will
-				 * be ignored. We can do better later.
-				 */
-				if (ntohs(ip6f->ip6f_offlg & IP6F_OFF_MASK) != 0)
-					flags |= NPC_IPFRAG;
+					return NPC_FMTERR;
 
-				hlen = sizeof(struct ip6_frag);
-				break;
-			case IPPROTO_AH:
-				hlen = (ip6e->ip6e_len + 2) << 2;
+				/* RFC6946: Skip dummy fragments. */
+				if (!ntohs(ip6f->ip6f_offlg & IP6F_OFF_MASK) &&
+				    !(ip6f->ip6f_offlg & IP6F_MORE_FRAG)) {
+					hlen = sizeof(struct ip6_frag);
+					break;
+				}
+
+				hlen = 0;
+				flags |= NPC_IPFRAG;
+
 				break;
 			default:
 				hlen = 0;
@@ -432,7 +437,7 @@
 		/* Cache: layer 3 - IPv6. */
 		npc->npc_alen = sizeof(struct in6_addr);
 		npc->npc_ips[NPF_SRC] = (npf_addr_t *)&ip6->ip6_src;
-		npc->npc_ips[NPF_DST]= (npf_addr_t *)&ip6->ip6_dst;
+		npc->npc_ips[NPF_DST] = (npf_addr_t *)&ip6->ip6_dst;
 
 		npc->npc_ip.v6 = ip6;
 		flags |= NPC_IP6;
@@ -469,7 +474,8 @@
 	 * fragmented, then we cannot look into L4.
 	 */
 	flags = npf_cache_ip(npc, nbuf);
-	if ((flags & NPC_IP46) == 0 || (flags & NPC_IPFRAG) != 0) {
+	if ((flags & NPC_IP46) == 0 || (flags & NPC_IPFRAG) != 0 ||
+	    (flags & NPC_FMTERR) != 0) {
 		nbuf_unset_flag(nbuf, NBUF_DATAREF_RESET);
 		npc->npc_info |= flags;
 		return flags;
--- a/sys/net/npf/npf_sendpkt.c	Wed May 09 15:28:44 2018 +0000
+++ b/sys/net/npf/npf_sendpkt.c	Wed May 09 15:35:37 2018 +0000
@@ -1,4 +1,4 @@
-/*	$NetBSD: npf_sendpkt.c,v 1.16 2016/12/26 23:05:06 christos Exp $	*/
+/*	$NetBSD: npf_sendpkt.c,v 1.16.8.1 2018/05/09 15:35:37 martin Exp $	*/
 
 /*-
  * Copyright (c) 2010-2011 The NetBSD Foundation, Inc.
@@ -35,7 +35,7 @@
 
 #ifdef _KERNEL
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: npf_sendpkt.c,v 1.16 2016/12/26 23:05:06 christos Exp $");
+__KERNEL_RCSID(0, "$NetBSD: npf_sendpkt.c,v 1.16.8.1 2018/05/09 15:35:37 martin Exp $");
 
 #include <sys/param.h>
 #include <sys/types.h>
@@ -49,6 +49,7 @@
 #include <netinet/ip6.h>
 #include <netinet/icmp6.h>
 #include <netinet6/ip6_var.h>
+#include <netinet6/scope6_var.h>
 #include <sys/mbuf.h>
 #endif
 
@@ -175,11 +176,29 @@
 		    sizeof(struct tcphdr));
 	}
 
+	/* Handle IPv6 scopes */
+	if (npf_iscached(npc, NPC_IP6)) {
+		const struct ifnet *rcvif = npc->npc_nbuf->nb_ifp;
+
+		if (in6_clearscope(&ip6->ip6_src) ||
+		    in6_clearscope(&ip6->ip6_dst)) {
+			goto bad;
+		}
+		if (in6_setscope(&ip6->ip6_src, rcvif, NULL) ||
+		    in6_setscope(&ip6->ip6_dst, rcvif, NULL)) {
+			goto bad;
+		}
+	}
+
 	/* Pass to IP layer. */
 	if (npf_iscached(npc, NPC_IP4)) {
 		return ip_output(m, NULL, NULL, IP_FORWARDING, NULL, NULL);
 	}
 	return ip6_output(m, NULL, NULL, IPV6_FORWARDING, NULL, NULL, NULL);
+
+bad:
+	m_freem(m);
+	return EINVAL;
 }
 
 /*
@@ -194,6 +213,18 @@
 		icmp_error(m, ICMP_UNREACH, ICMP_UNREACH_ADMIN_PROHIBIT, 0, 0);
 		return 0;
 	} else if (npf_iscached(npc, NPC_IP6)) {
+		/* Handle IPv6 scopes */
+		const struct ifnet *rcvif = npc->npc_nbuf->nb_ifp;
+		struct ip6_hdr *ip6 = mtod(m, struct ip6_hdr *);
+		if (in6_clearscope(&ip6->ip6_src) ||
+		    in6_clearscope(&ip6->ip6_dst)) {
+			return EINVAL;
+		}
+		if (in6_setscope(&ip6->ip6_src, rcvif, NULL) ||
+		    in6_setscope(&ip6->ip6_dst, rcvif, NULL)) {
+			return EINVAL;
+		}
+
 		icmp6_error(m, ICMP6_DST_UNREACH, ICMP6_DST_UNREACH_ADMIN, 0);
 		return 0;
 	}