[libvirt] [Qemu-devel] live snapshot wiki updated

[Jes Sorensen]

On 07/21/11 16:02, Eric Blake wrote:
> On 07/21/2011 02:38 AM, Jes Sorensen wrote:
>> On 07/20/11 11:36, Daniel P. Berrange wrote:
>>> On Wed, Jul 20, 2011 at 10:23:12AM +0200, Jes Sorensen wrote:
>>>> Pardon, but I fail to see the issue here. If QEMU passes a filename
>>>> back
>>>> to libvirt, libvirt still gets to make the decision whether or not
>>>> it is
>>>> legitimate for QEMU to get that file descriptor or not. It doesn't
>>>> change anything wrt who actually opens the file, hence the 'trust' is
>>>> unchanged.
>>>
>>> To make the decision whether the filename from QEMU is valid, we have
>>> to parse the master image header data to see if the filename actually
>>> matches the backing file required by the image assigned to the guest.
>>
>> Sorry but that doesn't make any sense. In other words, if someone hacks
>> an image and makes it point to a different file, you are going to allow
>> the backing file to be opened just because it is listed in the image?
> 
> Yes, because the only way someone could hack that image is if they have
> rights to that file in the first place.  And if they have rights to that
> file in the first place, then they also can call qemu-img to modify that
> file, or any other number of modifications - but that's not an
> escalation in privilege, so it is not a security hole.

Ehm, we're talking about the case where a guest is able to compromise
the QEMU process controlling it. This is what libvirt / selinux is
trying to prevent. Now if the guest is able to break out of it's shell,
it can modify the disk image headers. Crash the QEMU process and wait
for the guest to be rebooted by the admin..... Voila we now have access
to random files on the NFS store we couldn't reach before.

So back to the drawing board, we do have a security problem here....

>> To the best of my understanding, the whole idea with selinux attributes
>> was to be able to specify which files are allowed to be opened by a
>> given process. Mapping this to the libvirt model, it should mean libvirt
>> ought to carry a positive list of  files that are allowed to be opened,
> 
> Which it does, by reading the metadata of those files prior to the point
> of ever handing those files over to an untrusted process - except in one
> case: right now, libvirt is not validating that a qcow2 file is still in
> a sane state after a qemu process ends.

You have previously explained that anything written to the QEMU header
is trusted by libvirt in the first place.

>> rather than relying on what might be written to an image file.
> 
> Thank you for persisting - you've found another hole that needs to be
> plugged.  It sounds like you are proposing that after a qemu process
> dies, that libvirt re-reads the qcow2 metadata headers, and validates
> that the backing file information has not changed in a manner unexpected
> by libvirt.  If it has, then the qemu process that just died was
> compromised to the point that restarting a new qemu process from the old
> image is now a security risk.  So this is _yet another_ security aspect
> that needs to be coded into libvirt as part of hardening sVirt.
> 
> But it is an independent issue of the need for fd passing.

No it is not independent, we're back to the point where we started. The
issue at hand is still that libvirt has no business messing about in
metadata headers of guest images. libvirt has the 'right' to validate
file names, but that is not justification in messing about in metadata.
Allowing that is an endless cat and mouse hunt of keeping up, which is
guaranteed to get out of sync.

If this problem is to be fixed, lets fix it correctly.

Jes