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

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.

Regarding the seclabel / context, I agree that it should have a label consistent with other disk image files; for qemu it's just a -drive after all. The hunk in question looks consistent with the rest of "src/security/security_selinux.c". Acked-by: Laszlo Ersek <lersek(a)redhat.com&gt;