[libvirt PATCH] security: fix SELinux label generation logic
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
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.
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 :|
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.
participants (2)
-
Daniel P. Berrangé -
Peter Krempa