On 07/19/11 16:24, Eric Blake wrote: > [adding the libvir-list] > On 07/19/2011 08:09 AM, Jes Sorensen wrote: >> Urgh, libvirt parsing image files is really unfortunate, it really >> doesn't give me warm fuzzy feelings :( libvirt really should not know >> about internals of image formats. > > But even if you add new features to qemu to avoid needing this in the > future, it doesn't change the past - libvirt will always have to know > how to parse image files understood by older qemu, and so as long as > libvirt already knows how to do that parsing, we might as well take > advantage of it. What has been done here in the past is plain wrong. Continuing to do it isn't the right thing to do here. > Besides, I feel that having a well-documented file format, so that > independent applications can both parse the same file with the same > semantics by obeying the file format specification, is a good design goal. We all know that documentation is rarely uptodate, new features may not get added and libvirt will never be able to keep up. The driver for a file format belongs in QEMU and nowhere else.

On Tue, Jul 19, 2011 at 04:14:27PM +0100, Stefan Hajnoczi wrote: > On Tue, Jul 19, 2011 at 3:30 PM, Jes Sorensen <Jes.Sorensen(a)redhat.com&gt; wrote: > > On 07/19/11 16:24, Eric Blake wrote: > >> [adding the libvir-list] > >> On 07/19/2011 08:09 AM, Jes Sorensen wrote: > >>> Urgh, libvirt parsing image files is really unfortunate, it really > >>> doesn't give me warm fuzzy feelings :( libvirt really should not know > >>> about internals of image formats. > >> > >> But even if you add new features to qemu to avoid needing this in the > >> future, it doesn't change the past - libvirt will always have to know > >> how to parse image files understood by older qemu, and so as long as > >> libvirt already knows how to do that parsing, we might as well take > >> advantage of it. > > > > What has been done here in the past is plain wrong. Continuing to do it > > isn't the right thing to do here. > > > >> Besides, I feel that having a well-documented file format, so that > >> independent applications can both parse the same file with the same > >> semantics by obeying the file format specification, is a good design goal. > > > > We all know that documentation is rarely uptodate, new features may not > > get added and libvirt will never be able to keep up. The driver for a > > file format belongs in QEMU and nowhere else. > > It should be a goal to avoid dependencies in multiple layers of the > stack because it becomes are to add new features - they require > coordinated changes in multiple layers. Having both QEMU and libvirt > know the internals of image files is such a multi-dependency. If I > want to add a new format or change an existing format I have to touch > both layers. > > For fd-passing perhaps we have an opportunity to use a callback > mechanism (QEMU request: filename -> libvirt response: fd) and do all > the image format parsing in QEMU. The reason why libvirt does the parsing of file headers to determine backing files is to maintain the trust boundary. Everything run from the exec() of QEMU onwards is considered untrusted code. So having QEMU parsing the file headers & passing back open() requests to libvirt is breaking the trust boundary.

On 07/19/11 18:46, Daniel P. Berrange wrote: > On Tue, Jul 19, 2011 at 04:14:27PM +0100, Stefan Hajnoczi wrote: >> For fd-passing perhaps we have an opportunity to use a callback >> mechanism (QEMU request: filename -> libvirt response: fd) and do all >> the image format parsing in QEMU. > > The reason why libvirt does the parsing of file headers to determine > backing files is to maintain the trust boundary. Everything run from > the exec() of QEMU onwards is considered untrusted code. So having > QEMU parsing the file headers & passing back open() requests to libvirt > is breaking the trust boundary. 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.

> NB, i'm not happy about libvirt having to have knowledge of file format > headers, but we needed something more efficient & reliable than invoking > qemu-img info & parsing the output. Ideally QEMU (or something else) > would provide a library libblockformat.so with stable APIs for at least > reading metadata about image formats. If it had APIs for image creation, > etc too that would be a bonus, but we're more or less ok spawning qemu-img > for those cases currently. Even having a library for libvirt to link against is suboptimal here. Two processes shouldn't be fighting over the internals of metadata, the ownership of the metadata belongs solely with QEMU. In addition you have the constant issue of dependencies there, hence if QEMU is updated and it provides a newer block format library, it may prevent libvirt from running forcing an update of libvirt as well. That is not acceptable for development.

The 20/07/11, Daniel P. Berrange wrote: > 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. Actually, libvirt should not have to worry if the filename provided by QEMU is valid. I think it should trust QEMU. If QEMU doesn't provide information others can trust; it should be fixed at QEMU side.

> We're not fighting over the internals of metadata. We just need to know > ahead of launching QEMU, what backing files an image has & what format > they are in. We do that by reading at the metadata headers of the disk > images. We never attempt to write to the disk images. Either someone > provides a library todo that, or we write the probing code for each > file format in libvirt. Currently we have the latter. This is what I would call "fighting with QEMU internals". How do you prevent from concurrency access and modifications? Ideally speacking libvirt should be able to co-exist with foreign implementations, all requesting QEMU.

On Wed, Jul 20, 2011 at 12:15:02PM +0200, Nicolas Sebrecht wrote: > Actually, libvirt should not have to worry if the filename provided by > QEMU is valid. I think it should trust QEMU. If QEMU doesn't provide > information others can trust; it should be fixed at QEMU side. The security goal of libvirt is to protect the host from a compromised QEMU, therefore QEMU is, by definition, untrusted.

>> We're not fighting over the internals of metadata. We just need to know >> ahead of launching QEMU, what backing files an image has & what format >> they are in. We do that by reading at the metadata headers of the disk >> images. We never attempt to write to the disk images. Either someone >> provides a library todo that, or we write the probing code for each >> file format in libvirt. Currently we have the latter. > > This is what I would call "fighting with QEMU internals". How do you > prevent from concurrency access and modifications? Ideally speacking > libvirt should be able to co-exist with foreign implementations, all > requesting QEMU. QEMU is *not* yet running at the time libvirt reads the file metadata.

>> > > QEMU is *not* yet running at the time libvirt reads the file metadata. > > > > Of course it is: hotplug In the case of hotplug, the hotplug monitor commands have not yet been invoked when libvirt access the file metadata, or the drive has already been unplugged, so there is still no concurrent access to the file by QEMU and libvirt.

> The security goal of libvirt is to protect the host from a compromised > QEMU, therefore QEMU is, by definition, untrusted. Well that part goes both ways. By applying this model you are going to make it a hard requirement for libvirt to be upgraded with QEMU even for smaller updates.

There is a difference between upgrading to pick up additional features and forced upgrade. It is not just a question of libvirt possibly reviewing a file format, it is also having two codebases that needs to be implemented and maintained.

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?

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,

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.

On Thu, Jul 21, 2011 at 6:01 PM, Stefan Hajnoczi <stefanha(a)gmail.com&gt; wrote: > On Thu, Jul 21, 2011 at 3:02 PM, Eric Blake <eblake(a)redhat.com&gt; wrote: >> 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. > > The backing file information changes when image streaming completes. > > Before: fedora.img <- my_vm.qed > After: my_vm.qed (fedora.img is no longer referenced) > > The image streaming operation copies data out of fedora.img and > populates my_vm.qed.  When image streaming completes, the backing file > is no longer needed and my_vm.qed is updated to drop the backing file. > > I think we need to design carefully to prevent QEMU and libvirt making > incorrect assumptions about who does what.  I really wish that all > this image file business was outside QEMU and libvirt - that we had a > separate storage management service which handled the details.  QEMU > would only do block device operations (no image format manipulation), > and libvirt would only delegate to the storage management service. > Today we seem to be sprinkling a little bit of storage management into > QEMU and a little bit into libvirt :(. > > In that spirit it is much nicer to think of storage like a SAN > appliance where you have LUNs that you access as block devices.  It > also provides an API for snapshotting, cloning LUNs, etc. > > Let's move to that model instead of worrying about how to spread > storage logic across QEMU and libvirt. Would NBD protocol fit to this purpose, or is it too simple? Then libvirt would handle the storage format completely and present an NBD interface to QEMU (or give an fd to an external service) and QEMU would not care about the storage format in this mode at all.

On Thu, Jul 21, 2011 at 3:02 PM, Eric Blake <eblake(a)redhat.com&gt; wrote: > 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. The backing file information changes when image streaming completes. Before: fedora.img <- my_vm.qed After: my_vm.qed (fedora.img is no longer referenced) The image streaming operation copies data out of fedora.img and populates my_vm.qed. When image streaming completes, the backing file is no longer needed and my_vm.qed is updated to drop the backing file. I think we need to design carefully to prevent QEMU and libvirt making incorrect assumptions about who does what. I really wish that all this image file business was outside QEMU and libvirt - that we had a separate storage management service which handled the details. QEMU would only do block device operations (no image format manipulation), and libvirt would only delegate to the storage management service.

On Fri, Jul 22, 2011 at 8:22 AM, Kevin Wolf <kwolf(a)redhat.com&gt; wrote: > Am 21.07.2011 17:01, schrieb Stefan Hajnoczi: >> On Thu, Jul 21, 2011 at 3:02 PM, Eric Blake <eblake(a)redhat.com&gt; wrote: >>> 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. >> >> The backing file information changes when image streaming completes. >> >> Before: fedora.img <- my_vm.qed >> After: my_vm.qed (fedora.img is no longer referenced) >> >> The image streaming operation copies data out of fedora.img and >> populates my_vm.qed.  When image streaming completes, the backing file >> is no longer needed and my_vm.qed is updated to drop the backing file. >> >> I think we need to design carefully to prevent QEMU and libvirt making >> incorrect assumptions about who does what.  I really wish that all >> this image file business was outside QEMU and libvirt - that we had a >> separate storage management service which handled the details.  QEMU >> would only do block device operations (no image format manipulation), >> and libvirt would only delegate to the storage management service. > > And how do you implement that in a way that works on all platforms, and > without root privileges? I can't see this happen unless it stays > completely optional. The cross-platform way would be an iSCSI target that understands image formats.  But iSCSI requires copying when doing I/O and we can't pass through virtio-blk.

Am 19.07.2011 18:46, schrieb Daniel P. Berrange: > On Tue, Jul 19, 2011 at 04:14:27PM +0100, Stefan Hajnoczi wrote: >> On Tue, Jul 19, 2011 at 3:30 PM, Jes Sorensen <Jes.Sorensen(a)redhat.com&gt; wrote: >>> On 07/19/11 16:24, Eric Blake wrote: >>>> [adding the libvir-list] >>>> On 07/19/2011 08:09 AM, Jes Sorensen wrote: >>>>> Urgh, libvirt parsing image files is really unfortunate, it really >>>>> doesn't give me warm fuzzy feelings :( libvirt really should not know >>>>> about internals of image formats. >>>> >>>> But even if you add new features to qemu to avoid needing this in the >>>> future, it doesn't change the past - libvirt will always have to know >>>> how to parse image files understood by older qemu, and so as long as >>>> libvirt already knows how to do that parsing, we might as well take >>>> advantage of it. >>> >>> What has been done here in the past is plain wrong. Continuing to do it >>> isn't the right thing to do here. >>> >>>> Besides, I feel that having a well-documented file format, so that >>>> independent applications can both parse the same file with the same >>>> semantics by obeying the file format specification, is a good design goal. >>> >>> We all know that documentation is rarely uptodate, new features may not >>> get added and libvirt will never be able to keep up. The driver for a >>> file format belongs in QEMU and nowhere else. >> >> It should be a goal to avoid dependencies in multiple layers of the >> stack because it becomes are to add new features - they require >> coordinated changes in multiple layers. Having both QEMU and libvirt >> know the internals of image files is such a multi-dependency. If I >> want to add a new format or change an existing format I have to touch >> both layers. >> >> For fd-passing perhaps we have an opportunity to use a callback >> mechanism (QEMU request: filename -> libvirt response: fd) and do all >> the image format parsing in QEMU. > > The reason why libvirt does the parsing of file headers to determine > backing files is to maintain the trust boundary. Everything run from > the exec() of QEMU onwards is considered untrusted code. So having > QEMU parsing the file headers & passing back open() requests to libvirt > is breaking the trust boundary. > > NB, i'm not happy about libvirt having to have knowledge of file format > headers, but we needed something more efficient & reliable than invoking > qemu-img info & parsing the output. What's the real problem with this approach? Parsing the data meant for humans, from an interface that is potentially unstable? If this is it, it should be easy enough to add a JSON output mode to qemu-img info.

> Ideally QEMU (or something else) > would provide a library libblockformat.so with stable APIs for at least > reading metadata about image formats. If it had APIs for image creation, > etc too that would be a bonus, but we're more or less ok spawning qemu-img > for those cases currently. I'm afraid the block drivers have too many dependencies on the qemu core for this to be an option without investing a lot of effort.

>> As nice as that sentiment is, it will never fly, because it would be a >> regression in current behavior. The whole reason that the virt_use_nfs >> SELinux bool exists is that some people are willing to make the partial >> security tradeoff. Besides, the use of sVirt via SELinux is more than >> just open() protection - while the current virt_use_nfs bool makes NFS >> less secure than otherwise possible, it still gives some nice guarantees >> to the rest of the qemu process such as passthrough accesses to local >> pci devices. > > Well leaving things at status quo is not making it worse, it just leaves > an evil in place. NFS and SELinux is a fundamental problem with SELinux and NFS. We can piss and moan as much as we want about it but it's reality. SELinux fundamentally requires extended attributes. By the time NFS adds extended attribute support, we'll all be flying around in hover cars. As terrible as NFS is, people use it all of the time. It would be nice if libvirt had the ability to make better use of DAC to support isolation. The fact that MAC is the only way you can do isolation between guests is pretty unfortunate. If I could assign specific UIDs to a guest and use that to enforce isolation, it would go a long ways to solving this problem.

> > Right, we're stuck with the two horros of NFS and selinux, so we need > > something that gets around the problem. In a sane world we would simply > > say 'no NFS, no selinux', but as you say that will never happen. > > > > My suggestion of a callback mechanism where libvirt registers the > > callback with QEMU for open() calls, allowing libvirt to perform the > > open and return the open file descriptor would get around this problem. To me this sounds more like a problem than a solution. It basically means that during an open (which may even be initiated by a monitor command), you need monitor interaction. It basically means that open becomes asynchronous, and requires clients to deal with that, which sounds at least "interesting"... Also you have to add some magic to all places opening something. I think if libvirt wants qemu to use an fd instead of a file name, it shouldn't pass a file name but an fd in the first place. Which means that the two that we need are support for an fd: protocol (patches on the list, need review), and a way for libvirt to override the backing file of an image.

On 07/20/2011 07:25 AM, Jes Sorensen wrote: >> >> I think if libvirt wants qemu to use an fd instead of a file name, it >> shouldn't pass a file name but an fd in the first place. Which means >> that the two that we need are support for an fd: protocol (patches on >> the list, need review), and a way for libvirt to override the backing >> file of an image. > > The problem is that QEMU will find backing file file names inside the > images which it will be unable to open. How do you suggest we get around > that? We've already told you - qemu must have a way to be passed fds which are associated with names, and when a file refers to another backing file by name, then qemu falls back on its fd/name mapping to use the already-passed fd instead.  Which implies that someone else, either libvirt or a qemu-maintained libblockformat.so, needs to have a stable interface for parsing the backing file name out of an arbitrary qcow2 file, and that this interface must work no matter how many other extensions are added to qcow2.

On 07/20/2011 11:27 AM, Blue Swirl wrote: >> >> We've already told you - qemu must have a way to be passed fds which are >> associated with names, and when a file refers to another backing file by >> name, then qemu falls back on its fd/name mapping to use the >> already-passed >> fd instead.  Which implies that someone else, either libvirt or a >> qemu-maintained libblockformat.so, needs to have a stable interface for >> parsing the backing file name out of an arbitrary qcow2 file, and that >> this >> interface must work no matter how many other extensions are added to >> qcow2. > > I'd avoid any name based access in this case. If QEMU has write access > to main file, it could forge the backing file name in main file to > point to for example /etc/shadow and then request libvirt to perform > the opening. Won't work.  Well, it might work within the context of a single qemu process.  But when that process ends, then libvirt would have to touch up the qcow2 headers of that file to replace the /etc/shadow name with the real backing file name, otherwise, the next time you restart qemu-img or a new qemu guest using the same image, the information has been lost, since the fd has been closed in the meantime.

We really _do_ need a way to give qemu both an fd and the name of the file that the fd is tied to.  On Linux, qemu could use /proc/self/fd to reconstruct the name from fd, but that's not portable to other OS.  And we've already discussed how in the libvirt model, that libvirt is deemed more secure than qemu.  Therefore, I think it is reasonable for qemu to make the assumptions that if it exposes a monitor command where the supervisor (libvirt or otherwise) can pass in both an fd and a file name, that either the supervisor is passing in correct information, or that the bug is in the supervisor and not in qemu if the supervisor passes in wrong information and things blow up.

And the snapshot_blkdev monitor command is a case where qemu needs to create a new qcow2 image on the fly, while referencing the name of an existing file.  What backing name do you put in the new qcow2 file unless you already have a name association for all fds already open for the existing backing file?

On 07/20/11 21:51, Blue Swirl wrote: >> And the snapshot_blkdev monitor command is a case where qemu needs to create >>> a new qcow2 image on the fly, while referencing the name of an existing >>> file. What backing name do you put in the new qcow2 file unless you already >>> have a name association for all fds already open for the existing backing >>> file? > For backing file with original name of "/path/in/storage", QEMU could > present the fd and the backin path by requesting something like > "fd:12,/path/in/storage". The next file in chain "/path2/file" would > be "fd:12,fd:34,/path2/file". Or if possible, -fd 12 -backing > /path/in/storage with spaces and funny characters escaped etc. Rather than trying to do this by mangling files on the disk, I would suggest we allow registering a call-back open function, which calls back into libvirt and requests it to open a given file. It can then do all it's security foo to decide whether or not to allow the file to be open. This is relatively clean and avoids the mess of relying on outside processes messing about in the images.

On 07/20/11 21:51, Blue Swirl wrote: >> And the snapshot_blkdev monitor command is a case where qemu needs to create >> > a new qcow2 image on the fly, while referencing the name of an existing >> > file.  What backing name do you put in the new qcow2 file unless you already >> > have a name association for all fds already open for the existing backing >> > file? > For backing file with original name of "/path/in/storage", QEMU could > present the fd and the backin path by requesting something like > "fd:12,/path/in/storage". The next file in chain "/path2/file" would > be "fd:12,fd:34,/path2/file". Or if possible, -fd 12 -backing > /path/in/storage with spaces and funny characters escaped etc. Rather than trying to do this by mangling files on the disk, I would suggest we allow registering a call-back open function, which calls back into libvirt and requests it to open a given file. It can then do all it's security foo to decide whether or not to allow the file to be open.

This is relatively clean and avoids the mess of relying on outside processes messing about in the images. Cheers, Jes

We really _do_ need a way to give qemu both an fd and the name of the file that the fd is tied to. On Linux, qemu could use /proc/self/fd to reconstruct the name from fd, but that's not portable to other OS.

And we've already discussed how in the libvirt model, that libvirt is deemed more secure than qemu. Therefore, I think it is reasonable for qemu to make the assumptions that if it exposes a monitor command where the supervisor (libvirt or otherwise) can pass in both an fd and a file name, that either the supervisor is passing in correct information, or that the bug is in the supervisor and not in qemu if the supervisor passes in wrong information and things blow up. And the snapshot_blkdev monitor command is a case where qemu needs to create a new qcow2 image on the fly, while referencing the name of an existing file. What backing name do you put in the new qcow2 file unless you already have a name association for all fds already open for the existing backing file?

On 07/20/2011 07:25 AM, Jes Sorensen wrote: >> I think if libvirt wants qemu to use an fd instead of a file name, it >> shouldn't pass a file name but an fd in the first place. Which means >> that the two that we need are support for an fd: protocol (patches on >> the list, need review), and a way for libvirt to override the backing >> file of an image. > > The problem is that QEMU will find backing file file names inside the > images which it will be unable to open. How do you suggest we get around > that? We've already told you - qemu must have a way to be passed fds which are associated with names, and when a file refers to another backing file by name, then qemu falls back on its fd/name mapping to use the already-passed fd instead. Which implies that someone else, either libvirt or a qemu-maintained libblockformat.so, needs to have a stable interface for parsing the backing file name out of an arbitrary qcow2 file, and that this interface must work no matter how many other extensions are added to qcow2.

Am 20.07.2011 15:25, schrieb Jes Sorensen: > On 07/20/11 12:01, Kevin Wolf wrote: >>>> Right, we're stuck with the two horros of NFS and selinux, so we need >>>> something that gets around the problem. In a sane world we would simply >>>> say 'no NFS, no selinux', but as you say that will never happen. >>>> >>>> My suggestion of a callback mechanism where libvirt registers the >>>> callback with QEMU for open() calls, allowing libvirt to perform the >>>> open and return the open file descriptor would get around this problem. >> To me this sounds more like a problem than a solution. It basically >> means that during an open (which may even be initiated by a monitor >> command), you need monitor interaction. It basically means that open >> becomes asynchronous, and requires clients to deal with that, which >> sounds at least "interesting"... Also you have to add some magic to all >> places opening something. >> >> I think if libvirt wants qemu to use an fd instead of a file name, it >> shouldn't pass a file name but an fd in the first place. Which means >> that the two that we need are support for an fd: protocol (patches on >> the list, need review), and a way for libvirt to override the backing >> file of an image. > > The problem is that QEMU will find backing file file names inside the > images which it will be unable to open. How do you suggest we get around > that? This is the part with allowing libvirt to override the backing file. Of course, this is not something that we can add with five lines of code, it requires -blockdev.

On 07/20/2011 11:20 AM, Blue Swirl wrote: > > There could still be some issues: > Let's have files A, B, C etc. with backing files AA etc. How would > libvirt know that when QEMU wants to write to file CA, this is because > it's needed to access C, or is it just trickery by a devious guest to > corrupt storage? The fix for CVE-2010-2238 already deals with this: if primary image C refers to backing file CA of raw format, but does not state what file format CA contains, then a malicious guest can modify the contents of CA to appear to be yet another qcow2 image.  At which point, if libvirt follows the backing file specified in CA, then yes, the malicious guest really can cause libvirt to expose arbitrary file CB for manipulation by the guest.  But that security hole was already plugged - by default, libvirt refuses to probe backing files parsed from qcow2 headers for file format, but instead requires the outer qcow2 header to also include the a file format designation for the backing file.  At which point, you then have a safe chain: if C refers to CA, then libvirt knows that both C and CA are essential to the storage presented by giving qemu the file name C, and the guest will already be modifying CA, but there is no storage corruption involved.

That is, as long as libvirt can already accurately read the chain of backing files from any starting point, then it can hand that entire chain of backing files (whether by the topmost file name as it does now, or whether by a series of fds as is being proposed) to qemu. > > This could be handled so that instead of naming the backing file, QEMU > asks for a descriptor for the backing file by presenting the > descriptor to main file C, but I think the real solution is that > libvirt should handle the storage formats completely and it should > present QEMU with only a raw file like interface (read/write/seek) for > the data. Then any backing files would be handled within libvirt. > Performance could suffer, though. The monitor interface was not designed to throw the read()/write()/seek() burden back on libvirt, and indeed that would kill performance so it is a non-starter idea.  All we need for security is the open() burden to be shifted out of qemu and into libvirt.

On 07/20/2011 12:00 PM, Blue Swirl wrote: >>> >>> Let's have files A, B, C etc. with backing files AA etc. How would >>> libvirt know that when QEMU wants to write to file CA, this is because >>> it's needed to access C, or is it just trickery by a devious guest to >>> corrupt storage? >> >> The fix for CVE-2010-2238 already deals with this: if primary image C >> refers >> to backing file CA of raw format, but does not state what file format CA >> contains, then a malicious guest can modify the contents of CA to appear >> to >> be yet another qcow2 image.  At which point, if libvirt follows the >> backing >> file specified in CA, then yes, the malicious guest really can cause >> libvirt >> to expose arbitrary file CB for manipulation by the guest.  But that >> security hole was already plugged - by default, libvirt refuses to probe >> backing files parsed from qcow2 headers for file format, but instead >> requires the outer qcow2 header to also include the a file format >> designation for the backing file.  At which point, you then have a safe >> chain: if C refers to CA, then libvirt knows that both C and CA are >> essential to the storage presented by giving qemu the file name C, and >> the >> guest will already be modifying CA, but there is no storage corruption >> involved. > > But what if CA is accessed even if C is not? For example, QEMU opens C > (to determine CA and write new information about the path), closes it > and then requests CA? Why is qemu trying to access CA? Either because CA was mentioned as a backing file for C (in which case libvirt already knows about it, because either libvirt handed C to qemu at startup time after already parsing C's headers to learn that CA is a backing file, or because libvirt called the snapshot_blkdev command with the intent of having qemu populate CA with C as its backing file), or because qemu has a bug (in which case, libvirt should refuse the access to CA).

Libvirt is already perfectly capable of tracking all files that qemu might need to access, and whether it is qemu or libvirt that does the open() of those files, we can still have libvirt validate whether each request for a file makes sense given the context of all previous files in use from the time the qemu command line was invoked and across all monitor commands in the meantime. On non-NFS solutions, where every file can have a SELinux label, then the security is then present by merely having libvirt relabel all such files to a unique label for that particular qemu process, and SELinux merely enforces that qemu cannot open() anything but what libvirt has already labeled.  And since libvirt already knows which files to label in the non-NFS scenario, it already knows which fds to pass in the NFS scenario, at which point the ability to prevent qemu from open()ing an NFS file is a security enhancement. Your question about qemu wanting to use CA is thus answered independently of whether the fd management solution is solved by libvirt handing an fd for CA to qemu prior to any monitor command where qemu will then need to use CA, or whether qemu is taught to asynchronously ask libvirt to open an fd for CA on qemu's behalf.  The answer is that libvirt already tracks whether qemu should access CA, and just needs a way to enforce that knowledge.  The enforcement already exists for non-NFS via SELinux labels, and the proposal to add fd handling will expand that enforcement to also cover NFS.

On Wed, Jul 20, 2011 at 4:51 PM, Kevin Wolf <kwolf(a)redhat.com&gt; wrote: > Am 20.07.2011 15:25, schrieb Jes Sorensen: >> On 07/20/11 12:01, Kevin Wolf wrote: >>>>> Right, we're stuck with the two horros of NFS and selinux, so we need >>>>> something that gets around the problem. In a sane world we would simply >>>>> say 'no NFS, no selinux', but as you say that will never happen. >>>>> >>>>> My suggestion of a callback mechanism where libvirt registers the >>>>> callback with QEMU for open() calls, allowing libvirt to perform the >>>>> open and return the open file descriptor would get around this problem. >>> To me this sounds more like a problem than a solution. It basically >>> means that during an open (which may even be initiated by a monitor >>> command), you need monitor interaction. It basically means that open >>> becomes asynchronous, and requires clients to deal with that, which >>> sounds at least "interesting"... Also you have to add some magic to all >>> places opening something. >>> >>> I think if libvirt wants qemu to use an fd instead of a file name, it >>> shouldn't pass a file name but an fd in the first place. Which means >>> that the two that we need are support for an fd: protocol (patches on >>> the list, need review), and a way for libvirt to override the backing >>> file of an image. >> >> The problem is that QEMU will find backing file file names inside the >> images which it will be unable to open. How do you suggest we get around >> that? > > This is the part with allowing libvirt to override the backing file. Of > course, this is not something that we can add with five lines of code, > it requires -blockdev. There could still be some issues: Let's have files A, B, C etc. with backing files AA etc. How would libvirt know that when QEMU wants to write to file CA, this is because it's needed to access C, or is it just trickery by a devious guest to corrupt storage? This could be handled so that instead of naming the backing file, QEMU asks for a descriptor for the backing file by presenting the descriptor to main file C,

but I think the real solution is that libvirt should handle the storage formats completely and it should present QEMU with only a raw file like interface (read/write/seek) for the data. Then any backing files would be handled within libvirt. Performance could suffer, though.

> > The problem is that QEMU will find backing file file names inside the > images which it will be unable to open. How do you suggest we get around > that? This is the part with allowing libvirt to override the backing file. Of course, this is not something that we can add with five lines of code, it requires -blockdev.

Am 22.07.2011 09:36, schrieb Avi Kivity: > On 07/20/2011 04:51 PM, Kevin Wolf wrote: >>> >>>  The problem is that QEMU will find backing file file names inside the >>>  images which it will be unable to open. How do you suggest we get around >>>  that? >> >> This is the part with allowing libvirt to override the backing file. Of >> course, this is not something that we can add with five lines of code, >> it requires -blockdev. > > It can be done without blockdev.  Have a dictionary that translates > filenames, and populate it from the command line (for a bonus, translate > a filename to a file descriptor inherited from the caller or passed via > the monitor). Sure, you can always add ugly hacks, but it isn't the right solution that we want to use for all times. However, once we use it, it will show up in the external API and we'll never get rid of it again.

Jes Sorensen <Jes.Sorensen(a)redhat.com&gt; writes: > Right, we're stuck with the two horros of NFS and selinux, so we need > something that gets around the problem. In a sane world we would simply > say 'no NFS, no selinux', but as you say that will never happen. > > My suggestion of a callback mechanism where libvirt registers the > callback with QEMU for open() calls, allowing libvirt to perform the > open and return the open file descriptor would get around this problem. No go, I'm afraid. The problem at hand is how to confine the untrusted QEMU process so it can access exactly the resources the guest configuration specifies, no more, no less. When guest configuration says "use image A", it really means "file A plus certain other resources that are required to use it". For obvious usability reasons, we don't require spelling out "certain other" if we can safely get the information from A. Example: file foo.qcow2 requires its backing file file foo.img. Obviously, the code to get information from A must be trusted. Therefore, it can't run in the untrusted QEMU process. Example: the proposed callback mechanism. Guest configuration says "use image foo.qcow2". Libvirt (or whatever management layer) arranges that the QEMU process can use file foo.qcow2. The QEMU process then uses the callback to gain access to the backing file. Nothing stops it to ask for whatever file it wants. How can libvirt decide whether the file is one of the resources the guest configuration specifies? The only way I can see around having libvirt read resource information from images (preferably via a suitable library provided by QEMU) is to require the administrator to spell out the resouce information. Administrators generally don't fancy spelling out things the computer already knows.

On Tue, Jul 19, 2011 at 04:30:19PM +0200, Jes Sorensen wrote: > On 07/19/11 16:24, Eric Blake wrote: >> Besides, I feel that having a well-documented file format, so that >> independent applications can both parse the same file with the same >> semantics by obeying the file format specification, is a good design goal. > > We all know that documentation is rarely uptodate, new features may not > get added and libvirt will never be able to keep up. The driver for a > file format belongs in QEMU and nowhere else. This would be possible if QEMU to provide a libblockformat.so library which allowed apps to extract metadata from file formats using a stable API.

On Wed, Jul 20, 2011 at 10:26:49AM +0200, Jes Sorensen wrote: > On 07/19/11 18:47, Daniel P. Berrange wrote: >> On Tue, Jul 19, 2011 at 04:30:19PM +0200, Jes Sorensen wrote: >>> On 07/19/11 16:24, Eric Blake wrote: >>>> Besides, I feel that having a well-documented file format, so that >>>> independent applications can both parse the same file with the same >>>> semantics by obeying the file format specification, is a good design goal. >>> >>> We all know that documentation is rarely uptodate, new features may not >>> get added and libvirt will never be able to keep up. The driver for a >>> file format belongs in QEMU and nowhere else. >> >> This would be possible if QEMU to provide a libblockformat.so library >> which allowed apps to extract metadata from file formats using a stable >> API. > > There is no reason for libvirt or any external process to mess about > with the internals of image files. You have the same problem if the > image file is encrypted. Just repeating "libvirt doesn't need todo this" many times doesn't make it true. I have described why we need to read the disk image to determine its backing files ahead of QEMU being launched quite clearly.

On Tue, Jul 19, 2011 at 04:30:19PM +0200, Jes Sorensen wrote: > On 07/19/11 16:24, Eric Blake wrote: >> [adding the libvir-list] >> On 07/19/2011 08:09 AM, Jes Sorensen wrote: >>> Urgh, libvirt parsing image files is really unfortunate, it really >>> doesn't give me warm fuzzy feelings :( libvirt really should not know >>> about internals of image formats. >> >> But even if you add new features to qemu to avoid needing this in the >> future, it doesn't change the past - libvirt will always have to know >> how to parse image files understood by older qemu, and so as long as >> libvirt already knows how to do that parsing, we might as well take >> advantage of it. > > What has been done here in the past is plain wrong. Continuing to do it > isn't the right thing to do here. > >> Besides, I feel that having a well-documented file format, so that >> independent applications can both parse the same file with the same >> semantics by obeying the file format specification, is a good design goal. > > We all know that documentation is rarely uptodate, new features may not > get added and libvirt will never be able to keep up. The driver for a > file format belongs in QEMU and nowhere else. This would be possible if QEMU to provide a libblockformat.so library which allowed apps to extract metadata from file formats using a stable API.

Daniel