Pull up following revision(s) (requested by maxv in ticket #1605): netbsd-7
authormartin <martin@NetBSD.org>
Mon, 14 May 2018 16:16:04 +0000
branchnetbsd-7
changeset 319037 4a3b47cbe588
parent 319036 379a9b75ce51
child 319038 cc554459a1a6
Pull up following revision(s) (requested by maxv in ticket #1605): sys/net/npf/npf_inet.c: revision 1.45 sys/net/npf/npf_alg_icmp.c: revision 1.27-1.29 Fix use-after-free. The nbuf can be reallocated as a result of caching 'enpc', so it is necessary to recache 'npc', otherwise it contains pointers to the freed mbuf - pointers which are then used in the ruleset machinery. We recache 'npc' when we are sure we won't use 'enpc' anymore, because 'enpc' can be clobbered as a result of caching 'npc' (in other words, only one of the two can be cached at the same time). Also, we recache 'npc' unconditionally, because there is no way to know whether the nbuf got clobbered relatively to it. We can't use the NBUF_DATAREF_RESET flag, because it is stored in the nbuf and not in the cache. Discussed with rmind@. Change npf_cache_all so that it ensures the potential ICMP Query Id is in the nbuf. In such a way that we don't need to ensure that later. Change npfa_icmp4_inspect and npfa_icmp6_inspect so that they touch neither the nbuf nor npc. Adapt their callers accordingly. In the end, if a packet has a Query Id, we set NPC_ICMP_ID in npc and leave right away, without recaching npc (not needed since we didn't touch the nbuf). This fixes the handling of Query Id packets (that I broke in my previous commit), and also fixes another possible use-after-free. Ah, fix compilation. I tested my previous change by loading the kernel module from the filesystem, but the Makefile didn't have DIAGNOSTIC enabled, and the two KASSERTs I added did not compile properly.
sys/net/npf/npf_alg_icmp.c
sys/net/npf/npf_inet.c
--- a/sys/net/npf/npf_alg_icmp.c	Mon May 14 16:11:09 2018 +0000
+++ b/sys/net/npf/npf_alg_icmp.c	Mon May 14 16:16:04 2018 +0000
@@ -1,4 +1,4 @@
-/*	$NetBSD: npf_alg_icmp.c,v 1.23 2014/07/20 00:37:41 rmind Exp $	*/
+/*	$NetBSD: npf_alg_icmp.c,v 1.23.2.1 2018/05/14 16:16:04 martin Exp $	*/
 
 /*-
  * Copyright (c) 2010 The NetBSD Foundation, Inc.
@@ -34,7 +34,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: npf_alg_icmp.c,v 1.23 2014/07/20 00:37:41 rmind Exp $");
+__KERNEL_RCSID(0, "$NetBSD: npf_alg_icmp.c,v 1.23.2.1 2018/05/14 16:16:04 martin Exp $");
 
 #include <sys/param.h>
 #include <sys/module.h>
@@ -118,13 +118,15 @@
 /*
  * npfa_icmp{4,6}_inspect: retrieve unique identifiers - either ICMP query
  * ID or TCP/UDP ports of the original packet, which is embedded.
+ *
+ * => Sets hasqid=true if the packet has a Query Id. In this case neither
+ *    the nbuf nor npc is touched.
  */
 
 static bool
-npfa_icmp4_inspect(const int type, npf_cache_t *npc)
+npfa_icmp4_inspect(const int type, npf_cache_t *npc, bool *hasqid)
 {
 	nbuf_t *nbuf = npc->npc_nbuf;
-	u_int offby;
 
 	/* Per RFC 792. */
 	switch (type) {
@@ -148,12 +150,8 @@
 	case ICMP_TSTAMPREPLY:
 	case ICMP_IREQ:
 	case ICMP_IREQREPLY:
-		/* Should contain ICMP query ID - ensure. */
-		offby = offsetof(struct icmp, icmp_id);
-		if (!nbuf_advance(nbuf, offby, sizeof(uint16_t))) {
-			return false;
-		}
-		npc->npc_info |= NPC_ICMP_ID;
+		/* Contains ICMP query ID. */
+		*hasqid = true;
 		return true;
 	default:
 		break;
@@ -162,10 +160,9 @@
 }
 
 static bool
-npfa_icmp6_inspect(const int type, npf_cache_t *npc)
+npfa_icmp6_inspect(const int type, npf_cache_t *npc, bool *hasqid)
 {
 	nbuf_t *nbuf = npc->npc_nbuf;
-	u_int offby;
 
 	/* Per RFC 4443. */
 	switch (type) {
@@ -184,12 +181,8 @@
 
 	case ICMP6_ECHO_REQUEST:
 	case ICMP6_ECHO_REPLY:
-		/* Should contain ICMP query ID - ensure. */
-		offby = offsetof(struct icmp6_hdr, icmp6_id);
-		if (!nbuf_advance(nbuf, offby, sizeof(uint16_t))) {
-			return false;
-		}
-		npc->npc_info |= NPC_ICMP_ID;
+		/* Contains ICMP query ID. */
+		*hasqid = true;
 		return true;
 	default:
 		break;
@@ -200,13 +193,13 @@
 /*
  * npfa_icmp_inspect: ALG ICMP inspector.
  *
- * => Returns true if "enpc" is filled.
+ * => Returns false if there is a problem with the format.
  */
 static bool
 npfa_icmp_inspect(npf_cache_t *npc, npf_cache_t *enpc)
 {
 	nbuf_t *nbuf = npc->npc_nbuf;
-	bool ret;
+	bool ret, hasqid = false;
 
 	KASSERT(npf_iscached(npc, NPC_IP46));
 	KASSERT(npf_iscached(npc, NPC_ICMP));
@@ -225,10 +218,10 @@
 	 */
 	if (npf_iscached(npc, NPC_IP4)) {
 		const struct icmp *ic = npc->npc_l4.icmp;
-		ret = npfa_icmp4_inspect(ic->icmp_type, enpc);
+		ret = npfa_icmp4_inspect(ic->icmp_type, enpc, &hasqid);
 	} else if (npf_iscached(npc, NPC_IP6)) {
 		const struct icmp6_hdr *ic6 = npc->npc_l4.icmp6;
-		ret = npfa_icmp6_inspect(ic6->icmp6_type, enpc);
+		ret = npfa_icmp6_inspect(ic6->icmp6_type, enpc, &hasqid);
 	} else {
 		ret = false;
 	}
@@ -237,25 +230,34 @@
 	}
 
 	/* ICMP ID is the original packet, just indicate it. */
-	if (npf_iscached(enpc, NPC_ICMP_ID)) {
+	if (hasqid) {
 		npc->npc_info |= NPC_ICMP_ID;
-		return false;
 	}
 
-	/* Indicate that embedded packet is in the cache. */
 	return true;
 }
 
 static npf_conn_t *
 npfa_icmp_conn(npf_cache_t *npc, int di)
 {
+	npf_conn_t *conn = NULL;
 	npf_cache_t enpc;
+	bool hasqid = false;
 
 	/* Inspect ICMP packet for an embedded packet. */
 	if (!npf_iscached(npc, NPC_ICMP))
 		return NULL;
 	if (!npfa_icmp_inspect(npc, &enpc))
+		goto out;
+
+	/*
+	 * If the ICMP packet had a Query Id, leave now. The packet didn't get
+	 * modified, so no need to recache npc.
+	 */
+	if (npf_iscached(npc, NPC_ICMP_ID)) {
+		KASSERT(!nbuf_flag_p(npc->npc_nbuf, NBUF_DATAREF_RESET));
 		return NULL;
+	}
 
 	/*
 	 * Invert the identifiers of the embedded packet.
@@ -283,24 +285,34 @@
 		break;
 	case IPPROTO_ICMP: {
 		const struct icmp *ic = enpc.npc_l4.icmp;
-		ret = npfa_icmp4_inspect(ic->icmp_type, &enpc);
-		if (!ret || !npf_iscached(&enpc, NPC_ICMP_ID))
-			return false;
+		ret = npfa_icmp4_inspect(ic->icmp_type, &enpc, &hasqid);
+		if (!ret || !hasqid)
+			goto out;
+		enpc.npc_info |= NPC_ICMP_ID;
 		break;
 	}
 	case IPPROTO_ICMPV6: {
 		const struct icmp6_hdr *ic6 = enpc.npc_l4.icmp6;
-		ret = npfa_icmp6_inspect(ic6->icmp6_type, &enpc);
-		if (!ret || !npf_iscached(&enpc, NPC_ICMP_ID))
-			return false;
+		ret = npfa_icmp6_inspect(ic6->icmp6_type, &enpc, &hasqid);
+		if (!ret || !hasqid)
+			goto out;
+		enpc.npc_info |= NPC_ICMP_ID;
 		break;
 	}
 	default:
-		return false;
+		goto out;
 	}
 
 	/* Lookup a connection using the embedded packet. */
-	return npf_conn_lookup(&enpc, di, &forw);
+	conn = npf_conn_lookup(&enpc, di, &forw);
+
+out:
+	/*
+	 * Recache npc. The nbuf may have been updated as a result of
+	 * caching enpc.
+	 */
+	npf_recache(npc);
+	return conn;
 }
 
 /*
@@ -312,20 +324,32 @@
 {
 	const u_int which = NPF_SRC;
 	npf_cache_t enpc;
+	struct icmp *ic;
+	uint16_t cksum;
 
 	if (forw || !npf_iscached(npc, NPC_ICMP))
 		return false;
-	if (!npfa_icmp_inspect(npc, &enpc))
-		return false;
-
-	KASSERT(npf_iscached(&enpc, NPC_IP46));
-	KASSERT(npf_iscached(&enpc, NPC_LAYER4));
 
 	/*
 	 * ICMP: fetch the current checksum we are going to fixup.
 	 */
-	struct icmp *ic = npc->npc_l4.icmp;
-	uint16_t cksum = ic->icmp_cksum;
+	ic = npc->npc_l4.icmp;
+	cksum = ic->icmp_cksum;
+
+	if (!npfa_icmp_inspect(npc, &enpc))
+		goto err;
+
+	/*
+	 * If the ICMP packet had a Query Id, leave now. The packet didn't get
+	 * modified, so no need to recache npc.
+	 */
+	if (npf_iscached(npc, NPC_ICMP_ID)) {
+		KASSERT(!nbuf_flag_p(npc->npc_nbuf, NBUF_DATAREF_RESET));
+		return false;
+	}
+
+	KASSERT(npf_iscached(&enpc, NPC_IP46));
+	KASSERT(npf_iscached(&enpc, NPC_LAYER4));
 
 	CTASSERT(offsetof(struct icmp, icmp_cksum) ==
 	    offsetof(struct icmp6_hdr, icmp6_cksum));
@@ -365,7 +389,7 @@
 	case IPPROTO_ICMPV6:
 		break;
 	default:
-		return false;
+		goto err;
 	}
 
 	/*
@@ -380,7 +404,7 @@
 	 * npfa_icmp_match() matches only for the PFIL_OUT traffic.
 	 */
 	if (npf_napt_rwr(&enpc, which, addr, port)) {
-		return false;
+		goto err;
 	}
 
 	/*
@@ -404,8 +428,19 @@
 		}
 		break;
 	}
+	npf_recache(npc);
+	KASSERT(npf_iscached(npc, NPC_ICMP));
+	ic = npc->npc_l4.icmp;
 	ic->icmp_cksum = cksum;
 	return true;
+
+err:
+	/*
+	 * Recache npc. The nbuf may have been updated as a result of
+	 * caching enpc.
+	 */
+	npf_recache(npc);
+	return false;
 }
 
 /*
--- a/sys/net/npf/npf_inet.c	Mon May 14 16:11:09 2018 +0000
+++ b/sys/net/npf/npf_inet.c	Mon May 14 16:16:04 2018 +0000
@@ -1,4 +1,4 @@
-/*	$NetBSD: npf_inet.c,v 1.32.2.2 2017/05/22 18:56:35 martin Exp $	*/
+/*	$NetBSD: npf_inet.c,v 1.32.2.3 2018/05/14 16:16:04 martin Exp $	*/
 
 /*-
  * Copyright (c) 2009-2014 The NetBSD Foundation, Inc.
@@ -39,7 +39,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: npf_inet.c,v 1.32.2.2 2017/05/22 18:56:35 martin Exp $");
+__KERNEL_RCSID(0, "$NetBSD: npf_inet.c,v 1.32.2.3 2018/05/14 16:16:04 martin Exp $");
 
 #include <sys/param.h>
 #include <sys/types.h>
@@ -460,6 +460,11 @@
 	}
 	hlen = npc->npc_hlen;
 
+	/*
+	 * Note: we guarantee that the potential "Query Id" field of the
+	 * ICMPv4/ICMPv6 packets is in the nbuf. This field is used in the
+	 * ICMP ALG.
+	 */
 	switch (npc->npc_proto) {
 	case IPPROTO_TCP:
 		/* Cache: layer 4 - TCP. */
@@ -476,13 +481,13 @@
 	case IPPROTO_ICMP:
 		/* Cache: layer 4 - ICMPv4. */
 		npc->npc_l4.icmp = nbuf_advance(nbuf, hlen,
-		    offsetof(struct icmp, icmp_void));
+		    ICMP_MINLEN);
 		l4flags = NPC_LAYER4 | NPC_ICMP;
 		break;
 	case IPPROTO_ICMPV6:
 		/* Cache: layer 4 - ICMPv6. */
 		npc->npc_l4.icmp6 = nbuf_advance(nbuf, hlen,
-		    offsetof(struct icmp6_hdr, icmp6_data32));
+		    sizeof(struct icmp6_hdr));
 		l4flags = NPC_LAYER4 | NPC_ICMP;
 		break;
 	default: