[PATCH] security_selinux.c: Relabel existing mode="bind" UNIX sockets
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@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? 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
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@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. How did you come across this issue ? Is there a particular deployment/usage sceanrio where you're tripping up over this flaw ?
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 :|
On Tue, Jun 28, 2022 at 10:03 AM Daniel P. Berrangé <berrange@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@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.
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 :|
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@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@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?
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 :|
On Tue, Jun 28, 2022 at 4:14 PM Martin Kletzander <mkletzan@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@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@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/d6b8838dd83697f721fe0706068df765... 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 :|
On Tue, Jun 28, 2022 at 05:00:06PM -0400, David Michael wrote:
On Tue, Jun 28, 2022 at 4:14 PM Martin Kletzander <mkletzan@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@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@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/d6b8838dd83697f721fe0706068df765...
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?
Yeah that looks like the cause. Originally when QEMU invoked the bind() + listen(), it would inherit labelling, so we wouldn't need todo anything special ourselves. When we switched to creating it ourselves & passing the FD we broke that implicit behaviour we relied on. 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 :|
On 6/28/22 14:33, 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@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?
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
@@ -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; }
Regardless of the fate of the rest of the patch, this hunk is a bug fix and thus should be merged. It's just a coincidence that data.file.path maps onto data.nix.path in the union. Michal
On 6/28/22 14:33, 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@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?
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))) {
I've copied the comment from corresponding _dac.c function, so that it's obvious why we are relabelling in this case too.
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
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> and pushed. Congratulations on your first libvirt contribution! Michal
participants (4)
-
Daniel P. Berrangé -
David Michael -
Martin Kletzander -
Michal Prívozník