From a16df6566a703ac51bf9550549e64630773a309e Mon Sep 17 00:00:00 2001 From: Roman Tkachenko Date: Thu, 13 Feb 2025 10:42:43 -0800 Subject: [PATCH] adding ability to read ~/.tsh/environment files as the target user (#52136) instead of root Co-authored-by: Erik Tate --- constants.go | 7 ++- lib/srv/reexec.go | 68 +++++++++++++++++++++-- lib/srv/reexec_test.go | 77 ++++++++++++++++++++++++++ lib/utils/envutils/environment.go | 20 ++++--- lib/utils/envutils/environment_test.go | 4 +- 5 files changed, 158 insertions(+), 18 deletions(-) diff --git a/constants.go b/constants.go index be3e4e55f9932..6840111abaaea 100644 --- a/constants.go +++ b/constants.go @@ -555,13 +555,16 @@ const ( // RemoteCommandFailure is returned when a command has failed to execute and // we don't have another status code for it. RemoteCommandFailure = 255 - // HomeDirNotFound is returned when a the "teleport checkhomedir" command cannot + // HomeDirNotFound is returned when the "teleport checkhomedir" command cannot // find the user's home directory. HomeDirNotFound = 254 - // HomeDirNotAccessible is returned when a the "teleport checkhomedir" command has + // HomeDirNotAccessible is returned when the "teleport checkhomedir" command has // found the user's home directory, but the user does NOT have permissions to // access it. HomeDirNotAccessible = 253 + // UnexpectedCredentials is returned when a command is no longer running with the expected + // credentials. + UnexpectedCredentials = 252 ) // MaxEnvironmentFileLines is the maximum number of lines in a environment file. diff --git a/lib/srv/reexec.go b/lib/srv/reexec.go index 26199e056b4f2..bd0225a34efe6 100644 --- a/lib/srv/reexec.go +++ b/lib/srv/reexec.go @@ -25,6 +25,7 @@ import ( "errors" "fmt" "io" + "log/slog" "net" "os" "os/exec" @@ -933,10 +934,11 @@ func RunAndExit(commandType string) { // IsReexec determines if the current process is a teleport reexec command. // Used by tests to reroute the execution to RunAndExit. func IsReexec() bool { - if len(os.Args) == 2 { + if len(os.Args) >= 2 { switch os.Args[1] { case teleport.ExecSubCommand, teleport.NetworkingSubCommand, - teleport.CheckHomeDirSubCommand, teleport.ParkSubCommand, teleport.SFTPSubCommand: + teleport.CheckHomeDirSubCommand, + teleport.ParkSubCommand, teleport.SFTPSubCommand: return true } } @@ -944,6 +946,59 @@ func IsReexec() bool { return false } +// openFileAsUser opens a file as the given user to ensure proper access checks. This is unsafe and should not be used outside of +// bootstrapping reexec commands. +func openFileAsUser(localUser *user.User, path string) (file *os.File, err error) { + if os.Args[1] != teleport.ExecSubCommand { + return nil, trace.Errorf("opening files as a user is only possible in a reexec context") + } + + uid, err := strconv.Atoi(localUser.Uid) + if err != nil { + return nil, trace.Wrap(err) + } + + gid, err := strconv.Atoi(localUser.Gid) + if err != nil { + return nil, trace.Wrap(err) + } + + prevUID := os.Geteuid() + prevGID := os.Getegid() + + defer func() { + gidErr := syscall.Setegid(prevGID) + uidErr := syscall.Seteuid(prevUID) + if uidErr != nil || gidErr != nil { + file.Close() + slog.ErrorContext(context.Background(), "cannot proceed with invalid effective credentials", "uid_err", uidErr, "gid_err", gidErr, "error", err) + os.Exit(teleport.UnexpectedCredentials) + } + }() + + if err := syscall.Setegid(gid); err != nil { + return nil, trace.Wrap(err) + } + + if err := syscall.Seteuid(uid); err != nil { + return nil, trace.Wrap(err) + } + + file, err = utils.OpenFileNoUnsafeLinks(path) + return file, trace.Wrap(err) +} + +func readUserEnv(localUser *user.User, path string) ([]string, error) { + file, err := openFileAsUser(localUser, path) + if err != nil { + return nil, trace.Wrap(err) + } + defer file.Close() + + envs, err := envutils.ReadEnvironment(context.Background(), file) + return envs, trace.Wrap(err) +} + // buildCommand constructs a command that will execute the users shell. This // function is run by Teleport while it's re-executing. func buildCommand(c *ExecCommand, localUser *user.User, tty *os.File, pamEnvironment []string) (*exec.Cmd, error) { @@ -1012,12 +1067,13 @@ func buildCommand(c *ExecCommand, localUser *user.User, tty *os.File, pamEnviron // and pass environment variables along to new session. // User controlled values are added last to ensure administrator controlled sources take priority (duplicates ignored) if c.PermitUserEnvironment { - filename := filepath.Join(localUser.HomeDir, ".tsh", "environment") - userEnvs, err := envutils.ReadEnvironmentFile(filename) + path := filepath.Join(localUser.HomeDir, ".tsh", "environment") + userEnvs, err := readUserEnv(localUser, path) if err != nil { - return nil, trace.Wrap(err) + slog.WarnContext(context.Background(), "Could not read user environment", "error", err) + } else { + env.AddFullUnique(userEnvs...) } - env.AddFullUnique(userEnvs...) } // after environment is fully built, set it to cmd diff --git a/lib/srv/reexec_test.go b/lib/srv/reexec_test.go index a9d6934c7b233..3ba5d9a1ef1f8 100644 --- a/lib/srv/reexec_test.go +++ b/lib/srv/reexec_test.go @@ -36,6 +36,7 @@ import ( "syscall" "testing" + "github.com/gravitational/trace" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/crypto/ssh/agent" @@ -480,3 +481,79 @@ func changeHomeDir(t *testing.T, username, home string) { assert.NoError(t, err, "changing home should not error") assert.Equal(t, 0, cmd.ProcessState.ExitCode(), "changing home should exit 0") } + +func TestRootOpenFileAsUser(t *testing.T) { + utils.RequireRoot(t) + euid := os.Geteuid() + egid := os.Getegid() + + username := "processing-user" + + arg := os.Args[1] + os.Args[1] = teleport.ExecSubCommand + defer func() { + os.Args[1] = arg + }() + + _, err := host.UserAdd(username, nil, host.UserOpts{}) + require.NoError(t, err) + + t.Cleanup(func() { + _, err := host.UserDel(username) + require.NoError(t, err) + }) + + tmp := t.TempDir() + testFile := filepath.Join(tmp, "testfile") + fileContent := "one does not simply open without permission" + + err = os.WriteFile(testFile, []byte(fileContent), 0777) + require.NoError(t, err) + + testUser, err := user.Lookup(username) + require.NoError(t, err) + + // no access + file, err := openFileAsUser(testUser, testFile) + require.True(t, trace.IsAccessDenied(err)) + require.Nil(t, file) + + // ensure we fallback to root after + file, err = os.Open(testFile) + require.NoError(t, err) + require.NotNil(t, file) + file.Close() + + // has access + uid, err := strconv.Atoi(testUser.Uid) + require.NoError(t, err) + + gid, err := strconv.Atoi(testUser.Gid) + require.NoError(t, err) + + err = os.Chown(filepath.Dir(tmp), uid, gid) + require.NoError(t, err) + + err = os.Chown(tmp, uid, gid) + require.NoError(t, err) + + err = os.Chown(testFile, uid, gid) + require.NoError(t, err) + + file, err = openFileAsUser(testUser, testFile) + require.NoError(t, err) + require.NotNil(t, file) + + data, err := io.ReadAll(file) + file.Close() + require.NoError(t, err) + require.Equal(t, fileContent, string(data)) + + // not exist + file, err = openFileAsUser(testUser, filepath.Join(tmp, "no_exist")) + require.ErrorIs(t, err, os.ErrNotExist) + require.Nil(t, file) + + require.Equal(t, euid, os.Geteuid()) + require.Equal(t, egid, os.Getegid()) +} diff --git a/lib/utils/envutils/environment.go b/lib/utils/envutils/environment.go index 19380be212f5f..5f8923a8a2c29 100644 --- a/lib/utils/envutils/environment.go +++ b/lib/utils/envutils/environment.go @@ -20,12 +20,14 @@ package envutils import ( "bufio" + "context" "fmt" "io" + "log/slog" "os" "strings" - log "github.com/sirupsen/logrus" + "github.com/gravitational/trace" "github.com/gravitational/teleport" "github.com/gravitational/teleport/lib/utils" @@ -39,15 +41,15 @@ func ReadEnvironmentFile(filename string) ([]string, error) { // having this file for the user is optional. file, err := utils.OpenFileNoUnsafeLinks(filename) if err != nil { - log.Warnf("Unable to open environment file %v: %v, skipping", filename, err) + slog.WarnContext(context.TODO(), "Unable to open environment file, skipping", "filename", filename, "error", err) return []string{}, nil } defer file.Close() - return readEnvironment(file) + return ReadEnvironment(context.TODO(), file) } -func readEnvironment(r io.Reader) ([]string, error) { +func ReadEnvironment(ctx context.Context, r io.Reader) ([]string, error) { var lineno int env := &SafeEnv{} @@ -59,7 +61,7 @@ func readEnvironment(r io.Reader) ([]string, error) { // https://github.com/openssh/openssh-portable/blob/master/session.c#L873-L874 lineno = lineno + 1 if lineno > teleport.MaxEnvironmentFileLines { - log.Warnf("Too many lines in environment file, returning first %v lines", teleport.MaxEnvironmentFileLines) + slog.WarnContext(ctx, "Too many lines in environment file, returning truncated lines", "lines", teleport.MaxEnvironmentFileLines) return *env, nil } @@ -71,7 +73,7 @@ func readEnvironment(r io.Reader) ([]string, error) { // split on first =, if not found, log it and continue idx := strings.Index(line, "=") if idx == -1 { - log.Debugf("Bad line %v while reading environment file: no = separator found", lineno) + slog.DebugContext(ctx, "Bad line while reading environment file: no = separator found", "line_num", lineno) continue } @@ -79,7 +81,7 @@ func readEnvironment(r io.Reader) ([]string, error) { key := line[:idx] value := line[idx+1:] if strings.TrimSpace(key) == "" { - log.Debugf("Bad line %v while reading environment file: key without name", lineno) + slog.DebugContext(ctx, "Bad line while reading environment file: key without name", "line_num", lineno) continue } @@ -88,8 +90,8 @@ func readEnvironment(r io.Reader) ([]string, error) { } if err := scanner.Err(); err != nil { - log.Warnf("Unable to read environment file: %v", err) - return []string{}, nil + slog.ErrorContext(ctx, "Unable to read environment file", "error", err) + return []string{}, trace.Wrap(err, "reading environment file") } return *env, nil diff --git a/lib/utils/envutils/environment_test.go b/lib/utils/envutils/environment_test.go index ac1a58ac9014f..57ca2c2bc605d 100644 --- a/lib/utils/envutils/environment_test.go +++ b/lib/utils/envutils/environment_test.go @@ -20,6 +20,7 @@ package envutils import ( "bytes" + "context" "testing" "github.com/stretchr/testify/require" @@ -27,6 +28,7 @@ import ( func TestReadEnvironment(t *testing.T) { t.Parallel() + ctx := context.Background() // contents of environment file rawenv := []byte(` @@ -42,7 +44,7 @@ bar=foo LD_PRELOAD=attack `) - env, err := readEnvironment(bytes.NewReader(rawenv)) + env, err := ReadEnvironment(ctx, bytes.NewReader(rawenv)) require.NoError(t, err) // check we parsed it correctly