Bug 1652078 - RFE: the security drivers must remember original permissions/labels and restore them after
Summary: RFE: the security drivers must remember original permissions/labels and resto...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat Enterprise Linux Advanced Virtualization
Classification: Red Hat
Component: libvirt
Version: ---
Hardware: All
OS: Linux
low
medium
Target Milestone: rc
: ---
Assignee: Michal Privoznik
QA Contact: Luyao Huang
URL:
Whiteboard:
Depends On: 1780154 1740024 1740506 1741140 1741456 1771500 1771501
Blocks: 1203710 1205796 1420851 1477664 1524792 1623566 756082 822052 829181 1126678 1288337
TreeView+ depends on / blocked
 
Reported: 2018-11-21 14:24 UTC by Jaroslav Suchanek
Modified: 2019-12-05 14:08 UTC (History)
32 users (show)

Fixed In Version: libvirt-5.6.0-1.el8
Doc Type: Enhancement
Doc Text:
Clone Of: 547546
Environment:
Last Closed: 2019-11-06 07:12:03 UTC
Type: Feature Request
Target Upstream Version:


Attachments (Terms of Use)


Links
System ID Priority Status Summary Last Updated
Red Hat Product Errata RHBA-2019:3723 None None None 2019-11-06 07:12:47 UTC

Description Jaroslav Suchanek 2018-11-21 14:24:22 UTC
+++ This bug was initially created as a clone of Bug #547546 +++

Description of problem:
The current SELinux and uid/gid changing code always restores the permissions back to a default generic label, and root:root respectively. These are bad assumptions, because the original file may have had non-default/non-root labelling/ownership.

There needs to be a mechanism for the security drivers to record the original credentials, so they can be correctly restored later. This needs to be persistent across libvirtd restart, and cope with multiple guests using the same files concurrently.

Version-Release number of selected component (if applicable):
libvirt-0.7.4

How reproducible:


Steps to Reproduce:
1.
2.
3.
  
Actual results:


Expected results:


Additional info:



--- Additional comment from Laine Stump on 2011-08-24 21:50:34 CEST ---

Bug 534010 was initially opened for a much narrower problem (image files are chowned to root during domain restore, even if dynamic_ownership=0), which has been resolved. In order to track the larger problem properly, I'm re-opening this bug rather than creating a new one.

--- Additional comment from Wayne Sun on 2011-11-29 08:43:51 CET ---

package:
# rpm -q libvirt
libvirt-0.9.4-23.el6.x86_64

possible test steps:

scenario 1:
1. prepare a domain, chown the disk file to non-root user
# chown wayne:wayne /var/lib/libvirt/images/rhel6u2
# ll -Z /var/lib/libvirt/images/
-rw-r--r--. wayne wayne system_u:object_r:virt_image_t:s0 rhel6u2

2. start the domain
# virsh start rhel6u2
Domain rhel6u2 started

3. check the file ownership
# ll -Z /var/lib/libvirt/images/rhel6u2
-rw-r--r--. qemu qemu system_u:object_r:svirt_image_t:s0:c31,c303 /var/lib/libvirt/images/rhel6u2

4. destroy the domain and recheck
# virsh destroy rhel6u2
Domain rhel6u2 destroyed

# ll -Z /var/lib/libvirt/images/rhel6u2
-rw-r--r--. root root system_u:object_r:virt_image_t:s0 /var/lib/libvirt/images/rhel6u2

5. restart libvirtd and recheck
# service libvirtd restart
Stopping libvirtd daemon:                                  [  OK  ]
Starting libvirtd daemon:                                  [  OK  ]
# ll -Z /var/lib/libvirt/images/rhel6u2
-rw-r--r--. root root system_u:object_r:virt_image_t:s0 /var/lib/libvirt/images/rhel6u2

^^after destroy and libvirtd restart, the disk file restore to root:root

scenario 2:
1. prepare a shareable non-root iso file
# ll -Z /var/lib/libvirt/images/aa.iso
-rw-r--r--. wayne wayne system_u:object_r:virt_content_t:s0 aa.iso

2. attach the iso file to active domain and check ownership
prepare insert.xml using /var/lib/libvirt/images/aa.iso as source file
# virsh update-device rhel6u2 insert.xml
Device updated successfully

# ll -Z /var/lib/libvirt/images/aa.iso 
-rw-r--r--. qemu qemu system_u:object_r:virt_content_t:s0 /var/lib/libvirt/images/aa.iso

3. eject the iso file and recheck
# virsh update-device rhel6u2 /root/eject.xml 
Device updated successfully

# ll -Z /var/lib/libvirt/images/aa.iso 
-rw-r--r--. qemu qemu system_u:object_r:virt_content_t:s0 /var/lib/libvirt/images/aa.iso

4. destroy domain and recheck
# virsh destroy rhel6u2
Domain rhel6u2 destroyed

# ll -Z /var/lib/libvirt/images/aa.iso 
-rw-r--r--. qemu qemu system_u:object_r:virt_content_t:s0 /var/lib/libvirt/images/aa.iso

5. restart libvirtd and recheck
# service libvirtd restart
Stopping libvirtd daemon:                                  [  OK  ]
Starting libvirtd daemon:                                  [  OK  ]
# ll -Z /var/lib/libvirt/images/aa.iso 
-rw-r--r--. qemu qemu system_u:object_r:virt_content_t:s0 /var/lib/libvirt/images/aa.iso

^^after detach and restart libvirtd, the shareable iso file remain qemu:qemu

--- Additional comment from Martin Kletzander on 2012-05-25 15:38:02 CEST ---

As you mentioned in Bug 534010, there are three possibilities to go around this problem. The cleaner ones (xattr or acl) are not possible on all mounts/filesystems.
My question is if we can say that dynamic_ownership=1 is only supported on these mounts and otherwise behave the same way as with dynamic_ownership=0, but with warning (for example)?
Thanks


--- Additional comment from Michal Privoznik on 2014-09-10 15:27:45 CEST ---

I've proposed patches upstream:

https://www.redhat.com/archives/libvir-list/2014-September/msg00551.html


--- Additional comment from Michal Privoznik on 2018-07-27 14:37:21 CEST ---

I've started some discussion upstream:

https://www.redhat.com/archives/libvir-list/2018-July/msg01874.html

--- Additional comment from Olimp Bockowski on 2018-07-27 14:44:23 CEST ---

@Michal - thank you for details, appreciate, for me it is really helpful because I can explain why it is not resolved and write that we try to change it but it is not easy.

--- Additional comment from Olimp Bockowski on 2018-10-17 10:59:40 CEST ---

Hello Michal, 
I see you discussed the approach, have you had some time to work on it or are there any plans?

--- Additional comment from Michal Privoznik on 2018-10-17 12:53:55 CEST ---

(In reply to Olimp Bockowski from comment #35)
> Hello Michal, 
> I see you discussed the approach, have you had some time to work on it or
> are there any plans?

Yes, this is WIP and it is at the top of my TODO list. There is this metadata locking feature that needs to be implemented first (see bug 1524792). So I'm focusing on that as it is a prerequisite.

Comment 3 Whitney Chadwick 2018-12-07 14:04:44 UTC
Per Dec 6th blocker meeting, approved exception+

Comment 6 Jaroslav Suchanek 2019-01-09 13:36:17 UTC
Moving to post accordingly to bug 1652078.

Comment 7 Xuesong Zhang 2019-01-10 02:34:21 UTC
(In reply to Jaroslav Suchanek from comment #6)
> Moving to post accordingly to bug 1652078.

Should be BZ 547546, I guess. :)

Comment 8 Jiri Denemark 2019-01-25 11:29:53 UTC
This feature was disabled upstream due to some non-trivial bugs by

commit fc3990c7e64be1da1631952d3ec384ebef50e125
Refs: v5.0.0-rc2-4-gfc3990c7e6
Author:     Michal Privoznik <mprivozn@redhat.com>
AuthorDate: Mon Jan 14 17:53:43 2019 +0100
Commit:     Michal Privoznik <mprivozn@redhat.com>
CommitDate: Tue Jan 15 09:45:22 2019 +0100

    qemu: Temporary disable owner remembering

    Turns out, that there are few bugs that are not that trivial to
    fix (e.g. around block jobs). Instead of rushing in not
    thoroughly tested fixes disable the feature temporarily for the
    release.

    Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
    ACKed-by: Peter Krempa <pkrempa@redhat.com>

Comment 9 Michal Privoznik 2019-04-25 08:21:28 UTC
v4:

https://www.redhat.com/archives/libvir-list/2019-April/msg01351.html

Previous versions were targeting bug 547546.

Comment 12 jiyan 2019-09-11 05:51:26 UTC
Hi Michal
I found the following test result for guestfwd channel chardev.
Could you please check whether it is normal? Thank you.

Description:
Xattr for “guestfwd channel” device will not be cleaned after VM was destroyed 
or device was detached; thus; Other VM can not use it.

Version:
libvirt-5.6.0-4.module+el8.1.0+4160+b50057dc.x86_64
qemu-kvm-4.1.0-9.module+el8.1.0+4210+23b2046a.x86_64
kernel-4.18.0-141.el8.x86_64

Steps:
1: Prepare 2 guests as follows:
# virsh list --all
 Id   Name             State
---------------------------------
 -    avocado-vt-vm1   shut off
 -    test             running

2. Start 1 guest with the following guestfwd channel and check file label
# virsh dumpxml avocado-vt-vm1 --inactive |grep "<channel" -A5
    <channel type='unix'>
      <source mode='bind' path='/mnt/guestfwd'/>
      <target type='guestfwd' address='10.0.2.1' port='4600'/>
      <alias name='ua-d830c2c4-93ac-4eb7-b714-593483f10044'/>
    </channel>

# virsh start avocado-vt-vm1 
Domain avocado-vt-vm1 started

# getfattr -m trusted.libvirt.security -d /mnt/guestfwd 
getfattr: Removing leading '/' from absolute path names
# file: tmp/guestfwd
trusted.libvirt.security.dac="+0:+0"
trusted.libvirt.security.ref_dac="1"
trusted.libvirt.security.timestamp_dac="1568151376"

3. Detach channel device from the gust and check file label again; attach the device to the other guest
# virsh detach-device-alias avocado-vt-vm1 ua-d830c2c4-93ac-4eb7-b714-593483f10044
Device detach request sent successfully

# virsh dumpxml avocado-vt-vm1|grep "<channel" -A5
No output

# getfattr -m trusted.libvirt.security -d /mnt/guestfwd 
getfattr: Removing leading '/' from absolute path names
# file: tmp/guestfwd
trusted.libvirt.security.dac="+0:+0"
trusted.libvirt.security.ref_dac="1"
trusted.libvirt.security.timestamp_dac="1568151376"

# cat unix_channel_2.xml 
    <channel type='unix'>
      <source mode='bind' path='/mnt/guestfwd'/>
      <target type='guestfwd' address='10.0.2.1' port='4600'/>
      <alias name='ua-d830c2c4-93ac-4eb7-b714-593483f10044'/>
    </channel>

# virsh attach-device test unix_channel_2.xml 
error: Failed to attach device from unix_channel_2.xml
error: internal error: unable to execute QEMU command 'chardev-add': Failed to unlink socket /mnt/guestfwd: Permission denied

4. Destroy the first guest and attach the device to the other one again
# virsh destroy avocado-vt-vm1 
Domain avocado-vt-vm1 destroyed

# getfattr -m trusted.libvirt.security -d /tmp/guestfwd 
getfattr: Removing leading '/' from absolute path names
# file: tmp/guestfwd
trusted.libvirt.security.dac="+0:+0"
trusted.libvirt.security.ref_dac="2"
trusted.libvirt.security.timestamp_dac="1568151376"

# virsh attach-device test unix_channel_2.xml 
error: Failed to attach device from unix_channel_2.xml
error: internal error: unable to execute QEMU command 'chardev-add': Failed to unlink socket /mnt/guestfwd: Permission denied

Comment 13 Michal Privoznik 2019-09-11 13:11:30 UTC
(In reply to jiyan from comment #12)
> Hi Michal
> I found the following test result for guestfwd channel chardev.
> Could you please check whether it is normal? Thank you.
> 
> Description:
> Xattr for “guestfwd channel” device will not be cleaned after VM was
> destroyed 
> or device was detached; thus; Other VM can not use it.
> 
> Version:
> libvirt-5.6.0-4.module+el8.1.0+4160+b50057dc.x86_64
> qemu-kvm-4.1.0-9.module+el8.1.0+4210+23b2046a.x86_64
> kernel-4.18.0-141.el8.x86_64
> 
> Steps:
> 1: Prepare 2 guests as follows:
> # virsh list --all
>  Id   Name             State
> ---------------------------------
>  -    avocado-vt-vm1   shut off
>  -    test             running
> 
> 2. Start 1 guest with the following guestfwd channel and check file label
> # virsh dumpxml avocado-vt-vm1 --inactive |grep "<channel" -A5
>     <channel type='unix'>
>       <source mode='bind' path='/mnt/guestfwd'/>
>       <target type='guestfwd' address='10.0.2.1' port='4600'/>
>       <alias name='ua-d830c2c4-93ac-4eb7-b714-593483f10044'/>
>     </channel>
> 
> # virsh start avocado-vt-vm1 
> Domain avocado-vt-vm1 started
> 
> # getfattr -m trusted.libvirt.security -d /mnt/guestfwd 
> getfattr: Removing leading '/' from absolute path names
> # file: tmp/guestfwd
> trusted.libvirt.security.dac="+0:+0"
> trusted.libvirt.security.ref_dac="1"
> trusted.libvirt.security.timestamp_dac="1568151376"
> 
> 3. Detach channel device from the gust and check file label again; attach
> the device to the other guest
> # virsh detach-device-alias avocado-vt-vm1
> ua-d830c2c4-93ac-4eb7-b714-593483f10044
> Device detach request sent successfully
> 
> # virsh dumpxml avocado-vt-vm1|grep "<channel" -A5
> No output
> 
> # getfattr -m trusted.libvirt.security -d /mnt/guestfwd 
> getfattr: Removing leading '/' from absolute path names
> # file: tmp/guestfwd
> trusted.libvirt.security.dac="+0:+0"
> trusted.libvirt.security.ref_dac="1"
> trusted.libvirt.security.timestamp_dac="1568151376"

Why is the file still there? When I try to reproduce the file is unlinked as soon as qemu closes the socket. At this point I get 'no such file or directory' error. Do you have something connected to the socket?

> 
> # cat unix_channel_2.xml 
>     <channel type='unix'>
>       <source mode='bind' path='/mnt/guestfwd'/>
>       <target type='guestfwd' address='10.0.2.1' port='4600'/>
>       <alias name='ua-d830c2c4-93ac-4eb7-b714-593483f10044'/>
>     </channel>
> 
> # virsh attach-device test unix_channel_2.xml 
> error: Failed to attach device from unix_channel_2.xml
> error: internal error: unable to execute QEMU command 'chardev-add': Failed
> to unlink socket /mnt/guestfwd: Permission denied
> 
> 4. Destroy the first guest and attach the device to the other one again
> # virsh destroy avocado-vt-vm1 
> Domain avocado-vt-vm1 destroyed
> 
> # getfattr -m trusted.libvirt.security -d /tmp/guestfwd 
> getfattr: Removing leading '/' from absolute path names
> # file: tmp/guestfwd
> trusted.libvirt.security.dac="+0:+0"
> trusted.libvirt.security.ref_dac="2"
> trusted.libvirt.security.timestamp_dac="1568151376"

I don't understand how is /tmp/guestfwd related to all of this - the XMLs show /mnt/... and not /tmp/...

> 
> # virsh attach-device test unix_channel_2.xml 
> error: Failed to attach device from unix_channel_2.xml
> error: internal error: unable to execute QEMU command 'chardev-add': Failed
> to unlink socket /mnt/guestfwd: Permission denied

Anyway, from code inspection I think there might be a problem because our set and restore code doesn't look exactly the same. So in a few cases more we might set the label but then not restore it.  This is the patch:


diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 9e71513f14..1600abaf54 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -1565,9 +1565,11 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr,
         break;
 
     case VIR_DOMAIN_CHR_TYPE_UNIX:
-        if (!dev_source->data.nix.listen &&
-            virSecurityDACRestoreFileLabel(mgr, dev_source->data.nix.path) < 0) {
-            goto done;
+        if (!dev_source->data.nix.listen ||
+            (dev_source->data.nix.path &&
+             virFileExists(dev_source->data.nix.path))) {
+            if (virSecurityDACRestoreFileLabel(mgr, dev_source->data.nix.path) < 0)
+                goto done;
         }
         ret = 0;
         break;

Can you test it please (or I can create a scratch build for you with that patch if you want me to)?

Comment 14 jiyan 2019-09-12 01:39:40 UTC
Hi it is better to create a scratch build; I will test once I get it. :)

Comment 15 Michal Privoznik 2019-09-13 07:05:09 UTC
Here you go:

https://brewweb.engineering.redhat.com/brew/taskinfo?taskID=23486530

Comment 16 jiyan 2019-09-18 02:31:26 UTC
Hi Michal
Sry for testing the scratch build a little late.

The following test steps shows; after detaching the channel device; the xattr info will be cleared; 
however, attaching the channel device to another guest failed as well.

Steps:
# rpm -qa libvirt
libvirt-5.6.0-5.module+el8.1.0+4158+21722aafchannel.x86_64

# virsh list --all
 Id   Name     State
-------------------------
 -    test_1   shut off
 -    test_2   shut off

# virsh dumpxml test_1 --inactive |grep "<channel" -A4
    <channel type='unix'>
      <source mode='bind' path='/mnt/guestfwd'/>
      <target type='guestfwd' address='10.0.2.1' port='4600'/>
      <alias name='ua-d830c2c4-93ac-4eb7-b714-593483f10044'/>
    </channel>

# virsh dumpxml test_2 --inactive |grep "<channel" -A4
No output

# virsh start test_1 
Domain test_1 started

# virsh start test_2 
Domain test_2 started

# getfattr -m trusted.libvirt.security -d /mnt/guestfwd 
getfattr: Removing leading '/' from absolute path names
# file: mnt/guestfwd
trusted.libvirt.security.dac="+0:+0"
trusted.libvirt.security.ref_dac="1"
trusted.libvirt.security.timestamp_dac="1568706200"

# virsh detach-device-alias test_1 ua-d830c2c4-93ac-4eb7-b714-593483f10044
Device detach request sent successfully

# getfattr -m trusted.libvirt.security -d /mnt/guestfwd 
No output

# virsh dumpxml test_1 |grep "<channel" -A4
No output

# virsh attach-device test_2 channel.xml 
error: Failed to attach device from channel.xml
error: internal error: unable to execute QEMU command 'chardev-add': Failed to unlink socket /mnt/guestfwd: Permission denied

# getfattr -m trusted.libvirt.security -d /mnt/guestfwd 

# virsh attach-device test_2 channel.xml 
error: Failed to attach device from channel.xml
error: internal error: unable to execute QEMU command 'chardev-add': Failed to unlink socket /mnt/guestfwd: Permission denied

Since the bug: Bug 1591636 - Fails to hotplug/unplug guestfwd character device has been verified on libvirt-5.3.0-1.el8.
Could you check whether the code changes have a influence on this feature? Thank you. :)

Comment 17 jiyan 2019-09-23 07:12:49 UTC
The issue in comment 12 - comment 16 will be tracked in this bug https://bugzilla.redhat.com/show_bug.cgi?id=1754392, thank you. :)

Comment 18 Luyao Huang 2019-09-25 07:47:52 UTC
Verify this bug with libvirt-5.6.0-6.module+el8.1.0+4244+9aa4e6bb.x86_64:

S1 Test file permission selinux/dac label remembering:

1. prepare a guest which disk use local file

# virsh dumpxml vm1
...
    <disk type='file' device='disk'>
      <driver name='qemu' type='qcow2'/>
      <source file='/var/lib/libvirt/images/RHEL-8.1-x86_64-latest.qcow2'/>
      <target dev='vda' bus='virtio'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/>
    </disk>
...

2. check guest image label

# ll -Z /var/lib/libvirt/images/RHEL-8.1-x86_64-latest.qcow2
-rw-r--r--. 1 root root system_u:object_r:virt_image_t:s0 722403328 Sep 24 22:48 /var/lib/libvirt/images/RHEL-8.1-x86_64-latest.qcow2

3. check guest image XATTR param
# getfattr -m trusted.libvirt.security -d /var/lib/libvirt/images/RHEL-8.1-x86_64-latest.qcow2

4. start guest
# virsh start vm1
Domain vm1 started

5. check image label
# ll -Z /var/lib/libvirt/images/RHEL-8.1-x86_64-latest.qcow2
-rw-r--r--. 1 qemu qemu system_u:object_r:svirt_image_t:s0:c748,c836 722403328 Sep 24 22:49 /var/lib/libvirt/images/RHEL-8.1-x86_64-latest.qcow2

6. check guest image XATTR param
# getfattr -m trusted.libvirt.security -d /var/lib/libvirt/images/RHEL-8.1-x86_64-latest.qcow2 
getfattr: Removing leading '/' from absolute path names
# file: var/lib/libvirt/images/RHEL-8.1-x86_64-latest.qcow2
trusted.libvirt.security.dac="+0:+0"
trusted.libvirt.security.ref_dac="1"
trusted.libvirt.security.ref_selinux="1"
trusted.libvirt.security.selinux="system_u:object_r:virt_image_t:s0"
trusted.libvirt.security.timestamp_dac="1569204150"
trusted.libvirt.security.timestamp_selinux="1569204150"

7. destroy guest
# virsh destroy vm1
Domain vm1 destroyed

8. verify libvirt  restore the label
# ll -Z /var/lib/libvirt/images/RHEL-8.1-x86_64-latest.qcow2
-rw-r--r--. 1 root root system_u:object_r:virt_image_t:s0 722403328 Sep 24 22:50 /var/lib/libvirt/images/RHEL-8.1-x86_64-latest.qcow2

9. verify libvirt clear XATTR
# getfattr -m trusted.libvirt.security -d /var/lib/libvirt/images/RHEL-8.1-x86_64-latest.qcow2

And test with different operation: hotplug, unplug, start, destroy, migrate and with different device: bios, disk, interface, channel, console, input, memory


S2 Test ref count change

# getfattr -m trusted.libvirt.security -d /tmp/nvdimm 
getfattr: Removing leading '/' from absolute path names
# file: tmp/nvdimm
trusted.libvirt.security.dac="+0:+0"
trusted.libvirt.security.ref_dac="1"
trusted.libvirt.security.ref_selinux="1"
trusted.libvirt.security.selinux="unconfined_u:object_r:user_tmp_t:s0"
trusted.libvirt.security.timestamp_dac="1569204150"
trusted.libvirt.security.timestamp_selinux="1569204150"

# virsh attach-device vm1 nvdimm.xml
Device attached successfully

# getfattr -m trusted.libvirt.security -d /tmp/nvdimm 
getfattr: Removing leading '/' from absolute path names
# file: tmp/nvdimm
trusted.libvirt.security.dac="+0:+0"
trusted.libvirt.security.ref_dac="2"
trusted.libvirt.security.ref_selinux="2"
trusted.libvirt.security.selinux="unconfined_u:object_r:user_tmp_t:s0"
trusted.libvirt.security.timestamp_dac="1569204150"
trusted.libvirt.security.timestamp_selinux="1569204150"


S3 Test remember_owner in qemu.conf

1. do the same test with S1 when disable remember_owner and verify there is no regression and there is no XATTR
3. start a guest with enable remember_owner in qemu.conf then disable remember_owner and restart libvirtd
   then do hotplug/hotunplug/destroy to test XATTR set/clear
4. start a guest with disable remember_owner in qemu.conf then enable remember_owner and restart libvirtd
   then do hotplug/hotunplug/destroy and check there is no XATTR change

Comment 19 Michal Privoznik 2019-10-23 13:08:32 UTC
Clearing out sale needinfo.

Comment 21 errata-xmlrpc 2019-11-06 07:12:03 UTC
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2019:3723


Note You need to log in before you can comment on or make changes to this bug.