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