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

[Erik Skultety]

On Mon, Oct 01, 2018 at 01:14:34PM +0200, Ján Tomko wrote:
> 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.

s/means/meaning that
...otherwise sounds meaningful...

>
>
> > 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.

Fair enough, let's got with your proposed change then (wait for after the
release though):
Reviewed-by: Erik Skultety <eskultet at redhat.com>

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



> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list