From 80d99236c1ef9d389dbaca73c1a949da16b56b42 Mon Sep 17 00:00:00 2001 From: Eric Windisch Date: Mon, 11 May 2015 23:00:05 -0400 Subject: [PATCH] Move AppArmor policy to contrib & deb packaging The automatic installation of AppArmor policies prevents the management of custom, site-specific apparmor policies for the default container profile. Furthermore, this change will allow a future policy for the engine itself to be written without demanding the engine be able to arbitrarily create and manage AppArmor policies. - Add deb package suggests for apparmor. - Ubuntu postinst use aa-status & fix policy path - Add the policies to the debian packages. - Add apparmor tests for writing proc files Additional restrictions against modifying files in proc are enforced by AppArmor. Ensure that AppArmor is preventing access to these files, not simply Docker's configuration of proc. - Remove /proc/k?mem from AA policy The path to mem and kmem are in /dev, not /proc and cannot be restricted successfully through AppArmor. The device cgroup will need to be sufficient here. - Load contrib/apparmor during integration tests Note that this is somewhat dirty because we cannot restore the host to its original configuration. However, it should be noted that prior to this patch series, the Docker daemon itself was loading apparmor policy from within the tests, so this is no dirtier or uglier than the status-quo. Signed-off-by: Eric Windisch --- contrib/apparmor/docker | 25 +++++ contrib/builder/deb/generate.sh | 1 + daemon/execdriver/native/apparmor.go | 124 --------------------- daemon/execdriver/native/driver.go | 4 - hack/make/.build-deb/docker-engine.install | 1 + hack/make/.build-deb/rules | 3 + hack/make/.integration-daemon-start | 2 + hack/make/ubuntu | 9 ++ integration-cli/docker_cli_run_test.go | 22 ++++ 9 files changed, 63 insertions(+), 128 deletions(-) create mode 100644 contrib/apparmor/docker delete mode 100644 daemon/execdriver/native/apparmor.go diff --git a/contrib/apparmor/docker b/contrib/apparmor/docker new file mode 100644 index 0000000000..4674ecf6e9 --- /dev/null +++ b/contrib/apparmor/docker @@ -0,0 +1,25 @@ +#include + +profile docker-default flags=(attach_disconnected,mediate_deleted) { + #include + + network, + capability, + file, + umount, + + deny @{PROC}/sys/fs/** wklx, + deny @{PROC}/sysrq-trigger rwklx, + deny @{PROC}/sys/kernel/[^s][^h][^m]* wklx, + deny @{PROC}/sys/kernel/*/** wklx, + + deny mount, + + deny /sys/[^f]*/** wklx, + deny /sys/f[^s]*/** wklx, + deny /sys/fs/[^c]*/** wklx, + deny /sys/fs/c[^g]*/** wklx, + deny /sys/fs/cg[^r]*/** wklx, + deny /sys/firmware/efi/efivars/** rwklx, + deny /sys/kernel/security/** rwklx, +} diff --git a/contrib/builder/deb/generate.sh b/contrib/builder/deb/generate.sh index 4bb7320eaf..4ab31605a3 100755 --- a/contrib/builder/deb/generate.sh +++ b/contrib/builder/deb/generate.sh @@ -50,6 +50,7 @@ for version in "${versions[@]}"; do build-essential # "essential for building Debian packages" curl ca-certificates # for downloading Go debhelper # for easy ".deb" building + dh-apparmor # for apparmor debhelper dh-systemd # for systemd debhelper integration git # for "git commit" info in "docker -v" libapparmor-dev # for "sys/apparmor.h" diff --git a/daemon/execdriver/native/apparmor.go b/daemon/execdriver/native/apparmor.go deleted file mode 100644 index 894b2d489a..0000000000 --- a/daemon/execdriver/native/apparmor.go +++ /dev/null @@ -1,124 +0,0 @@ -// +build linux - -package native - -import ( - "fmt" - "io" - "os" - "os/exec" - "path" - "text/template" - - "github.com/opencontainers/runc/libcontainer/apparmor" -) - -const ( - apparmorProfilePath = "/etc/apparmor.d/docker" -) - -type data struct { - Name string - Imports []string - InnerImports []string -} - -const baseTemplate = ` -{{range $value := .Imports}} -{{$value}} -{{end}} - -profile {{.Name}} flags=(attach_disconnected,mediate_deleted) { -{{range $value := .InnerImports}} - {{$value}} -{{end}} - - network, - capability, - file, - umount, - - deny @{PROC}/sys/fs/** wklx, - deny @{PROC}/sysrq-trigger rwklx, - deny @{PROC}/mem rwklx, - deny @{PROC}/kmem rwklx, - deny @{PROC}/sys/kernel/[^s][^h][^m]* wklx, - deny @{PROC}/sys/kernel/*/** wklx, - - deny mount, - - deny /sys/[^f]*/** wklx, - deny /sys/f[^s]*/** wklx, - deny /sys/fs/[^c]*/** wklx, - deny /sys/fs/c[^g]*/** wklx, - deny /sys/fs/cg[^r]*/** wklx, - deny /sys/firmware/efi/efivars/** rwklx, - deny /sys/kernel/security/** rwklx, -} -` - -func generateProfile(out io.Writer) error { - compiled, err := template.New("apparmor_profile").Parse(baseTemplate) - if err != nil { - return err - } - data := &data{ - Name: "docker-default", - } - if tunablesExists() { - data.Imports = append(data.Imports, "#include ") - } else { - data.Imports = append(data.Imports, "@{PROC}=/proc/") - } - if abstractionsExists() { - data.InnerImports = append(data.InnerImports, "#include ") - } - if err := compiled.Execute(out, data); err != nil { - return err - } - return nil -} - -// check if the tunables/global exist -func tunablesExists() bool { - _, err := os.Stat("/etc/apparmor.d/tunables/global") - return err == nil -} - -// check if abstractions/base exist -func abstractionsExists() bool { - _, err := os.Stat("/etc/apparmor.d/abstractions/base") - return err == nil -} - -func installApparmorProfile() error { - if !apparmor.IsEnabled() { - return nil - } - - // Make sure /etc/apparmor.d exists - if err := os.MkdirAll(path.Dir(apparmorProfilePath), 0755); err != nil { - return err - } - - f, err := os.OpenFile(apparmorProfilePath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0644) - if err != nil { - return err - } - if err := generateProfile(f); err != nil { - f.Close() - return err - } - f.Close() - - cmd := exec.Command("/sbin/apparmor_parser", "-r", "-W", "docker") - // to use the parser directly we have to make sure we are in the correct - // dir with the profile - cmd.Dir = "/etc/apparmor.d" - - output, err := cmd.CombinedOutput() - if err != nil { - return fmt.Errorf("Error loading docker apparmor profile: %s (%s)", err, output) - } - return nil -} diff --git a/daemon/execdriver/native/driver.go b/daemon/execdriver/native/driver.go index 7e13c267ef..4573d94629 100644 --- a/daemon/execdriver/native/driver.go +++ b/daemon/execdriver/native/driver.go @@ -50,10 +50,6 @@ func NewDriver(root, initPath string, options []string) (*driver, error) { if err := sysinfo.MkdirAll(root, 0700); err != nil { return nil, err } - // native driver root is at docker_root/execdriver/native. Put apparmor at docker_root - if err := installApparmorProfile(); err != nil { - return nil, err - } // choose cgroup manager // this makes sure there are no breaking changes to people diff --git a/hack/make/.build-deb/docker-engine.install b/hack/make/.build-deb/docker-engine.install index a8857a96dc..9371ac8730 100644 --- a/hack/make/.build-deb/docker-engine.install +++ b/hack/make/.build-deb/docker-engine.install @@ -9,3 +9,4 @@ contrib/init/systemd/docker.socket lib/systemd/system/ contrib/mk* usr/share/docker-engine/contrib/ contrib/nuke-graph-directory.sh usr/share/docker-engine/contrib/ contrib/syntax/nano/Dockerfile.nanorc usr/share/nano/ +contrib/apparmor/* etc/apparmor.d/ diff --git a/hack/make/.build-deb/rules b/hack/make/.build-deb/rules index b4c8e2b4c7..bb184bfa58 100755 --- a/hack/make/.build-deb/rules +++ b/hack/make/.build-deb/rules @@ -32,5 +32,8 @@ override_dh_installudev: # match our existing priority dh_installudev --priority=z80 +override_dh_install: + dh_apparmor --profile-name=docker -pdocker-engine + %: dh $@ --with=bash-completion $(shell command -v dh_systemd_enable > /dev/null 2>&1 && echo --with=systemd) diff --git a/hack/make/.integration-daemon-start b/hack/make/.integration-daemon-start index dcc09fa920..b4cdf86fec 100644 --- a/hack/make/.integration-daemon-start +++ b/hack/make/.integration-daemon-start @@ -35,6 +35,8 @@ if [ -z "$DOCKER_TEST_HOST" ]; then ( set -x /etc/init.d/apparmor start + + /sbin/apparmor_parser -r -W -T contrib/apparmor/ ) fi diff --git a/hack/make/ubuntu b/hack/make/ubuntu index a791f9c964..489f8089bd 100644 --- a/hack/make/ubuntu +++ b/hack/make/ubuntu @@ -72,6 +72,10 @@ bundle_ubuntu() { done done + # Include contributed apparmor policy + mkdir -p "$DIR/etc/apparmor.d/" + cp contrib/apparmor/docker "$DIR/etc/apparmor.d/" + # Copy the binary # This will fail if the binary bundle hasn't been built mkdir -p "$DIR/usr/bin" @@ -89,6 +93,10 @@ if [ "$1" = 'configure' ] && [ -z "$2" ]; then fi fi +if ( aa-status --enabled ); then + /sbin/apparmor_parser -r -W -T /etc/apparmor.d/docker +fi + if ! { [ -x /sbin/initctl ] && /sbin/initctl version 2>/dev/null | grep -q upstart; }; then # we only need to do this if upstart isn't in charge update-rc.d docker defaults > /dev/null || true @@ -149,6 +157,7 @@ EOF --deb-recommends git \ --deb-recommends xz-utils \ --deb-recommends 'cgroupfs-mount | cgroup-lite' \ + --deb-suggests apparmor \ --description "$PACKAGE_DESCRIPTION" \ --maintainer "$PACKAGE_MAINTAINER" \ --conflicts docker \ diff --git a/integration-cli/docker_cli_run_test.go b/integration-cli/docker_cli_run_test.go index 841ec421bf..4ae7a96486 100644 --- a/integration-cli/docker_cli_run_test.go +++ b/integration-cli/docker_cli_run_test.go @@ -2518,3 +2518,25 @@ func (s *DockerSuite) TestVolumeFromMixedRWOptions(c *check.C) { c.Fatalf("Expected RW volume was RO") } } + +func (s *DockerSuite) TestRunWriteFilteredProc(c *check.C) { + testRequires(c, Apparmor) + + testWritePaths := []string{ + /* modprobe and core_pattern should both be denied by generic + * policy of denials for /proc/sys/kernel. These files have been + * picked to be checked as they are particularly sensitive to writes */ + "/proc/sys/kernel/modprobe", + "/proc/sys/kernel/core_pattern", + "/proc/sysrq-trigger", + } + for i, filePath := range testWritePaths { + name := fmt.Sprintf("writeprocsieve-%d", i) + + shellCmd := fmt.Sprintf("exec 3>%s", filePath) + runCmd := exec.Command(dockerBinary, "run", "--privileged", "--security-opt", "apparmor:docker-default", "--name", name, "busybox", "sh", "-c", shellCmd) + if out, exitCode, err := runCommandWithOutput(runCmd); err == nil || exitCode == 0 { + c.Fatalf("Open FD for write should have failed with permission denied, got: %s, %v", out, err) + } + } +}