[libvirt] RFC: a couple ideas regarding selinux/DAC labeling

[Cole Robinson]

Hi all,

libvirt's selinux/DAC labeling behavior has been a repeated source of
frustration for desktop virt users. Granted much of the frustration comes from
the fact that virt-manager runs as $user, talks to libvirtd running as root,
which launches VMs running as qemu, and we are mixing it all with selinux :)

But the flip side is that I've lost count of how many times I've heard someone
complain 'why is libvirt chowning/relabeling my .iso? it's world readable'

I spent some time looking at bug reports and looking at the relevant libvirt
code, and I have a couple ideas for reducing user complaints in this area. The
patches should be pretty simple but I haven't written them yet since I'd like
other opinions on this stuff, given it could impact security. Here's my two
suggestions:


1) Don't attempt to change selinux label for readonly/shareable images, if the
image already has a label that svirt_t can access.

Background: svirt is about giving r/w disk images a unique selinux label tied
to one running VM, so that if other VMs are compromised, they can't access
another VMs disk image. Maintaining this guarantee is important, which is why
libvirt will raise an explicit error if attempting to change the disk image
label errors with EPERM and selinux==Enforcing. Makes sense.

The problem is with readonly media. We don't give a unique label to RO media,
we just give it plain virt_content_t. RO media labeling isn't about VM
isolation, it's about ensuring the svirt VM can even access the media.
However, if labeling fails with EPERM, we still fail in the same way mentioned
above.

Here's a situation where that is problematic:

- Running as qemu:///session (unprivileged)
- Point a VM at /path/to/my/isos/foo.iso
- foo.iso is: owner=root, mode=644, label=public_content_t. We
  are sharing this media for qemu:///system usage as well

Without any libvirt label changing, the VM should be able to access that ISO:
permissions are fine, and svirt_t can access public_content_t. However the
unprivileged user does not have permissions to change the label to
virt_content_t, libvirt fails to set the label, raises the error, VM startup
fails.

I suggest that in the case of readonly (and maybe shareable which uses the
plain svirt_image_t label) disk, we first check to see if svirt_t can access
the existing label, and if so, don't do any explicit labeling. This makes
things 'just work' in the above example, and doesn't seem to have a
detrimental effect on security. I don't know exactly how to do this with the
selinux API but I assume there's something that reproduces what sesearch does
on the CLI.

Another case this helps: qemu:///system VM wants to use /dev/sr0, changes the
label to virt_content_t, which can generate selinux violations from udev when
media is ejected/inserted and the VM is running (I don't remember specifics
here but we've had bug reports in the past). Instead it just leaves /dev/sr0
as removable_device_t, which svirt_t can already read from.

(Libvirt already does something along these lines: If the labeling fails, it
checks to see if the disk image _already_ has the label we just requested, and
if so, it's not an error condition. But this suggestion goes a step further
than that)


2) DAC shouldn't change owner if qemu will be able to access the disk.

Background: My understanding of the DAC driver is it's _just_ to facilitate
running qemu as non-root; we chown the image to ensure the emulator has
permissions to access it so that VM startup doesn't fail. So I suggest that if
libvirt determines the disk is already accessible by the qemu user, we skip
the chown step.

Coupled with the above selinux erroring mentioned above, unnecessary chowning
can cause issues when sharing media between qemu:///system and
qemu:///session; once the file is owned by qemu, qemu:///session can't change
the selinux label ever again, and labeling failures may mean the VM fails to
start.

Thankfully we have libvirt infrastructure for checking whether a file can be
read by arbitrary uid:gid: virStorageFileInitAs and friends. We already use
that to check the file is searchable at qemu startup time, via:
    qemuProcessState->
      qemuDomainCheckDiskPresence->
        qemuDomainDetermineDiskChain->
          virStorageFileGetMetadata->
            virStorageFileGetMetadataRecurse

So I suggest we add a check along those lines in security_dac.c, and if the
file is already accessible for the disk's needs, we skip the chown'ing.


Thoughts/comments appreciated.

Thanks,
Cole