From d0d18dbbaba27b342bbb10df89e75d2156c136fe Mon Sep 17 00:00:00 2001 From: Franco Fichtner Date: Mon, 9 Sep 2024 21:09:48 +0200 Subject: [PATCH] Revert "pf: be less strict about icmp state checking for sloppy state tracking" This reverts commit 781221f084cdf971ec59c73e16a4f7ef36798eaf. Revert "pf tests: ensure that neighbour discovery works as expected" This reverts commit 631d6e5300563d64ede3d2304275e22eab17af63. Revert "pf: fully annotated patch of disabling state tracking and issues for ND" This reverts commit f8582728966ff4117f52fcecb6e697faf1b37951. Revert "pf: invert direction for inner icmp state lookups" This reverts commit c61a3c23fb1a8977c0d6e8ab0ac1f717c4bb072e. Revert "pf tests: ensure that traceroutes using ICMP work" This reverts commit 9c5396516962903d8a86352875f2fea5f521fd00. Revert "pf: fix icmp-in-icmp state lookup" This reverts commit e854cb47897c953471fd9004eadf3b1c34113f91. Revert "pf: allow MLD LR to be sent without state" This reverts commit 9b2e3cf60b23fac18c578b14b2c9cf74854bd3d2. Revert "pf: split ICMP/ICMPv6 number space in pf_icmp_mapping()" This reverts commit ee1b7126a95fec2b67d1310f4c223ca919fcdb1a. Revert "pf: some ICMP types that also have icmp_id, pointed out by markus@" This reverts commit c21004ce412d6928e3e8ab78da0875b26362ae6f. Revert "pf: stricter state checking for ICMP and ICMPv6 packets" This reverts commit 7f1f57ed78d3bff644937fb1e64a169e85d0b48b. PR: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=280701 --- sys/netpfil/pf/pf.c | 430 +++++++--------------------------- sys/netpfil/pf/pf_lb.c | 19 +- tests/sys/netpfil/pf/icmp.sh | 67 ------ tests/sys/netpfil/pf/icmp6.sh | 115 --------- 4 files changed, 88 insertions(+), 543 deletions(-) diff --git a/sys/netpfil/pf/pf.c b/sys/netpfil/pf/pf.c index bedb3ae1239..a731dbc4b59 100644 --- a/sys/netpfil/pf/pf.c +++ b/sys/netpfil/pf/pf.c @@ -295,8 +295,6 @@ static void pf_change_ap(struct mbuf *, struct pf_addr *, u_int16_t *, u_int16_t, u_int8_t, sa_family_t); static int pf_modulate_sack(struct mbuf *, int, struct pf_pdesc *, struct tcphdr *, struct pf_state_peer *); -int pf_icmp_mapping(struct pf_pdesc *, u_int8_t, int *, - int *, u_int16_t *, u_int16_t *); static void pf_change_icmp(struct pf_addr *, u_int16_t *, struct pf_addr *, struct pf_addr *, u_int16_t, u_int16_t *, u_int16_t *, u_int16_t *, @@ -343,10 +341,6 @@ static int pf_test_state_tcp(struct pf_kstate **, static int pf_test_state_udp(struct pf_kstate **, struct pfi_kkif *, struct mbuf *, int, void *, struct pf_pdesc *); -int pf_icmp_state_lookup(struct pf_state_key_cmp *, - struct pf_pdesc *, struct pf_kstate **, struct mbuf *, - int, struct pfi_kkif *, u_int16_t, u_int16_t, - int, int *, int, int); static int pf_test_state_icmp(struct pf_kstate **, struct pfi_kkif *, struct mbuf *, int, void *, struct pf_pdesc *, u_short *); @@ -399,8 +393,6 @@ extern struct proc *pf_purge_proc; VNET_DEFINE(struct pf_limit, pf_limits[PF_LIMIT_MAX]); -enum { PF_ICMP_MULTI_NONE, PF_ICMP_MULTI_SOLICITED, PF_ICMP_MULTI_LINK }; - #define PACKET_UNDO_NAT(_m, _pd, _off, _s) \ do { \ struct pf_state_key *nk; \ @@ -1767,174 +1759,6 @@ pf_isforlocal(struct mbuf *m, int af) return (false); } -int -pf_icmp_mapping(struct pf_pdesc *pd, u_int8_t type, - 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 - * PF_IN, and will match states in the opposite direction. - * PF_IN ICMP types need to match a state with that type. - */ - *icmp_dir = PF_OUT; - *multi = PF_ICMP_MULTI_LINK; - /* Queries (and responses) */ - 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: - *virtual_type = ICMP_TSTAMP; - *virtual_id = 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: - *virtual_type = ICMP_MASKREQ; - *virtual_id = pd->hdr.icmp.icmp_id; - 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: - *virtual_type = ICMP_MOBILE_REGREQUEST; - *virtual_id = 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; - - /* 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 */ - - /* - * 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; - - case MLD_LISTENER_QUERY: - case MLD_LISTENER_REPORT: { - /* - * Listener Report can be sent by clients - * without an associated Listener Query. - * In addition to that, when Report is sent as a - * reply to a Query its source and destination - * address are different. - */ - *icmp_dir = PF_IN; - *virtual_type = MLD_LISTENER_QUERY; - *virtual_id = 0; /* XXX missing fake id */ - 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 ND_NEIGHBOR_SOLICIT: - *icmp_dir = PF_IN; - case ND_NEIGHBOR_ADVERT: { - *virtual_type = ND_NEIGHBOR_SOLICIT; - *multi = PF_ICMP_MULTI_SOLICITED; - *virtual_id = 0; /* XXX missing fake id */ - HTONS(*virtual_type); return (1); /* XXX disable state tracking for now */ - break; - } - - /* - * 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 */ - default: - *icmp_dir = PF_IN; - *virtual_type = type; - *virtual_id = 0; - break; - } - HTONS(*virtual_type); - return (0); /* These types match to their own state */ -} - void pf_intr(void *v) { @@ -4598,8 +4422,8 @@ pf_test_rule(struct pf_krule **rm, struct pf_kstate **sm, struct pfi_kkif *kif, int tag = -1; int asd = 0; int match = 0; - int state_icmp = 0, icmp_dir, multi; - u_int16_t sport = 0, dport = 0, virtual_type, virtual_id; + int state_icmp = 0; + u_int16_t sport = 0, dport = 0; u_int16_t bproto_sum = 0, bip_sum = 0; u_int8_t icmptype = 0, icmpcode = 0; struct pf_kanchor_stackframe anchor_stack[PF_ANCHOR_STACKSIZE]; @@ -4633,37 +4457,33 @@ pf_test_rule(struct pf_krule **rm, struct pf_kstate **sm, struct pfi_kkif *kif, case IPPROTO_ICMP: if (pd->af != AF_INET) break; + sport = dport = pd->hdr.icmp.icmp_id; hdrlen = sizeof(pd->hdr.icmp); icmptype = pd->hdr.icmp.icmp_type; icmpcode = pd->hdr.icmp.icmp_code; - state_icmp = pf_icmp_mapping(pd, icmptype, - &icmp_dir, &multi, &virtual_id, &virtual_type); - if (icmp_dir == PF_IN) { - sport = virtual_id; - dport = virtual_type; - } else { - sport = virtual_type; - dport = virtual_id; - } + + if (icmptype == ICMP_UNREACH || + icmptype == ICMP_SOURCEQUENCH || + icmptype == ICMP_REDIRECT || + icmptype == ICMP_TIMXCEED || + icmptype == ICMP_PARAMPROB) + state_icmp++; break; #endif /* INET */ #ifdef INET6 case IPPROTO_ICMPV6: if (af != AF_INET6) break; + sport = dport = pd->hdr.icmp6.icmp6_id; hdrlen = sizeof(pd->hdr.icmp6); icmptype = pd->hdr.icmp6.icmp6_type; icmpcode = pd->hdr.icmp6.icmp6_code; - state_icmp = pf_icmp_mapping(pd, icmptype, - &icmp_dir, &multi, &virtual_id, &virtual_type); - if (icmp_dir == PF_IN) { - sport = virtual_id; - dport = virtual_type; - } else { - sport = virtual_type; - dport = virtual_id; - } + if (icmptype == ICMP6_DST_UNREACH || + icmptype == ICMP6_PACKET_TOO_BIG || + icmptype == ICMP6_TIME_EXCEEDED || + icmptype == ICMP6_PARAM_PROB) + state_icmp++; break; #endif /* INET6 */ default: @@ -4757,6 +4577,7 @@ pf_test_rule(struct pf_krule **rm, struct pf_kstate **sm, struct pfi_kkif *kif, } #ifdef INET case IPPROTO_ICMP: + nk->port[0] = nk->port[1]; if (PF_ANEQ(saddr, &nk->addr[pd->sidx], AF_INET)) pf_change_a(&saddr->v4.s_addr, pd->ip_sum, nk->addr[pd->sidx].v4.s_addr, 0); @@ -4765,12 +4586,11 @@ pf_test_rule(struct pf_krule **rm, struct pf_kstate **sm, struct pfi_kkif *kif, pf_change_a(&daddr->v4.s_addr, pd->ip_sum, nk->addr[pd->didx].v4.s_addr, 0); - if (virtual_type == htons(ICMP_ECHO) && - nk->port[pd->sidx] != pd->hdr.icmp.icmp_id) { + if (nk->port[1] != pd->hdr.icmp.icmp_id) { pd->hdr.icmp.icmp_cksum = pf_cksum_fixup( pd->hdr.icmp.icmp_cksum, sport, - nk->port[pd->sidx], 0); - pd->hdr.icmp.icmp_id = nk->port[pd->sidx]; + nk->port[1], 0); + pd->hdr.icmp.icmp_id = nk->port[1]; pd->sport = &pd->hdr.icmp.icmp_id; } m_copyback(m, off, ICMP_MINLEN, (caddr_t)&pd->hdr.icmp); @@ -4778,6 +4598,7 @@ pf_test_rule(struct pf_krule **rm, struct pf_kstate **sm, struct pfi_kkif *kif, #endif /* INET */ #ifdef INET6 case IPPROTO_ICMPV6: + nk->port[0] = nk->port[1]; if (PF_ANEQ(saddr, &nk->addr[pd->sidx], AF_INET6)) pf_change_a6(saddr, &pd->hdr.icmp6.icmp6_cksum, &nk->addr[pd->sidx], 0); @@ -6619,78 +6440,15 @@ pf_multihome_scan_asconf(struct mbuf *m, int start, int len, return (pf_multihome_scan(m, start, len, pd, kif, SCTP_ADD_IP_ADDRESS)); } -int -pf_icmp_state_lookup(struct pf_state_key_cmp *key, struct pf_pdesc *pd, - struct pf_kstate **state, struct mbuf *m, int direction, struct pfi_kkif *kif, - u_int16_t icmpid, u_int16_t type, int icmp_dir, int *iidx, int multi, - int inner) -{ - key->af = pd->af; - key->proto = pd->proto; - if (icmp_dir == PF_IN) { - *iidx = pd->sidx; - key->port[pd->sidx] = icmpid; - key->port[pd->didx] = type; - } else { - *iidx = pd->didx; - key->port[pd->sidx] = type; - key->port[pd->didx] = icmpid; - } - if (pd->af == AF_INET6 && multi != PF_ICMP_MULTI_NONE) { - switch (multi) { - case PF_ICMP_MULTI_SOLICITED: - key->addr[pd->sidx].addr32[0] = IPV6_ADDR_INT32_MLL; - key->addr[pd->sidx].addr32[1] = 0; - key->addr[pd->sidx].addr32[2] = IPV6_ADDR_INT32_ONE; - key->addr[pd->sidx].addr32[3] = pd->src->addr32[3]; - key->addr[pd->sidx].addr8[12] = 0xff; - break; - case PF_ICMP_MULTI_LINK: - key->addr[pd->sidx].addr32[0] = IPV6_ADDR_INT32_MLL; - key->addr[pd->sidx].addr32[1] = 0; - key->addr[pd->sidx].addr32[2] = 0; - key->addr[pd->sidx].addr32[3] = IPV6_ADDR_INT32_ONE; - break; - } - } else - PF_ACPY(&key->addr[pd->sidx], pd->src, key->af); - PF_ACPY(&key->addr[pd->didx], pd->dst, key->af); - - STATE_LOOKUP(kif, key, *state, pd); - - if ((*state)->state_flags & PFSTATE_SLOPPY) - return (-1); - - /* Is this ICMP message flowing in right direction? */ - if ((*state)->rule.ptr->type && - (((!inner && (*state)->direction == direction) || - (inner && (*state)->direction != direction)) ? - PF_IN : PF_OUT) != icmp_dir) { - if (V_pf_status.debug >= PF_DEBUG_MISC) { - printf("pf: icmp type %d in wrong direction (%d): ", - icmp_dir, pd->dir); - pf_print_state(*state); - printf("\n"); - } - return (PF_DROP); - } - return (-1); -} - static int pf_test_state_icmp(struct pf_kstate **state, struct pfi_kkif *kif, struct mbuf *m, int off, void *h, struct pf_pdesc *pd, u_short *reason) { struct pf_addr *saddr = pd->src, *daddr = pd->dst; - u_int16_t *icmpsum, virtual_id, virtual_type; + u_int16_t icmpid = 0, *icmpsum; u_int8_t icmptype, icmpcode; - int icmp_dir, iidx, ret, multi; + int state_icmp = 0; struct pf_state_key_cmp key; -#ifdef INET - u_int16_t icmpid; -#endif - - MPASS(*state == NULL); bzero(&key, sizeof(key)); switch (pd->proto) { @@ -6700,43 +6458,49 @@ pf_test_state_icmp(struct pf_kstate **state, struct pfi_kkif *kif, icmpcode = pd->hdr.icmp.icmp_code; icmpid = pd->hdr.icmp.icmp_id; icmpsum = &pd->hdr.icmp.icmp_cksum; + + if (icmptype == ICMP_UNREACH || + icmptype == ICMP_SOURCEQUENCH || + icmptype == ICMP_REDIRECT || + icmptype == ICMP_TIMXCEED || + icmptype == ICMP_PARAMPROB) + state_icmp++; break; #endif /* INET */ #ifdef INET6 case IPPROTO_ICMPV6: icmptype = pd->hdr.icmp6.icmp6_type; icmpcode = pd->hdr.icmp6.icmp6_code; -#ifdef INET icmpid = pd->hdr.icmp6.icmp6_id; -#endif icmpsum = &pd->hdr.icmp6.icmp6_cksum; + + if (icmptype == ICMP6_DST_UNREACH || + icmptype == ICMP6_PACKET_TOO_BIG || + icmptype == ICMP6_TIME_EXCEEDED || + icmptype == ICMP6_PARAM_PROB) + state_icmp++; break; #endif /* INET6 */ } - if (pf_icmp_mapping(pd, icmptype, &icmp_dir, &multi, - &virtual_id, &virtual_type) == 0) { + if (!state_icmp) { /* * ICMP query/reply message not related to a TCP/UDP packet. * Search for an ICMP state. */ - ret = pf_icmp_state_lookup(&key, pd, state, m, pd->dir, - kif, virtual_id, virtual_type, icmp_dir, &iidx, - PF_ICMP_MULTI_NONE, 0); - if (ret >= 0) { - if (ret == PF_DROP && pd->af == AF_INET6 && - icmp_dir == PF_OUT) { - if (*state != NULL) - PF_STATE_UNLOCK((*state)); - ret = pf_icmp_state_lookup(&key, pd, state, m, - pd->dir, kif, virtual_id, virtual_type, - icmp_dir, &iidx, multi, 0); - if (ret >= 0) - return (ret); - } else - return (ret); + key.af = pd->af; + key.proto = pd->proto; + key.port[0] = key.port[1] = icmpid; + if (pd->dir == PF_IN) { /* wire side, straight */ + PF_ACPY(&key.addr[0], pd->src, key.af); + PF_ACPY(&key.addr[1], pd->dst, key.af); + } else { /* stack side, reverse */ + PF_ACPY(&key.addr[1], pd->src, key.af); + PF_ACPY(&key.addr[0], pd->dst, key.af); } + STATE_LOOKUP(kif, &key, *state, pd); + (*state)->expire = time_uptime; (*state)->timeout = PFTM_ICMP_ERROR_REPLY; @@ -6759,14 +6523,14 @@ pf_test_state_icmp(struct pf_kstate **state, struct pfi_kkif *kif, pd->ip_sum, nk->addr[pd->didx].v4.s_addr, 0); - if (nk->port[iidx] != + if (nk->port[0] != pd->hdr.icmp.icmp_id) { pd->hdr.icmp.icmp_cksum = pf_cksum_fixup( pd->hdr.icmp.icmp_cksum, icmpid, - nk->port[iidx], 0); + nk->port[pd->sidx], 0); pd->hdr.icmp.icmp_id = - nk->port[iidx]; + nk->port[pd->sidx]; } m_copyback(m, off, ICMP_MINLEN, @@ -6814,7 +6578,6 @@ pf_test_state_icmp(struct pf_kstate **state, struct pfi_kkif *kif, int off2 = 0; pd2.af = pd->af; - pd2.dir = pd->dir; /* Payload packet is from the opposite direction. */ pd2.sidx = (pd->dir == PF_IN) ? 1 : 0; pd2.didx = (pd->dir == PF_IN) ? 0 : 1; @@ -7122,9 +6885,9 @@ pf_test_state_icmp(struct pf_kstate **state, struct pfi_kkif *kif, } #ifdef INET case IPPROTO_ICMP: { - struct icmp *iih = &pd2.hdr.icmp; + struct icmp iih; - if (!pf_pull_hdr(m, off2, iih, ICMP_MINLEN, + if (!pf_pull_hdr(m, off2, &iih, ICMP_MINLEN, NULL, reason, pd2.af)) { DPFPRINTF(PF_DEBUG_MISC, ("pf: ICMP error message too short i" @@ -7132,15 +6895,13 @@ pf_test_state_icmp(struct pf_kstate **state, struct pfi_kkif *kif, return (PF_DROP); } - icmpid = iih->icmp_id; - pf_icmp_mapping(&pd2, iih->icmp_type, - &icmp_dir, &multi, &virtual_id, &virtual_type); + key.af = pd2.af; + key.proto = IPPROTO_ICMP; + PF_ACPY(&key.addr[pd2.sidx], pd2.src, key.af); + PF_ACPY(&key.addr[pd2.didx], pd2.dst, key.af); + key.port[0] = key.port[1] = iih.icmp_id; - ret = pf_icmp_state_lookup(&key, &pd2, state, m, - pd2.dir, kif, virtual_id, virtual_type, - icmp_dir, &iidx, PF_ICMP_MULTI_NONE, 1); - if (ret >= 0) - return (ret); + STATE_LOOKUP(kif, &key, *state, pd); /* translate source/destination address, if necessary */ if ((*state)->key[PF_SK_WIRE] != @@ -7150,27 +6911,25 @@ pf_test_state_icmp(struct pf_kstate **state, struct pfi_kkif *kif, if (PF_ANEQ(pd2.src, &nk->addr[pd2.sidx], pd2.af) || - (virtual_type == htons(ICMP_ECHO) && - nk->port[iidx] != iih->icmp_id)) - pf_change_icmp(pd2.src, - (virtual_type == htons(ICMP_ECHO)) ? - &iih->icmp_id : NULL, + nk->port[pd2.sidx] != iih.icmp_id) + pf_change_icmp(pd2.src, &iih.icmp_id, daddr, &nk->addr[pd2.sidx], - (virtual_type == htons(ICMP_ECHO)) ? - nk->port[iidx] : 0, NULL, + nk->port[pd2.sidx], NULL, pd2.ip_sum, icmpsum, pd->ip_sum, 0, AF_INET); if (PF_ANEQ(pd2.dst, - &nk->addr[pd2.didx], pd2.af)) - pf_change_icmp(pd2.dst, NULL, NULL, - &nk->addr[pd2.didx], 0, NULL, - pd2.ip_sum, icmpsum, pd->ip_sum, 0, - AF_INET); + &nk->addr[pd2.didx], pd2.af) || + nk->port[pd2.didx] != iih.icmp_id) + pf_change_icmp(pd2.dst, &iih.icmp_id, + saddr, &nk->addr[pd2.didx], + nk->port[pd2.didx], NULL, + pd2.ip_sum, icmpsum, + pd->ip_sum, 0, AF_INET); m_copyback(m, off, ICMP_MINLEN, (caddr_t)&pd->hdr.icmp); m_copyback(m, ipoff2, sizeof(h2), (caddr_t)&h2); - m_copyback(m, off2, ICMP_MINLEN, (caddr_t)iih); + m_copyback(m, off2, ICMP_MINLEN, (caddr_t)&iih); } return (PF_PASS); break; @@ -7178,9 +6937,9 @@ pf_test_state_icmp(struct pf_kstate **state, struct pfi_kkif *kif, #endif /* INET */ #ifdef INET6 case IPPROTO_ICMPV6: { - struct icmp6_hdr *iih = &pd2.hdr.icmp6; + struct icmp6_hdr iih; - if (!pf_pull_hdr(m, off2, iih, + if (!pf_pull_hdr(m, off2, &iih, sizeof(struct icmp6_hdr), NULL, reason, pd2.af)) { DPFPRINTF(PF_DEBUG_MISC, ("pf: ICMP error message too short " @@ -7188,26 +6947,13 @@ pf_test_state_icmp(struct pf_kstate **state, struct pfi_kkif *kif, return (PF_DROP); } - pf_icmp_mapping(&pd2, iih->icmp6_type, - &icmp_dir, &multi, &virtual_id, &virtual_type); + key.af = pd2.af; + key.proto = IPPROTO_ICMPV6; + PF_ACPY(&key.addr[pd2.sidx], pd2.src, key.af); + PF_ACPY(&key.addr[pd2.didx], pd2.dst, key.af); + key.port[0] = key.port[1] = iih.icmp6_id; - ret = pf_icmp_state_lookup(&key, &pd2, state, m, - pd->dir, kif, virtual_id, virtual_type, - icmp_dir, &iidx, PF_ICMP_MULTI_NONE, 1); - if (ret >= 0) { - if (ret == PF_DROP && pd->af == AF_INET6 && - icmp_dir == PF_OUT) { - if (*state != NULL) - PF_STATE_UNLOCK((*state)); - ret = pf_icmp_state_lookup(&key, pd, - state, m, pd->dir, kif, - virtual_id, virtual_type, - icmp_dir, &iidx, multi, 1); - if (ret >= 0) - return (ret); - } else - return (ret); - } + STATE_LOOKUP(kif, &key, *state, pd); /* translate source/destination address, if necessary */ if ((*state)->key[PF_SK_WIRE] != @@ -7217,21 +6963,19 @@ pf_test_state_icmp(struct pf_kstate **state, struct pfi_kkif *kif, if (PF_ANEQ(pd2.src, &nk->addr[pd2.sidx], pd2.af) || - ((virtual_type == htons(ICMP6_ECHO_REQUEST)) && - nk->port[pd2.sidx] != iih->icmp6_id)) - pf_change_icmp(pd2.src, - (virtual_type == htons(ICMP6_ECHO_REQUEST)) - ? &iih->icmp6_id : NULL, + nk->port[pd2.sidx] != iih.icmp6_id) + pf_change_icmp(pd2.src, &iih.icmp6_id, daddr, &nk->addr[pd2.sidx], - (virtual_type == htons(ICMP6_ECHO_REQUEST)) - ? nk->port[iidx] : 0, NULL, + nk->port[pd2.sidx], NULL, pd2.ip_sum, icmpsum, pd->ip_sum, 0, AF_INET6); if (PF_ANEQ(pd2.dst, - &nk->addr[pd2.didx], pd2.af)) - pf_change_icmp(pd2.dst, NULL, NULL, - &nk->addr[pd2.didx], 0, NULL, + &nk->addr[pd2.didx], pd2.af) || + nk->port[pd2.didx] != iih.icmp6_id) + pf_change_icmp(pd2.dst, &iih.icmp6_id, + saddr, &nk->addr[pd2.didx], + nk->port[pd2.didx], NULL, pd2.ip_sum, icmpsum, pd->ip_sum, 0, AF_INET6); @@ -7239,7 +6983,7 @@ pf_test_state_icmp(struct pf_kstate **state, struct pfi_kkif *kif, (caddr_t)&pd->hdr.icmp6); m_copyback(m, ipoff2, sizeof(h2_6), (caddr_t)&h2_6); m_copyback(m, off2, sizeof(struct icmp6_hdr), - (caddr_t)iih); + (caddr_t)&iih); } return (PF_PASS); break; diff --git a/sys/netpfil/pf/pf_lb.c b/sys/netpfil/pf/pf_lb.c index 4fcad7e578a..eb3d087e3df 100644 --- a/sys/netpfil/pf/pf_lb.c +++ b/sys/netpfil/pf/pf_lb.c @@ -225,23 +225,6 @@ pf_get_sport(sa_family_t af, u_int8_t proto, struct pf_krule *r, if (pf_map_addr(af, r, saddr, naddr, NULL, &init_addr, sn)) return (1); - if (proto == IPPROTO_ICMP) { - if (*nport == htons(ICMP_ECHO)) { - low = 1; - high = 65535; - } else - return (0); /* Don't try to modify non-echo ICMP */ - } -#ifdef INET6 - if (proto == IPPROTO_ICMPV6) { - if (*nport == htons(ICMP6_ECHO_REQUEST)) { - low = 1; - high = 65535; - } else - return (0); /* Don't try to modify non-echo ICMP */ - } -#endif /* INET6 */ - bzero(&key, sizeof(key)); key.af = af; key.proto = proto; @@ -650,7 +633,7 @@ pf_get_translation(struct pf_pdesc *pd, struct mbuf *m, int off, switch (r->action) { case PF_NAT: if (pd->proto == IPPROTO_ICMP) { - low = 1; + low = 1; high = 65535; } else { low = r->rpool.proxy_port[0]; diff --git a/tests/sys/netpfil/pf/icmp.sh b/tests/sys/netpfil/pf/icmp.sh index f4c8ec5e583..72b531b08c5 100644 --- a/tests/sys/netpfil/pf/icmp.sh +++ b/tests/sys/netpfil/pf/icmp.sh @@ -71,74 +71,7 @@ cve_2019_5598_cleanup() pft_cleanup } -atf_test_case "ttl_exceeded" "cleanup" -ttl_exceeded_head() -{ - atf_set descr 'Test that we correctly translate TTL exceeded back' - atf_set require.user root -} - -ttl_exceeded_body() -{ - pft_init - - epair_srv=$(vnet_mkepair) - epair_int=$(vnet_mkepair) - epair_cl=$(vnet_mkepair) - - vnet_mkjail srv ${epair_srv}a - jexec srv ifconfig ${epair_srv}a 192.0.2.1/24 up - jexec srv route add default 192.0.2.2 - - vnet_mkjail int ${epair_srv}b ${epair_int}a - jexec int sysctl net.inet.ip.forwarding=1 - jexec int ifconfig ${epair_srv}b 192.0.2.2/24 up - jexec int ifconfig ${epair_int}a 203.0.113.2/24 up - - vnet_mkjail nat ${epair_int}b ${epair_cl}b - jexec nat ifconfig ${epair_int}b 203.0.113.1/24 up - jexec nat ifconfig ${epair_cl}b 198.51.100.2/24 up - jexec nat sysctl net.inet.ip.forwarding=1 - jexec nat route add default 203.0.113.2 - - vnet_mkjail cl ${epair_cl}a - jexec cl ifconfig ${epair_cl}a 198.51.100.1/24 up - jexec cl route add default 198.51.100.2 - - jexec nat pfctl -e - pft_set_rules nat \ - "nat on ${epair_int}b from 198.51.100.0/24 -> (${epair_int}b)" \ - "block" \ - "pass inet proto udp" \ - "pass inet proto icmp icmp-type { echoreq }" - - # Sanity checks - atf_check -s exit:0 -o ignore \ - jexec cl ping -c 1 198.51.100.2 - atf_check -s exit:0 -o ignore \ - jexec cl ping -c 1 203.0.113.1 - atf_check -s exit:0 -o ignore \ - jexec cl ping -c 1 203.0.113.2 - atf_check -s exit:0 -o ignore \ - jexec cl ping -c 1 192.0.2.1 - - echo "UDP" - atf_check -s exit:0 -e ignore -o match:".*203.0.113.2.*" \ - jexec cl traceroute 192.0.2.1 - jexec nat pfctl -Fs - - echo "ICMP" - atf_check -s exit:0 -e ignore -o match:".*203.0.113.2.*" \ - jexec cl traceroute -I 192.0.2.1 -} - -ttl_exceeded_cleanup() -{ - pft_cleanup -} - atf_init_test_cases() { atf_add_test_case "cve_2019_5598" - atf_add_test_case "ttl_exceeded" } diff --git a/tests/sys/netpfil/pf/icmp6.sh b/tests/sys/netpfil/pf/icmp6.sh index eb286e23ef4..35d20c83cc4 100644 --- a/tests/sys/netpfil/pf/icmp6.sh +++ b/tests/sys/netpfil/pf/icmp6.sh @@ -83,122 +83,7 @@ zero_id_cleanup() pft_cleanup } -atf_test_case "ttl_exceeded" "cleanup" -ttl_exceeded_head() -{ - atf_set descr 'Test that we correctly translate TTL exceeded back' - atf_set require.user root -} - -ttl_exceeded_body() -{ - pft_init - - epair_srv=$(vnet_mkepair) - epair_int=$(vnet_mkepair) - epair_cl=$(vnet_mkepair) - - vnet_mkjail srv ${epair_srv}a - jexec srv ifconfig ${epair_srv}a inet6 2001:db8:1::1/64 no_dad up - jexec srv route add -6 default 2001:db8:1::2 - - vnet_mkjail int ${epair_srv}b ${epair_int}a - jexec int sysctl net.inet6.ip6.forwarding=1 - jexec int ifconfig ${epair_srv}b inet6 2001:db8:1::2/64 no_dad up - jexec int ifconfig ${epair_int}a inet6 2001:db8:2::2/64 no_dad up - - vnet_mkjail nat ${epair_int}b ${epair_cl}b - jexec nat ifconfig ${epair_int}b inet6 2001:db8:2::1 no_dad up - jexec nat ifconfig ${epair_cl}b inet6 2001:db8:3::2/64 no_dad up - jexec nat sysctl net.inet6.ip6.forwarding=1 - jexec nat route add -6 default 2001:db8:2::2 - - vnet_mkjail cl ${epair_cl}a - jexec cl ifconfig ${epair_cl}a inet6 2001:db8:3::1/64 no_dad up - jexec cl route add -6 default 2001:db8:3::2 - - jexec nat pfctl -e - pft_set_rules nat \ - "nat on ${epair_int}b from 2001:db8:3::/64 -> (${epair_int}b:0)" \ - "block" \ - "pass inet6 proto udp" \ - "pass inet6 proto icmp6 icmp6-type { neighbrsol, neighbradv, echoreq }" - - # Sanity checks - atf_check -s exit:0 -o ignore \ - jexec cl ping -c 1 2001:db8:3::2 - atf_check -s exit:0 -o ignore \ - jexec cl ping -c 1 2001:db8:2::1 - atf_check -s exit:0 -o ignore \ - jexec cl ping -c 1 2001:db8:2::2 - atf_check -s exit:0 -o ignore \ - jexec cl ping -c 1 2001:db8:1::1 - - echo "UDP" - atf_check -s exit:0 -e ignore -o match:".*2001:db8:2::2.*" \ - jexec cl traceroute6 2001:db8:1::1 - jexec nat pfctl -Fs - - echo "ICMP" - atf_check -s exit:0 -e ignore -o match:".*2001:db8:2::2.*" \ - jexec cl traceroute6 -I 2001:db8:1::1 -} - -ttl_exceeded_cleanup() -{ - pft_cleanup -} - -atf_test_case "repeat" "cleanup" -repeat_head() -{ - atf_set descr 'Ensure that repeated NDs work' - atf_set require.user root - atf_set require.progs ndisc6 -} - -repeat_body() -{ - pft_init - - epair=$(vnet_mkepair) - ifconfig ${epair}a inet6 2001:db8::2/64 up no_dad - - vnet_mkjail alcatraz ${epair}b - jexec alcatraz ifconfig ${epair}b inet6 2001:db8::1/64 up no_dad - - # Sanity check - atf_check -s exit:0 -o ignore \ - ping -c 1 2001:db8::1 - - jexec alcatraz pfctl -e - pft_set_rules alcatraz \ - "block all" \ - "pass quick inet6 proto ipv6-icmp all icmp6-type neighbrsol keep state (if-bound) ridentifier 1000000107" - - jexec alcatraz pfctl -x loud - ndisc6 -m -n -r 1 2001:db8::1 ${epair}a - jexec alcatraz pfctl -ss -vv - - atf_check -s exit:0 -o ignore \ - ndisc6 -m -n -r 1 2001:db8::1 ${epair}a - jexec alcatraz pfctl -ss -vv - atf_check -s exit:0 -o ignore \ - ndisc6 -m -n -r 1 2001:db8::1 ${epair}a - jexec alcatraz pfctl -ss -vv - atf_check -s exit:0 -o ignore \ - ndisc6 -m -n -r 1 2001:db8::1 ${epair}a - jexec alcatraz pfctl -ss -vv -} - -repeat_cleanup() -{ - pft_cleanup -} - atf_init_test_cases() { atf_add_test_case "zero_id" - atf_add_test_case "ttl_exceeded" - atf_add_test_case "repeat" }