fixup! initial safe MkdirAll implementation

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

mkdir_linux.go

@@ -92,13 +92,14 @@ func partialOpen(root, unsafePath string) (_ *os.File, _ string, Err error) {
 		nextDir, err := openatFile(currentDir, part, unix.O_PATH|unix.O_NOFOLLOW|unix.O_DIRECTORY|unix.O_CLOEXEC, 0)
 		switch {
 		case err == nil:
-			// If we are operating on a .., make sure we haven't escaped. We
-			// only have to check for ".." here because walking down into a
-			// regular component component cannot cause you to escape. This
-			// mirrors the logic in RESOLVE_IN_ROOT, except we have to check
-			// every ".." rather than only checking after a rename or mount on
-			// the system.
 			if part == ".." {
+				// If we are operating on a .., make sure we haven't escaped. We
+				// only have to check for ".." here because walking down into a
+				// regular component component cannot cause you to escape. This
+				// mirrors the logic in RESOLVE_IN_ROOT, except we have to check
+				// every ".." rather than only checking after a rename or mount on
+				// the system.
+
 				// Make sure the root hasn't moved.
 				if err := checkProcSelfFdPath(logicalRootPath, rootDir); err != nil {
 					return nil, "", fmt.Errorf("root path moved during lookup: %w", err)
@@ -182,10 +183,26 @@ func MkdirAll(root, unsafePath string, mode os.FileMode) error {
 	// Try to open as much of the path as possible.
 	currentDir, remainingPath, err := partialOpen(root, unsafePath)
 	if err != nil {
-		return fmt.Errorf("find existing subpath: %w", err)
+		return fmt.Errorf("find existing subpath of %q: %w", unsafePath, err)
 	}
 	defer currentDir.Close()
 
+	// If there is an attacker deleting directories as we walk into them,
+	// detect this proactively. Note this is guaranteed to detect if the
+	// attacker deleted any part of the tree up to currentDir.
+	//
+	// Once we walk into a dead directory, partialOpen would not be able to
+	// walk further down the tree (directories must be empty before they are
+	// deleted), and if the attacker has removed the entire tree we can be sure
+	// that anything that was originally inside a dead directory must also be
+	// deleted and thus is a dead directory in its own right.
+	//
+	// This is mostly a quality-of-life check, because mkdir will simply fail
+	// later if the attacker deletes the tree after this check.
+	if err := isDeadInode(currentDir); err != nil {
+		return fmt.Errorf("finding existing subpath of %q: %w", unsafePath, err)
+	}
+
 	// The remaining path can be cleaned because none of the path components
 	// exist and thus there are no symlinks to worry about (and removing ..
 	// also removes the possibility of a parallel-write attack tricking us).
@@ -207,7 +224,12 @@ func MkdirAll(root, unsafePath string, mode os.FileMode) error {
 		// create the finaly component without worrying about symlink-exchange
 		// attacks.
 		if err := unix.Mkdirat(int(currentDir.Fd()), part, uint32(mode)); err != nil {
-			return &os.PathError{Op: "mkdirat", Path: currentDir.Name() + "/" + part, Err: err}
+			err = &os.PathError{Op: "mkdirat", Path: currentDir.Name() + "/" + part, Err: err}
+			// Make the error a bit nicer if the directory is dead.
+			if err2 := isDeadInode(currentDir); err2 != nil {
+				err = fmt.Errorf("%w (%w)", err, err2)
+			}
+			return err
 		}
 
 		// Get a handle to the next component.

procfs_linux.go

@@ -23,18 +23,34 @@ const (
 	PROC_ROOT_INO    = 1
 )
 
-func verifyProcRoot(procRoot *os.File) error {
+func fstat(f *os.File) (unix.Stat_t, error) {
 	var stat unix.Stat_t
-	if err := unix.Fstat(int(procRoot.Fd()), &stat); err != nil {
-		return &os.PathError{Op: "fstat", Path: procRoot.Name(), Err: err}
+	if err := unix.Fstat(int(f.Fd()), &stat); err != nil {
+		return stat, &os.PathError{Op: "fstat", Path: f.Name(), Err: err}
+	}
+	return stat, nil
+}
+
+func fstatfs(f *os.File) (unix.Statfs_t, error) {
+	var statfs unix.Statfs_t
+	if err := unix.Fstatfs(int(f.Fd()), &statfs); err != nil {
+		return statfs, &os.PathError{Op: "fstatfs", Path: f.Name(), Err: err}
+	}
+	return statfs, nil
+}
+
+func verifyProcRoot(procRoot *os.File) error {
+	stat, err := fstat(procRoot)
+	if err != nil {
+		return err
 	}
 	if stat.Ino != PROC_ROOT_INO {
 		return fmt.Errorf("%w: incorrect procfs root inode number %d", errUnsafeProcfs, stat.Ino)
 	}
 
-	var statfs unix.Statfs_t
-	if err := unix.Fstatfs(int(procRoot.Fd()), &statfs); err != nil {
-		return &os.PathError{Op: "fstatfs", Path: procRoot.Name(), Err: err}
+	statfs, err := fstatfs(procRoot)
+	if err != nil {
+		return err
 	}
 	if statfs.Type != PROC_SUPER_MAGIC {
 		return fmt.Errorf("%w: incorrect procfs root filesystem type 0x%x", errUnsafeProcfs, statfs.Type)
@@ -192,9 +208,9 @@ func doGetProcSelfFd() (*os.File, error) {
 		// We can't detect bind-mounts of different parts of procfs on top of
 		// /proc (a-la RESOLVE_NO_XDEV), but we can at least be sure that we
 		// aren't on the wrong filesystem here.
-		var statfs unix.Statfs_t
-		if err := unix.Fstatfs(int(handle.Fd()), &statfs); err != nil {
-			return nil, &os.PathError{Op: "fstatfs", Path: handle.Name(), Err: err}
+		statfs, err := fstatfs(handle)
+		if err != nil {
+			return nil, err
 		}
 		if statfs.Type != PROC_SUPER_MAGIC {
 			return nil, fmt.Errorf("%w: incorrect /proc/self/fd filesystem type 0x%x", errUnsafeProcfs, statfs.Type)
@@ -222,9 +238,34 @@ func procSelfFdReadlink(f *os.File) (string, error) {
 	return readlinkatFile(procSelfFd, strconv.Itoa(int(f.Fd())))
 }
 
-var errPossibleBreakout = errors.New("possible breakout detected")
+var (
+	errPossibleBreakout = errors.New("possible breakout detected")
+	errInvalidDirectory = errors.New("wandered into deleted directory")
+	errDeletedInode     = errors.New("cannot verify path of deleted inode")
+)
+
+func isDeadInode(file *os.File) error {
+	// If the nlink of a file drops to 0, there is an attacker deleting
+	// directories during our walk, which could result in weird /proc values.
+	// It's better to error out in this case.
+	stat, err := fstat(file)
+	if err != nil {
+		return err
+	}
+	if stat.Nlink == 0 {
+		err := errDeletedInode
+		if stat.Mode&unix.S_IFMT == unix.S_IFDIR {
+			err = errInvalidDirectory
+		}
+		return fmt.Errorf("%w %q", err, file.Name())
+	}
+	return nil
+}
 
 func checkProcSelfFdPath(path string, file *os.File) error {
+	if err := isDeadInode(file); err != nil {
+		return err
+	}
 	actualPath, err := procSelfFdReadlink(file)
 	if err != nil {
 		return fmt.Errorf("get path of handle: %w", err)