On 09/05/18(Wed) 10:18, Theo Buehler wrote:
> Currently the privilege checks in in_ioctl() happen rather late and for
> SIOCAIFADDR and SIOCDIFADDR after a /* FALLTHROUGH */. As a further
> small cleanup step in this function, I'd like to move this check to the
> top. This way it's clearer from the start which ioctls are explicitly
> taken care of in this function, that all of them need `privileged' to be
> set, and what happens in the default case.
deraadt@ insisted last year that we should not go this road. Doing so
makes it easier to forget a security check.
In the end what you want is a single 'case' per ioctl(2).
> Index: sys/netinet/in.c
> ===================================================================
> RCS file: /var/cvs/src/sys/netinet/in.c,v
> retrieving revision 1.151
> diff -u -p -r1.151 in.c
> --- sys/netinet/in.c 2 May 2018 12:40:52 -0000 1.151
> +++ sys/netinet/in.c 7 May 2018 15:40:04 -0000
> @@ -227,6 +227,17 @@ in_ioctl(u_long cmd, caddr_t data, struc
> case SIOCGIFDSTADDR:
> case SIOCGIFBRDADDR:
> return in_ioctl_get(cmd, data, ifp);
> + case SIOCAIFADDR:
> + case SIOCDIFADDR:
> + case SIOCSIFADDR:
> + case SIOCSIFNETMASK:
> + case SIOCSIFDSTADDR:
> + case SIOCSIFBRDADDR:
> + if (!privileged)
> + return EPERM;
> + break;
> + default:
> + return EOPNOTSUPP;
> }
>
> NET_LOCK();
> @@ -256,11 +267,6 @@ in_ioctl(u_long cmd, caddr_t data, struc
> }
> /* FALLTHROUGH */
> case SIOCSIFADDR:
> - if (!privileged) {
> - error = EPERM;
> - goto err;
> - }
> -
> if (ia == NULL) {
> ia = malloc(sizeof *ia, M_IFADDR, M_WAITOK | M_ZERO);
> ia->ia_addr.sin_family = AF_INET;
> @@ -283,11 +289,6 @@ in_ioctl(u_long cmd, caddr_t data, struc
> case SIOCSIFNETMASK:
> case SIOCSIFDSTADDR:
> case SIOCSIFBRDADDR:
> - if (!privileged) {
> - error = EPERM;
> - goto err;
> - }
> -
> 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) &&
> @@ -389,10 +390,6 @@ in_ioctl(u_long cmd, caddr_t data, struc
> */
> in_purgeaddr(&ia->ia_ifa);
> dohooks(ifp->if_addrhooks, 0);
> - break;
> -
> - default:
> - error = EOPNOTSUPP;
> break;
> }
>
>
