On Tue, Aug 23, 2016 at 06:17:44PM -0400, Martin Kletzander wrote: > On Tue, Aug 23, 2016 at 05:54:29PM -0400, Daniel P. Berrange wrote: > > On Tue, Aug 23, 2016 at 05:06:20PM -0400, Martin Kletzander wrote: > > > Hi everyone, > > > > > > so there was an idea about limiting the relabelling of images that > > > libvirt does. And I'm taking the liberty of pitching my idea how to > > > approach this. I feel like it's pretty simple thing and there's not > > > much to talk about, but a) I could've missed something and b) you might > > > hate the way I approach it. > > > > > > The idea is to extend the seclabel XML, for example: > > > > > > <seclabel type='dynamic' model='dac' relabel='whitelist'> > > > <path>/var/lib/libvirt/images</path> > > > <path>/data/virt-stuff</path> > > > </seclabel> > > > > > > Either we allow 'relabel' to be set to 'whitelist' or add a new > > > attribute with a name like 'mode' or something, which will control how > > > we relabel the files (actually relabel='no' can mean 'whitelist' and > > > relabel='yes' can mean blacklist without adding anything there). After > > > that you can specify what paths are (dis)allowed to be labelled. > > > > > > Actually thinking about it I like the following the most: > > > > > > <seclabel type='dynamic' model='dac' relabel='no'> > > > <whitelist path='/data'/> > > > <blacklist path='/data/private/non-virt/stuff'/> > > > </seclabel> > > > > > > which I believe is pretty explanatory. Feel free to ask if it's not. > > > And let me know what you think. > > > > I don't think we need to get involved in the <seclabel> configuration. > > > > I forgot to mention that I like that because you would be able to > specify this per-disk as well as for the whole VM. Of course using info in <disk> directly achieves the same thing > > We've already got the ability in the <disk> config to provide the > > full backing chain explicitly. If a management app provides a full > > backing chain in the XML, we could validate the app provided chain > > against the chain we probe and report error if they mis-match. (Maybe > > we in fact already report this?) > > > > Not yet: > > "It is currently ignored on input and only used for output to describe > the detected backing chains of running domains." > > It would help with this, but I don't feel like it's that usable. Also > I feel like everyone will overuse that while misunderstanding what it > actually does. Also if you do some block-merge or whatever, you need to > update the backing chain and everyone will just re-probe it because no > management layer likes keeping more information than needed. If you provide the <seclabel> whitelist though, you're essentially going to want to provide the same info that you would provide with the <backing> data, as the whitelist you're providing there is essentially trying to whitelist only the expected backing file. I don't feel like we should be inventing new seclabel elements to duplicate info we could already provide via existing backing data elements.

> > Thinking bout this in the context of a recent OpenStack Nova CVE, > > where Nova mistakenly set format=qcow2, instead of format=raw, allowing > > a malicious guest to write a qcow2 header with backing file. If Nova > > had provided the full backing chain it expected (no backing chain at > > all), then libvirt would have seen the maliciously created backing > > chain, and caught the problem despite Nova giving the wrong format. > > > > > > Separately from this, in the context of storage pools, when giving > > a <disk type=pool> in the domain XML, we could do validation to > > ensure the backing file of the volume always pointed to a volume > > that was also inside a storage pool. This would allow us to have > > backing files pointing to volumes in different storage pools, but > > would prevent them pointing to arbitrary files that were not in > > storage pools at all (eg /etc/password, or /dev/root, etc). > > > > That is good idea, but it won't cover all the cases, for example if > you're not using storage pools. Yep, at least from OpenStack POV our goal is to switch 100% to using storage pools. For non-storage pool deployments though, I think it is common that the mgmt app will have a specific place where it will always put disks. eg on OpenStack file based disks will always live under /var/lib/nova/instances. So if there was a qemu.conf setting to whitelist allowed disk image locations, we could lock down where we permit relabelling for the openstack host as a whole, and not need to change anything on a per guest basis.

> It could be nice to get feedback from upper mgmt layers, any idea where > else to post this questions? ovirt might have feedback i guess

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

-----BEGIN PGP SIGNATURE----- iQIcBAEBCAAGBQJXw/4jAAoJEAgfwp8kF4bdEV4P/1JP07oQ2ug+sGs5CImqbJak 6eajVCCcobapZgT7rDWbla5e2G5XMdzbV1vVzuIl2OVo4rGmcLuZjozVH8dewg0M tEjWLenbciXnUcqIrEA8PTP5qAdEAdVUZ/7kwFT3shRjTUOEYk46BM+fiRWz/pQM DI/xaPpXcZarfohLOGUgdRv+00shQPVZFg786FXBL+aYNFwTfbFDOnboYEYwKhq2 WGHG0PWLF3iIBlJWBFs4Nz1pQ48cw4CGWmXcOGtusIiqud5FsbKjNX+s64PDY1Yz 8UzC4c9+4LyqobUJp5EulsAmXedKK0bTxs4GI/eePkHZfBaRuOJtshbY2iETljsS Ce1s9Y3gpfP6UzAZImOa67F2rms9Fgv8Mm4pRnG2l/FTCvFSq7Grn8pPKL12jWAN Ti6hOQGCX45h8lN17zfERlWWPBFk3Mtw3L8bbh8Ja0xhYgsH0qS1mdeE/VmjKKFC OqZvT9GgtwFg4t77nHCIhFBSTEc3DoV6lNMrPxaTVswXCKSlYJAz2IUd8DFN5FFF 71WlKgvCKWl6iqo2FpxklhqfsUNSL6J31Kgh29r54qYcNZc1oobWsyJAqyeQcpER 3nHyY14QhIDc2LojcEisGXyPErtHZiVfexlB++Ap2O5W3tkxH08yWnV0Cd2J+CK2 MLB3K7JjrEt84oY8EGP8 =JNYW -----END PGP SIGNATURE-----