NetBSD Problem Report #54507

From kardel@pip.kardel.name  Thu Aug 29 14:15:41 2019
Return-Path: <kardel@pip.kardel.name>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id A69C97A176
	for <gnats-bugs@gnats.NetBSD.org>; Thu, 29 Aug 2019 14:15:41 +0000 (UTC)
Message-Id: <20190829141535.76A62DA0D90@pip.kardel.name>
Date: Thu, 29 Aug 2019 16:15:35 +0200 (CEST)
From: kardel@netbsd.org
Reply-To: kardel@netbsd.org
To: gnats-bugs@NetBSD.org
Subject: -9/-current: NPF double NAT doesn't work
X-Send-Pr-Version: 3.95

>Number:         54507
>Category:       kern
>Synopsis:       double NAT doesn't work with NPF
>Confidential:   no
>Severity:       serious
>Priority:       high
>Responsible:    rmind
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Aug 29 14:20:00 +0000 2019
>Closed-Date:    Sun May 31 17:21:59 +0000 2020
>Last-Modified:  Sun May 31 17:21:59 +0000 2020
>Originator:     Frank Kardel
>Release:        NetBSD 9.99.10
>Organization:

>Environment:


System: NetBSD pip.kardel.name 9.99.10 NetBSD 9.99.10 (PIPGEN) #20: Sat Aug 10 12:14:12 CEST 2019 kardel@...:/usr/src/sys/arch/amd64/compile/PIPGEN amd64
Architecture: x86_64
Machine: amd64
>Description:
	Attempting to port a double NAT setup from PF to NPF uncovered an issue in the NPF implementation:

	Double NAT is used to map a service on an internal server/port to an external ip-address/port at the gateway.
	Thus the setup is as follows:

	(the internet) <-> public service ip-address/port@gateway <-> (internal network) <-> internal service ip-address/port@server
									       ^
                                                                               |
	(the internet) <-> @other gateways <-----------------------------------+

	This would work with a simple NAT rule if the (default) route to the source would pass through the original gateway. When the
	internal network has several gateways to the internet the way back to the source of the connection is ambiguous. For
	NAT to work in this case the packets from server must travel back to gateway for the correct NAT reverse translation.
	In PF following construct accomplishes this:
		# example to map htts on an external ip address to an internal server:9444
		# forward nat mapping addresses the service on server.
		rdr pass log on $ext_if inet proto tcp from any to $external_service port https -> $internal_server port 9444

		# double NAT mapping replacing the external source address by an internal gateway address
		nat log on $internal_interface inet proto tcp from any to $internal_server port 9444 -> $internal_interface

	This translates in NPF to:
		# forward nat mapping addresses the service on server.
		map $ext_if dynamic $internal_server port 9444 <- any pass any to $external_service port https

		# double NAT mapping replacing the external source address by an internal gateway address
		map $internal_interface dynamic any -> $internal_interface pass any to $internal_server port 9444

	In the current code this does not work as the double nat mapping fails to be instantiated due to a connection db key conflict.
	This connection db key conflict is caused by npf_conn_establish() inserting a backward key into the connection db althought
	the correct nat values are not yet known. In the above scenario the backward key collides with the nat connection key created
	in the forward mapping. At this point the mapping setup is aborted and an ICMP (with wrong addresses) error is returned.

	The current code corrects the incorrect backward key in the npf_conn_setnat() function by REMOVING the wrong backward key,
	adjusting the key to the NAT values and inserting the correct backward key.

	In the double NAT case we do no reach this 'workaround' as we run into the connection db key conflict earlier.

	The correct handling would delay entering the backward key until the final values are known in npf_conn_setnat().

	Scope: probably all NPF implementations up not now (including but not limited to -8, -9).

>How-To-Repeat:
	Attempt to configure the above double NAT scenario from above and examine the packet flow.

>Fix:
	Following patch is for -current (-9 has some fuss)

Index: npf_conn.c
===================================================================
RCS file: /cvsroot/src/sys/net/npf/npf_conn.c,v
retrieving revision 1.29
diff -u -r1.29 npf_conn.c
--- npf_conn.c	6 Aug 2019 11:40:15 -0000	1.29
+++ npf_conn.c	29 Aug 2019 14:06:56 -0000
@@ -390,9 +390,13 @@
  *
  * => Connection is created with the reference held for the caller.
  * => Connection will be activated on the first reference release.
+ *
+ * If fornat is true the the insertion of the backward link is
+ * suppressed to allow the nat rule to correctly set up the backward link
+ *
  */
 npf_conn_t *
-npf_conn_establish(npf_cache_t *npc, int di, bool global)
+npf_conn_establish(npf_cache_t *npc, int di, bool global, bool fornat)
 {
 	npf_t *npf = npc->npc_ctx;
 	const unsigned alen = npc->npc_alen;
@@ -465,12 +469,24 @@
 		error = EISCONN;
 		goto err;
 	}
-	if (!npf_conndb_insert(npf->conn_db, bk, con, false)) {
-		npf_conn_t *ret __diagused;
-		ret = npf_conndb_remove(npf->conn_db, fw);
-		KASSERT(ret == con);
-		error = EISCONN;
-		goto err;
+	/*
+	 * When setting up NAT:
+	 *   defer entry of backward key until it has been fully determined
+	 *   instead of inserting key that may conflict with other NAT rules
+	 *   in the double NAT case:
+	 *     map <if> dynamic <target ip> port <target port>
+	 *         <- <destination ip> port <destination port>
+	 *     map <if> dynamic 0.0.0.0/0 -> <local source ip>
+	 *          pass from any to <target ip> port <target port>
+	 */
+	if (!fornat) {
+		if (!npf_conndb_insert(npf->conn_db, bk, con, false)) {
+			npf_conn_t *ret __diagused;
+			ret = npf_conndb_remove(npf->conn_db, fw);
+			KASSERT(ret == con);
+			error = EISCONN;
+		        goto err;
+		}
 	}
 err:
 	/*
@@ -565,11 +581,9 @@
 		return EISCONN;
 	}

-	/* Remove the "backwards" key. */
+	/* set up the "backwards" key. */
 	fw = npf_conn_getforwkey(con);
 	bk = npf_conn_getbackkey(con, NPF_CONNKEY_ALEN(fw));
-	ret = npf_conndb_remove(npf->conn_db, bk);
-	KASSERT(ret == con);

 	/* Set the source/destination IDs to the translation values. */
 	npf_conn_adjkey(bk, taddr, tport, nat_type_dimap[ntype]);
Index: npf_conn.h
===================================================================
RCS file: /cvsroot/src/sys/net/npf/npf_conn.h,v
retrieving revision 1.18
diff -u -r1.18 npf_conn.h
--- npf_conn.h	11 Aug 2019 20:26:33 -0000	1.18
+++ npf_conn.h	29 Aug 2019 14:06:56 -0000
@@ -129,7 +129,7 @@

 npf_conn_t *	npf_conn_lookup(const npf_cache_t *, const int, bool *);
 npf_conn_t *	npf_conn_inspect(npf_cache_t *, const int, int *);
-npf_conn_t *	npf_conn_establish(npf_cache_t *, int, bool);
+npf_conn_t *	npf_conn_establish(npf_cache_t *, int, bool, bool);
 void		npf_conn_release(npf_conn_t *);
 void		npf_conn_destroy(npf_t *, npf_conn_t *);
 void		npf_conn_expire(npf_conn_t *);
Index: npf_connkey.c
===================================================================
RCS file: /cvsroot/src/sys/net/npf/npf_connkey.c,v
retrieving revision 1.1
diff -u -r1.1 npf_connkey.c
--- npf_connkey.c	23 Jul 2019 00:52:01 -0000	1.1
+++ npf_connkey.c	29 Aug 2019 14:06:56 -0000
@@ -267,8 +267,8 @@
 	npf_addr_t ips[2];

 	connkey_getkey(key, &proto, ips, ids, &alen);
-	printf("\tforw %s:%d", npf_addr_dump(&ips[0], alen), ids[0]);
-	printf("-> %s:%d\n", npf_addr_dump(&ips[1], alen), ids[1]);
+	printf("\tforw %s:%d", npf_addr_dump(&ips[0], alen), ntohs(ids[0]));
+	printf("-> %s:%d\n", npf_addr_dump(&ips[1], alen), ntohs(ids[1]));
 }

 #endif
Index: npf_handler.c
===================================================================
RCS file: /cvsroot/src/sys/net/npf/npf_handler.c,v
retrieving revision 1.48
diff -u -r1.48 npf_handler.c
--- npf_handler.c	25 Aug 2019 13:21:03 -0000	1.48
+++ npf_handler.c	29 Aug 2019 14:06:56 -0000
@@ -232,7 +232,7 @@
 	 */
 	if ((mi.mi_retfl & NPF_RULE_STATEFUL) != 0 && !con) {
 		con = npf_conn_establish(&npc, di,
-		    (mi.mi_retfl & NPF_RULE_GSTATEFUL) == 0);
+		    (mi.mi_retfl & NPF_RULE_GSTATEFUL) == 0, false);
 		if (con) {
 			/*
 			 * Note: the reference on the rule procedure is
Index: npf_nat.c
===================================================================
RCS file: /cvsroot/src/sys/net/npf/npf_nat.c,v
retrieving revision 1.48
diff -u -r1.48 npf_nat.c
--- npf_nat.c	25 Aug 2019 13:21:03 -0000	1.48
+++ npf_nat.c	29 Aug 2019 14:06:56 -0000
@@ -660,7 +660,7 @@
 	 * "backwards" stream depends on other, stateless filtering rules.
 	 */
 	if (con == NULL) {
-		ncon = npf_conn_establish(npc, di, true);
+		ncon = npf_conn_establish(npc, di, true, true);
 		if (ncon == NULL) {
 			atomic_dec_uint(&np->n_refcnt);
 			return ENOMEM;

>Release-Note:

>Audit-Trail:

Responsible-Changed-From-To: gnats-admin->kern-bug-people
Responsible-Changed-By: spz@NetBSD.org
Responsible-Changed-When: Mon, 02 Sep 2019 21:26:26 +0000
Responsible-Changed-Why:
adjusted category from pending to kern


Responsible-Changed-From-To: kern-bug-people->rmind
Responsible-Changed-By: rmind@NetBSD.org
Responsible-Changed-When: Thu, 14 May 2020 20:11:24 +0000
Responsible-Changed-Why:


State-Changed-From-To: open->closed
State-Changed-By: rmind@NetBSD.org
State-Changed-When: Sun, 31 May 2020 17:21:59 +0000
State-Changed-Why:
Should be fixed in -current now.


>Unformatted:

NetBSD Home
NetBSD PR Database Search

(Contact us) $NetBSD: query-full-pr,v 1.46 2020/01/03 16:35:01 leot Exp $
$NetBSD: gnats_config.sh,v 1.9 2014/08/02 14:16:04 spz Exp $
Copyright © 1994-2020 The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.