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

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(a)redhat.com&gt; > > > > > > > > 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.

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?