From 0adbb08ab15479764ec3dc7f6aa6d1b35291f824 Mon Sep 17 00:00:00 2001 From: Erik Tate Date: Fri, 25 Oct 2024 10:44:49 -0400 Subject: [PATCH] checking for a valid home directory now ensures that the local user has (#47916) access and child processes fallback to the root directory ("/") in the case that they do not Co-authored-by: Tim Ross --- constants.go | 4 ++ lib/srv/exec_linux_test.go | 37 ++++++++--- lib/srv/reexec.go | 115 ++++++++++++++++++++++++++++------- lib/srv/reexec_test.go | 71 +++++++++++++++++++++ lib/srv/regular/sshserver.go | 2 +- 5 files changed, 198 insertions(+), 31 deletions(-) diff --git a/constants.go b/constants.go index e213751db1372..0e2a1e1f4a2b5 100644 --- a/constants.go +++ b/constants.go @@ -543,6 +543,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. diff --git a/lib/srv/exec_linux_test.go b/lib/srv/exec_linux_test.go index 3eaf3347c65f0..58b57412ed9fe 100644 --- a/lib/srv/exec_linux_test.go +++ b/lib/srv/exec_linux_test.go @@ -26,6 +26,7 @@ import ( "os" "os/exec" "os/user" + "path/filepath" "strconv" "syscall" "testing" @@ -41,17 +42,42 @@ 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, host.UserOpts{ + Home: 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 +130,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" diff --git a/lib/srv/reexec.go b/lib/srv/reexec.go index 130dd30594ac5..da1d84afa9079 100644 --- a/lib/srv/reexec.go +++ b/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 } diff --git a/lib/srv/reexec_test.go b/lib/srv/reexec_test.go index ec59d44f2d5a8..76e5417979240 100644 --- a/lib/srv/reexec_test.go +++ b/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, host.UserOpts{Home: 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") +} diff --git a/lib/srv/regular/sshserver.go b/lib/srv/regular/sshserver.go index ecbd0734b2622..df0e87f4abb48 100644 --- a/lib/srv/regular/sshserver.go +++ b/lib/srv/regular/sshserver.go @@ -1454,7 +1454,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.