FS#63152 - [libvirt] dropping privileges via libvirt seclabel does not work

Attached to Project: Community Packages
Opened by Thomas Krug (phragment) - Wednesday, 10 July 2019, 20:49 GMT
Last edited by Toolybird (Toolybird) - Thursday, 27 April 2023, 06:22 GMT
Task Type Bug Report
Category Packages
Status Closed
Assigned To Robin Broda (coderobe)
Architecture All
Severity Low
Priority Normal
Reported Version
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

Description:
Dropping privileges via libvirt seclabel does not work due to incorrect permissions.

Additional info:
package version: libvirt 5.4.0-1

Steps to reproduce:

# $EDITOR /etc/libvirt/qemu.conf
user = "root"
group = "root"

# virsh edit somedomain
<seclabel type='static' model='dac' relabel='yes'>
<label>nobody:kvm</label>
</seclabel>

Started virtual machine does not run as nobody:kvm.

Further info:
https://www.redhat.com/archives/libvir-list/2019-July/msg00511.html

# ls -ld /var/lib/libvirt/qemu/
drwxrwx--- 9 nobody kvm 4096 19. Jun 17:05 /var/lib/libvirt/qemu/

# chmod a+x /var/lib/libvirt/qemu/

# ls -ld /var/lib/libvirt/qemu/
drwxr-x--x 9 nobody kvm 4096 19. Jun 17:05 /var/lib/libvirt/qemu/

   permission.patch (0.4 KiB)
This task depends upon

Closed by  Toolybird (Toolybird)
Thursday, 27 April 2023, 06:22 GMT
Reason for closing:  Fixed
Comment by Toolybird (Toolybird) - Thursday, 11 July 2019, 04:45 GMT
Erm, patch is definitely wrong. Look a few lines above in the PKGBUILD and note:

rm -rf \
..
"${pkgdir}"/var/lib/libvirt/qemu

Arch is deleting that dir and letting libvirt create it on first run of daemon which appears to work fine.

On my system the perms are correct:

drwxr-xr-x 8 zzz kvm 4096 Jul 11 12:43 /var/lib/libvirt/qemu

(Sidenote: I'm not sure what libvirt does when /etc/libvirt/qemu.conf is subsequently edited with different user/group.)

In summary, it looks like upstream bug reporter has somehow acquired wrong perms on /var/lib/libvirt/qemu

On the question of whether Arch should be touching that dir or not? Dunno.
Comment by Thomas Krug (phragment) - Thursday, 11 July 2019, 12:50 GMT
Sorry for bogus patch. (It was late for me.)

The makefile from libvirt creates the path:
$(MKDIR_P) -m 0751 "$(DESTDIR)$(localstatedir)/lib/libvirt/qemu"

It is unclear for me why the path is deleted in the pkgbuild.

I could reproduce the issue on my system without modifying the permissions.
But my system is an "old" arch install (installed libvirt pkg around 2014) maybe this has changed a while ago?

I will look into it.
Comment by Thomas Krug (phragment) - Thursday, 11 July 2019, 20:55 GMT
Deletion of the directory was added in " FS#54943  do not hardcode kvm group number":
https://git.archlinux.org/svntogit/community.git/commit/trunk?h=packages/libvirt&id=9f43c804cfea3ff39bb912f800202a542ffd210e
(git-svn-id: file:///srv/repos/svn-community/svn@274310 9fca08f4-af9d-4005-b8df-a31f2cc04f65)

I cannot find the reason therefore, I would propose the attached patch.
   permission.patch (0.5 KiB)
Comment by loqs (loqs) - Thursday, 11 July 2019, 21:39 GMT
@phragment the reasoning for the directory removal is in https://bugs.archlinux.org/task/54943#comment163506 onward
Edit:
In your patch why the need for the chmod? The permissions were already set in the Makefile.
Also how would you resolve the possible mismatch mismatches between the dynamically assigned GID for kvm and UID for nobody between the build machine and the target machine?
Comment by Thomas Krug (phragment) - Thursday, 11 July 2019, 22:04 GMT
Thank you. I get why its not necessary to create them.
But libvirt will chmod them once and further config changes will silently fail, not dropping privileges.
Comment by loqs (loqs) - Thursday, 11 July 2019, 22:13 GMT
What about a tmpfiles snippet if you want to force the ownership / permissions on /var/lib/libvirt/qemu
This would avoid possible ID mismatches and I believe pacman will not change directory permissions on existing directories.
I still trying to understand how /var/lib/libvirt/qemu on your system became 0770.
Edit:
possibly a tmpfiles.d snippet such as

d /var/lib/libvirt/qemu 0751 kvm nobody
d /var/cache/libvirt/qemu 0750 kvm nobody
Comment by Toolybird (Toolybird) - Friday, 12 July 2019, 06:53 GMT
@loqs, the commit linked by Thomas shows Arch used to chmod 0770 "$pkgdir"/var/lib/libvirt/qemu

This explains why older libvirt installations (pre Dec 2017) will have the issue and newer ones won't.

I don't know much about tmpfiles.d but it sounds plausible.
Comment by Thomas Krug (phragment) - Friday, 19 July 2019, 13:23 GMT
@Toolybird, thank you for further investigating.

@loqs, I concur with you.

I attached a proposed patch.


Off Topic:
zfs is missing in makedepends. And there is a problem with wireshark deps?
wireshark/src/plugin.c:17:10: fatal error: wireshark/config.h: No such file or directory

pacman -Ss wireshark
community/wireshark-cli 3.0.2-1 [installed]
   tmpfiles.patch (0.5 KiB)
Comment by loqs (loqs) - Monday, 30 March 2020, 15:14 GMT
@phragment please try the attached source bundle. The PKGBUILD.diff shows the changes for this issue,
changes from the official build in previous commits updated libvirt to version 6.1.0 and fix firewalld support.
(application/x-gzip)    libvirt-6.1.0-2.src.tar.gz (3.5 KiB)
   PKGBUILD.diff (3.1 KiB)
Comment by Darrell (denns) - Sunday, 05 April 2020, 22:55 GMT
Same issue here with 6.1.0-2:

virsh -c lxc:/// start mycontainer
error: Failed to start domain mycontainer
error: internal error: guest failed to start: free(): invalid size
Comment by loqs (loqs) - Sunday, 05 April 2020, 23:05 GMT
@denns did you mean to comment on  FS#64789  instead of this bug?
Edit:
Are there any additional messages in libvirt's log [1] files when the container fails to start?

[1] https://libvirt.org/logging.html
Comment by loqs (loqs) - Wednesday, 06 May 2020, 17:07 GMT
5.3.0-1 only adjusts the ownership of /var/lib/libvirt/qemu if it exists. Is it expected to manually create the directory on a fresh install or run libvirt without seclabel first to create it with the wrong ownership then rerun tmpfiles?
Comment by Toolybird (Toolybird) - Thursday, 07 May 2020, 01:41 GMT
@loqs, my understanding is the perms are the problem, not the ownership. The dir is created on first run of daemon (with incorrect perm) then the tmpfiles.d snippet should kick in and set the correct perm (it does for me).

But I could easily be wrong as I don't use seclabel. It would be good if someone affected by this bug could test latest 6.3.0.
Comment by loqs (loqs) - Thursday, 07 May 2020, 13:08 GMT
d /var/lib/libvirt/qemu 0751 kvm nobody would create the directory on installation if missing avoiding having to run libvirt then trigger tmpfiles then run libvirt again.
Comment by Toolybird (Toolybird) - Thursday, 07 May 2020, 21:30 GMT
> d /var/lib/libvirt/qemu 0751 kvm nobody would create the directory on installation

True. But doesn't that mean we fail to fix up the already broken installations out there? That's why your earlier suggestion of using z was pure genius :)
Comment by loqs (loqs) - Thursday, 07 May 2020, 22:53 GMT
Absolutely I should have made it clear the d line was in addition to the z line.
The z line could be dropped at some future point after all users are expected to have updated.

Loading...