diff --git a/lib/srv/ctx.go b/lib/srv/ctx.go index 836e06146eb38..a9a1297886cbe 100644 --- a/lib/srv/ctx.go +++ b/lib/srv/ctx.go @@ -1192,9 +1192,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()) @@ -1205,8 +1202,8 @@ 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)) } } @@ -1214,19 +1211,22 @@ func buildEnvironment(ctx *ServerContext) []string { 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 } diff --git a/lib/srv/reexec.go b/lib/srv/reexec.go index d1d7cbd7b32f9..3677ef86aa0d2 100644 --- a/lib/srv/reexec.go +++ b/lib/srv/reexec.go @@ -798,22 +798,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 @@ -955,7 +956,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. diff --git a/lib/utils/envutils/environment.go b/lib/utils/envutils/environment.go index e025f4f1ed9bb..da07a1eb32a8c 100644 --- a/lib/utils/envutils/environment.go +++ b/lib/utils/envutils/environment.go @@ -73,7 +73,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() @@ -85,53 +86,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()) } diff --git a/lib/utils/envutils/environment_test.go b/lib/utils/envutils/environment_test.go index 69ea7647397e3..38b856fb2d014 100644 --- a/lib/utils/envutils/environment_test.go +++ b/lib/utils/envutils/environment_test.go @@ -36,6 +36,7 @@ foo=bar=baz foo= =bar +bar=foo LD_PRELOAD=attack `) @@ -53,53 +54,81 @@ LD_PRELOAD=attack require.NoError(t, err) // check we parsed it correctly - require.Empty(t, cmp.Diff(env, []string{"foo=bar", "foo=bar=baz", "foo="})) + require.Empty(t, cmp.Diff(env, []string{"foo=bar", "foo=bar=baz", "foo=", "bar=foo"})) } func TestSafeEnvAdd(t *testing.T) { t.Parallel() testCases := []struct { - name string - keys []string - values []string - expected []string + name string + excludeDuplicate bool + keys []string + values []string + expected []string }{ { - name: "normal add", - keys: []string{"foo"}, - values: []string{"bar"}, - expected: []string{"foo=bar"}, + name: "normal add", + excludeDuplicate: true, + keys: []string{"foo"}, + values: []string{"bar"}, + expected: []string{"foo=bar"}, }, { - name: "double add", - keys: []string{"one", "two"}, - values: []string{"v1", "v2"}, - expected: []string{"one=v1", "two=v2"}, + name: "double add", + excludeDuplicate: true, + keys: []string{"one", "two"}, + values: []string{"v1", "v2"}, + expected: []string{"one=v1", "two=v2"}, }, { - name: "whitespace trim", - keys: []string{" foo "}, - values: []string{" bar "}, - expected: []string{"foo=bar"}, + name: "whitespace trim", + excludeDuplicate: true, + keys: []string{" foo "}, + values: []string{" bar "}, + expected: []string{"foo=bar"}, }, { - name: "skip dangerous exact", - keys: []string{"foo", "LD_PRELOAD"}, - values: []string{"bar", "ignored"}, - expected: []string{"foo=bar"}, + name: "duplicate ignore", + excludeDuplicate: true, + keys: []string{"one", "one"}, + values: []string{"v1", "v2"}, + expected: []string{"one=v1"}, }, { - name: "skip dangerous lowercase", - keys: []string{"foo", "ld_preload"}, - values: []string{"bar", "ignored"}, - expected: []string{"foo=bar"}, + name: "duplicate different case ignore", + excludeDuplicate: true, + keys: []string{"one", "ONE"}, + values: []string{"v1", "v2"}, + expected: []string{"one=v1"}, }, { - name: "skip dangerous with whitespace", - keys: []string{"foo", " LD_PRELOAD"}, - values: []string{"bar", "ignored"}, - expected: []string{"foo=bar"}, + name: "duplicate allowed", + excludeDuplicate: false, + keys: []string{"one", "one"}, + values: []string{"v1", "v2"}, + expected: []string{"one=v1", "one=v2"}, + }, + { + name: "skip dangerous exact", + excludeDuplicate: true, + keys: []string{"foo", "LD_PRELOAD"}, + values: []string{"bar", "ignored"}, + expected: []string{"foo=bar"}, + }, + { + name: "skip dangerous lowercase", + excludeDuplicate: true, + keys: []string{"foo", "ld_preload"}, + values: []string{"bar", "ignored"}, + expected: []string{"foo=bar"}, + }, + { + name: "skip dangerous with whitespace", + excludeDuplicate: true, + keys: []string{"foo", " LD_PRELOAD"}, + values: []string{"bar", "ignored"}, + expected: []string{"foo=bar"}, }, } @@ -110,7 +139,7 @@ func TestSafeEnvAdd(t *testing.T) { env := &SafeEnv{} for i := range tc.keys { - env.Add(tc.keys[i], tc.values[i]) + env.add(tc.excludeDuplicate, tc.keys[i], tc.values[i]) } result := []string(*env) @@ -121,39 +150,70 @@ func TestSafeEnvAdd(t *testing.T) { func TestSafeEnvAddFull(t *testing.T) { testCases := []struct { - name string - fullValues []string - expected []string + name string + excludeDuplicate bool + fullValues []string + expected []string }{ { - name: "normal add", - fullValues: []string{"foo=bar"}, - expected: []string{"foo=bar"}, + name: "normal add", + excludeDuplicate: true, + fullValues: []string{"foo=bar"}, + expected: []string{"foo=bar"}, + }, + { + name: "double add", + excludeDuplicate: true, + fullValues: []string{"one=v1", "two=v2"}, + expected: []string{"one=v1", "two=v2"}, + }, + { + name: "whitespace trim", + excludeDuplicate: true, + fullValues: []string{" foo=bar "}, + expected: []string{"foo=bar"}, + }, + { + name: "duplicate ignore", + excludeDuplicate: true, + fullValues: []string{"one=v1", "one=v2"}, + expected: []string{"one=v1"}, + }, + { + name: "duplicate ignore different case", + excludeDuplicate: true, + fullValues: []string{"one=v1", "ONE=v2"}, + expected: []string{"one=v1"}, }, { - name: "double add", - fullValues: []string{"one=v1", "two=v2"}, - expected: []string{"one=v1", "two=v2"}, + name: "duplicate allowed", + excludeDuplicate: false, + fullValues: []string{"one=v1", "one=v2"}, + expected: []string{"one=v1", "one=v2"}, }, { - name: "whitespace trim", - fullValues: []string{" foo=bar "}, - expected: []string{"foo=bar"}, + name: "double equal value", + excludeDuplicate: true, + fullValues: []string{"foo=bar=badvalue"}, + expected: []string{"foo=bar=badvalue"}, }, { - name: "skip dangerous exact", - fullValues: []string{"foo=bar", "LD_PRELOAD=ignored"}, - expected: []string{"foo=bar"}, + name: "skip dangerous exact", + excludeDuplicate: true, + fullValues: []string{"foo=bar", "LD_PRELOAD=ignored"}, + expected: []string{"foo=bar"}, }, { - name: "skip dangerous lowercase", - fullValues: []string{"foo=bar", "ld_preload=ignored"}, - expected: []string{"foo=bar"}, + name: "skip dangerous lowercase", + excludeDuplicate: true, + fullValues: []string{"foo=bar", "ld_preload=ignored"}, + expected: []string{"foo=bar"}, }, { - name: "skip dangerous with whitespace", - fullValues: []string{"foo=bar", " LD_PRELOAD=ignored"}, - expected: []string{"foo=bar"}, + name: "skip dangerous with whitespace", + excludeDuplicate: true, + fullValues: []string{"foo=bar", " LD_PRELOAD=ignored"}, + expected: []string{"foo=bar"}, }, } @@ -163,7 +223,7 @@ func TestSafeEnvAddFull(t *testing.T) { t.Parallel() env := &SafeEnv{} - env.AddFull(tc.fullValues...) + env.addFull(tc.excludeDuplicate, tc.fullValues) result := []string(*env) require.Equal(t, tc.expected, result)