adding ability to read ~/.tsh/environment files as the target user (#…

…52135)

instead of root

Co-authored-by: Erik Tate <[email protected]>

constants.go

@@ -558,13 +558,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.

lib/srv/reexec.go

@@ -935,17 +935,71 @@ 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
 		}
 	}
 
 	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) {
@@ -1014,12 +1068,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

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())
+}

lib/utils/envutils/environment.go

@@ -27,6 +27,8 @@ import (
 	"os"
 	"strings"
 
+	"github.com/gravitational/trace"
+
 	"github.com/gravitational/teleport"
 	"github.com/gravitational/teleport/lib/utils"
 )
@@ -47,14 +49,13 @@ func ReadEnvironmentFile(filename string) ([]string, error) {
 	}
 	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{}
 
-	ctx := context.Background()
 	scanner := bufio.NewScanner(r)
 	for scanner.Scan() {
 		line := strings.TrimSpace(scanner.Text())
@@ -94,8 +95,8 @@ func readEnvironment(r io.Reader) ([]string, error) {
 	}
 
 	if err := scanner.Err(); err != nil {
-		slog.WarnContext(ctx, "Unable to read environment file", "error", 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

lib/utils/envutils/environment_test.go

@@ -20,13 +20,15 @@ package envutils
 
 import (
 	"bytes"
+	"context"
 	"testing"
 
 	"github.com/stretchr/testify/require"
 )
 
 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