Hi, syszcaller shows that we lack kernel input validation when setting addresses.
https://syzkaller.appspot.com/bug?id=4196491189556b467eafcc8daf3f9a051b4123a0 Althogh this is not exactly the ioctl(2) syzcaller found, I would like to start fixing SIOCAIFADDR and SIOCDIFADDR. Others will be fixed later. - in_sa2sin() validates inet address family and address length - merge the two ifa loops into one and describe behavior - netmaskt does not need af, but sufficent len - validate destination and broadcast address - ia_netmask and ia_sockmask length has been initialized, set only address ok? bluhm Index: netinet/in.c =================================================================== RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/in.c,v retrieving revision 1.163 diff -u -p -r1.163 in.c --- netinet/in.c 25 Jul 2019 13:56:24 -0000 1.163 +++ netinet/in.c 16 Oct 2019 21:32:01 -0000 @@ -185,6 +185,18 @@ in_nam2sin(const struct mbuf *nam, struc } int +in_sa2sin(struct sockaddr *sa, struct sockaddr_in **sin) +{ + if (sa->sa_family != AF_INET) + return EAFNOSUPPORT; + if (sa->sa_len != sizeof(struct sockaddr_in)) + return EINVAL; + *sin = satosin(sa); + + return 0; +} + +int in_control(struct socket *so, u_long cmd, caddr_t data, struct ifnet *ifp) { int privileged; @@ -372,28 +384,29 @@ in_ioctl_change_ifaddr(u_long cmd, caddr struct ifaddr *ifa; struct in_ifaddr *ia = NULL; struct in_aliasreq *ifra = (struct in_aliasreq *)data; + struct sockaddr_in *sin = NULL, *dstsin = NULL, *broadsin = NULL; int error = 0; int newifaddr; + if (ifra->ifra_addr.sin_family == AF_INET) { + error = in_sa2sin(sintosa(&ifra->ifra_addr), &sin); + if (error) + return (error); + } + NET_LOCK(); TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) { - if (ifa->ifa_addr->sa_family == AF_INET) { + if (ifa->ifa_addr->sa_family != AF_INET) + continue; + /* find first address, if no exact match wanted */ + if (sin == NULL || sin->sin_addr.s_addr == + ifatoia(ifa)->ia_addr.sin_addr.s_addr) { ia = ifatoia(ifa); break; } } - if (ifra->ifra_addr.sin_family == AF_INET) { - for (; ifa != NULL; ifa = TAILQ_NEXT(ifa, ifa_list)) { - if ((ifa->ifa_addr->sa_family == AF_INET) && - ifatoia(ifa)->ia_addr.sin_addr.s_addr == - ifra->ifra_addr.sin_addr.s_addr) - break; - } - ia = ifatoia(ifa); - } - switch (cmd) { case SIOCAIFADDR: { int needinit = 0; @@ -403,6 +416,27 @@ in_ioctl_change_ifaddr(u_long cmd, caddr break; } + if (ifra->ifra_mask.sin_len) { + if (ifra->ifra_mask.sin_len < 8) { + error = EINVAL; + break; + } + } + if ((ifp->if_flags & IFF_POINTOPOINT) && + ifra->ifra_dstaddr.sin_family == AF_INET) { + error = in_sa2sin(sintosa(&ifra->ifra_dstaddr), + &dstsin); + if (error) + break; + } + if ((ifp->if_flags & IFF_BROADCAST) && + ifra->ifra_broadaddr.sin_family == AF_INET) { + error = in_sa2sin(sintosa(&ifra->ifra_broadaddr), + &broadsin); + if (error) + break; + } + if (ia == NULL) { ia = malloc(sizeof *ia, M_IFADDR, M_WAITOK | M_ZERO); ia->ia_addr.sin_family = AF_INET; @@ -421,41 +455,38 @@ in_ioctl_change_ifaddr(u_long cmd, caddr } else newifaddr = 0; - if (ia->ia_addr.sin_family == AF_INET) { - if (ifra->ifra_addr.sin_len == 0) - ifra->ifra_addr = ia->ia_addr; - else if (ifra->ifra_addr.sin_addr.s_addr != - ia->ia_addr.sin_addr.s_addr || newifaddr) - needinit = 1; + if (sin == NULL) { + sin = &ia->ia_addr; + } else if (newifaddr || + sin->sin_addr.s_addr != ia->ia_addr.sin_addr.s_addr) { + needinit = 1; } if (ifra->ifra_mask.sin_len) { in_ifscrub(ifp, ia); - ia->ia_sockmask = ifra->ifra_mask; - ia->ia_netmask = ia->ia_sockmask.sin_addr.s_addr; + ia->ia_netmask = ia->ia_sockmask.sin_addr.s_addr = + ifra->ifra_mask.sin_addr.s_addr; needinit = 1; } - if ((ifp->if_flags & IFF_POINTOPOINT) && - (ifra->ifra_dstaddr.sin_family == AF_INET)) { + if (dstsin != NULL) { in_ifscrub(ifp, ia); - ia->ia_dstaddr = ifra->ifra_dstaddr; - needinit = 1; + ia->ia_dstaddr = *dstsin; + needinit = 1; } - if ((ifp->if_flags & IFF_BROADCAST) && - (ifra->ifra_broadaddr.sin_family == AF_INET)) { + if (broadsin != NULL) { if (newifaddr) - ia->ia_broadaddr = ifra->ifra_broadaddr; + ia->ia_broadaddr = *broadsin; else ifa_update_broadaddr(ifp, &ia->ia_ifa, - sintosa(&ifra->ifra_broadaddr)); + sintosa(broadsin)); } - if (ifra->ifra_addr.sin_family == AF_INET && needinit) { - error = in_ifinit(ifp, ia, &ifra->ifra_addr, newifaddr); + if (sin != NULL && needinit) { + error = in_ifinit(ifp, ia, sin, newifaddr); + if (error) + break; } - if (error) - break; dohooks(ifp->if_addrhooks, 0); break; - } + } case SIOCDIFADDR: if (!privileged) { error = EPERM; Index: netinet/in.h =================================================================== RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/in.h,v retrieving revision 1.133 diff -u -p -r1.133 in.h --- netinet/in.h 13 Oct 2018 18:36:01 -0000 1.133 +++ netinet/in.h 16 Oct 2019 15:44:32 -0000 @@ -818,6 +818,7 @@ void in_ifdetach(struct ifnet *); int in_mask2len(struct in_addr *); void in_len2mask(struct in_addr *, int); int in_nam2sin(const struct mbuf *, struct sockaddr_in **); +int in_sa2sin(struct sockaddr *, struct sockaddr_in **); char *inet_ntoa(struct in_addr); int inet_nat64(int, const void *, void *, const void *, u_int8_t);
