8000 Fix AppArmor not being applied to Exec processes by thaJeztah · Pull Request #36466 · moby/moby · GitHub
[go: up one dir, main page]
Skip to content

Fix AppArmor not being applied to Exec processes #36466

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 19, 2018
Merged

Fix AppArmor not being applied to Exec processes #36466

merged 1 commit into from
Mar 19, 2018

Conversation

thaJeztah
Copy link
Member
@thaJeztah thaJeztah commented Mar 2, 2018 •
edited
Loading

fixes #36456

Exec processes do not automatically inherit AppArmor profiles from the container.

This patch sets the AppArmor profile for the exec process.

Before this change:

apparmor_parser -q -r <<EOF
#include <tunables/global>
profile deny-write flags=(attach_disconnected) {
  #include <abstractions/base>
  file,
  network,
  deny /tmp/** w,
  capability,
}
EOF

docker run -dit --security-opt "apparmor=deny-write" --name aa busybox

Running docker exec doesn't get the profile applied:

docker exec aa sh -c 'mkdir /tmp/test'
(no error)

With this change:

Running docker exec gets the AppArmor profile applied

docker exec aa sh -c 'mkdir /tmp/test'
mkdir: can't create directory '/tmp/test': Permission denied

- How to verify it

Build a .deb from this PR, using the Docker CE packaging scripts; https://github.com/docker/docker-ce-packaging/tree/master/deb

make \
  ENGINE_DIR=$GOPATH/src/github.com/docker/docker \
  CLI_DIR=$GOPATH/src/github.com/docker/cli \
  ubuntu-xenial

Which puts the package in debbuild/ubuntu-xenial

On an Ubuntu 16.04 machine, install the package:

dpkg -i ./docker-ce_0.0.0~dev~git20180302.121756.0.dae4588d4-0~ubuntu_amd64.deb

Run the reproduction steps above

- Description for the changelog

* Fix AppArmor profiles not being applied to `docker exec` processes [moby/moby#36466](https://github.com/moby/moby/pull/36466)

@thaJeztah
Fix AppArmor not being applied to Exec processes
8f3308a 
Exec processes do not automatically inherit AppArmor
profiles from the container.

This patch sets the AppArmor profile for the exec
process.

Before this change:

    apparmor_parser -q -r <<EOF
    #include <tunables/global>
    profile deny-write flags=(attach_disconnected) {
      #include <abstractions/base>
      file,
      network,
      deny /tmp/** w,
      capability,
    }
    EOF

    docker run -dit --security-opt "apparmor=deny-write" --name aa busybox

    docker exec aa sh -c 'mkdir /tmp/test'
    (no error)

With this change applied:

    docker exec aa sh -c 'mkdir /tmp/test'
    mkdir: can't create directory '/tmp/test': Permission denied

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Copy link
Member Author

ping @justincormack @mlaventure PTAL

thaJeztah
thaJeztah commented Mar 2, 2018
View reviewed changes
daemon/exec_linux_test.go
)

func TestExecSetPlatformOpt(t *testing.T) {
if !apparmor.IsEnabled() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference; looks like this check also checks for an CONTAINER env-var;

moby/vendor/github.com/opencontainers/runc/libcontainer/apparmor/apparmor.go

Lines 11 to 20 in f58aa31

// IsEnabled returns true if apparmor is enabled for the host.
func IsEnabled() bool {
if _, err := os.Stat("/sys/kernel/security/apparmor"); err == nil && os.Getenv("container") == "" {
if _, err = os.Stat("/sbin/apparmor_parser"); err == nil {
buf, err := ioutil.ReadFile("/sys/module/apparmor/parameters/enabled")
return err == nil && len(buf) > 1 && buf[0] == 'Y'
}
}
return false
}

But our integration-cli checks in a different way;

moby/integration-cli/requirements_test.go

Lines 110 to 113 in f58aa31

func Apparmor() bool {
buf, err := ioutil.ReadFile("/sys/module/apparmor/parameters/enabled")
return err == nil && len(buf) > 1 && buf[0] == 'Y'
}

mlaventure
mlaventure approved these changes Mar 2, 2018
View reviewed changes
Copy link
Contributor
@mlaventure mlaventure left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Failure seems to be related to libnetwork, restarting the ci

@codecov
Copy link
codecov bot commented Mar 2, 2018

Codecov Report

Merging #36466 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master   #36466      +/-   ##
==========================================
- Coverage   34.65%   34.65%   -0.01%     
==========================================
  Files         613      613              
  Lines       45375    45376       +1     
==========================================
  Hits        15725    15725              
- Misses      27588    27590       +2     
+ Partials     2062     2061       -1

@tonistiigi tonistiigi merged commit 0c1006f into moby:master Mar 19, 2018
@thaJeztah thaJeztah mentioned this pull request Mar 20, 2018
Merged
@thaJeztah thaJeztah deleted the fix-exec-apparmor branch March 20, 2018 09:15
@d d mentioned this pull request Apr 26, 2018
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
2 more reviewers

@justincormack justincormack justincormack approved these changes

@mlaventure mlaventure mlaventure approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@tonistiigi tonistiigi

Labels
area/runtime Runtime area/security/apparmor impact/changelog status/2-code-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

apparmor profile is not effect
6 participants
@thaJeztah @justincormack @mlaventure @tonistiigi @yongtang @GordonTheTurtle
0