git: 2fe5a79425c7 - main - Fix dst/netmask handling in routing socket code.

[Kristof Provost]

On 19 Feb 2021, at 16:24, Kristof Provost wrote:
> On 16 Feb 2021, at 21:31, Alexander V. Chernikov wrote:
>> The branch main has been updated by melifaro:
>>
>> URL: 
>> https://cgit.FreeBSD.org/src/commit/?id=2fe5a79425c79f7b828acd91da66d97230925fc8
>>
>> commit 2fe5a79425c79f7b828acd91da66d97230925fc8
>> Author:     Alexander V. Chernikov <melifaro at FreeBSD.org>
>> AuthorDate: 2021-02-16 20:30:04 +0000
>> Commit:     Alexander V. Chernikov <melifaro at 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(-)
>>
>
>> +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))
>> +	};
>> +
> This breaks things like `arp -d 10.0.2.1`. It always masks off the 
> network address, which is the right thing to do in the routing table, 
> but not in the arp table.
>
> I’ve worked around it for now with this hack:
>
> 	diff --git a/sys/net/rtsock.c b/sys/net/rtsock.c
> 	index 3c1fea497af6..533076db99a5 100644
> 	--- a/sys/net/rtsock.c
> 	+++ b/sys/net/rtsock.c
> 	@@ -638,9 +638,12 @@ fill_addrinfo(struct rt_msghdr *rtm, int len, 
> u_int fibnum, struct rt_addrinfo *
> 	                return (EINVAL);
>
> 	        info->rti_flags = rtm->rtm_flags;
> 	-       error = cleanup_xaddrs(info);
> 	-       if (error != 0)
> 	-               return (error);
> 	+       /* XXX HACK */
> 	+       if (! (rtm->rtm_flags & RTF_LLDATA)) {
> 	+               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
>
> But I’m not totally happy with this, obviously.

This may be a bit more reasonable:

	diff --git a/sys/net/rtsock.c b/sys/net/rtsock.c
	index 3c1fea497af6..5147b92e95d5 100644
	--- a/sys/net/rtsock.c
	+++ b/sys/net/rtsock.c
	@@ -1393,6 +1393,10 @@ cleanup_xaddrs_inet(struct rt_addrinfo *info)
	                .s_addr = htonl(ntohl(dst_sa->sin_addr.s_addr) & 
ntohl(mask.s_addr))
	        };

	+       /* Keep the address if we're LL */
	+       if (info->rti_flags & RTF_LLDATA)
	+               dst.s_addr = dst_sa->sin_addr.s_addr;
	+
	        if (dst_sa->sin_len < sizeof(struct sockaddr_in)) {
	                printf("dst sin_len too small\n");
	                return (EINVAL);
	@@ -1431,7 +1435,10 @@ cleanup_xaddrs_inet6(struct rt_addrinfo *info)
	        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);
	+
	+       /* Keep the address if we're LL */
	+       if (! (info->rti_flags & RTF_LLDATA))
	+               IN6_MASK_ADDR(&dst_sa->sin6_addr, &mask);

	        if (dst_sa->sin6_len < sizeof(struct sockaddr_in6)) {
	                printf("dst sin6_len too small\n");

Best regards,
Kristof