[libvirt] [PATCH] security: dac: also label listen UNIX sockets
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@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) -- 2.16.4
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@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? Basically what I'm aiming at is that nix.path will always be set at this point, 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. I'm just simply wondering whether the condition cannot be further simplified to: if (virFileExists(foo) && virSecurityDACSetOwnership(...) < 0) goto done; ret = 0; break; Erik
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@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.
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. Hope that answers your questions. Looking forward to hearing from you. Jano
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@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@redhat.com>
Hope that answers your questions. Looking forward to hearing from you.
Jano
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 09/27/2018 05:02 PM, 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@redhat.com> --- src/security/security_dac.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
How come SELinux is not affected? We shouldn't rely on default policy doing the right thing. Michal
On Mon, Oct 01, 2018 at 10:34:38AM +0200, Michal Privoznik wrote:
On 09/27/2018 05:02 PM, 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@redhat.com> --- src/security/security_dac.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
How come SELinux is not affected? We shouldn't rely on default policy doing the right thing.
As mentioned in the commit message, the SELinux label is set before the socket creation since the commit mentioned in the commit message. Jano
participants (3)
-
Erik Skultety -
Ján Tomko -
Michal Privoznik