git: 7ce98cf2f87a - main - pfctl: fix incorrect mask on dynamic address

From: Kristof Provost <kp_at_FreeBSD.org>
Date: Fri, 06 Oct 2023 16:16:35 UTC
The branch main has been updated by kp:

URL: https://cgit.FreeBSD.org/src/commit/?id=7ce98cf2f87a22240b66e4c38fd887431a25bf7d

commit 7ce98cf2f87a22240b66e4c38fd887431a25bf7d
Author:     Kristof Provost <kp@FreeBSD.org>
AuthorDate: 2023-10-06 12:20:17 +0000
Commit:     Kristof Provost <kp@FreeBSD.org>
CommitDate: 2023-10-06 16:11:28 +0000

    pfctl: fix incorrect mask on dynamic address
    
    A PF rule using an IPv4 address followed by an IPv6 address and then a
    dynamic address, e.g. "pass from {192.0.2.1 2001:db8::1} to (pppoe0)",
    will have an incorrect /32 mask applied to the dynamic address.
    
    MFC after:      3 weeks
    Obtained from:  OpenBSD
    See also:       https://ftp.openbsd.org/pub/OpenBSD/patches/5.6/common/007_pfctl.patch.sig
    Sponsored by:   Rubicon Communications, LLC ("Netgate")
    Event:          Oslo Hackathon at Modirum
---
 sbin/pfctl/parse.y                   | 39 +++++++++++++++++++++++++++++++++++-
 sbin/pfctl/pfctl_parser.c            | 30 ++++++++++++++++++++++-----
 sbin/pfctl/pfctl_parser.h            |  1 +
 sbin/pfctl/tests/files/pf0102.ok     |  4 ++--
 sbin/pfctl/tests/files/pf1018.in     |  1 +
 sbin/pfctl/tests/files/pf1018.ok     |  2 ++
 sbin/pfctl/tests/pfctl_test_list.inc |  1 +
 7 files changed, 70 insertions(+), 8 deletions(-)

diff --git a/sbin/pfctl/parse.y b/sbin/pfctl/parse.y
index 041dcb0587b3..17227b674814 100644
--- a/sbin/pfctl/parse.y
+++ b/sbin/pfctl/parse.y
@@ -4720,6 +4720,10 @@ natrule		: nataction interface af proto fromto tag tagged rtable
 				remove_invalid_hosts(&$9->host, &r.af);
 				if (invalid_redirect($9->host, r.af))
 					YYERROR;
+				if ($9->host->addr.type == PF_ADDR_DYNIFTL) {
+					if (($9->host = gen_dynnode($9->host, r.af)) == NULL)
+						err(1, "calloc");
+				}
 				if (check_netmask($9->host, r.af))
 					YYERROR;
 
@@ -4936,6 +4940,10 @@ binatrule	: no BINAT natpasslog interface af proto FROM ipspec toipspec tag
 					yyerror("binat ip versions must match");
 					YYERROR;
 				}
+				if ($8->addr.type == PF_ADDR_DYNIFTL) {
+					if (($8 = gen_dynnode($8, binat.af)) == NULL)
+						err(1, "calloc");
+				}
 				if (check_netmask($8, binat.af))
 					YYERROR;
 				memcpy(&binat.src.addr, &$8->addr,
@@ -4951,6 +4959,10 @@ binatrule	: no BINAT natpasslog interface af proto FROM ipspec toipspec tag
 					yyerror("binat ip versions must match");
 					YYERROR;
 				}
+				if ($9->addr.type == PF_ADDR_DYNIFTL) {
+					if (($9 = gen_dynnode($9, binat.af)) == NULL)
+						err(1, "calloc");
+				}
 				if (check_netmask($9, binat.af))
 					YYERROR;
 				memcpy(&binat.dst.addr, &$9->addr,
@@ -4980,6 +4992,10 @@ binatrule	: no BINAT natpasslog interface af proto FROM ipspec toipspec tag
 					    "a single address");
 					YYERROR;
 				}
+				if ($13->host->addr.type == PF_ADDR_DYNIFTL) {
+					if (($13->host = gen_dynnode($13->host, binat.af)) == NULL)
+						err(1, "calloc");
+				}
 				if (check_netmask($13->host, binat.af))
 					YYERROR;
 
@@ -5982,7 +5998,7 @@ expand_rule(struct pfctl_rule *r,
 	char			 tagname[PF_TAG_NAME_SIZE];
 	char			 match_tagname[PF_TAG_NAME_SIZE];
 	struct pf_pooladdr	*pa;
-	struct node_host	*h;
+	struct node_host	*h, *osrch, *odsth;
 	u_int8_t		 flags, flagset, keep_state;
 
 	memcpy(label, r->label, sizeof(r->label));
@@ -6043,6 +6059,18 @@ expand_rule(struct pfctl_rule *r,
 		    sizeof(r->match_tagname)) >= sizeof(r->match_tagname))
 			errx(1, "expand_rule: strlcpy");
 
+		osrch = odsth = NULL;
+		if (src_host->addr.type == PF_ADDR_DYNIFTL) {
+			osrch = src_host;
+			if ((src_host = gen_dynnode(src_host, r->af)) == NULL)
+				err(1, "expand_rule: calloc");
+		}
+		if (dst_host->addr.type == PF_ADDR_DYNIFTL) {
+			odsth = dst_host;
+			if ((dst_host = gen_dynnode(dst_host, r->af)) == NULL)
+				err(1, "expand_rule: calloc");
+		}
+
 		error += check_netmask(src_host, r->af);
 		error += check_netmask(dst_host, r->af);
 
@@ -6121,6 +6149,15 @@ expand_rule(struct pfctl_rule *r,
 			added++;
 		}
 
+		if (osrch && src_host->addr.type == PF_ADDR_DYNIFTL) {
+			free(src_host);
+			src_host = osrch;
+		}
+		if (odsth && dst_host->addr.type == PF_ADDR_DYNIFTL) {
+			free(dst_host);
+			dst_host = odsth;
+		}
+
 	))))))))));
 
 	FREE_LIST(struct node_if, interfaces);
diff --git a/sbin/pfctl/pfctl_parser.c b/sbin/pfctl/pfctl_parser.c
index 936c5ec53759..925848055bba 100644
--- a/sbin/pfctl/pfctl_parser.c
+++ b/sbin/pfctl/pfctl_parser.c
@@ -1306,16 +1306,12 @@ int
 check_netmask(struct node_host *h, sa_family_t af)
 {
 	struct node_host	*n = NULL;
-	struct pf_addr	*m;
+	struct pf_addr		*m;
 
 	for (n = h; n != NULL; n = n->next) {
 		if (h->addr.type == PF_ADDR_TABLE)
 			continue;
 		m = &h->addr.v.a.mask;
-		/* fix up netmask for dynaddr */
-		if (af == AF_INET && h->addr.type == PF_ADDR_DYNIFTL &&
-		    unmask(m, AF_INET6) > 32)
-			set_ipmask(n, 32);
 		/* netmasks > 32 bit are invalid on v4 */
 		if (af == AF_INET &&
 		    (m->addr32[1] || m->addr32[2] || m->addr32[3])) {
@@ -1327,6 +1323,30 @@ check_netmask(struct node_host *h, sa_family_t af)
 	return (0);
 }
 
+struct node_host *
+gen_dynnode(struct node_host *h, sa_family_t af)
+{
+	struct node_host	*n;
+	struct pf_addr		*m;
+
+	if (h->addr.type != PF_ADDR_DYNIFTL)
+		return (NULL);
+
+	if ((n = calloc(1, sizeof(*n))) == NULL)
+		return (NULL);
+	bcopy(h, n, sizeof(*n));
+	n->ifname = NULL;
+	n->next = NULL;
+	n->tail = NULL;
+
+	/* fix up netmask */
+	m = &n->addr.v.a.mask;
+	if (af == AF_INET && unmask(m, AF_INET6) > 32)
+		set_ipmask(n, 32);
+
+	return (n);
+}
+
 /* interface lookup routines */
 
 static struct node_host	*iftab;
diff --git a/sbin/pfctl/pfctl_parser.h b/sbin/pfctl/pfctl_parser.h
index 83a4880106a8..d0f3bc3c303c 100644
--- a/sbin/pfctl/pfctl_parser.h
+++ b/sbin/pfctl/pfctl_parser.h
@@ -360,6 +360,7 @@ extern const struct pf_timeout pf_timeouts[];
 void			 set_ipmask(struct node_host *, u_int8_t);
 int			 check_netmask(struct node_host *, sa_family_t);
 int			 unmask(struct pf_addr *, sa_family_t);
+struct node_host	*gen_dynnode(struct node_host *, sa_family_t);
 void			 ifa_load(void);
 int			 get_query_socket(void);
 struct node_host	*ifa_exists(char *);
diff --git a/sbin/pfctl/tests/files/pf0102.ok b/sbin/pfctl/tests/files/pf0102.ok
index 3233ca5a2643..1c76ec2725ba 100644
--- a/sbin/pfctl/tests/files/pf0102.ok
+++ b/sbin/pfctl/tests/files/pf0102.ok
@@ -1,8 +1,8 @@
 pass inet from 1.1.1.1 to (self) flags S/SA keep state
-pass inet6 from 2002:: to (self)/32 flags S/SA keep state
+pass inet6 from 2002:: to (self) flags S/SA keep state
 pass inet6 from 2002:: to (self) flags S/SA keep state
 pass inet from 1.1.1.1 to (self) flags S/SA keep state
 pass inet from 1.1.1.1 to (self) flags S/SA keep state
-pass inet6 from 2002:: to (self)/32 flags S/SA keep state
+pass inet6 from 2002:: to (self)/40 flags S/SA keep state
 pass inet6 from 2002:: to (self)/40 flags S/SA keep state
 pass inet from 1.1.1.1 to (self) flags S/SA keep state
diff --git a/sbin/pfctl/tests/files/pf1018.in b/sbin/pfctl/tests/files/pf1018.in
new file mode 100644
index 000000000000..90f0a3a0bab7
--- /dev/null
+++ b/sbin/pfctl/tests/files/pf1018.in
@@ -0,0 +1 @@
+pass from { 192.0.2.1 2001:db8::1 } to (pppoe0)
diff --git a/sbin/pfctl/tests/files/pf1018.ok b/sbin/pfctl/tests/files/pf1018.ok
new file mode 100644
index 000000000000..04950f0035b8
--- /dev/null
+++ b/sbin/pfctl/tests/files/pf1018.ok
@@ -0,0 +1,2 @@
+pass inet from 192.0.2.1 to (pppoe0) flags S/SA keep state
+pass inet6 from 2001:db8::1 to (pppoe0) flags S/SA keep state
diff --git a/sbin/pfctl/tests/pfctl_test_list.inc b/sbin/pfctl/tests/pfctl_test_list.inc
index 98ea3257d492..4a63c0ba48b7 100644
--- a/sbin/pfctl/tests/pfctl_test_list.inc
+++ b/sbin/pfctl/tests/pfctl_test_list.inc
@@ -126,3 +126,4 @@ PFCTL_TEST(1014, "Ethernet rule with one label")
 PFCTL_TEST(1015, "Ethernet rule with several labels")
 PFCTL_TEST(1016, "Ethernet rule with ridentifier and one label")
 PFCTL_TEST(1017, "Ethernet rule with ridentifier and several labels")
+PFCTL_TEST(1018, "Test dynamic address mask")