[v14] Prevent .tsh/environment values from overloading prior set values

lib/srv/ctx.go

@@ -1207,9 +1207,6 @@ func (id *IdentityContext) GetUserMetadata() apievents.UserMetadata {
 func buildEnvironment(ctx *ServerContext) []string {
 	env := &envutils.SafeEnv{}
 
-	// gather all dynamically defined environment variables
-	ctx.VisitEnv(env.Add)
-
 	// Parse the local and remote addresses to build SSH_CLIENT and
 	// SSH_CONNECTION environment variables.
 	remoteHost, remotePort, err := net.SplitHostPort(ctx.ServerConn.RemoteAddr().String())
@@ -1220,28 +1217,31 @@ func buildEnvironment(ctx *ServerContext) []string {
 		if err != nil {
 			ctx.Logger.Debugf("Failed to split local address: %v.", err)
 		} else {
-			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))
+			env.AddTrusted("SSH_CLIENT", fmt.Sprintf("%s %s %s", remoteHost, remotePort, localPort))
+			env.AddTrusted("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.Add("TERM", session.term.GetTermType())
-			env.Add("SSH_TTY", session.term.TTY().Name())
+			env.AddTrusted("TERM", session.term.GetTermType())
+			env.AddTrusted("SSH_TTY", session.term.TTY().Name())
 		}
 		if session.id != "" {
-			env.Add(teleport.SSHSessionID, string(session.id))
+			env.AddTrusted(teleport.SSHSessionID, string(session.id))
 		}
 	}
 
 	// Set some Teleport specific environment variables: SSH_TELEPORT_USER,
 	// SSH_TELEPORT_HOST_UUID, and SSH_TELEPORT_CLUSTER_NAME.
-	env.Add(teleport.SSHTeleportHostUUID, ctx.srv.ID())
-	env.Add(teleport.SSHTeleportClusterName, ctx.ClusterName)
-	env.Add(teleport.SSHTeleportUser, ctx.Identity.TeleportUser)
+	env.AddTrusted(teleport.SSHTeleportHostUUID, ctx.srv.ID())
+	env.AddTrusted(teleport.SSHTeleportClusterName, ctx.ClusterName)
+	env.AddTrusted(teleport.SSHTeleportUser, ctx.Identity.TeleportUser)
+
+	// At the end gather all dynamically defined environment variables
+	ctx.VisitEnv(env.AddUnique)
 
 	return *env
 }

lib/srv/reexec.go

@@ -801,22 +801,23 @@ func buildCommand(c *ExecCommand, localUser *user.User, tty *os.File, pty *os.Fi
 	}
 
 	// Add in Teleport specific environment variables.
-	env.AddFull(c.Environment...)
+	env.AddFullTrusted(c.Environment...)
+
+	// If any additional environment variables come from PAM, apply them as well.
+	env.AddFullTrusted(pamEnvironment...)
 
 	// If the server allows reading in of ~/.tsh/environment read it in
 	// 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)
 		if err != nil {
 			return nil, trace.Wrap(err)
 		}
-		env.AddFull(userEnvs...)
+		env.AddFullUnique(userEnvs...)
 	}
 
-	// If any additional environment variables come from PAM, apply them as well.
-	env.AddFull(pamEnvironment...)
-
 	// after environment is fully built, set it to cmd
 	cmd.Env = *env
 
@@ -958,7 +959,7 @@ func ConfigureCommand(ctx *ServerContext, extraFiles ...*os.File) (*exec.Cmd, er
 
 	// build env for `teleport exec`
 	env := &envutils.SafeEnv{}
-	env.AddFull(cmdmsg.Environment...)
+	env.AddFullTrusted(cmdmsg.Environment...)
 	env.AddExecEnvironment()
 
 	// Build the "teleport exec" command.

lib/utils/envutils/environment.go

@@ -74,7 +74,8 @@ func ReadEnvironmentFile(filename string) ([]string, error) {
 			continue
 		}
 
-		env.Add(key, value)
+		// key is added trusted within this context, but should be "AddFullUnique" when combined with any other values
+		env.AddTrusted(key, value)
 	}
 
 	err = scanner.Err()
@@ -86,53 +87,104 @@ func ReadEnvironmentFile(filename string) ([]string, error) {
 	return *env, nil
 }
 
-var unsafeEnvironmentVars = []string{
+var unsafeEnvironmentVars = map[string]struct{}{
 	// 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",
+	"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",
+	"DYLD_INSERT_LIBRARIES": {},
+	"DYLD_LIBRARY_PATH":     {},
 }
 
-// SafeEnv allows you to build a system environment while avoiding potentially dangerous environment conditions.
+// SafeEnv allows you to build a system environment while avoiding potentially dangerous environment conditions.  In
+// addition, SafeEnv will ignore any values added if the key already exists.  This allows earlier inserts to take
+// priority and ensure there is no conflicting values.
 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) {
+// AddTrusted will add the key and value to the environment if it's a safe value to forward on for fork / exec.  This
+// will not check for duplicates.
+func (e *SafeEnv) AddTrusted(k, v string) {
+	e.add(false, k, v)
+}
+
+// AddUnique will add the key and value to the environment if it's a safe value to forward on for fork / exec.  If the
+// key already exists (case-insensitive) it will be ignored.
+func (e *SafeEnv) AddUnique(k, v string) {
+	e.add(true, k, v)
+}
+
+func (e *SafeEnv) add(preventDuplicates bool, k, v string) {
 	k = strings.TrimSpace(k)
 	v = strings.TrimSpace(v)
-	if k == "" || k == "=" {
+	if e.unsafeKey(preventDuplicates, 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:
+// AddFullTrusted adds an exact value, in the KEY=VALUE format. This should only be used if they values are already
+// combined.  When the values are separate the [Add] function is generally preferred.  This will not check for
+// duplicates.
+func (e *SafeEnv) AddFullTrusted(fullValues ...string) {
+	e.addFull(false, fullValues)
+}
+
+// AddFullUnique adds an exact value, in the KEY=VALUE format. This should only be used if they values are already
+// combined.  When the values are separate the [Add] function is generally preferred.  If any keys already exists
+// (case-insensitive) they will be ignored.
+func (e *SafeEnv) AddFullUnique(fullValues ...string) {
+	e.addFull(true, fullValues)
+}
+
+func (e *SafeEnv) addFull(preventDuplicates bool, fullValues []string) {
 	for _, kv := range fullValues {
 		kv = strings.TrimSpace(kv)
 
-		for _, unsafeKey := range unsafeEnvironmentVars {
-			if strings.HasPrefix(strings.ToUpper(kv), unsafeKey) {
-				continue valueLoop
-			}
+		key := strings.SplitN(kv, "=", 2)[0]
+		if e.unsafeKey(preventDuplicates, key) {
+			continue
 		}
 
 		*e = append(*e, kv)
 	}
 }
 
-// AddExecEnvironment will add safe values from [os.Environ].
+func (e *SafeEnv) unsafeKey(preventDuplicates bool, key string) bool {
+	if key == "" || key == "=" {
+		return false
+	}
+
+	upperKey := strings.ToUpper(key)
+	if _, unsafe := unsafeEnvironmentVars[upperKey]; unsafe {
+		return true
+	}
+
+	if preventDuplicates {
+		prefix := upperKey + "="
+		for _, kv := range *e {
+			if strings.HasPrefix(strings.ToUpper(kv), prefix) {
+				return true
+			}
+		}
+	}
+
+	return false
+}
+
+// AddExecEnvironment will add safe values from [os.Environ], ignoring any duplicates that may have already been added.
 func (e *SafeEnv) AddExecEnvironment() {
-	e.AddFull(os.Environ()...)
+	e.addFull(true, os.Environ())
 }