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

[Richard W.M. Jones]

On Thu, Feb 28, 2013 at 03:01:20PM +0000, Matthew Booth wrote:
> On Thu, 2013-02-28 at 14:37 +0000, Daniel P. Berrange wrote:
> > 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'.
> 
> A couple of observations:
> 
> Firstly, this patch is essentially exposing an internal implementation
> detail to the user, one which may well change. I suspect we can infer
> from available metadata whether or not we require this. If so, I think
> it should be inferred rather than passed in by the user. I'm not
> convinced this is something we want to be adding to the API.

I agree.

Currently selinuxnorelabel is congruent to internal_-
set_libvirt_selinux_label having been called.  However in order to
overload that API a bit less, I suggest adding another internal API
which will be something like internal_set_libvirt_selinux_-
norelabel_disks.  Patch to follow.

> Secondly, by relabelling disk images we're potentially making them
> temporarily unavailable to other processes, which is something we
> weren't doing before. It's possible there may be no way round this, but
> if so we ought to highlight it as a major gotcha. Is there any way we
> can give a file 2 different SELinux contexts? If there were, we could
> leave the original untouched and only allow our ephemeral process to
> access the second one. A hard link doesn't allow this, btw. A copy is
> almost certainly infeasible in the general case, but if the FS allows
> reflink it might fly. Thoughts?

I can't really see a good way to fix this (note for the peanut
gallery: all this only applies to people who use libvirt + SELinux +
sVirt).

Copying disks doesn't sound like a good idea, even if the copies are
lightweight, because you have to examine disks looking for backing
files and potentially even modify the names of backing files within
the copied disk images.  It all sounds like a big mess.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming blog: http://rwmj.wordpress.com
Fedora now supports 80 OCaml packages (the OPEN alternative to F#)