[libvirt] [SECURITY] PATCH: Fix missing read-only access checks (CVE-2008-5086)

[Daniel P. Berrange]

The following methods in libvirt.c are missing a check against the
read-only connection flag:

    virDomainMigrate
    virDomainMigratePrepare
    virDomainMigratePerform
    virDomainMigrateFinish
    virDomainMigratePrepare2
    virDomainMigrateFinish2
    virDomainBlockPeek
    virDomainMemoryPeek
    virDomainSetAutostart
    virNetworkSetAutostart
    virConnectFindStoragePoolSources
    virStoragePoolSetAutostart

If using PolicyKit auth, the default policy will allow any local user 
to make a read-only connection to the libvirtd daemon without needing
authentication.

If not using PolicyKit, the default libvirtd.conf configuration settings
will allow an unprivileged user to make a read-only connection to the
libvirtd daemon without needing authentication. 

Thus out of the box unprivileged local users may be able to migrate VMs,
set or unset the autostart flag for domains, networks & storage pools,
and access privileged data in the VM memory, or disks.

All TCP remote connections are read-write, and default settings require
full authentication, thus remote access is not impacted by this flaw.

Administrators can apply a workaround by editting /etc/libvirt/libvirtd.conf
to explicitly set 'unix_sock_ro_perms'   parameter to  '0700'. Restart the
libvirtd daemon after making this change.

The first vulnerable release was 0.3.2, where the virDomainMigrate API
was added for the Xen driver. Other APIs were added in various subsequent
releases depending on the hypervisor driver in question.

The attached patch has been committed to CVS, and OS distributors are
recommended to apply this patch to all existing releases shipped. It
was diff'd against current CVS head, and applies against 0.5.1, and
is trivially re-diffable for all earlier releases.

This flaw has been assigned the identifier CVE-2008-5086

Daniel

diff --git a/src/libvirt.c b/src/libvirt.c
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -2296,6 +2296,16 @@ virDomainMigrate (virDomainPtr domain,
     conn = domain->conn;        /* Source connection. */
     if (!VIR_IS_CONNECT (dconn)) {
         virLibConnError (conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
+        return NULL;
+    }
+
+    if (domain->conn->flags & VIR_CONNECT_RO) {
+        virLibDomainError(domain, VIR_ERR_OPERATION_DENIED, __FUNCTION__);
+        return NULL;
+    }
+    if (dconn->flags & VIR_CONNECT_RO) {
+        /* NB, delibrately report error against source object, not dest here */
+        virLibDomainError(domain, VIR_ERR_OPERATION_DENIED, __FUNCTION__);
         return NULL;
     }
 
@@ -2426,6 +2436,11 @@ virDomainMigratePrepare (virConnectPtr d
         return -1;
     }
 
+    if (dconn->flags & VIR_CONNECT_RO) {
+        virLibConnError(dconn, VIR_ERR_OPERATION_DENIED, __FUNCTION__);
+        return -1;
+    }
+
     if (dconn->driver->domainMigratePrepare)
         return dconn->driver->domainMigratePrepare (dconn, cookie, cookielen,
                                                     uri_in, uri_out,
@@ -2457,6 +2472,11 @@ virDomainMigratePerform (virDomainPtr do
     }
     conn = domain->conn;
 
+    if (domain->conn->flags & VIR_CONNECT_RO) {
+        virLibDomainError(domain, VIR_ERR_OPERATION_DENIED, __FUNCTION__);
+        return -1;
+    }
+
     if (conn->driver->domainMigratePerform)
         return conn->driver->domainMigratePerform (domain, cookie, cookielen,
                                                    uri,
@@ -2482,6 +2502,11 @@ virDomainMigrateFinish (virConnectPtr dc
 
     if (!VIR_IS_CONNECT (dconn)) {
         virLibConnError (NULL, VIR_ERR_INVALID_CONN, __FUNCTION__);
+        return NULL;
+    }
+
+    if (dconn->flags & VIR_CONNECT_RO) {
+        virLibConnError(dconn, VIR_ERR_OPERATION_DENIED, __FUNCTION__);
         return NULL;
     }
 
@@ -2517,6 +2542,11 @@ virDomainMigratePrepare2 (virConnectPtr 
         return -1;
     }
 
+    if (dconn->flags & VIR_CONNECT_RO) {
+        virLibConnError(dconn, VIR_ERR_OPERATION_DENIED, __FUNCTION__);
+        return -1;
+    }
+
     if (dconn->driver->domainMigratePrepare2)
         return dconn->driver->domainMigratePrepare2 (dconn, cookie, cookielen,
                                                      uri_in, uri_out,
@@ -2547,6 +2577,11 @@ virDomainMigrateFinish2 (virConnectPtr d
         return NULL;
     }
 
+    if (dconn->flags & VIR_CONNECT_RO) {
+        virLibConnError(dconn, VIR_ERR_OPERATION_DENIED, __FUNCTION__);
+        return NULL;
+    }
+
     if (dconn->driver->domainMigrateFinish2)
         return dconn->driver->domainMigrateFinish2 (dconn, dname,
                                                     cookie, cookielen,
@@ -2905,6 +2940,11 @@ virDomainBlockPeek (virDomainPtr dom,
     }
     conn = dom->conn;
 
+    if (dom->conn->flags & VIR_CONNECT_RO) {
+        virLibDomainError(dom, VIR_ERR_OPERATION_DENIED, __FUNCTION__);
+        return (-1);
+    }
+
     if (!path) {
         virLibDomainError (dom, VIR_ERR_INVALID_ARG,
                            _("path is NULL"));
@@ -2980,6 +3020,11 @@ virDomainMemoryPeek (virDomainPtr dom,
     }
     conn = dom->conn;
 
+    if (dom->conn->flags & VIR_CONNECT_RO) {
+        virLibDomainError(dom, VIR_ERR_OPERATION_DENIED, __FUNCTION__);
+        return (-1);
+    }
+
     /* Flags must be VIR_MEMORY_VIRTUAL at the moment.
      *
      * Note on access to physical memory: A VIR_MEMORY_PHYSICAL flag is
@@ -3246,6 +3291,11 @@ virDomainSetAutostart(virDomainPtr domai
     }
 
     conn = domain->conn;
+
+    if (domain->conn->flags & VIR_CONNECT_RO) {
+        virLibDomainError(domain, VIR_ERR_OPERATION_DENIED, __FUNCTION__);
+        return (-1);
+    }
 
     if (conn->driver->domainSetAutostart)
         return conn->driver->domainSetAutostart (domain, autostart);
@@ -4197,6 +4247,11 @@ virNetworkSetAutostart(virNetworkPtr net
         return (-1);
     }
 
+    if (network->conn->flags & VIR_CONNECT_RO) {
+        virLibNetworkError(network, VIR_ERR_OPERATION_DENIED, __FUNCTION__);
+        return (-1);
+    }
+
     conn = network->conn;
 
     if (conn->networkDriver && conn->networkDriver->networkSetAutostart)
@@ -4395,6 +4450,11 @@ virConnectFindStoragePoolSources(virConn
         return NULL;
     }
 
+    if (conn->flags & VIR_CONNECT_RO) {
+        virLibConnError(conn, VIR_ERR_OPERATION_DENIED, __FUNCTION__);
+        return NULL;
+    }
+
     if (conn->storageDriver && conn->storageDriver->findPoolSources)
         return conn->storageDriver->findPoolSources(conn, type, srcSpec, flags);
 
@@ -5068,6 +5128,11 @@ virStoragePoolSetAutostart(virStoragePoo
         return (-1);
     }
 
+    if (pool->conn->flags & VIR_CONNECT_RO) {
+        virLibStoragePoolError(pool, VIR_ERR_OPERATION_DENIED, __FUNCTION__);
+        return (-1);
+    }
+
     conn = pool->conn;
 
     if (conn->storageDriver && conn->storageDriver->poolSetAutostart)


-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|