From e68866db1b25858ab779e12d309ead03958172cd Mon Sep 17 00:00:00 2001 From: Mike Jensen Date: Wed, 13 Dec 2023 14:49:24 -0700 Subject: [PATCH 1/2] Better control on user injected environment values This commit includes two changes: * In `environment` we expanded the list of MacOS environment values which should be filtered. It was demonstrated that these can be used to get code execution in MacOS. * In `reexec` we no longer provide the cmdmsg.Environment for the `teleport exec`. As part of the development of a9055bccd9ef24e8f5cb691fae36a2e1fdc227c0 it was attempted to fully clear out the environment, but testing showed that to be potentially problematic. It was believed the safest option was to use the cmd environment, however this introduces a new source of environment variables. Because this exec happens under `root` this is particularly dangerous (even more so when combined with the missed OSX values mentioned above). As such we now are only providing the filtered exec environment, which is a closer (but safer) option to the functionality prior to a9055bccd9ef24e8f5cb691fae36a2e1fdc227c0. --- lib/srv/exec_linux_test.go | 17 +++++++++++++++++ lib/srv/reexec.go | 1 - lib/utils/envutils/environment.go | 20 +++++++++++++++----- 3 files changed, 32 insertions(+), 6 deletions(-) diff --git a/lib/srv/exec_linux_test.go b/lib/srv/exec_linux_test.go index debe0e2ab997d..6b4d208bc486b 100644 --- a/lib/srv/exec_linux_test.go +++ b/lib/srv/exec_linux_test.go @@ -115,6 +115,23 @@ func TestOSCommandPrep(t *testing.T) { require.Equal(t, expectedEnv, cmd.Env) } +func TestConfigureCommand(t *testing.T) { + srv := newMockServer(t) + scx := newExecServerContext(t, srv) + + unexpectedKey := "FOO" + unexpectedValue := "BAR" + // environment values in the server context should not be forwarded + scx.SetEnv(unexpectedKey, unexpectedValue) + + cmd, err := ConfigureCommand(scx) + require.NoError(t, err) + + require.NotNil(t, cmd) + require.Equal(t, "/proc/self/exe", cmd.Path) + require.NotContains(t, cmd.Env, unexpectedKey+"="+unexpectedValue) +} + // TestContinue tests if the process hangs if a continue signal is not sent // and makes sure the process continues once it has been sent. func TestContinue(t *testing.T) { diff --git a/lib/srv/reexec.go b/lib/srv/reexec.go index 90e3c22dddf40..82dc678e52963 100644 --- a/lib/srv/reexec.go +++ b/lib/srv/reexec.go @@ -961,7 +961,6 @@ func ConfigureCommand(ctx *ServerContext, extraFiles ...*os.File) (*exec.Cmd, er // build env for `teleport exec` env := &envutils.SafeEnv{} - 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 7eaf8c5f457c9..9c1cc65588b11 100644 --- a/lib/utils/envutils/environment.go +++ b/lib/utils/envutils/environment.go @@ -93,6 +93,7 @@ func ReadEnvironmentFile(filename string) ([]string, error) { var unsafeEnvironmentVars = map[string]struct{}{ // Linux + // values taken from 'man ld.so' https://man7.org/linux/man-pages/man8/ld.so.8.html "LD_ASSUME_KERNEL": {}, "LD_AUDIT": {}, "LD_BIND_NOW": {}, @@ -108,8 +109,17 @@ var unsafeEnvironmentVars = map[string]struct{}{ "LD_RPATH": {}, "LD_USE_LOAD_BIAS": {}, // macOS - "DYLD_INSERT_LIBRARIES": {}, - "DYLD_LIBRARY_PATH": {}, + // values taken from 'man dyld' https://www.manpagez.com/man/1/dyld/ + "DYLD_FRAMEWORK_PATH": {}, + "DYLD_FALLBACK_FRAMEWORK_PATH": {}, + "DYLD_VERSIONED_FRAMEWORK_PATH": {}, + "DYLD_LIBRARY_PATH": {}, + "DYLD_FALLBACK_LIBRARY_PATH": {}, + "DYLD_VERSIONED_LIBRARY_PATH": {}, + "DYLD_IMAGE_SUFFIX": {}, + "DYLD_INSERT_LIBRARIES": {}, + "DYLD_SHARED_REGION": {}, + "DYLD_SHARED_CACHE_DIR:": {}, } // SafeEnv allows you to build a system environment while avoiding potentially dangerous environment conditions. In @@ -132,7 +142,7 @@ func (e *SafeEnv) AddUnique(k, v string) { func (e *SafeEnv) add(preventDuplicates bool, k, v string) { k = strings.TrimSpace(k) v = strings.TrimSpace(v) - if e.unsafeKey(preventDuplicates, k) { + if e.isUnsafeKey(preventDuplicates, k) { return } @@ -158,7 +168,7 @@ func (e *SafeEnv) addFull(preventDuplicates bool, fullValues []string) { kv = strings.TrimSpace(kv) key := strings.SplitN(kv, "=", 2)[0] - if e.unsafeKey(preventDuplicates, key) { + if e.isUnsafeKey(preventDuplicates, key) { continue } @@ -166,7 +176,7 @@ func (e *SafeEnv) addFull(preventDuplicates bool, fullValues []string) { } } -func (e *SafeEnv) unsafeKey(preventDuplicates bool, key string) bool { +func (e *SafeEnv) isUnsafeKey(preventDuplicates bool, key string) bool { if key == "" || key == "=" { return false } From 062a5d8525f3b7568ddda6801e354bfd29e695cd Mon Sep 17 00:00:00 2001 From: Mike Jensen Date: Fri, 1 Dec 2023 10:41:44 -0700 Subject: [PATCH 2/2] Update filter to exclude all DYLD_ and LD_ prefixed variables --- lib/utils/envutils/environment.go | 38 ++++++++----------------------- 1 file changed, 9 insertions(+), 29 deletions(-) diff --git a/lib/utils/envutils/environment.go b/lib/utils/envutils/environment.go index 9c1cc65588b11..5c3ad51e359dd 100644 --- a/lib/utils/envutils/environment.go +++ b/lib/utils/envutils/environment.go @@ -91,35 +91,13 @@ func ReadEnvironmentFile(filename string) ([]string, error) { return *env, nil } -var unsafeEnvironmentVars = map[string]struct{}{ +var unsafeEnvironmentPrefixes = []string{ // Linux - // values taken from 'man ld.so' https://man7.org/linux/man-pages/man8/ld.so.8.html - "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": {}, + // Covering cases from LD (man ld.so) to prevent injection like LD_PRELOAD + "LD_", // macOS - // values taken from 'man dyld' https://www.manpagez.com/man/1/dyld/ - "DYLD_FRAMEWORK_PATH": {}, - "DYLD_FALLBACK_FRAMEWORK_PATH": {}, - "DYLD_VERSIONED_FRAMEWORK_PATH": {}, - "DYLD_LIBRARY_PATH": {}, - "DYLD_FALLBACK_LIBRARY_PATH": {}, - "DYLD_VERSIONED_LIBRARY_PATH": {}, - "DYLD_IMAGE_SUFFIX": {}, - "DYLD_INSERT_LIBRARIES": {}, - "DYLD_SHARED_REGION": {}, - "DYLD_SHARED_CACHE_DIR:": {}, + // Covering cases from DYLD (man dyld) to prevent injection like DYLD_LIBRARY_PATH + "DYLD_", } // SafeEnv allows you to build a system environment while avoiding potentially dangerous environment conditions. In @@ -182,8 +160,10 @@ func (e *SafeEnv) isUnsafeKey(preventDuplicates bool, key string) bool { } upperKey := strings.ToUpper(key) - if _, unsafe := unsafeEnvironmentVars[upperKey]; unsafe { - return true + for _, prefix := range unsafeEnvironmentPrefixes { + if strings.HasPrefix(upperKey, prefix) { + return true + } } if preventDuplicates {