Skip to content

Conversation

@rhatdan
Copy link
Collaborator

@rhatdan rhatdan commented Apr 1, 2019

Some users of go-selinux attempt to set the SELinux label to ""
even on SELinux disabled systems. This can cause these apps to blow
up (runc). Rather then complicated these tools API, we can just ignore
the attempts to set "" labels.

If the caller attempts to set a label != "", then we should continue to
attempt and fail appropriately.

Signed-off-by: Daniel J Walsh dwalsh@redhat.com

Some users of go-selinux attempt to set the SELinux label to ""
even on SELinux disabled systems. This can cause these apps to blow
up (runc).  Rather then complicated these tools API, we can just ignore
the attempts to set "" labels.

If the caller attempts to set a label != "", then we should continue to
attempt and fail appropriately.

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
@rhatdan
Copy link
Collaborator Author

rhatdan commented Apr 1, 2019

@cyphar @mrunalp @vrothberg @runcom PTAL

@cyphar
Copy link
Member

cyphar commented Apr 1, 2019

LGTM -- though there should also be a check for err == ENOENT on os.OpenFile which we should also ignore in order to handle older kernels. The current code will fail on older SELinux-enabled kernels where /proc/self/attr/keycreate is not present.

@rhatdan
Copy link
Collaborator Author

rhatdan commented Apr 1, 2019

@cyphar What OS does not support this? RHEL7/Centos7 or something older?

@cyphar
Copy link
Member

cyphar commented Apr 2, 2019

When writing my comment I didn't realise that keycreate was added in 2008 -- I assumed it was much newer (and we've been seeing ENOENT errors from Ubuntu -- which is the actual cause of this issue).

So having backwards compatibility support probably makes little sense for decade-old kernels -- however several users clearly are using decade-old kernels (hence the reports of runc being broken). Having an ENOENT check (which will never actually be hit) would help avoid those error reports, but it's ultimately up to you as to whether that's worth it.

@rhatdan
Copy link
Collaborator Author

rhatdan commented Apr 2, 2019

@cyphar I think this PR is a better check, since if those callers are actually pushing in actual labels on Ubuntu, then they should be informed, if this is just pushing "" can causing the issue then this would fix it.

@giuseppe
Copy link
Member

giuseppe commented Apr 2, 2019

LGTM

1 similar comment
@vrothberg
Copy link
Collaborator

LGTM

@mrunalp
Copy link
Contributor

mrunalp commented Apr 2, 2019

LGTM

Approved with PullApprove

@mrunalp mrunalp merged commit 5ba962c into opencontainers:master Apr 2, 2019
@cyphar
Copy link
Member

cyphar commented Apr 3, 2019

@rhatdan I would've suggested doing both checks, but if you're okay with failures on (very) old SELinux systems that's fine.

thaJeztah added a commit to thaJeztah/docker that referenced this pull request Apr 3, 2019
full diff: opencontainers/selinux@v1.2...v1.2.1

brings in opencontainers/selinux#49 Ignore attempts to setLabels "" on SELinux disabled systems

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah added a commit to thaJeztah/cri that referenced this pull request Apr 3, 2019
full diff: opencontainers/selinux@v1.2...v1.2.1

brings in opencontainers/selinux#49 Ignore attempts to setLabels "" on SELinux disabled systems

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah added a commit to thaJeztah/cri that referenced this pull request Apr 4, 2019
full diff: opencontainers/selinux@v1.2...v1.2.1

brings in opencontainers/selinux#49 Ignore attempts to setLabels "" on SELinux disabled systems

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah added a commit to thaJeztah/cri that referenced this pull request Apr 4, 2019
full diff: opencontainers/selinux@v1.2...v1.2.1

brings in opencontainers/selinux#49 Ignore attempts to setLabels "" on SELinux disabled systems

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit e5d8491)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah added a commit to thaJeztah/cri that referenced this pull request Apr 4, 2019
full diff: opencontainers/selinux@v1.2...v1.2.1

brings in opencontainers/selinux#49 Ignore attempts to setLabels "" on SELinux disabled systems

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah added a commit to thaJeztah/cri that referenced this pull request Apr 4, 2019
full diff: opencontainers/selinux@v1.2...v1.2.1

brings in opencontainers/selinux#49 Ignore attempts to setLabels "" on SELinux disabled systems

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 5e7d59f)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah added a commit to thaJeztah/cri that referenced this pull request Apr 4, 2019
full diff: opencontainers/selinux@v1.2...v1.2.1

brings in opencontainers/selinux#49 Ignore attempts to setLabels "" on SELinux disabled systems

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 5e7d59f)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this pull request Apr 4, 2019
full diff: opencontainers/selinux@v1.2...v1.2.1

brings in opencontainers/selinux#49 Ignore attempts to setLabels "" on SELinux disabled systems

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Upstream-commit: 606ac478adb74754597074f5377f4341bc5ea369
Component: engine
adi-dhulipala pushed a commit to adi-dhulipala/docker that referenced this pull request Apr 11, 2019
full diff: opencontainers/selinux@v1.2...v1.2.1

brings in opencontainers/selinux#49 Ignore attempts to setLabels "" on SELinux disabled systems

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@kolyshkin
Copy link
Collaborator

Some users of go-selinux attempt to set the SELinux label to "" even on SELinux disabled systems.

This is somewhat incorrect description. What I am seeing on a SELinux-enabled system is runc fails like this:

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

This is CentOS 7, latest kernel (3.10.0-957.12.2.el7.x86_64) as of now.

@rhatdan
Copy link
Collaborator Author

rhatdan commented Jun 10, 2019

You need an updated container-selinux most likely.

What AVC are you seeing?

@kolyshkin
Copy link
Collaborator

kolyshkin commented Jun 11, 2019

You need an updated container-selinux most likely.

@rhatdan took me the best part of my day to figure it out, but upgrading (or downgrading) container-selinux makes the bug disappear, until the next reboot (allegedly something in %preun or %post rpm scripts makes a difference, but alas I was not able to figure out what it is exactly).

I was also able to reproduce this with a small C proggie, on which container-selinux has no effect. Here's what I see:

[root@kir-centos-selinux-2 ~]# uname -a
Linux kir-centos-selinux-2 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-centos-selinux-2 ~]# 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-centos-selinux-2 ~]# gcc -Wall -o a a.c
[root@kir-centos-selinux-2 ~]# ./a
write: Permission denied
[root@kir-centos-selinux-2 ~]# ausearch -m avc -ts recent
----
time->Tue Jun 11 01:28:24 2019
type=PROCTITLE msg=audit(1560216504.270:90): proctitle="./a"
type=SYSCALL msg=audit(1560216504.270:90): arch=c000003e syscall=1 success=no exit=-13 a0=3 a1=4006bf a2=0 a3=7fff75ff05a0 items=0 ppid=3113 pid=3188 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=1 comm="a" exe="/root/a" subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)
type=AVC msg=audit(1560216504.270:90): avc:  denied  { create } for  pid=3188 comm="a" scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tcontext=system_u:object_r:unlabeled_t:s0 tclass=key permissive=0
[root@kir-centos-selinux-2 ~]# 

Now, installation of container-selinux fixes this (although it should definitely not afffect it)!

[root@kir-centos-selinux-2 ~]# rpm -q container-selinux
package container-selinux is not installed
[root@kir-centos-selinux-2 ~]# yum install container-selinux
...
Installed:
  container-selinux.noarch 2:2.95-2.el7_6                                                                                                                                                                                                     

Complete!
[root@kir-centos-selinux-2 ~]# ls -Z ./a
-rwxr-xr-x. root root unconfined_u:object_r:admin_home_t:s0 ./a
[root@kir-centos-selinux-2 ~]# ./a
[root@kir-centos-selinux-2 ~]# echo $?
0

and once this bug disappears, it won't reappear until reboot, and sometimes it's not reappearing after a reboot.

@rhatdan
Copy link
Collaborator Author

rhatdan commented Jun 11, 2019

Grab the latest RHEL7.5 branch of container-selinux and build and install the policy. This should allow the access.

thaJeztah added a commit to thaJeztah/docker that referenced this pull request Aug 27, 2019
full diff: opencontainers/selinux@v1.2...v1.2.1

brings in opencontainers/selinux#49 Ignore attempts to setLabels "" on SELinux disabled systems

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 606ac47)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants