From 90140a56880b274067c454b3f9f9c88f9a627d37 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Thu, 11 Feb 2016 19:57:46 +1100 Subject: [PATCH 1/3] libcontainer: cgroups: fs: fix innerPath Fix m.Path legacy code to actually work. Signed-off-by: Aleksa Sarai --- libcontainer/cgroups/fs/apply_raw.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libcontainer/cgroups/fs/apply_raw.go b/libcontainer/cgroups/fs/apply_raw.go index 21646e57..81bf0b89 100644 --- a/libcontainer/cgroups/fs/apply_raw.go +++ b/libcontainer/cgroups/fs/apply_raw.go @@ -283,7 +283,7 @@ func getCgroupData(c *configs.Cgroup, pid int) (*cgroupData, error) { return &cgroupData{ root: root, - innerPath: c.Path, + innerPath: innerPath, config: c, pid: pid, }, nil From b8dc5213e8d9f200dc1f5bbf22599f25a04fe3a3 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Thu, 11 Feb 2016 21:15:50 +1100 Subject: [PATCH 2/3] libcontainer: cgroups: fs: fix path safety Ensure that path safety is maintained, this essentially reapplies c0cad6aa5e44 ("cgroups: fs: fix cgroup.Parent path sanitisation"), which was accidentally removed in 256f3a8ebc11 ("Add support for CgroupsPath field"). Signed-off-by: Aleksa Sarai --- libcontainer/cgroups/fs/apply_raw.go | 10 ++++++++-- libcontainer/utils/utils.go | 5 +++++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/libcontainer/cgroups/fs/apply_raw.go b/libcontainer/cgroups/fs/apply_raw.go index 81bf0b89..b7461b93 100644 --- a/libcontainer/cgroups/fs/apply_raw.go +++ b/libcontainer/cgroups/fs/apply_raw.go @@ -14,6 +14,7 @@ import ( "github.com/opencontainers/runc/libcontainer/cgroups" "github.com/opencontainers/runc/libcontainer/configs" + libcontainerUtils "github.com/opencontainers/runc/libcontainer/utils" ) var ( @@ -276,9 +277,14 @@ func getCgroupData(c *configs.Cgroup, pid int) (*cgroupData, error) { return nil, fmt.Errorf("cgroup: either Path or Name and Parent should be used") } - innerPath := c.Path + // XXX: Do not remove this code. Path safety is important! -- cyphar + cgPath := libcontainerUtils.CleanPath(c.Path) + cgParent := libcontainerUtils.CleanPath(c.Parent) + cgName := libcontainerUtils.CleanPath(c.Name) + + innerPath := cgPath if innerPath == "" { - innerPath = filepath.Join(c.Parent, c.Name) + innerPath = filepath.Join(cgParent, cgName) } return &cgroupData{ diff --git a/libcontainer/utils/utils.go b/libcontainer/utils/utils.go index 1f5528ff..68ae3c47 100644 --- a/libcontainer/utils/utils.go +++ b/libcontainer/utils/utils.go @@ -63,6 +63,11 @@ func WriteJSON(w io.Writer, v interface{}) error { // be a subdirectory of the prefixed path. This is all done lexically, so paths // that include symlinks won't be safe as a result of using CleanPath. func CleanPath(path string) string { + // Deal with empty strings nicely. + if path == "" { + return "" + } + // Ensure that all paths are cleaned (especially problematic ones like // "/../../../../../" which can cause lots of issues). path = filepath.Clean(path) From 21dc85c4b830f1b2ae2da875690b9086e6c5fb58 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Fri, 12 Feb 2016 18:46:17 +1100 Subject: [PATCH 3/3] libcontainer: cgroups: fs: add cgroup path safety unit tests In order to avoid problems with security regressions going unnoticed, add some unit tests that should make sure security regressions in cgroup path safety cause tests to fail in runC. Signed-off-by: Aleksa Sarai --- libcontainer/cgroups/fs/apply_raw_test.go | 272 ++++++++++++++++++++++ 1 file changed, 272 insertions(+) create mode 100644 libcontainer/cgroups/fs/apply_raw_test.go diff --git a/libcontainer/cgroups/fs/apply_raw_test.go b/libcontainer/cgroups/fs/apply_raw_test.go new file mode 100644 index 00000000..ba4e9e54 --- /dev/null +++ b/libcontainer/cgroups/fs/apply_raw_test.go @@ -0,0 +1,272 @@ +// +build linux + +package fs + +import ( + "path/filepath" + "strings" + "testing" + + "github.com/opencontainers/runc/libcontainer/configs" +) + +func TestInvalidCgroupPath(t *testing.T) { + root, err := getCgroupRoot() + if err != nil { + t.Errorf("couldn't get cgroup root: %v", err) + } + + config := &configs.Cgroup{ + Path: "../../../../../../../../../../some/path", + } + + data, err := getCgroupData(config, 0) + if err != nil { + t.Errorf("couldn't get cgroup data: %v", err) + } + + // Make sure the final innerPath doesn't go outside the cgroup mountpoint. + if strings.HasPrefix(data.innerPath, "..") { + t.Errorf("SECURITY: cgroup innerPath is outside cgroup mountpoint!") + } + + // Double-check, using an actual cgroup. + deviceRoot := filepath.Join(root, "devices") + devicePath, err := data.path("devices") + if err != nil { + t.Errorf("couldn't get cgroup path: %v", err) + } + if !strings.HasPrefix(devicePath, deviceRoot) { + t.Errorf("SECURITY: cgroup path() is outside cgroup mountpoint!") + } +} + +func TestInvalidAbsoluteCgroupPath(t *testing.T) { + root, err := getCgroupRoot() + if err != nil { + t.Errorf("couldn't get cgroup root: %v", err) + } + + config := &configs.Cgroup{ + Path: "/../../../../../../../../../../some/path", + } + + data, err := getCgroupData(config, 0) + if err != nil { + t.Errorf("couldn't get cgroup data: %v", err) + } + + // Make sure the final innerPath doesn't go outside the cgroup mountpoint. + if strings.HasPrefix(data.innerPath, "..") { + t.Errorf("SECURITY: cgroup innerPath is outside cgroup mountpoint!") + } + + // Double-check, using an actual cgroup. + deviceRoot := filepath.Join(root, "devices") + devicePath, err := data.path("devices") + if err != nil { + t.Errorf("couldn't get cgroup path: %v", err) + } + if !strings.HasPrefix(devicePath, deviceRoot) { + t.Errorf("SECURITY: cgroup path() is outside cgroup mountpoint!") + } +} + +// XXX: Remove me after we get rid of configs.Cgroup.Name and configs.Cgroup.Parent. +func TestInvalidCgroupParent(t *testing.T) { + root, err := getCgroupRoot() + if err != nil { + t.Errorf("couldn't get cgroup root: %v", err) + } + + config := &configs.Cgroup{ + Parent: "../../../../../../../../../../some/path", + Name: "name", + } + + data, err := getCgroupData(config, 0) + if err != nil { + t.Errorf("couldn't get cgroup data: %v", err) + } + + // Make sure the final innerPath doesn't go outside the cgroup mountpoint. + if strings.HasPrefix(data.innerPath, "..") { + t.Errorf("SECURITY: cgroup innerPath is outside cgroup mountpoint!") + } + + // Double-check, using an actual cgroup. + deviceRoot := filepath.Join(root, "devices") + devicePath, err := data.path("devices") + if err != nil { + t.Errorf("couldn't get cgroup path: %v", err) + } + if !strings.HasPrefix(devicePath, deviceRoot) { + t.Errorf("SECURITY: cgroup path() is outside cgroup mountpoint!") + } +} + +// XXX: Remove me after we get rid of configs.Cgroup.Name and configs.Cgroup.Parent. +func TestInvalidAbsoluteCgroupParent(t *testing.T) { + root, err := getCgroupRoot() + if err != nil { + t.Errorf("couldn't get cgroup root: %v", err) + } + + config := &configs.Cgroup{ + Parent: "/../../../../../../../../../../some/path", + Name: "name", + } + + data, err := getCgroupData(config, 0) + if err != nil { + t.Errorf("couldn't get cgroup data: %v", err) + } + + // Make sure the final innerPath doesn't go outside the cgroup mountpoint. + if strings.HasPrefix(data.innerPath, "..") { + t.Errorf("SECURITY: cgroup innerPath is outside cgroup mountpoint!") + } + + // Double-check, using an actual cgroup. + deviceRoot := filepath.Join(root, "devices") + devicePath, err := data.path("devices") + if err != nil { + t.Errorf("couldn't get cgroup path: %v", err) + } + if !strings.HasPrefix(devicePath, deviceRoot) { + t.Errorf("SECURITY: cgroup path() is outside cgroup mountpoint!") + } +} + +// XXX: Remove me after we get rid of configs.Cgroup.Name and configs.Cgroup.Parent. +func TestInvalidCgroupName(t *testing.T) { + root, err := getCgroupRoot() + if err != nil { + t.Errorf("couldn't get cgroup root: %v", err) + } + + config := &configs.Cgroup{ + Parent: "parent", + Name: "../../../../../../../../../../some/path", + } + + data, err := getCgroupData(config, 0) + if err != nil { + t.Errorf("couldn't get cgroup data: %v", err) + } + + // Make sure the final innerPath doesn't go outside the cgroup mountpoint. + if strings.HasPrefix(data.innerPath, "..") { + t.Errorf("SECURITY: cgroup innerPath is outside cgroup mountpoint!") + } + + // Double-check, using an actual cgroup. + deviceRoot := filepath.Join(root, "devices") + devicePath, err := data.path("devices") + if err != nil { + t.Errorf("couldn't get cgroup path: %v", err) + } + if !strings.HasPrefix(devicePath, deviceRoot) { + t.Errorf("SECURITY: cgroup path() is outside cgroup mountpoint!") + } + +} + +// XXX: Remove me after we get rid of configs.Cgroup.Name and configs.Cgroup.Parent. +func TestInvalidAbsoluteCgroupName(t *testing.T) { + root, err := getCgroupRoot() + if err != nil { + t.Errorf("couldn't get cgroup root: %v", err) + } + + config := &configs.Cgroup{ + Parent: "parent", + Name: "/../../../../../../../../../../some/path", + } + + data, err := getCgroupData(config, 0) + if err != nil { + t.Errorf("couldn't get cgroup data: %v", err) + } + + // Make sure the final innerPath doesn't go outside the cgroup mountpoint. + if strings.HasPrefix(data.innerPath, "..") { + t.Errorf("SECURITY: cgroup innerPath is outside cgroup mountpoint!") + } + + // Double-check, using an actual cgroup. + deviceRoot := filepath.Join(root, "devices") + devicePath, err := data.path("devices") + if err != nil { + t.Errorf("couldn't get cgroup path: %v", err) + } + if !strings.HasPrefix(devicePath, deviceRoot) { + t.Errorf("SECURITY: cgroup path() is outside cgroup mountpoint!") + } +} + +// XXX: Remove me after we get rid of configs.Cgroup.Name and configs.Cgroup.Parent. +func TestInvalidCgroupNameAndParent(t *testing.T) { + root, err := getCgroupRoot() + if err != nil { + t.Errorf("couldn't get cgroup root: %v", err) + } + + config := &configs.Cgroup{ + Parent: "../../../../../../../../../../some/path", + Name: "../../../../../../../../../../some/path", + } + + data, err := getCgroupData(config, 0) + if err != nil { + t.Errorf("couldn't get cgroup data: %v", err) + } + + // Make sure the final innerPath doesn't go outside the cgroup mountpoint. + if strings.HasPrefix(data.innerPath, "..") { + t.Errorf("SECURITY: cgroup innerPath is outside cgroup mountpoint!") + } + + // Double-check, using an actual cgroup. + deviceRoot := filepath.Join(root, "devices") + devicePath, err := data.path("devices") + if err != nil { + t.Errorf("couldn't get cgroup path: %v", err) + } + if !strings.HasPrefix(devicePath, deviceRoot) { + t.Errorf("SECURITY: cgroup path() is outside cgroup mountpoint!") + } +} + +// XXX: Remove me after we get rid of configs.Cgroup.Name and configs.Cgroup.Parent. +func TestInvalidAbsoluteCgroupNameAndParent(t *testing.T) { + root, err := getCgroupRoot() + if err != nil { + t.Errorf("couldn't get cgroup root: %v", err) + } + + config := &configs.Cgroup{ + Parent: "/../../../../../../../../../../some/path", + Name: "/../../../../../../../../../../some/path", + } + + data, err := getCgroupData(config, 0) + if err != nil { + t.Errorf("couldn't get cgroup data: %v", err) + } + + // Make sure the final innerPath doesn't go outside the cgroup mountpoint. + if strings.HasPrefix(data.innerPath, "..") { + t.Errorf("SECURITY: cgroup innerPath is outside cgroup mountpoint!") + } + + // Double-check, using an actual cgroup. + deviceRoot := filepath.Join(root, "devices") + devicePath, err := data.path("devices") + if err != nil { + t.Errorf("couldn't get cgroup path: %v", err) + } + if !strings.HasPrefix(devicePath, deviceRoot) { + t.Errorf("SECURITY: cgroup path() is outside cgroup mountpoint!") + } +}