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/<pid>/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 <patrick@tailscale.com>

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/<pid>/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) {

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)
+			}
+		})
+	}
+}

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/<pid>/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/<pid>/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/<pid>/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/<pid>/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 {

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/<pid>/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/<pid>/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
 }

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/<pid>/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)
+				}
+			}
+		})
+	}
+}

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/<pid>/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...)

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/<action-name>.
 	// 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)
+	}
+}