api: require write permission for guest agent interaction

I noticed that we allow virDomainGetVcpusFlags even for read-only
connections, but that with a flag, it can require guest agent
interaction.  It is feasible that a malicious guest could
intentionally abuse the replies it sends over the guest agent
connection to possibly trigger a bug in libvirt's JSON parser,
or withhold an answer so as to prevent the use of the agent
in a later command such as a shutdown request.  Although we
don't know of any such exploits now (and therefore don't mind
posting this patch publicly without trying to get a CVE assigned),
it is better to err on the side of caution and explicitly require
full access to any domain where the API requires guest interaction
to operate correctly.

I audited all commands that are marked as conditionally using a
guest agent.  Note that at least virDomainFSTrim is documented
as needing a guest agent, but that such use is unconditional
depending on the hypervisor (so the existing domain:fs_trim ACL
should be sufficient there, rather than also requirng domain:write).
But when designing future APIs, such as the plans for obtaining
a domain's IP addresses, we should copy the approach of this patch
in making interaction with the guest be specified via a flag, and
use that flag to also require stricter access checks.

* src/libvirt.c (virDomainGetVcpusFlags): Forbid guest interaction
on read-only connection.
(virDomainShutdownFlags, virDomainReboot): Improve docs on agent
interaction.
* src/remote/remote_protocol.x
(REMOTE_PROC_DOMAIN_SNAPSHOT_CREATE_XML)
(REMOTE_PROC_DOMAIN_SET_VCPUS_FLAGS)
(REMOTE_PROC_DOMAIN_GET_VCPUS_FLAGS, REMOTE_PROC_DOMAIN_REBOOT)
(REMOTE_PROC_DOMAIN_SHUTDOWN_FLAGS): Require domain:write for any
conditional use of a guest agent.
* src/xen/xen_driver.c: Fix clients.
* src/libxl/libxl_driver.c: Likewise.
* src/uml/uml_driver.c: Likewise.
* src/qemu/qemu_driver.c: Likewise.
* src/lxc/lxc_driver.c: Likewise.

Signed-off-by: Eric Blake <eblake@redhat.com>

diff --git a/src/libvirt.c b/src/libvirt.c
index 6a41fd76f507580a1f813b3e268b3a791ae712e2..c15e29a581b4e9c374fc751a8e00a2365a0ac844 100644 (file)
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -3134,6 +3134,9 @@ error:
  * in which the hypervisor tries each shutdown method is undefined,
  * and a hypervisor is not required to support all methods.
  *
+ * To use guest agent (VIR_DOMAIN_SHUTDOWN_GUEST_AGENT) the domain XML
+ * must have <channel> configured.
+ *
  * Returns 0 in case of success and -1 in case of failure.
  */
 int
@@ -3180,7 +3183,7 @@ error:
  *
  * If @flags is set to zero, then the hypervisor will choose the
  * method of shutdown it considers best. To have greater control
- * pass one or more of the virDomainShutdownFlagValues. The order
+ * pass one or more of the virDomainRebootFlagValues. The order
  * in which the hypervisor tries each shutdown method is undefined,
  * and a hypervisor is not required to support all methods.
  *
@@ -9347,7 +9350,7 @@ error:
  * current virtual CPU count.
  *
  * If @flags includes VIR_DOMAIN_VCPU_GUEST, then the state of the processors
- * is modified in the guest instead of the hypervisor. This flag is only usable
+ * is queried in the guest instead of the hypervisor. This flag is only usable
  * on live domains. Guest agent may be needed for this flag to be available.
  *
  * Returns the number of vCPUs in case of success, -1 in case of failure.
@@ -9362,6 +9365,10 @@ virDomainGetVcpusFlags(virDomainPtr domain, unsigned int flags)
     virResetLastError();
 
     virCheckDomainReturn(domain, -1);
+    conn = domain->conn;
+
+    if (flags & VIR_DOMAIN_VCPU_GUEST)
+        virCheckReadOnlyGoto(conn->flags, error);
 
     /* At most one of these two flags should be set.  */
     if ((flags & VIR_DOMAIN_AFFECT_LIVE) &&
@@ -9372,7 +9379,6 @@ virDomainGetVcpusFlags(virDomainPtr domain, unsigned int flags)
                             __FUNCTION__);
         goto error;
     }
-    conn = domain->conn;
 
     if (conn->driver->domainGetVcpusFlags) {
         int ret;

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 4115fffe3c374ad419245bf4904821453a2e537f..fc0efa2e5a91e765fc829959f8605816cb123fb7 100644 (file)
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -1409,7 +1409,7 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int flags)
     if (!(vm = libxlDomObjFromDomain(dom)))
         goto cleanup;
 
-    if (virDomainShutdownFlagsEnsureACL(dom->conn, vm->def) < 0)
+    if (virDomainShutdownFlagsEnsureACL(dom->conn, vm->def, flags) < 0)
         goto cleanup;
 
     if (!virDomainObjIsActive(vm)) {
@@ -1456,7 +1456,7 @@ libxlDomainReboot(virDomainPtr dom, unsigned int flags)
     if (!(vm = libxlDomObjFromDomain(dom)))
         goto cleanup;
 
-    if (virDomainRebootEnsureACL(dom->conn, vm->def) < 0)
+    if (virDomainRebootEnsureACL(dom->conn, vm->def, flags) < 0)
         goto cleanup;
 
     if (!virDomainObjIsActive(vm)) {
@@ -2316,7 +2316,7 @@ libxlDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags)
     if (!(vm = libxlDomObjFromDomain(dom)))
         goto cleanup;
 
-    if (virDomainGetVcpusFlagsEnsureACL(dom->conn, vm->def) < 0)
+    if (virDomainGetVcpusFlagsEnsureACL(dom->conn, vm->def, flags) < 0)
         goto cleanup;
 
     active = virDomainObjIsActive(vm);

diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index e3192344c2b7c03a6d0f11d0279a24fcf5412de6..982f3fc8c6ca062bf7506e4b06967609aed8d516 100644 (file)
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -3335,7 +3335,7 @@ lxcDomainShutdownFlags(virDomainPtr dom,
 
     priv = vm->privateData;
 
-    if (virDomainShutdownFlagsEnsureACL(dom->conn, vm->def) < 0)
+    if (virDomainShutdownFlagsEnsureACL(dom->conn, vm->def, flags) < 0)
         goto cleanup;
 
     if (!virDomainObjIsActive(vm)) {
@@ -3415,7 +3415,7 @@ lxcDomainReboot(virDomainPtr dom,
 
     priv = vm->privateData;
 
-    if (virDomainRebootEnsureACL(dom->conn, vm->def) < 0)
+    if (virDomainRebootEnsureACL(dom->conn, vm->def, flags) < 0)
         goto cleanup;
 
     if (!virDomainObjIsActive(vm)) {

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index bc2971446be7ebb13bfb8b2a251791028d8b19e8..6e2126760ca691cb5322ecff9eba2d6cb6504259 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1835,7 +1835,7 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, unsigned int flags) {
     if (agentRequested || (!flags && priv->agent))
         useAgent = true;
 
-    if (virDomainShutdownFlagsEnsureACL(dom->conn, vm->def) < 0)
+    if (virDomainShutdownFlagsEnsureACL(dom->conn, vm->def, flags) < 0)
         goto cleanup;
 
     if (priv->agentError) {
@@ -1936,7 +1936,7 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags)
 
     priv = vm->privateData;
 
-    if (virDomainRebootEnsureACL(dom->conn, vm->def) < 0)
+    if (virDomainRebootEnsureACL(dom->conn, vm->def, flags) < 0)
         goto cleanup;
 
     if ((flags & VIR_DOMAIN_REBOOT_GUEST_AGENT) ||
@@ -4898,7 +4898,7 @@ qemuDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags)
 
     priv = vm->privateData;
 
-    if (virDomainGetVcpusFlagsEnsureACL(dom->conn, vm->def) < 0)
+    if (virDomainGetVcpusFlagsEnsureACL(dom->conn, vm->def, flags) < 0)
         goto cleanup;
 
     if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
@@ -13090,7 +13090,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
 
     cfg = virQEMUDriverGetConfig(driver);
 
-    if (virDomainSnapshotCreateXMLEnsureACL(domain->conn, vm->def) < 0)
+    if (virDomainSnapshotCreateXMLEnsureACL(domain->conn, vm->def, flags) < 0)
         goto cleanup;
 
     if (!(caps = virQEMUDriverGetCapabilities(driver, false)))

diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
index f94a38a63a3db9c3d9bee814be6b2272d0be87d0..790a0204c1eb71deb7eee1a34060ea02c4e80005 100644 (file)
--- a/src/remote/remote_protocol.x
+++ b/src/remote/remote_protocol.x
@@ -3105,6 +3105,7 @@ enum remote_procedure {
     /**
      * @generate: both
      * @acl: domain:init_control
+     * @acl: domain:write:VIR_DOMAIN_REBOOT_GUEST_AGENT
      */
     REMOTE_PROC_DOMAIN_REBOOT = 27,
 
@@ -4198,6 +4199,7 @@ enum remote_procedure {
     /**
      * @generate: both
      * @acl: domain:snapshot
+     * @acl: domain:write:VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE
      */
     REMOTE_PROC_DOMAIN_SNAPSHOT_CREATE_XML = 185,
 
@@ -4290,12 +4292,14 @@ enum remote_procedure {
      * @acl: domain:write
      * @acl: domain:save:!VIR_DOMAIN_AFFECT_CONFIG|VIR_DOMAIN_AFFECT_LIVE
      * @acl: domain:save:VIR_DOMAIN_AFFECT_CONFIG
+     * @acl: domain:write:VIR_DOMAIN_VCPU_GUEST
      */
     REMOTE_PROC_DOMAIN_SET_VCPUS_FLAGS = 199,
 
     /**
      * @generate: both
      * @acl: domain:read
+     * @acl: domain:write:VIR_DOMAIN_VCPU_GUEST
      */
     REMOTE_PROC_DOMAIN_GET_VCPUS_FLAGS = 200,
 
@@ -4682,6 +4686,7 @@ enum remote_procedure {
     /**
      * @generate: both
      * @acl: domain:init_control
+     * @acl: domain:write:VIR_DOMAIN_SHUTDOWN_GUEST_AGENT
      */
     REMOTE_PROC_DOMAIN_SHUTDOWN_FLAGS = 258,
 

diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
index f286f41e6c5c2573a36921eacefbadae51b4d5aa..89afefef2e02f13e664ec2a856ab0bc802ed7434 100644 (file)
--- a/src/uml/uml_driver.c
+++ b/src/uml/uml_driver.c
@@ -1635,7 +1635,7 @@ static int umlDomainShutdownFlags(virDomainPtr dom,
         goto cleanup;
     }
 
-    if (virDomainShutdownFlagsEnsureACL(dom->conn, vm->def) < 0)
+    if (virDomainShutdownFlagsEnsureACL(dom->conn, vm->def, flags) < 0)
         goto cleanup;
 
 #if 0

diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c
index 65c3a5c1050e74f949188d49d6792decd69609c0..c45d980ef8f307c4b2db5a3574a9aa5d210a2f7e 100644 (file)
--- a/src/xen/xen_driver.c
+++ b/src/xen/xen_driver.c
@@ -952,7 +952,7 @@ xenUnifiedDomainShutdownFlags(virDomainPtr dom,
     if (!(def = xenGetDomainDefForDom(dom)))
         goto cleanup;
 
-    if (virDomainShutdownFlagsEnsureACL(dom->conn, def) < 0)
+    if (virDomainShutdownFlagsEnsureACL(dom->conn, def, flags) < 0)
         goto cleanup;
 
     ret = xenDaemonDomainShutdown(dom->conn, def);
@@ -979,7 +979,7 @@ xenUnifiedDomainReboot(virDomainPtr dom, unsigned int flags)
     if (!(def = xenGetDomainDefForDom(dom)))
         goto cleanup;
 
-    if (virDomainRebootEnsureACL(dom->conn, def) < 0)
+    if (virDomainRebootEnsureACL(dom->conn, def, flags) < 0)
         goto cleanup;
 
     ret = xenDaemonDomainReboot(dom->conn, def);
@@ -1526,7 +1526,7 @@ xenUnifiedDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags)
     if (!(def = xenGetDomainDefForDom(dom)))
         goto cleanup;
 
-    if (virDomainGetVcpusFlagsEnsureACL(dom->conn, def) < 0)
+    if (virDomainGetVcpusFlagsEnsureACL(dom->conn, def, flags) < 0)
         goto cleanup;
 
     ret = xenUnifiedDomainGetVcpusFlagsInternal(dom, def, flags);