[PATCH] security_selinux.c: Relabel existing mode="bind" UNIX sockets

[David Michael]

On Tue, Jun 28, 2022 at 4:14 PM Martin Kletzander <mkletzan at redhat.com> wrote:
> On Tue, Jun 28, 2022 at 10:18:25AM -0400, David Michael wrote:
> >On Tue, Jun 28, 2022 at 10:03 AM Daniel P. Berrangé <berrange at redhat.com> wrote:
> >> On Tue, Jun 28, 2022 at 08:33:41AM -0400, David Michael wrote:
> >> > This supports sockets created by libvirt and passed by FD using the
> >> > same method as in security_dac.c.
> >> >
> >> > Signed-off-by: David Michael <david at bigbadwolfsecurity.com>
> >> > ---
> >> >
> >> > Hi,
> >> >
> >> > Custom SELinux labels are not applied to sockets when they have
> >> > mode="bind", but other security models (DAC) allow changing these
> >> > sockets.  Can the same method be used to support SELinux?
> >>
> >> This is rather intriguing. There must have been some compelling
> >> reason why we intentionally skipped listener sockets for SELinux
> >> labelling originally, but I'm struggling to recall what it could
> >> have been. Conceptually it makes sense to want to label the
> >> listener sockets with the per-VM label.
> >>
>
> Could it be that we only thought about the scenario of someone
> connecting to the socket (and the fact that it matters more what the
> actual socket label is rather than its file representation) but did not
> think about other possibilities, e.g. QEMU rewriting the socket (because
> we remove it before starting or any other reason) or some custom policy?

I checked the Git history for when DAC made the change, which has some
further links for context:

https://gitlab.com/libvirt/libvirt/-/commit/d6b8838dd83697f721fe0706068df765148154de

It looks like socket DAC relabeling was added shortly after libvirt
started creating the sockets to pass as FDs within the last few years,
but the related SELinux socket code hasn't been modified since it was
added a decade ago.  Seems like it just got left behind after that
change?

> >> How did you come across this issue ?  Is there a particular
> >> deployment/usage sceanrio where you're tripping up over this
> >> flaw ?
> >
> >Yes, setting custom MLS labels on the VMs (replacing the default sVirt
> >type and random categories) resulted in sockets being created with the
> >system policy's type and SystemLow.  So then there are denials for
> >QEMU and user processes running at specific MLS levels trying to work
> >with the sockets' unexpected type and level.  It's a customized
> >appliance-specific SELinux policy.
> >
> >> > Thanks.
> >> >
> >> > David
> >> >
> >> >  src/security/security_selinux.c            | 6 ++++--
> >> >  tests/securityselinuxlabeldata/chardev.txt | 2 +-
> >> >  2 files changed, 5 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> >> > index e2f34a27dc..8b258c9e36 100644
> >> > --- a/src/security/security_selinux.c
> >> > +++ b/src/security/security_selinux.c
> >> > @@ -2541,7 +2541,9 @@ virSecuritySELinuxSetChardevLabel(virSecurityManager *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))) {
> >> >              if (virSecuritySELinuxSetFilecon(mgr,
> >> >                                               dev_source->data.nix.path,
> >> >                                               imagelabel,
> >> > @@ -2618,7 +2620,7 @@ virSecuritySELinuxRestoreChardevLabel(virSecurityManager *mgr,
> >> >      case VIR_DOMAIN_CHR_TYPE_UNIX:
> >> >          if (!dev_source->data.nix.listen) {
> >> >              if (virSecuritySELinuxRestoreFileLabel(mgr,
> >> > -                                                   dev_source->data.file.path,
> >> > +                                                   dev_source->data.nix.path,
> >> >                                                     true) < 0)
> >> >                  goto done;
> >> >          }
> >> > diff --git a/tests/securityselinuxlabeldata/chardev.txt b/tests/securityselinuxlabeldata/chardev.txt
> >> > index 3f4b6302b9..bdb367f7a5 100644
> >> > --- a/tests/securityselinuxlabeldata/chardev.txt
> >> > +++ b/tests/securityselinuxlabeldata/chardev.txt
> >> > @@ -2,6 +2,6 @@
> >> >  /plain.dev;system_u:object_r:svirt_image_t:s0:c41,c264
> >> >  /plain.fifo;system_u:object_r:svirt_image_t:s0:c41,c264
> >> >  /nolabel.sock;
> >> > -/plain.sock;
> >> > +/plain.sock;system_u:object_r:svirt_image_t:s0:c41,c264
> >> >  /yeslabel.sock;system_u:object_r:svirt_image_t:s0:c41,c264
> >> >  /altlabel.sock;system_u:object_r:svirt_image_custom_t:s0:c41,c264
> >> > --
> >> > 2.36.1
> >> >
> >>
> >> With regards,
> >> Daniel
> >> --
> >> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> >> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> >> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> >