Date: Thu, 11 Mar 2021 08:26:01 GMT From: "Alexander V. Chernikov" <melifaro@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org Subject: git: 2d227a6ec371 - releng/13.0 - Fix dst/netmask handling in routing socket code. Message-ID: <202103110826.12B8Q1w0025937@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch releng/13.0 has been updated by melifaro: URL: https://cgit.FreeBSD.org/src/commit/?id=2d227a6ec371b970c174c0e916af5abd83deded7 commit 2d227a6ec371b970c174c0e916af5abd83deded7 Author: Alexander V. Chernikov <melifaro@FreeBSD.org> AuthorDate: 2021-02-16 20:30:04 +0000 Commit: Alexander V. Chernikov <melifaro@FreeBSD.org> CommitDate: 2021-03-11 08:25:01 +0000 Fix dst/netmask handling in routing socket code. Traditionally routing socket code did almost zero checks on the input message except for the most basic size checks. This resulted in the unclear KPI boundary for the routing system code (`rtrequest*` and now `rib_action()`) w.r.t message validness. Multiple potential problems and nuances exists: * Host bits in RTAX_DST sockaddr. Existing applications do send prefixes with hostbits uncleared. Even `route(8)` does this, as they hope the kernel would do the job of fixing it. Code inside `rib_action()` needs to handle it on its own (see `rt_maskedcopy()` ugly hack). * There are multiple way of adding the host route: it can be DST without netmask or DST with /32(/128) netmask. Also, RTF_HOST has to be set correspondingly. Currently, these 2 options create 2 DIFFERENT routes in the kernel. * no sockaddr length/content checking for the "secondary" fields exists: nothing stops rtsock application to send sockaddr_in with length of 25 (instead of 16). Kernel will accept it, install to RIB as is and propagate to all rtsock consumers, potentially triggering bugs in their code. Same goes for sin_port, sin_zero, etc. The goal of this change is to make rtsock verify all sockaddr and prefix consistency. Said differently, `rib_action()` or internals should NOT require to change any of the sockaddrs supplied by `rt_addrinfo` structure due to incorrectness. To be more specific, this change implements the following: * sockaddr cleanup/validation check is added immediately after getting sockaddrs from rtm. * Per-family dst/netmask checks clears host bits in dst and zeros all dst/netmask "secondary" fields. * The same netmask checking code converts /32(/128) netmasks to "host" route case (NULL netmask, RTF_HOST), removing the dualism. * Instead of allowing ANY "known" sockaddr families (0<..<AF_MAX), allow only actually supported ones (inet, inet6, link). * Automatically convert `sockaddr_sdl` (AF_LINK) gateways to `sockaddr_sdl_short`. Reported by: Guy Yur <guyyur at gmail.com> Reviewed By: donner Approved by: re(gjb) Differential Revision: https://reviews.freebsd.org/D28668 (cherry picked from commit e1bdecd9f60a80604a351e38cab7cfc56e308c66) --- sys/net/if_llatbl.c | 1 + sys/net/rtsock.c | 246 +++++++++++++++++++++++++++++++++- tests/sys/net/routing/rtsock_common.h | 4 - usr.sbin/arp/arp.c | 9 -- usr.sbin/ndp/ndp.c | 10 -- 5 files changed, 241 insertions(+), 29 deletions(-) diff --git a/sys/net/if_llatbl.c b/sys/net/if_llatbl.c index 97a8e3e9ccc1..7225869a07d0 100644 --- a/sys/net/if_llatbl.c +++ b/sys/net/if_llatbl.c @@ -693,6 +693,7 @@ lla_rt_output(struct rt_msghdr *rtm, struct rt_addrinfo *info) if (dl == NULL || dl->sdl_family != AF_LINK) return (EINVAL); + /* XXX: should be ntohs() */ ifp = ifnet_byindex(dl->sdl_index); if (ifp == NULL) { log(LOG_INFO, "%s: invalid ifp (sdl_index %d)\n", diff --git a/sys/net/rtsock.c b/sys/net/rtsock.c index ba1182d55439..d9294441b2bc 100644 --- a/sys/net/rtsock.c +++ b/sys/net/rtsock.c @@ -70,6 +70,7 @@ #include <netinet/if_ether.h> #include <netinet/ip_carp.h> #ifdef INET6 +#include <netinet6/in6_var.h> #include <netinet6/ip6_var.h> #include <netinet6/scope6_var.h> #endif @@ -173,6 +174,7 @@ static int rtsock_msg_buffer(int type, struct rt_addrinfo *rtinfo, struct walkarg *w, int *plen); static int rt_xaddrs(caddr_t cp, caddr_t cplim, struct rt_addrinfo *rtinfo); +static int cleanup_xaddrs(struct rt_addrinfo *info); static int sysctl_dumpentry(struct rtentry *rt, void *vw); static int sysctl_dumpnhop(struct rtentry *rt, struct nhop_object *nh, uint32_t weight, struct walkarg *w); @@ -590,11 +592,9 @@ fill_addrinfo(struct rt_msghdr *rtm, int len, u_int fibnum, struct rt_addrinfo * if (rtm->rtm_flags & RTF_RNH_LOCKED) return (EINVAL); info->rti_flags = rtm->rtm_flags; - if (info->rti_info[RTAX_DST] == NULL || - info->rti_info[RTAX_DST]->sa_family >= AF_MAX || - (info->rti_info[RTAX_GATEWAY] != NULL && - info->rti_info[RTAX_GATEWAY]->sa_family >= AF_MAX)) - return (EINVAL); + error = cleanup_xaddrs(info); + if (error != 0) + return (error); saf = info->rti_info[RTAX_DST]->sa_family; /* * Verify that the caller has the appropriate privilege; RTM_GET @@ -693,7 +693,14 @@ handle_rtm_get(struct rt_addrinfo *info, u_int fibnum, RIB_RLOCK(rnh); - if (info->rti_info[RTAX_NETMASK] == NULL) { + /* + * By (implicit) convention host route (one without netmask) + * means longest-prefix-match request and the route with netmask + * means exact-match lookup. + * As cleanup_xaddrs() cleans up info flags&addrs for the /32,/128 + * prefixes, use original data to check for the netmask presence. + */ + if ((rtm->rtm_addrs & RTA_NETMASK) == 0) { /* * Provide longest prefix match for * address lookup (no mask). @@ -1230,6 +1237,233 @@ rt_xaddrs(caddr_t cp, caddr_t cplim, struct rt_addrinfo *rtinfo) return (0); } +#ifdef INET +static inline void +fill_sockaddr_inet(struct sockaddr_in *sin, struct in_addr addr) +{ + + const struct sockaddr_in nsin = { + .sin_family = AF_INET, + .sin_len = sizeof(struct sockaddr_in), + .sin_addr = addr, + }; + *sin = nsin; +} +#endif + +#ifdef INET6 +static inline void +fill_sockaddr_inet6(struct sockaddr_in6 *sin6, const struct in6_addr *addr6, + uint32_t scopeid) +{ + + const struct sockaddr_in6 nsin6 = { + .sin6_family = AF_INET6, + .sin6_len = sizeof(struct sockaddr_in6), + .sin6_addr = *addr6, + .sin6_scope_id = scopeid, + }; + *sin6 = nsin6; +} +#endif + +/* + * Checks if gateway is suitable for lltable operations. + * Lltable code requires AF_LINK gateway with ifindex + * and mac address specified. + * Returns 0 on success. + */ +static int +cleanup_xaddrs_lladdr(struct rt_addrinfo *info) +{ + struct sockaddr_dl *sdl = (struct sockaddr_dl *)info->rti_info[RTAX_GATEWAY]; + + if (sdl->sdl_family != AF_LINK) + return (EINVAL); + + if (sdl->sdl_index == 0) + return (EINVAL); + + if (offsetof(struct sockaddr_dl, sdl_data) + sdl->sdl_nlen + sdl->sdl_alen > sdl->sdl_len) + return (EINVAL); + + return (0); +} + +static int +cleanup_xaddrs_gateway(struct rt_addrinfo *info) +{ + struct sockaddr *gw = info->rti_info[RTAX_GATEWAY]; + + if (info->rti_flags & RTF_LLDATA) + return (cleanup_xaddrs_lladdr(info)); + + switch (gw->sa_family) { +#ifdef INET + case AF_INET: + { + struct sockaddr_in *gw_sin = (struct sockaddr_in *)gw; + if (gw_sin->sin_len < sizeof(struct sockaddr_in)) { + printf("gw sin_len too small\n"); + return (EINVAL); + } + fill_sockaddr_inet(gw_sin, gw_sin->sin_addr); + } + break; +#endif +#ifdef INET6 + case AF_INET6: + { + struct sockaddr_in6 *gw_sin6 = (struct sockaddr_in6 *)gw; + if (gw_sin6->sin6_len < sizeof(struct sockaddr_in6)) { + printf("gw sin6_len too small\n"); + return (EINVAL); + } + fill_sockaddr_inet6(gw_sin6, &gw_sin6->sin6_addr, 0); + break; + } +#endif + case AF_LINK: + { + struct sockaddr_dl_short *gw_sdl; + + gw_sdl = (struct sockaddr_dl_short *)gw; + if (gw_sdl->sdl_len < sizeof(struct sockaddr_dl_short)) { + printf("gw sdl_len too small\n"); + return (EINVAL); + } + + const struct sockaddr_dl_short sdl = { + .sdl_family = AF_LINK, + .sdl_len = sizeof(struct sockaddr_dl_short), + .sdl_index = gw_sdl->sdl_index, + }; + *gw_sdl = sdl; + break; + } + } + + return (0); +} + +static void +remove_netmask(struct rt_addrinfo *info) +{ + info->rti_info[RTAX_NETMASK] = NULL; + info->rti_flags |= RTF_HOST; + info->rti_addrs &= ~RTA_NETMASK; +} + +#ifdef INET +static int +cleanup_xaddrs_inet(struct rt_addrinfo *info) +{ + struct sockaddr_in *dst_sa, *mask_sa; + + /* Check & fixup dst/netmask combination first */ + dst_sa = (struct sockaddr_in *)info->rti_info[RTAX_DST]; + mask_sa = (struct sockaddr_in *)info->rti_info[RTAX_NETMASK]; + + struct in_addr mask = { + .s_addr = mask_sa ? mask_sa->sin_addr.s_addr : INADDR_BROADCAST, + }; + struct in_addr dst = { + .s_addr = htonl(ntohl(dst_sa->sin_addr.s_addr) & ntohl(mask.s_addr)) + }; + + if (dst_sa->sin_len < sizeof(struct sockaddr_in)) { + printf("dst sin_len too small\n"); + return (EINVAL); + } + if (mask_sa && mask_sa->sin_len < sizeof(struct sockaddr_in)) { + printf("mask sin_len too small\n"); + return (EINVAL); + } + fill_sockaddr_inet(dst_sa, dst); + + if (mask.s_addr != INADDR_BROADCAST) + fill_sockaddr_inet(mask_sa, mask); + else + remove_netmask(info); + + /* Check gateway */ + if (info->rti_info[RTAX_GATEWAY] != NULL) + return (cleanup_xaddrs_gateway(info)); + + return (0); +} +#endif + +#ifdef INET6 +static int +cleanup_xaddrs_inet6(struct rt_addrinfo *info) +{ + struct sockaddr_in6 *dst_sa, *mask_sa; + struct in6_addr mask; + + /* Check & fixup dst/netmask combination first */ + dst_sa = (struct sockaddr_in6 *)info->rti_info[RTAX_DST]; + mask_sa = (struct sockaddr_in6 *)info->rti_info[RTAX_NETMASK]; + + mask = mask_sa ? mask_sa->sin6_addr : in6mask128; + IN6_MASK_ADDR(&dst_sa->sin6_addr, &mask); + + if (dst_sa->sin6_len < sizeof(struct sockaddr_in6)) { + printf("dst sin6_len too small\n"); + return (EINVAL); + } + if (mask_sa && mask_sa->sin6_len < sizeof(struct sockaddr_in6)) { + printf("mask sin6_len too small\n"); + return (EINVAL); + } + fill_sockaddr_inet6(dst_sa, &dst_sa->sin6_addr, 0); + + if (!IN6_ARE_ADDR_EQUAL(&mask, &in6mask128)) + fill_sockaddr_inet6(mask_sa, &mask, 0); + else + remove_netmask(info); + + /* Check gateway */ + if (info->rti_info[RTAX_GATEWAY] != NULL) + return (cleanup_xaddrs_gateway(info)); + + return (0); +} +#endif + +static int +cleanup_xaddrs(struct rt_addrinfo *info) +{ + int error = EAFNOSUPPORT; + + if (info->rti_info[RTAX_DST] == NULL) + return (EINVAL); + + if (info->rti_flags & RTF_LLDATA) { + /* + * arp(8)/ndp(8) sends RTA_NETMASK for the associated + * prefix along with the actual address in RTA_DST. + * Remove netmask to avoid unnecessary address masking. + */ + remove_netmask(info); + } + + switch (info->rti_info[RTAX_DST]->sa_family) { +#ifdef INET + case AF_INET: + error = cleanup_xaddrs_inet(info); + break; +#endif +#ifdef INET6 + case AF_INET6: + error = cleanup_xaddrs_inet6(info); + break; +#endif + } + + return (error); +} + /* * Fill in @dmask with valid netmask leaving original @smask * intact. Mostly used with radix netmasks. diff --git a/tests/sys/net/routing/rtsock_common.h b/tests/sys/net/routing/rtsock_common.h index 7da88e0eb512..71476d2b5f3c 100644 --- a/tests/sys/net/routing/rtsock_common.h +++ b/tests/sys/net/routing/rtsock_common.h @@ -826,10 +826,6 @@ _validate_message_sockaddrs(char *buffer, int rtm_len, size_t offset, int rtm_ad } sa = (struct sockaddr *)((char *)sa + SA_SIZE(sa)); } - - RTSOCK_ATF_REQUIRE_MSG((struct rt_msghdr *)buffer, parsed_len == rtm_len, - "message len != parsed len: expected %d parsed %d", - rtm_len, (int)parsed_len); } /* diff --git a/usr.sbin/arp/arp.c b/usr.sbin/arp/arp.c index 07e07f1f2da9..08698c7bc299 100644 --- a/usr.sbin/arp/arp.c +++ b/usr.sbin/arp/arp.c @@ -717,7 +717,6 @@ rtmsg(int cmd, struct sockaddr_in *dst, struct sockaddr_dl *sdl) static int seq; int rlen; int l; - struct sockaddr_in so_mask, *som = &so_mask; static int s = -1; static pid_t pid; @@ -735,9 +734,6 @@ rtmsg(int cmd, struct sockaddr_in *dst, struct sockaddr_dl *sdl) xo_err(1, "socket"); pid = getpid(); } - bzero(&so_mask, sizeof(so_mask)); - so_mask.sin_len = 8; - so_mask.sin_addr.s_addr = 0xffffffff; errno = 0; /* @@ -758,10 +754,6 @@ rtmsg(int cmd, struct sockaddr_in *dst, struct sockaddr_dl *sdl) rtm->rtm_rmx.rmx_expire = expire_time; rtm->rtm_inits = RTV_EXPIRE; rtm->rtm_flags |= (RTF_HOST | RTF_STATIC | RTF_LLDATA); - if (doing_proxy) { - rtm->rtm_addrs |= RTA_NETMASK; - rtm->rtm_flags &= ~RTF_HOST; - } /* FALLTHROUGH */ case RTM_GET: rtm->rtm_addrs |= RTA_DST; @@ -776,7 +768,6 @@ rtmsg(int cmd, struct sockaddr_in *dst, struct sockaddr_dl *sdl) NEXTADDR(RTA_DST, dst); NEXTADDR(RTA_GATEWAY, sdl); - NEXTADDR(RTA_NETMASK, som); rtm->rtm_msglen = cp - (char *)&m_rtmsg; doit: diff --git a/usr.sbin/ndp/ndp.c b/usr.sbin/ndp/ndp.c index aa40e2775a59..ce21e34417c3 100644 --- a/usr.sbin/ndp/ndp.c +++ b/usr.sbin/ndp/ndp.c @@ -860,12 +860,6 @@ rtmsg(int cmd) rtm->rtm_inits = RTV_EXPIRE; } rtm->rtm_flags |= (RTF_HOST | RTF_STATIC | RTF_LLDATA); -#if 0 /* we don't support ipv6addr/128 type proxying */ - if (rtm->rtm_flags & RTF_ANNOUNCE) { - rtm->rtm_flags &= ~RTF_HOST; - rtm->rtm_addrs |= RTA_NETMASK; - } -#endif /* FALLTHROUGH */ case RTM_GET: rtm->rtm_addrs |= RTA_DST; @@ -873,10 +867,6 @@ rtmsg(int cmd) NEXTADDR(RTA_DST, sin_m); NEXTADDR(RTA_GATEWAY, sdl_m); -#if 0 /* we don't support ipv6addr/128 type proxying */ - memset(&so_mask.sin6_addr, 0xff, sizeof(so_mask.sin6_addr)); - NEXTADDR(RTA_NETMASK, so_mask); -#endif rtm->rtm_msglen = cp - (char *)&m_rtmsg; doit:
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202103110826.12B8Q1w0025937>