From 797fa5b9bc3063aaf46e65e69a974de13cd004c2 Mon Sep 17 00:00:00 2001 From: Patrick O'Doherty Date: Fri, 17 Jul 2026 00:48:42 +0000 Subject: [PATCH] ssh/tailssh: keep acceptEnv values and names out of the incubator cmdline Previously the acceptEnv variables forwarded to the incubator child were JSON-encoded onto its command line (--encoded-env), so their values were visible in /proc//cmdline to any other local user and were logged in the session-start argv (locally and to log.tailscale.com except where --no-logs-no-support was specified). Carry the accepted "KEY=VALUE" pairs to the child via its environment (cmd.Env) instead of the argv. The key names are carried alongside them in a dedicated TS_SSH_ALLOWED_ENV_KEYS variable rather than on the argv, so the incubator can rebuild the "su -w" allowlist. The incubator strips TS_SSH_ALLOWED_ENV_KEYS from the environment before handing off to the user's process. Adds incubator_test.go and accept_env_test.go coverage: a forwarded secret is delivered via cmd.Env and neither its value nor its key name appears on cmd.Args. Updates tailscale/corp#44903 Signed-off-by: Patrick O'Doherty --- ssh/tailssh/accept_env.go | 57 ++++++++ ssh/tailssh/accept_env_test.go | 89 +++++++++++ ssh/tailssh/incubator.go | 135 +++++++++-------- ssh/tailssh/incubator_plan9.go | 79 +++++----- ssh/tailssh/incubator_test.go | 259 +++++++++++++++++++++++++++++++++ ssh/tailssh/tailssh.go | 22 +++ ssh/tailssh/tailssh_test.go | 54 +++++++ 7 files changed, 593 insertions(+), 102 deletions(-) create mode 100644 ssh/tailssh/incubator_test.go diff --git a/ssh/tailssh/accept_env.go b/ssh/tailssh/accept_env.go index 64f979c2a..02922047e 100644 --- a/ssh/tailssh/accept_env.go +++ b/ssh/tailssh/accept_env.go @@ -20,6 +20,56 @@ func isDangerousEnvVar(name string) bool { return strings.HasPrefix(upper, "LD_") || strings.HasPrefix(upper, "DYLD_") } +// allowedEnvKeysEnv is the name of the environment variable used to pass the +// comma-separated list of client-forwarded environment variable names from the +// parent (tailscaled) to the incubator child. The names are passed via the +// environment rather than on the argv so they never appear in the child's +// /proc//cmdline (visible to other local users) or in process logs. The +// incubator strips this variable from the environment before handing off to the +// user's process (see stripAllowedEnvKeys). +// +// Because the child reads this back out of its own environment (which also +// contains the client-forwarded variables) a client that could forward a +// variable of this exact name, or a key containing the "," separator, could +// spoof or widen the reconstructed "su -w" allowlist. filterEnv rejects both, +// so this name is reserved and never client-controllable. +const allowedEnvKeysEnv = "TS_SSH_ALLOWED_ENV_KEYS" + +// reservedEnvKey reports whether name must never be accepted from the client as +// a forwarded environment variable, independent of the acceptEnv policy. A key +// is reserved if it collides with the allowedEnvKeysEnv bookkeeping variable or +// if it contains the "," character used to separate key names within that +// variable (which would let a client inject additional entries into the +// incubator's "su -w" allowlist). An empty key name is likewise rejected as +// malformed. +func reservedEnvKey(name string) bool { + return name == "" || name == allowedEnvKeysEnv || strings.Contains(name, ",") +} + +// stripAllowedEnvKeys removes every occurrence of the allowedEnvKeysEnv +// bookkeeping variable from environ and returns the remaining environment along +// with the key names that variable carried (its comma-separated value, empty +// entries skipped). It filters environ in place (via slices.DeleteFunc), so the +// caller must pass a slice it owns, such as a fresh os.Environ(). It is the +// child-side counterpart to forwardedEnvAndKeys and is shared by the +// platform-specific forwardedEnviron implementations so the stripping and +// allowlist reconstruction behave identically everywhere. +func stripAllowedEnvKeys(environ []string) (env, keys []string) { + env = slices.DeleteFunc(environ, func(kv string) bool { + k, v, ok := strings.Cut(kv, "=") + if !ok || k != allowedEnvKeysEnv { + return false + } + for _, ek := range strings.Split(v, ",") { + if ek != "" { + keys = append(keys, ek) + } + } + return true + }) + return env, keys +} + // 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. @@ -53,6 +103,13 @@ func filterEnv(acceptEnv []string, environ []string) ([]string, error) { continue } + // Always reject names reserved for our own parent->child bookkeeping + // (see reservedEnvKey), regardless of acceptEnv policy, so a client + // cannot spoof or widen the incubator's "su -w" allowlist. + if reservedEnvKey(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 bf37c581e..f5703a15d 100644 --- a/ssh/tailssh/accept_env_test.go +++ b/ssh/tailssh/accept_env_test.go @@ -5,6 +5,8 @@ import ( "fmt" + "slices" + "strings" "testing" "github.com/google/go-cmp/cmp" @@ -204,6 +206,31 @@ func TestFilterEnv(t *testing.T) { environ: []string{"DYLD_INSERT_LIBRARIES=/tmp/evil.dylib", "DYLD_LIBRARY_PATH=/tmp", "TERM=xterm"}, expectedFiltered: []string{"TERM=xterm"}, }, + { + // A client must not be able to forward the parent->child + // bookkeeping variable, even under a wildcard policy: doing so + // would let it spoof the incubator's "su -w" allowlist. + name: "reserved-bookkeeping-key-rejected", + acceptEnv: []string{"*"}, + environ: []string{allowedEnvKeysEnv + "=EVIL1,EVIL2", "SAFE=ok"}, + expectedFiltered: []string{"SAFE=ok"}, + }, + { + // A forwarded key containing the "," allowlist separator must be + // rejected, since it would otherwise inject extra "su -w" entries. + name: "comma-in-key-rejected", + acceptEnv: []string{"*"}, + environ: []string{"A,B=x", "GOOD=1"}, + expectedFiltered: []string{"GOOD=1"}, + }, + { + // Even an explicit (non-wildcard) allowlist entry cannot override + // the reserved-key rejection. + name: "reserved-key-rejected-explicit-match", + acceptEnv: []string{allowedEnvKeysEnv}, + environ: []string{allowedEnvKeysEnv + "=EVIL", "TERM=xterm"}, + expectedFiltered: nil, + }, } for _, tc := range testCases { @@ -223,3 +250,65 @@ func TestFilterEnv(t *testing.T) { }) } } + +func TestStripAllowedEnvKeys(t *testing.T) { + tests := []struct { + name string + environ []string + wantEnv []string + wantKeys []string + }{ + { + name: "unset", + environ: []string{"PATH=/bin", "HOME=/home/alice"}, + wantEnv: []string{"PATH=/bin", "HOME=/home/alice"}, + wantKeys: nil, + }, + { + name: "single", + environ: []string{"PATH=/bin", allowedEnvKeysEnv + "=FOO,BAR", "FOO=1", "BAR=2"}, + wantEnv: []string{"PATH=/bin", "FOO=1", "BAR=2"}, + wantKeys: []string{"FOO", "BAR"}, + }, + { + // Defense in depth: even if two copies of the bookkeeping var reach + // the child (e.g. a client spoof plus the parent's real one), ALL + // occurrences must be stripped so none survives into the user's + // process, and the allowlist must not depend on execve dedup order. + name: "duplicate-all-stripped", + environ: []string{allowedEnvKeysEnv + "=EVIL", "PATH=/bin", allowedEnvKeysEnv + "=REAL"}, + wantEnv: []string{"PATH=/bin"}, + wantKeys: []string{"EVIL", "REAL"}, + }, + { + // A never-strips a look-alike prefix. + name: "prefix-not-stripped", + environ: []string{allowedEnvKeysEnv + "_EXTRA=keep", "PATH=/bin"}, + wantEnv: []string{allowedEnvKeysEnv + "_EXTRA=keep", "PATH=/bin"}, + wantKeys: nil, + }, + { + name: "empty-value-yields-no-keys", + environ: []string{allowedEnvKeysEnv + "=", "PATH=/bin"}, + wantEnv: []string{"PATH=/bin"}, + wantKeys: nil, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + gotEnv, gotKeys := stripAllowedEnvKeys(slices.Clone(tc.environ)) + // No occurrence of the bookkeeping var may remain. + for _, kv := range gotEnv { + if k, _, _ := strings.Cut(kv, "="); k == allowedEnvKeysEnv { + t.Errorf("%s survived stripping: env = %q", allowedEnvKeysEnv, gotEnv) + } + } + if diff := cmp.Diff(tc.wantEnv, gotEnv); diff != "" { + t.Errorf("env (-want,+got):\n%s", diff) + } + if !slices.Equal(gotKeys, tc.wantKeys) { + t.Errorf("keys = %q, want %q", gotKeys, tc.wantKeys) + } + }) + } +} diff --git a/ssh/tailssh/incubator.go b/ssh/tailssh/incubator.go index 48c65e8e5..d279bc5c0 100644 --- a/ssh/tailssh/incubator.go +++ b/ssh/tailssh/incubator.go @@ -13,7 +13,6 @@ import ( "context" - "encoding/json" "errors" "flag" "fmt" @@ -114,8 +113,12 @@ func tryExecInDir(ctx context.Context, dir string) error { // behavior of SSHD when by falling back to the root directory if it cannot run // a command in the user’s home directory. // -// The returned Cmd.Env is guaranteed to be nil; the caller populates it. -func (ss *sshSession) newIncubatorCommand(logf logger.Logf) (cmd *exec.Cmd, err error) { +// It also returns forwardedEnv, the set of client-forwarded "KEY=VALUE" +// environment variables accepted by the acceptEnv policy. These may contain +// secrets, so the caller must pass them to the child via its environment +// (cmd.Env) rather than on the command line, where they would leak via +// /proc//cmdline and the session-start argv log. +func (ss *sshSession) newIncubatorCommand(logf logger.Logf) (cmd *exec.Cmd, forwardedEnv []string, err error) { defer func() { if cmd != nil && cmd.Env != nil { panic("internal error") @@ -136,7 +139,7 @@ func (ss *sshSession) newIncubatorCommand(logf logger.Logf) (cmd *exec.Cmd, err if isSFTP { // SFTP relies on the embedded Go-based SFTP server in tailscaled, // so without tailscaled, we can't serve SFTP. - return nil, errors.New("no tailscaled found on path, can't serve SFTP") + return nil, nil, errors.New("no tailscaled found on path, can't serve SFTP") } loginShell := ss.conn.localUser.LoginShell() @@ -163,13 +166,13 @@ func (ss *sshSession) newIncubatorCommand(logf logger.Logf) (cmd *exec.Cmd, err // we will try to run this command in the root directory. cmd.Dir = "/" } else { - return nil, err + return nil, nil, err } case err != nil: - return nil, err + return nil, nil, err } - return cmd, nil + return cmd, nil, nil } lu := ss.conn.localUser @@ -231,22 +234,24 @@ func (ss *sshSession) newIncubatorCommand(logf logger.Logf) (cmd *exec.Cmd, err if allowSendEnv { env, err := filterEnv(ss.conn.acceptEnv, ss.Session.Environ()) if err != nil { - return nil, err + return nil, nil, err } if len(env) > 0 { - encoded, err := json.Marshal(env) - if err != nil { - return nil, fmt.Errorf("failed to encode environment: %w", err) - } - incubatorArgs = append(incubatorArgs, fmt.Sprintf("--encoded-env=%q", encoded)) + // The accepted environment may contain secrets, in both the + // values and the key names, so neither is placed on the + // incubator's command line; doing so would leak them via + // /proc//cmdline to any local user. The values travel via the + // child's environment (cmd.Env, see incubatorEnv), and the key + // names travel via the allowedEnvKeysEnv environment variable. + forwardedEnv = env } } cmd = exec.CommandContext(ss.ctx, ss.conn.srv.tailscaledPath, incubatorArgs...) // The incubator will chdir into the home directory after it drops privileges. cmd.Dir = "/" - return cmd, nil + return cmd, forwardedEnv, nil } var debugIncubator bool @@ -284,7 +289,6 @@ type incubatorArgs struct { forceV1Behavior bool debugTest bool isSELinuxEnforcing bool - encodedEnv string } func parseIncubatorArgs(args []string) (incubatorArgs, error) { @@ -308,7 +312,6 @@ func parseIncubatorArgs(args []string) (incubatorArgs, error) { flags.BoolVar(&ia.forceV1Behavior, "force-v1-behavior", false, "allow falling back to the su command if login is unavailable") flags.BoolVar(&ia.debugTest, "debug-test", false, "should debug in test mode") flags.BoolVar(&ia.isSELinuxEnforcing, "is-selinux-enforcing", false, "whether SELinux is in enforcing mode") - flags.StringVar(&ia.encodedEnv, "encoded-env", "", "JSON encoded array of environment variables in '['key=value']' format") flags.Parse(args) for g := range strings.SplitSeq(groups, ",") { @@ -322,44 +325,29 @@ func parseIncubatorArgs(args []string) (incubatorArgs, error) { return ia, nil } -// forwardedEnviron returns the concatenation of the current environment with -// any environment variables specified in ia.encodedEnv. +// forwardedEnviron returns the current environment, which already includes any +// client-forwarded environment variables injected by the parent, with the +// allowedEnvKeysEnv bookkeeping variable removed. // -// It also returns allowedExtraKeys, containing the env keys that were passed in -// to ia.encodedEnv. +// It also returns allowedExtraKeys, containing the env keys that the parent +// forwarded (named in the allowedEnvKeysEnv environment variable), plus +// SSH_AUTH_SOCK. func (ia incubatorArgs) forwardedEnviron() (env, allowedExtraKeys []string, err error) { - environ := os.Environ() - // pass through SSH_AUTH_SOCK environment variable to support ssh agent forwarding // TODO(bradfitz,percy): why is this listed specially? If the parent wanted to included - // it, couldn't it have just passed it to the incubator in encodedEnv? + // it, couldn't it have just forwarded it to the incubator like the other accepted env vars? // If it didn't, no reason for us to pass it to "su -w ..." if it's not in our env // anyway? (Surely we don't want to inherit the tailscaled parent SSH_AUTH_SOCK, if any) allowedExtraKeys = []string{"SSH_AUTH_SOCK"} - if ia.encodedEnv != "" { - unquoted, err := strconv.Unquote(ia.encodedEnv) - if err != nil { - return nil, nil, fmt.Errorf("unable to parse encodedEnv %q: %w", ia.encodedEnv, err) - } + // The parent forwards the accepted env var names via allowedEnvKeysEnv + // rather than on the argv, to keep them out of /proc//cmdline. Strip + // that bookkeeping variable from the environment we hand to the user's + // process, and use its value to build the allowlist. + env, keys := stripAllowedEnvKeys(os.Environ()) + allowedExtraKeys = append(allowedExtraKeys, keys...) - var extraEnviron []string - - err = json.Unmarshal([]byte(unquoted), &extraEnviron) - if err != nil { - return nil, nil, fmt.Errorf("unable to parse encodedEnv %q: %w", ia.encodedEnv, err) - } - - environ = append(environ, extraEnviron...) - - for _, kv := range extraEnviron { - if k, _, ok := strings.Cut(kv, "="); ok { - allowedExtraKeys = append(allowedExtraKeys, k) - } - } - } - - return environ, allowedExtraKeys, nil + return env, allowedExtraKeys, nil } // beIncubator is the entrypoint to the `tailscaled be-child ssh` subcommand. @@ -832,6 +820,43 @@ func doDropPrivileges(dlogf logger.Logf, wantUid, wantGid int, supplementaryGrou return nil } +// incubatorEnv builds the environment (cmd.Env) for the incubator child. +// +// Ordering is security-relevant: server-derived variables are set first, then +// the client's TERM/LANG/LC_* (via acceptEnvPair), the connection metadata, the +// optional agent socket, and finally the acceptEnv-forwarded variables. Because +// the forwarded variables are appended, a client-forwarded value for a key that +// the server also sets (e.g. via a permissive acceptEnv policy) appears as a +// later duplicate. Callers that add security-critical keys must ensure they +// cannot be overridden by a later duplicate for the target execution path. +// +// forwardedEnv holds the acceptEnv-accepted "KEY=VALUE" pairs, whose values may +// be secret; they are carried here (never on the argv) so they are not logged +// or exposed via /proc//cmdline. Their key names are carried alongside +// them via the allowedEnvKeysEnv variable so the incubator can rebuild the +// "su -w" allowlist without them ever touching the argv. See forwardedEnvAndKeys. +func (ss *sshSession) incubatorEnv(forwardedEnv []string) []string { + env := envForUser(ss.conn.localUser) + for _, kv := range ss.Environ() { + if acceptEnvPair(kv) { + env = append(env, kv) + } + } + + ci := ss.conn.info + env = append(env, + fmt.Sprintf("SSH_CLIENT=%s %d %d", ci.src.Addr(), ci.src.Port(), ci.dst.Port()), + fmt.Sprintf("SSH_CONNECTION=%s %d %s %d", ci.src.Addr(), ci.src.Port(), ci.dst.Addr(), ci.dst.Port()), + ) + + if ss.agentListener != nil { + env = append(env, fmt.Sprintf("SSH_AUTH_SOCK=%s", ss.agentListener.Addr())) + } + + env = append(env, forwardedEnvAndKeys(forwardedEnv)...) + return env +} + // launchProcess launches an incubator process for the provided session. // It is responsible for configuring the process execution environment. // The caller can wait for the process to exit by calling cmd.Wait(). @@ -839,28 +864,14 @@ func doDropPrivileges(dlogf logger.Logf, wantUid, wantGid int, supplementaryGrou // It sets ss.cmd, stdin, stdout, and stderr. func (ss *sshSession) launchProcess() error { var err error - ss.cmd, err = ss.newIncubatorCommand(ss.logf) + var forwardedEnv []string + ss.cmd, forwardedEnv, err = ss.newIncubatorCommand(ss.logf) if err != nil { return err } cmd := ss.cmd - cmd.Env = envForUser(ss.conn.localUser) - for _, kv := range ss.Environ() { - if acceptEnvPair(kv) { - cmd.Env = append(cmd.Env, kv) - } - } - - ci := ss.conn.info - cmd.Env = append(cmd.Env, - fmt.Sprintf("SSH_CLIENT=%s %d %d", ci.src.Addr(), ci.src.Port(), ci.dst.Port()), - fmt.Sprintf("SSH_CONNECTION=%s %d %s %d", ci.src.Addr(), ci.src.Port(), ci.dst.Addr(), ci.dst.Port()), - ) - - if ss.agentListener != nil { - cmd.Env = append(cmd.Env, fmt.Sprintf("SSH_AUTH_SOCK=%s", ss.agentListener.Addr())) - } + cmd.Env = ss.incubatorEnv(forwardedEnv) ptyReq, winCh, isPty := ss.Pty() if !isPty { diff --git a/ssh/tailssh/incubator_plan9.go b/ssh/tailssh/incubator_plan9.go index 8d0031413..a198c5b60 100644 --- a/ssh/tailssh/incubator_plan9.go +++ b/ssh/tailssh/incubator_plan9.go @@ -10,7 +10,6 @@ package tailssh import ( - "encoding/json" "errors" "flag" "fmt" @@ -19,7 +18,6 @@ "os" "os/exec" "runtime" - "strconv" "strings" "sync/atomic" @@ -42,10 +40,16 @@ func init() { // If ss.srv.tailscaledPath is empty, this method is equivalent to // exec.CommandContext. // +// It also returns forwardedEnv, the client-forwarded environment variables +// accepted by the acceptEnv policy. These may contain secrets, so the caller +// must pass them to the child via its environment (cmd.Env) rather than on the +// command line. See incubator.go's newIncubatorCommand for the +// full rationale. +// // The returned Cmd.Env is guaranteed to be nil; the caller populates it. -func (ss *sshSession) newIncubatorCommand(logf logger.Logf) (cmd *exec.Cmd, err error) { +func (ss *sshSession) newIncubatorCommand(logf logger.Logf) (cmd *exec.Cmd, forwardedEnv []string, err error) { defer func() { - if cmd.Env != nil { + if cmd != nil && cmd.Env != nil { panic("internal error") } }() @@ -64,12 +68,12 @@ func (ss *sshSession) newIncubatorCommand(logf logger.Logf) (cmd *exec.Cmd, err if isSFTP { // SFTP relies on the embedded Go-based SFTP server in tailscaled, // so without tailscaled, we can't serve SFTP. - return nil, errors.New("no tailscaled found on path, can't serve SFTP") + return nil, nil, errors.New("no tailscaled found on path, can't serve SFTP") } loginShell := ss.conn.localUser.LoginShell() logf("directly running /bin/rc -c %q", ss.RawCommand()) - return exec.CommandContext(ss.ctx, loginShell, "-c", ss.RawCommand()), nil + return exec.CommandContext(ss.ctx, loginShell, "-c", ss.RawCommand()), nil, nil } lu := ss.conn.localUser @@ -121,19 +125,20 @@ func (ss *sshSession) newIncubatorCommand(logf logger.Logf) (cmd *exec.Cmd, err if allowSendEnv { env, err := filterEnv(ss.conn.acceptEnv, ss.Session.Environ()) if err != nil { - return nil, err + return nil, nil, err } if len(env) > 0 { - encoded, err := json.Marshal(env) - if err != nil { - return nil, fmt.Errorf("failed to encode environment: %w", err) - } - incubatorArgs = append(incubatorArgs, fmt.Sprintf("--encoded-env=%q", encoded)) + // The accepted environment may contain secrets, in both the + // values and the key names, so neither is placed on the argv; + // doing so would leak them via /proc//cmdline. The values + // travel via the child's environment (cmd.Env, see launchProcess), + // and the key names via the allowedEnvKeysEnv environment variable. + forwardedEnv = env } } - return exec.CommandContext(ss.ctx, ss.conn.srv.tailscaledPath, incubatorArgs...), nil + return exec.CommandContext(ss.ctx, ss.conn.srv.tailscaledPath, incubatorArgs...), forwardedEnv, nil } var debugTest atomic.Bool @@ -166,7 +171,6 @@ type incubatorArgs struct { forceV1Behavior bool debugTest bool isSELinuxEnforcing bool - encodedEnv string } func parseIncubatorArgs(args []string) (incubatorArgs, error) { @@ -185,37 +189,22 @@ func parseIncubatorArgs(args []string) (incubatorArgs, error) { flags.BoolVar(&ia.forceV1Behavior, "force-v1-behavior", false, "allow falling back to the su command if login is unavailable") flags.BoolVar(&ia.debugTest, "debug-test", false, "should debug in test mode") flags.BoolVar(&ia.isSELinuxEnforcing, "is-selinux-enforcing", false, "whether SELinux is in enforcing mode") - flags.StringVar(&ia.encodedEnv, "encoded-env", "", "JSON encoded array of environment variables in '['key=value']' format") flags.Parse(args) return ia, nil } func (ia incubatorArgs) forwardedEnviron() ([]string, string, error) { - environ := os.Environ() // pass through SSH_AUTH_SOCK environment variable to support ssh agent forwarding - allowListKeys := "SSH_AUTH_SOCK" + allowListKeys := []string{"SSH_AUTH_SOCK"} - if ia.encodedEnv != "" { - unquoted, err := strconv.Unquote(ia.encodedEnv) - if err != nil { - return nil, "", fmt.Errorf("unable to parse encodedEnv %q: %w", ia.encodedEnv, err) - } + // The parent forwards the accepted env var names via allowedEnvKeysEnv + // rather than on the argv, to keep them out of /proc//cmdline. Strip + // that bookkeeping variable from the environment we hand to the user's + // process, and use its value to extend the allowlist. + environ, keys := stripAllowedEnvKeys(os.Environ()) + allowListKeys = append(allowListKeys, keys...) - var extraEnviron []string - - err = json.Unmarshal([]byte(unquoted), &extraEnviron) - if err != nil { - return nil, "", fmt.Errorf("unable to parse encodedEnv %q: %w", ia.encodedEnv, err) - } - - environ = append(environ, extraEnviron...) - - for _, v := range extraEnviron { - allowListKeys = fmt.Sprintf("%s,%s", allowListKeys, strings.Split(v, "=")[0]) - } - } - - return environ, allowListKeys, nil + return environ, strings.Join(allowListKeys, ","), nil } func beNetshell(args []string) error { @@ -346,7 +335,8 @@ func newCommand(cmdPath string, cmdEnviron []string, cmdArgs []string) *exec.Cmd // It sets ss.cmd, stdin, stdout, and stderr. func (ss *sshSession) launchProcess() error { var err error - ss.cmd, err = ss.newIncubatorCommand(ss.logf) + var forwardedEnv []string + ss.cmd, forwardedEnv, err = ss.newIncubatorCommand(ss.logf) if err != nil { return err } @@ -370,6 +360,11 @@ func (ss *sshSession) launchProcess() error { cmd.Env = append(cmd.Env, fmt.Sprintf("SSH_AUTH_SOCK=%s", ss.agentListener.Addr())) } + // Client-forwarded environment variables travel via the environment + // (not the argv) to keep their values and names out of process listings + // and logs. + cmd.Env = append(cmd.Env, forwardedEnvAndKeys(forwardedEnv)...) + return ss.startWithStdPipes() } @@ -416,6 +411,10 @@ func acceptEnvPair(kv string) bool { if !ok { return false } - _ = k - return true // permit anything on plan9 during bringup, for debugging at least + // Never forward names reserved for our own parent->child bookkeeping, + // even during bringup, so a client cannot spoof the incubator's env. + if reservedEnvKey(k) { + return false + } + return true // permit anything else on plan9 during bringup, for debugging at least } diff --git a/ssh/tailssh/incubator_test.go b/ssh/tailssh/incubator_test.go new file mode 100644 index 000000000..5834fc880 --- /dev/null +++ b/ssh/tailssh/incubator_test.go @@ -0,0 +1,259 @@ +// Copyright (c) Tailscale Inc & contributors +// SPDX-License-Identifier: BSD-3-Clause + +//go:build (linux && !android) || (darwin && !ios) || freebsd || openbsd + +package tailssh + +import ( + "context" + "net/netip" + "os/user" + "slices" + "strings" + "testing" + + gliderssh "github.com/tailscale/gliderssh" + "tailscale.com/tailcfg" + "tailscale.com/types/logger" +) + +// fakeSession is a minimal gliderssh.Session for exercising the incubator +// command/env construction. Only the methods used by newIncubatorCommand and +// incubatorEnv are implemented; the embedded interface is nil, so any other +// method call panics and catches unexpected use. +type fakeSession struct { + gliderssh.Session + rawCommand string + subsystem string + environ []string +} + +func (s fakeSession) RawCommand() string { return s.rawCommand } +func (s fakeSession) Subsystem() string { return s.subsystem } +func (s fakeSession) Environ() []string { return s.environ } +func (s fakeSession) Pty() (gliderssh.Pty, <-chan gliderssh.Window, bool) { + return gliderssh.Pty{}, nil, false +} + +// newTestSession builds an sshSession wired to the localState fake, with the +// NodeAttrSSHEnvironmentVariables capability enabled and the given acceptEnv +// policy, so newIncubatorCommand exercises the env-forwarding path. +func newTestSession(t *testing.T, acceptEnv []string, clientEnviron []string) *sshSession { + t.Helper() + srv := &server{ + logf: logger.Discard, + lb: &localState{ + sshEnabled: true, + caps: []tailcfg.NodeCapability{tailcfg.NodeAttrSSHEnvironmentVariables}, + }, + // tailscaledPath must be non-empty to take the incubator (be-child) + // path rather than the direct-exec fallback. + tailscaledPath: "/usr/sbin/tailscaled", + } + c := &conn{ + srv: srv, + acceptEnv: acceptEnv, + info: &sshConnInfo{ + sshUser: "alice", + src: netip.MustParseAddrPort("100.100.100.101:2222"), + dst: netip.MustParseAddrPort("100.100.100.102:22"), + node: (&tailcfg.Node{}).View(), + }, + localUser: &userMeta{User: user.User{Username: "alice", Uid: "1000", Gid: "1000", HomeDir: "/home/alice"}}, + userGroupIDs: []string{"1000"}, + } + ctx, cancel := context.WithCancelCause(context.Background()) + t.Cleanup(func() { cancel(nil) }) + return &sshSession{ + Session: fakeSession{environ: clientEnviron}, + conn: c, + ctx: ctx, + } +} + +// TestForwardedEnvSecretNotOnArgv is the core security regression test for the +// acceptEnv secret leak: a forwarded value must be delivered via the child's +// environment (cmd.Env) and must appear nowhere on the command line (cmd.Args), +// which is logged at session start and visible in /proc//cmdline. Neither +// the value nor the (potentially sensitive) key name may appear on the argv; +// the key names travel via the allowedEnvKeysEnv environment variable instead. +func TestForwardedEnvSecretNotOnArgv(t *testing.T) { + const secret = "s3cr3t-token-value" + ss := newTestSession(t, + []string{"GITLAB_API_TOKEN"}, + []string{"GITLAB_API_TOKEN=" + secret, "IGNORED=nope"}, + ) + + cmd, forwardedEnv, err := ss.newIncubatorCommand(logger.Discard) + if err != nil { + t.Fatalf("newIncubatorCommand: %v", err) + } + + // The value must be returned for delivery via the environment... + if !slices.Contains(forwardedEnv, "GITLAB_API_TOKEN="+secret) { + t.Errorf("forwardedEnv = %q, want it to contain the forwarded secret", forwardedEnv) + } + // ...and it must end up in the child's actual environment, together with + // the allowedEnvKeysEnv entry naming the forwarded key. + env := ss.incubatorEnv(forwardedEnv) + if !slices.Contains(env, "GITLAB_API_TOKEN="+secret) { + t.Errorf("incubatorEnv did not contain the forwarded secret; got %q", env) + } + if !slices.Contains(env, allowedEnvKeysEnv+"=GITLAB_API_TOKEN") { + t.Errorf("incubatorEnv did not carry the key name via %s; got %q", allowedEnvKeysEnv, env) + } + + // Neither the secret value nor the key name may appear anywhere on the argv. + argv := strings.Join(cmd.Args, "\x00") + if strings.Contains(argv, secret) { + t.Errorf("secret value leaked onto cmd.Args: %q", cmd.Args) + } + if strings.Contains(argv, "GITLAB_API_TOKEN") { + t.Errorf("forwarded key name leaked onto cmd.Args: %q", cmd.Args) + } +} + +// TestForwardedEnvKeysSpoofRejected checks the trust boundary end-to-end: a +// client that tries to forward the parent->child bookkeeping variable +// (allowedEnvKeysEnv), or a key containing the "," allowlist separator, under a +// wildcard acceptEnv policy must not be able to inject entries into the +// incubator's env. filterEnv drops both, so newIncubatorCommand never forwards +// them and the only allowedEnvKeysEnv entry in the child env is the one the +// parent set for the legitimately-accepted keys. +func TestForwardedEnvKeysSpoofRejected(t *testing.T) { + ss := newTestSession(t, + []string{"*"}, // permissive policy: the worst case for spoofing + []string{ + allowedEnvKeysEnv + "=EVIL1,EVIL2", // name collision attempt + "A,B=x", // comma-in-key injection attempt + "LEGIT=ok", // a genuinely accepted var + }, + ) + + _, forwardedEnv, err := ss.newIncubatorCommand(logger.Discard) + if err != nil { + t.Fatalf("newIncubatorCommand: %v", err) + } + + // Only the legitimate variable is forwarded; neither spoof survives filtering. + if !slices.Contains(forwardedEnv, "LEGIT=ok") { + t.Errorf("forwardedEnv = %q, want it to contain LEGIT=ok", forwardedEnv) + } + for _, kv := range forwardedEnv { + if k, _, _ := strings.Cut(kv, "="); reservedEnvKey(k) { + t.Errorf("reserved/injecting key %q was forwarded: %q", k, forwardedEnv) + } + } + + // In the assembled child env there is exactly one allowedEnvKeysEnv entry + // (the parent's), and it names only the legitimate key. + env := ss.incubatorEnv(forwardedEnv) + var bookkeeping []string + for _, kv := range env { + if k, _, _ := strings.Cut(kv, "="); k == allowedEnvKeysEnv { + bookkeeping = append(bookkeeping, kv) + } + } + if want := []string{allowedEnvKeysEnv + "=LEGIT"}; !slices.Equal(bookkeeping, want) { + t.Errorf("bookkeeping entries = %q, want %q", bookkeeping, want) + } + + // And the child-side reconstruction (stripAllowedEnvKeys, shared by both + // platforms' forwardedEnviron) yields neither spoofed name in the su -w + // allowlist, and strips the bookkeeping var back out entirely. + strippedEnv, allowedKeys := stripAllowedEnvKeys(slices.Clone(env)) + for _, kv := range strippedEnv { + if k, _, _ := strings.Cut(kv, "="); k == allowedEnvKeysEnv { + t.Errorf("%s survived stripping: %q", allowedEnvKeysEnv, strippedEnv) + } + } + for _, bad := range []string{"EVIL1", "EVIL2", "A", "B"} { + if slices.Contains(allowedKeys, bad) { + t.Errorf("injected key %q reached su -w allowlist %q", bad, allowedKeys) + } + } + if !slices.Contains(allowedKeys, "LEGIT") { + t.Errorf("legitimate key LEGIT missing from allowlist %q", allowedKeys) + } +} + +// TestIncubatorEnvOrdering pins the current environment precedence: server-set +// variables come first, then client TERM/LANG/LC_*, then connection metadata, +// and finally the acceptEnv-forwarded pairs (appended last). This documents the +// ordering so any future change that alters which value wins on a duplicate key +// is caught by review. +func TestIncubatorEnvOrdering(t *testing.T) { + ss := newTestSession(t, + []string{"FORWARDED"}, + []string{"TERM=xterm", "FORWARDED=fromclient"}, + ) + _, forwardedEnv, err := ss.newIncubatorCommand(logger.Discard) + if err != nil { + t.Fatalf("newIncubatorCommand: %v", err) + } + env := ss.incubatorEnv(forwardedEnv) + + idx := func(prefix string) int { + return slices.IndexFunc(env, func(kv string) bool { + return strings.HasPrefix(kv, prefix) + }) + } + + // Server-set vars (from envForUser) come before client-provided ones. + if got := idx("PATH="); got == -1 { + t.Fatalf("expected server-set PATH in env; got %q", env) + } + pathIdx, termIdx, fwdIdx := idx("PATH="), idx("TERM="), idx("FORWARDED=") + if !(pathIdx < termIdx && termIdx < fwdIdx) { + t.Errorf("unexpected env ordering: PATH=%d TERM=%d FORWARDED=%d in %q", pathIdx, termIdx, fwdIdx, env) + } + // The forwarded value is present (appended last). + if !slices.Contains(env, "FORWARDED=fromclient") { + t.Errorf("forwarded value missing from env: %q", env) + } +} + +// TestForwardedEnvironAllowedKeys verifies that the incubator reconstructs the +// su -w allowlist from the (non-secret) key names carried in the allowedEnvKeysEnv +// environment variable, always including SSH_AUTH_SOCK, strips that bookkeeping +// variable from the environment handed to the user's process, and never depends +// on anything being present on the command line. +func TestForwardedEnvironAllowedKeys(t *testing.T) { + tests := []struct { + name string + allowedKeys string // value of allowedEnvKeysEnv, or "" to leave it unset + wantKeys []string + }{ + { + name: "unset", + allowedKeys: "", + wantKeys: []string{"SSH_AUTH_SOCK"}, + }, + { + name: "some_keys", + allowedKeys: "GIT_ENV_VAR,EXACT_MATCH", + wantKeys: []string{"SSH_AUTH_SOCK", "GIT_ENV_VAR", "EXACT_MATCH"}, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if tc.allowedKeys != "" { + t.Setenv(allowedEnvKeysEnv, tc.allowedKeys) + } + gotEnv, gotKeys, err := incubatorArgs{}.forwardedEnviron() + if err != nil { + t.Fatalf("forwardedEnviron: %v", err) + } + if !slices.Equal(gotKeys, tc.wantKeys) { + t.Errorf("allowedExtraKeys = %q, want %q", gotKeys, tc.wantKeys) + } + // The bookkeeping variable must never leak into the child's env. + for _, kv := range gotEnv { + if strings.HasPrefix(kv, allowedEnvKeysEnv+"=") { + t.Errorf("%s leaked into child environment: %q", allowedEnvKeysEnv, kv) + } + } + }) + } +} diff --git a/ssh/tailssh/tailssh.go b/ssh/tailssh/tailssh.go index 9ce3d1991..c0c6e987a 100644 --- a/ssh/tailssh/tailssh.go +++ b/ssh/tailssh/tailssh.go @@ -777,6 +777,28 @@ type sshSession struct { exitHandled chan struct{} } +// forwardedEnvAndKeys returns the environment entries the parent should append +// to the incubator child's cmd.Env for the given client-forwarded "KEY=VALUE" +// pairs: the pairs themselves, plus an allowedEnvKeysEnv entry naming their +// keys. Both the values and the key names travel via the environment (never the +// argv) so they stay out of /proc//cmdline and process logs. Returns nil +// when there are no forwarded variables, so no bookkeeping entry is added. +func forwardedEnvAndKeys(forwardedEnv []string) []string { + if len(forwardedEnv) == 0 { + return nil + } + keys := make([]string, 0, len(forwardedEnv)) + for _, kv := range forwardedEnv { + if k, _, ok := strings.Cut(kv, "="); ok { + keys = append(keys, k) + } + } + out := make([]string, 0, len(forwardedEnv)+1) + out = append(out, forwardedEnv...) + out = append(out, allowedEnvKeysEnv+"="+strings.Join(keys, ",")) + return out +} + func (ss *sshSession) vlogf(format string, args ...any) { if sshVerboseLogging() { ss.logf(format, args...) diff --git a/ssh/tailssh/tailssh_test.go b/ssh/tailssh/tailssh_test.go index ec14405e7..1a992a41a 100644 --- a/ssh/tailssh/tailssh_test.go +++ b/ssh/tailssh/tailssh_test.go @@ -52,6 +52,7 @@ "tailscale.com/util/cibuild" "tailscale.com/util/lineiter" "tailscale.com/util/must" + "tailscale.com/util/set" "tailscale.com/version/distro" "tailscale.com/wgengine" ) @@ -381,6 +382,10 @@ type localState struct { matchingRule *tailcfg.SSHRule varRoot string // if empty, TailscaleVarRoot returns "" + // caps, if non-empty, are advertised via NetMap().AllCaps. Used to gate + // features like NodeAttrSSHEnvironmentVariables in tests. + caps []tailcfg.NodeCapability + // serverActions is a map of the action name to the action. // It is served for paths like https://unused/ssh-action/. // The action name is the last part of the action URL. @@ -419,6 +424,7 @@ func (ts *localState) NetMap() *netmap.NetworkMap { ID: 1, }).View(), SSHPolicy: policy, + AllCaps: set.SetOf(ts.caps), } } @@ -1418,3 +1424,51 @@ func mockRecordingServer(t *testing.T, handleRecord http.HandlerFunc) *httptest. t.Cleanup(srv.Close) return srv } + +// TestForwardedEnvKeysRoundTrip verifies that the client-forwarded env var +// names travel from parent to incubator child via the allowedEnvKeysEnv +// environment variable (never the argv), and that forwardedEnviron strips that +// bookkeeping variable back out while reconstructing the allowlist. This is the +// unit-level counterpart to the end-to-end coverage in TestIntegrationSSH. +func TestForwardedEnvKeysRoundTrip(t *testing.T) { + forwarded := []string{"GIT_ENV_VAR=working1", "EXACT_MATCH=working2", "TESTING=working3"} + + // Parent side: forwardedEnvAndKeys returns the values plus a single + // allowedEnvKeysEnv entry naming their keys, and nothing else. + got := forwardedEnvAndKeys(forwarded) + want := append(slices.Clone(forwarded), allowedEnvKeysEnv+"=GIT_ENV_VAR,EXACT_MATCH,TESTING") + if !slices.Equal(got, want) { + t.Fatalf("forwardedEnvAndKeys:\n got %q\nwant %q", got, want) + } + + // With no forwarded vars, no bookkeeping entry is added. + if got := forwardedEnvAndKeys(nil); got != nil { + t.Fatalf("forwardedEnvAndKeys(nil) = %q, want nil", got) + } + + // Child side: forwardedEnviron reads the keys from allowedEnvKeysEnv, + // removes that variable from the returned environment, and includes the + // keys (plus SSH_AUTH_SOCK) in the allowlist. + t.Setenv(allowedEnvKeysEnv, "GIT_ENV_VAR,EXACT_MATCH,TESTING") + t.Setenv("GIT_ENV_VAR", "working1") + env, allowedKeys, err := incubatorArgs{}.forwardedEnviron() + if err != nil { + t.Fatal(err) + } + for _, kv := range env { + if strings.HasPrefix(kv, allowedEnvKeysEnv+"=") { + t.Errorf("%s leaked into child environment: %q", allowedEnvKeysEnv, kv) + } + } + if !slices.Contains(env, "GIT_ENV_VAR=working1") { + t.Errorf("forwarded value GIT_ENV_VAR missing from child environment") + } + for _, k := range []string{"SSH_AUTH_SOCK", "GIT_ENV_VAR", "EXACT_MATCH", "TESTING"} { + if !slices.Contains(allowedKeys, k) { + t.Errorf("allowlist %q missing key %q", allowedKeys, k) + } + } + if slices.Contains(allowedKeys, allowedEnvKeysEnv) { + t.Errorf("allowlist %q must not contain the bookkeeping var name", allowedKeys) + } +}