api: disallow virConnectGetDomainCapabilities on read-only connections

This API can be used to execute arbitrary emulators.
Forbid it on read-only connections.

Fixes: CVE-2019-10167
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
(cherry picked from commit 8afa68bac0)
Signed-off-by: Ján Tomko <jtomko@redhat.com>

cfg.mk

@@ -1158,7 +1158,7 @@ _src2=src/(util/vircommand|libvirt|lxc/lxc_controller|locking/lock_daemon|loggin
 exclude_file_name_regexp--sc_prohibit_fork_wrappers = \
   (^($(_src2)|tests/testutils|daemon/libvirtd)\.c$$)
 
-exclude_file_name_regexp--sc_prohibit_gethostname = ^src/util/virutil\.c$$
+exclude_file_name_regexp--sc_prohibit_gethostname = ^src/util/vir(util|log)\.c$$
 
 exclude_file_name_regexp--sc_prohibit_internal_functions = \
   ^src/(util/(viralloc|virutil|virfile)\.[hc]|esx/esx_vi\.c)$$

src/libvirt-domain.c

@@ -1073,9 +1073,7 @@ virDomainRestoreFlags(virConnectPtr conn, const char *from, const char *dxml,
  * previously by virDomainSave() or virDomainSaveFlags().
  *
  * No security-sensitive data will be included unless @flags contains
- * VIR_DOMAIN_XML_SECURE; this flag is rejected on read-only
+ * VIR_DOMAIN_XML_SECURE.
- * connections.  For this API, @flags should not contain either
- * VIR_DOMAIN_XML_INACTIVE or VIR_DOMAIN_XML_UPDATE_CPU.
  *
  * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of
  * error.  The caller must free() the returned value.
@@ -1091,12 +1089,7 @@ virDomainSaveImageGetXMLDesc(virConnectPtr conn, const char *file,
 
     virCheckConnectReturn(conn, NULL);
     virCheckNonNullArgGoto(file, error);
-
+    virCheckReadOnlyGoto(conn->flags, error);
-    if ((conn->flags & VIR_CONNECT_RO) && (flags & VIR_DOMAIN_XML_SECURE)) {
-        virReportError(VIR_ERR_OPERATION_DENIED, "%s",
-                       _("virDomainSaveImageGetXMLDesc with secure flag"));
-        goto error;
-    }
 
     if (conn->driver->domainSaveImageGetXMLDesc) {
         char *ret;
@@ -9428,6 +9421,7 @@ virDomainManagedSaveDefineXML(virDomainPtr domain, const char *dxml,
 
     virCheckDomainReturn(domain, -1);
     conn = domain->conn;
+    virCheckReadOnlyGoto(conn->flags, error);
 
     if (conn->driver->domainManagedSaveDefineXML) {
         int ret;
@@ -11219,6 +11213,7 @@ virConnectGetDomainCapabilities(virConnectPtr conn,
     virResetLastError();
 
     virCheckConnectReturn(conn, NULL);
+    virCheckReadOnlyGoto(conn->flags, error);
 
     if (conn->driver->connectGetDomainCapabilities) {
         char *ret;

src/nwfilter/nwfilter_dhcpsnoop.c

@@ -253,10 +253,21 @@ struct _virNWFilterDHCPDecodeJob {
 # define DHCP_BURST_INTERVAL_S  10 /* sec */
 
 /*
- * libpcap 1.5 requires a 128kb buffer
+ * NB: Any libpcap built with HAVE_TPACKET3 will require
- * 128 kb is bigger than (DHCP_PKT_BURST * PCAP_PBUFSIZE / 2)
+ * PCAP_BUFFERSIZE to be at least 262144 (although
+ * pcap_set_buffer_size() with a lower value will succeed, and the
+ * error will only show up later when pcap_setfilter() is called).
+ *
+ * It is possible that in the future libpcap could increase the
+ * minimum size even further, but due to the fact that each guest
+ * using dhcp snooping keeps 2 pcap sockets open (and thus 2 buffers
+ * allocated) for the life of the guest, we want to minimize the
+ * length of the buffer, so instead of leaving it at the default size
+ * (2MB), we are setting it to the minimum viable size and including
+ * this clue in the source to help quickly resolve the problem when/if
+ * it reoccurs.
  */
-# define PCAP_BUFFERSIZE        (128 * 1024)
+# define PCAP_BUFFERSIZE        (256 * 1024)
 
 # define MAX_QUEUED_JOBS        (DHCP_PKT_BURST + 2 * DHCP_PKT_RATE)
 
@@ -1113,6 +1124,11 @@ virNWFilterSnoopDHCPOpen(const char *ifname, virMacAddr *mac,
         goto cleanup_nohandle;
     }
 
+    /* IMPORTANT: If there is any failure of *any* pcap_* function
+     * during setup of the socket, look to the comment where
+     * PCAP_BUFFERSIZE is defined. It may be too small, even if the
+     * generated error doesn't imply that.
+     */
     if (pcap_set_snaplen(handle, PCAP_PBUFSIZE) < 0 ||
         pcap_set_buffer_size(handle, PCAP_BUFFERSIZE) < 0 ||
         pcap_activate(handle) < 0) {

src/qemu/qemu_command.c

@@ -718,7 +718,7 @@ qemuBuildTLSx509BackendProps(const char *tlspath,
     if (virJSONValueObjectCreate(propsret,
                                  "s:dir", path,
                                  "s:endpoint", (isListen ? "server": "client"),
-                                 "b:verify-peer", verifypeer,
+                                 "b:verify-peer", (isListen ? verifypeer : true),
                                  NULL) < 0)
         goto cleanup;
 

src/qemu/qemu_domain.c

@@ -8837,13 +8837,23 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED,
 
     if (isLink) {
         VIR_DEBUG("Creating symlink %s -> %s", data->file, data->target);
+
+        /* First, unlink the symlink target. Symlinks change and
+         * therefore we have no guarantees that pre-existing
+         * symlink is still valid. */
+        if (unlink(data->file) < 0 &&
+            errno != ENOENT) {
+            virReportSystemError(errno,
+                                 _("Unable to remove symlink %s"),
+                                 data->file);
+            goto cleanup;
+        }
+
         if (symlink(data->target, data->file) < 0) {
-            if (errno != EEXIST) {
+            virReportSystemError(errno,
-                virReportSystemError(errno,
+                                 _("Unable to create symlink %s (pointing to %s)"),
-                                     _("Unable to create symlink %s"),
+                                 data->file, data->target);
-                                     data->target);
+            goto cleanup;
-                goto cleanup;
-            }
         } else {
             delDevice = true;
         }

src/qemu/qemu_driver.c

@@ -6712,7 +6712,7 @@ qemuDomainSaveImageGetXMLDesc(virConnectPtr conn, const char *path,
     if (fd < 0)
         goto cleanup;
 
-    if (virDomainSaveImageGetXMLDescEnsureACL(conn, def, flags) < 0)
+    if (virDomainSaveImageGetXMLDescEnsureACL(conn, def) < 0)
         goto cleanup;
 
     ret = qemuDomainDefFormatXML(driver, def, flags);

src/qemu/qemu_monitor.c

@@ -434,8 +434,8 @@ qemuMonitorIOProcess(qemuMonitorPtr mon)
 # endif
 #endif
 
-    PROBE(QEMU_MONITOR_IO_PROCESS,
+    PROBE_QUIET(QEMU_MONITOR_IO_PROCESS, "mon=%p buf=%s len=%zu",
-          "mon=%p buf=%s len=%zu", mon, mon->buffer, mon->bufferOffset);
+                mon, mon->buffer, mon->bufferOffset);
 
     if (mon->json)
         len = qemuMonitorJSONIOProcess(mon,

src/qemu/qemu_monitor_json.c

@@ -259,7 +259,10 @@ int qemuMonitorJSONIOProcess(qemuMonitorPtr mon,
         }
     }
 
+#if DEBUG_IO
     VIR_DEBUG("Total used %d bytes out of %zd available in buffer", used, len);
+#endif
+
     return used;
 }
 

src/remote/remote_protocol.x

@@ -5103,8 +5103,7 @@ enum remote_procedure {
     /**
      * @generate: both
      * @priority: high
-     * @acl: domain:read
+     * @acl: domain:write
-     * @acl: domain:read_secure:VIR_DOMAIN_XML_SECURE
      */
     REMOTE_PROC_DOMAIN_SAVE_IMAGE_GET_XML_DESC = 235,
 

src/util/virlog.c

@@ -64,6 +64,7 @@
 VIR_LOG_INIT("util.log");
 
 static regex_t *virLogRegex;
+static char virLogHostname[HOST_NAME_MAX+1];
 
 
 #define VIR_LOG_DATE_REGEX "[0-9]{4}-[0-9]{2}-[0-9]{2}"
@@ -260,6 +261,8 @@ virLogPriorityString(virLogPriority lvl)
 static int
 virLogOnceInit(void)
 {
+    int r;
+
     if (virMutexInit(&virLogMutex) < 0)
         return -1;
 
@@ -271,6 +274,21 @@ virLogOnceInit(void)
             VIR_FREE(virLogRegex);
     }
 
+    /* We get and remember the hostname early, because at later time
+     * it might not be possible to load NSS modules via getaddrinfo()
+     * (e.g. at container startup the host filesystem will not be
+     * accessible anymore.
+     * Must not use virGetHostname though as that causes re-entrancy
+     * problems if it triggers logging codepaths
+     */
+    r = gethostname(virLogHostname, sizeof(virLogHostname));
+    if (r == -1) {
+        ignore_value(virStrcpy(virLogHostname,
+                               "(unknown)", sizeof(virLogHostname)));
+    } else {
+        NUL_TERMINATE(virLogHostname);
+    }
+
     virLogUnlock();
     return 0;
 }
@@ -466,18 +484,11 @@ static int
 virLogHostnameString(char **rawmsg,
                      char **msg)
 {
-    char *hostname = virGetHostnameQuiet();
     char *hoststr;
 
-    if (!hostname)
+    if (virAsprintfQuiet(&hoststr, "hostname: %s", virLogHostname) < 0)
         return -1;
 
-    if (virAsprintfQuiet(&hoststr, "hostname: %s", hostname) < 0) {
-        VIR_FREE(hostname);
-        return -1;
-    }
-    VIR_FREE(hostname);
-
     if (virLogFormatString(msg, 0, NULL, VIR_LOG_INFO, hoststr) < 0) {
         VIR_FREE(hoststr);
         return -1;

src/util/virprobe.h

@@ -90,11 +90,19 @@
         PROBE_EXPAND(LIBVIRT_ ## NAME,                       \
                      VIR_ADD_CASTS(__VA_ARGS__));            \
     }
+
+#  define PROBE_QUIET(NAME, FMT, ...) \
+    if (LIBVIRT_ ## NAME ## _ENABLED()) { \
+        PROBE_EXPAND(LIBVIRT_ ## NAME, \
+                     VIR_ADD_CASTS(__VA_ARGS__)); \
+    }
 # else
 #  define PROBE(NAME, FMT, ...)                              \
     VIR_INFO_INT(&virLogSelf,                                \
                  __FILE__, __LINE__, __func__,               \
                  #NAME ": " FMT, __VA_ARGS__);
+
+#  define PROBE_QUIET(NAME, FMT, ...)
 # endif
 
 #endif /* __VIR_PROBE_H__ */

src/util/virtpm.c

@@ -61,9 +61,7 @@ virTPMCreateCancelPath(const char *devpath)
                 VIR_FREE(path);
             }
             if (!path)
-                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                ignore_value(VIR_STRDUP(path, "/dev/null"));
-                               _("No usable sysfs TPM cancel file could be "
-                                 "found"));
         } else {
             virReportError(VIR_ERR_INTERNAL_ERROR,
                            _("TPM device path %s is invalid"), devpath);

src/vbox/vbox_common.c

@@ -3604,7 +3604,7 @@ vboxDumpAudio(virDomainDefPtr def, vboxDriverPtr data ATTRIBUTE_UNUSED,
     }
 }
 
-static void
+static int
 vboxDumpSerial(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine, PRUint32 serialPortCount)
 {
     PRUint32 serialPortIncCount = 0;
@@ -3628,9 +3628,15 @@ vboxDumpSerial(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine, PRUin
     }
 
     /* Allocate memory for the serial ports which are enabled */
-    if ((def->nserials > 0) && (VIR_ALLOC_N(def->serials, def->nserials) >= 0)) {
+    if (def->nserials > 0) {
-        for (i = 0; i < def->nserials; i++)
+        if (VIR_ALLOC_N(def->serials, def->nserials) < 0)
-            ignore_value(VIR_ALLOC(def->serials[i]));
+            return -1;
+
+        for (i = 0; i < def->nserials; i++) {
+            def->serials[i] = virDomainChrDefNew(NULL);
+            if (!def->serials[i])
+                return -1;
+        }
     }
 
     /* Now get the details about the serial ports here */
@@ -3678,7 +3684,8 @@ vboxDumpSerial(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine, PRUin
 
                 if (pathUtf16) {
                     VBOX_UTF16_TO_UTF8(pathUtf16, &path);
-                    ignore_value(VIR_STRDUP(def->serials[serialPortIncCount]->source->data.file.path, path));
+                    if (VIR_STRDUP(def->serials[serialPortIncCount]->source->data.file.path, path) < 0)
+                        return -1;
                 }
 
                 serialPortIncCount++;
@@ -3690,9 +3697,10 @@ vboxDumpSerial(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine, PRUin
             VBOX_RELEASE(serialPort);
         }
     }
+    return 0;
 }
 
-static void
+static int
 vboxDumpParallel(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine, PRUint32 parallelPortCount)
 {
     PRUint32 parallelPortIncCount = 0;
@@ -3716,9 +3724,15 @@ vboxDumpParallel(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine, PRU
     }
 
     /* Allocate memory for the parallel ports which are enabled */
-    if ((def->nparallels > 0) && (VIR_ALLOC_N(def->parallels, def->nparallels) >= 0)) {
+    if (def->nparallels > 0) {
-        for (i = 0; i < def->nparallels; i++)
+        if (VIR_ALLOC_N(def->parallels, def->nparallels) < 0)
-            ignore_value(VIR_ALLOC(def->parallels[i]));
+            return -1;
+
+        for (i = 0; i < def->nparallels; i++) {
+            def->parallels[i] = virDomainChrDefNew(NULL);
+            if (!def->parallels[i])
+                return -1;
+        }
     }
 
     /* Now get the details about the parallel ports here */
@@ -3753,7 +3767,8 @@ vboxDumpParallel(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine, PRU
                 gVBoxAPI.UIParallelPort.GetPath(parallelPort, &pathUtf16);
 
                 VBOX_UTF16_TO_UTF8(pathUtf16, &path);
-                ignore_value(VIR_STRDUP(def->parallels[parallelPortIncCount]->source->data.file.path, path));
+                if (VIR_STRDUP(def->parallels[parallelPortIncCount]->source->data.file.path, path) < 0)
+                    return -1;
 
                 parallelPortIncCount++;
 
@@ -3764,6 +3779,7 @@ vboxDumpParallel(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine, PRU
             VBOX_RELEASE(parallelPort);
         }
     }
+    return 0;
 }
 
 static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags)
@@ -3902,8 +3918,11 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags)
     vboxDumpSharedFolders(def, data, machine);
     vboxDumpNetwork(def, data, machine, networkAdapterCount);
     vboxDumpAudio(def, data, machine);
-    vboxDumpSerial(def, data, machine, serialPortCount);
+
-    vboxDumpParallel(def, data, machine, parallelPortCount);
+    if (vboxDumpSerial(def, data, machine, serialPortCount) < 0)
+        goto cleanup;
+    if (vboxDumpParallel(def, data, machine, parallelPortCount) < 0)
+        goto cleanup;
 
     /* dump USB devices/filters if active */
     vboxHostDeviceGetXMLDesc(data, def, machine);

tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev.args

@@ -26,7 +26,7 @@ server,nowait \
 localport=1111 \
 -device isa-serial,chardev=charserial0,id=serial0 \
 -object tls-creds-x509,id=objcharserial1_tls0,dir=/etc/pki/libvirt-chardev,\
-endpoint=client,verify-peer=no \
+endpoint=client,verify-peer=yes \
 -chardev socket,id=charserial1,host=127.0.0.1,port=5555,\
 tls-creds=objcharserial1_tls0 \
 -device isa-serial,chardev=charserial1,id=serial1 \

tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-secret-chardev.args

@@ -31,7 +31,7 @@ localport=1111 \
 data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\
 keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \
 -object tls-creds-x509,id=objcharserial1_tls0,dir=/etc/pki/libvirt-chardev,\
-endpoint=client,verify-peer=no,passwordid=charserial1-secret0 \
+endpoint=client,verify-peer=yes,passwordid=charserial1-secret0 \
 -chardev socket,id=charserial1,host=127.0.0.1,port=5555,\
 tls-creds=objcharserial1_tls0 \
 -device isa-serial,chardev=charserial1,id=serial1 \