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

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(a)redhat.com&gt; > > --- > > 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

-- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list