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

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.