[Libguestfs] [PATCH 3/7] add_drive: Add selinuxnorelabel optional boolean.

[Daniel P. Berrange]

On Thu, Feb 28, 2013 at 02:20:54PM +0000, Richard W.M. Jones wrote:
> On Thu, Feb 28, 2013 at 02:00:50PM +0000, Matthew Booth wrote:
> > On Thu, 2013-02-28 at 10:57 +0000, Richard W.M. Jones wrote:
> > > From: "Richard W.M. Jones" <rjones at redhat.com>
> > > 
> > > If set, this causes <seclabel model=selinux relabel=no> to be added to
> > > the disk element in the libvirt XML.
> > > 
> > > It has no effect *except* on the libvirt attach method when SELinux
> > > and sVirt is being used.
> > > ---
> > >  generator/actions.ml   |  8 +++++++-
> > >  src/guestfs-internal.h |  1 +
> > >  src/launch-libvirt.c   | 20 ++++++++++++++++++++
> > >  src/launch.c           | 21 ++++++++++++++-------
> > >  4 files changed, 42 insertions(+), 8 deletions(-)
> > 
> > Is there any instance where we would *not* want to do this? Off the top
> > of my head, I can't think of a good reason for libguestfs to be in the
> > business of relabelling drives. Can we not just hard-code this off?
> > 
> > If, on the other hand, there is a good reason to make this option, I
> > think it should be explained in the docs below.
> 
> It's a tricky situation, but if libguestfs's appliance is confined by
> sVirt, and if we're accessing an ordinary, non-shared disk, then you
> have to have libvirt label the disk otherwise qemu won't be able to open it.
> 
> The situation here is different: we want to confine libguestfs's
> appliance with sVirt, and access a shared disk which already has some
> other label, so we label the appliance to conform to the disk.  The
> reason we don't want libvirt to label the disk is really a bug in
> libvirt: when the libvirt connection is closed, it resets the label
> *not* to what it was before, but to some other label.  We want to tell
> libvirt to leave it alone and not do this.  So you can argue that the
> need for this flag is to work around a bug in libvirt.
> 
> Another way to do this would be not to use svirt_t at all, but to have
> some other process label (sguestfs_t) which has fewer restrictions
> than svirt_t.  The trick would be to make the confinement meaningful,
> since the whole point of using sVirt with libguestfs is to confine the
> appliance against the threat of rogue disk images.  Also I guess this
> would need deep changes to libvirt.

There are several distinct aspects to this - the question of labelling,
the process domain and the process MCS category.

Before this patch, everything the libguestfs VM did was under a random
MCS level, which obviously clashed with what the existing running VM
required.

IMHO we want the libguestfs appliances to be as confined as is possible.
So if the libguestfs handle is using disks for a particular VM, then
policy must only allow it to access the disks for that specific VM.

Thus, from a policy POV, the key iss is that we need the MCS level of
the libguestfs VM to match the MCS level of the running VM.

What we're doing here with using a static label which 100% matches
the label of the running VM achieves that, at a cost though. The
cost is that the libguestfs VM is being given full read-write access
to every resource associated with the running VM, when it really only
needs to have read-only access to disks. The libguesfs VM monitor
sockets ends up with the same label sas the main VM monitor socket
which is also sub-optimal from a security POV. libguestfs should only
needs read-write access to its own qcow2 overlay image and its own
monitor socket.

I don't believe we can lock this down without a new policy domain for
the libguestfs VM and a libvirt enhancement.


We would need

 - A domain type  'svirt_guestfs_t'

 - A disk type 'svirt_guesfs_image_t'

 - A process with type  svirt_guestfs_t:s0:c123,456 would have
   *readonly* access to files svirt_image_t:s0:c123,456

 - A process with type  svirt_guestfs_t:s0:c123,456 would have
   *readwrite* access to files svirt_guestfs_image_t:s0:c123,456


So in that case you'd use

  <seclabel type="static" relabel="yes">
    <label>svirt_guestfs_t:s0:c123,456</label>
    <imagelabel>svirt_guestfs_image_t:s0:c123,456</imagelabel>
  </seclabel>


You would *allow* libvirt to relabel the qcow2 overlay that libguestfs
needs write access to.

There would need to be an enhancement to libvirt XML to allow you to
tell it to *not* try to relabel the qcow2 backing file though. That
must remain on 'svirt_image_t:s0:c123,456'.


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