From 3c25c9b9cfc97ac342b4eba74248ef48a7e51743 Mon Sep 17 00:00:00 2001 From: Michael Crosby Date: Tue, 21 Apr 2015 13:46:27 -0700 Subject: [PATCH 1/6] Eval mount destination after each mount User specified mounts much be evaluated after each mount because symlinks in nested mounts can invalidate the next mount. Also check that any bind mounts are not inside /proc or /sys to ensure that we are able to mask over certian paths inside. Signed-off-by: Michael Crosby --- rootfs_linux.go | 43 +++++++++++++++++++++++++++++++++++++++++++ rootfs_linux_test.go | 29 +++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+) create mode 100644 rootfs_linux_test.go diff --git a/rootfs_linux.go b/rootfs_linux.go index d8c61e97..bbc59d19 100644 --- a/rootfs_linux.go +++ b/rootfs_linux.go @@ -13,6 +13,7 @@ import ( "syscall" "time" + "github.com/docker/docker/pkg/symlink" "github.com/docker/libcontainer/cgroups" "github.com/docker/libcontainer/configs" "github.com/docker/libcontainer/label" @@ -139,6 +140,16 @@ func mountToRootfs(m *configs.Mount, rootfs, mountLabel string) error { // unable to bind anything to it. return err } + // ensure that the destination of the bind mount is resolved of symlinks at mount time because + // any previous mounts can invalidate the next mount's destination. + // this can happen when a user specifies mounts within other mounts to cause breakouts or other + // evil stuff to try to escape the container's rootfs. + if dest, err = symlink.FollowSymlinkInScope(filepath.Join(rootfs, m.Destination), rootfs); err != nil { + return err + } + if err := checkMountDestination(rootfs, dest); err != nil { + return err + } if err := createIfNotExists(dest, stat.IsDir()); err != nil { return err } @@ -197,6 +208,38 @@ func mountToRootfs(m *configs.Mount, rootfs, mountLabel string) error { return nil } +// checkMountDestination checks to ensure that the mount destination is not over the +// top of /proc or /sys. +// dest is required to be an abs path and have any symlinks resolved before calling this function. +func checkMountDestination(rootfs, dest string) error { + invalidDestinations := []string{ + "/proc", + "/sys", + } + for _, invalid := range invalidDestinations { + if dirIsChild(filepath.Join(rootfs, invalid), dest) { + return fmt.Errorf("%q cannot be mounted because it is located inside %q", dest, invalid) + } + } + return nil +} + +// dirIsChild compare the parts of the dir to check if it is located +// inside root. comparing the individual parts ensures that false positives +// are not found. +func dirIsChild(root, dir string) bool { + var ( + rootParts = strings.Split(filepath.Clean(root), string(filepath.Separator)) + dirParts = strings.Split(filepath.Clean(dir), string(filepath.Separator)) + ) + for i, p := range rootParts { + if p != dirParts[i] { + return false + } + } + return true +} + func setupDevSymlinks(rootfs string) error { var links = [][2]string{ {"/proc/self/fd", "/dev/fd"}, diff --git a/rootfs_linux_test.go b/rootfs_linux_test.go new file mode 100644 index 00000000..d3e0cf36 --- /dev/null +++ b/rootfs_linux_test.go @@ -0,0 +1,29 @@ +// +build linux + +package libcontainer + +import "testing" + +func TestCheckMountDestOnProc(t *testing.T) { + dest := "/rootfs/proc/" + err := checkMountDestination("/rootfs", dest) + if err == nil { + t.Fatal("destination inside proc should return an error") + } +} + +func TestCheckMountDestInSys(t *testing.T) { + dest := "/rootfs//sys/fs/cgroup" + err := checkMountDestination("/rootfs", dest) + if err == nil { + t.Fatal("destination inside proc should return an error") + } +} + +func TestCheckMountDestFalsePositive(t *testing.T) { + dest := "/rootfs/sysfiles/fs/cgroup" + err := checkMountDestination("/rootfs", dest) + if err != nil { + t.Fatal(err) + } +} From e3e7c47123cc146bf9b9a41b73b34aae2f7e2aea Mon Sep 17 00:00:00 2001 From: Michael Crosby Date: Thu, 30 Apr 2015 10:23:42 -0700 Subject: [PATCH 2/6] Prohibit bind mounts into / Signed-off-by: Michael Crosby --- rootfs_linux.go | 6 ++++++ rootfs_linux_test.go | 8 ++++++++ 2 files changed, 14 insertions(+) diff --git a/rootfs_linux.go b/rootfs_linux.go index bbc59d19..e63a394f 100644 --- a/rootfs_linux.go +++ b/rootfs_linux.go @@ -212,6 +212,9 @@ func mountToRootfs(m *configs.Mount, rootfs, mountLabel string) error { // top of /proc or /sys. // dest is required to be an abs path and have any symlinks resolved before calling this function. func checkMountDestination(rootfs, dest string) error { + if filepath.Clean(rootfs) == filepath.Clean(dest) { + return fmt.Errorf("mounting into / is prohibited") + } invalidDestinations := []string{ "/proc", "/sys", @@ -232,6 +235,9 @@ func dirIsChild(root, dir string) bool { rootParts = strings.Split(filepath.Clean(root), string(filepath.Separator)) dirParts = strings.Split(filepath.Clean(dir), string(filepath.Separator)) ) + if len(dirParts) < len(rootParts) { + return false + } for i, p := range rootParts { if p != dirParts[i] { return false diff --git a/rootfs_linux_test.go b/rootfs_linux_test.go index d3e0cf36..54df065c 100644 --- a/rootfs_linux_test.go +++ b/rootfs_linux_test.go @@ -27,3 +27,11 @@ func TestCheckMountDestFalsePositive(t *testing.T) { t.Fatal(err) } } + +func TestCheckMountRoot(t *testing.T) { + dest := "/rootfs" + err := checkMountDestination("/rootfs", dest) + if err == nil { + t.Fatal(err) + } +} From c08e43409d6736da2239bda1869cc70103d95584 Mon Sep 17 00:00:00 2001 From: Michael Crosby Date: Thu, 30 Apr 2015 11:03:07 -0700 Subject: [PATCH 3/6] Move reopenDevNull until after rootfs jail We need to do this incase /dev/null is a symlink pointing somewhere outside the container's rootfs. Signed-off-by: Michael Crosby --- rootfs_linux.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/rootfs_linux.go b/rootfs_linux.go index e63a394f..89d8710a 100644 --- a/rootfs_linux.go +++ b/rootfs_linux.go @@ -49,11 +49,6 @@ func setupRootfs(config *configs.Config, console *linuxConsole) (err error) { if err := setupPtmx(config, console); err != nil { return newSystemError(err) } - // stdin, stdout and stderr could be pointing to /dev/null from parent namespace. - // re-open them inside this namespace. - if err := reOpenDevNull(config.Rootfs); err != nil { - return newSystemError(err) - } if err := setupDevSymlinks(config.Rootfs); err != nil { return newSystemError(err) } @@ -68,6 +63,9 @@ func setupRootfs(config *configs.Config, console *linuxConsole) (err error) { if err != nil { return newSystemError(err) } + if err := reOpenDevNull(config.Rootfs); err != nil { + return newSystemError(err) + } if config.Readonlyfs { if err := setReadonly(); err != nil { return newSystemError(err) @@ -270,11 +268,13 @@ func setupDevSymlinks(rootfs string) error { return nil } -// If stdin, stdout or stderr are pointing to '/dev/null' in the global mount namespace, -// this method will make them point to '/dev/null' in this namespace. +// If stdin, stdout, and/or stderr are pointing to `/dev/null` in the parent's rootfs +// this method will make them point to `/dev/null` in this container's rootfs. This +// needs to be called after we chroot/pivot into the container's rootfs so that any +// symlinks are resolved locally. func reOpenDevNull(rootfs string) error { var stat, devNullStat syscall.Stat_t - file, err := os.Open(filepath.Join(rootfs, "/dev/null")) + file, err := os.Open("/dev/null") if err != nil { return fmt.Errorf("Failed to open /dev/null - %s", err) } From 2323c4c48d7f732289cbfb8a233df6c29befae41 Mon Sep 17 00:00:00 2001 From: Michael Crosby Date: Thu, 30 Apr 2015 12:39:29 -0700 Subject: [PATCH 4/6] Use filepath.Rel for subdirectory comparison Signed-off-by: Michael Crosby --- rootfs_linux.go | 25 +++++-------------------- 1 file changed, 5 insertions(+), 20 deletions(-) diff --git a/rootfs_linux.go b/rootfs_linux.go index 89d8710a..0cd60373 100644 --- a/rootfs_linux.go +++ b/rootfs_linux.go @@ -218,32 +218,17 @@ func checkMountDestination(rootfs, dest string) error { "/sys", } for _, invalid := range invalidDestinations { - if dirIsChild(filepath.Join(rootfs, invalid), dest) { + path, err := filepath.Rel(filepath.Join(rootfs, invalid), dest) + if err != nil { + return err + } + if path == "." || !strings.HasPrefix(path, "..") { return fmt.Errorf("%q cannot be mounted because it is located inside %q", dest, invalid) } } return nil } -// dirIsChild compare the parts of the dir to check if it is located -// inside root. comparing the individual parts ensures that false positives -// are not found. -func dirIsChild(root, dir string) bool { - var ( - rootParts = strings.Split(filepath.Clean(root), string(filepath.Separator)) - dirParts = strings.Split(filepath.Clean(dir), string(filepath.Separator)) - ) - if len(dirParts) < len(rootParts) { - return false - } - for i, p := range rootParts { - if p != dirParts[i] { - return false - } - } - return true -} - func setupDevSymlinks(rootfs string) error { var links = [][2]string{ {"/proc/self/fd", "/dev/fd"}, From 364d8e15050018e2a56f1c106e678ab32b167c40 Mon Sep 17 00:00:00 2001 From: Eric Windisch Date: Tue, 28 Apr 2015 23:51:31 -0400 Subject: [PATCH 5/6] Disable all mounts in AppArmor profile Allowing mounts in containers is dangerous. Bugs in mount namespaces or quirks of the container configuration could allow for various breakouts. By default, processes in containers will not be able to mount anyway, rendering the allowances in the default AppArmor profile nearly useless. Manually created sub-containers were able to mount, but were yet restricted from performing most of the mounts flags indicated in the profile. Signed-off-by: Eric Windisch --- SPEC.md | 15 +-------------- apparmor/gen.go | 15 +-------------- 2 files changed, 2 insertions(+), 28 deletions(-) diff --git a/SPEC.md b/SPEC.md index 386cc7f8..5d37fe93 100644 --- a/SPEC.md +++ b/SPEC.md @@ -217,17 +217,6 @@ profile flags=(attach_disconnected,mediate_deleted) { file, umount, - mount fstype=tmpfs, - mount fstype=mqueue, - mount fstype=fuse.*, - mount fstype=binfmt_misc -> /proc/sys/fs/binfmt_misc/, - mount fstype=efivarfs -> /sys/firmware/efi/efivars/, - mount fstype=fusectl -> /sys/fs/fuse/connections/, - mount fstype=securityfs -> /sys/kernel/security/, - mount fstype=debugfs -> /sys/kernel/debug/, - mount fstype=proc -> /proc/, - mount fstype=sysfs -> /sys/, - deny @{PROC}/sys/fs/** wklx, deny @{PROC}/sysrq-trigger rwklx, deny @{PROC}/mem rwklx, @@ -235,9 +224,7 @@ profile flags=(attach_disconnected,mediate_deleted) { deny @{PROC}/sys/kernel/[^s][^h][^m]* wklx, deny @{PROC}/sys/kernel/*/** wklx, - deny mount options=(ro, remount) -> /, - deny mount fstype=debugfs -> /var/lib/ureadahead/debugfs/, - deny mount fstype=devpts, + deny mount, deny /sys/[^f]*/** wklx, deny /sys/f[^s]*/** wklx, diff --git a/apparmor/gen.go b/apparmor/gen.go index 4565f6df..a3192e23 100644 --- a/apparmor/gen.go +++ b/apparmor/gen.go @@ -27,17 +27,6 @@ profile {{.Name}} flags=(attach_disconnected,mediate_deleted) { file, umount, - mount fstype=tmpfs, - mount fstype=mqueue, - mount fstype=fuse.*, - mount fstype=binfmt_misc -> /proc/sys/fs/binfmt_misc/, - mount fstype=efivarfs -> /sys/firmware/efi/efivars/, - mount fstype=fusectl -> /sys/fs/fuse/connections/, - mount fstype=securityfs -> /sys/kernel/security/, - mount fstype=debugfs -> /sys/kernel/debug/, - mount fstype=proc -> /proc/, - mount fstype=sysfs -> /sys/, - deny @{PROC}/sys/fs/** wklx, deny @{PROC}/sysrq-trigger rwklx, deny @{PROC}/mem rwklx, @@ -45,9 +34,7 @@ profile {{.Name}} flags=(attach_disconnected,mediate_deleted) { deny @{PROC}/sys/kernel/[^s][^h][^m]* wklx, deny @{PROC}/sys/kernel/*/** wklx, - deny mount options=(ro, remount) -> /, - deny mount fstype=debugfs -> /var/lib/ureadahead/debugfs/, - deny mount fstype=devpts, + deny mount, deny /sys/[^f]*/** wklx, deny /sys/f[^s]*/** wklx, From 8ef205cd1c48d97e9c8a5fd4331c9fd4055c0103 Mon Sep 17 00:00:00 2001 From: Michael Crosby Date: Thu, 7 May 2015 14:46:19 -0700 Subject: [PATCH 6/6] Update mnt command test path You cannot use an abs path inside the conatiner's rootfs. Signed-off-by: Michael Crosby --- integration/exec_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration/exec_test.go b/integration/exec_test.go index c8b16687..fea5f7ee 100644 --- a/integration/exec_test.go +++ b/integration/exec_test.go @@ -651,7 +651,7 @@ func TestMountCmds(t *testing.T) { config := newTemplateConfig(rootfs) config.Mounts = append(config.Mounts, &configs.Mount{ Source: tmpDir, - Destination: filepath.Join(rootfs, "tmp"), + Destination: "/tmp", Device: "bind", Flags: syscall.MS_BIND | syscall.MS_REC, PremountCmds: []configs.Command{