On Wed, Aug 28, 2019 at 11:58:36PM +0200, Alexander Bluhm wrote:
> Hi,
>
> The kernel may crash as there is not enough input validation in
> routing messages.
>
> https://syzkaller.appspot.com/bug?id=e2076a6518b49730aefe64acf0a266f8e79685a5
>
> Here the name of a routing label is not NUL terminated, but there
> are more things that can go wrong. So I added some checks for
> incoming routing addresses from userland that the kernel actually
> uses.
>
> It is not super strict as userland may provide incomplete addresses
> that work anyway. I remember openvpn caused some problems in this
> area. Could someone test it with this diff?
>
> ok?
I don't think this is the right way to do this. The consumer of rtinfo
need to check the values based on their needs. Ideally we add some helpers
to make that easier. I think it is close to impossible to properly
validate the sockaddrs in rtm_xaddrs() since that function is missing
needed context.
> bluhm
>
> Index: net/rtsock.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/rtsock.c,v
> retrieving revision 1.290
> diff -u -p -w -r1.290 rtsock.c
> --- net/rtsock.c 28 Aug 2019 20:54:24 -0000 1.290
> +++ net/rtsock.c 28 Aug 2019 21:23:34 -0000
> @@ -1366,6 +1366,91 @@ rtm_xaddrs(caddr_t cp, caddr_t cplim, st
> rtinfo->rti_info[i] = sa;
> ADVANCE(cp, sa);
> }
> + for (i = 0; i < RTAX_MAX; i++) {
> + size_t len, maxlen, size;
> +
> + sa = rtinfo->rti_info[i];
> + if (sa == NULL)
> + continue;
> + maxlen = size = 0;
> + switch (i) {
> + case RTAX_DST:
> + case RTAX_GATEWAY:
> + case RTAX_SRC:
> + switch (sa->sa_family) {
> + case AF_INET:
> + size = sizeof(struct sockaddr_in);
> + break;
> +#ifdef INET6
> + case AF_INET6:
> + size = sizeof(struct sockaddr_in6);
> + break;
> +#endif
> +#ifdef MPLS
> + case AF_MPLS:
> + size = sizeof(struct sockaddr_mpls);
> + break;
> +#endif
> + }
> + break;
> + case RTAX_IFP:
> + if (sa->sa_family != AF_LINK)
> + return (EAFNOSUPPORT);
> + /*
> + * XXX Should be sizeof(struct sockaddr_dl), but
> + * route(8) has a bug and provides less memory.
> + */
> + size = 16;
> + break;
> + case RTAX_IFA:
> + switch (sa->sa_family) {
> + case AF_INET:
> + size = sizeof(struct sockaddr_in);
> + break;
> +#ifdef INET6
> + case AF_INET6:
> + size = sizeof(struct sockaddr_in6);
> + break;
> +#endif
> + default:
> + return (EAFNOSUPPORT);
> + }
> + break;
> + case RTAX_LABEL:
> + maxlen = RTLABEL_LEN;
> + size = sizeof(struct sockaddr_rtlabel);
> + break;
> +#ifdef BFD
> + case RTAX_BFD:
> + size = sizeof(struct sockaddr_bfd);
> + break;
> +#endif
> + case RTAX_DNS:
> + maxlen = RTDNS_LEN;
> + size = sizeof(struct sockaddr_rtdns);
> + break;
> + case RTAX_STATIC:
> + maxlen = RTSTATIC_LEN;
> + size = sizeof(struct sockaddr_rtstatic);
> + break;
> + case RTAX_SEARCH:
> + maxlen = RTSEARCH_LEN;
> + size = sizeof(struct sockaddr_rtsearch);
> + break;
> + }
> + if (size) {
> + if (sa->sa_len < size)
> + return (EINVAL);
> + }
> + if (maxlen) {
> + if (2 + maxlen >= size)
> + return (EINVAL);
> + len = strnlen(sa->sa_data, maxlen);
> + if (len >= maxlen || 2 + len >= sa->sa_len)
> + return (EINVAL);
> + break;
> + }
> + }
> return (0);
> }
>
--
:wq Claudio
