net: Fix handling of sockaddrs in the SIOC{ADD,DEL}MULTI handlers

[markj]

Summary

The SIOCADDMULTI and SIOCDELMULTI handlers add or delete a link-layer
multicast address from an interface's multicast filter list. The
link-layer address is passed using the ifr_addr field of the request
structure.

struct ifreq's ifr_addr field is a struct sockaddr, which is a fair bit
smaller than struct sockaddr_dl (though big enough to hold an ethernet
address). Existing callers set the sockaddr length to
sizeof(struct sockaddr_dl), which is too large, and causes OOB accesses
when if_findmulti() is used to compare the address with others, or when
if_addmulti() makes a copy.

Fix this without breaking compatibility: copy the user-supplied address
into a sockaddr_dl on the stack, and use the latter for the respective
operation.

Reported by: Yuxiang Yang, Yizhou Zhao, Ao Wang, Xuewei Feng, Qi Li,

		and Ke Xu from Tsinghua University using GLM-5.2 from Z.ai

[glebius]

The fix is good enough. Note: the core problem is use of struct sockaddr which is a socket address for socket(2)-related APIs inside internal kernel structures like struct ifaddr.

[zlei]

sys/net/if.c

2875

potential out-of-bounds accesses

I think for SIOCADDMULTI/SIOCDELMULTI the kernel should do sanity check against the values of members.

/*
 * Structure of a Link-Level sockaddr:
 */
struct sockaddr_dl {
        u_char  sdl_len;        /* Total length of sockaddr */
        u_char  sdl_family;     /* AF_LINK */
        u_short sdl_index;      /* if != 0, system given index for interface */
        u_char  sdl_type;       /* interface type */
        u_char  sdl_nlen;       /* interface name length, no trailing 0 reqd. */
        u_char  sdl_alen;       /* link level address length */
        u_char  sdl_slen;       /* link layer selector length */
        char    sdl_data[46];   /* minimum work area, can be larger;
                                   contains both if name and ll address */
};

/*
 * Structure used by kernel to store most
 * addresses.
 */
struct sockaddr {
        unsigned char   sa_len;         /* total length */
        sa_family_t     sa_family;      /* address family */
        char            sa_data[14];    /* actually longer; address value */
};

The sizeof sockaddr is 16, well a typical usage of sockaddr_dl is (8 + ETHER_ADDR_LEN) == 14. So typically no out-of-bounds accesses happens.

Well, an ill-behaved program may pass in unreasonable large values of sdl_nlen or/and sdl_alen or/and sdl_slen so out-of-bounds accesses is still possible.

So I suggested the following sanity check against ifr->ifr_addr,

	if (ifr->ifr_addr.sa_family != AF_LINK)
		return (EINVAL);
	struct sockaddr_dl *sdl = (struct sockaddr_dl *)&ifr->ifr_addr;
	if (sdl->sdl_nlen + sdl->sdl_alen + sdl->sdl_slen + 8 > sizeof(ifr->ifr_addr))
		return (EINVAL);

As for ifr->ifr_addr.sa_len we can always safely truncate it to MIN(ifr->ifr_addr.sa_len, sizeof(ifr->ifr_addr)) . I'd personally prefer adding a verbose log when the truncating happens, say

if (ifr->ifr_addr.sa_len > sizeof(ifr->ifr_addr)) {
    log(LOG_DEBUG, "SIOCADDMULTI: Truncating ifr->ifr_addr.sa_len from %d to %d\n"), ifr->ifr_addr.sa_len, sizeof(ifr->ifr_addr));
    ifr->ifr_addr.sa_len = sizeof(ifr->ifr_addr);
}

[markj]

Check the inner lengths as well.

[markj]

Well, an ill-behaved program may pass in unreasonable large values of sdl_nlen or/and sdl_alen or/and sdl_slen so out-of-bounds accesses is still possible.

I do not see exactly how any OOB access can happen in this case, but yes we should validate the lengths anyway.

I'd personally prefer adding a verbose log when the truncating happens, say

All the users of this ioctl I found (there are not many) set sa_len = sizeof(struct sockaddr_dl), so this log message will always be printed. I think it is not very useful.

[zlei]

Looks good to me.