Check the disk/kernel/cdrom file permissions after openening the fd.

This prevents time of TOCTOU attacks for instances.

OK mlarkin@

diff --git a/usr.sbin/vmd/config.c b/usr.sbin/vmd/config.c
index fc8114a..5f6db1a 100644 (file)
--- a/usr.sbin/vmd/config.c
+++ b/usr.sbin/vmd/config.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: config.c,v 1.46 2018/07/11 13:19:47 reyk Exp $        */
+/*     $OpenBSD: config.c,v 1.47 2018/07/13 10:26:57 reyk Exp $        */
 
 /*
  * Copyright (c) 2015 Reyk Floeter <reyk@openbsd.org>
@@ -173,7 +173,6 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm, uint32_t peerid, uid_t uid)
        struct vmd_if           *vif;
        struct vmop_create_params *vmc = &vm->vm_params;
        struct vm_create_params *vcp = &vmc->vmc_params;
-       struct stat              stat_buf;
        unsigned int             i;
        int                      fd = -1, vmboot = 0;
        int                      kernfd = -1, *diskfds = NULL, *tapfds = NULL;
@@ -241,6 +240,15 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm, uint32_t peerid, uid_t uid)
                        errno = VMD_BIOS_MISSING;
                        goto fail;
                }
+
+               if (vm_checkaccess(kernfd,
+                   vmc->vmc_checkaccess & VMOP_CREATE_KERNEL,
+                   uid, R_OK) == -1) {
+                       log_warnx("vm \"%s\" no read access to %s",
+                           vcp->vcp_name, vcp->vcp_kernel);
+                       errno = EPERM;
+                       goto fail;
+               }
        }
 
        /* Open CDROM image for child */
@@ -253,16 +261,13 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm, uint32_t peerid, uid_t uid)
                        errno = VMD_CDROM_MISSING;
                        goto fail;
                }
-               if (fstat(cdromfd, &stat_buf) == -1) {
-                       log_warn("%s: can't open cdrom %s", __func__,
-                           vcp->vcp_cdrom);
-                       errno = VMD_CDROM_MISSING;
-                       goto fail;
-               }
-               if (S_ISREG(stat_buf.st_mode) == 0) {
-                       log_warnx("%s: cdrom %s is not a regular file",
-                           __func__, vcp->vcp_cdrom);
-                       errno = VMD_CDROM_INVALID;
+
+               if (vm_checkaccess(cdromfd,
+                   vmc->vmc_checkaccess & VMOP_CREATE_CDROM,
+                   uid, R_OK) == -1) {
+                       log_warnx("vm \"%s\" no read access to %s",
+                           vcp->vcp_name, vcp->vcp_cdrom);
+                       errno = EPERM;
                        goto fail;
                }
        }
@@ -277,16 +282,13 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm, uint32_t peerid, uid_t uid)
                        errno = VMD_DISK_MISSING;
                        goto fail;
                }
-               if (fstat(diskfds[i], &stat_buf) == -1) {
-                       log_warn("%s: can't open disk %s", __func__,
-                           vcp->vcp_disks[i]);
-                       errno = VMD_DISK_INVALID;
-                       goto fail;
-               }
-               if (S_ISREG(stat_buf.st_mode) == 0) {
-                       log_warnx("%s: disk %s is not a regular file", __func__,
-                           vcp->vcp_disks[i]);
-                       errno = VMD_DISK_INVALID;
+
+               if (vm_checkaccess(diskfds[i],
+                   vmc->vmc_checkaccess & VMOP_CREATE_DISK,
+                   uid, R_OK|W_OK) == -1) {
+                       log_warnx("vm \"%s\" no read access to %s",
+                           vcp->vcp_name, vcp->vcp_kernel);
+                       errno = EPERM;
                        goto fail;
                }
        }

diff --git a/usr.sbin/vmd/vmd.c b/usr.sbin/vmd/vmd.c
index 9a148e2..da18462 100644 (file)
--- a/usr.sbin/vmd/vmd.c
+++ b/usr.sbin/vmd/vmd.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: vmd.c,v 1.96 2018/07/13 08:42:49 reyk Exp $   */
+/*     $OpenBSD: vmd.c,v 1.97 2018/07/13 10:26:57 reyk Exp $   */
 
 /*
  * Copyright (c) 2015 Reyk Floeter <reyk@openbsd.org>
@@ -1360,12 +1360,7 @@ vm_instance(struct privsep *ps, struct vmd_vm **vm_parent,
                                return (-1);
                        }
                }
-               if (vm_checkaccess(vcp->vcp_disks[i], uid, R_OK|W_OK) == -1) {
-                       log_warnx("vm \"%s\" no read/write access to %s", name,
-                           vcp->vcp_disks[i]);
-                       errno = EPERM;
-                       return (-1);
-               }
+               vmc->vmc_checkaccess |= VMOP_CREATE_DISK;
        }
 
        /* interfaces */
@@ -1424,12 +1419,7 @@ vm_instance(struct privsep *ps, struct vmd_vm **vm_parent,
                        errno = EPERM;
                        return (-1);
                }
-               if (vm_checkaccess(vcp->vcp_kernel, uid, R_OK) == -1) {
-                       log_warnx("vm \"%s\" no read access to %s", name,
-                           vcp->vcp_kernel);
-                       errno = EPERM;
-                       return (-1);
-               }
+               vmc->vmc_checkaccess |= VMOP_CREATE_KERNEL;
        } else if (strlcpy(vcp->vcp_kernel, vcpp->vcp_kernel,
            sizeof(vcp->vcp_kernel)) >= sizeof(vcp->vcp_kernel)) {
                log_warnx("vm \"%s\" kernel name too long", name);
@@ -1444,12 +1434,7 @@ vm_instance(struct privsep *ps, struct vmd_vm **vm_parent,
                        errno = EPERM;
                        return (-1);
                }
-               if (vm_checkaccess(vcp->vcp_cdrom, uid, R_OK) == -1) {
-                       log_warnx("vm \"%s\" no read access to %s", name,
-                           vcp->vcp_cdrom);
-                       errno = EPERM;
-                       return (-1);
-               }
+               vmc->vmc_checkaccess |= VMOP_CREATE_CDROM;
        } else if (strlcpy(vcp->vcp_cdrom, vcpp->vcp_cdrom,
            sizeof(vcp->vcp_cdrom)) >= sizeof(vcp->vcp_cdrom)) {
                log_warnx("vm \"%s\" cdrom name too long", name);
@@ -1592,15 +1577,17 @@ vm_checkinsflag(struct vmop_create_params *vmc, unsigned int flag, uid_t uid)
  * access the file described by the 'path' parameter.
  *
  * Parameters:
- *  path: the path of a file
+ *  fd: the file descriptor of the opened file
+ *  uflag: check if the userid has access to the file
  *  uid: the user ID of the user making the request
+ *  amode: the access flags of R_OK and W_OK
  *
  * Return values:
  *   0: the permission should be granted
  *  -1: the permission check failed
  */
 int
-vm_checkaccess(const char *path, uid_t uid, int amode)
+vm_checkaccess(int fd, unsigned int uflag, uid_t uid, int amode)
 {
        struct group    *gr;
        struct passwd   *pw;
@@ -1608,35 +1595,34 @@ vm_checkaccess(const char *path, uid_t uid, int amode)
        struct stat      st;
        mode_t           mode;
 
-       /* root has no restrictions */
-       if (uid == 0)
-               return (0);
-
-       if (path == NULL || strlen(path) == 0)
+       if (fd == -1)
                return (-1);
 
        /*
         * File has to be accessible and a regular file
-        * (symlinks would allow TOCTTOU-like attacks).
         */
-       if (stat(path, &st) == -1 && !S_ISREG(st.st_mode))
+       if (fstat(fd, &st) == -1 || !S_ISREG(st.st_mode))
                return (-1);
 
+       /* root has no restrictions */
+       if (uid == 0 || uflag == 0)
+               return (0);
+
        /* check other */
-       mode = amode == W_OK ? S_IWOTH : 0;
-       mode |= amode == R_OK ? S_IROTH : 0;
+       mode = amode & W_OK ? S_IWOTH : 0;
+       mode |= amode & R_OK ? S_IROTH : 0;
        if ((st.st_mode & mode) == mode)
                return (0);
 
        /* check user */
-       mode = amode == W_OK ? S_IWUSR : 0;
-       mode |= amode == R_OK ? S_IRUSR : 0;
+       mode = amode & W_OK ? S_IWUSR : 0;
+       mode |= amode & R_OK ? S_IRUSR : 0;
        if (uid == st.st_uid && (st.st_mode & mode) == mode)
                return (0);
 
        /* check groups */
-       mode = amode == W_OK ? S_IWGRP : 0;
-       mode |= amode == R_OK ? S_IRGRP : 0;
+       mode = amode & W_OK ? S_IWGRP : 0;
+       mode |= amode & R_OK ? S_IRGRP : 0;
        if ((st.st_mode & mode) != mode)
                return (-1);
        if ((pw = getpwuid(uid)) == NULL)

diff --git a/usr.sbin/vmd/vmd.h b/usr.sbin/vmd/vmd.h
index 7da5318..bf670f5 100644 (file)
--- a/usr.sbin/vmd/vmd.h
+++ b/usr.sbin/vmd/vmd.h
@@ -1,4 +1,4 @@
-/*     $OpenBSD: vmd.h,v 1.76 2018/07/13 08:42:49 reyk Exp $   */
+/*     $OpenBSD: vmd.h,v 1.77 2018/07/13 10:26:57 reyk Exp $   */
 
 /*
  * Copyright (c) 2015 Mike Larkin <mlarkin@openbsd.org>
@@ -149,6 +149,9 @@ struct vmop_create_params {
 #define VMOP_CREATE_CDROM      0x20
 #define VMOP_CREATE_INSTANCE   0x40
 
+       /* same flags; check for access to these resources */
+       unsigned int             vmc_checkaccess;
+
        /* userland-only part of the create params */
        unsigned int             vmc_ifflags[VMM_MAX_NICS_PER_VM];
 #define VMIFF_UP               0x01
@@ -321,7 +324,7 @@ void         vm_remove(struct vmd_vm *, const char *);
 int     vm_register(struct privsep *, struct vmop_create_params *,
            struct vmd_vm **, uint32_t, uid_t);
 int     vm_checkperm(struct vmd_vm *, struct vmop_owner *, uid_t);
-int     vm_checkaccess(const char *, uid_t, int);
+int     vm_checkaccess(int, unsigned int, uid_t, int);
 int     vm_opentty(struct vmd_vm *);
 void    vm_closetty(struct vmd_vm *);
 void    switch_remove(struct vmd_switch *);