mkdirall: correctly handle sgid directory parent

S_ISGID is applied to directories created underneath it and affects the
gid of all inodes created under a directory with it set. We need to
handle this properly in order to avoid errors from our mode/owner
validation logic.

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

CHANGELOG.md

@@ -15,6 +15,13 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
   users thinking their code sets these bits when it doesn't. Programs that need
   to deal with compatibility can mask the bits themselves. (#23, #25)
 
+## Fixes ##
+- If a directory has `S_ISGID` set, then all child directories will have
+  `S_ISGID` set when created and a different gid will be used for any inode
+  created under the directory. Previously, the "expected owner and mode"
+  validation in `securejoin.MkdirAll` did not correctly handle this. We now
+  correctly handle this case. (#24, #25)
+
 ## [0.3.1] - 2024-07-23 ##
 
 ### Changed ###

mkdir_linux.go

@@ -127,6 +127,17 @@ func MkdirAllHandle(root *os.File, unsafePath string, mode int) (_ *os.File, Err
 		expectedGid = uint32(unix.Getegid())
 	)
 
+	// The setgid bit (S_ISGID = 0o2000) is inherited to child directories and
+	// affects the group of any inodes created in said directory, so if the
+	// starting directory has it set we need to adjust our expected mode and
+	// owner to match.
+	if st, err := fstatFile(currentDir); err != nil {
+		return nil, fmt.Errorf("failed to stat starting path for mkdir %q: %w", currentDir.Name(), err)
+	} else if st.Mode&unix.S_ISGID == unix.S_ISGID {
+		expectedMode |= unix.S_ISGID
+		expectedGid = st.Gid
+	}
+
 	// Create the remaining components.
 	for _, part := range remainingParts {
 		switch part {

mkdir_linux_test.go

@@ -54,7 +54,7 @@ var mkdirAll_MkdirAllHandle mkdirAllFunc = func(t *testing.T, root, unsafePath s
 	return nil
 }
 
-func checkMkdirAll(t *testing.T, partialLookupFn partialLookupFunc, root, unsafePath string, mode, expectedMode int, expectedErr error) {
+func checkMkdirAll(t *testing.T, mkdirAll mkdirAllFunc, root, unsafePath string, mode, expectedMode int, expectedErr error) {
 	rootDir, err := os.OpenFile(root, unix.O_PATH|unix.O_DIRECTORY|unix.O_CLOEXEC, 0)
 	require.NoError(t, err)
 	defer rootDir.Close()
@@ -73,9 +73,6 @@ func checkMkdirAll(t *testing.T, partialLookupFn partialLookupFunc, root, unsafe
 		}
 	}()
 
-	// This mode is different to the one set up by createTree.
-	const expectedMode = 0o711
-
 	// Actually make the tree.
 	err = mkdirAll(t, root, unsafePath, mode)
 	assert.ErrorIsf(t, err, expectedErr, "MkdirAll(%q, %q)", root, unsafePath)
@@ -128,6 +125,9 @@ func testMkdirAll_Basic(t *testing.T, mkdirAll mkdirAllFunc) {
 		// Symlink loop.
 		"dir loop",
 		"symlink loop/link ../loop/link",
+		// S_ISGID directory.
+		"dir sgid-self ::2755",
+		"dir sgid-sticky-self ::3755",
 	}
 
 	withWithoutOpenat2(t, true, func(t *testing.T) {
@@ -182,13 +182,14 @@ func testMkdirAll_Basic(t *testing.T, mkdirAll mkdirAllFunc) {
 			"loop-basic":    {unsafePath: "loop/link/foo", expectedErr: unix.ELOOP},
 			"loop-dotdot":   {unsafePath: "loop/link/../foo", expectedErr: unix.ELOOP},
 			// Make sure the S_ISGID handling is correct.
-			"sgid-self": {unsafePath: "sgid-self/"}
+			"sgid-dir-ownedbyus":        {unsafePath: "sgid-self/foo/bar/baz", expectedModeBits: unix.S_ISGID},
+			"sgid-sticky-dir-ownedbyus": {unsafePath: "sgid-sticky-self/foo/bar/baz", expectedModeBits: unix.S_ISGID},
 		} {
 			test := test // copy iterator
 			t.Run(name, func(t *testing.T) {
 				root := createTree(t, tree...)
 				const mode = 0o711
-				checkMkdirAll(t, mkdirAll, root, test.unsafePath, mode, test.expectedModeBits|mode)
+				checkMkdirAll(t, mkdirAll, root, test.unsafePath, mode, test.expectedModeBits|mode, test.expectedErr)
 			})
 		}
 	})
@@ -202,7 +203,49 @@ func TestMkdirAllHandle_Basic(t *testing.T) {
 	testMkdirAll_Basic(t, mkdirAll_MkdirAllHandle)
 }
 
-func testMkdirAll_InvalidMode(t *testing.T, mkdirAll func(t *testing.T, root, unsafePath string, mode int) error) {
+func testMkdirAll_AsRoot(t *testing.T, mkdirAll mkdirAllFunc) {
+	requireRoot(t) // chown
+
+	// We create a new tree for each test, but the template is the same.
+	tree := []string{
+		// S_ISGID directories.
+		"dir sgid-self ::2755",
+		"dir sgid-other 1000:1000:2755",
+		"dir sgid-sticky-self ::3755",
+		"dir sgid-sticky-other 1000:1000:3755",
+	}
+
+	withWithoutOpenat2(t, true, func(t *testing.T) {
+		for name, test := range map[string]struct {
+			unsafePath       string
+			expectedErr      error
+			expectedModeBits int
+		}{
+			// Make sure the S_ISGID handling is correct.
+			"sgid-dir-ownedbyus":           {unsafePath: "sgid-self/foo/bar/baz", expectedModeBits: unix.S_ISGID},
+			"sgid-dir-ownedbyother":        {unsafePath: "sgid-other/foo/bar/baz", expectedModeBits: unix.S_ISGID},
+			"sgid-sticky-dir-ownedbyus":    {unsafePath: "sgid-sticky-self/foo/bar/baz", expectedModeBits: unix.S_ISGID},
+			"sgid-sticky-dir-ownedbyother": {unsafePath: "sgid-sticky-other/foo/bar/baz", expectedModeBits: unix.S_ISGID},
+		} {
+			test := test // copy iterator
+			t.Run(name, func(t *testing.T) {
+				root := createTree(t, tree...)
+				const mode = 0o711
+				checkMkdirAll(t, mkdirAll, root, test.unsafePath, mode, test.expectedModeBits|mode, test.expectedErr)
+			})
+		}
+	})
+}
+
+func TestMkdirAll_AsRoot(t *testing.T) {
+	testMkdirAll_AsRoot(t, mkdirAll_MkdirAll)
+}
+
+func TestMkdirAllHandle_AsRoot(t *testing.T) {
+	testMkdirAll_AsRoot(t, mkdirAll_MkdirAllHandle)
+}
+
+func testMkdirAll_InvalidMode(t *testing.T, mkdirAll mkdirAllFunc) {
 	for _, test := range []struct {
 		mode        int
 		expectedErr error

openat_linux.go

@@ -42,6 +42,10 @@ func fstatatFile(dir *os.File, path string, flags int) (unix.Stat_t, error) {
 	return stat, nil
 }
 
+func fstatFile(fd *os.File) (unix.Stat_t, error) {
+	return fstatatFile(fd, "", unix.AT_EMPTY_PATH)
+}
+
 func readlinkatFile(dir *os.File, path string) (string, error) {
 	size := 4096
 	for {