[v18] Prevent chdir race condition in hasAccessibleHomeDir

lib/srv/reexec.go

@@ -35,6 +35,7 @@ import (
 	"runtime"
 	"strconv"
 	"strings"
+	"sync"
 	"syscall"
 	"time"
 
@@ -1229,7 +1230,6 @@ func ConfigureCommand(ctx *ServerContext, extraFiles ...*os.File) (*exec.Cmd, er
 	if err != nil {
 		return nil, trace.Wrap(err)
 	}
-	executableDir, _ := filepath.Split(executable)
 
 	// The channel/request type determines the subcommand to execute.
 	var subCommand string
@@ -1252,7 +1252,6 @@ func ConfigureCommand(ctx *ServerContext, extraFiles ...*os.File) (*exec.Cmd, er
 	cmd := &exec.Cmd{
 		Path: executable,
 		Args: args,
-		Dir:  executableDir,
 		Env:  *env,
 		ExtraFiles: []*os.File{
 			ctx.cmdr,
@@ -1305,6 +1304,13 @@ func coerceHomeDirError(usr *user.User, err error) error {
 	return err
 }
 
+// accessibleHomeDirMu is locked by [hasAccessibleHomeDir] to avoid race
+// conditions between different goroutines while manipulating the global state
+// of the process' working directory. This should be made into a more general
+// global lock if we ever end up relying on this sort of temporary chdir in more
+// places (but we really should not).
+var accessibleHomeDirMu sync.Mutex
+
 // hasAccessibleHomeDir checks if the current user has access to an existing home directory.
 func hasAccessibleHomeDir() error {
 	// this should usually be fetching a cached value
@@ -1322,12 +1328,20 @@ func hasAccessibleHomeDir() error {
 		return trace.BadParameter("%q is not a directory", currentUser.HomeDir)
 	}
 
-	cwd, err := os.Getwd()
+	accessibleHomeDirMu.Lock()
+	defer accessibleHomeDirMu.Unlock()
+
+	cwd, err := os.Open(".")
 	if err != nil {
 		return trace.Wrap(err)
 	}
-	// make sure we return to the original working directory
-	defer os.Chdir(cwd)
+	defer cwd.Close()
+
+	// make sure we return to the original working directory; we ought to panic
+	// if this fails but nothing should actually depend on the working directory
+	// (which is why we can afford to just change it without additional
+	// synchronization here) so we just let it slide
+	defer cwd.Chdir()
 
 	// attemping to cd into the target directory is the easiest, cross-platform way to test
 	// whether or not the current user has access

lib/srv/reexec_test.go

@@ -417,6 +417,10 @@ func testX11Forward(ctx context.Context, t *testing.T, proc *networking.Process,
 func TestRootCheckHomeDir(t *testing.T) {
 	testutils.RequireRoot(t)
 
+	// this test manipulates global state, ensure we're not going to run it in
+	// parallel with something else
+	t.Setenv("foo", "bar")
+
 	tmp := t.TempDir()
 	require.NoError(t, os.Chmod(filepath.Dir(tmp), 0777))
 	require.NoError(t, os.Chmod(tmp, 0777))