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

This prevents time of TOCTOU attacks for instances.

OK mlarkin@

3 files changed, 49 insertions, 58 deletions

diff --git a/usr.sbin/vmd/config.c b/usr.sbin/vmd/config.c
index fc8114a9e5e..5f6db1afba8 100644
--- 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 9a148e288e6..da184629dc5 100644
--- 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 7da53185887..bf670f5eb95 100644
--- 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 *);