go-selinux/SetKeyLabel: fix for RHEL7 kernels

[kolyshkin]

As it was pointed out earlier, on a freshly installed CentOS 7
system (kernel 3.10.0-957.12.2.el7.x86_64) with SELinux enabled,
the kernel does not allow to write an empty label to the keycreate
file. Here's an strace except from containerd (which uses runc
which uses go-selinux):

20396 openat(AT_FDCWD, "/proc/self/attr/keycreate", O_WRONLY|O_CLOEXEC) = 7
...
20396 write(7, "", 0) = -1 EACCES (Permission denied)

NOTE: upgrading or downgrading container-selinux makes this to go
away, so in order to reproduce you need a freshly booted system.

Here is a reproducer in C:

[root@kir-ce7-selinux-01 ~]# cat a.c
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <fcntl.h>
 #include <unistd.h>
 #include <stdio.h>

int main(void) {
	int fd, r;

	fd = open("/proc/self/attr/keycreate", O_WRONLY);
	if (fd < 0) {
		perror("open");
	}

	r = write(fd, "", 0);
	if (r < 0) {
		perror("write");
	}

	return 0;
}

[root@kir-ce7-selinux-01 ~]# uname -a
Linux kir-ce7-selinux-01 3.10.0-957.12.2.el7.x86_64 #1 SMP Tue May 14 21:24:32 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux

[root@kir-ce7-selinux-01 ~]# gcc -Wall -O2 -o a a.c

[root@kir-ce7-selinux-01 ~]# ./a
write: Permission denied
[root@kir-ce7-selinux-01 ~]# strace ./a
...
open("/proc/self/attr/keycreate", O_WRONLY) = 3
write(3, "", 0)                         = -1 EACCES (Permission denied)
...

So, this is presumably a kernel bug, but since there is no update
available for this kernel, we have to modify the earlier workaround
(see commit d75cf62) to NOT take into account whether
SELinux is enabled or not.

This fixes non-working docker/containerd/runc on CentOS 7 (and,
presumably, RHEL 7, too).

See also: #52, opencontainers/runc#2033

[kolyshkin]

All credit goes to @lifubang who tackled it back in April

@rhatdan @crosbymichael PTAL

[crosbymichael]

LGTM

[kolyshkin]

@vrothberg @mrunalp PTAL

[kolyshkin]

Filed a kernel bug to RHEL7: https://bugzilla.redhat.com/show_bug.cgi?id=1719067

[rhatdan]

If the Container SELinux package fixes this issue, why cover it up? I don't like ignoring the error? The policy should be updated to support the user setting the flag.

[kolyshkin]

If the Container SELinux package fixes this issue

It does not (and in case of this C reproducer it should not, as this binary is definitely not labeled to belong to container runtime etc.)

What triggers the issue go away (but not fix it!) are some operations with container-selinux package (upgrade or remove). I suspect there is something in %preun or %post changes the (kernel?) state and the bug is no longer reproducible.

In other words, I failed to establish the consistent pattern of "having container-selinux >= x.y installed" fixes the issue".

why cover it up? I don't like ignoring the error?

Yes, this is a workaround. It is here because it makes it possible to use runc under RHEL/CentOS 7 kernels. To my best knowledge, it does not introduce any regressions (as setting the label to "" is redundant), but it still tries to set it to reduce the changes to the code logic to the minimum.

The policy should be updated to support the user setting the flag.

As I have explained, it looks like this is not about the policy. I suspect this is a RHEL7 kernel bug, and I filed it (https://bugzilla.redhat.com/show_bug.cgi?id=1719067), but as there is no fix yet, we need a workaround at some level (either here, which this PR suggests, or in runc -- see opencontainers/runc#2070).

[kolyshkin]

OK, it most probably was a mislabeled binary, which I managed to overlook 😮
Apologies to everyone for wasting your time.

[rhatdan]

Actually @kolyshkin This pointed out an issue in container-selinux which I have moved to fix and have pushed a new version.

I really believe there is an issue in older RHEL7/Centos7 kernels that is causing this problem. The keyring fields, I believe, should not be labeled unlabeled_t, but should be labeled based on the process that is running. In this case it should be unconfined_service_t. This is why we have not seen this issue on newer kernels in the Fedora/RHEL8 world.

I don't see us fixing these kernels, since we don't really support container keyrings yet.

[kolyshkin]

OK, I have revisited this one and it looks like it still makes sense to have it, if we want runc to work for RHEL7 kernels. Please see all the gory details here: containerd/containerd#3862 (comment)

@rhatdan @mrunalp PTAL

[rhatdan]

/approve
I guess I am fine with this.

[mrunalp]

LGTM

[kolyshkin]

@rhatdan can we please have a tagged release so we can update runc vendording?