diff --git a/libcontainer/cgroups/fs/fs.go b/libcontainer/cgroups/fs/fs.go index b9f298b41ed..f476db6966d 100644 --- a/libcontainer/cgroups/fs/fs.go +++ b/libcontainer/cgroups/fs/fs.go @@ -3,11 +3,9 @@ package fs import ( - "bufio" "fmt" "os" "path/filepath" - "strings" "sync" "github.com/opencontainers/runc/libcontainer/cgroups" @@ -78,66 +76,9 @@ func NewManager(cg *configs.Cgroup, paths map[string]string, rootless bool) cgro } // The absolute path to the root of the cgroup hierarchies. -var cgroupRootLock sync.Mutex -var cgroupRoot string - -// Gets the cgroupRoot. -func getCgroupRoot() (string, error) { - cgroupRootLock.Lock() - defer cgroupRootLock.Unlock() - - if cgroupRoot != "" { - return cgroupRoot, nil - } - - f, err := os.Open("/proc/self/mountinfo") - if err != nil { - return "", err - } - defer f.Close() - - var root string - scanner := bufio.NewScanner(f) - for scanner.Scan() { - text := scanner.Text() - fields := strings.Split(text, " ") - // Safe as mountinfo encodes mountpoints with spaces as \040. - index := strings.Index(text, " - ") - postSeparatorFields := strings.Fields(text[index+3:]) - numPostFields := len(postSeparatorFields) - - // This is an error as we can't detect if the mount is for "cgroup" - if numPostFields == 0 { - return "", fmt.Errorf("mountinfo: found no fields post '-' in %q", text) - } - - if postSeparatorFields[0] == "cgroup" { - // Check that the mount is properly formatted. - if numPostFields < 3 { - return "", fmt.Errorf("Error found less than 3 fields post '-' in %q", text) - } - - root = filepath.Dir(fields[4]) - break - } - } - if err := scanner.Err(); err != nil { - return "", err - } - if root == "" { - return "", errors.New("no cgroup mount found in mountinfo") - } - - if _, err := os.Stat(root); err != nil { - return "", err - } - - cgroupRoot = root - return cgroupRoot, nil -} +const cgroupRoot = "/sys/fs/cgroup" type cgroupData struct { - root string innerPath string config *configs.Cgroup pid int @@ -323,11 +264,6 @@ func (m *manager) GetAllPids() ([]int, error) { } func getCgroupData(c *configs.Cgroup, pid int) (*cgroupData, error) { - root, err := getCgroupRoot() - if err != nil { - return nil, err - } - if (c.Name != "" || c.Parent != "") && c.Path != "" { return nil, errors.New("cgroup: either Path or Name and Parent should be used") } @@ -343,7 +279,6 @@ func getCgroupData(c *configs.Cgroup, pid int) (*cgroupData, error) { } return &cgroupData{ - root: root, innerPath: innerPath, config: c, pid: pid, @@ -353,14 +288,14 @@ func getCgroupData(c *configs.Cgroup, pid int) (*cgroupData, error) { func (raw *cgroupData) path(subsystem string) (string, error) { // If the cgroup name/path is absolute do not look relative to the cgroup of the init process. if filepath.IsAbs(raw.innerPath) { - mnt, err := cgroups.FindCgroupMountpoint(raw.root, subsystem) + mnt, err := cgroups.FindCgroupMountpoint(cgroupRoot, subsystem) // If we didn't mount the subsystem, there is no point we make the path. if err != nil { return "", err } // Sometimes subsystems can be mounted together as 'cpu,cpuacct'. - return filepath.Join(raw.root, filepath.Base(mnt), raw.innerPath), nil + return filepath.Join(cgroupRoot, filepath.Base(mnt), raw.innerPath), nil } // Use GetOwnCgroupPath instead of GetInitCgroupPath, because the creating diff --git a/libcontainer/cgroups/fs/fs_test.go b/libcontainer/cgroups/fs/fs_test.go index 4971d547554..bc036d0a584 100644 --- a/libcontainer/cgroups/fs/fs_test.go +++ b/libcontainer/cgroups/fs/fs_test.go @@ -15,283 +15,74 @@ func TestInvalidCgroupPath(t *testing.T) { if cgroups.IsCgroup2UnifiedMode() { t.Skip("cgroup v2 is not supported") } - 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) { - if cgroups.IsCgroup2UnifiedMode() { - t.Skip("cgroup v2 is not supported") - } - 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) { - if cgroups.IsCgroup2UnifiedMode() { - t.Skip("cgroup v2 is not supported") - } - 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) { - if cgroups.IsCgroup2UnifiedMode() { - t.Skip("cgroup v2 is not supported") - } - 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) { - if cgroups.IsCgroup2UnifiedMode() { - t.Skip("cgroup v2 is not supported") - } - 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) { - if cgroups.IsCgroup2UnifiedMode() { - t.Skip("cgroup v2 is not supported") - } - 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) { - if cgroups.IsCgroup2UnifiedMode() { - t.Skip("cgroup v2 is not supported") - } - 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) { - if cgroups.IsCgroup2UnifiedMode() { - t.Skip("cgroup v2 is not supported") - } - 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!") + testCases := []struct { + test string + path, name, parent string + }{ + { + test: "invalid cgroup path", + path: "../../../../../../../../../../some/path", + }, + { + test: "invalid absolute cgroup path", + path: "/../../../../../../../../../../some/path", + }, + { + test: "invalid cgroup parent", + parent: "../../../../../../../../../../some/path", + name: "name", + }, + { + test: "invalid absolute cgroup parent", + parent: "/../../../../../../../../../../some/path", + name: "name", + }, + { + test: "invalid cgroup name", + parent: "parent", + name: "../../../../../../../../../../some/path", + }, + { + test: "invalid absolute cgroup name", + parent: "parent", + name: "/../../../../../../../../../../some/path", + }, + { + test: "invalid cgroup name and parent", + parent: "../../../../../../../../../../some/path", + name: "../../../../../../../../../../some/path", + }, + { + test: "invalid absolute cgroup name and parent", + parent: "/../../../../../../../../../../some/path", + name: "/../../../../../../../../../../some/path", + }, + } + + for _, tc := range testCases { + t.Run(tc.test, func(t *testing.T) { + config := &configs.Cgroup{Path: tc.path, Name: tc.name, Parent: tc.parent} + + data, err := getCgroupData(config, 0) + if err != nil { + t.Fatalf("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(cgroupRoot, "devices") + devicePath, err := data.path("devices") + if err != nil { + t.Fatalf("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/cgroups/fs/util_test.go b/libcontainer/cgroups/fs/util_test.go index 2c50d6f3a29..3745d06b612 100644 --- a/libcontainer/cgroups/fs/util_test.go +++ b/libcontainer/cgroups/fs/util_test.go @@ -39,8 +39,7 @@ func NewCgroupTestUtil(subsystem string, t *testing.T) *cgroupTestUtil { if err != nil { t.Fatal(err) } - d.root = tempDir - testCgroupPath := filepath.Join(d.root, subsystem) + testCgroupPath := filepath.Join(tempDir, subsystem) if err != nil { t.Fatal(err) }