[v1] security: fix SELinux label generation logic

[libvirt PATCH] security: fix SELinux label generation logic

Posted by Daniel P. Berrangé 2 years, 9 months ago

A process can access a file if the set of MCS categories
for the file is equal-to *or* a subset-of, the set of
MCS categories for the process.

If there are two VMs:

  a) svirt_t:s0:c117
  b) svirt_t:s0:c117,c720

Then VM (b) is able to access files labelled for VM (a).

IOW, we must discard case where the categories are equal
because that is a subset of many other valid category pairs.

Fixes: https://gitlab.com/libvirt/libvirt/-/issues/153
CVE-2021-xxxx - tbd before pushing
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/security/security_selinux.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index b50f4463cc..c98dab0d6f 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -383,7 +383,15 @@ virSecuritySELinuxMCSFind(virSecurityManager *mgr,
         VIR_DEBUG("Try cat %s:c%d,c%d", sens, c1 + catMin, c2 + catMin);
 
         if (c1 == c2) {
-            mcs = g_strdup_printf("%s:c%d", sens, catMin + c1);
+            /*
+             * A process can access a file if the set of MCS categories
+             * for the file is equal-to *or* a subset-of, the set of
+             * MCS categories for the process.
+             *
+             * IOW, we must discard case where the categories are equal
+             * because that is a subset of other category pairs.
+             */
+            continue
         } else {
             if (c1 > c2) {
                 int t = c1;
-- 
2.31.1

Re: [libvirt PATCH] security: fix SELinux label generation logic

Posted by Peter Krempa 2 years, 9 months ago

On Wed, Jun 30, 2021 at 11:03:12 +0100, Daniel P. Berrangé wrote:
> A process can access a file if the set of MCS categories
> for the file is equal-to *or* a subset-of, the set of
> MCS categories for the process.
> 
> If there are two VMs:
> 
>   a) svirt_t:s0:c117
>   b) svirt_t:s0:c117,c720
> 
> Then VM (b) is able to access files labelled for VM (a).
> 
> IOW, we must discard case where the categories are equal
> because that is a subset of many other valid category pairs.
> 
> Fixes: https://gitlab.com/libvirt/libvirt/-/issues/153
> CVE-2021-xxxx - tbd before pushing
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  src/security/security_selinux.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index b50f4463cc..c98dab0d6f 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -383,7 +383,15 @@ virSecuritySELinuxMCSFind(virSecurityManager *mgr,
>          VIR_DEBUG("Try cat %s:c%d,c%d", sens, c1 + catMin, c2 + catMin);
>  
>          if (c1 == c2) {
> -            mcs = g_strdup_printf("%s:c%d", sens, catMin + c1);
> +            /*
> +             * A process can access a file if the set of MCS categories
> +             * for the file is equal-to *or* a subset-of, the set of
> +             * MCS categories for the process.
> +             *
> +             * IOW, we must discard case where the categories are equal
> +             * because that is a subset of other category pairs.
> +             */
> +            continue

Missing ';'

>          } else {
>              if (c1 > c2) {
>                  int t = c1;

This algorithm seems to be susceptible to infinite loops in case when
'catMin' and 'catMax' are too close or there's already enough of them
taken. Not a problem with this patch per-se, but it makes it more
likely.

Re: [libvirt PATCH] security: fix SELinux label generation logic

Posted by Daniel P. Berrangé 2 years, 9 months ago

On Wed, Jun 30, 2021 at 12:50:29PM +0200, Peter Krempa wrote:
> On Wed, Jun 30, 2021 at 11:03:12 +0100, Daniel P. Berrangé wrote:
> > A process can access a file if the set of MCS categories
> > for the file is equal-to *or* a subset-of, the set of
> > MCS categories for the process.
> > 
> > If there are two VMs:
> > 
> >   a) svirt_t:s0:c117
> >   b) svirt_t:s0:c117,c720
> > 
> > Then VM (b) is able to access files labelled for VM (a).
> > 
> > IOW, we must discard case where the categories are equal
> > because that is a subset of many other valid category pairs.
> > 
> > Fixes: https://gitlab.com/libvirt/libvirt/-/issues/153
> > CVE-2021-xxxx - tbd before pushing
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  src/security/security_selinux.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> > index b50f4463cc..c98dab0d6f 100644
> > --- a/src/security/security_selinux.c
> > +++ b/src/security/security_selinux.c
> > @@ -383,7 +383,15 @@ virSecuritySELinuxMCSFind(virSecurityManager *mgr,
> >          VIR_DEBUG("Try cat %s:c%d,c%d", sens, c1 + catMin, c2 + catMin);
> >  
> >          if (c1 == c2) {
> > -            mcs = g_strdup_printf("%s:c%d", sens, catMin + c1);
> > +            /*
> > +             * A process can access a file if the set of MCS categories
> > +             * for the file is equal-to *or* a subset-of, the set of
> > +             * MCS categories for the process.
> > +             *
> > +             * IOW, we must discard case where the categories are equal
> > +             * because that is a subset of other category pairs.
> > +             */
> > +            continue
> 
> Missing ';'
> 
> >          } else {
> >              if (c1 > c2) {
> >                  int t = c1;
> 
> This algorithm seems to be susceptible to infinite loops in case when
> 'catMin' and 'catMax' are too close or there's already enough of them
> taken. Not a problem with this patch per-se, but it makes it more
> likely.

Categories must be ordered, and we can't use matching categories, so
with the range 0->1023, we have something like (1024*1024/2)-1024
total unique pairs. aka 523264.

I'll be impressed if someone has enough VMs on a single host to use
more than 1% of that total space.

So you're right that its tehcnically an inifinite loop but in practice
we can ignore the problem (for now).

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

Re: [libvirt PATCH] security: fix SELinux label generation logic

Posted by Peter Krempa 2 years, 9 months ago

On Wed, Jun 30, 2021 at 12:04:27 +0100, Daniel P. Berrangé wrote:
> On Wed, Jun 30, 2021 at 12:50:29PM +0200, Peter Krempa wrote:
> > On Wed, Jun 30, 2021 at 11:03:12 +0100, Daniel P. Berrangé wrote:
> > > A process can access a file if the set of MCS categories
> > > for the file is equal-to *or* a subset-of, the set of
> > > MCS categories for the process.
> > > 
> > > If there are two VMs:
> > > 
> > >   a) svirt_t:s0:c117
> > >   b) svirt_t:s0:c117,c720
> > > 
> > > Then VM (b) is able to access files labelled for VM (a).
> > > 
> > > IOW, we must discard case where the categories are equal
> > > because that is a subset of many other valid category pairs.
> > > 
> > > Fixes: https://gitlab.com/libvirt/libvirt/-/issues/153
> > > CVE-2021-xxxx - tbd before pushing
> > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > ---
> > >  src/security/security_selinux.c | 10 +++++++++-
> > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> > > index b50f4463cc..c98dab0d6f 100644
> > > --- a/src/security/security_selinux.c
> > > +++ b/src/security/security_selinux.c
> > > @@ -383,7 +383,15 @@ virSecuritySELinuxMCSFind(virSecurityManager *mgr,
> > >          VIR_DEBUG("Try cat %s:c%d,c%d", sens, c1 + catMin, c2 + catMin);
> > >  
> > >          if (c1 == c2) {
> > > -            mcs = g_strdup_printf("%s:c%d", sens, catMin + c1);
> > > +            /*
> > > +             * A process can access a file if the set of MCS categories
> > > +             * for the file is equal-to *or* a subset-of, the set of
> > > +             * MCS categories for the process.
> > > +             *
> > > +             * IOW, we must discard case where the categories are equal
> > > +             * because that is a subset of other category pairs.
> > > +             */
> > > +            continue
> > 
> > Missing ';'
> > 
> > >          } else {
> > >              if (c1 > c2) {
> > >                  int t = c1;
> > 
> > This algorithm seems to be susceptible to infinite loops in case when
> > 'catMin' and 'catMax' are too close or there's already enough of them
> > taken. Not a problem with this patch per-se, but it makes it more
> > likely.
> 
> Categories must be ordered, and we can't use matching categories, so
> with the range 0->1023, we have something like (1024*1024/2)-1024
> total unique pairs. aka 523264.
> 
> I'll be impressed if someone has enough VMs on a single host to use
> more than 1% of that total space.
> 
> So you're right that its tehcnically an inifinite loop but in practice
> we can ignore the problem (for now).

Okay, I had to go back and read actually how the catMin/Max values are
obtained, and it's based on the process label that libvirtd gets. So I
guess users could shoot themselves in the foot when messing with the
label of libvirtd, but by default they get 0-1023.

I guess a failsafe in this regard would be to allow e.g. max 1000
iterations and then fail.

Either way that's for a different patch.

Reviewed-by: Peter Krempa <pkrempa@redhat.com>

once you add the semicolon.