diff --git a/lib/srv/ctx.go b/lib/srv/ctx.go index d39734d55d367..836e06146eb38 100644 --- a/lib/srv/ctx.go +++ b/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" ) @@ -1189,12 +1190,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. @@ -1206,9 +1205,8 @@ 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)) } } @@ -1216,21 +1214,21 @@ func buildEnvironment(ctx *ServerContext) []string { 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 { diff --git a/lib/srv/reexec.go b/lib/srv/reexec.go index 50720d791592a..d1d7cbd7b32f9 100644 --- a/lib/srv/reexec.go +++ b/lib/srv/reexec.go @@ -46,6 +46,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 @@ -788,7 +789,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, @@ -797,21 +798,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 @@ -949,11 +953,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, diff --git a/lib/utils/environment_test.go b/lib/utils/environment_test.go deleted file mode 100644 index ac206c1bbea39..0000000000000 --- a/lib/utils/environment_test.go +++ /dev/null @@ -1,56 +0,0 @@ -/* -Copyright 2017 Gravitational, Inc. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ -package utils - -import ( - "os" - "testing" - - "github.com/google/go-cmp/cmp" - "github.com/stretchr/testify/require" -) - -func TestReadEnvironmentFile(t *testing.T) { - t.Parallel() - - // contents of environment file - rawenv := []byte(` -foo=bar -# comment -foo=bar=baz - # comment 2 -= -foo= - -=bar -`) - - // create a temp file with an environment in it - f, err := os.CreateTemp("", "teleport-environment-") - require.NoError(t, err) - defer os.Remove(f.Name()) - _, err = f.Write(rawenv) - require.NoError(t, err) - err = f.Close() - require.NoError(t, err) - - // read in the temp file - env, err := ReadEnvironmentFile(f.Name()) - require.NoError(t, err) - - // check we parsed it correctly - require.Empty(t, cmp.Diff(env, []string{"foo=bar", "foo=bar=baz", "foo="})) -} diff --git a/lib/utils/environment.go b/lib/utils/envutils/environment.go similarity index 61% rename from lib/utils/environment.go rename to lib/utils/envutils/environment.go index eaa98bb81d20a..e025f4f1ed9bb 100644 --- a/lib/utils/environment.go +++ b/lib/utils/envutils/environment.go @@ -12,10 +12,11 @@ // See the License for the specific language governing permissions and // limitations under the License. -package utils +package envutils import ( "bufio" + "fmt" "os" "strings" @@ -38,7 +39,7 @@ func ReadEnvironmentFile(filename string) ([]string, error) { defer file.Close() var lineno int - var envs []string + env := &SafeEnv{} scanner := bufio.NewScanner(file) for scanner.Scan() { @@ -49,7 +50,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 @@ -72,7 +73,7 @@ func ReadEnvironmentFile(filename string) ([]string, error) { continue } - envs = append(envs, key+"="+value) + env.Add(key, value) } err = scanner.Err() @@ -81,5 +82,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()...) } diff --git a/lib/utils/envutils/environment_test.go b/lib/utils/envutils/environment_test.go new file mode 100644 index 0000000000000..69ea7647397e3 --- /dev/null +++ b/lib/utils/envutils/environment_test.go @@ -0,0 +1,172 @@ +/* +Copyright 2017 Gravitational, Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ +package envutils + +import ( + "os" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/require" +) + +func TestReadEnvironmentFile(t *testing.T) { + t.Parallel() + + // contents of environment file + rawenv := []byte(` +foo=bar +# comment +foo=bar=baz + # comment 2 += +foo= + +=bar +LD_PRELOAD=attack +`) + + // create a temp file with an environment in it + f, err := os.CreateTemp(t.TempDir(), "teleport-environment-") + require.NoError(t, err) + defer os.Remove(f.Name()) + _, err = f.Write(rawenv) + require.NoError(t, err) + err = f.Close() + require.NoError(t, err) + + // read in the temp file + env, err := ReadEnvironmentFile(f.Name()) + require.NoError(t, err) + + // check we parsed it correctly + require.Empty(t, cmp.Diff(env, []string{"foo=bar", "foo=bar=baz", "foo="})) +} + +func TestSafeEnvAdd(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + keys []string + values []string + expected []string + }{ + { + name: "normal add", + 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: "whitespace trim", + 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: "skip dangerous lowercase", + keys: []string{"foo", "ld_preload"}, + values: []string{"bar", "ignored"}, + expected: []string{"foo=bar"}, + }, + { + name: "skip dangerous with whitespace", + keys: []string{"foo", " LD_PRELOAD"}, + values: []string{"bar", "ignored"}, + expected: []string{"foo=bar"}, + }, + } + + for _, tc := range testCases { + tc := tc // capture range variable + t.Run(tc.name, func(t *testing.T) { + require.Len(t, tc.keys, len(tc.values)) + + env := &SafeEnv{} + for i := range tc.keys { + env.Add(tc.keys[i], tc.values[i]) + } + result := []string(*env) + + require.Equal(t, tc.expected, result) + }) + } +} + +func TestSafeEnvAddFull(t *testing.T) { + testCases := []struct { + name string + fullValues []string + expected []string + }{ + { + name: "normal add", + fullValues: []string{"foo=bar"}, + expected: []string{"foo=bar"}, + }, + { + name: "double add", + fullValues: []string{"one=v1", "two=v2"}, + expected: []string{"one=v1", "two=v2"}, + }, + { + name: "whitespace trim", + fullValues: []string{" foo=bar "}, + expected: []string{"foo=bar"}, + }, + { + name: "skip dangerous exact", + 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 with whitespace", + fullValues: []string{"foo=bar", " LD_PRELOAD=ignored"}, + expected: []string{"foo=bar"}, + }, + } + + for _, tc := range testCases { + tc := tc // capture range variable + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + env := &SafeEnv{} + env.AddFull(tc.fullValues...) + result := []string(*env) + + require.Equal(t, tc.expected, result) + }) + } +}