mkdirall: verify that the newly created directory is sane

An attacker could swap out the directory we created between us creating
it and opening it. However, the worst thing they could do is force us to
make a directory inside a directory with different permissions or a
different mode. If they swap our directory with a directory that has the
same owner and permissions as the one we created, they didn't gain
anything. So, verify that the directory looks like the one we would've
created.

Unfortunately, there is no way to atomically create a directory and open
it. I discussed this with Al a while ago and I believe he said he didn't
like it (though I'm not sure if it was a blanket veto or not).

Signed-off-by: Aleksa Sarai <[email protected]>

mkdir_linux.go

@@ -9,6 +9,7 @@ package securejoin
 import (
 	"errors"
 	"fmt"
+	"io"
 	"os"
 	"path"
 	"path/filepath"
@@ -309,6 +310,8 @@ func partialOpen(root *os.File, unsafePath string) (_ *os.File, _ string, Err er
 	return currentDir, "", nil
 }
 
+var errPossibleAttack = errors.New("possible attack detected")
+
 // MkdirAllHandle is equivalent to MkdirAll, except that it is safer to use in
 // two respects:
 //
@@ -360,6 +363,27 @@ func MkdirAllHandle(root *os.File, unsafePath string, mode os.FileMode) (_ *os.F
 	// also removes the possibility of a parallel-write attack tricking us).
 	remainingPath = path.Join("/", filepath.ToSlash(remainingPath))
 
+	// 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())
+	)
+
 	// Create the remaining components.
 	for _, part := range strings.Split(remainingPath, "/") {
 		switch part {
@@ -400,6 +424,42 @@ func MkdirAllHandle(root *os.File, unsafePath string, mode os.FileMode) (_ *os.F
 		}
 		_ = 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)
+			}
+			// Re-open our O_PATH handle for the directory with O_RDONLY so we
+			// can check if it the directory is empty. This is safe to do with
+			// open(2) because there is no way for "." to be replaced or
+			// mounted over.
+			if dir, err := openatFile(currentDir, ".", unix.O_RDONLY|unix.O_NOFOLLOW|unix.O_DIRECTORY|unix.O_CLOEXEC, 0); err != nil {
+				return nil, fmt.Errorf("re-open newly created directory %q: %w", currentDir.Name(), err)
+			} else {
+				// We only need to check for a single entry, and we should get EOF
+				// if the directory is empty.
+				_, err := dir.Readdirnames(1)
+				_ = dir.Close()
+				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)
+				}
+			}
+		}
 	}
 	return currentDir, nil
 }

procfs_linux.go

@@ -346,6 +346,22 @@ func isDeadInode(file *os.File) error {
 	return nil
 }
 
+func getUmask() os.FileMode {
+	// 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 os.FileMode(umask)
+}
+
 func checkProcSelfFdPath(path string, file *os.File) error {
 	if err := isDeadInode(file); err != nil {
 		return err