diff --git a/ssh/tailssh/accept_env.go b/ssh/tailssh/accept_env.go index 6354d41d7..64f979c2a 100644 --- a/ssh/tailssh/accept_env.go +++ b/ssh/tailssh/accept_env.go @@ -9,6 +9,17 @@ "strings" ) +// isDangerousEnvVar reports whether the given environment variable name +// is unconditionally prohibited from being forwarded, regardless of +// acceptEnv policy. This prevents privilege escalation via dynamic +// linker environment variables (e.g. LD_PRELOAD, LD_LIBRARY_PATH, +// DYLD_INSERT_LIBRARIES) even when a wildcard acceptEnv pattern like +// "*" is configured. +func isDangerousEnvVar(name string) bool { + upper := strings.ToUpper(name) + return strings.HasPrefix(upper, "LD_") || strings.HasPrefix(upper, "DYLD_") +} + // filterEnv filters a passed in environ string slice (a slice with strings // representing environment variables in the form "key=value") based on // the supplied slice of acceptEnv values. @@ -18,6 +29,10 @@ // // acceptEnv values may contain * and ? wildcard characters which match against // zero or one or more characters and a single character respectively. +// +// Certain dangerous environment variables (such as those controlling the +// dynamic linker) are always rejected regardless of the acceptEnv policy. +// See isDangerousEnvVar. func filterEnv(acceptEnv []string, environ []string) ([]string, error) { var acceptedPairs []string @@ -32,6 +47,12 @@ func filterEnv(acceptEnv []string, environ []string) ([]string, error) { return nil, fmt.Errorf(`invalid environment variable: %q. Variables must be in "KEY=VALUE" format`, envPair) } + // Always reject dangerous environment variables that could + // enable privilege escalation, regardless of acceptEnv policy. + if isDangerousEnvVar(variableName) { + continue + } + // Short circuit if we have a direct match between the environment // variable and an AcceptEnv value. if slices.Contains(acceptEnv, variableName) { diff --git a/ssh/tailssh/accept_env_test.go b/ssh/tailssh/accept_env_test.go index fef13877a..bf37c581e 100644 --- a/ssh/tailssh/accept_env_test.go +++ b/ssh/tailssh/accept_env_test.go @@ -10,6 +10,39 @@ "github.com/google/go-cmp/cmp" ) +func TestIsDangerousEnvVar(t *testing.T) { + tests := []struct { + name string + dangerous bool + }{ + {"LD_PRELOAD", true}, + {"LD_LIBRARY_PATH", true}, + {"LD_AUDIT", true}, + {"LD_DEBUG", true}, + {"LD_PROFILE", true}, + {"ld_preload", true}, + {"DYLD_INSERT_LIBRARIES", true}, + {"DYLD_LIBRARY_PATH", true}, + {"DYLD_FRAMEWORK_PATH", true}, + {"dyld_insert_libraries", true}, + {"TERM", false}, + {"LANG", false}, + {"LC_ALL", false}, + {"PATH", false}, + {"HOME", false}, + {"LDFLAGS", false}, + {"MY_LD_PRELOAD", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := isDangerousEnvVar(tt.name); got != tt.dangerous { + t.Errorf("isDangerousEnvVar(%q) = %v, want %v", tt.name, got, tt.dangerous) + } + }) + } +} + func TestMatchAcceptEnvPattern(t *testing.T) { testCases := []struct { pattern string @@ -135,6 +168,42 @@ func TestFilterEnv(t *testing.T) { expectedFiltered: nil, wantErrMessage: `invalid environment variable: "FOOBAR". Variables must be in "KEY=VALUE" format`, }, + { + name: "ld-preload-rejected-with-wildcard", + acceptEnv: []string{"*"}, + environ: []string{"LD_PRELOAD=/tmp/evil.so", "TERM=xterm"}, + expectedFiltered: []string{"TERM=xterm"}, + }, + { + name: "ld-vars-rejected-with-wildcard", + acceptEnv: []string{"*"}, + environ: []string{"LD_PRELOAD=/tmp/evil.so", "LD_LIBRARY_PATH=/tmp", "LD_AUDIT=/tmp/audit.so", "SAFE_VAR=ok"}, + expectedFiltered: []string{"SAFE_VAR=ok"}, + }, + { + name: "ld-vars-rejected-with-explicit-match", + acceptEnv: []string{"LD_PRELOAD", "LD_LIBRARY_PATH"}, + environ: []string{"LD_PRELOAD=/tmp/evil.so", "LD_LIBRARY_PATH=/tmp"}, + expectedFiltered: nil, + }, + { + name: "ld-vars-rejected-with-prefix-pattern", + acceptEnv: []string{"LD_*"}, + environ: []string{"LD_PRELOAD=/tmp/evil.so", "LD_LIBRARY_PATH=/tmp"}, + expectedFiltered: nil, + }, + { + name: "ld-vars-case-insensitive", + acceptEnv: []string{"*"}, + environ: []string{"ld_preload=/tmp/evil.so", "Ld_Library_Path=/tmp", "SAFE=ok"}, + expectedFiltered: []string{"SAFE=ok"}, + }, + { + name: "dyld-vars-rejected", + acceptEnv: []string{"*"}, + environ: []string{"DYLD_INSERT_LIBRARIES=/tmp/evil.dylib", "DYLD_LIBRARY_PATH=/tmp", "TERM=xterm"}, + expectedFiltered: []string{"TERM=xterm"}, + }, } for _, tc := range testCases {