[libvirt] [PATCH] security_selinux: Don't relabel /dev/net/tun
https://bugzilla.redhat.com/show_bug.cgi?id=1147057 The code for relabelling the TAP FD is there due to a race. When libvirt creates a /dev/tapN device it's labeled as 'system_u:object_r:device_t:s0' by default. Later, when udev/systemd reacts to this device, it's relabelled to the expected label 'system_u:object_r:tun_tap_device_t:s0'. Hence, we have a code that relabels the device, to cut the race down. For more info see ae368ebfcc4. But the problem is, the relabel function is called on all TUN/TAP devices. Yes, on /dev/net/tun too. This is however a special kind of device - other processes uses it too. We shouldn't touch it's label then. Ideally, there would an API in SELinux that would label just the passed FD and not the underlying path. That way, we wouldn't need to care as we would be not labeling /dev/net/tun but the FD passed to the domain. Unfortunately, there's no such API so we have to workaround until then. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_selinux.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index b7c1015..25e8320 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2352,7 +2352,7 @@ virSecuritySELinuxSetTapFDLabel(virSecurityManagerPtr mgr, struct stat buf; security_context_t fcon = NULL; virSecurityLabelDefPtr secdef; - char *str = NULL; + char *str = NULL, *proc = NULL, *fd_path = NULL; int rc = -1; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); @@ -2370,6 +2370,23 @@ virSecuritySELinuxSetTapFDLabel(virSecurityManagerPtr mgr, goto cleanup; } + /* Label /dev/tap.* devices only. Leave /dev/net/tun alone! */ + if (virAsprintf(&proc, "/proc/self/fd/%d", fd) == -1) + goto cleanup; + + if (virFileResolveLink(proc, &fd_path) < 0) { + virReportSystemError(errno, + _("Unable to resolve link: %s"), proc); + goto cleanup; + } + + if (!STRPREFIX(fd_path, "/dev/tap")) { + VIR_DEBUG("fd=%d points to %s not setting SELinux label", + fd, fd_path); + rc = 0; + goto cleanup; + } + if (getContext(mgr, "/dev/tap.*", buf.st_mode, &fcon) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot lookup default selinux label for tap fd %d"), fd); @@ -2384,6 +2401,8 @@ virSecuritySELinuxSetTapFDLabel(virSecurityManagerPtr mgr, cleanup: freecon(fcon); + VIR_FREE(fd_path); + VIR_FREE(proc); VIR_FREE(str); return rc; } -- 2.0.4
On 10/07/2014 08:53 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1147057
The code for relabelling the TAP FD is there due to a race. When libvirt creates a /dev/tapN device it's labeled as 'system_u:object_r:device_t:s0' by default. Later, when udev/systemd reacts to this device, it's relabelled to the expected label 'system_u:object_r:tun_tap_device_t:s0'. Hence, we have a code that relabels the device, to cut the race down. For more info see ae368ebfcc4.
But the problem is, the relabel function is called on all TUN/TAP devices. Yes, on /dev/net/tun too. This is however a special kind of device - other processes uses it too. We shouldn't touch it's label then.
Ideally, there would an API in SELinux that would label just the passed FD and not the underlying path. That way, we wouldn't need to care as we would be not labeling /dev/net/tun but the FD passed to the domain. Unfortunately, there's no such API so we have to workaround until then.
+ + if (!STRPREFIX(fd_path, "/dev/tap")) {
Should this be "/dev/tap.", since...
+ VIR_DEBUG("fd=%d points to %s not setting SELinux label", + fd, fd_path); + rc = 0; + goto cleanup; + } + if (getContext(mgr, "/dev/tap.*", buf.st_mode, &fcon) < 0) {
...you require a '.' in the context lookup? Without the '.' in the filter, you would let the (unlikely) name '/dev/tapX' get through. ACK with that tweaked. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 07.10.2014 17:19, Eric Blake wrote:
On 10/07/2014 08:53 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1147057
The code for relabelling the TAP FD is there due to a race. When libvirt creates a /dev/tapN device it's labeled as 'system_u:object_r:device_t:s0' by default. Later, when udev/systemd reacts to this device, it's relabelled to the expected label 'system_u:object_r:tun_tap_device_t:s0'. Hence, we have a code that relabels the device, to cut the race down. For more info see ae368ebfcc4.
But the problem is, the relabel function is called on all TUN/TAP devices. Yes, on /dev/net/tun too. This is however a special kind of device - other processes uses it too. We shouldn't touch it's label then.
Ideally, there would an API in SELinux that would label just the passed FD and not the underlying path. That way, we wouldn't need to care as we would be not labeling /dev/net/tun but the FD passed to the domain. Unfortunately, there's no such API so we have to workaround until then.
+ + if (!STRPREFIX(fd_path, "/dev/tap")) {
Should this be "/dev/tap.", since...
+ VIR_DEBUG("fd=%d points to %s not setting SELinux label", + fd, fd_path); + rc = 0; + goto cleanup; + } + if (getContext(mgr, "/dev/tap.*", buf.st_mode, &fcon) < 0) {
...you require a '.' in the context lookup? Without the '.' in the filter, you would let the (unlikely) name '/dev/tapX' get through.
ACK with that tweaked.
In fact, /dev/tapX is what is created. getContext should be using it too as it accepts shell expendable names, not regular expressions. I'm adjusting getContext's argument too. Michal
On 10/08/2014 07:23 AM, Michal Privoznik wrote:
+ if (!STRPREFIX(fd_path, "/dev/tap")) {
Should this be "/dev/tap.", since...
+ VIR_DEBUG("fd=%d points to %s not setting SELinux label", + fd, fd_path); + rc = 0; + goto cleanup; + } + if (getContext(mgr, "/dev/tap.*", buf.st_mode, &fcon) < 0) {
...you require a '.' in the context lookup? Without the '.' in the filter, you would let the (unlikely) name '/dev/tapX' get through.
ACK with that tweaked.
In fact, /dev/tapX is what is created. getContext should be using it too as it accepts shell expendable names, not regular expressions. I'm adjusting getContext's argument too.
I assume "shell expendable" meant "glob" :) Oh wow - so you're saying the only reason this even worked is that getContext was getting lucky and realizing that any file that matches the stricter glob '/dev/tap.*' also matches the looser glob '/dev/tap*', and that we were lucky the context rule we were trying to look up was not written against a tighter glob such as '/dev/tap[0-9]*'. Yes, you've convinced me that the name really is /dev/tapX and that removing the spurious '.' in the call to getContext is correct. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Oct 07, 2014 at 04:53:24PM +0200, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1147057
The code for relabelling the TAP FD is there due to a race. When libvirt creates a /dev/tapN device it's labeled as 'system_u:object_r:device_t:s0' by default. Later, when udev/systemd reacts to this device, it's relabelled to the expected label 'system_u:object_r:tun_tap_device_t:s0'. Hence, we have a code that relabels the device, to cut the race down. For more info see ae368ebfcc4.
But the problem is, the relabel function is called on all TUN/TAP devices. Yes, on /dev/net/tun too. This is however a special kind of device - other processes uses it too. We shouldn't touch it's label then.
Ideally, there would an API in SELinux that would label just the passed FD and not the underlying path. That way, we wouldn't need to care as we would be not labeling /dev/net/tun but the FD passed to the domain. Unfortunately, there's no such API so we have to workaround until then.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_selinux.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index b7c1015..25e8320 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2352,7 +2352,7 @@ virSecuritySELinuxSetTapFDLabel(virSecurityManagerPtr mgr, struct stat buf; security_context_t fcon = NULL; virSecurityLabelDefPtr secdef; - char *str = NULL; + char *str = NULL, *proc = NULL, *fd_path = NULL; int rc = -1;
secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); @@ -2370,6 +2370,23 @@ virSecuritySELinuxSetTapFDLabel(virSecurityManagerPtr mgr, goto cleanup; }
+ /* Label /dev/tap.* devices only. Leave /dev/net/tun alone! */ + if (virAsprintf(&proc, "/proc/self/fd/%d", fd) == -1) + goto cleanup; + + if (virFileResolveLink(proc, &fd_path) < 0) { + virReportSystemError(errno, + _("Unable to resolve link: %s"), proc); + goto cleanup; + } + + if (!STRPREFIX(fd_path, "/dev/tap")) { + VIR_DEBUG("fd=%d points to %s not setting SELinux label", + fd, fd_path); + rc = 0; + goto cleanup; + } + if (getContext(mgr, "/dev/tap.*", buf.st_mode, &fcon) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot lookup default selinux label for tap fd %d"), fd); @@ -2384,6 +2401,8 @@ virSecuritySELinuxSetTapFDLabel(virSecurityManagerPtr mgr,
cleanup: freecon(fcon); + VIR_FREE(fd_path); + VIR_FREE(proc); VIR_FREE(str); return rc; }
I applied this patch (without any of Eric's suggested changes) to libvirt on Rawhide to see if it would fix the relabelling problems that are stopping libguestfs networking from working. It does indeed appear to fix them. Hence you can add: Tested-by: Richard W.M. Jones <rjones@redhat.com> Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
participants (3)
-
Eric Blake -
Michal Privoznik -
Richard W.M. Jones