Date: Wed, 29 Dec 2021 12:56:19 GMT From: "Bjoern A. Zeeb" <bz@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org Subject: git: a7e7700fa741 - stable/13 - IPv4: fix redirect sending conditions Message-ID: <202112291256.1BTCuJZr074309@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch stable/13 has been updated by bz: URL: https://cgit.FreeBSD.org/src/commit/?id=a7e7700fa741d64a31e9d7596175fc0461687b86 commit a7e7700fa741d64a31e9d7596175fc0461687b86 Author: Bjoern A. Zeeb <bz@FreeBSD.org> AuthorDate: 2021-12-26 15:33:48 +0000 Commit: Bjoern A. Zeeb <bz@FreeBSD.org> CommitDate: 2021-12-29 10:52:52 +0000 IPv4: fix redirect sending conditions RFC792,1009,1122 state the original conditions for sending a redirect. RFC1812 further refine these. ip_forward() still sepcifies the checks originally implemented for these (we do slightly more/different than suggested as makes sense). The implementation added in 8ad114c082a159c0dde95aa35d2e3e108aa30a75 to ip_tryforward() however is flawed and may send a "multi-hop" redirects (to a host not on the directly connected network). Do proper checks in ip_tryforward() to stop us from sending redirects in situations we may not. Keep as much logic out of ip_tryforward() and in ip_redir_alloc() and only do the mbuf copy once we are sure we will send a redirect. While here enhance and fix comments as to which conditions are handled for sending redirects in various places. Reported by: pi (on net@ 2021-12-04) Sponsored by: Dr.-Ing. Nepustil & Co. GmbH Reviewed by: cy, others (earlier versions) Differential Revision: https://reviews.freebsd.org/D33274 (cherry picked from commit f389439f50fc4c27d15d3017b622270e25ba71c7) --- sys/netinet/ip_fastfwd.c | 101 +++++++++++++++++++++++++++++++---------------- sys/netinet/ip_input.c | 9 ++++- 2 files changed, 75 insertions(+), 35 deletions(-) diff --git a/sys/netinet/ip_fastfwd.c b/sys/netinet/ip_fastfwd.c index facf876f18cc..95a601ced3ef 100644 --- a/sys/netinet/ip_fastfwd.c +++ b/sys/netinet/ip_fastfwd.c @@ -60,14 +60,6 @@ * * We take full advantage of hardware support for IP checksum and * fragmentation offloading. - * - * We don't do ICMP redirect in the fast forwarding path. I have had my own - * cases where two core routers with Zebra routing suite would send millions - * ICMP redirects to connected hosts if the destination router was not the - * default gateway. In one case it was filling the routing table of a host - * with approximately 300.000 cloned redirect entries until it ran out of - * kernel memory. However the networking code proved very robust and it didn't - * crash or fail in other ways. */ /* @@ -114,11 +106,68 @@ __FBSDID("$FreeBSD$"); #define V_ipsendredirects VNET(ipsendredirects) static struct mbuf * -ip_redir_alloc(struct mbuf *m, struct nhop_object *nh, - struct ip *ip, in_addr_t *addr) +ip_redir_alloc(struct mbuf *m, struct nhop_object *nh, u_short ip_len, + struct in_addr *osrc, struct in_addr *newgw) { - struct mbuf *mcopy = m_gethdr(M_NOWAIT, m->m_type); + struct in_ifaddr *nh_ia; + struct mbuf *mcopy; + + KASSERT(nh != NULL, ("%s: m %p nh is NULL\n", __func__, m)); + + /* + * Only send a redirect if: + * - Redirects are not disabled (must be checked by caller), + * - We have not applied NAT (must be checked by caller as possible), + * - Neither a MCAST or BCAST packet (must be checked by caller) + * [RFC1009 Appendix A.2]. + * - The packet does not do IP source routing or having any other + * IP options (this case was handled already by ip_input() calling + * ip_dooptions() [RFC792, p13], + * - The packet is being forwarded out the same physical interface + * that it was received from [RFC1812, 5.2.7.2]. + */ + + /* + * - The forwarding route was not created by a redirect + * [RFC1812, 5.2.7.2], or + * if it was to follow a default route (see below). + * - The next-hop is reachable by us [RFC1009 Appendix A.2]. + */ + if ((nh->nh_flags & (NHF_DEFAULT | NHF_REDIRECT | + NHF_BLACKHOLE | NHF_REJECT)) != 0) + return (NULL); + + /* Get the new gateway. */ + if ((nh->nh_flags & NHF_GATEWAY) == 0 || nh->gw_sa.sa_family != AF_INET) + return (NULL); + newgw->s_addr = nh->gw4_sa.sin_addr.s_addr; + + /* + * - The resulting forwarding destination is not "This host on this + * network" [RFC1122, Section 3.2.1.3] (default route check above). + */ + if (newgw->s_addr == 0) + return (NULL); + + /* + * - We know how to reach the sender and the source address is + * directly connected to us [RFC792, p13]. + * + The new gateway address and the source address are on the same + * subnet [RFC1009 Appendix A.2, RFC1122 3.2.2.2, RFC1812, 5.2.7.2]. + * NB: if you think multiple logical subnets on the same wire should + * receive redirects read [RFC1812, APPENDIX C (14->15)]. + */ + nh_ia = (struct in_ifaddr *)nh->nh_ifa; + if ((ntohl(osrc->s_addr) & nh_ia->ia_subnetmask) != nh_ia->ia_subnet) + return (NULL); + + /* Prepare for sending the redirect. */ + /* + * Make a copy of as much as we need of the packet as the original + * one will be forwarded but we need (a portion) for icmp_error(). + */ + mcopy = m_gethdr(M_NOWAIT, m->m_type); if (mcopy == NULL) return (NULL); @@ -132,23 +181,10 @@ ip_redir_alloc(struct mbuf *m, struct nhop_object *nh, m_free(mcopy); return (NULL); } - mcopy->m_len = min(ntohs(ip->ip_len), M_TRAILINGSPACE(mcopy)); + mcopy->m_len = min(ip_len, M_TRAILINGSPACE(mcopy)); mcopy->m_pkthdr.len = mcopy->m_len; m_copydata(m, 0, mcopy->m_len, mtod(mcopy, caddr_t)); - if (nh != NULL && - ((nh->nh_flags & (NHF_REDIRECT|NHF_DEFAULT)) == 0)) { - struct in_ifaddr *nh_ia = (struct in_ifaddr *)(nh->nh_ifa); - u_long src = ntohl(ip->ip_src.s_addr); - - if (nh_ia != NULL && - (src & nh_ia->ia_subnetmask) == nh_ia->ia_subnet) { - if (nh->nh_flags & NHF_GATEWAY) - *addr = nh->gw4_sa.sin_addr.s_addr; - else - *addr = ip->ip_dst.s_addr; - } - } return (mcopy); } @@ -202,7 +238,7 @@ ip_tryforward(struct mbuf *m) struct route ro; struct sockaddr_in *dst; const struct sockaddr *gw; - struct in_addr dest, odest, rtdest; + struct in_addr dest, odest, rtdest, osrc; uint16_t ip_len, ip_off; int error = 0; struct m_tag *fwd_tag = NULL; @@ -274,6 +310,7 @@ ip_tryforward(struct mbuf *m) */ odest.s_addr = dest.s_addr = ip->ip_dst.s_addr; + osrc.s_addr = ip->ip_src.s_addr; /* * Run through list of ipfilter hooks for input packets @@ -434,13 +471,11 @@ passout: } else gw = (const struct sockaddr *)dst; - /* - * Handle redirect case. - */ + /* Handle redirect case. */ redest.s_addr = 0; - if (V_ipsendredirects && (nh->nh_ifp == m->m_pkthdr.rcvif) && - gw->sa_family == AF_INET) - mcopy = ip_redir_alloc(m, nh, ip, &redest.s_addr); + if (V_ipsendredirects && osrc.s_addr == ip->ip_src.s_addr && + nh->nh_ifp == m->m_pkthdr.rcvif) + mcopy = ip_redir_alloc(m, nh, ip_len, &osrc, &redest); /* * Check if packet fits MTU or if hardware will fragment for us @@ -514,7 +549,7 @@ passout: /* Send required redirect */ if (mcopy != NULL) { icmp_error(mcopy, ICMP_REDIRECT, ICMP_REDIRECT_HOST, redest.s_addr, 0); - mcopy = NULL; /* Freed by caller */ + mcopy = NULL; /* Was consumed by callee. */ } consumed: diff --git a/sys/netinet/ip_input.c b/sys/netinet/ip_input.c index 3d3c350ded85..62de3ef92bf2 100644 --- a/sys/netinet/ip_input.c +++ b/sys/netinet/ip_input.c @@ -562,8 +562,9 @@ tooshort: /* * Try to forward the packet, but if we fail continue. - * ip_tryforward() does not generate redirects, so fall - * through to normal processing if redirects are required. + * ip_tryforward() may generate redirects these days. + * XXX the logic below falling through to normal processing + * if redirects are required should be revisited as well. * ip_tryforward() does inbound and outbound packet firewall * processing. If firewall has decided that destination becomes * our local address, it sets M_FASTFWD_OURS flag. In this @@ -576,6 +577,10 @@ tooshort: IPSEC_CAPS(ipv4, m, IPSEC_CAP_OPERABLE) == 0) #endif ) { + /* + * ip_dooptions() was run so we can ignore the source route (or + * any IP options case) case for redirects in ip_tryforward(). + */ if ((m = ip_tryforward(m)) == NULL) return; if (m->m_flags & M_FASTFWD_OURS) {
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202112291256.1BTCuJZr074309>