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> --- src/libvirt.c | 12 +++++++++--- src/libxl/libxl_driver.c | 6 +++--- src/lxc/lxc_driver.c | 4 ++-- src/qemu/qemu_driver.c | 8 ++++---- src/remote/remote_protocol.x | 5 +++++ src/uml/uml_driver.c | 2 +- src/xen/xen_driver.c | 6 +++--- 7 files changed, 27 insertions(+), 16 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index 6a41fd7..c15e29a 100644 --- 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 4115fff..fc0efa2 100644 --- 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 4c2744d..4c716ef 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2721,7 +2721,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)) { @@ -2798,7 +2798,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 7059f7a..8006882 100644 --- 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))) @@ -13070,7 +13070,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 5a82395..8238405 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -3185,6 +3185,7 @@ enum remote_procedure { /** * @generate: both * @acl: domain:init_control + * @acl: domain:write:VIR_DOMAIN_REBOOT_GUEST_AGENT */ REMOTE_PROC_DOMAIN_REBOOT = 27, @@ -4278,6 +4279,7 @@ enum remote_procedure { /** * @generate: both * @acl: domain:snapshot + * @acl: domain:write:VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE */ REMOTE_PROC_DOMAIN_SNAPSHOT_CREATE_XML = 185, @@ -4370,12 +4372,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, @@ -4762,6 +4766,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 f286f41..89afefe 100644 --- 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 65c3a5c..c45d980 100644 --- 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); -- 1.8.4.2