[libvirt] [PATCH] qemuProcessStart: Be tolerant to relabel errors for session mode

[Michal Privoznik]

On 20.07.2015 15:50, Daniel P. Berrange wrote:
> On Wed, Jul 15, 2015 at 03:02:13PM +0200, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1124841
>>
>> When the daemon is running under unprivileged user, that is under
>> qemu:///session, there are plenty of operations we can't do. What
>> we can do is to go with best effort. One of such cases is
>> relabeling domain resources (be it disks, sockets, regular files,
>> etc.) during domain startup process. While we may successfully set
>> DAC labels, we can be fairly certain that any attempt to change
>> SELinux labels will fail. Therefore we should tolerate relabelling
>> errors and just let qemu to try access the resources. If it fails,
>> our error reporting system is strong enough to articulate the
>> exact error to the user anyway.
> 
> Errr, isn't it entirely the opposite to what you say. Running as
> an unprivileged user ID has no bearing on whether you are allowed
> to set SELinux labels. If the user acount is unconfined_t it can
> set any SELinux labels it wants. It will only fail if the libvird
> process is confined in some way.  IMHO we shold not be ignoring
> such failures.
> 
> What *will* fail is any attempt to set DAC labels, since you need
> CAP_CHOWN capability, but we shouldn't have the DAC security
> maanger running when in session mode.
> 
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index 1c0c734..58ed631 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -4856,8 +4856,13 @@ int qemuProcessStart(virConnectPtr conn,
>>  
>>      VIR_DEBUG("Setting domain security labels");
>>      if (virSecurityManagerSetAllLabel(driver->securityManager,
>> -                                      vm->def, stdin_path) < 0)
>> -        goto cleanup;
>> +                                      vm->def, stdin_path) < 0) {
>> +        /* Be tolerant to relabel errors if we are running unprivileged. */
>> +        if (virQEMUDriverIsPrivileged(driver))
>> +            goto cleanup;
>> +        else
>> +            VIR_DEBUG("Ignoring relabel errors for unprivileged daemon");
>> +    }
> 
> I really don't think we should do this here as it affects all
> security managers.
> 
> What is the failure you are actually seeing without this ?  SElinux
> label changes should be succeeding in session mode and we should not
> even be applying DAC labels

Unable to complete install: 'unable to set security context 'system_u:object_r:virt_content_t:s0' on '/usr/share/virtio-win/virtio-win.iso': Operation not permitted'

Traceback (most recent call last):
      File "/usr/share/virt-manager/virtManager/asyncjob.py", line 89, in cb_wrapper
          callback(asyncjob, *args, **kwargs)
        File "/usr/share/virt-manager/virtManager/create.py", line 1873, in do_install
          guest.start_install(meter=meter)
        File "/usr/share/virt-manager/virtinst/guest.py", line 417, in start_install
          noboot)
        File "/usr/share/virt-manager/virtinst/guest.py", line 481, in _create_guest
          dom = self.conn.createLinux(start_xml or final_xml, 0)
        File "/usr/lib64/python2.7/site-packages/libvirt.py", line 3585, in createLinux
          if ret is None:raise libvirtError('virDomainCreateLinux() failed', conn=self)
      libvirtError: unable to set security context 'system_u:object_r:virt_content_t:s0' on '/usr/share/virtio-win/virtio-win.iso': Operation not permitted


# ls -lZ /usr/share/virtio-win/virtio-win.iso
lrwxrwxrwx. root root system_u:object_r:usr_t:s0       /usr/share/virtio-win/virtio-win.iso -> virtio-win-1.7.4.iso

# ls -lZ /usr/share/virtio-win/virtio-win-1.7.4.iso
-rw-r--r--. root root system_u:object_r:usr_t:s0       /usr/share/virtio-win/virtio-win-1.7.4.iso


So maybe we should ignore just selinux labelling errors when running as unprivileged?

Michal