From 9d48dbd561e1d4fa386aa413759e616b3f1bd92b Mon Sep 17 00:00:00 2001 From: Patrick O'Doherty Date: Thu, 30 Jul 2026 08:57:55 -0700 Subject: [PATCH] ssh/tailssh: keep acceptEnv values and names out of the incubator cmdline (#20552) 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). This change now carries those variables through an os.Pipe file descriptor as a json encoded payload. Added end-to-end testing helps validate secrets reach the session but are not in flags or logged. Fixes tailscale/corp#44903 Change-Id: I5b137b20e9c06feec6b70aaf4e6925e6db74017e Signed-off-by: Mike Jensen Co-authored-by: Mike Jensen --- ssh/tailssh/accept_env.go | 32 ++- ssh/tailssh/accept_env_test.go | 42 ++++ ssh/tailssh/incubator.go | 219 +++++++++++------- ssh/tailssh/incubator_plan9.go | 140 ++++++++---- ssh/tailssh/incubator_test.go | 286 ++++++++++++++++++++++++ ssh/tailssh/localstate_test.go | 105 +++++++++ ssh/tailssh/tailssh.go | 29 +++ ssh/tailssh/tailssh_integration_test.go | 98 +++++++- ssh/tailssh/tailssh_test.go | 83 ------- 9 files changed, 822 insertions(+), 212 deletions(-) create mode 100644 ssh/tailssh/incubator_test.go create mode 100644 ssh/tailssh/localstate_test.go diff --git a/ssh/tailssh/accept_env.go b/ssh/tailssh/accept_env.go index 64f979c2a..8473282bc 100644 --- a/ssh/tailssh/accept_env.go +++ b/ssh/tailssh/accept_env.go @@ -13,11 +13,26 @@ // 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. +// DYLD_INSERT_LIBRARIES) or leaking of secrets (e.g. GOTRACEBACK) +// 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_") + return strings.HasPrefix(upper, "LD_") || strings.HasPrefix(upper, "DYLD_") || + upper == "GOTRACEBACK" +} + +// forbiddenEnvKey reports whether name must never be accepted from the client as a +// forwarded environment variable, independent of the acceptEnv policy. Names are +// restricted to a known-safe charset so they cannot corrupt the "su -w" allowlist +// built from them, truncate on exec, or confuse downstream consumers of the user's +// environment. +func forbiddenEnvKey(name string) bool { + for _, r := range name { + if r != '_' && r != '-' && (r < 'a' || r > 'z') && (r < 'A' || r > 'Z') && (r < '0' || r > '9') { + return true + } + } + return name == "" } // filterEnv filters a passed in environ string slice (a slice with strings @@ -47,12 +62,23 @@ func filterEnv(acceptEnv []string, environ []string) ([]string, error) { return nil, fmt.Errorf(`invalid environment variable: %q. Variables must be in "KEY=VALUE" format`, envPair) } + // Reject NUL bytes: envp entries are NUL-terminated, so a NUL would silently truncate on exec + if strings.Contains(envPair, "\x00") { + continue + } + // Always reject dangerous environment variables that could // enable privilege escalation, regardless of acceptEnv policy. if isDangerousEnvVar(variableName) { continue } + // Always reject names that would corrupt the incubator's "su -w" + // allowlist (see reservedEnvKey), regardless of acceptEnv policy. + if forbiddenEnvKey(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..d92193466 100644 --- a/ssh/tailssh/accept_env_test.go +++ b/ssh/tailssh/accept_env_test.go @@ -204,6 +204,48 @@ func TestFilterEnv(t *testing.T) { environ: []string{"DYLD_INSERT_LIBRARIES=/tmp/evil.dylib", "DYLD_LIBRARY_PATH=/tmp", "TERM=xterm"}, expectedFiltered: []string{"TERM=xterm"}, }, + { + // 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"}, + }, + { + name: "empty-key-rejected", + acceptEnv: []string{"*"}, + environ: []string{"=x", "GOOD=1"}, + expectedFiltered: []string{"GOOD=1"}, + }, + { + // GOTRACEBACK controls crash tracebacks/core dumps of the + // privileged incubator child, which could leak secrets + name: "gotraceback-rejected", + acceptEnv: []string{"*"}, + environ: []string{"GOTRACEBACK=crash", "TERM=xterm"}, + expectedFiltered: []string{"TERM=xterm"}, + }, + { + name: "nul-in-name-rejected", + acceptEnv: []string{"*"}, + environ: []string{"A\x00B=x", "GOOD=1"}, + expectedFiltered: []string{"GOOD=1"}, + }, + { + name: "nul-in-value-rejected", + acceptEnv: []string{"*"}, + environ: []string{"GOOD=a\x00b"}, + expectedFiltered: nil, + }, + { + // Key names are closed to a known charset: spaces, control + // chars, punctuation other than '-' and non-ASCII are rejected. + name: "key-charset-restricted", + acceptEnv: []string{"*"}, + environ: []string{"MY VAR=1", "WEIRD\tNAME=2", "MY.VAR=3", "MY-VAR=ok", "B\xc3\xa4R=5", "MY_VAR=ok2"}, + expectedFiltered: []string{"MY-VAR=ok", "MY_VAR=ok2"}, + }, } for _, tc := range testCases { diff --git a/ssh/tailssh/incubator.go b/ssh/tailssh/incubator.go index 48c65e8e5..faf70af90 100644 --- a/ssh/tailssh/incubator.go +++ b/ssh/tailssh/incubator.go @@ -114,8 +114,14 @@ 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 passes them to the child via an inherited file rather +// than the command line or environment, where they could leak, or influence +// the privileged child. +// +// The returned Cmd.Env is guaranteed to be nil; the caller must populate it. +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 +142,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 +169,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,26 +237,27 @@ 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, so it is communicated through a fd + // created by the caller, never the argv or the child's environment. + incubatorArgs = append(incubatorArgs, fmt.Sprintf("--env-fd=%d", forwardedEnvChildFD)) + 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 -var debugTest atomic.Bool +var ( + debugIncubator bool + debugTest atomic.Bool +) type stdRWC struct{} @@ -284,7 +291,15 @@ type incubatorArgs struct { forceV1Behavior bool debugTest bool isSELinuxEnforcing bool - encodedEnv string + // Deprecated: encodedEnv is deprecated and must not be used by new code. + // It is parsed only so this child keeps working when exec'd by an + // outdated parent tailscaled that still passes it. + encodedEnv string + // envFD is the file descriptor to read the forwarded environment from + // (a JSON array of KEY=VALUE pairs), or -1 if none. + envFD int + // forwardedEnv holds the pairs loaded by loadForwardedEnv. + forwardedEnv []string } func parseIncubatorArgs(args []string) (incubatorArgs, error) { @@ -308,7 +323,9 @@ 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") + // DEPRECATED: retained for version-skew compatibility only. DO NOT USE. + flags.StringVar(&ia.encodedEnv, "encoded-env", "", "deprecated; do not use") + flags.IntVar(&ia.envFD, "env-fd", -1, "file descriptor to read the forwarded environment from (JSON array of KEY=VALUE pairs)") flags.Parse(args) for g := range strings.SplitSeq(groups, ",") { @@ -319,47 +336,62 @@ func parseIncubatorArgs(args []string) (incubatorArgs, error) { ia.gids = append(ia.gids, gid) } + // envFD comes from an ExtraFiles entry, so it must never name stdin/out/err + if ia.envFD >= 0 && ia.envFD < 3 { + return ia, fmt.Errorf("invalid --env-fd %d: must be >= 3", ia.envFD) + } + return ia, nil } -// forwardedEnviron returns the concatenation of the current environment with -// any environment variables specified in ia.encodedEnv. -// -// It also returns allowedExtraKeys, containing the env keys that were passed in -// to ia.encodedEnv. -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? - // 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) +// loadForwardedEnv reads the client-forwarded environment pairs into ia.forwardedEnv, from the +// inherited file named by --env-fd. The pairs only enter the su/login/shell environment, +// never this process's own environment. +func (ia *incubatorArgs) loadForwardedEnv() error { + var pairs []string + switch { + case ia.envFD >= 0: + if ia.envFD < 3 { + return fmt.Errorf("invalid --env-fd=%d: must be >= 3", ia.envFD) } - - 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) + f := os.NewFile(uintptr(ia.envFD), "forwarded-env") + defer f.Close() + if err := json.NewDecoder(f).Decode(&pairs); err != nil { + return fmt.Errorf("unable to read forwarded environment: %w", err) } - - environ = append(environ, extraEnviron...) - - for _, kv := range extraEnviron { - if k, _, ok := strings.Cut(kv, "="); ok { - allowedExtraKeys = append(allowedExtraKeys, k) - } + case ia.encodedEnv != "": // Legacy path to support an outdated parent tailscaled + if unquoted, err := strconv.Unquote(ia.encodedEnv); err != nil { + return fmt.Errorf("unable to parse encodedEnv %q: %w", ia.encodedEnv, err) + } else if err := json.Unmarshal([]byte(unquoted), &pairs); err != nil { + return fmt.Errorf("unable to parse encodedEnv %q: %w", ia.encodedEnv, err) } } + // Enforce "su -w" integrity child-side: old parents may not have filtered these + pairs = slices.DeleteFunc(pairs, func(kv string) bool { + k, _, ok := strings.Cut(kv, "=") + return !ok || forbiddenEnvKey(k) || strings.Contains(kv, "\x00") + }) + ia.forwardedEnv = pairs + return nil +} - return environ, allowedExtraKeys, nil +// forwardedEnviron returns the environment to hand to the user's process. This includes the +// current environment plus the client-forwarded pairs. It also returns allowedExtraKeys for +// the "su -w" allowlist, the forwarded key names, plus SSH_AUTH_SOCK. +func (ia *incubatorArgs) forwardedEnviron() (env, allowedExtraKeys []string) { + // SSH_AUTH_SOCK is allowlisted here rather than forwarded because old + // parents set it without forwarding any keys. It can only be present if + // the parent enabled agent forwarding: the child's environment is built + // by incubatorEnv, never from the parent's os.Environ. + allowedExtraKeys = []string{"SSH_AUTH_SOCK"} + + env = append(os.Environ(), ia.forwardedEnv...) + for _, kv := range ia.forwardedEnv { + if k, _, ok := strings.Cut(kv, "="); ok { + allowedExtraKeys = append(allowedExtraKeys, k) + } + } + return env, allowedExtraKeys } // beIncubator is the entrypoint to the `tailscaled be-child ssh` subcommand. @@ -383,6 +415,13 @@ func beIncubator(args []string) error { if err != nil { return err } + if ia.encodedEnv != "" { + log.Printf("WARNING: tailscaled be-child: accepted SSH environment variables were passed via the deprecated --encoded-env flag; " + + "the running tailscaled is outdated. Update tailscaled to the latest version and restart for the latest security fixes.") + } + if err := ia.loadForwardedEnv(); err != nil { + return err + } if ia.isSFTP && ia.isShell { return fmt.Errorf("--sftp and --shell are mutually exclusive") } @@ -395,7 +434,7 @@ func beIncubator(args []string) error { } } else if ia.debugTest { // In testing, we don't always have syslog, so log to a temp file. - if logFile, err := os.OpenFile("/tmp/tailscalessh.log", os.O_APPEND|os.O_WRONLY, 0666); err == nil { + if logFile, err := os.OpenFile("/tmp/tailscalessh.log", os.O_APPEND|os.O_WRONLY, 0o666); err == nil { lf := log.New(logFile, "", 0) dlogf = func(msg string, args ...any) { lf.Printf(msg, args...) @@ -539,10 +578,7 @@ func tryExecLogin(dlogf logger.Logf, ia incubatorArgs) error { loginArgs := ia.loginArgs(loginCmdPath) dlogf("logging in with %+v", loginArgs) - environ, _, err := ia.forwardedEnviron() - if err != nil { - return err - } + environ, _ := ia.forwardedEnviron() // If Exec works, the Go code will not proceed past this: err = unix.Exec(loginCmdPath, loginArgs, environ) @@ -578,10 +614,7 @@ func trySU(dlogf logger.Logf, ia incubatorArgs) (handled bool, err error) { defer sessionCloser() } - environ, allowListEnvKeys, err := ia.forwardedEnviron() - if err != nil { - return false, err - } + environ, allowListEnvKeys := ia.forwardedEnviron() loginArgs := []string{ su, @@ -626,10 +659,7 @@ func findSU(dlogf logger.Logf, ia incubatorArgs) string { return "" } - _, allowListEnvKeys, err := ia.forwardedEnviron() - if err != nil { - return "" - } + _, allowListEnvKeys := ia.forwardedEnviron() // First try to execute su -w -l -c true // to make sure su supports the necessary arguments. @@ -662,15 +692,12 @@ func handleSSHInProcess(dlogf logger.Logf, ia incubatorArgs) error { return err } - environ, _, err := ia.forwardedEnviron() - if err != nil { - return err - } + environ, _ := ia.forwardedEnviron() args := shellArgs(ia.isShell, ia.cmd) dlogf("running %s %q", ia.loginShell, args) cmd := newCommand(ia.hasTTY, ia.loginShell, environ, args) - err = cmd.Run() + err := cmd.Run() if ee, ok := err.(*exec.ExitError); ok { ps := ee.ProcessState code := ps.ExitCode() @@ -832,6 +859,32 @@ func doDropPrivileges(dlogf logger.Logf, wantUid, wantGid int, supplementaryGrou return nil } +// incubatorEnv builds the environment (cmd.Env) for the incubator child: +// the user's login environment, the client's TERM/LANG/LC_* (matching +// OpenSSH's default AcceptEnv), the connection metadata, and the optional +// agent socket. acceptEnv-forwarded variables are not included; they travel +// via an inherited file instead (see forwardedEnvFile). +func (ss *sshSession) incubatorEnv() []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())) + } + + 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,27 +892,25 @@ 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) + cmd.Env = ss.incubatorEnv() + + if len(forwardedEnv) > 0 { + // The accepted environment may contain secrets, so it is passed to the child via an + // inherited file, never the argv or the environment. Closing after Start is safe: the + // child has its own copy by then. + envFile, err := forwardedEnvFile(forwardedEnv) + if err != nil { + return err } - } - - 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())) + defer envFile.Close() + cmd.ExtraFiles = []*os.File{envFile} } ptyReq, winCh, isPty := ss.Pty() @@ -1123,7 +1174,7 @@ func updateStringInSlice(ss []string, a, b string) { // AcceptEnv. func acceptEnvPair(kv string) bool { k, _, ok := strings.Cut(kv, "=") - if !ok { + if !ok || isDangerousEnvVar(k) || forbiddenEnvKey(k) { return false } return k == "TERM" || k == "LANG" || strings.HasPrefix(k, "LC_") diff --git a/ssh/tailssh/incubator_plan9.go b/ssh/tailssh/incubator_plan9.go index 8d0031413..4eb945914 100644 --- a/ssh/tailssh/incubator_plan9.go +++ b/ssh/tailssh/incubator_plan9.go @@ -19,6 +19,7 @@ "os" "os/exec" "runtime" + "slices" "strconv" "strings" "sync/atomic" @@ -42,10 +43,15 @@ func init() { // If ss.srv.tailscaledPath is empty, this method is equivalent to // exec.CommandContext. // -// 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 client-forwarded environment variables +// accepted by the acceptEnv policy. These may contain secrets, so the caller +// passes them to the child via an inherited file rather than the command line +// or environment. +// +// The returned Cmd.Env is guaranteed to be nil; the caller must populate it. +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 +70,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 +127,18 @@ 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, so it travels via an + // inherited file (created by the caller). The fd number is not sensitive. + incubatorArgs = append(incubatorArgs, fmt.Sprintf("--env-fd=%d", forwardedEnvChildFD)) + 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,15 @@ type incubatorArgs struct { forceV1Behavior bool debugTest bool isSELinuxEnforcing bool - encodedEnv string + // Deprecated: encodedEnv is deprecated and must not be used by new code. + // It is parsed only so this child keeps working when exec'd by an + // outdated parent tailscaled that still passes it. + encodedEnv string + // envFD is the file descriptor to read the forwarded environment from + // (a JSON array of KEY=VALUE pairs), or -1 if none. + envFD int + // forwardedEnv holds the pairs loaded by loadForwardedEnv. + forwardedEnv []string } func parseIncubatorArgs(args []string) (incubatorArgs, error) { @@ -185,37 +198,62 @@ 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") + // DEPRECATED: retained for version-skew compatibility only. DO NOT USE. + flags.StringVar(&ia.encodedEnv, "encoded-env", "", "deprecated; do not use") + flags.IntVar(&ia.envFD, "env-fd", -1, "file descriptor to read the forwarded environment from (JSON array of KEY=VALUE pairs)") flags.Parse(args) + // envFD comes from an ExtraFiles entry, so it must never name stdin/out/err + if ia.envFD >= 0 && ia.envFD < 3 { + return ia, fmt.Errorf("invalid --env-fd %d: must be >= 3", ia.envFD) + } 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" - - 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) +// loadForwardedEnv reads the client-forwarded environment pairs into ia.forwardedEnv, from the +// inherited file named by --env-fd. The pairs only enter the su/login/shell environment, +// never this process's own environment. +func (ia *incubatorArgs) loadForwardedEnv() error { + var pairs []string + switch { + case ia.envFD >= 0: + if ia.envFD < 3 { + return fmt.Errorf("invalid --env-fd=%d: must be >= 3", ia.envFD) } - - 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) + f := os.NewFile(uintptr(ia.envFD), "forwarded-env") + defer f.Close() + if err := json.NewDecoder(f).Decode(&pairs); err != nil { + return fmt.Errorf("unable to read forwarded environment: %w", err) } + case ia.encodedEnv != "": // Legacy path to support an outdated parent tailscaled + if unquoted, err := strconv.Unquote(ia.encodedEnv); err != nil { + return fmt.Errorf("unable to parse encodedEnv %q: %w", ia.encodedEnv, err) + } else if err := json.Unmarshal([]byte(unquoted), &pairs); err != nil { + return fmt.Errorf("unable to parse encodedEnv %q: %w", ia.encodedEnv, err) + } + } + // Enforce "su -w" integrity child-side: old parents may not have filtered these + pairs = slices.DeleteFunc(pairs, func(kv string) bool { + k, _, ok := strings.Cut(kv, "=") + return !ok || forbiddenEnvKey(k) || strings.Contains(kv, "\x00") + }) + ia.forwardedEnv = pairs + return nil +} - environ = append(environ, extraEnviron...) +// forwardedEnviron returns the environment to hand to the user's process (the +// current environment plus the client-forwarded pairs) and the comma-separated +// allowlist of forwarded key names, plus SSH_AUTH_SOCK. +func (ia *incubatorArgs) forwardedEnviron() ([]string, string) { + allowListKeys := []string{"SSH_AUTH_SOCK"} - for _, v := range extraEnviron { - allowListKeys = fmt.Sprintf("%s,%s", allowListKeys, strings.Split(v, "=")[0]) + environ := append(os.Environ(), ia.forwardedEnv...) + for _, kv := range ia.forwardedEnv { + if k, _, ok := strings.Cut(kv, "="); ok { + allowListKeys = append(allowListKeys, k) } } - return environ, allowListKeys, nil + return environ, strings.Join(allowListKeys, ",") } func beNetshell(args []string) error { @@ -244,6 +282,13 @@ func beIncubator(args []string) error { if err != nil { return err } + if ia.encodedEnv != "" { + log.Printf("WARNING: tailscaled be-child: accepted SSH environment variables were passed via the deprecated --encoded-env flag; " + + "the running tailscaled is outdated. Update tailscaled to the latest version and restart for the latest security fixes.") + } + if err := ia.loadForwardedEnv(); err != nil { + return err + } if ia.isSFTP && ia.isShell { return fmt.Errorf("--sftp and --shell are mutually exclusive") } @@ -306,14 +351,11 @@ func serveSFTP() error { // login shell. func handleSSHInProcess(dlogf logger.Logf, ia incubatorArgs) error { - environ, _, err := ia.forwardedEnviron() - if err != nil { - return err - } + environ, _ := ia.forwardedEnviron() dlogf("running /bin/rc -c %q", ia.cmd) cmd := newCommand("/bin/rc", environ, []string{"-c", ia.cmd}) - err = cmd.Run() + err := cmd.Run() if ee, ok := err.(*exec.ExitError); ok { ps := ee.ProcessState code := ps.ExitCode() @@ -346,7 +388,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 +413,17 @@ func (ss *sshSession) launchProcess() error { cmd.Env = append(cmd.Env, fmt.Sprintf("SSH_AUTH_SOCK=%s", ss.agentListener.Addr())) } + // Client-forwarded environment variables may contain secrets, so they + // are passed to the child via an inherited pipe. + if len(forwardedEnv) > 0 { + envFile, err := forwardedEnvFile(forwardedEnv) + if err != nil { + return err + } + defer envFile.Close() + cmd.ExtraFiles = []*os.File{envFile} + } + return ss.startWithStdPipes() } @@ -416,6 +470,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 or unsafe for the child, + // even during bringup, so a client cannot spoof the incubator's env. + if forbiddenEnvKey(k) || isDangerousEnvVar(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..8f29ff916 --- /dev/null +++ b/ssh/tailssh/incubator_test.go @@ -0,0 +1,286 @@ +// Copyright (c) Tailscale Inc & contributors +// SPDX-License-Identifier: BSD-3-Clause + +//go:build (linux && !android) || (darwin && !ios) || freebsd || openbsd + +package tailssh + +import ( + "context" + "encoding/json" + "fmt" + "net/netip" + "os/user" + "slices" + "strings" + "testing" + + gliderssh "github.com/tailscale/gliderssh" + "golang.org/x/sys/unix" + "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 returned for delivery via +// an inherited file, and must appear neither on the command line (cmd.Args, +// logged at session start and visible in /proc//cmdline) nor in the +// privileged child's environment (see incubatorEnv). +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 pair must be returned for delivery via the inherited file, and the + // argv must name the fd to read it from. + if !slices.Contains(forwardedEnv, "GITLAB_API_TOKEN="+secret) { + t.Errorf("forwardedEnv = %q, want it to contain the forwarded secret", forwardedEnv) + } + if !slices.Contains(cmd.Args, "--env-fd=3") { + t.Errorf("cmd.Args = %q, want --env-fd=3", cmd.Args) + } + + // 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) + } + + // The privileged child's environment must not contain the forwarded pair. + for _, kv := range ss.incubatorEnv() { + if strings.Contains(kv, secret) { + t.Errorf("forwarded pair present in child environment: %q", kv) + } + } +} + +// TestIncubatorEnvServerOnly verifies that no client-forwarded variable enters +// the privileged child's environment, even under a wildcard acceptEnv policy: +// cmd.Env carries only server-chosen values and the client's TERM/LANG/LC_* +// (matching OpenSSH's default AcceptEnv). Forwarded pairs travel via an +// inherited file instead (see forwardedEnvFile), so even names like PATH and +// GODEBUG that are unsafe in the privileged child can still be delivered to +// the user's session. +func TestIncubatorEnvServerOnly(t *testing.T) { + ss := newTestSession(t, + []string{"*"}, + []string{ + "PATH=/client/evil", + "HOME=/tmp/evil", + "GODEBUG=asyncpreemptoff=1", + "GIT_TOKEN=fromclient", + "TERM=xterm-256color", // accepted via acceptEnvPair, not filterEnv + }, + ) + _, forwardedEnv, err := ss.newIncubatorCommand(logger.Discard) + if err != nil { + t.Fatalf("newIncubatorCommand: %v", err) + } + env := ss.incubatorEnv() + + for _, kv := range env { + for _, evil := range []string{"/client/evil", "/tmp/evil", "asyncpreemptoff", "fromclient"} { + if strings.Contains(kv, evil) { + t.Errorf("client-controlled value %q present in child environment: %q", evil, env) + } + } + } + // USER/HOME come from the server; TERM is the one client-controlled value + // allowed in (OpenSSH parity). + for _, want := range []string{"USER=alice", "HOME=/home/alice", "TERM=xterm-256color"} { + if !slices.Contains(env, want) { + t.Errorf("%q missing from child environment: %q", want, env) + } + } + // filterEnv accepted the pairs for fd delivery to the user's session. + for _, want := range []string{"GIT_TOKEN=fromclient", "GODEBUG=asyncpreemptoff=1", "PATH=/client/evil"} { + if !slices.Contains(forwardedEnv, want) { + t.Errorf("%q missing from forwardedEnv: %q", want, forwardedEnv) + } + } +} + +// TestForwardedEnvFileRoundTrip verifies the parent-side payload pipe: the +// child decodes the JSON-encoded pairs from the read end, and calling it with +// nothing to forward is an error. +func TestForwardedEnvFileRoundTrip(t *testing.T) { + if f, err := forwardedEnvFile(nil); err == nil { + t.Fatalf("forwardedEnvFile(nil) = %v, nil; want error", f) + } + + pairs := []string{"GITLAB_API_TOKEN=s3cr3t", "PATH=/client/bin"} + f, err := forwardedEnvFile(pairs) + if err != nil { + t.Fatalf("forwardedEnvFile: %v", err) + } + + // Dup the read end (like the child's ExtraFiles fd) and close the parent's copy + dup, err := unix.Dup(int(f.Fd())) + if err != nil { + t.Fatalf("dup: %v", err) + } + f.Close() + ia := incubatorArgs{envFD: dup} + if err := ia.loadForwardedEnv(); err != nil { + t.Fatalf("loadForwardedEnv: %v", err) + } + if !slices.Equal(ia.forwardedEnv, pairs) { + t.Errorf("forwardedEnv = %q, want %q", ia.forwardedEnv, pairs) + } + + // forwardedEnviron applies the pairs to the user's environment and names + // them in the "su -w" allowlist, alongside SSH_AUTH_SOCK. + env, keys := ia.forwardedEnviron() + for _, p := range pairs { + if !slices.Contains(env, p) { + t.Errorf("pair %q missing from forwardedEnviron env", p) + } + } + for _, k := range []string{"SSH_AUTH_SOCK", "GITLAB_API_TOKEN", "PATH"} { + if !slices.Contains(keys, k) { + t.Errorf("allowlist missing %q: %q", k, keys) + } + } +} + +// TestParseIncubatorArgsEnvFD verifies that --env-fd values naming +// stdin/stdout/stderr are rejected: a legitimate payload fd always comes from +// an ExtraFiles entry, so it is >= 3. +func TestParseIncubatorArgsEnvFD(t *testing.T) { + if _, err := parseIncubatorArgs([]string{"--groups=1000", "--env-fd=2"}); err == nil { + t.Errorf("--env-fd=2: got nil error, want rejection") + } + for _, args := range [][]string{ + {"--groups=1000", "--env-fd=3"}, + {"--groups=1000"}, // unset defaults to -1 + } { + if _, err := parseIncubatorArgs(args); err != nil { + t.Errorf("%v: got error %v, want nil", args, err) + } + } +} + +// TestLoadForwardedEnvLegacyEncodedEnv covers the deprecated --encoded-env +// compatibility path: an outdated parent tailscaled passes the accepted +// environment as a quoted JSON argv flag, and the child must still decode it +// into the user's environment and the "su -w" allowlist. +func TestLoadForwardedEnvLegacyEncodedEnv(t *testing.T) { + pairs := []string{"GITLAB_API_TOKEN=s3cr3t", "OTHER=1"} + raw, _ := json.Marshal(pairs) + // Exactly what an old parent puts on the argv. + ia, err := parseIncubatorArgs([]string{"--groups=1000", "--encoded-env=" + fmt.Sprintf("%q", raw)}) + if err != nil { + t.Fatalf("parseIncubatorArgs: %v", err) + } + if err := ia.loadForwardedEnv(); err != nil { + t.Fatalf("loadForwardedEnv: %v", err) + } + if !slices.Equal(ia.forwardedEnv, pairs) { + t.Errorf("forwardedEnv = %q, want %q", ia.forwardedEnv, pairs) + } + + env, keys := ia.forwardedEnviron() + for _, p := range pairs { + if !slices.Contains(env, p) { + t.Errorf("legacy pair %q missing from env", p) + } + } + for _, k := range []string{"SSH_AUTH_SOCK", "GITLAB_API_TOKEN", "OTHER"} { + if !slices.Contains(keys, k) { + t.Errorf("allowlist missing %q: %q", k, keys) + } + } + + // Malformed values are rejected. + bad := incubatorArgs{encodedEnv: "%q-not-json"} + if err := bad.loadForwardedEnv(); err == nil { + t.Errorf("malformed encodedEnv: got nil error") + } +} + +// TestLoadForwardedEnvSanitizesPairs verifies the child-side integrity check: +// pairs that would corrupt the "su -w" allowlist (empty or comma-carrying +// names), truncate on exec (NUL bytes), or lack "=" are dropped when the +// payload is loaded, even though a new parent would never send them. +func TestLoadForwardedEnvSanitizesPairs(t *testing.T) { + pairs := []string{"GOOD=1", "A,B=x", "C\x00D=2", "E=3\x004", "=x", "malformed", "MY VAR=6", "MY.VAR=7"} + raw, _ := json.Marshal(pairs) + ia, err := parseIncubatorArgs([]string{"--groups=1000", "--encoded-env=" + fmt.Sprintf("%q", raw)}) + if err != nil { + t.Fatalf("parseIncubatorArgs: %v", err) + } + if err := ia.loadForwardedEnv(); err != nil { + t.Fatalf("loadForwardedEnv: %v", err) + } + if !slices.Equal(ia.forwardedEnv, []string{"GOOD=1"}) { + t.Errorf("forwardedEnv = %q, want [GOOD=1]", ia.forwardedEnv) + } + _, keys := ia.forwardedEnviron() + if !slices.Equal(keys, []string{"SSH_AUTH_SOCK", "GOOD"}) { + t.Errorf("allowlist = %q, want [SSH_AUTH_SOCK GOOD]", keys) + } +} diff --git a/ssh/tailssh/localstate_test.go b/ssh/tailssh/localstate_test.go new file mode 100644 index 000000000..c64afc87e --- /dev/null +++ b/ssh/tailssh/localstate_test.go @@ -0,0 +1,105 @@ +// Copyright (c) Tailscale Inc & contributors +// SPDX-License-Identifier: BSD-3-Clause + +//go:build (linux && !android) || (darwin && !ios) || freebsd || openbsd + +package tailssh + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "net/netip" + "strings" + + "tailscale.com/net/tsdial" + "tailscale.com/tailcfg" + "tailscale.com/types/key" + "tailscale.com/types/netmap" + "tailscale.com/util/set" +) + +// localState implements ipnLocalBackend for testing. +type localState struct { + sshEnabled bool + 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. + serverActions map[string]*tailcfg.SSHAction +} + +func (ts *localState) Dialer() *tsdial.Dialer { + return &tsdial.Dialer{} +} + +func (ts *localState) ShouldRunSSH() bool { + return ts.sshEnabled +} + +func (ts *localState) NetMap() *netmap.NetworkMap { + var policy *tailcfg.SSHPolicy + if ts.matchingRule != nil { + policy = &tailcfg.SSHPolicy{ + Rules: []*tailcfg.SSHRule{ + ts.matchingRule, + }, + } + } + + return &netmap.NetworkMap{ + SelfNode: (&tailcfg.Node{ + ID: 1, + }).View(), + SSHPolicy: policy, + AllCaps: set.SetOf(ts.caps), + } +} + +func (ts *localState) NetMapNoPeers() *netmap.NetworkMap { return ts.NetMap() } + +func (ts *localState) WhoIs(proto string, ipp netip.AddrPort) (n tailcfg.NodeView, u tailcfg.UserProfile, ok bool) { + if proto != "tcp" { + return tailcfg.NodeView{}, tailcfg.UserProfile{}, false + } + + return (&tailcfg.Node{ + ID: 2, + StableID: "peer-id", + }).View(), tailcfg.UserProfile{ + LoginName: "peer", + }, true + +} + +func (ts *localState) DoNoiseRequest(req *http.Request) (*http.Response, error) { + rec := httptest.NewRecorder() + k, ok := strings.CutPrefix(req.URL.Path, "/ssh-action/") + if !ok { + rec.WriteHeader(http.StatusNotFound) + } + a, ok := ts.serverActions[k] + if !ok { + rec.WriteHeader(http.StatusNotFound) + return rec.Result(), nil + } + rec.WriteHeader(http.StatusOK) + if err := json.NewEncoder(rec).Encode(a); err != nil { + return nil, err + } + return rec.Result(), nil +} + +func (ts *localState) TailscaleVarRoot() string { + return ts.varRoot +} + +func (ts *localState) NodeKey() key.NodePublic { + return key.NewNode().Public() +} diff --git a/ssh/tailssh/tailssh.go b/ssh/tailssh/tailssh.go index 984e21cea..23931485f 100644 --- a/ssh/tailssh/tailssh.go +++ b/ssh/tailssh/tailssh.go @@ -771,6 +771,35 @@ type sshSession struct { exitHandled chan struct{} } +// forwardedEnvChildFD is the fd the incubator child reads the forwarded environment from, sent via +// --env-fd. It must match the payload file's index in launchProcess's ExtraFiles (fd = 3 + index). +const forwardedEnvChildFD = 3 + +// forwardedEnvFile returns the read end of a pipe holding the JSON-encoded forwarded pairs. +// The read end is passed to the incubator child via exec.Cmd.ExtraFiles to communicate +// secrets and config; the payload only ever exists in memory, never on any filesystem. A +// goroutine writes the payload and closes the write end. Caller must close the read end +// after the child starts. +func forwardedEnvFile(forwardedEnv []string) (*os.File, error) { + if len(forwardedEnv) == 0 { + return nil, errors.New("no forwarded environment") + } + b, err := json.Marshal(forwardedEnv) + if err != nil { + return nil, fmt.Errorf("marshaling forwarded environment: %w", err) + } + r, w, err := os.Pipe() + if err != nil { + return nil, fmt.Errorf("creating forwarded environment pipe: %w", err) + } + go func() { + defer w.Close() + // A short read fails the session child-side + _, _ = w.Write(b) + }() + return r, nil +} + func (ss *sshSession) vlogf(format string, args ...any) { if sshVerboseLogging() { ss.logf(format, args...) diff --git a/ssh/tailssh/tailssh_integration_test.go b/ssh/tailssh/tailssh_integration_test.go index 7b70a6d51..36473af1a 100644 --- a/ssh/tailssh/tailssh_integration_test.go +++ b/ssh/tailssh/tailssh_integration_test.go @@ -24,6 +24,7 @@ "path/filepath" "runtime" "strings" + "sync" "testing" "time" @@ -36,6 +37,7 @@ "tailscale.com/net/tsdial" "tailscale.com/tailcfg" "tailscale.com/types/key" + "tailscale.com/types/logger" "tailscale.com/types/netmap" "tailscale.com/util/set" ) @@ -190,6 +192,65 @@ func TestIntegrationSSH(t *testing.T) { } } +// TestIntegrationAcceptEnvSecretNotLogged is the end-to-end regression test +// for the acceptEnv secret leak: a forwarded secret must reach the session +// environment, but its value must NOT appear on any process command line +// nor in the server or incubator logs. +func TestIntegrationAcceptEnvSecretNotLogged(t *testing.T) { + for _, forceV1Behavior := range []bool{false, true} { + name := "v2" + if forceV1Behavior { + name = "v1" + } + t.Run(name, func(t *testing.T) { + canary := fmt.Sprintf("e2e-canary-%d", time.Now().UnixNano()) + + var logBuf lockedBuffer + addr := testServerWithOpts(t, testServerOpts{ + username: "testuser", + forceV1Behavior: forceV1Behavior, + allowSendEnv: true, + logf: log.New(&logBuf, "", 0).Printf, + }) + cl, err := ssh.Dial("tcp", addr, &ssh.ClientConfig{ + HostKeyCallback: ssh.InsecureIgnoreHostKey(), + }) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { cl.Close() }) + + s := testSessionFor(t, cl, map[string]string{"GIT_E2E_CANARY": canary}) + + // Prove delivery, and keep the session alive while we scan /proc + if err := s.Start("env; sleep 5"); err != nil { + t.Fatalf("unable to start command: %s", err) + } + if got := s.read(); !strings.Contains(got, "GIT_E2E_CANARY="+canary) { + t.Fatalf("forwarded secret not delivered to session env; got %q", got) + } + if runtime.GOOS == "linux" { + if path := findInProcCmdlines(canary); path != "" { + t.Errorf("secret value visible on command line at %s", path) + } + } + s.Close() + + // The parent's session logs include the session-start argv log + if got := logBuf.String(); strings.Contains(got, canary) { + t.Errorf("secret value present in server logs: %q", got) + } else if strings.Contains(got, "GIT_E2E_CANARY") { + t.Errorf("forwarded key name present in server logs: %q", got) + } + + // The incubator child writes its debug log in debugTest mode + if b, err := os.ReadFile("/tmp/tailscalessh.log"); err == nil && bytes.Contains(b, []byte(canary)) { + t.Errorf("secret value present in incubator debug log") + } + }) + } +} + func TestIntegrationSFTP(t *testing.T) { for _, forceV1Behavior := range []bool{false, true} { name := "v2" @@ -663,6 +724,36 @@ func fallbackToSUAvailable() bool { return err == nil } +// lockedBuffer is a goroutine-safe bytes.Buffer for capturing server logs. +type lockedBuffer struct { + mu sync.Mutex + buf bytes.Buffer +} + +func (b *lockedBuffer) Write(p []byte) (int, error) { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.Write(p) +} + +func (b *lockedBuffer) String() string { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.String() +} + +// findInProcCmdlines returns the path of the first /proc//cmdline +// containing s, or "" if none. Linux only. +func findInProcCmdlines(s string) string { + paths, _ := filepath.Glob("/proc/[0-9]*/cmdline") + for _, p := range paths { + if b, err := os.ReadFile(p); err == nil && strings.Contains(string(b), s) { + return p + } + } + return "" +} + type session struct { *ssh.Session @@ -779,10 +870,15 @@ type testServerOpts struct { allowSendEnv bool allowLocalPortForwarding bool allowRemotePortForwarding bool + logf logger.Logf // defaults to log.Printf } func testServerWithOpts(t *testing.T, opts testServerOpts) string { t.Helper() + logf := opts.logf + if logf == nil { + logf = log.Printf + } srv := &server{ lb: &testBackend{ localUser: opts.username, @@ -791,7 +887,7 @@ func testServerWithOpts(t *testing.T, opts testServerOpts) string { allowLocalPortForwarding: opts.allowLocalPortForwarding, allowRemotePortForwarding: opts.allowRemotePortForwarding, }, - logf: log.Printf, + logf: logf, tailscaledPath: os.Getenv("TAILSCALED_PATH"), timeNow: time.Now, } diff --git a/ssh/tailssh/tailssh_test.go b/ssh/tailssh/tailssh_test.go index 1b5fdd2d7..c4fe0741d 100644 --- a/ssh/tailssh/tailssh_test.go +++ b/ssh/tailssh/tailssh_test.go @@ -37,13 +37,10 @@ "golang.org/x/net/http2/h2c" "tailscale.com/cmd/testwrapper/flakytest" "tailscale.com/net/memnet" - "tailscale.com/net/tsdial" "tailscale.com/sessionrecording" "tailscale.com/tailcfg" testssh "tailscale.com/tempfork/sshtest/ssh" "tailscale.com/tstest" - "tailscale.com/types/key" - "tailscale.com/types/netmap" "tailscale.com/util/cibuild" "tailscale.com/util/lineiter" "tailscale.com/util/must" @@ -373,18 +370,6 @@ func TestEvalSSHPolicy(t *testing.T) { } } -// localState implements ipnLocalBackend for testing. -type localState struct { - sshEnabled bool - matchingRule *tailcfg.SSHRule - varRoot string // if empty, TailscaleVarRoot returns "" - - // 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. - serverActions map[string]*tailcfg.SSHAction -} - var currentUser = func() string { // Prefer user.Current because the USER env var is not set in // some environments (e.g. the golang:latest container used by CI). @@ -427,74 +412,6 @@ func aNonRootUser(t *testing.T) string { return "" } -func (ts *localState) Dialer() *tsdial.Dialer { - return &tsdial.Dialer{} -} - -func (ts *localState) ShouldRunSSH() bool { - return ts.sshEnabled -} - -func (ts *localState) NetMap() *netmap.NetworkMap { - var policy *tailcfg.SSHPolicy - if ts.matchingRule != nil { - policy = &tailcfg.SSHPolicy{ - Rules: []*tailcfg.SSHRule{ - ts.matchingRule, - }, - } - } - - return &netmap.NetworkMap{ - SelfNode: (&tailcfg.Node{ - ID: 1, - }).View(), - SSHPolicy: policy, - } -} - -func (ts *localState) NetMapNoPeers() *netmap.NetworkMap { return ts.NetMap() } - -func (ts *localState) WhoIs(proto string, ipp netip.AddrPort) (n tailcfg.NodeView, u tailcfg.UserProfile, ok bool) { - if proto != "tcp" { - return tailcfg.NodeView{}, tailcfg.UserProfile{}, false - } - - return (&tailcfg.Node{ - ID: 2, - StableID: "peer-id", - }).View(), tailcfg.UserProfile{ - LoginName: "peer", - }, true - -} - -func (ts *localState) DoNoiseRequest(req *http.Request) (*http.Response, error) { - rec := httptest.NewRecorder() - k, ok := strings.CutPrefix(req.URL.Path, "/ssh-action/") - if !ok { - rec.WriteHeader(http.StatusNotFound) - } - a, ok := ts.serverActions[k] - if !ok { - rec.WriteHeader(http.StatusNotFound) - return rec.Result(), nil - } - rec.WriteHeader(http.StatusOK) - if err := json.NewEncoder(rec).Encode(a); err != nil { - return nil, err - } - return rec.Result(), nil -} - -func (ts *localState) TailscaleVarRoot() string { - return ts.varRoot -} - -func (ts *localState) NodeKey() key.NodePublic { - return key.NewNode().Public() -} - func newSSHRule(action *tailcfg.SSHAction) *tailcfg.SSHRule { return &tailcfg.SSHRule{ SSHUsers: map[string]string{