Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fallback to root when user's home directory is not accessible #47524

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions constants.go
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,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.
Expand Down
64 changes: 51 additions & 13 deletions lib/srv/reexec.go
Original file line number Diff line number Diff line change
Expand Up @@ -866,16 +866,24 @@ func getConnFile(conn net.Conn) (*os.File, error) {
}
}

// runCheckHomeDir check's if the active user's $HOME dir exists.
// runCheckHomeDir check's if the active user's $HOME dir exists and is accessible.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// runCheckHomeDir check's if the active user's $HOME dir exists and is accessible.
// 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()
currentUser, err := user.Current()
if err != nil {
return io.Discard, teleport.HomeDirNotFound, nil
return io.Discard, teleport.HomeDirNotAccessible, nil
Copy link
Contributor

@Tener Tener Oct 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks incorrect per this comment:

// 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.

If there is no user then there is no home dir, yet we are returning the code as if that dir existed (but we couldn't access it).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I agree this is a confusing response. I think a generic RemoteCommandFailure might make more sense here since user.Current() returning an error would be fairly unexpected and I'm not sure adding a new error code would be worth it

}
if !utils.IsDir(home) {
return io.Discard, teleport.HomeDirNotFound, nil

err = HasAccessibleHomeDir(currentUser)
switch {
case trace.IsNotFound(err):
code = teleport.HomeDirNotFound
case trace.IsAccessDenied(err):
code = teleport.HomeDirNotAccessible
case err != nil:
code = teleport.HomeDirNotFound
}
return io.Discard, teleport.RemoteCommandSuccess, nil

return io.Discard, code, nil
}

// runPark does nothing, forever.
Expand Down Expand Up @@ -1194,10 +1202,42 @@ func copyCommand(ctx *ServerContext, cmdmsg *ExecCommand) {
}
}

// CheckHomeDir checks if the user's home dir exists
// HasAccessibleHomeDir checks if the given User has access to an existing home directory.
func HasAccessibleHomeDir(localUser *user.User) error {
fi, err := os.Stat(localUser.HomeDir)
if err != nil {
return trace.Wrap(err)
}

if !fi.IsDir() {
return trace.NotFound("user home is not a directory")
}

stat, ok := fi.Sys().(*syscall.Stat_t)
if !ok {
return trace.AccessDenied("could not retrieve owner of home directory")
}

uid := strconv.Itoa(int(stat.Uid))
gid := strconv.Itoa(int(stat.Gid))

if uid == localUser.Uid && gid == localUser.Gid {
return nil
}

return trace.AccessDenied("user does not own configured home directory")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks incorrect, here are some scenarios:

  • if the home is owned by user 1000 and group 999, but the user is UID=1000/GID=1000 (&& vs ||)
  • user is a member of whatever group owning the home dir
  • if home is o+rwx
  • ACLs are used

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are great points. We're probably better off not inspecting the the ownership information manually and instead just trying to stat the directory as the target user. This was mostly to avoid having to reexec if we didn't have to, but I think we do have to 😄 I'll get a new commit up for this 👍

}

// CheckHomeDir checks if the user's home dir exists and is accessible. This also handles cases where
// the home directory isn't visible to the root user, which HasAccessibleHomeDir doesn't account for.
func CheckHomeDir(localUser *user.User) (bool, error) {
if fi, err := os.Stat(localUser.HomeDir); err == nil {
return fi.IsDir(), nil
err := HasAccessibleHomeDir(localUser)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Funnily enough, simple os.Stat() is likely more complete check due to leveraging the actual system stack to do the check.

What is the problem with the root user?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the problem with the root user?

Can you add to this? I'm not sure I understand what you're asking 🤔

if err == nil {
return true, nil
}

if trace.IsAccessDenied(err) {
return false, nil
}

// In some environments, the user's home directory exists but isn't visible to
Expand Down Expand Up @@ -1229,12 +1269,10 @@ func CheckHomeDir(localUser *user.User) (bool, error) {
reexecCommandOSTweaks(cmd)

if err := cmd.Run(); err != nil {
if cmd.ProcessState.ExitCode() == teleport.HomeDirNotFound {
return false, nil
}
return false, trace.Wrap(err)
}
return true, nil

return cmd.ProcessState.ExitCode() == teleport.RemoteCommandSuccess, nil
}

// Spawns a process with the given credentials, outliving the context.
Expand Down
43 changes: 43 additions & 0 deletions lib/srv/reexec_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ import (
"github.com/gravitational/teleport/lib/teleagent"
"github.com/gravitational/teleport/lib/utils"
"github.com/gravitational/teleport/lib/utils/host"
"github.com/gravitational/trace"
)

type stubUser struct {
Expand Down Expand Up @@ -410,3 +411,45 @@ func testX11Forward(ctx context.Context, t *testing.T, proc *networking.Process,
require.NoError(t, err)
require.Equal(t, fakeXauthEntry, readXauthEntry)
}

func TestRootHasAccessibleHomeDir(t *testing.T) {
utils.RequireRoot(t)

uid := 1000
gid := 1000

tmp := t.TempDir()
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)

require.NoError(t, os.Chown(home, uid, gid))
require.NoError(t, os.Chown(file, uid, gid))

testUser := user.User{
Uid: strconv.Itoa(uid),
Gid: strconv.Itoa(gid),
HomeDir: home,
}

err = HasAccessibleHomeDir(&testUser)
require.NoError(t, err)

testUser.HomeDir = noAccess
err = HasAccessibleHomeDir(&testUser)
require.True(t, trace.IsAccessDenied(err))

testUser.HomeDir = file
err = HasAccessibleHomeDir(&testUser)
require.True(t, trace.IsNotFound(err))

testUser.HomeDir = notFound
err = HasAccessibleHomeDir(&testUser)
require.True(t, trace.IsNotFound(err))
}
2 changes: 1 addition & 1 deletion lib/srv/regular/sshserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -1266,7 +1266,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.
Expand Down
Loading