[libvirt] [SECURITY] PATCH: Fix missing read-only access checks (CVE-2008-5086)
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 :|
On Wed, 17 Dec 2008, Daniel P. Berrange wrote:
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.
Dan (Walsh or otherwise), Any idea if the standard SELinux policy helps here? We'll need to assess policy for these sockets for sVirt, in any case.
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 :|
-- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- James Morris <jmorris@namei.org>
On Thu, Dec 18, 2008 at 09:23:27AM +1100, James Morris wrote:
On Wed, 17 Dec 2008, Daniel P. Berrange wrote:
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.
Dan (Walsh or otherwise),
Any idea if the standard SELinux policy helps here?
Nope, doesn't do anything in this context, because there is no policy controlling what actions you can invoke. The limited policy that exists thus far only serves to protect the host OS from the guest OS, not protect the guest OS' from malicious admins using the host mgmt APIs.
We'll need to assess policy for these sockets for sVirt, in any case.
It is not really the socket policy that's the problem - the policy is working exactly as intended - which was to allow users ability to monitor system status. eg in same way a user can use 'top' to monitor status of all processes in an OS, this read-only socket allows you to use 'virt-top' to monitor all VMs on a local host. The issues which contributed to this vulnerability are: 1. Inadequate code review of public APIs - many people reviewed the patches adding this public APIs and none of us noticed the missing read-only flag check 2. Bad code structure/model for permissions check, such that a missing permission check results in an allow-by-default scenario. With the code review issue, the problem is that of the hundreds of patches sent to the list, only a very few deal with new public facing APIs. It is only in the public facing APIs that the permissions checks are relevant, so reviewers are not used to checking for this as a general rule. To address this I suggest we create a list of mandatory review points for all new public APIs. Anyone posting a new public API should post the list of review points and how their patch complies. Reviewers then have a clear explicit list of things to validate. For the second problem, we need to refactor the code such that every single public API includes a permissions check in its entry point and centralize this check one place, and structure it such that a mistake results in a deny-unless-readonly policy by default. In the a past a number of people have suggested that we include some form of fine grained access control checks - perhaps RBAC, on a per-API basis, or even per-API, per-Object instead of just a global read-write vs read-only checks. Daniel -- |: 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 :|
participants (2)
-
Daniel P. Berrange -
James Morris