On Wed, Oct 16, 2019 at 11:42:16PM +0200, Alexander Bluhm wrote:
> 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.
Here comes ioctl(2) kernel input validation for
SIOCGIFADDR
SIOCGIFNETMASK
SIOCGIFDSTADDR
SIOCGIFBRDADDR
SIOCSIFADDR
SIOCSIFNETMASK
SIOCSIFDSTADDR
SIOCSIFBRDADDR
- Name in_ioctl_set_ifaddr() consitently.
- Use in_sa2sin() to validate inet address.
- Combine if_addrlist loops and add comment.
- Netmask is not a inet address, but lenght must be valid.
ok?
bluhm
Index: netinet/in.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/in.c,v
retrieving revision 1.164
diff -u -p -r1.164 in.c
--- netinet/in.c 23 Oct 2019 19:58:32 -0000 1.164
+++ netinet/in.c 24 Oct 2019 23:04:34 -0000
@@ -84,7 +84,7 @@
void in_socktrim(struct sockaddr_in *);
-int in_ioctl_sifaddr(u_long, caddr_t, struct ifnet *, int);
+int in_ioctl_set_ifaddr(u_long, caddr_t, struct ifnet *, int);
int in_ioctl_change_ifaddr(u_long, caddr_t, struct ifnet *, int);
int in_ioctl_get(u_long, caddr_t, struct ifnet *);
void in_purgeaddr(struct ifaddr *);
@@ -227,7 +227,7 @@ in_ioctl(u_long cmd, caddr_t data, struc
struct ifreq *ifr = (struct ifreq *)data;
struct ifaddr *ifa;
struct in_ifaddr *ia = NULL;
- struct sockaddr_in oldaddr;
+ struct sockaddr_in *sin = NULL, oldaddr;
int error = 0;
if (ifp == NULL)
@@ -240,7 +240,7 @@ in_ioctl(u_long cmd, caddr_t data, struc
case SIOCGIFBRDADDR:
return in_ioctl_get(cmd, data, ifp);
case SIOCSIFADDR:
- return in_ioctl_sifaddr(cmd, data, ifp, privileged);
+ return in_ioctl_set_ifaddr(cmd, data, ifp, privileged);
case SIOCAIFADDR:
case SIOCDIFADDR:
return in_ioctl_change_ifaddr(cmd, data, ifp, privileged);
@@ -252,28 +252,31 @@ in_ioctl(u_long cmd, caddr_t data, struc
return (EOPNOTSUPP);
}
+ if (ifr->ifr_addr.sa_family == AF_INET) {
+ error = in_sa2sin(&ifr->ifr_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 or exact match */
+ if (ia == NULL)
+ ia = ifatoia(ifa);
+ if (sin == NULL || sin->sin_addr.s_addr == INADDR_ANY)
+ break;
+ if (ifatoia(ifa)->ia_addr.sin_addr.s_addr ==
+ sin->sin_addr.s_addr) {
ia = ifatoia(ifa);
break;
- }
- }
-
- if (ia && satosin(&ifr->ifr_addr)->sin_addr.s_addr) {
- 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 ==
- satosin(&ifr->ifr_addr)->sin_addr.s_addr) {
- ia = ifatoia(ifa);
- break;
- }
}
}
if (ia == NULL) {
- NET_UNLOCK();
- return (EADDRNOTAVAIL);
+ error = EADDRNOTAVAIL;
+ goto err;
}
switch (cmd) {
@@ -287,8 +290,11 @@ in_ioctl(u_long cmd, caddr_t data, struc
error = EINVAL;
break;
}
+ error = in_sa2sin(&ifr->ifr_dstaddr, &sin);
+ if (error)
+ break;
oldaddr = ia->ia_dstaddr;
- ia->ia_dstaddr = *satosin(&ifr->ifr_dstaddr);
+ ia->ia_dstaddr = *sin;
error = (*ifp->if_ioctl)(ifp, SIOCSIFDSTADDR, (caddr_t)ia);
if (error) {
ia->ia_dstaddr = oldaddr;
@@ -308,7 +314,10 @@ in_ioctl(u_long cmd, caddr_t data, struc
error = EINVAL;
break;
}
- ifa_update_broadaddr(ifp, &ia->ia_ifa, &ifr->ifr_broadaddr);
+ error = in_sa2sin(&ifr->ifr_broadaddr, &sin);
+ if (error)
+ break;
+ ifa_update_broadaddr(ifp, &ia->ia_ifa, sintosa(sin));
break;
case SIOCSIFNETMASK:
@@ -317,21 +326,27 @@ in_ioctl(u_long cmd, caddr_t data, struc
break;
}
+ if (ifr->ifr_addr.sa_len < 8) {
+ error = EINVAL;
+ break;
+ }
ia->ia_netmask = ia->ia_sockmask.sin_addr.s_addr =
satosin(&ifr->ifr_addr)->sin_addr.s_addr;
break;
}
-
+err:
NET_UNLOCK();
return (error);
}
int
-in_ioctl_sifaddr(u_long cmd, caddr_t data, struct ifnet *ifp, int privileged)
+in_ioctl_set_ifaddr(u_long cmd, caddr_t data, struct ifnet *ifp,
+ int privileged)
{
struct ifreq *ifr = (struct ifreq *)data;
struct ifaddr *ifa;
struct in_ifaddr *ia = NULL;
+ struct sockaddr_in *sin;
int error = 0;
int newifaddr;
@@ -341,15 +356,19 @@ in_ioctl_sifaddr(u_long cmd, caddr_t dat
if (!privileged)
return (EPERM);
+ error = in_sa2sin(&ifr->ifr_addr, &sin);
+ if (error)
+ return (error);
+
NET_LOCK();
TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
- if (ifa->ifa_addr->sa_family == AF_INET) {
- ia = ifatoia(ifa);
- break;
- }
+ if (ifa->ifa_addr->sa_family != AF_INET)
+ continue;
+ /* find first address */
+ ia = ifatoia(ifa);
+ break;
}
-
if (ia == NULL) {
ia = malloc(sizeof *ia, M_IFADDR, M_WAITOK | M_ZERO);
ia->ia_addr.sin_family = AF_INET;
@@ -369,7 +388,7 @@ in_ioctl_sifaddr(u_long cmd, caddr_t dat
newifaddr = 0;
in_ifscrub(ifp, ia);
- error = in_ifinit(ifp, ia, satosin(&ifr->ifr_addr), newifaddr);
+ error = in_ifinit(ifp, ia, sin, newifaddr);
if (!error)
dohooks(ifp->if_addrhooks, 0);
@@ -521,25 +540,29 @@ in_ioctl_get(u_long cmd, caddr_t data, s
struct ifreq *ifr = (struct ifreq *)data;
struct ifaddr *ifa;
struct in_ifaddr *ia = NULL;
+ struct sockaddr_in *sin = NULL;
int error = 0;
+ if (ifr->ifr_addr.sa_family == AF_INET) {
+ error = in_sa2sin(&ifr->ifr_addr, &sin);
+ if (error)
+ return (error);
+ }
+
NET_RLOCK();
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 or exact match */
+ if (ia == NULL)
+ ia = ifatoia(ifa);
+ if (sin == NULL || sin->sin_addr.s_addr == INADDR_ANY)
+ break;
+ if (ifatoia(ifa)->ia_addr.sin_addr.s_addr ==
+ sin->sin_addr.s_addr) {
ia = ifatoia(ifa);
break;
- }
- }
-
- if (ia && satosin(&ifr->ifr_addr)->sin_addr.s_addr) {
- 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 ==
- satosin(&ifr->ifr_addr)->sin_addr.s_addr) {
- ia = ifatoia(ifa);
- break;
- }
}
}
if (ia == NULL) {
