Re: [libvirt] [PATCH 1/2] nvram: Fix permissions

On 11.09.2014 16:07, Laszlo Ersek wrote: > On 09/11/14 14:09, Michal Privoznik wrote: >> I've noticed two problem with the automatically created NVRAM varstore >> file. The first, even though I run qemu as root:root for some reason I >> get Permission denied when trying to open the _VARS.fd file. The >> problem is, the upper directory misses execute permissions, which in >> combination with us dropping some capabilities result in EPERM. >> >> The next thing is, that if I switch SELinux to enforcing mode, I get >> another EPERM because the vars file is not labeled correctly. It is >> passed to qemu as disk and hence should be labelled as disk. QEMU may >> write to it eventually, so this is different to kernel or initrd. >> >> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; >> --- >> libvirt.spec.in | 2 +- >> src/security/security_selinux.c | 5 ++++- >> 2 files changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/libvirt.spec.in b/libvirt.spec.in >> index a6a58cf..ecf160b 100644 >> --- a/libvirt.spec.in >> +++ b/libvirt.spec.in >> @@ -1938,7 +1938,7 @@ exit 0 >> %dir %attr(0750, %{qemu_user}, %{qemu_group}) >> %{_localstatedir}/lib/libvirt/qemu/ >> %dir %attr(0750, %{qemu_user}, %{qemu_group}) >> %{_localstatedir}/lib/libvirt/qemu/channel/ >> %dir %attr(0750, %{qemu_user}, %{qemu_group}) >> %{_localstatedir}/lib/libvirt/qemu/channel/target/ >> -%dir %attr(0750, %{qemu_user}, %{qemu_group}) >> %{_localstatedir}/lib/libvirt/qemu/nvram/ >> +%dir %attr(0711, %{qemu_user}, %{qemu_group}) >> %{_localstatedir}/lib/libvirt/qemu/nvram/ >> %dir %attr(0750, %{qemu_user}, %{qemu_group}) >> %{_localstatedir}/cache/libvirt/qemu/ >> %{_datadir}/augeas/lenses/libvirtd_qemu.aug >> %{_datadir}/augeas/lenses/tests/test_libvirtd_qemu.aug >> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c >> index bf67fb5..3db2b27 100644 >> --- a/src/security/security_selinux.c >> +++ b/src/security/security_selinux.c >> @@ -2300,8 +2300,11 @@ >> virSecuritySELinuxSetSecurityAllLabel(virSecurityManagerPtr mgr, >> mgr) < 0) >> return -1; >> >> + /* This is different than kernel or initrd. The nvram store >> + * is really a disk, qemu can read and write to it. */ >> if (def->os.loader && def->os.loader->nvram && >> - virSecuritySELinuxSetFilecon(def->os.loader->nvram, >> data->content_context) < 0) >> + secdef && secdef->imagelabel && >> + virSecuritySELinuxSetFilecon(def->os.loader->nvram, >> secdef->imagelabel) < 0) >> return -1; >> >> if (def->os.kernel && >> > > Good detective work! > > Regarding the g+x,o+x change on > %{_localstatedir}/lib/libvirt/qemu/nvram. This change theoretically > allows a qemu instance to "probe" for the presence of "foreign" varstore > files (it won't be able to open any, but eg. error codes for open() > would differ between ENOENT vs. EACCES, and stat() would fail vs. > succeed). However I think we can live with this, and anyway, it's simply > impossible to open a file in directory D if directory D doesn't provide > the user with search permission. So that looks like a must. Indeed. Moreover, I was first surprised that even if was running qemu under root:root I got EACCES. Problem was, libvirt drops nearly all caps (even CAP_SYS_ADMIN) after fork and prior to executing qemu binary.