[libvirt] [BUG,RFC] directory traversal vulnerability / qemu: name→uuid

[Daniel P. Berrange]

On Wed, Sep 07, 2011 at 03:02:51PM +0100, Eric Blake wrote:
> On 09/07/2011 11:12 AM, Philipp Hahn wrote:
> >Hello,
> >
> >I just tried the following command  with libvirt-0.9.5git:
> ># virsh snapshot-create "$VM" /dev/stdin
> ><<<'<domainsnapshot><name>../../../../../../etc/passwd</name></domainsnapshot>'
> >
> >"Luckily" it adds a .xml suffix, but this still looks like a security problem
> >to me, because you can overwrite any .xml-file with libvirt gibberish.
> >Actually this was found by a user trying to create a snapshot with an
> >embedded /, which didn't work, because the sub-directory didn't exist. I know
> >SELinux can solve this, but I really would prefer the Qemu driver to reject
> >such names.
> 
> Qemu won't reject names with /, but I agree with your thought that
> libvirt needs to prevent such names, particularly since it creates
> several other file names (such as log files, managed save,
> snapshots, and even the monitor file) all based on the domain name.
> 
> >
> >Another problem is, that I sometimes would like to rename a VM to a new name,
> >because the old name doesn't describe the VM good enough.<description>  is
> >not an option, because 1) Xen doesn't store it, and 2) virsh list doesn't
> >show it.
> 
> Adding a virDomainRename command would indeed be a nice API
> addition, but it certainly involves quite a bit of work.
> 
> >Renaming a Qemu-VM is currently impossible, since the name of the VM is used
> >for several files and directories and a undefine+define would loose state:
> >  /etc/libvirt/qemu/$VM.xml
> >  /var/lib/libvirt/qemu/$VM.monitor
> >  /var/lib/libvirt/qemu/save/$VM.save
> >  /var/lib/libvirt/qemu/snapshot/$VM/$SNAPSHOT.xml
> 
> All of these files would have to be edited as part of a
> virDomainRename.  You are also missing:
> 
> /var/log/libvirt/qemu/$VM.log
> 
> >Would it be possible and feasible to convert the Qemu driver to use the UUID
> >instead for file and directory naming?
> 
> Maybe, but I prefer seeing files by name rather than by UUID when
> browsing through the libvirt internal directories.  If we supported
> renaming, and properly altered the name of all affected files, then
> I see no reason to keep the files by name instead of uuid.

I really don't like the idea of using UUID for files we store on disk
because it makes it really unpleasant when debugging / troubleshooting.

Clearly we should forbid '/' in any guest name though. In addition
libvirt code should not be using the shell when running commands,
so we avoid the shell meta-charcter problem already.

Note, that this isn't a serious security flaw at this time, since access
to a privileged libvirtd daemon is already effectively equivalent to
having a root shell. Only once we get RBAC controls would this kind of
thing be able to be used for privilege escalation / DOS.


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