Filter dangerous environment variables before reexec

lib/srv/ctx.go

@@ -52,6 +52,7 @@ import (
 	"github.com/gravitational/teleport/lib/sshutils"
 	"github.com/gravitational/teleport/lib/sshutils/x11"
 	"github.com/gravitational/teleport/lib/utils"
+	"github.com/gravitational/teleport/lib/utils/envutils"
 	"github.com/gravitational/teleport/lib/utils/parse"
 )
 
@@ -1204,12 +1205,10 @@ func (id *IdentityContext) GetUserMetadata() apievents.UserMetadata {
 // buildEnvironment constructs a list of environment variables from
 // cluster information.
 func buildEnvironment(ctx *ServerContext) []string {
-	var env []string
+	env := &envutils.SafeEnv{}
 
 	// gather all dynamically defined environment variables
-	ctx.VisitEnv(func(key, val string) {
-		env = append(env, fmt.Sprintf("%s=%s", key, val))
-	})
+	ctx.VisitEnv(env.Add)
 
 	// Parse the local and remote addresses to build SSH_CLIENT and
 	// SSH_CONNECTION environment variables.
@@ -1221,31 +1220,30 @@ func buildEnvironment(ctx *ServerContext) []string {
 		if err != nil {
 			ctx.Logger.Debugf("Failed to split local address: %v.", err)
 		} else {
-			env = append(env,
-				fmt.Sprintf("SSH_CLIENT=%s %s %s", remoteHost, remotePort, localPort),
-				fmt.Sprintf("SSH_CONNECTION=%s %s %s %s", remoteHost, remotePort, localHost, localPort))
+			env.Add("SSH_CLIENT", fmt.Sprintf("%s %s %s", remoteHost, remotePort, localPort))
+			env.Add("SSH_CONNECTION", fmt.Sprintf("%s %s %s %s", remoteHost, remotePort, localHost, localPort))
 		}
 	}
 
 	// If a session has been created try and set TERM, SSH_TTY, and SSH_SESSION_ID.
 	session := ctx.getSession()
 	if session != nil {
 		if session.term != nil {
-			env = append(env, fmt.Sprintf("TERM=%v", session.term.GetTermType()))
-			env = append(env, fmt.Sprintf("SSH_TTY=%s", session.term.TTY().Name()))
+			env.Add("TERM", session.term.GetTermType())
+			env.Add("SSH_TTY", session.term.TTY().Name())
 		}
 		if session.id != "" {
-			env = append(env, fmt.Sprintf("%s=%s", teleport.SSHSessionID, session.id))
+			env.Add(teleport.SSHSessionID, string(session.id))
 		}
 	}
 
 	// Set some Teleport specific environment variables: SSH_TELEPORT_USER,
 	// SSH_TELEPORT_HOST_UUID, and SSH_TELEPORT_CLUSTER_NAME.
-	env = append(env, teleport.SSHTeleportHostUUID+"="+ctx.srv.ID())
-	env = append(env, teleport.SSHTeleportClusterName+"="+ctx.ClusterName)
-	env = append(env, teleport.SSHTeleportUser+"="+ctx.Identity.TeleportUser)
+	env.Add(teleport.SSHTeleportHostUUID, ctx.srv.ID())
+	env.Add(teleport.SSHTeleportClusterName, ctx.ClusterName)
+	env.Add(teleport.SSHTeleportUser, ctx.Identity.TeleportUser)
 
-	return env
+	return *env
 }
 
 func closeAll(closers ...io.Closer) error {

lib/srv/reexec.go

@@ -47,6 +47,7 @@ import (
 	"github.com/gravitational/teleport/lib/sshutils"
 	"github.com/gravitational/teleport/lib/sshutils/x11"
 	"github.com/gravitational/teleport/lib/utils"
+	"github.com/gravitational/teleport/lib/utils/envutils"
 )
 
 // FileFD is a file descriptor passed down from a parent process when
@@ -791,7 +792,7 @@ func buildCommand(c *ExecCommand, localUser *user.User, tty *os.File, pty *os.Fi
 	}
 
 	// Create default environment for user.
-	cmd.Env = []string{
+	env := &envutils.SafeEnv{
 		"LANG=en_US.UTF-8",
 		getDefaultEnvPath(localUser.Uid, defaultLoginDefsPath),
 		"HOME=" + localUser.HomeDir,
@@ -800,21 +801,24 @@ func buildCommand(c *ExecCommand, localUser *user.User, tty *os.File, pty *os.Fi
 	}
 
 	// Add in Teleport specific environment variables.
-	cmd.Env = append(cmd.Env, c.Environment...)
+	env.AddFull(c.Environment...)
 
 	// If the server allows reading in of ~/.tsh/environment read it in
 	// and pass environment variables along to new session.
 	if c.PermitUserEnvironment {
 		filename := filepath.Join(localUser.HomeDir, ".tsh", "environment")
-		userEnvs, err := utils.ReadEnvironmentFile(filename)
+		userEnvs, err := envutils.ReadEnvironmentFile(filename)
 		if err != nil {
 			return nil, trace.Wrap(err)
 		}
-		cmd.Env = append(cmd.Env, userEnvs...)
+		env.AddFull(userEnvs...)
 	}
 
 	// If any additional environment variables come from PAM, apply them as well.
-	cmd.Env = append(cmd.Env, pamEnvironment...)
+	env.AddFull(pamEnvironment...)
+
+	// after environment is fully built, set it to cmd
+	cmd.Env = *env
 
 	// If a terminal was requested, connect std{in,out,err} to the TTY and set
 	// the controlling TTY. Otherwise, connect std{in,out,err} to
@@ -952,11 +956,17 @@ func ConfigureCommand(ctx *ServerContext, extraFiles ...*os.File) (*exec.Cmd, er
 	// is appended if Teleport is running in debug mode.
 	args := []string{executable, subCommand}
 
+	// build env for `teleport exec`
+	env := &envutils.SafeEnv{}
+	env.AddFull(cmdmsg.Environment...)
+	env.AddExecEnvironment()
+
 	// Build the "teleport exec" command.
 	cmd := &exec.Cmd{
 		Path: executable,
 		Args: args,
 		Dir:  executableDir,
+		Env:  *env,
 		ExtraFiles: []*os.File{
 			ctx.cmdr,
 			ctx.contr,

lib/utils/environment.go → lib/utils/envutils/environment.go

@@ -12,15 +12,18 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package utils
+package envutils
 
 import (
 	"bufio"
+	"fmt"
+	"os"
 	"strings"
 
 	log "github.com/sirupsen/logrus"
 
 	"github.com/gravitational/teleport"
+	"github.com/gravitational/teleport/lib/utils"
 )
 
 // ReadEnvironmentFile will read environment variables from a passed in location.
@@ -29,15 +32,15 @@ import (
 func ReadEnvironmentFile(filename string) ([]string, error) {
 	// open the users environment file. if we don't find a file, move on as
 	// having this file for the user is optional.
-	file, err := OpenFileNoUnsafeLinks(filename)
+	file, err := utils.OpenFileNoUnsafeLinks(filename)
 	if err != nil {
 		log.Warnf("Unable to open environment file %v: %v, skipping", filename, err)
 		return []string{}, nil
 	}
 	defer file.Close()
 
 	var lineno int
-	var envs []string
+	env := &SafeEnv{}
 
 	scanner := bufio.NewScanner(file)
 	for scanner.Scan() {
@@ -48,7 +51,7 @@ func ReadEnvironmentFile(filename string) ([]string, error) {
 		lineno = lineno + 1
 		if lineno > teleport.MaxEnvironmentFileLines {
 			log.Warnf("Too many lines in environment file %v, returning first %v lines", filename, teleport.MaxEnvironmentFileLines)
-			return envs, nil
+			return *env, nil
 		}
 
 		// empty lines or lines that start with # are ignored
@@ -71,7 +74,7 @@ func ReadEnvironmentFile(filename string) ([]string, error) {
 			continue
 		}
 
-		envs = append(envs, key+"="+value)
+		env.Add(key, value)
 	}
 
 	err = scanner.Err()
@@ -80,5 +83,56 @@ func ReadEnvironmentFile(filename string) ([]string, error) {
 		return []string{}, nil
 	}
 
-	return envs, nil
+	return *env, nil
+}
+
+var unsafeEnvironmentVars = []string{
+	// Linux
+	"LD_ASSUME_KERNEL", "LD_AUDIT", "LD_BIND_NOW", "LD_BIND_NOT",
+	"LD_DYNAMIC_WEAK", "LD_LIBRARY_PATH", "LD_ORIGIN_PATH", "LD_POINTER_GUARD", "LD_PREFER_MAP_32BIT_EXEC",
+	"LD_PRELOAD", "LD_PROFILE", "LD_RUNPATH", "LD_RPATH", "LD_USE_LOAD_BIAS",
+	// macOS
+	"DYLD_INSERT_LIBRARIES", "DYLD_LIBRARY_PATH",
+}
+
+// SafeEnv allows you to build a system environment while avoiding potentially dangerous environment conditions.
+type SafeEnv []string
+
+// Add will add the key and value to the environment if it's a safe value to forward on for fork / exec.
+func (e *SafeEnv) Add(k, v string) {
+	k = strings.TrimSpace(k)
+	v = strings.TrimSpace(v)
+	if k == "" || k == "=" {
+		return
+	}
+
+	for _, unsafeKey := range unsafeEnvironmentVars {
+		if strings.EqualFold(k, unsafeKey) {
+			return
+		}
+	}
+
+	*e = append(*e, fmt.Sprintf("%s=%s", k, v))
+}
+
+// AddFull adds an exact value, typically in KEY=VALUE format.  This should only be used if they values are already
+// combined.
+func (e *SafeEnv) AddFull(fullValues ...string) {
+valueLoop:
+	for _, kv := range fullValues {
+		kv = strings.TrimSpace(kv)
+
+		for _, unsafeKey := range unsafeEnvironmentVars {
+			if strings.HasPrefix(strings.ToUpper(kv), unsafeKey) {
+				continue valueLoop
+			}
+		}
+
+		*e = append(*e, kv)
+	}
+}
+
+// AddExecEnvironment will add safe values from [os.Environ].
+func (e *SafeEnv) AddExecEnvironment() {
+	e.AddFull(os.Environ()...)
 }