From 27c8fd7490a852570314be6dbb02e647e3871802 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 14 Feb 2019 14:25:01 -0600 Subject: [PATCH] domain: Fix unknown flags diagnosis in virDomainGetXMLDesc Many drivers had a comment that they did not validate the incoming 'flags' to virDomainGetXMLDesc() because they were relying on virDomainDefFormat() to do it instead. This used to be the case (at least since 461e0f1a and friends in 0.9.4 added unknown flag checking in general), but regressed in commit 0ecd6851 (1.2.12), when all of the drivers were changed to pass 'flags' through the new helper virDomainDefFormatConvertXMLFlags(). Since this helper silently ignores unknown flags, we need to implement flag checking in each driver instead. Annoyingly, this means that any new flag values added will silently be ignored when targeting an older libvirt, rather than our usual practice of loudly diagnosing an unsupported flag. Add comments in domain_conf.[ch] to remind us to be extra vigilant about the impact when adding flags (a new flag to add data is safe if the older server omitting the requested data doesn't break things in the newer client; a new flag to suppress data rather than enhancing the existing VIR_DOMAIN_XML_SECURE may form a data leak or even a security hole). In the qemu driver, there are multiple callers all funnelling to qemuDomainDefFormatBufInternal(); many of them already validated flags (and often only a subset of the full set of possible flags), but for ease of maintenance, we can also check flags at the common helper function. Signed-off-by: Eric Blake Reviewed-by: John Ferlan --- src/bhyve/bhyve_driver.c | 2 ++ src/conf/domain_conf.c | 5 +++++ src/conf/domain_conf.h | 9 +++++++++ src/esx/esx_driver.c | 2 +- src/hyperv/hyperv_driver.c | 2 +- src/libxl/libxl_driver.c | 2 +- src/lxc/lxc_driver.c | 2 +- src/openvz/openvz_driver.c | 2 +- src/phyp/phyp_driver.c | 2 +- src/qemu/qemu_domain.c | 2 ++ src/qemu/qemu_driver.c | 3 ++- src/test/test_driver.c | 2 +- src/vbox/vbox_common.c | 2 +- src/vmware/vmware_driver.c | 2 +- src/vz/vz_driver.c | 2 +- 15 files changed, 30 insertions(+), 11 deletions(-) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 912797cfcf..3e192284cc 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -484,6 +484,8 @@ bhyveDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) virCapsPtr caps = NULL; char *ret = NULL; + virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS, NULL); + if (!(vm = bhyveDomObjFromDomain(domain))) goto cleanup; diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 51eea46b4e..56c437ca0a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -29083,6 +29083,11 @@ virDomainDefFormatInternal(virDomainDefPtr def, return -1; } +/* Converts VIR_DOMAIN_XML_COMMON_FLAGS into VIR_DOMAIN_DEF_FORMAT_* + * flags, and silently ignores any other flags. Note that the caller + * should validate the set of flags it is willing to accept; see also + * the comment on VIR_DOMAIN_XML_COMMON_FLAGS about security + * considerations with adding new flags. */ unsigned int virDomainDefFormatConvertXMLFlags(unsigned int flags) { unsigned int formatFlags = 0; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 2bc3f879f7..9bccd8bcd1 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3110,6 +3110,15 @@ virDomainIOThreadIDDefPtr virDomainIOThreadIDAdd(virDomainDefPtr def, unsigned int iothread_id); void virDomainIOThreadIDDel(virDomainDefPtr def, unsigned int iothread_id); +/* When extending this list, remember that libvirt 1.2.12-5.0.0 had a + * bug that silently ignored unknown flags. A new flag to add + * information is okay as long as clients still work when an older + * server omits the requested output, but a new flag to suppress + * information could result in a security hole when older libvirt + * supplies the sensitive information in spite of the flag. */ +# define VIR_DOMAIN_XML_COMMON_FLAGS \ + (VIR_DOMAIN_XML_SECURE | VIR_DOMAIN_XML_INACTIVE | \ + VIR_DOMAIN_XML_MIGRATABLE) unsigned int virDomainDefFormatConvertXMLFlags(unsigned int flags); char *virDomainDefFormat(virDomainDefPtr def, diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index b1af646155..379c2bae73 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -2604,7 +2604,7 @@ esxDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) virDomainDefPtr def = NULL; char *xml = NULL; - /* Flags checked by virDomainDefFormat */ + virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS, NULL); memset(&data, 0, sizeof(data)); diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index f41cd1c939..0e2c6c55ef 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -754,7 +754,7 @@ hypervDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) Msvm_ProcessorSettingData *processorSettingData = NULL; Msvm_MemorySettingData *memorySettingData = NULL; - /* Flags checked by virDomainDefFormat */ + virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS, NULL); if (!(def = virDomainDefNew())) goto cleanup; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 7981ccaf21..31b842aeee 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2621,7 +2621,7 @@ libxlDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) virDomainDefPtr def; char *ret = NULL; - /* Flags checked by virDomainDefFormat */ + virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS, NULL); if (!(vm = libxlDomObjFromDomain(dom))) goto cleanup; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index c48f6d9067..516a6b4de3 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -987,7 +987,7 @@ static char *lxcDomainGetXMLDesc(virDomainPtr dom, virDomainObjPtr vm; char *ret = NULL; - /* Flags checked by virDomainDefFormat */ + virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS, NULL); if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 06950ce9ed..39eeb2c12e 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -519,7 +519,7 @@ static char *openvzDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) { virDomainObjPtr vm; char *ret = NULL; - /* Flags checked by virDomainDefFormat */ + virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS, NULL); if (!(vm = openvzDomObjFromDomain(driver, dom->uuid))) return NULL; diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index dc082b1d08..e54799dbb4 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -3214,7 +3214,7 @@ phypDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) unsigned long long memory; unsigned int vcpus; - /* Flags checked by virDomainDefFormat */ + virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS, NULL); memset(&def, 0, sizeof(virDomainDef)); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index a0af56695e..bbd4a5efe8 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7725,6 +7725,8 @@ qemuDomainDefFormatBufInternal(virQEMUDriverPtr driver, virCapsPtr caps = NULL; virQEMUCapsPtr qemuCaps = NULL; + virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS | VIR_DOMAIN_XML_UPDATE_CPU, -1); + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) goto cleanup; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5118f4ad42..2c8bcde10b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7339,7 +7339,8 @@ static char virDomainObjPtr vm; char *ret = NULL; - /* Flags checked by virDomainDefFormat */ + virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS | VIR_DOMAIN_XML_UPDATE_CPU, + NULL); if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index c1faff46ff..cde9e3d417 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -2628,7 +2628,7 @@ static char *testDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) virDomainObjPtr privdom; char *ret = NULL; - /* Flags checked by virDomainDefFormat */ + virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS, NULL); if (!(privdom = testDomObjFromDomain(domain))) return NULL; diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 664650f217..d95fe7c7ae 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -4052,7 +4052,7 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) if (!data->vboxObj) return ret; - /* Flags checked by virDomainDefFormat */ + virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS, NULL); if (openSessionForMachine(data, dom->uuid, &iid, &machine) < 0) goto cleanup; diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index f94b3252fe..f4b0989afd 100644 --- a/src/vmware/vmware_driver.c +++ b/src/vmware/vmware_driver.c @@ -932,7 +932,7 @@ vmwareDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) virDomainObjPtr vm; char *ret = NULL; - /* Flags checked by virDomainDefFormat */ + virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS, NULL); if (!(vm = vmwareDomObjFromDomain(driver, dom->uuid))) return NULL; diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index b22a44d6ad..f99ade82b6 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -724,7 +724,7 @@ vzDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) virDomainObjPtr dom; char *ret = NULL; - /* Flags checked by virDomainDefFormat */ + virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS, NULL); if (!(dom = vzDomObjFromDomain(domain))) return NULL; -- GitLab