Merge pull request #552 from cyphar/fix-cgroup-path

libcontainer: cgroups: fs: fix innerPath

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

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!")
+	}
+}

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)