Fix creation of subpath with SUID/SGID directories.

SafeMakeDir() should apply SUID/SGID/sticky bits to the directory it creates.
kubernetes · Mar 17, 2018 · 791a03c · 791a03c
1 parent 1ab6e8a
commit 791a03c
Show file tree

Hide file tree

Showing 2 changed files with 65 additions and 5 deletions.
diff --git a/pkg/util/mount/mount_linux.go b/pkg/util/mount/mount_linux.go
@@ -995,7 +995,19 @@ func doSafeMakeDir(pathname string, base string, perm os.FileMode) error {
 	// so user can read/write it. This is the behavior of previous code.
 	// TODO: chmod all created directories, not just the last one.
 	// parentFD is the last created directory.
-	if err = syscall.Fchmod(parentFD, uint32(perm)&uint32(os.ModePerm)); err != nil {
+
+	// Translate perm (os.FileMode) to uint32 that fchmod() expects
+	kernelPerm := uint32(perm & os.ModePerm)
+	if perm&os.ModeSetgid > 0 {
+		kernelPerm |= syscall.S_ISGID
+	}
+	if perm&os.ModeSetuid > 0 {
+		kernelPerm |= syscall.S_ISUID
+	}
+	if perm&os.ModeSticky > 0 {
+		kernelPerm |= syscall.S_ISVTX
+	}
+	if err = syscall.Fchmod(parentFD, kernelPerm); err != nil {
 		return fmt.Errorf("chmod %q failed: %s", currentPath, err)
 	}
 	return nil

diff --git a/pkg/util/mount/mount_linux_test.go b/pkg/util/mount/mount_linux_test.go
@@ -418,14 +418,15 @@ func TestPathWithinBase(t *testing.T) {
 }
 
 func TestSafeMakeDir(t *testing.T) {
-	defaultPerm := os.FileMode(0750)
+	defaultPerm := os.FileMode(0750) + os.ModeDir
 	tests := []struct {
 		name string
 		// Function that prepares directory structure for the test under given
 		// base.
 		prepare     func(base string) error
 		path        string
 		checkPath   string
+		perm        os.FileMode
 		expectError bool
 	}{
 		{
@@ -435,6 +436,37 @@ func TestSafeMakeDir(t *testing.T) {
 			},
 			"test/directory",
 			"test/directory",
+			defaultPerm,
+			false,
+		},
+		{
+			"directory-with-sgid",
+			func(base string) error {
+				return nil
+			},
+			"test/directory",
+			"test/directory",
+			os.FileMode(0777) + os.ModeDir + os.ModeSetgid,
+			false,
+		},
+		{
+			"directory-with-suid",
+			func(base string) error {
+				return nil
+			},
+			"test/directory",
+			"test/directory",
+			os.FileMode(0777) + os.ModeDir + os.ModeSetuid,
+			false,
+		},
+		{
+			"directory-with-sticky-bit",
+			func(base string) error {
+				return nil
+			},
+			"test/directory",
+			"test/directory",
+			os.FileMode(0777) + os.ModeDir + os.ModeSticky,
 			false,
 		},
 		{
@@ -444,6 +476,7 @@ func TestSafeMakeDir(t *testing.T) {
 			},
 			"test/directory",
 			"test/directory",
+			defaultPerm,
 			false,
 		},
 		{
@@ -453,6 +486,7 @@ func TestSafeMakeDir(t *testing.T) {
 			},
 			"",
 			"",
+			defaultPerm,
 			false,
 		},
 		{
@@ -462,6 +496,7 @@ func TestSafeMakeDir(t *testing.T) {
 			},
 			"..",
 			"",
+			defaultPerm,
 			true,
 		},
 		{
@@ -471,6 +506,7 @@ func TestSafeMakeDir(t *testing.T) {
 			},
 			"test/../../..",
 			"",
+			defaultPerm,
 			true,
 		},
 		{
@@ -483,6 +519,7 @@ func TestSafeMakeDir(t *testing.T) {
 			},
 			"test/directory",
 			"destination/directory",
+			defaultPerm,
 			false,
 		},
 		{
@@ -492,6 +529,7 @@ func TestSafeMakeDir(t *testing.T) {
 			},
 			"test/directory",
 			"",
+			defaultPerm,
 			true,
 		},
 		{
@@ -514,6 +552,7 @@ func TestSafeMakeDir(t *testing.T) {
 			},
 			"test1/dir/dir/dir/dir/dir/dir/dir/foo",
 			"test2/foo",
+			defaultPerm,
 			false,
 		},
 		{
@@ -523,6 +562,7 @@ func TestSafeMakeDir(t *testing.T) {
 			},
 			"test/directory",
 			"",
+			defaultPerm,
 			true,
 		},
 		{
@@ -532,6 +572,7 @@ func TestSafeMakeDir(t *testing.T) {
 			},
 			"test/directory",
 			"",
+			defaultPerm,
 			true,
 		},
 		{
@@ -541,6 +582,7 @@ func TestSafeMakeDir(t *testing.T) {
 			},
 			"test",
 			"",
+			defaultPerm,
 			true,
 		},
 		{
@@ -556,6 +598,7 @@ func TestSafeMakeDir(t *testing.T) {
 			},
 			"dir/test",
 			"",
+			defaultPerm,
 			false,
 		},
 		{
@@ -568,6 +611,7 @@ func TestSafeMakeDir(t *testing.T) {
 			},
 			"dir/test",
 			"",
+			defaultPerm,
 			true,
 		},
 		{
@@ -577,6 +621,7 @@ func TestSafeMakeDir(t *testing.T) {
 			},
 			"test/directory",
 			"",
+			defaultPerm,
 			true,
 		},
 	}
@@ -589,7 +634,7 @@ func TestSafeMakeDir(t *testing.T) {
 		}
 		test.prepare(base)
 		pathToCreate := filepath.Join(base, test.path)
-		err = doSafeMakeDir(pathToCreate, base, defaultPerm)
+		err = doSafeMakeDir(pathToCreate, base, test.perm)
 		if err != nil && !test.expectError {
 			t.Errorf("test %q: %s", test.name, err)
 		}
@@ -601,14 +646,17 @@ func TestSafeMakeDir(t *testing.T) {
 		}
 
 		if test.checkPath != "" {
-			if _, err := os.Stat(filepath.Join(base, test.checkPath)); err != nil {
+			st, err := os.Stat(filepath.Join(base, test.checkPath))
+			if err != nil {
 				t.Errorf("test %q: cannot read path %s", test.name, test.checkPath)
 			}
+			if st.Mode() != test.perm {
+				t.Errorf("test %q: expected permissions %o, got %o", test.name, test.perm, st.Mode())
+			}
 		}
 
 		os.RemoveAll(base)
 	}
-
 }
 
 func validateDirEmpty(dir string) error {