mkdir: do not return errors for incorrect directory modes or owners

We've had several examples of unexpected semantics with how modes are
calculated, and there will likely be many more in the future. In
addition, mounting filesystems like vfat with mount options that mess
with ownership (like "uid=1234,gid=5678,umask=0") will result in
unexpected behaviour that would be very difficult to emulate.

To avoid further regressions, just remove the checks entirely. In theory
we could switch to adding warnings, but there's no real benefit IMHO.
The semantics of MkdirAll are quite loose already so arguably there is
no practical difference between re-using a directory that already
existed and being tricked into opening an intermediate directory you
didn't create.

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

Author: cyphar

CHANGELOG.md

@@ -6,6 +6,12 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ## [Unreleased] ##
 
+### Fixed ###
+- The mode and owner verification logic in `MkdirAll` has been removed. This
+  was originally intended to protect against some theoretical attacks but upon
+  further consideration these protections don't actually buy us anything and
+  they were causing spurious errors with more complicated filesystem setups.
+
 ## [0.3.2] - 2024-09-13 ##
 
 ### Changed ###

mkdir_linux.go

@@ -108,35 +108,6 @@ func MkdirAllHandle(root *os.File, unsafePath string, mode int) (_ *os.File, Err
 
 	// Make sure the mode doesn't have any type bits.
 	mode &^= unix.S_IFMT
-	// What properties do we expect any newly created directories to have?
-	var (
-		// While umask(2) is a per-thread property, and thus this value could
-		// vary between threads, a functioning Go program would LockOSThread
-		// threads with different umasks and so we don't need to LockOSThread
-		// for this entire mkdirat loop (if we are in the locked thread with a
-		// different umask, we are already locked and there's nothing for us to
-		// do -- and if not then it doesn't matter which thread we run on and
-		// there's nothing for us to do).
-		expectedMode = uint32(unix.S_IFDIR | (mode &^ getUmask()))
-
-		// We would want to get the fs[ug]id here, but we can't access those
-		// from userspace. In practice, nobody uses setfs[ug]id() anymore, so
-		// just use the effective [ug]id (which is equivalent to the fs[ug]id
-		// for programs that don't use setfs[ug]id).
-		expectedUid = uint32(unix.Geteuid())
-		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 {
@@ -175,35 +146,33 @@ func MkdirAllHandle(root *os.File, unsafePath string, mode int) (_ *os.File, Err
 		_ = currentDir.Close()
 		currentDir = nextDir
 
-		// Make sure that the directory matches what we expect. An attacker
-		// could have swapped the directory between us making it and opening
-		// it. There's no way for us to be sure that the directory is
-		// _precisely_ the same as the directory we created, but if we are in
-		// an empty directory with the same owner and mode as the one we
-		// created then there is nothing the attacker could do with this new
-		// directory that they couldn't do with the old one.
-		if stat, err := fstat(currentDir); err != nil {
-			return nil, fmt.Errorf("check newly created directory: %w", err)
-		} else {
-			if stat.Mode != expectedMode {
-				return nil, fmt.Errorf("%w: newly created directory %q has incorrect mode 0o%.3o (expected 0o%.3o)", errPossibleAttack, currentDir.Name(), stat.Mode, expectedMode)
-			}
-			if stat.Uid != expectedUid || stat.Gid != expectedGid {
-				return nil, fmt.Errorf("%w: newly created directory %q has incorrect owner %d:%d (expected %d:%d)", errPossibleAttack, currentDir.Name(), stat.Uid, stat.Gid, expectedUid, expectedGid)
-			}
-			// Check that the directory is empty. We only need to check for
-			// a single entry, and we should get EOF if the directory is
-			// empty.
-			_, err := currentDir.Readdirnames(1)
-			if !errors.Is(err, io.EOF) {
-				if err == nil {
-					err = fmt.Errorf("%w: newly created directory %q is non-empty", errPossibleAttack, currentDir.Name())
-				}
-				return nil, fmt.Errorf("check if newly created directory %q is empty: %w", currentDir.Name(), err)
+		// Try our best to check that the directory is empty and so is unlikely
+		// to have been swapped by an attacker.
+		//
+		// Ideally we would also check that the owner and mode match what we
+		// would've created -- unfortunately, it is non-trivial to verify that
+		// the owner and mode of the created directory match. While plain Unix
+		// DAC rules seem simple enough to emulate, there are a bunch of other
+		// factors that can change the mode or owner of created directories
+		// (default POSIX ACLs, mount options like uid=1,gid=2,umask=0 on
+		// filesystems like vfat, etc etc). We used to try to verify this but
+		// it just lead to a series of spurious errors.
+		//
+		// To be honest, since MkdirAll allows you to use existing directories,
+		// the practical scope of this protection seems very limited (if it
+		// even exists) so it really isn't that important.
+
+		// We only need to check for a single entry to see if it's empty, and
+		// we should get EOF if the directory is empty.
+		_, err := currentDir.Readdirnames(1)
+		if !errors.Is(err, io.EOF) {
+			if err == nil {
+				err = fmt.Errorf("%w: newly created directory %q is non-empty", errPossibleAttack, currentDir.Name())
 			}
-			// Reset the offset.
-			_, _ = currentDir.Seek(0, unix.SEEK_SET)
+			return nil, fmt.Errorf("check if newly created directory %q is empty: %w", currentDir.Name(), err)
 		}
+		// Reset the offset.
+		_, _ = currentDir.Seek(0, unix.SEEK_SET)
 	}
 	return currentDir, nil
 }

mkdir_linux_test.go

@@ -314,12 +314,9 @@ func (m *racingMkdirMeta) checkMkdirAllHandle_Racing(t *testing.T, root, unsafeP
 	}
 	defer handle.Close()
 
-	// Make sure the handle has the right owner/mode.
-	unixStat, err := fstat(handle)
-	require.NoError(t, err, "stat mkdirall handle")
-	assert.Equal(t, uint32(unix.S_IFDIR|mode), unixStat.Mode, "mkdirall handle mode")
-	assert.Equal(t, uint32(unix.Geteuid()), unixStat.Uid, "mkdirall handle uid")
-	assert.Equal(t, uint32(unix.Getegid()), unixStat.Gid, "mkdirall handle gid")
+	// It's possible for an attacker to have swapped the final directory, but
+	// this is okay because MkdirAll will use pre-existing directories anyway.
+	// So there's no need to check the returned handle.
 	// TODO: Does it make sense to even try to check the handle path?
 	m.passOkCount++
 }
@@ -346,8 +343,8 @@ func TestMkdirAllHandle_RacingRename(t *testing.T) {
 
 		tests := []test{
 			{"good", "target/a/b/c/d/e", "swapdir-empty-ok", "target/a/b/c/d/e/f/g/h/i/j/k", nil},
-			{"trailing", "target/a/b/c/d/e", "swapdir-empty-badmode", "target/a/b/c/d/e", []error{errPossibleAttack}},
-			{"partial", "target/a/b/c/d/e", "swapdir-empty-badmode", "target/a/b/c/d/e/f/g/h/i/j/k", []error{errPossibleAttack}},
+			{"trailing", "target/a/b/c/d/e", "swapdir-empty-badmode", "target/a/b/c/d/e", nil},
+			{"partial", "target/a/b/c/d/e", "swapdir-empty-badmode", "target/a/b/c/d/e/f/g/h/i/j/k", nil},
 			{"trailing", "target/a/b/c/d/e", "swapdir-nonempty1", "target/a/b/c/d/e", []error{errPossibleAttack}},
 			{"partial", "target/a/b/c/d/e", "swapdir-nonempty1", "target/a/b/c/d/e/f/g/h/i/j/k", []error{errPossibleAttack}},
 			{"trailing", "target/a/b/c/d/e", "swapdir-nonempty2", "target/a/b/c/d/e", []error{errPossibleAttack}},
@@ -364,12 +361,12 @@ func TestMkdirAllHandle_RacingRename(t *testing.T) {
 				"dir swapdir-empty-badowner3 111:222:0711",
 			)
 			tests = append(tests, []test{
-				{"trailing", "target/a/b/c/d/e", "swapdir-empty-badowner1", "target/a/b/c/d/e", []error{errPossibleAttack}},
-				{"partial", "target/a/b/c/d/e", "swapdir-empty-badowner1", "target/a/b/c/d/e/f/g/h/i/j/k", []error{errPossibleAttack}},
-				{"trailing", "target/a/b/c/d/e", "swapdir-empty-badowner2", "target/a/b/c/d/e", []error{errPossibleAttack}},
-				{"partial", "target/a/b/c/d/e", "swapdir-empty-badowner2", "target/a/b/c/d/e/f/g/h/i/j/k", []error{errPossibleAttack}},
-				{"trailing", "target/a/b/c/d/e", "swapdir-empty-badowner3", "target/a/b/c/d/e", []error{errPossibleAttack}},
-				{"partial", "target/a/b/c/d/e", "swapdir-empty-badowner3", "target/a/b/c/d/e/f/g/h/i/j/k", []error{errPossibleAttack}},
+				{"trailing", "target/a/b/c/d/e", "swapdir-empty-badowner1", "target/a/b/c/d/e", nil},
+				{"partial", "target/a/b/c/d/e", "swapdir-empty-badowner1", "target/a/b/c/d/e/f/g/h/i/j/k", nil},
+				{"trailing", "target/a/b/c/d/e", "swapdir-empty-badowner2", "target/a/b/c/d/e", nil},
+				{"partial", "target/a/b/c/d/e", "swapdir-empty-badowner2", "target/a/b/c/d/e/f/g/h/i/j/k", nil},
+				{"trailing", "target/a/b/c/d/e", "swapdir-empty-badowner3", "target/a/b/c/d/e", nil},
+				{"partial", "target/a/b/c/d/e", "swapdir-empty-badowner3", "target/a/b/c/d/e/f/g/h/i/j/k", nil},
 			}...)
 		}
 

procfs_linux.go

@@ -411,22 +411,6 @@ func isDeadInode(file *os.File) error {
 	return nil
 }
 
-func getUmask() int {
-	// umask is a per-thread property, but it is inherited by children, so we
-	// need to lock our OS thread to make sure that no other goroutine runs in
-	// this thread and no goroutines are spawned from this thread until we
-	// revert to the old umask.
-	//
-	// We could parse /proc/self/status to avoid this get-set problem, but
-	// /proc/thread-self requires LockOSThread anyway, so there's no real
-	// benefit over just using umask(2).
-	runtime.LockOSThread()
-	umask := unix.Umask(0)
-	unix.Umask(umask)
-	runtime.UnlockOSThread()
-	return umask
-}
-
 func checkProcSelfFdPath(path string, file *os.File) error {
 	if err := isDeadInode(file); err != nil {
 		return err