Older kernels do not support keyring labeling #51

rhatdan · 2019-04-04T13:52:41Z

Ignore setting the kernel keyring label if the
/proc/self/attr/keycreate file does not exists.

runc attempts to set this file on older kernels and is blowing up.

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

rhatdan · 2019-04-04T13:54:01Z

cyphar · 2019-04-04T14:49:18Z

This is similar to what I suggested in #49, though I do wonder why not just do it like this:

diff --git a/go-selinux/selinux_linux.go b/go-selinux/selinux_linux.go
index 51fa8de68a33..45e47f5eda00 100644
--- a/go-selinux/selinux_linux.go
+++ b/go-selinux/selinux_linux.go
@@ -341,6 +341,10 @@ func writeCon(fpath string, val string) error {
 
        out, err := os.OpenFile(fpath, os.O_WRONLY, 0)
        if err != nil {
+               // Non-existent /proc files indicate a vintage kernel.
+               if os.IsNotExist(err) {
+                       err = nil
+               }
                return err
        }
        defer out.Close()

So any new writeCon users in the future will also handle old kernels.

In any case, LGTM.

lifubang · 2019-04-04T15:25:54Z

@cyphar @rhatdan I think this PR can't fix my issue.
I spent some time dig into it, and with @rhatdan 's help, find the real reason.

My English is not very well, please see code in #52 .

rhatdan · 2019-04-04T15:52:19Z

@cyphar I think the problem I have with this is it would be easy to mistake a success versus a typo in the code.
Which would not be caught until far later. We know of a specific case of keycreate, so we handle it.

rhatdan · 2019-04-04T15:53:00Z

@lifubang Could you show me the failure you are seeing with permission denied? I would always want that feedback.

rhatdan · 2019-04-04T17:10:41Z

@lifubang I updated this PR to handle your issue.

Ignore setting the kernel keyring label if the /proc/self/attr/keycreate file does not exists. runc attempts to set this file on older kernels and is blowing up. If a system has a /proc/self/attr/keyring and the caller attempts to set the label to "" and SELinux is disabled, ignore the error. Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

lifubang · 2019-04-05T04:16:49Z

@lifubang I updated this PR to handle your issue.

No, this can't fix my issue. Please see #52 .
If have any suggestions, please input in #52 . I will debug it. Thanks.

rhatdan · 2019-04-05T15:08:45Z

What error does this patch miss? Your patch misses Permission Denied.

lifubang · 2019-04-05T15:28:16Z

What error does this patch miss? Your patch misses Permission Denied.

I think we don't need it any more.

lifubang · 2019-04-05T15:56:59Z

go-selinux/selinux_linux.go

+	if os.IsNotExist(err) {
+		return nil
+	}
+	if label == "" && os.IsPermission(err) && !GetEnabled() {


Once we really have no permission to keycreate?
So I don't think we should use this err check.

This will check if someone executes runc on a machine with /proc/self/attr/keycreate, attempted to set "", got permission denied, and SELinux was disabled, so we ignore the error.

Oh, not SELinux disabled, but SELinux enabled.

lifubang · 2019-04-05T16:00:09Z

What error does this patch miss?

I can't explain it in one sentence because of my English's level.
Please see #52 (comment) to get full understand.

rhatdan · 2019-04-05T16:02:37Z

@lifubang Lets stick to my PR. Rather then switching back and forth.
This misses the case where the user is allowed to read keycreate but is not allowed to write to keycreate, and is writing "" with a disabled SELinux system.

lifubang · 2019-04-05T16:12:33Z

I think I need to debug it to ensure it is really right, so I will work on in my PR #52 .
If I don't explain clearly, you can see my code.

rhatdan · 2019-04-19T09:08:41Z

@cpuguy83 Does this fix the issue you are seeing. I will merge and open a vendor for runc if it does. I am not in favor of the other patch.

cpuguy83

Looks like it would. I will give it a try later today.
Thanks!

cpuguy83 · 2019-04-19T15:33:06Z

Ok, so the issue is GetEnabled() is is true when runc is compiled with selinux support.

rhatdan · 2019-04-20T10:08:49Z

@cpuguy83 Is this on an SELinux system? Is SELinux enabled?

cyphar · 2019-04-20T15:22:42Z

VERSION

@@ -1 +1 @@
-1.3.0-dev
+1.2.2


Did you mean to commit this?

Yes I am only bumping the minor number, Since this is only a bugfix.

Sure but usually releases in OCI go through a voting process and so on. Bit odd to include it in an ordinary PR.

I have not been following any such procedure on the SELinux package. I guess I could, but have not in the past. Bottom line, I would like to get this PR Verified as fixing the issue and then vendored into runc, so we can fix it there.

Sure, do whatever works for you.

The release procedures for all opencontainers projects (except I guess selinux) have all used the voting system we use for spec releases, but it's pretty obvious that's not a great model for non-spec projects. I'm hoping to correct this after we get 1.0.0 out.

cpuguy83 · 2019-04-20T23:02:46Z

@rhatdan

Is this on an SELinux system? Is SELinux enabled?

Yes and yes.

rhatdan · 2019-04-21T10:29:23Z

@cpuguy83 And did a patched version of runc with this patch fix the issue. I take it this is using an older RHEL/Centos kernel that did not support keycreate labels?

cpuguy83 · 2019-04-22T16:58:56Z

@rhatdan I'm using CentOS on Azure (kernel 3.10.0-862.11.6.el7.x86_64).
Unpatched this works fine if I set --selinux-enabled=true on docker, but otherwise (--selinux-enabled=false, the default) I get EPERM even with this patch.

rhatdan · 2019-04-22T17:49:00Z

Are you seeing AVCs?
Does it work with setenforce 0.

cpuguy83 · 2019-04-22T17:59:48Z

Denial is:

type=PROCTITLE msg=audit(1555611120.741:866): proctitle=2F7573722F62696E2F72756E6300696E6974
type=SYSCALL msg=audit(1555611120.741:866): arch=c000003e syscall=1 success=no exit=-13 a0=8 a1=557a6f0ab780 a2=0 a3=0 items=0 ppid=10782 pid=10790 auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=4294967295 comm="runc:[2:INIT]" exe="/" subj=system_u:system_r:container_runtime_t:s0 key=(null)
type=AVC msg=audit(1555611120.741:866): avc:  denied  { create } for  pid=10790 comm="runc:[2:INIT]" scontext=system_u:system_r:container_runtime_t:s0 tcontext=system_u:object_r:unlabeled_t:s0 tclass=key

With setenforce 0 there is no issue.

rhatdan · 2019-04-22T19:58:58Z

This is a container-selinux issue. The latest container-selinux-2.91 and greater allows this AVC. So give this a LGTM and we can move on.
Of course I would like to know what SELinux label is being put into the process label of a container when SELinux is disabled in docker. It should be "", but this patch would catch that. So I am thinking you are sending some other label. Can you grab the runtime spec json file and see what the process label is?

@mrunalp @runcom PTAL at this PR, I believe it is correct and @cpuguy83 is experiencing an different issue, that is fixed in container-selinux package.

mrunalp · 2019-04-23T19:14:34Z

LGTM

rhatdan · 2019-04-23T19:55:11Z

@cpuguy83 I just fixed another issue in container-selinux to allow spc_t to write unlabeled_t files.
We are packaging up the fix in container-selinux-2.100

cyphar · 2019-04-24T05:48:49Z

Can we get a tagged release so we can bump runc? 💚

rhatdan · 2019-04-24T13:03:02Z

opencontainers/runc#2043

full diff: opencontainers/selinux@v1.2.1...v1.2.2 - opencontainers/selinux#51 Older kernels do not support keyring labeling Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

full diff: opencontainers/selinux@v1.2.1...v1.2.2 - opencontainers/selinux#51 Older kernels do not support keyring labeling Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit e5aab17) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

full diff: opencontainers/selinux@v1.2.1...v1.2.2 - opencontainers/selinux#51 Older kernels do not support keyring labeling Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit e5aab17) Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit 05c3be6) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

full diff: opencontainers/selinux@v1.2.1...v1.2.2 - opencontainers/selinux#51 Older kernels do not support keyring labeling Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Upstream-commit: 0d453115fe0b1b19c08c614b6029c4edf92a0f0a Component: engine

full diff: opencontainers/selinux@v1.2.1...v1.2.2 - opencontainers/selinux#51 Older kernels do not support keyring labeling Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit 0d45311) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

full diff: opencontainers/selinux@v1.2.1...v1.2.2 - opencontainers/selinux#51 Older kernels do not support keyring labeling Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit 0d453115fe0b1b19c08c614b6029c4edf92a0f0a) Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Upstream-commit: e7a837120de1f4f2d45c673e758bd444441a0c8f Component: engine

full diff: opencontainers/selinux@v1.2.1...v1.2.2 - opencontainers/selinux#51 Older kernels do not support keyring labeling Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

rhatdan mentioned this pull request Apr 4, 2019

fix can't use empty str as label in some old kernels #50

Closed

rhatdan force-pushed the keyring branch from 856bf76 to 78b0b02 Compare April 4, 2019 17:09

rhatdan force-pushed the keyring branch from 78b0b02 to 3a1f366 Compare April 4, 2019 17:24

lifubang reviewed Apr 5, 2019

View reviewed changes

cpuguy83 reviewed Apr 19, 2019

View reviewed changes

cyphar reviewed Apr 20, 2019

View reviewed changes

cyphar mentioned this pull request Apr 22, 2019

Question: runc 1.0.0 release plan opencontainers/runc#1905

Closed

mrunalp merged commit bed8545 into opencontainers:master Apr 23, 2019

cpuguy83 mentioned this pull request Apr 23, 2019

[18.09 backport] Bump containerd v1.2.6, runc v1.0.0-rc8 docker-archive/engine#194

Merged

thaJeztah mentioned this pull request Apr 26, 2019

Bump runc 1.0.0-rc8, opencontainers/selinux v1.2.2 containerd/containerd#3240

Merged

thaJeztah mentioned this pull request Apr 26, 2019

Bump runc 1.0.0-rc8, opencontainers/selinux v1.2.2 moby/moby#39143

Merged

thaJeztah mentioned this pull request Apr 28, 2019

[release/1.2 backport] Bump runc 1.0.0-rc8, opencontainers/selinux v1.2.2 containerd/containerd#3247

Merged

thaJeztah mentioned this pull request Apr 28, 2019

[release/1.1 backport] Bump runc 1.0.0-rc8, opencontainers/selinux v1.2.2 containerd/containerd#3248

Merged

thaJeztah mentioned this pull request Jun 5, 2019

runc regression - EPERM running containers from selinux moby/moby#39109

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Older kernels do not support keyring labeling #51

Older kernels do not support keyring labeling #51

rhatdan commented Apr 4, 2019

rhatdan commented Apr 4, 2019

cyphar commented Apr 4, 2019 •

edited

Loading

lifubang commented Apr 4, 2019

rhatdan commented Apr 4, 2019

rhatdan commented Apr 4, 2019

rhatdan commented Apr 4, 2019

lifubang commented Apr 5, 2019

rhatdan commented Apr 5, 2019

lifubang commented Apr 5, 2019

lifubang Apr 5, 2019

rhatdan Apr 5, 2019

lifubang Apr 5, 2019

lifubang commented Apr 5, 2019 •

edited

Loading

rhatdan commented Apr 5, 2019

lifubang commented Apr 5, 2019

rhatdan commented Apr 19, 2019

cpuguy83 left a comment

cpuguy83 commented Apr 19, 2019

rhatdan commented Apr 20, 2019

cyphar Apr 20, 2019

rhatdan Apr 20, 2019

cyphar Apr 21, 2019

rhatdan Apr 22, 2019

cyphar Apr 22, 2019 •

edited

Loading

cpuguy83 commented Apr 20, 2019

rhatdan commented Apr 21, 2019

cpuguy83 commented Apr 22, 2019 •

edited

Loading

rhatdan commented Apr 22, 2019

cpuguy83 commented Apr 22, 2019

rhatdan commented Apr 22, 2019

mrunalp commented Apr 23, 2019 •

edited by caniszczyk

Loading

rhatdan commented Apr 23, 2019

cyphar commented Apr 24, 2019

rhatdan commented Apr 24, 2019

		@@ -1 +1 @@
		1.3.0-dev
		1.2.2

Older kernels do not support keyring labeling #51

Older kernels do not support keyring labeling #51

Conversation

rhatdan commented Apr 4, 2019

rhatdan commented Apr 4, 2019

cyphar commented Apr 4, 2019 • edited Loading

lifubang commented Apr 4, 2019

rhatdan commented Apr 4, 2019

rhatdan commented Apr 4, 2019

rhatdan commented Apr 4, 2019

lifubang commented Apr 5, 2019

rhatdan commented Apr 5, 2019

lifubang commented Apr 5, 2019

lifubang Apr 5, 2019

Choose a reason for hiding this comment

rhatdan Apr 5, 2019

Choose a reason for hiding this comment

lifubang Apr 5, 2019

Choose a reason for hiding this comment

lifubang commented Apr 5, 2019 • edited Loading

rhatdan commented Apr 5, 2019

lifubang commented Apr 5, 2019

rhatdan commented Apr 19, 2019

cpuguy83 left a comment

Choose a reason for hiding this comment

cpuguy83 commented Apr 19, 2019

rhatdan commented Apr 20, 2019

cyphar Apr 20, 2019

Choose a reason for hiding this comment

rhatdan Apr 20, 2019

Choose a reason for hiding this comment

cyphar Apr 21, 2019

Choose a reason for hiding this comment

rhatdan Apr 22, 2019

Choose a reason for hiding this comment

cyphar Apr 22, 2019 • edited Loading

Choose a reason for hiding this comment

cpuguy83 commented Apr 20, 2019

rhatdan commented Apr 21, 2019

cpuguy83 commented Apr 22, 2019 • edited Loading

rhatdan commented Apr 22, 2019

cpuguy83 commented Apr 22, 2019

rhatdan commented Apr 22, 2019

mrunalp commented Apr 23, 2019 • edited by caniszczyk Loading

rhatdan commented Apr 23, 2019

cyphar commented Apr 24, 2019

rhatdan commented Apr 24, 2019

cyphar commented Apr 4, 2019 •

edited

Loading

lifubang commented Apr 5, 2019 •

edited

Loading

cyphar Apr 22, 2019 •

edited

Loading

cpuguy83 commented Apr 22, 2019 •

edited

Loading

mrunalp commented Apr 23, 2019 •

edited by caniszczyk

Loading