[libvirt] [PATCH 08/11] Don't allow renaming of domains by the backdoor

[Daniel P. Berrange]

On Fri, May 03, 2013 at 10:27:41AM -0600, Eric Blake wrote:
> On 05/02/2013 06:03 AM, Daniel P. Berrange wrote:
> > From: "Daniel P. Berrange" <berrange at redhat.com>
> > 
> > Several APIs allow for custom XML to be passed in. This is
> > checked for ABI stability, which will ensure the UUID is
> > not being changed. There isn't validation that the name
> > did not change though. This could allow renaming of guests
> > via the backdoor, which in turn could allow for bypassing
> > access control restrictions based on names.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
> > ---
> >  src/conf/domain_conf.c    | 11 +++++++++++
> >  src/qemu/qemu_migration.c |  6 ------
> >  2 files changed, 11 insertions(+), 6 deletions(-)
> 
> How does this interact with APIs like virDomainMigrate(), which
> intentionally has a dname parameter for renaming the domain on the
> destination?  Or is the idea that in virDomainMigrate2(), if you want to
> rename the domain, you have to do so through the dname argument and NOT
> via the backdoor of the dxml argument?

Yep, you must use the @dname parameter.

> 
> /me reads docs:
> 
> ... The migration will fail
>  * if @dxml would cause any guest-visible changes.  Pass NULL
>  * if no changes are needed to the XML between source and destination.
>  * @dxml cannot be used to rename the domain during migration (use
>  * @dname for that purpose).  Domain name in @dxml must match the
>  * original domain name.
> 
> Ah, so it looks like we indeed expect dname to be the only supported
> way, and that it is applied after dxml is first checked for ABI
> compatibility.
> 
> > 
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index a8b5dfd..d945b74 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -12560,6 +12560,17 @@ virDomainDefCheckABIStability(virDomainDefPtr src,
> >          return false;
> >      }
> >  
> > +    /* Not strictly ABI related, but we want to make sure domains
> > +     * don't get silently re-named through the backdoor when passing
> > +     * custom XML into various APIs, since this would create havoc
> > +     */
> > +    if (STRNEQ(src->name, dst->name)) {
> > +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +                       _("Target domain name '%s' does not match source '%s'"),
> > +                       dst->name, src->name);
> > +        return false;
> > +    }
> 
> The code makes sense, but I'd feel better delaying my ack until getting
> confirmation that I'm correctly interpreting that rename is
> intentionally denied on 'virsh save/restore', and rename during
> migration is allowed only through the dname argument, after dxml is
> already validated against the pre-rename xml.

Yes, rename is intentionally denied in save/restore.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|