[libvirt] [PATCH] security: dac: also label listen UNIX sockets

[Ján Tomko]

On Mon, Oct 01, 2018 at 10:22:59AM +0200, Erik Skultety wrote:
>On Thu, Sep 27, 2018 at 05:02:13PM +0200, Ján Tomko wrote:
>> We switched to opening mode='bind' sockets ourselves:
>> commit 30fb2276d88b275dc2aad6ddd28c100d944b59a5
>>     qemu: support passing pre-opened UNIX socket listen FD
>> in v4.5.0-rc1~251
>>
>> Then fixed qemuBuildChrChardevStr to change libvirtd's label
>> while creating the socket:
>> commit b0c6300fc42bbc3e5eb0b236392f7344581c5810
>>     qemu: ensure FDs passed to QEMU for chardevs have correct SELinux labels
>> v4.5.0-rc1~52
>>
>> Also add labeling of these sockets to the DAC driver.
>> Instead of trying to figure out which one was created by libvirt,
>> label it if it exists.
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1633389
>>
>> Signed-off-by: Ján Tomko <jtomko at redhat.com>
>> ---
>>  src/security/security_dac.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
>> index 62442745dd..da4a6c72fe 100644
>> --- a/src/security/security_dac.c
>> +++ b/src/security/security_dac.c
>> @@ -1308,7 +1308,12 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
>>          break;
>>
>>      case VIR_DOMAIN_CHR_TYPE_UNIX:
>> -        if (!dev_source->data.nix.listen) {
>> +        if (!dev_source->data.nix.listen ||
>> +            (dev_source->data.nix.path &&
>> +             virFileExists(dev_source->data.nix.path))) {
>> +            /* Also label mode='bind' sockets if they exist,
>> +             * e.g. because they were created by libvirt
>> +             * and passed via FD */
>>              if (virSecurityDACSetOwnership(mgr, NULL,
>>                                             dev_source->data.nix.path,
>>                                             user, group) < 0)
>
>So we're labeling path even if nix.listen == false, shouldn't we check for the
>file's existence too? Or have we already done it and I missed this fact?

For mode='connect', the path not existing is fatal, so we can safely
call virSecurityDACSetOwnership and let it fail.

For mode='bind', the path not existing is valid for QEMUs which do not
support FD passing for chardevs. If QEMU supports FD passing, the
path should exist because libvirt created it earlier. I could not think
of a nicer idea on how to pass down the logic that decides whether
libvirt pre-creates it in qemuBuildChrChardevStr (which, of course, is
an odd place to be creating sockets), so I opted to use the file's
existence as a witness of libvirt pre-creating it. Which is what I tried
to say by this commit message snippet:
>> Instead of trying to figure out which one was created by libvirt,
>> label it if it exists.

Shall I reword the message? E.g.:
Instead of duplicating the logic which decides whether libvirt should
pre-create the socket, assume an existing path means it was created by
libvirt.


>Basically what I'm aiming at is that nix.path will always be set at this point,

Yes, the dev_source->data.nix.path condition might be superfluous. I was
being overly cautious.

As evidenced by:
commit 614193fac67445a7e92bf620ffef726ed1bd6f07
    conf: Fix check for chardev source path
An empty path should be caught by our validation.

>either explicitly by the user (most cases) or it would have been generated by
>us if the target type was either VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN or
>VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN.

VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN |
VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN is still
VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN. ;)

>I'm just simply wondering whether the
>condition cannot be further simplified to:
>
>if (virFileExists(foo) && virSecurityDACSetOwnership(...) < 0)

This introduces a change in behavior for mode='connect' - the DAC driver
will no longer report an error for a non-existent path.
I have not checked whether it's checked anywhere else (possibly the
SELinux driver), but it is a legitimate error and I don't see the need
to skip it.

Moreover, this hides the reason for using virFileExists in the first
place - it's only needed for type='listen' sockets.

Hope that answers your questions.
Looking forward to hearing from you.

Jano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20181001/970b619d/attachment-0001.sig>