Skip to content

Commit

Permalink
Filter dangerous environment variables before reexec (#34177)
Browse files Browse the repository at this point in the history
* Filter dangerous environment variables before reexec

This change filters potentially dangerous environment variables that could result in code execution.

This seemed safest to integrate as a new struct within the `environment.go` in utils.  This struct allows us to validate variables as they are built, making sure that regardless of the source we have validated them against our filter list.

The environment specific logic was significant enough that the current and new logic was refactored into a new package `envutils`.

* Allow the easy addition of execution environment into SafeEnv

In addition this commit adds in a check to look for duplicate keys which may be attempting to overload our set values.

* Apply PR Feedback and remove env duplicate handling

* Apply additional PR feedback
  • Loading branch information
jentfoo committed Nov 6, 2023
1 parent a67dc0a commit a9055bc
Show file tree
Hide file tree
Showing 5 changed files with 259 additions and 81 deletions.
26 changes: 12 additions & 14 deletions lib/srv/ctx.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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.
Expand All @@ -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 {
Expand Down
20 changes: 15 additions & 5 deletions lib/srv/reexec.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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
Expand Down Expand Up @@ -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,
Expand Down
56 changes: 0 additions & 56 deletions lib/utils/environment_test.go

This file was deleted.

66 changes: 60 additions & 6 deletions lib/utils/environment.go → lib/utils/envutils/environment.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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() {
Expand All @@ -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
Expand All @@ -71,7 +74,7 @@ func ReadEnvironmentFile(filename string) ([]string, error) {
continue
}

envs = append(envs, key+"="+value)
env.Add(key, value)
}

err = scanner.Err()
Expand All @@ -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()...)
}
Loading

0 comments on commit a9055bc

Please sign in to comment.