diff --git a/lib/srv/reexec.go b/lib/srv/reexec.go index 578d29679ffbd..d0377e58f377a 100644 --- a/lib/srv/reexec.go +++ b/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 diff --git a/lib/srv/reexec_test.go b/lib/srv/reexec_test.go index 8c7ba0595a3d4..baf8350fd679a 100644 --- a/lib/srv/reexec_test.go +++ b/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))