Date: Tue, 16 Feb 2021 20:31:00 GMT From: "Alexander V. Chernikov" <melifaro@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: 2fe5a79425c7 - main - Fix dst/netmask handling in routing socket code. Message-ID: <202102162031.11GKV0T6060307@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch main has been updated by melifaro: URL: https://cgit.FreeBSD.org/src/commit/?id=2fe5a79425c79f7b828acd91da66d97230925fc8 commit 2fe5a79425c79f7b828acd91da66d97230925fc8 Author: Alexander V. Chernikov <melifaro@FreeBSD.org> AuthorDate: 2021-02-16 20:30:04 +0000 Commit: Alexander V. Chernikov <melifaro@FreeBSD.org> CommitDate: 2021-02-16 20:30:04 +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 Differential Revision: https://reviews.freebsd.org/D28668 MFC after: 3 days --- sys/net/rtsock.c | 201 +++++++++++++++++++++++++++++++++- tests/sys/net/routing/rtsock_common.h | 4 - 2 files changed, 195 insertions(+), 10 deletions(-) diff --git a/sys/net/rtsock.c b/sys/net/rtsock.c index 3a98b366dfc3..40ce62c77c2a 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); @@ -636,11 +638,9 @@ fill_addrinfo(struct rt_msghdr *rtm, int len, u_int fibnum, struct rt_addrinfo * 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 @@ -739,7 +739,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). @@ -1286,6 +1293,188 @@ rt_xaddrs(caddr_t cp, caddr_t cplim, struct rt_addrinfo *rtinfo) return (0); } +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; +} + +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; +} + +static int +cleanup_xaddrs_gateway(struct rt_addrinfo *info) +{ + struct sockaddr *gw = info->rti_info[RTAX_GATEWAY]; + + 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 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 { + info->rti_info[RTAX_NETMASK] = NULL; + info->rti_flags |= RTF_HOST; + info->rti_addrs &= ~RTA_NETMASK; + } + + /* Check gateway */ + if (info->rti_info[RTAX_GATEWAY] != NULL) + return (cleanup_xaddrs_gateway(info)); + + return (0); +} + +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 { + info->rti_info[RTAX_NETMASK] = NULL; + info->rti_flags |= RTF_HOST; + info->rti_addrs &= ~RTA_NETMASK; + } + + /* Check gateway */ + if (info->rti_info[RTAX_GATEWAY] != NULL) + return (cleanup_xaddrs_gateway(info)); + + return (0); +} + +static int +cleanup_xaddrs(struct rt_addrinfo *info) +{ + int error = EAFNOSUPPORT; + + if (info->rti_info[RTAX_DST] == NULL) + return (EINVAL); + + 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); } /*
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202102162031.11GKV0T6060307>