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

[Michal Privoznik]

On 12.09.2014 00:20, Laszlo Ersek wrote:
> On 09/11/14 16:25, Michal Privoznik wrote:
>> 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 at redhat.com>
>>>> ---
>>>>    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.
>>
>>>
>>> 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 at redhat.com>
>>>
>>
>> Thanks, applied.
>
> ... Unfortunately the patch is insufficient. Commit 742b08e3 added directory
>
>    %{_localstatedir}/lib/libvirt/qemu/nvram/
>
> to two sub-packages:
>
>    %files daemon
>    %files daemon-driver-qemu
>
> I assume that was a correct thing to do (it is consistent with the rest
> of the directories being listed for both subpackages).
>
> However, this recent patch (commit 37d8c75f) changes the 0750 permission
> bits to 0711 only in the "daemon" subpackage, and the directory in the
> daemon-driver-qemu subpackage remains with 0750. In my testing, the
> daemon package's modification doesn't take effect at all. In fact the
> daemon RPM doesn't even seem to contain the nvram directory.

Ah, thanks for catching that. I'm pushing the obvious trivial fix. Thanks!

Michal