git: dcc05aee1495 - releng/13.3 - pf: split ICMP/ICMPv6 number space in pf_icmp_mapping()
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Wed, 07 Aug 2024 13:44:55 UTC
The branch releng/13.3 has been updated by markj:
URL: https://cgit.FreeBSD.org/src/commit/?id=dcc05aee14951f35e5d66082d82a58298bc79d5b
commit dcc05aee14951f35e5d66082d82a58298bc79d5b
Author: Kristof Provost <kp@FreeBSD.org>
AuthorDate: 2024-07-10 12:10:50 +0000
Commit: Mark Johnston <markj@FreeBSD.org>
CommitDate: 2024-08-07 13:37:28 +0000
pf: split ICMP/ICMPv6 number space in pf_icmp_mapping()
In pf_icmp_mapping() the ICMP and ICMPv6 types shared the same
number space. In fact they are independent and must be handled
separately. Fix traceroute via pf by splitting pf_icmp_mapping()
into IPv4 and IPv6 sections.
ok henning@ mcbride@; tested mcbride@; sure deraadt@
Approved by: so
Security: FreeBSD-SA-24:05.pf
Security: CVE-2024-6640
MFC after: 1 day
Obtained From: OpenBSD, bluhm <bluhm@openbsd.org> ef4bccd7509e
Sponsored by: Rubicon Communications, LLC ("Netgate")
(cherry picked from commit 46755f52247bd34a7f013d6844ed0c673ac0defc)
---
sys/netpfil/pf/pf.c | 247 ++++++++++++++++++++++++++++------------------------
1 file changed, 135 insertions(+), 112 deletions(-)
diff --git a/sys/netpfil/pf/pf.c b/sys/netpfil/pf/pf.c
index b0ef27b2e462..388d632e8d50 100644
--- a/sys/netpfil/pf/pf.c
+++ b/sys/netpfil/pf/pf.c
@@ -1698,7 +1698,7 @@ pf_isforlocal(struct mbuf *m, int af)
int
pf_icmp_mapping(struct pf_pdesc *pd, u_int8_t type,
- int *icmp_dir, int *multi, u_int16_t *icmpid, u_int16_t *icmptype)
+ int *icmp_dir, int *multi, u_int16_t *virtual_id, u_int16_t *virtual_type)
{
/*
* ICMP types marked with PF_OUT are typically responses to
@@ -1708,128 +1708,151 @@ pf_icmp_mapping(struct pf_pdesc *pd, u_int8_t type,
*icmp_dir = PF_OUT;
*multi = PF_ICMP_MULTI_LINK;
/* Queries (and responses) */
- switch (type) {
- case ICMP_ECHO:
- *icmp_dir = PF_IN;
- case ICMP_ECHOREPLY:
- *icmptype = ICMP_ECHO;
- *icmpid = pd->hdr.icmp.icmp_id;
- break;
+ switch (pd->af) {
+#ifdef INET
+ case AF_INET:
+ switch (type) {
+ case ICMP_ECHO:
+ *icmp_dir = PF_IN;
+ case ICMP_ECHOREPLY:
+ *virtual_type = ICMP_ECHO;
+ *virtual_id = pd->hdr.icmp.icmp_id;
+ break;
- case ICMP_TSTAMP:
- *icmp_dir = PF_IN;
- case ICMP_TSTAMPREPLY:
- *icmptype = ICMP_TSTAMP;
- *icmpid = pd->hdr.icmp.icmp_id;
- break;
+ case ICMP_TSTAMP:
+ *icmp_dir = PF_IN;
+ case ICMP_TSTAMPREPLY:
+ *virtual_type = ICMP_TSTAMP;
+ *virtual_id = pd->hdr.icmp.icmp_id;
+ break;
- case ICMP_IREQ:
- *icmp_dir = PF_IN;
- case ICMP_IREQREPLY:
- *icmptype = ICMP_IREQ;
- *icmpid = pd->hdr.icmp.icmp_id;
- break;
+ case ICMP_IREQ:
+ *icmp_dir = PF_IN;
+ case ICMP_IREQREPLY:
+ *virtual_type = ICMP_IREQ;
+ *virtual_id = pd->hdr.icmp.icmp_id;
+ break;
- case ICMP_MASKREQ:
- *icmp_dir = PF_IN;
- case ICMP_MASKREPLY:
- *icmptype = ICMP_MASKREQ;
- *icmpid = pd->hdr.icmp.icmp_id;
- break;
+ case ICMP_MASKREQ:
+ *icmp_dir = PF_IN;
+ case ICMP_MASKREPLY:
+ *virtual_type = ICMP_MASKREQ;
+ *virtual_id = pd->hdr.icmp.icmp_id;
+ break;
- case ICMP_IPV6_WHEREAREYOU:
- *icmp_dir = PF_IN;
- case ICMP_IPV6_IAMHERE:
- *icmptype = ICMP_IPV6_WHEREAREYOU;
- *icmpid = 0; /* Nothing sane to match on! */
- break;
+ case ICMP_IPV6_WHEREAREYOU:
+ *icmp_dir = PF_IN;
+ case ICMP_IPV6_IAMHERE:
+ *virtual_type = ICMP_IPV6_WHEREAREYOU;
+ *virtual_id = 0; /* Nothing sane to match on! */
+ break;
- case ICMP_MOBILE_REGREQUEST:
- *icmp_dir = PF_IN;
- case ICMP_MOBILE_REGREPLY:
- *icmptype = ICMP_MOBILE_REGREQUEST;
- *icmpid = 0; /* Nothing sane to match on! */
- break;
+ case ICMP_MOBILE_REGREQUEST:
+ *icmp_dir = PF_IN;
+ case ICMP_MOBILE_REGREPLY:
+ *virtual_type = ICMP_MOBILE_REGREQUEST;
+ *virtual_id = 0; /* Nothing sane to match on! */
+ break;
- case ICMP_ROUTERSOLICIT:
- *icmp_dir = PF_IN;
- case ICMP_ROUTERADVERT:
- *icmptype = ICMP_ROUTERSOLICIT;
- *icmpid = 0; /* Nothing sane to match on! */
- break;
+ case ICMP_ROUTERSOLICIT:
+ *icmp_dir = PF_IN;
+ case ICMP_ROUTERADVERT:
+ *virtual_type = ICMP_ROUTERSOLICIT;
+ *virtual_id = 0; /* Nothing sane to match on! */
+ break;
-#ifdef INET6
- case ICMP6_ECHO_REQUEST:
- *icmp_dir = PF_IN;
- case ICMP6_ECHO_REPLY:
- *icmptype = ICMP6_ECHO_REQUEST;
- *icmpid = pd->hdr.icmp6.icmp6_id;
- break;
+ /* These ICMP types map to other connections */
+ case ICMP_UNREACH:
+ case ICMP_SOURCEQUENCH:
+ case ICMP_REDIRECT:
+ case ICMP_TIMXCEED:
+ case ICMP_PARAMPROB:
+ /* These will not be used, but set them anyway */
+ *icmp_dir = PF_IN;
+ *virtual_type = type;
+ *virtual_id = 0;
+ HTONS(*virtual_type);
+ return (1); /* These types match to another state */
- case MLD_LISTENER_QUERY:
- *icmp_dir = PF_IN;
- case MLD_LISTENER_REPORT: {
- *icmptype = MLD_LISTENER_QUERY;
- *icmpid = 0;
+ /*
+ * All remaining ICMP types get their own states,
+ * and will only match in one direction.
+ */
+ default:
+ *icmp_dir = PF_IN;
+ *virtual_type = type;
+ *virtual_id = 0;
+ break;
+ }
break;
- }
+#endif /* INET */
+#ifdef INET6
+ case AF_INET6:
+ switch (type) {
+ case ICMP6_ECHO_REQUEST:
+ *icmp_dir = PF_IN;
+ case ICMP6_ECHO_REPLY:
+ *virtual_type = ICMP6_ECHO_REQUEST;
+ *virtual_id = pd->hdr.icmp6.icmp6_id;
+ break;
- /* ICMP6_FQDN and ICMP6_NI query/reply are the same type as ICMP6_WRU */
- case ICMP6_WRUREQUEST:
- *icmp_dir = PF_IN;
- case ICMP6_WRUREPLY:
- *icmptype = ICMP6_WRUREQUEST;
- *icmpid = 0; /* Nothing sane to match on! */
- break;
+ case MLD_LISTENER_QUERY:
+ *icmp_dir = PF_IN;
+ case MLD_LISTENER_REPORT: {
+ *virtual_type = MLD_LISTENER_QUERY;
+ *virtual_id = 0;
+ break;
+ }
+ case MLD_MTRACE:
+ *icmp_dir = PF_IN;
+ case MLD_MTRACE_RESP:
+ *virtual_type = MLD_MTRACE;
+ *virtual_id = 0; /* Nothing sane to match on! */
+ break;
- case MLD_MTRACE:
- *icmp_dir = PF_IN;
- case MLD_MTRACE_RESP:
- *icmptype = MLD_MTRACE;
- *icmpid = 0; /* Nothing sane to match on! */
- break;
+ case ND_NEIGHBOR_SOLICIT:
+ *icmp_dir = PF_IN;
+ case ND_NEIGHBOR_ADVERT: {
+ *virtual_type = ND_NEIGHBOR_SOLICIT;
+ *virtual_id = 0;
+ break;
+ }
- case ND_NEIGHBOR_SOLICIT:
- *icmp_dir = PF_IN;
- case ND_NEIGHBOR_ADVERT: {
- *icmptype = ND_NEIGHBOR_SOLICIT;
- *multi = PF_ICMP_MULTI_SOLICITED;
- *icmpid = 0;
+ /*
+ * These ICMP types map to other connections.
+ * ND_REDIRECT can't be in this list because the triggering
+ * packet header is optional.
+ */
+ case ICMP6_DST_UNREACH:
+ case ICMP6_PACKET_TOO_BIG:
+ case ICMP6_TIME_EXCEEDED:
+ case ICMP6_PARAM_PROB:
+ /* These will not be used, but set them anyway */
+ *icmp_dir = PF_IN;
+ *virtual_type = type;
+ *virtual_id = 0;
+ HTONS(*virtual_type);
+ return (1); /* These types match to another state */
+ /*
+ * All remaining ICMP6 types get their own states,
+ * and will only match in one direction.
+ */
+ default:
+ *icmp_dir = PF_IN;
+ *virtual_type = type;
+ *virtual_id = 0;
+ break;
+ }
break;
- }
-
#endif /* INET6 */
- /* These ICMP types map to other connections */
- case ICMP_UNREACH:
- case ICMP_SOURCEQUENCH:
- case ICMP_REDIRECT:
- case ICMP_TIMXCEED:
- case ICMP_PARAMPROB:
-#ifdef INET6
- /*
- * ICMP6_TIME_EXCEEDED is the same type as ICMP_UNREACH
- * ND_REDIRECT can't be in this list because the triggering packet
- * header is optional.
- */
- case ICMP6_PACKET_TOO_BIG:
-#endif /* INET6 */
- /* These will not be used, but set them anyways */
- *icmp_dir = PF_IN;
- *icmptype = htons(type);
- *icmpid = 0;
- return (1); /* These types are matched to other state */
- /*
- * All remaining ICMP types get their own states,
- * and will only match in one direction.
- */
default:
*icmp_dir = PF_IN;
- *icmptype = type;
- *icmpid = 0;
+ *virtual_type = type;
+ *virtual_id = 0;
break;
}
- HTONS(*icmptype);
- return (0);
+ HTONS(*virtual_type);
+ return (0); /* These types match to their own state */
}
void
@@ -4156,7 +4179,7 @@ pf_test_rule(struct pf_krule **rm, struct pf_kstate **sm, int direction,
pf_change_a(&daddr->v4.s_addr, pd->ip_sum,
nk->addr[pd->didx].v4.s_addr, 0);
- if (virtual_type == ICMP_ECHO &&
+ if (virtual_type == htons(ICMP_ECHO) &&
nk->port[pd->sidx] != pd->hdr.icmp.icmp_id) {
pd->hdr.icmp.icmp_cksum = pf_cksum_fixup(
pd->hdr.icmp.icmp_cksum, sport,
@@ -6483,13 +6506,13 @@ pf_test_state_icmp(struct pf_kstate **state, int direction, struct pfi_kkif *kif
if (PF_ANEQ(pd2.src,
&nk->addr[pd2.sidx], pd2.af) ||
- (virtual_type == ICMP_ECHO &&
+ (virtual_type == htons(ICMP_ECHO) &&
nk->port[iidx] != iih.icmp_id))
pf_change_icmp(pd2.src,
- (virtual_type == ICMP_ECHO) ?
+ (virtual_type == htons(ICMP_ECHO)) ?
&iih.icmp_id : NULL,
daddr, &nk->addr[pd2.sidx],
- (virtual_type == ICMP_ECHO) ?
+ (virtual_type == htons(ICMP_ECHO)) ?
nk->port[iidx] : 0, NULL,
pd2.ip_sum, icmpsum,
pd->ip_sum, 0, AF_INET);
@@ -6549,13 +6572,13 @@ pf_test_state_icmp(struct pf_kstate **state, int direction, struct pfi_kkif *kif
if (PF_ANEQ(pd2.src,
&nk->addr[pd2.sidx], pd2.af) ||
- ((virtual_type == ICMP6_ECHO_REQUEST) &&
+ ((virtual_type == htons(ICMP6_ECHO_REQUEST)) &&
nk->port[pd2.sidx] != iih.icmp6_id))
pf_change_icmp(pd2.src,
- (virtual_type == ICMP6_ECHO_REQUEST)
+ (virtual_type == htons(ICMP6_ECHO_REQUEST))
? &iih.icmp6_id : NULL,
daddr, &nk->addr[pd2.sidx],
- (virtual_type == ICMP6_ECHO_REQUEST)
+ (virtual_type == htons(ICMP6_ECHO_REQUEST))
? nk->port[iidx] : 0, NULL,
pd2.ip_sum, icmpsum,
pd->ip_sum, 0, AF_INET6);