Merge pull request #896 from HusterWan/zr/fix-cgroup-parent

bugfix: cgroup-parent must be abs
AliyunContainerService · Mar 15, 2018 · a6e3da4 · a6e3da4
2 parents 3391aa8 + 290cfcb
commit a6e3da4
Show file tree

Hide file tree

Showing 2 changed files with 22 additions and 1 deletion.
diff --git a/daemon/mgr/container.go b/daemon/mgr/container.go
@@ -486,7 +486,12 @@ func (mgr *ContainerManager) Start(ctx context.Context, id, detachKeys string) (
 		cgroupsParent = mgr.Config.CgroupParent
 	}
 
+	// cgroupsPath must be absolute path
 	if cgroupsParent != "" {
+		if !filepath.IsAbs(cgroupsParent) {
+			cgroupsParent = filepath.Clean("/" + cgroupsParent)
+		}
+
 		s.Linux.CgroupsPath = filepath.Join(cgroupsParent, c.ID())
 	}
 

diff --git a/test/cli_run_test.go b/test/cli_run_test.go
@@ -732,7 +732,7 @@ func testRunWithCgroupParent(c *check.C, cgroupParent, name string) {
 
 	file := "/sys/fs/cgroup/memory/" + cgroupParent + "/" + containerID + "/memory.limit_in_bytes"
 	if _, err := os.Stat(file); err != nil {
-		c.Fatalf("container %s cgroup mountpoint not exists", containerID)
+		c.Fatalf("container %s cgroup mountpoint not exists", name)
 	}
 
 	out, err := exec.Command("cat", file).Output()
@@ -745,3 +745,19 @@ func testRunWithCgroupParent(c *check.C, cgroupParent, name string) {
 	}
 
 }
+
+// TestRunInvalidCgroupParent checks that a specially-crafted cgroup parent doesn't cause Docker to crash or start modifying /.
+func (suite *PouchRunSuite) TestRunInvalidCgroupParent(c *check.C) {
+	testRunInvalidCgroupParent(c, "../../../../../../../../SHOULD_NOT_EXIST", "SHOULD_NOT_EXIST", "cgroup-invalid-test")
+
+	testRunInvalidCgroupParent(c, "/../../../../../../../../SHOULD_NOT_EXIST", "/SHOULD_NOT_EXIST", "cgroup-absolute-invalid-test")
+}
+
+func testRunInvalidCgroupParent(c *check.C, cgroupParent, cleanCgroupParent, name string) {
+	command.PouchRun("run", "-m", "300M", "--cgroup-parent", cgroupParent, "--name", name, busyboxImage, "cat", "/proc/self/cgroup").Assert(c, icmd.Success)
+
+	// We expect "/SHOULD_NOT_EXIST" to not exist. If not, we have a security issue.
+	if _, err := os.Stat("/SHOULD_NOT_EXIST"); err == nil || !os.IsNotExist(err) {
+		c.Fatalf("SECURITY: --cgroup-parent with ../../ relative paths cause files to be created in the host (this is bad) !!")
+	}
+}