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,
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. We'll have to
be extra vigilant that any future added flags do not cause a security
hole when sent from a newer libvirt client that expects the flag
to do one thing, but where the older server silently ignores it
instead.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
src/conf/domain_conf.h | 3 +++
src/bhyve/bhyve_driver.c | 2 ++
src/conf/domain_conf.c | 2 ++
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_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 +-
14 files changed, 19 insertions(+), 11 deletions(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 2bc3f879f7..324fc247b6 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3110,6 +3110,9 @@ virDomainIOThreadIDDefPtr virDomainIOThreadIDAdd(virDomainDefPtr def,
unsigned int iothread_id);
void virDomainIOThreadIDDel(virDomainDefPtr def, unsigned int iothread_id);
+# 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/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 5d49f4388c..37bbf211c5 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -28996,6 +28996,8 @@ virDomainDefFormatInternal(virDomainDefPtr def,
return -1;
}
+/* Converts VIR_DOMAIN_XML_COMMON_FLAGS into VIR_DOMAIN_DEF_FORMAT_* flags,
+ * and silently ignores any other flags. */
unsigned int virDomainDefFormatConvertXMLFlags(unsigned int flags)
{
unsigned int formatFlags = 0;
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_driver.c b/src/qemu/qemu_driver.c
index 971f915619..b039675d1a 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7342,7 +7342,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;
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 2/14/19 4:29 PM, Eric Blake wrote:
> 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,
> 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. We'll have to
> be extra vigilant that any future added flags do not cause a security
> hole when sent from a newer libvirt client that expects the flag
> to do one thing, but where the older server silently ignores it
> instead.
Since this is limited to the GetXMLDesc processing wouldn't this limit
the exposure in such a way that some new flag expecting some default
action would not return expected or "new" results on the newer libvirt
vs. some older one? That is limited to VIR_DOMAIN_XML_COMMON_FLAGS
(SECURE, INACTIVE, MIGRATABLE). If say, CHECKPOINTABLE was added to that
list, the get wouldn't fail because it doesn't know the flag, but it
also wouldn't return any XML related to the new flag, right?
Secondary to that knowing this would require someone to read this
specific commit message to garnish at least a small understanding of the
issue. Perhaps some comments in domain_conf.h near the new
VIR_DOMAIN_XML_COMMON_FLAGS would help ensure someone doesn't expand
those without understanding the impact. Similarly, near the flags
definition either a similar noteworthy comment or a comment to go read
the domain_conf.h comment. There's a quick comment added before
virDomainDefFormatConvertXMLFlags in this patch that could be expandable
instead. Doesn't mean someone will read and/or understand it, but at
least leaves a trail.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> src/conf/domain_conf.h | 3 +++
> src/bhyve/bhyve_driver.c | 2 ++
> src/conf/domain_conf.c | 2 ++
> 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_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 +-
> 14 files changed, 19 insertions(+), 11 deletions(-)
>
There is one extra I'd question, qemuDomainDefFormatBufInternal that
perhaps could add the virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS |
VIR_DOMAIN_XML_UPDATE_CPU). Chasing current callers seems to eventually
ensure no more than those flags are used. That may mean an extra check
for a couple of paths, but it does ensure others don't need to check.
Here's my lookup/tree of callers and what could be passed originally.
* qemuDomainDefFormatBuf
* qemuMigrationCookieXMLFormat
* Uses INACTIVE | SECURE | MIGRATABLE
* qemuDomainDefFormatXMLInternal
* qemuDomainDefFormatXML
* qemuDomainDefCopy
* qemuDomainDefCheckABIStability
* qemuDomainCheckABIStability
* Each uses SECURE | MIGRATABLE (via COPY_FLAGS)
* qemuDomainSaveImageUpdateDef
* Uses SECURE (via QEMU_DOMAIN_FORMAT_LIVE_FLAGS) | MIGRATABLE
* qemuMigrationSrcRun
* Uses SECURE | MIGRATABLE
* qemuDomainSaveImageGetXMLDesc
* qemuDomainManagedSaveGetXMLDesc
* Each checks virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL);
* qemuDomainSaveImageDefineXML
* Uses INACTIVE | SECURE | MIGRATABLE
* qemuConnectDomainXMLFromNative
* Checks virCheckFlags(0, NULL);
* qemuMigrationDstPrepareAny
* Uses SECURE | MIGRATABLE
* qemuProcessStartHook
* qemuProcessStop
* qemuProcessAttach
* qemuProcessReconnect
* Each passes 0
* qemuDomainFormatXML
* qemuDomainCheckABIStability
* Same as above using COPY_FLAGS
* qemuDomainGetXMLDesc
* Newly repatched (and why I added UPDATE_CPU flag check for the
virCheckFlags in my qemuDomainDefFormatBufInternal query).
* qemuMigrationSrcPerformPeer2Peer2
* Uses SECURE (via QEMU_DOMAIN_FORMAT_LIVE_FLAGS) | MIGRATABLE
* qemuDomainDefFormatLive
* Will pass only SECURE (via QEMU_DOMAIN_FORMAT_LIVE_FLAGS) and
possibly INACTIVE or MIGRATABLE
BTW: For the two callers that pass all 3 maybe it'd be good to change
those to pass VIR_DOMAIN_XML_COMMON_FLAGS. Currently:
* qemuMigrationCookieXMLFormat
* qemuDomainSaveImageDefineXML
What's here so far is fine. The qemuDomainDefFormatBufInternal
processing is something that perhaps is "extra protection" considering
all the paths that could get there. I'll let you decide whether it needs
to be part of this, not part of this, or another patch.
Beyond that, I assume you can add the correct attribution/comments...
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
FWIW:
The following were my notes during review - which I think covers the
history that I found... Whether any of it is useful or not is up to you.
Commit 461e0f1a2 (0.9.4) is where you first added code to
virDomainDefFormat in order to check (VIR_DOMAIN_XML_SECURE |
VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_UPDATE_CPU).
Commit 20135c70 (0.9.4) introduces virDomainDefFormatInternal and
DUMPXML_FLAGS to mean the same thing as well as adding
VIR_DOMAIN_XML_INTERNAL_STATUS to the virCheckFlags so that
virDomainObjFormat will work properly. It adds the verify macro to
ensure no overlap.
Commit 07f413699 (0.9.4) adds VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET to the
list of flags that virDomainDefFormatInternal cares about. The verify
gets adjusted as well.
Commit d84b3626 (0.9.7) adds VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES to
the list of flags that virDomainDefFormatInternal cares about. The
verify gets adjusted as well.
Commit c01ba1a48 (0.9.10) adds VIR_DOMAIN_XML_INTERNAL_ALLOW_ROM and
VIR_DOMAIN_XML_INTERNAL_ALLOW_BOOT, but does not update the verify macro.
Commit 28f8dfdcc (1.0.0) adds VIR_DOMAIN_XML_MIGRATABLE to the
DUMPXML_FLAGS list (includes virDomainObjFormat caller too). There is no
need to change the verify.
Commit e31b5cf39 (1.1.0) adds VIR_DOMAIN_XML_INTERNAL_BASEDATE to the
list of flags that virDomainDefFormatInternal cares about. The verify is
updated; however, still missing the ALLOW_ROM and ALLOW_BOOT.
Commit b8efa6f2e (1.2.5 reverts the VIR_DOMAIN_XML_INTERNAL_BASEDATE
from the virDomainDefFormatInternal list of flags.
Commit b62d67da3 (1.2.5) adds VIR_DOMAIN_XML_INTERNAL_CLOCK_ADJUST to
the list of flags that virDomainDefFormatInternal cares about. The
verify macro gets adjusted, but still not with ALLOW_ROM or ALLOW_BOOT.
Commit 79f4c4e69 (1.2.8) moves where the the verify macro closer to
where the internal flags are defined and adds ALLOW_ROM and ALLOW_BOOT.
(a task that would require a couple of patches in today's world).
Commit 37588b25 (1.2.8) adds VIR_DOMAIN_XML_INTERNAL_DISK_SOURCE to the
list of flags, but doesn't change virDomainDefFormatInternal since all
it cares about is <disk> processing and adds that flag to the list of
flags processed in virDomainDiskDefSourceParse.
Commit 0ecd6851 (1.2.12) as you note comes along and alters things. It
too would probably require more steps in today's world ;-).
Commit 43a90eb7 (3.8.0) drops VIR_DOMAIN_DEF_FORMAT_UPDATE_CPU
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 2/19/19 1:31 PM, John Ferlan wrote: > > > On 2/14/19 4:29 PM, Eric Blake wrote: >> 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, >> 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. We'll have to >> be extra vigilant that any future added flags do not cause a security >> hole when sent from a newer libvirt client that expects the flag >> to do one thing, but where the older server silently ignores it >> instead. > > Since this is limited to the GetXMLDesc processing wouldn't this limit > the exposure in such a way that some new flag expecting some default > action would not return expected or "new" results on the newer libvirt > vs. some older one? That is limited to VIR_DOMAIN_XML_COMMON_FLAGS > (SECURE, INACTIVE, MIGRATABLE). If say, CHECKPOINTABLE was added to that > list, the get wouldn't fail because it doesn't know the flag, but it > also wouldn't return any XML related to the new flag, right? Indeed - and that's how I found it: I'm trying to add VIR_DOMAIN_XML_SNAPSHOTS and VIR_DOMAIN_XML_CHECKPOINTS flags, and was confused when I patched virsh to support the new flag but the old libvirt didn't react to it in any way (I then confirmed that libvirt running on RHEL 6 _does_ react to the unknown flag). Missing output is not a security breach, but is annoying enough to be worth comments somewhere as to the fact that the problem exists (worse would be if we wanted to add a flag comparable in nature to VIR_DOMAIN_XML_SECURE, where failure to obey the flag results in leaking XML information that should have been suppressed). > > Secondary to that knowing this would require someone to read this > specific commit message to garnish at least a small understanding of the > issue. Perhaps some comments in domain_conf.h near the new > VIR_DOMAIN_XML_COMMON_FLAGS would help ensure someone doesn't expand > those without understanding the impact. Similarly, near the flags > definition either a similar noteworthy comment or a comment to go read > the domain_conf.h comment. There's a quick comment added before > virDomainDefFormatConvertXMLFlags in this patch that could be expandable > instead. Doesn't mean someone will read and/or understand it, but at > least leaves a trail. Sure, I can add some strategic comments. > >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> src/conf/domain_conf.h | 3 +++ >> src/bhyve/bhyve_driver.c | 2 ++ >> src/conf/domain_conf.c | 2 ++ >> 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_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 +- >> 14 files changed, 19 insertions(+), 11 deletions(-) >> > > There is one extra I'd question, qemuDomainDefFormatBufInternal that > perhaps could add the virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS | > VIR_DOMAIN_XML_UPDATE_CPU). Chasing current callers seems to eventually > ensure no more than those flags are used. That may mean an extra check > for a couple of paths, but it does ensure others don't need to check. It would be a redundant check at the moment, but you are right that from the maintenance point of view it serves as self-documentation that might help the next person chasing things (that is, if all points that format XML have a local check, even if that local check is a superset of what was done in the caller, then you don't have to chase all callers). > > Here's my lookup/tree of callers and what could be passed originally. Thanks for that audit; it matches what I saw while working on the patch. > BTW: For the two callers that pass all 3 maybe it'd be good to change > those to pass VIR_DOMAIN_XML_COMMON_FLAGS. Currently: > > * qemuMigrationCookieXMLFormat > * qemuDomainSaveImageDefineXML Makes sense, although I might split it as a separate patch. > > What's here so far is fine. The qemuDomainDefFormatBufInternal > processing is something that perhaps is "extra protection" considering > all the paths that could get there. I'll let you decide whether it needs > to be part of this, not part of this, or another patch. > > Beyond that, I assume you can add the correct attribution/comments... > > Reviewed-by: John Ferlan <jferlan@redhat.com> > > John > > FWIW: > The following were my notes during review - which I think covers the > history that I found... Whether any of it is useful or not is up to you. I might add some of it into the final commit message. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 2/19/19 2:24 PM, Eric Blake wrote: >> Since this is limited to the GetXMLDesc processing wouldn't this limit >> the exposure in such a way that some new flag expecting some default >> action would not return expected or "new" results on the newer libvirt >> vs. some older one? That is limited to VIR_DOMAIN_XML_COMMON_FLAGS >> (SECURE, INACTIVE, MIGRATABLE). If say, CHECKPOINTABLE was added to that >> list, the get wouldn't fail because it doesn't know the flag, but it >> also wouldn't return any XML related to the new flag, right? > > Indeed - and that's how I found it: I'm trying to add > VIR_DOMAIN_XML_SNAPSHOTS and VIR_DOMAIN_XML_CHECKPOINTS flags, and was > confused when I patched virsh to support the new flag but the old > libvirt didn't react to it in any way (I then confirmed that libvirt > running on RHEL 6 _does_ react to the unknown flag). Missing output is > not a security breach, but is annoying enough to be worth comments > somewhere as to the fact that the problem exists (worse would be if we > wanted to add a flag comparable in nature to VIR_DOMAIN_XML_SECURE, > where failure to obey the flag results in leaking XML information that > should have been suppressed). > >> >> Secondary to that knowing this would require someone to read this >> specific commit message to garnish at least a small understanding of the >> issue. Perhaps some comments in domain_conf.h near the new >> VIR_DOMAIN_XML_COMMON_FLAGS would help ensure someone doesn't expand >> those without understanding the impact. Similarly, near the flags >> definition either a similar noteworthy comment or a comment to go read >> the domain_conf.h comment. There's a quick comment added before >> virDomainDefFormatConvertXMLFlags in this patch that could be expandable >> instead. Doesn't mean someone will read and/or understand it, but at >> least leaves a trail. > > Sure, I can add some strategic comments. Hmm - is there an easy way to add comments to the public include/ headers that are useful to developers, but which don't pollute the generated documentation? Or is this the sort of fix where a public doc comment that "some older versions of libvirt failed to diagnose unknown flags" is acceptable? Comments in domain_conf.[ch] are much easier as they don't affect the public docs. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 2/19/19 3:33 PM, Eric Blake wrote: > On 2/19/19 2:24 PM, Eric Blake wrote: > >>> Since this is limited to the GetXMLDesc processing wouldn't this limit >>> the exposure in such a way that some new flag expecting some default >>> action would not return expected or "new" results on the newer libvirt >>> vs. some older one? That is limited to VIR_DOMAIN_XML_COMMON_FLAGS >>> (SECURE, INACTIVE, MIGRATABLE). If say, CHECKPOINTABLE was added to that >>> list, the get wouldn't fail because it doesn't know the flag, but it >>> also wouldn't return any XML related to the new flag, right? >> >> Indeed - and that's how I found it: I'm trying to add >> VIR_DOMAIN_XML_SNAPSHOTS and VIR_DOMAIN_XML_CHECKPOINTS flags, and was >> confused when I patched virsh to support the new flag but the old >> libvirt didn't react to it in any way (I then confirmed that libvirt >> running on RHEL 6 _does_ react to the unknown flag). Missing output is >> not a security breach, but is annoying enough to be worth comments >> somewhere as to the fact that the problem exists (worse would be if we >> wanted to add a flag comparable in nature to VIR_DOMAIN_XML_SECURE, >> where failure to obey the flag results in leaking XML information that >> should have been suppressed). >> >>> >>> Secondary to that knowing this would require someone to read this >>> specific commit message to garnish at least a small understanding of the >>> issue. Perhaps some comments in domain_conf.h near the new >>> VIR_DOMAIN_XML_COMMON_FLAGS would help ensure someone doesn't expand >>> those without understanding the impact. Similarly, near the flags >>> definition either a similar noteworthy comment or a comment to go read >>> the domain_conf.h comment. There's a quick comment added before >>> virDomainDefFormatConvertXMLFlags in this patch that could be expandable >>> instead. Doesn't mean someone will read and/or understand it, but at >>> least leaves a trail. >> >> Sure, I can add some strategic comments. > > Hmm - is there an easy way to add comments to the public include/ > headers that are useful to developers, but which don't pollute the > generated documentation? Or is this the sort of fix where a public doc > comment that "some older versions of libvirt failed to diagnose unknown > flags" is acceptable? Comments in domain_conf.[ch] are much easier as > they don't affect the public docs. > I'm fine keeping away from the public doc comment about this. If (so far) no one has noticed, then why call attention to it. In the long run, I would think it's more important for a libvirt developer to not lose sight of the issue as opposed to a developer on libvirt. When/if the new flag is added whomever adds it would hopefully read the comments and if they don't we can always hope whomever reviews it may ask. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 2/19/19 2:24 PM, Eric Blake wrote: > >> BTW: For the two callers that pass all 3 maybe it'd be good to change >> those to pass VIR_DOMAIN_XML_COMMON_FLAGS. Currently: >> >> * qemuMigrationCookieXMLFormat >> * qemuDomainSaveImageDefineXML > > Makes sense, although I might split it as a separate patch. > Actually, no, it doesn't make sense. If VIR_DOMAIN_XML_COMMON_FLAGS gains a new flag, we don't want these callers to start outputting new information merely because they used the convenience macro. (That is, the macro is good for virCheckFlags(), to allow a driver to automatically support a new public flag by deferring to the common domain_conf implementation's support of the new flag; but it is not good for internal uses where we are generating specific output without regards to flags passed in through the public API). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 2/19/19 6:00 PM, Eric Blake wrote: > On 2/19/19 2:24 PM, Eric Blake wrote: > >> >>> BTW: For the two callers that pass all 3 maybe it'd be good to change >>> those to pass VIR_DOMAIN_XML_COMMON_FLAGS. Currently: >>> >>> * qemuMigrationCookieXMLFormat >>> * qemuDomainSaveImageDefineXML >> >> Makes sense, although I might split it as a separate patch. >> > > Actually, no, it doesn't make sense. If VIR_DOMAIN_XML_COMMON_FLAGS > gains a new flag, we don't want these callers to start outputting new > information merely because they used the convenience macro. (That is, > the macro is good for virCheckFlags(), to allow a driver to > automatically support a new public flag by deferring to the common > domain_conf implementation's support of the new flag; but it is not good > for internal uses where we are generating specific output without > regards to flags passed in through the public API). > oh right, good thought. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.