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

[Kristof Provost]

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.

Best regards,
Kristof