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@xxxxxxxxxx> >> --- >> 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@xxxxxxxxxx> > > 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
