Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 19 additions & 40 deletions libcontainer/cgroups/fs/apply_raw.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,11 +94,10 @@ func getCgroupRoot() (string, error) {
}

type cgroupData struct {
root string
parent string
name string
config *configs.Cgroup
pid int
root string
innerPath string
config *configs.Cgroup
pid int
}

func (m *Manager) Apply(pid int) (err error) {
Expand Down Expand Up @@ -267,45 +266,26 @@ func getCgroupPath(c *configs.Cgroup) (string, error) {
return d.path("devices")
}

// pathClean makes a path safe for use with filepath.Join. This is done by not
// only cleaning the path, but also (if the path is relative) adding a leading
// '/' and cleaning it (then removing the leading '/'). This ensures that a
// path resulting from prepending another path will always resolve to lexically
// 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 pathClean.
func pathClean(path string) string {
// Ensure that all paths are cleaned (especially problematic ones like
// "/../../../../../" which can cause lots of issues).
path = filepath.Clean(path)

// If the path isn't absolute, we need to do more processing to fix paths
// such as "../../../../<etc>/some/path". We also shouldn't convert absolute
// paths to relative ones.
if !filepath.IsAbs(path) {
path = filepath.Clean(string(os.PathSeparator) + path)
// This can't fail, as (by definition) all paths are relative to root.
path, _ = filepath.Rel(string(os.PathSeparator), path)
}

// Clean the path again for good measure.
return filepath.Clean(path)
}

func getCgroupData(c *configs.Cgroup, pid int) (*cgroupData, error) {
root, err := getCgroupRoot()
if err != nil {
return nil, err
}

// Clean the parent slice path.
c.Parent = pathClean(c.Parent)
if (c.Name != "" || c.Parent != "") && c.Path != "" {
return nil, fmt.Errorf("cgroup: either Path or Name and Parent should be used")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NACK on removing this. It fixes a security vulnerability. Please use this function to clean paths that need to be scoped to a mountpoint, not filepath.Clean.

EDIT: Ah, you've just moved it. Still NACK, moving it means that Docker will have a security regression.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should have noted that, yes.

What about making it a public method within the libcontainer/utils package?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM. There are probably some other places where we aren't cleaning paths properly.

}

innerPath := c.Path
if innerPath == "" {
innerPath = filepath.Join(c.Parent, c.Name)
}

return &cgroupData{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If cgroup data is not used between systemd and fs we could remove the name and parent fields here and only hold the joined "path" if that makes sense

root: root,
parent: c.Parent,
name: c.Name,
config: c,
pid: pid,
root: root,
innerPath: c.Path,
config: c,
pid: pid,
}, nil
}

Expand Down Expand Up @@ -333,19 +313,18 @@ func (raw *cgroupData) path(subsystem string) (string, error) {
return "", err
}

cgPath := filepath.Join(raw.parent, raw.name)
// If the cgroup name/path is absolute do not look relative to the cgroup of the init process.
if filepath.IsAbs(cgPath) {
if filepath.IsAbs(raw.innerPath) {
// Sometimes subsystems can be mounted togethger as 'cpu,cpuacct'.
return filepath.Join(raw.root, filepath.Base(mnt), cgPath), nil
return filepath.Join(raw.root, filepath.Base(mnt), raw.innerPath), nil
}

parentPath, err := raw.parentPath(subsystem, mnt, root)
if err != nil {
return "", err
}

return filepath.Join(parentPath, cgPath), nil
return filepath.Join(parentPath, raw.innerPath), nil
}

func (raw *cgroupData) join(subsystem string) (string, error) {
Expand Down
3 changes: 2 additions & 1 deletion libcontainer/cgroups/fs/cpuset.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (

"github.com/opencontainers/runc/libcontainer/cgroups"
"github.com/opencontainers/runc/libcontainer/configs"
libcontainerUtils "github.com/opencontainers/runc/libcontainer/utils"
)

type CpusetGroup struct {
Expand Down Expand Up @@ -88,7 +89,7 @@ func (s *CpusetGroup) getSubsystemSettings(parent string) (cpus []byte, mems []b
// it's parent.
func (s *CpusetGroup) ensureParent(current, root string) error {
parent := filepath.Dir(current)
if filepath.Clean(parent) == root {
if libcontainerUtils.CleanPath(parent) == root {
return nil
}
// Avoid infinite recursion.
Expand Down
15 changes: 11 additions & 4 deletions libcontainer/configs/cgroup_unix.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,22 @@ const (
)

type Cgroup struct {
Name string `json:"name"`
// Deprecated, use Path instead
Name string `json:"name,omitempty"`

// name of parent cgroup or slice
Parent string `json:"parent"`
// name of parent of cgroup or slice
// Deprecated, use Path instead
Parent string `json:"parent,omitempty"`

// Path specifies the path to cgroups that are created and/or joined by the container.
// The path is assumed to be relative to the host system cgroup mountpoint.
Path string `json:"path"`

// ScopePrefix decribes prefix for the scope name
ScopePrefix string `json:"scope_prefix"`

// Paths represent the cgroups paths to join
// Paths represent the absolute cgroups paths to join.
// This takes precedence over Path.
Paths map[string]string

// Resources contains various cgroups settings to apply
Expand Down
3 changes: 1 addition & 2 deletions libcontainer/integration/template_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,7 @@ func newTemplateConfig(rootfs string) *configs.Config {
{Type: configs.NEWNET},
}),
Cgroups: &configs.Cgroup{
Name: "test",
Parent: "integration",
Path: "integration/test",
Resources: &configs.Resources{
MemorySwappiness: -1,
AllowAllDevices: false,
Expand Down
3 changes: 2 additions & 1 deletion libcontainer/rootfs_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"github.com/opencontainers/runc/libcontainer/configs"
"github.com/opencontainers/runc/libcontainer/label"
"github.com/opencontainers/runc/libcontainer/system"
libcontainerUtils "github.com/opencontainers/runc/libcontainer/utils"
)

const defaultMountFlags = syscall.MS_NOEXEC | syscall.MS_NOSUID | syscall.MS_NODEV
Expand Down Expand Up @@ -294,7 +295,7 @@ func getCgroupMounts(m *configs.Mount) ([]*configs.Mount, error) {
// checkMountDestination checks to ensure that the mount destination is not over the top of /proc.
// 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) {
if libcontainerUtils.CleanPath(rootfs) == libcontainerUtils.CleanPath(dest) {
return fmt.Errorf("mounting into / is prohibited")
}
invalidDestinations := []string{
Expand Down
25 changes: 25 additions & 0 deletions libcontainer/utils/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"encoding/hex"
"encoding/json"
"io"
"os"
"path/filepath"
"syscall"
)
Expand Down Expand Up @@ -54,3 +55,27 @@ func WriteJSON(w io.Writer, v interface{}) error {
_, err = w.Write(data)
return err
}

// CleanPath makes a path safe for use with filepath.Join. This is done by not
// only cleaning the path, but also (if the path is relative) adding a leading
// '/' and cleaning it (then removing the leading '/'). This ensures that a
// path resulting from prepending another path will always resolve to lexically
// 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 {
// Ensure that all paths are cleaned (especially problematic ones like
// "/../../../../../" which can cause lots of issues).
path = filepath.Clean(path)

// If the path isn't absolute, we need to do more processing to fix paths
// such as "../../../../<etc>/some/path". We also shouldn't convert absolute
// paths to relative ones.
if !filepath.IsAbs(path) {
path = filepath.Clean(string(os.PathSeparator) + path)
// This can't fail, as (by definition) all paths are relative to root.
path, _ = filepath.Rel(string(os.PathSeparator), path)
}

// Clean the path again for good measure.
return filepath.Clean(path)
}
20 changes: 15 additions & 5 deletions spec.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"github.com/opencontainers/runc/libcontainer/cgroups"
"github.com/opencontainers/runc/libcontainer/configs"
"github.com/opencontainers/runc/libcontainer/seccomp"
libcontainerUtils "github.com/opencontainers/runc/libcontainer/utils"
"github.com/opencontainers/specs"
)

Expand Down Expand Up @@ -326,13 +327,22 @@ func createLibcontainerMount(cwd string, m specs.Mount) *configs.Mount {
}

func createCgroupConfig(name string, spec *specs.LinuxSpec) (*configs.Cgroup, error) {
myCgroupPath, err := cgroups.GetThisCgroupDir("devices")
if err != nil {
return nil, err
var (
err error
myCgroupPath string
)

if spec.Linux.CgroupsPath != nil {
myCgroupPath = libcontainerUtils.CleanPath(*spec.Linux.CgroupsPath)
} else {
myCgroupPath, err = cgroups.GetThisCgroupDir("devices")
if err != nil {
return nil, err
}
}

c := &configs.Cgroup{
Name: name,
Parent: myCgroupPath,
Path: filepath.Join(myCgroupPath, name),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need to do a join here with name right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it depends, if I have a bundle named busybox and set my cgroup path to be foo/bar

Do we want to use <cgroup-mount-point>/foo/bar/busybox or <cgroup-mount-point>/foo/bar when setting the rules?

Atm the code produce the former; if we want the later, I'm not sure what is the use for name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

Resources: &configs.Resources{},
}
c.Resources.AllowedDevices = allowedDevices
Expand Down