[Libguestfs] [libvirt] qemu command line quoting (was: Re: [PATCH 0/5] rbd improvements)

[Daniel P. Berrange]

On Sun, May 12, 2013 at 09:15:33PM +0100, Richard W.M. Jones wrote:
> On Sun, May 12, 2013 at 02:42:36PM -0400, Mike Kelly wrote:
> > On Thu, May 9, 2013 at 12:21 PM, Richard W.M. Jones <rjones at redhat.com> wrote:
> > > On Thu, May 09, 2013 at 11:23:55AM -0400, Mike Kelly wrote:
> > >> On Wed, May 8, 2013 at 6:53 AM, Richard W.M. Jones <rjones at redhat.com> wrote:
> > >> > One worry I have is whether quoting is required for the server
> > >> > name(s), export name, username and secret.
> > >>
> > >> Well. I think the main things we had to quote were ':' and ';', but
> > >> none of those are valid in a hostname. Username also probably doesn't
> > >> contain anything special, and secret is a base64-encoded string. I
> > >> confirmed that even with the string ending in '==', it was parsed just
> > >> fine by qemu, at least in my limited manual testing.
> > >>
> > >> If you can suggest a way to be more robust this, though, then I can
> > >> try to work that into a future patch series.
> > >
> > > The quoting problem happens when someone writes a program which takes
> > > (eg) a hostname string from the user and passes it unmodified to the
> > > guestfs API.  It's an issue if this string can cause unexpected [even
> > > malicious/exploitable] things to happen when passed unquoted on the
> > > qemu command line.
> > 
> > Well, I'm not sure if this way of setting things up is still
> > encouraged, but at least this documentation suggests basically using
> > the fact that libvirt won't quote the image name as a "feature":
> > 
> >   http://ceph.com/w/index.php?title=QEMU-RBD#Caching
> > 
> >    <disk type='network' device='disk'>
> >       <source protocol='rbd'
> > name='poolname/imagename:rbd_cache=1:rbd_cache_size=67108864:rbd_cache_max_dirty=0'/>
> 
> Hmmm ...  This is a bug in libvirt, but also a missing feature of
> libvirt since it cannot express these other configuration fields.

Yes, that lack of escaping is a nasty bug. In fact it seems there is
no way to escape ':' in a pool/image name, so I've sent a patch to
just forbid it entirely in the XML, to avoid this kind of abuse.


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