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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list