Skip to content
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

Add selinux validate in runc exec #2031

Merged
merged 1 commit into from Apr 3, 2019

Conversation

Projects
None yet
5 participants
@lifubang
Copy link
Contributor

lifubang commented Mar 29, 2019

Signed-off-by: Lifubang lifubang@acmcoder.com

  1. Fix regression introduced by cd96170 , the related commit is in opencontainers/selinux@e5c68ba#diff-08374585d1f5b66358d612f6292a3fae
    There is no if processLabel == "" { check now.
    Fixed by #2032

  2. Fixes #2030 check nil for selinuxLabel before we label.SetKeyLabel, this is also introduced by cd96170 ;
    Because the validate is :

    func (v *ConfigValidator) security(config *configs.Config) error {
    // restrict sys without mount namespace
    if (len(config.MaskPaths) > 0 || len(config.ReadonlyPaths) > 0) &&
    !config.Namespaces.Contains(configs.NEWNS) {
    return fmt.Errorf("unable to restrict sys entries without a private MNT namespace")
    }
    if config.ProcessLabel != "" && !selinux.GetEnabled() {
    return fmt.Errorf("selinux label is specified in config, but selinux is disabled or not supported")
    }
    return nil
    }

    Fixed by #2032

  3. Fix if we use selinuxLabel in runc exec, we should validate selinux again. If the system disable selinux, it will raise error exec failed: container_linux.go:345: starting container process caused "write /proc/self/attr/keycreate: invalid argument". For example:
    (1) use '--process-label' in runc exec;
    (2) use selinuxLabel in process.json when use --process in runc exec;
    (3) add selinuxLabel to config.json in bundle dir after the container started.

@thaJeztah

This comment has been minimized.

Copy link

thaJeztah commented Mar 30, 2019

ping @rhatdan ptal

// If config.json is modified after the container started, we should validate selinux again.
if l.config.ProcessLabel != "" && !selinux.GetEnabled() {
return fmt.Errorf("selinux label is specified in config, but selinux is disabled or not supported")
}

This comment has been minimized.

Copy link
@cyphar

cyphar Mar 30, 2019

Member

From memory, the config.json is not re-parsed for runc exec -- we use the config that's in /run/runc/$ctr/... which means that there's no chance it could change between runs (other than runc update but that only really does cgroup changes).

This comment has been minimized.

Copy link
@lifubang

lifubang Mar 30, 2019

Author Contributor

From memory, the config.json is not re-parsed for runc exec

No, it re-parsed in here:

runc/exec.go

Line 170 in 84cba4c

spec, err := loadSpec(specConfig)

This comment has been minimized.

Copy link
@lifubang

lifubang Mar 30, 2019

Author Contributor

@cyphar Besides re-parse config.json in bundle dir, there are 3 situations use selinux in runc exec, I have update the PR description.
(1) use '--process-label' in runc exec;
(2) add selinuxLabel in process.json when use --process in runc exec;
(3) add selinuxLabel to config.json in bundle dir after the container started.

This comment has been minimized.

Copy link
@lifubang

lifubang Mar 31, 2019

Author Contributor

I move the selinux validate to utils_linux.go when exec, so we can do this check before we start runc init. Does it make sense?

This comment has been minimized.

Copy link
@cyphar

cyphar Mar 31, 2019

Member

Yes, it would be much better to do it a lot earlier.

if err := label.SetKeyLabel(l.config.ProcessLabel); err != nil {
return err
if l.config.ProcessLabel != "" {
if err := label.SetKeyLabel(l.config.ProcessLabel); err != nil {

This comment has been minimized.

Copy link
@cyphar

cyphar Mar 30, 2019

Member

@rhatdan I think that all of the Set*Label commands should be a no-op if l.config.ProcessLabel is "" -- so as to avoid these sorts of problems.

This comment has been minimized.

Copy link
@lifubang

lifubang Mar 30, 2019

Author Contributor

Yes, it does make sense for don't check nil in definition of label.Set*Label, because we maybe need to delete the process label, so we should check l.config.ProcessLabel is "" for all the Set*Label call in runc.

@cyphar

This comment has been minimized.

Copy link
Member

cyphar commented Mar 30, 2019

Yeah, I didn't catch this when I reviewed #2012. Looks okay, minus my comments. We really should've fixed this for rc7... Guess it's time for an rc8 then...

@rhatdan

This comment has been minimized.

Copy link
Contributor

rhatdan commented Mar 30, 2019

Yes, I think we need to check this in Runc. What I don't understand is why this is blowing up, while the other calls do not.

But no SELinux calls should be made if the label is ""

@rhatdan

This comment has been minimized.

Copy link
Contributor

rhatdan commented Mar 30, 2019

LGTM

@lifubang lifubang force-pushed the lifubang:selinux branch 2 times, most recently from 8d32762 to dda6158 Mar 31, 2019

@cyphar

This comment has been minimized.

Copy link
Member

cyphar commented Mar 31, 2019

What I don't understand is why this is blowing up, while the other calls do not.

I've figured it out -- it's because of older kernels and so this fix actually isn't sufficient (if you're using an older SELinux machine). I'm running AppArmor on a 5.0.3 kernel and I have a /proc/self/attr/keycreate file which will happily accept "" as an argument (even though it's wrong for SELinux code to be touching it on an AppArmor machine).

So, either runc or go-selinux need to detect whether /proc/self/attr/keycreate is present before trying anything (and maybe this should be the case for all writeCon cases). I would argue this would both be cleaner and make more sense in go-selinux than runc -- because then we'll have to be aware of SELinux internals.

And if we're going to do that sort of check, I think it would also make sense to no-op go-selinux calls where the label is "" -- unless "" actually makes sense as an SELinux label?

@cyphar

This comment has been minimized.

Copy link
Member

cyphar commented Mar 31, 2019

I will submit a PR to go-selinux along the above lines, and then we can update this PR to re-vendor it and then remove some of the changes (but the "is SELinux enabled" for runc exec change should stay).

@rhatdan

This comment has been minimized.

Copy link
Contributor

rhatdan commented Mar 31, 2019

"" actuall makes sense as an SELinux label. This tells go-selinux to turn back on the default labeling.

label.SetProcessLabel("system_u:system_r:container_t:s0:c1,c2)
defer label.SetProcessLabel("")

@cyphar

This comment has been minimized.

Copy link
Member

cyphar commented Mar 31, 2019

Hmm, so that means really runc should be gating all Set.*Label calls with selinux.IsEnabled (or the standard Label != "")? We do this already, but not nearly as explicitly as it appears we should be -- because it's a bit weird for a SELinux library to write to /proc/self/attr even though SELinux isn't in use. It would end up with weird results for AppArmor profiles (for instance).

@rhatdan

This comment has been minimized.

Copy link
Contributor

rhatdan commented Mar 31, 2019

We could embed the isselinuxenabled check in the label calls, and only check it if the "" is passed.

@cyphar

This comment has been minimized.

Copy link
Member

cyphar commented Mar 31, 2019

I will send a PR with these changes (the is-selinux-enabled checks and the backwards-compatibility changes) tomorrow.

@lifubang

This comment has been minimized.

Copy link
Contributor Author

lifubang commented Apr 2, 2019

Dear experts, after I see opencontainers/selinux#49 , do we really need patch 49 in runc? Because consider:

label.SetProcessLabel(labelVar)
defer label.SetProcessLabel("")

If labelVar is not empty, it is checked whether selinux enabled or not by runc.
If labelVar is empty, the code becomes:

label.SetProcessLabel("")
defer label.SetProcessLabel("")

I think it has no meanings to the system even though selinux is enabled.

So I think we should check labelVal in runc, just like check config.cwd is empty or not.

If I'm wrong, please point out. Thanks.

@lifubang

This comment has been minimized.

Copy link
Contributor Author

lifubang commented Apr 2, 2019

If labelVar is empty, I think this means the user doesn't want to use selinux in runc.
We shouldn't call any selinux API at that time.

@cyphar

This comment has been minimized.

Copy link
Member

cyphar commented Apr 2, 2019

@lifubang While it would make the operations a no-op, it's much better to do it in go-selinux so that we don't forget to do it in runc (like we forgot in #2012 and in several other places throughout runc). Function calls are cheaper than future maintenance work when someone forgets to add a check again.

@lifubang

This comment has been minimized.

Copy link
Contributor Author

lifubang commented Apr 2, 2019

Function calls are cheaper than future maintenance work when someone forgets to add a check again.

I know what's the purpose now. Thank you for your explanation.

@amimof amimof referenced this pull request Apr 2, 2019

Merged

Update component versions #44

@rhatdan

This comment has been minimized.

Copy link
Contributor

rhatdan commented Apr 2, 2019

I have opened this PR opencontainers/selinux#49 to fix the issue in go-selinux.

@lifubang

This comment has been minimized.

Copy link
Contributor Author

lifubang commented Apr 3, 2019

As the regression is fixed by #2032 , so this PR becomes enhancement. The main purpose of this PR is:

  1. If the user don't provide selinuxLabel, I think this means the user doesn't want to use selinux in runc, so we shouldn't call any selinux API at that time.
  2. Add selinux validate if the user provide selinuxLabel when runc exec.

@lifubang lifubang changed the title Fix several selinux issues Add selinux validate in runc exec Apr 3, 2019

@@ -34,10 +34,12 @@ func (l *linuxSetnsInit) Init() error {
defer runtime.UnlockOSThread()

if !l.config.Config.NoNewKeyring {
if err := label.SetKeyLabel(l.config.ProcessLabel); err != nil {
return err
if l.config.ProcessLabel != "" {

This comment has been minimized.

Copy link
@cyphar

cyphar Apr 3, 2019

Member

These checks aren't necessary anymore.

This comment has been minimized.

Copy link
@lifubang

lifubang Apr 3, 2019

Author Contributor

updated

@cyphar

This comment has been minimized.

Copy link
Member

cyphar commented Apr 3, 2019

My only comment is we don't need any of the ProcessLabel != "" checks. Everything else looks fine. I'll send out the vote for an rc8 release once this is merged.

@lifubang lifubang force-pushed the lifubang:selinux branch from dda6158 to b1ea070 Apr 3, 2019

add selinux label for runc exec
Signed-off-by: lifubang <lifubang@acmcoder.com>

@lifubang lifubang force-pushed the lifubang:selinux branch from b1ea070 to 3e6688f Apr 3, 2019

@cyphar

This comment has been minimized.

Copy link
Member

cyphar commented Apr 3, 2019

LGTM. Thanks @lifubang.

Approved with PullApprove

@crosbymichael

This comment has been minimized.

Copy link
Member

crosbymichael commented Apr 3, 2019

LGTM

Approved with PullApprove

@crosbymichael crosbymichael merged commit 029124d into opencontainers:master Apr 3, 2019

2 checks passed

code-review/pullapprove Approved by crosbymichael, cyphar
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

thaJeztah added a commit to thaJeztah/containerd that referenced this pull request Apr 5, 2019

bump runc to 029124da (v1.0.0-rc7-6-g029124da)
no changes in vendored files

full diff: opencontainers/runc@v1.0.0-rc7...029124d

- opencontainers/runc#2031 Add selinux validate in runc exec
- opencontainers/runc#2032 Fix SELinux failures on disabled SELinux Machines
- addresses opencontainers#2030 "container init caused "write /proc/self/attr/keycreate: invalid argument"

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah added a commit to thaJeztah/containerd that referenced this pull request Apr 5, 2019

bump runc to 029124da (v1.0.0-rc7-6-g029124da)
no changes in vendored files

full diff: opencontainers/runc@v1.0.0-rc7...029124d

- opencontainers/runc#2031 Add selinux validate in runc exec
- opencontainers/runc#2032 Fix SELinux failures on disabled SELinux Machines
- addresses opencontainers#2030 "container init caused "write /proc/self/attr/keycreate: invalid argument"

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
You can’t perform that action at this time.