checking for a valid home directory now ensures that the local user h…

…as (#47917)

access and child processes fallback to the root directory ("/") in the
case that they do not

Co-authored-by: Tim Ross <[email protected]>

constants.go

@@ -538,6 +538,10 @@ const (
 	// HomeDirNotFound is returned when a the "teleport checkhomedir" command cannot
 	// find the user's home directory.
 	HomeDirNotFound = 254
+	// HomeDirNotAccessible is returned when a the "teleport checkhomedir" command has
+	// found the user's home directory, but the user does NOT have permissions to
+	// access it.
+	HomeDirNotAccessible = 253
 )
 
 // MaxEnvironmentFileLines is the maximum number of lines in a environment file.

lib/srv/exec_linux_test.go

@@ -26,6 +26,7 @@ import (
 	"os"
 	"os/exec"
 	"os/user"
+	"path/filepath"
 	"strconv"
 	"syscall"
 	"testing"
@@ -41,17 +42,40 @@ import (
 )
 
 func TestOSCommandPrep(t *testing.T) {
+	utils.RequireRoot(t)
+
 	srv := newMockServer(t)
 	scx := newExecServerContext(t, srv)
 
-	usr, err := user.Current()
+	// because CheckHomeDir now inspects access to the home directory as the actual user after a rexec,
+	// we need to setup a real, non-root user with a valid home directory in order for this test to
+	// exercise the correct paths
+	tempHome := t.TempDir()
+	require.NoError(t, os.Chmod(filepath.Dir(tempHome), 0777))
+
+	username := "test-os-command-prep"
+	scx.Identity.Login = username
+	_, err := host.UserAdd(username, nil, tempHome, "", "")
+	require.NoError(t, err)
+	t.Cleanup(func() {
+		// change homedir back so user deletion doesn't fail
+		changeHomeDir(t, username, tempHome)
+		_, err := host.UserDel(username)
+		require.NoError(t, err)
+	})
+
+	usr, err := user.Lookup(username)
+	require.NoError(t, err)
+
+	uid, err := strconv.Atoi(usr.Uid)
 	require.NoError(t, err)
 
+	require.NoError(t, os.Chown(tempHome, uid, -1))
 	expectedEnv := []string{
 		"LANG=en_US.UTF-8",
-		getDefaultEnvPath(strconv.Itoa(os.Geteuid()), defaultLoginDefsPath),
+		getDefaultEnvPath(usr.Uid, defaultLoginDefsPath),
 		fmt.Sprintf("HOME=%s", usr.HomeDir),
-		fmt.Sprintf("USER=%s", usr.Username),
+		fmt.Sprintf("USER=%s", username),
 		"SHELL=/bin/sh",
 		"SSH_CLIENT=10.0.0.5 4817 3022",
 		"SSH_CONNECTION=10.0.0.5 4817 127.0.0.1 3022",
@@ -104,12 +128,9 @@ func TestOSCommandPrep(t *testing.T) {
 	require.Equal(t, []string{"/bin/sh", "-c", "top"}, cmd.Args)
 	require.Equal(t, syscall.SIGKILL, cmd.SysProcAttr.Pdeathsig)
 
-	if os.Geteuid() != 0 {
-		t.Skip("skipping portion of test which must run as root")
-	}
-
 	// Missing home directory - HOME should still be set to the given
 	// home dir, but the command should set it's CWD to root instead.
+	changeHomeDir(t, username, "/wrong/place")
 	usr.HomeDir = "/wrong/place"
 	root := string(os.PathSeparator)
 	expectedEnv[2] = "HOME=/wrong/place"

lib/srv/reexec.go

@@ -615,6 +615,8 @@ func (o *osWrapper) startNewParker(ctx context.Context, credential *syscall.Cred
 
 type forwardHandler func(ctx context.Context, addr string, file *os.File) error
 
+const rootDirectory = "/"
+
 func handleLocalPortForward(ctx context.Context, addr string, file *os.File) error {
 	conn, err := uds.FromFile(file)
 	_ = file.Close()
@@ -799,16 +801,21 @@ func RunRemoteForward() (errw io.Writer, code int, err error) {
 	return errw, code, trace.Wrap(err)
 }
 
-// runCheckHomeDir check's if the active user's $HOME dir exists.
+// runCheckHomeDir checks if the active user's $HOME dir exists and is accessible.
 func runCheckHomeDir() (errw io.Writer, code int, err error) {
-	home, err := os.UserHomeDir()
-	if err != nil {
-		return io.Discard, teleport.HomeDirNotFound, nil
-	}
-	if !utils.IsDir(home) {
-		return io.Discard, teleport.HomeDirNotFound, nil
+	code = teleport.RemoteCommandSuccess
+	if err := hasAccessibleHomeDir(); err != nil {
+		switch {
+		case trace.IsNotFound(err), trace.IsBadParameter(err):
+			code = teleport.HomeDirNotFound
+		case trace.IsAccessDenied(err):
+			code = teleport.HomeDirNotAccessible
+		default:
+			code = teleport.RemoteCommandFailure
+		}
 	}
-	return io.Discard, teleport.RemoteCommandSuccess, nil
+
+	return io.Discard, code, nil
 }
 
 // runPark does nothing, forever.
@@ -984,18 +991,20 @@ func buildCommand(c *ExecCommand, localUser *user.User, tty *os.File, pamEnviron
 	// Set the command's cwd to the user's $HOME, or "/" if
 	// they don't have an existing home dir.
 	// TODO (atburke): Generalize this to support Windows.
-	exists, err := CheckHomeDir(localUser)
+	hasAccess, err := CheckHomeDir(localUser)
 	if err != nil {
 		return nil, trace.Wrap(err)
-	} else if exists {
+	}
+
+	if hasAccess {
 		cmd.Dir = localUser.HomeDir
-	} else if !exists {
+	} else {
 		// Write failure to find home dir to stdout, same as OpenSSH.
-		msg := fmt.Sprintf("Could not set shell's cwd to home directory %q, defaulting to %q\n", localUser.HomeDir, string(os.PathSeparator))
+		msg := fmt.Sprintf("Could not set shell's cwd to home directory %q, defaulting to %q\n", localUser.HomeDir, rootDirectory)
 		if _, err := cmd.Stdout.Write([]byte(msg)); err != nil {
 			return nil, trace.Wrap(err)
 		}
-		cmd.Dir = string(os.PathSeparator)
+		cmd.Dir = rootDirectory
 	}
 
 	// Only set process credentials if the UID/GID of the requesting user are
@@ -1157,16 +1166,73 @@ func copyCommand(ctx *ServerContext, cmdmsg *ExecCommand) {
 	}
 }
 
-// CheckHomeDir checks if the user's home dir exists
+func coerceHomeDirError(usr *user.User, err error) error {
+	if os.IsNotExist(err) {
+		return trace.NotFound("home directory %q not found for user %q", usr.HomeDir, usr.Name)
+	}
+
+	if os.IsPermission(err) {
+		return trace.AccessDenied("%q does not have permission to access %q", usr.Name, usr.HomeDir)
+	}
+
+	return err
+}
+
+// hasAccessibleHomeDir checks if the current user has access to an existing home directory.
+func hasAccessibleHomeDir() error {
+	// this should usually be fetching a cached value
+	currentUser, err := user.Current()
+	if err != nil {
+		return trace.Wrap(err)
+	}
+
+	fi, err := os.Stat(currentUser.HomeDir)
+	if err != nil {
+		return trace.Wrap(coerceHomeDirError(currentUser, err))
+	}
+
+	if !fi.IsDir() {
+		return trace.BadParameter("%q is not a directory", currentUser.HomeDir)
+	}
+
+	cwd, err := os.Getwd()
+	if err != nil {
+		return trace.Wrap(err)
+	}
+	// make sure we return to the original working directory
+	defer os.Chdir(cwd)
+
+	// attemping to cd into the target directory is the easiest, cross-platform way to test
+	// whether or not the current user has access
+	if err := os.Chdir(currentUser.HomeDir); err != nil {
+		return trace.Wrap(coerceHomeDirError(currentUser, err))
+	}
+
+	return nil
+}
+
+// CheckHomeDir checks if the user's home directory exists and is accessible to the user. Only catastrophic
+// errors will be returned, which means a missing, inaccessible, or otherwise invalid home directory will result
+// in a return of (false, nil)
 func CheckHomeDir(localUser *user.User) (bool, error) {
-	if fi, err := os.Stat(localUser.HomeDir); err == nil {
-		return fi.IsDir(), nil
+	currentUser, err := user.Current()
+	if err != nil {
+		return false, trace.Wrap(err)
+	}
+
+	// don't spawn a subcommand if already running as the user in question
+	if currentUser.Uid == localUser.Uid {
+		if err := hasAccessibleHomeDir(); err != nil {
+			if trace.IsNotFound(err) || trace.IsAccessDenied(err) || trace.IsBadParameter(err) {
+				return false, nil
+			}
+
+			return false, trace.Wrap(err)
+		}
+
+		return true, nil
 	}
 
-	// In some environments, the user's home directory exists but isn't visible to
-	// root, e.g. /home is mounted to an nfs export with root_squash enabled.
-	// In case we are in that scenario, re-exec teleport as the user to check
-	// if the home dir actually does exist.
 	executable, err := os.Executable()
 	if err != nil {
 		return false, trace.Wrap(err)
@@ -1182,6 +1248,7 @@ func CheckHomeDir(localUser *user.User) (bool, error) {
 		Path: executable,
 		Args: []string{executable, teleport.CheckHomeDirSubCommand},
 		Env:  []string{"HOME=" + localUser.HomeDir},
+		Dir:  rootDirectory,
 		SysProcAttr: &syscall.SysProcAttr{
 			Setsid:     true,
 			Credential: credential,
@@ -1192,11 +1259,13 @@ func CheckHomeDir(localUser *user.User) (bool, error) {
 	reexecCommandOSTweaks(cmd)
 
 	if err := cmd.Run(); err != nil {
-		if cmd.ProcessState.ExitCode() == teleport.HomeDirNotFound {
-			return false, nil
+		if cmd.ProcessState.ExitCode() == teleport.RemoteCommandFailure {
+			return false, trace.Wrap(err)
 		}
-		return false, trace.Wrap(err)
+
+		return false, nil
 	}
+
 	return true, nil
 }
 

lib/srv/reexec_test.go

@@ -28,11 +28,13 @@ import (
 	"os"
 	"os/exec"
 	"os/user"
+	"path/filepath"
 	"strconv"
 	"syscall"
 	"testing"
 
 	"github.com/gravitational/trace"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"github.com/gravitational/teleport"
@@ -312,3 +314,72 @@ func TestRootRemotePortForwardCommand(t *testing.T) {
 
 	testRemotePortForwardCommand(t, login)
 }
+
+func TestRootCheckHomeDir(t *testing.T) {
+	utils.RequireRoot(t)
+
+	tmp := t.TempDir()
+	require.NoError(t, os.Chmod(filepath.Dir(tmp), 0777))
+	require.NoError(t, os.Chmod(tmp, 0777))
+
+	home := filepath.Join(tmp, "home")
+	noAccess := filepath.Join(tmp, "no_access")
+	file := filepath.Join(tmp, "file")
+	notFound := filepath.Join(tmp, "not_found")
+
+	require.NoError(t, os.Mkdir(home, 0700))
+	require.NoError(t, os.Mkdir(noAccess, 0700))
+	_, err := os.Create(file)
+	require.NoError(t, err)
+
+	login := utils.GenerateLocalUsername(t)
+	_, err = host.UserAdd(login, nil, home, "", "")
+	require.NoError(t, err)
+	t.Cleanup(func() {
+		// change back to accessible home so deletion works
+		changeHomeDir(t, login, home)
+		_, err := host.UserDel(login)
+		require.NoError(t, err)
+	})
+
+	testUser, err := user.Lookup(login)
+	require.NoError(t, err)
+
+	uid, err := strconv.Atoi(testUser.Uid)
+	require.NoError(t, err)
+
+	gid, err := strconv.Atoi(testUser.Gid)
+	require.NoError(t, err)
+
+	require.NoError(t, os.Chown(home, uid, gid))
+	require.NoError(t, os.Chown(file, uid, gid))
+
+	hasAccess, err := CheckHomeDir(testUser)
+	require.NoError(t, err)
+	require.True(t, hasAccess)
+
+	changeHomeDir(t, login, file)
+	hasAccess, err = CheckHomeDir(testUser)
+	require.NoError(t, err)
+	require.False(t, hasAccess)
+
+	changeHomeDir(t, login, notFound)
+	hasAccess, err = CheckHomeDir(testUser)
+	require.NoError(t, err)
+	require.False(t, hasAccess)
+
+	changeHomeDir(t, login, noAccess)
+	hasAccess, err = CheckHomeDir(testUser)
+	require.NoError(t, err)
+	require.False(t, hasAccess)
+}
+
+func changeHomeDir(t *testing.T, username, home string) {
+	usermodBin, err := exec.LookPath("usermod")
+	assert.NoError(t, err, "usermod binary must be present")
+
+	cmd := exec.Command(usermodBin, "--home", home, username)
+	_, err = cmd.CombinedOutput()
+	assert.NoError(t, err, "changing home should not error")
+	assert.Equal(t, 0, cmd.ProcessState.ExitCode(), "changing home should exit 0")
+}

lib/srv/regular/sshserver.go

@@ -1485,7 +1485,7 @@ func (s *Server) HandleNewConn(ctx context.Context, ccx *sshutils.ConnectionCont
 	// Create host user.
 	created, userCloser, err := s.termHandlers.SessionRegistry.UpsertHostUser(identityContext)
 	if err != nil {
-		log.Infof("error while creating host users: %s", err)
+		log.Warnf("error while creating host users: %s", err)
 	}
 
 	// Indicate that the user was created by Teleport.