diff --git a/libcontainer/cgroups/fs/apply_raw.go b/libcontainer/cgroups/fs/apply_raw.go index 21646e571..b7461b932 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,14 +277,19 @@ 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{ root: root, - innerPath: c.Path, + innerPath: innerPath, config: c, pid: pid, }, nil diff --git a/libcontainer/cgroups/fs/apply_raw_test.go b/libcontainer/cgroups/fs/apply_raw_test.go new file mode 100644 index 000000000..ba4e9e543 --- /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!") + } +} diff --git a/libcontainer/utils/utils.go b/libcontainer/utils/utils.go index 1f5528ff3..68ae3c477 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)