qemu: Let empty default VNC password work as documented

CVE-2016-5008

Setting an empty graphics password is documented as a way to disable
VNC/SPICE access, but QEMU does not always behaves like that. VNC would
happily accept the empty password. Let's enforce the behavior by setting
password expiration to "now".

https://bugzilla.redhat.com/show_bug.cgi?id=1180092

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
(cherry picked from commit bb848feec0)

src/libvirt_private.syms

@@ -1447,6 +1447,7 @@ virFileSanitizePath;
 virFileSkipRoot;
 virFileStripSuffix;
 virFileTouch;
+virFileUnlink;
 virFileUnlock;
 virFileUpdatePerm;
 virFileWaitForDevices;

src/libvirt_remote.syms

@@ -65,6 +65,7 @@ virNetDaemonAddSignalHandler;
 virNetDaemonAutoShutdown;
 virNetDaemonClose;
 virNetDaemonGetServer;
+virNetDaemonHasClients;
 virNetDaemonIsPrivileged;
 virNetDaemonNew;
 virNetDaemonNewPostExecRestart;

src/lxc/lxc_driver.c

@@ -597,7 +597,7 @@ static int lxcDomainGetInfo(virDomainPtr dom,
 
     if (!virDomainObjIsActive(vm)) {
         info->cpuTime = 0;
-        info->memory = 0;
+        info->memory = vm->def->mem.cur_balloon;
     } else {
         if (virCgroupGetCpuacctUsage(priv->cgroup, &(info->cpuTime)) < 0) {
             virReportError(VIR_ERR_OPERATION_FAILED,

src/qemu/qemu_driver.c

@@ -2641,13 +2641,13 @@ qemuDomainGetInfo(virDomainPtr dom,
         goto cleanup;
     }
 
-    if (virDomainObjIsActive(vm)) {
-        if (VIR_ASSIGN_IS_OVERFLOW(info->memory, vm->def->mem.cur_balloon)) {
-            virReportError(VIR_ERR_OVERFLOW, "%s",
-                           _("Current memory size too large"));
-            goto cleanup;
-        }
+    if (VIR_ASSIGN_IS_OVERFLOW(info->memory, vm->def->mem.cur_balloon)) {
+        virReportError(VIR_ERR_OVERFLOW, "%s",
+                       _("Current memory size too large"));
+        goto cleanup;
+    }
 
+    if (virDomainObjIsActive(vm)) {
         if (qemuGetProcessInfo(&(info->cpuTime), NULL, NULL, vm->pid, 0) < 0) {
             virReportError(VIR_ERR_OPERATION_FAILED, "%s",
                            _("cannot read cputime for domain"));

src/qemu/qemu_hotplug.c

@@ -4026,6 +4026,7 @@ qemuDomainChangeGraphicsPasswords(virQEMUDriverPtr driver,
     time_t now = time(NULL);
     char expire_time [64];
     const char *connected = NULL;
+    const char *password;
     int ret = -1;
     virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
 
@@ -4033,16 +4034,14 @@ qemuDomainChangeGraphicsPasswords(virQEMUDriverPtr driver,
         ret = 0;
         goto cleanup;
     }
+    password = auth->passwd ? auth->passwd : defaultPasswd;
 
     if (auth->connected)
         connected = virDomainGraphicsAuthConnectedTypeToString(auth->connected);
 
     if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
         goto cleanup;
-    ret = qemuMonitorSetPassword(priv->mon,
-                                 type,
-                                 auth->passwd ? auth->passwd : defaultPasswd,
-                                 connected);
+    ret = qemuMonitorSetPassword(priv->mon, type, password, connected);
 
     if (ret == -2) {
         if (type != VIR_DOMAIN_GRAPHICS_TYPE_VNC) {
@@ -4050,14 +4049,15 @@ qemuDomainChangeGraphicsPasswords(virQEMUDriverPtr driver,
                            _("Graphics password only supported for VNC"));
             ret = -1;
         } else {
-            ret = qemuMonitorSetVNCPassword(priv->mon,
-                                            auth->passwd ? auth->passwd : defaultPasswd);
+            ret = qemuMonitorSetVNCPassword(priv->mon, password);
         }
     }
     if (ret != 0)
         goto end_job;
 
-    if (auth->expires) {
+    if (password[0] == '\0') {
+        snprintf(expire_time, sizeof(expire_time), "now");
+    } else if (auth->expires) {
         time_t lifetime = auth->validTo - now;
         if (lifetime <= 0)
             snprintf(expire_time, sizeof(expire_time), "now");

src/remote/remote_driver.c

@@ -546,10 +546,6 @@ remoteClientCloseFunc(virNetClientPtr client ATTRIBUTE_UNUSED,
         cbdata->freeCallback = NULL;
     }
     virObjectUnlock(cbdata);
-
-    /* free the connection reference that comes along with the callback
-     * registration */
-    virObjectUnref(cbdata->conn);
 }
 
 /* helper macro to ease extraction of arguments from the URI */

src/rpc/virnetdaemon.c

@@ -682,23 +682,17 @@ virNetDaemonRun(virNetDaemonPtr dmn)
          */
         if (dmn->autoShutdownTimeout) {
             if (timerActive) {
-                for (i = 0; i < dmn->nservers; i++) {
-                    if (virNetServerHasClients(dmn->servers[i])) {
-                        VIR_DEBUG("Deactivating shutdown timer %d", timerid);
-                        virEventUpdateTimeout(timerid, -1);
-                        timerActive = false;
-                        break;
-                    }
+                if (virNetDaemonHasClients(dmn)) {
+                    VIR_DEBUG("Deactivating shutdown timer %d", timerid);
+                    virEventUpdateTimeout(timerid, -1);
+                    timerActive = false;
                 }
             } else {
-                for (i = 0; i < dmn->nservers; i++) {
-                    if (!virNetServerHasClients(dmn->servers[i])) {
-                        VIR_DEBUG("Activating shutdown timer %d", timerid);
-                        virEventUpdateTimeout(timerid,
-                                              dmn->autoShutdownTimeout * 1000);
-                        timerActive = true;
-                        break;
-                    }
+                if (!virNetDaemonHasClients(dmn)) {
+                    VIR_DEBUG("Activating shutdown timer %d", timerid);
+                    virEventUpdateTimeout(timerid,
+                                          dmn->autoShutdownTimeout * 1000);
+                    timerActive = true;
                 }
             }
         }
@@ -747,3 +741,16 @@ virNetDaemonClose(virNetDaemonPtr dmn)
 
     virObjectUnlock(dmn);
 }
+
+bool
+virNetDaemonHasClients(virNetDaemonPtr dmn)
+{
+    size_t i = 0;
+
+    for (i = 0; i < dmn->nservers; i++) {
+        if (virNetServerHasClients(dmn->servers[i]))
+            return true;
+    }
+
+    return false;
+}

src/rpc/virnetdaemon.h

@@ -76,6 +76,8 @@ void virNetDaemonQuit(virNetDaemonPtr dmn);
 
 void virNetDaemonClose(virNetDaemonPtr dmn);
 
+bool virNetDaemonHasClients(virNetDaemonPtr dmn);
+
 virNetServerPtr virNetDaemonGetServer(virNetDaemonPtr dmn,
                                       int subServerID);
 

src/storage/storage_backend.c

@@ -479,6 +479,7 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED,
     int fd = -1;
     int operation_flags;
     bool reflink_copy = false;
+    mode_t open_mode = VIR_STORAGE_DEFAULT_VOL_PERM_MODE;
 
     virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA |
                   VIR_STORAGE_VOL_CREATE_REFLINK,
@@ -511,11 +512,12 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED,
     if (pool->def->type == VIR_STORAGE_POOL_NETFS)
         operation_flags |= VIR_FILE_OPEN_FORK;
 
+    if (vol->target.perms->mode != (mode_t) -1)
+        open_mode = vol->target.perms->mode;
+
     if ((fd = virFileOpenAs(vol->target.path,
                             O_RDWR | O_CREAT | O_EXCL,
-                            (vol->target.perms->mode ?
-                             VIR_STORAGE_DEFAULT_VOL_PERM_MODE :
-                             vol->target.perms->mode),
+                            open_mode,
                             vol->target.perms->uid,
                             vol->target.perms->gid,
                             operation_flags)) < 0) {

src/storage/storage_backend_fs.c

@@ -1,7 +1,7 @@
 /*
  * storage_backend_fs.c: storage backend for FS and directory handling
  *
- * Copyright (C) 2007-2014 Red Hat, Inc.
+ * Copyright (C) 2007-2015 Red Hat, Inc.
  * Copyright (C) 2007-2008 Daniel P. Berrange
  *
  * This library is free software; you can redistribute it and/or
@@ -784,7 +784,8 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED,
     char *parent = NULL;
     char *p = NULL;
     mode_t mode;
-    bool needs_create_as_uid, dir_create_flags;
+    bool needs_create_as_uid;
+    unsigned int dir_create_flags;
 
     virCheckFlags(VIR_STORAGE_POOL_BUILD_OVERWRITE |
                   VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, ret);
@@ -1056,6 +1057,14 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn ATTRIBUTE_UNUSED,
     else
         vol->type = VIR_STORAGE_VOL_FILE;
 
+    /* Volumes within a directory pools are not recursive; do not
+     * allow escape to ../ or a subdir */
+    if (strchr(vol->name, '/')) {
+        virReportError(VIR_ERR_OPERATION_INVALID,
+                       _("volume name '%s' cannot contain '/'"), vol->name);
+        return -1;
+    }
+
     VIR_FREE(vol->target.path);
     if (virAsprintf(&vol->target.path, "%s/%s",
                     pool->def->target.path,
@@ -1202,7 +1211,8 @@ virStorageBackendFileSystemVolDelete(virConnectPtr conn ATTRIBUTE_UNUSED,
 
     switch ((virStorageVolType) vol->type) {
     case VIR_STORAGE_VOL_FILE:
-        if (unlink(vol->target.path) < 0) {
+        if (virFileUnlink(vol->target.path, vol->target.perms->uid,
+                          vol->target.perms->gid) < 0) {
             /* Silently ignore failures where the vol has already gone away */
             if (errno != ENOENT) {
                 virReportSystemError(errno,

src/storage/storage_driver.c

@@ -1864,8 +1864,12 @@ storageVolCreateXML(virStoragePoolPtr obj,
         goto cleanup;
 
     if (backend->refreshVol &&
-        backend->refreshVol(obj->conn, pool, voldef) < 0)
+        backend->refreshVol(obj->conn, pool, voldef) < 0) {
+        storageVolDeleteInternal(volobj, backend, pool, voldef,
+                                 0, false);
+        voldef = NULL;
         goto cleanup;
+    }
 
     /* Update pool metadata ignoring the disk backend since
      * it updates the pool values.

src/util/virfile.c

@@ -2280,6 +2280,112 @@ virFileOpenAs(const char *path, int openflags, mode_t mode,
     return ret;
 }
 
+
+/* virFileUnlink:
+ * @path: file to unlink
+ * @uid: uid that was used to create the file (not required)
+ * @gid: gid that was used to create the file (not required)
+ *
+ * If a file/volume was created in an NFS root-squash environment,
+ * then we must 'unlink' the file in the same environment. Unlike
+ * the virFileOpenAs[Forked] and virDirCreate[NoFork], this code
+ * takes no extra flags and does not bother with EACCES failures
+ * from the child.
+ */
+int
+virFileUnlink(const char *path,
+              uid_t uid,
+              gid_t gid)
+{
+    pid_t pid;
+    int waitret;
+    int status, ret = 0;
+    gid_t *groups;
+    int ngroups;
+
+    /* If not running as root or if a non explicit uid/gid was being used for
+     * the file/volume, then use unlink directly
+     */
+    if ((geteuid() != 0) ||
+        ((uid == (uid_t) -1) && (gid == (gid_t) -1)))
+        return unlink(path);
+
+    /* Otherwise, we have to deal with the NFS root-squash craziness
+     * to run under the uid/gid that created the volume in order to
+     * perform the unlink of the volume.
+     */
+    if (uid == (uid_t) -1)
+        uid = geteuid();
+    if (gid == (gid_t) -1)
+        gid = getegid();
+
+    ngroups = virGetGroupList(uid, gid, &groups);
+    if (ngroups < 0)
+        return -errno;
+
+    pid = virFork();
+
+    if (pid < 0) {
+        ret = -errno;
+        VIR_FREE(groups);
+        return ret;
+    }
+
+    if (pid) { /* parent */
+        /* wait for child to complete, and retrieve its exit code */
+        VIR_FREE(groups);
+
+        while ((waitret = waitpid(pid, &status, 0)) == -1 && errno == EINTR);
+        if (waitret == -1) {
+            ret = -errno;
+            virReportSystemError(errno,
+                                 _("failed to wait for child unlinking '%s'"),
+                                 path);
+            goto parenterror;
+        }
+
+        /*
+         * If waitpid succeeded, but if the child exited abnormally or
+         * reported non-zero status, report failure
+         */
+        if (!WIFEXITED(status) || (WEXITSTATUS(status)) != 0) {
+            char *msg = virProcessTranslateStatus(status);
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("child failed to unlink '%s': %s"),
+                           path, msg);
+            VIR_FREE(msg);
+            if (WIFEXITED(status))
+                ret = -WEXITSTATUS(status);
+            else
+                ret = -EACCES;
+        }
+
+ parenterror:
+        return ret;
+    }
+
+    /* child */
+
+    /* set desired uid/gid, then attempt to unlink the file */
+    if (virSetUIDGID(uid, gid, groups, ngroups) < 0) {
+        ret = errno;
+        goto childerror;
+    }
+
+    if (unlink(path) < 0) {
+        ret = errno;
+        goto childerror;
+    }
+
+ childerror:
+    if ((ret & 0xff) != ret) {
+        VIR_WARN("unable to pass desired return value %d", ret);
+        ret = 0xff;
+    }
+    _exit(ret);
+}
+
+
 /* return -errno on failure, or 0 on success */
 static int
 virDirCreateNoFork(const char *path,

src/util/virfile.h

@@ -219,6 +219,7 @@ int virFileOpenAs(const char *path, int openflags, mode_t mode,
                   uid_t uid, gid_t gid,
                   unsigned int flags)
     ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
+int virFileUnlink(const char *path, uid_t uid, gid_t gid);
 
 enum {
     VIR_DIR_CREATE_NONE        = 0,