-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
libcontainer: workaround for empty key label bug in RHEL7 #2070
Conversation
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: > 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 have this workaround here. This fixes non-working docker/containerd/runc on CentOS 7 (and, presumably, RHEL 7, too). Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This is an alternative to #2033 in case opencontainers/selinux#56 won't be accepted. |
/cc @rhatdan |
@@ -35,7 +35,10 @@ func (l *linuxSetnsInit) Init() error { | |||
|
|||
if !l.config.Config.NoNewKeyring { | |||
if err := label.SetKeyLabel(l.config.ProcessLabel); err != nil { | |||
return err | |||
// workaround for RHEL7 kernel which may return EACCES for an empty label | |||
if !(l.config.ProcessLabel == "" && os.IsPermission(err)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if l.config.ProcessLabel != "" || !os.IsPermission(err)
reads nicer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me your version is less readable (despite looking better and being equally correct). I read mine as "ignore the error unless label is empty and error is permission".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cyphar but I can certainly change it, please let me know
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really mind either way. I read mine as "this is an error if the label is not empty, or the error is not a permission-related one" but either works.
Just checked that this helps to avoid the issue. Still see the AVC but it no longer affects the operation.
|
ping @cyphar can you review this one again. I looked into this and I think its good to handle this at the runc level and let the selinux pkg just pass through errors. |
cc @opencontainers/runc-maintainers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (not a maintainer)
Again you are ignoring a legitimate denial of permissions, that could become serious in the future. The issue is to fix the SElinux policy not to ignore Permission Denied. |
If you want to allow this for now, you can simply
This will allow the domain to set the key label. Is runc running in an init script or is it started by Docker/Containerd. If the later then Docker/Containerd is labeled incorrectly. ls -lZ /usr/bin/docker* /usr/bin/containerd* /usr/bin/runc* |
@rhatdan on package install, should |
OK the reason for this was that
|
It is better to do a Recommends then a Requires, since some people might install contained.io inside of a container, and you probably don't want to pull selinux-policy into a container. |
Alas, RHEL7 has rpm-4.11 which does not understand |
Sorry I did not realize this was for RHEL7, you are correct. |
BTW @kolyshkin The SELinux labeling issue with KeyCreate turns out to be a thirteen year old bug in the kernel. Which just got merged into the SELinux kernel, and should be moving it's way into the upstream kernel, and eventually possibly back into RHEL7. |
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:
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:
So, this is presumably a kernel bug, but since there is no update
available for this kernel, we have to have this workaround here.
This fixes non-working docker/containerd/runc on CentOS 7 (and,
presumably, RHEL 7, too).
Signed-off-by: Kir Kolyshkin kolyshkin@gmail.com