[libvirt] [PATCH v2 1/5] domain: Fix unknown flags diagnosis in virDomainGetXMLDesc

[Eric Blake]

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 at 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 at 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