Module Name:src
Committed By: martin
Date: Mon May 14 16:17:19 UTC 2018
Modified Files:
src/sys/net/npf [netbsd-7-1]: npf_alg_icmp.c npf_inet.c
Log Message:
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.
To generate a diff of this commit:
cvs rdiff -u -r1.23 -r1.23.12.1 src/sys/net/npf/npf_alg_icmp.c
cvs rdiff -u -r1.32 -r1.32.10.1 src/sys/net/npf/npf_inet.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/sys/net/npf/npf_alg_icmp.c
diff -u src/sys/net/npf/npf_alg_icmp.c:1.23 src/sys/net/npf/npf_alg_icmp.c:1.23.12.1
--- src/sys/net/npf/npf_alg_icmp.c:1.23 Sun Jul 20 00:37:41 2014
+++ src/sys/net/npf/npf_alg_icmp.c Mon May 14 16:17:19 2018
@@ -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.12.1 2018/05/14 16:17:19 martin Exp $ */
/*-
* Copyright (c) 2010 The NetBSD Foundation, Inc.
@@ -34,7 +34,7 @@
*/
#include
-__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.12.1 2018/05/14 16:17:19 martin Exp $");
#include
#include
@@ -118,13 +118,15 @@ npfa_icmp_match(npf_cache_t *npc, npf_na
/*
* 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 @@ npfa_icmp4_inspect(const int type, npf_c
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 @@ npfa_icmp4_inspect(const int type, npf_c
}
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 @@ npfa_icmp6_inspect(const int type, npf_c
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_icmp6_inspect(const int type, npf_c
/*
* 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 @@ npfa_icmp_inspect(npf_cache_t *npc, npf_
*/
if (npf_iscached(npc, NPC_IP4)) {