[client] Start ConPty SSH shells with bInheritHandles=FALSE so they no longer inherit the daemon's os/exec pipes of other users' sessions (#8391)

* Start ConPty SSH shells with bInheritHandles=FALSE so they no longer inherit the daemon's os/exec pipes of other users' sessions

* Wait for the ConPty test's terminated helper and give its skip path its own exit code

* Skip the ConPty handle-inheritance helper in normal test runs instead of exiting the process

client/ssh/server/winpty/conpty.go

@@ -243,6 +243,12 @@ func createConPtyProcess(commandLine string, userToken windows.Handle, userEnv [
 	siEx.StartupInfo.StdOutput = windows.Handle(0)
 	siEx.StartupInfo.StdErr = siEx.StartupInfo.StdOutput
 
+	// The pseudo console reaches the child through PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE and the
+	// standard handles are already redirected above, so the child needs no inherited handle.
+	// Inheriting here would hand it every inheritable handle open in the daemon, including the
+	// stdio pipes of other OS accounts' non-PTY SSH and SFTP sessions.
+	const inheritHandles = false
+
 	if userToken != windows.InvalidHandle {
 		err = windows.CreateProcessAsUser(
 			windows.Token(userToken),
@@ -250,7 +256,7 @@ func createConPtyProcess(commandLine string, userToken windows.Handle, userEnv [
 			commandLinePtr,
 			nil,
 			nil,
-			true,
+			inheritHandles,
 			creationFlags,
 			envPtr,
 			workingDirPtr,
@@ -263,7 +269,7 @@ func createConPtyProcess(commandLine string, userToken windows.Handle, userEnv [
 			commandLinePtr,
 			nil,
 			nil,
-			true,
+			inheritHandles,
 			creationFlags,
 			envPtr,
 			workingDirPtr,

client/ssh/server/winpty/conpty_test.go

@@ -3,7 +3,11 @@
 package winpty
 
 import (
+	"io"
+	"os"
+	"strconv"
 	"testing"
+	"unsafe"
 
 	log "github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
@@ -267,6 +271,128 @@ func TestWindowsHandleWriter(t *testing.T) {
 	assert.Equal(t, testData, buffer, "Should read back expected data")
 }
 
+const (
+	conPtyHelperHandleEnv = "NB_CONPTY_TEST_INHERITED_HANDLE"
+	// conPtyHelperInvokedEnv marks that this process is the dedicated child started by
+	// TestCreateConPtyProcess_NoHandleInheritance, as opposed to TestConPtyHandleInheritanceHelper
+	// running as an ordinary test in a normal `go test` invocation. Only the child exits the
+	// process; a normal run must skip instead, or it would kill the whole test binary.
+	conPtyHelperInvokedEnv = "NB_CONPTY_TEST_HELPER_INVOKED"
+	// exit codes of the helper process started by TestCreateConPtyProcess_NoHandleInheritance.
+	// conPtyHelperExitSkipped is distinct from conPtyHelperExitNotInherited so the parent can
+	// tell "probed and not inherited" apart from "never probed" (for example if the env var
+	// carrying the handle value failed to reach the child); either outcome exits 0 as a bare
+	// go test binary, so the exit code alone must disambiguate them.
+	conPtyHelperExitNotInherited = 0
+	conPtyHelperExitSkipped      = 2
+	conPtyHelperExitInherited    = 3
+)
+
+// TestCreateConPtyProcess_NoHandleInheritance verifies that a ConPty child does not inherit
+// inheritable handles of this process. The daemon holds os/exec pipes (created inheritable by
+// os.Pipe) for other users' SSH and SFTP sessions; a shell started with bInheritHandles=TRUE and
+// no PROC_THREAD_ATTRIBUTE_HANDLE_LIST would receive all of them.
+func TestCreateConPtyProcess_NoHandleInheritance(t *testing.T) {
+	if testing.Short() {
+		t.Skip("Skipping process tests in short mode")
+	}
+
+	// os.Pipe marks both handles inheritable on Windows, like the pipes os/exec creates
+	pipeRead, pipeWrite, err := os.Pipe()
+	require.NoError(t, err, "Should create pipe")
+	defer func() {
+		_ = pipeRead.Close()
+		_ = pipeWrite.Close()
+	}()
+
+	inputRead, inputWrite, outputRead, outputWrite, err := createConPtyPipes()
+	require.NoError(t, err, "Should create ConPty pipes successfully")
+
+	hPty, err := createConPty(80, 24, inputRead, outputWrite)
+	require.NoError(t, err, "Should create ConPty successfully")
+
+	siEx, err := setupConPtyStartupInfo(hPty)
+	require.NoError(t, err, "Should set up startup info")
+	defer func() {
+		_, _, _ = procDeleteProcThreadAttributeList.Call(uintptr(unsafe.Pointer(siEx.ProcThreadAttributeList)))
+	}()
+
+	// The child is this test binary running only the helper below; with no environment
+	// passed it inherits ours, including the handle value to probe.
+	executable, err := os.Executable()
+	require.NoError(t, err, "Should find test binary")
+	t.Setenv(conPtyHelperInvokedEnv, "1")
+	t.Setenv(conPtyHelperHandleEnv, strconv.FormatUint(uint64(pipeWrite.Fd()), 10))
+	commandLine := buildCommandLine([]string{executable, "-test.run=^TestConPtyHandleInheritanceHelper$"})
+
+	pi, err := createConPtyProcess(commandLine, windows.InvalidHandle, nil, "", siEx)
+	require.NoError(t, err, "Should create ConPty process")
+	defer closeProcessInfo(pi)
+
+	closeHandles(inputRead, outputWrite)
+
+	// Drain the pseudo console output so the child and conhost never block on a full pipe.
+	drained := make(chan struct{})
+	go func() {
+		defer close(drained)
+		_, _ = io.Copy(io.Discard, &windowsHandleReader{handle: outputRead})
+	}()
+
+	event, err := windows.WaitForSingleObject(pi.Process, 30000)
+	require.NoError(t, err, "Should wait for ConPty process")
+	if event != windows.WAIT_OBJECT_0 {
+		_ = windows.TerminateProcess(pi.Process, 1)
+		// Wait for the termination to actually land before reading the exit code or
+		// closing handles, or the process can briefly survive as a leaked child.
+		_, _ = windows.WaitForSingleObject(pi.Process, 5000)
+	}
+
+	var exitCode uint32
+	require.NoError(t, windows.GetExitCodeProcess(pi.Process, &exitCode), "Should get exit code")
+
+	if ret, _, err := procClosePseudoConsole.Call(uintptr(hPty)); ret == 0 {
+		t.Logf("ClosePseudoConsole failed: %v", err)
+	}
+	closeHandles(inputWrite)
+	<-drained
+	closeHandles(outputRead)
+
+	require.NotEqual(t, uint32(conPtyHelperExitSkipped), exitCode, "helper must probe the handle, not skip: the env var should have reached the child")
+	require.NotEqual(t, uint32(conPtyHelperExitInherited), exitCode, "ConPty child must not inherit the daemon's pipe handles")
+	require.Equal(t, uint32(conPtyHelperExitNotInherited), exitCode, "helper process should run to completion")
+}
+
+// TestConPtyHandleInheritanceHelper is the child of TestCreateConPtyProcess_NoHandleInheritance.
+// A normal `go test` run also matches and executes this test name, so it must skip rather than
+// exit the process unless conPtyHelperInvokedEnv marks it as the dedicated child invocation.
+// Once invoked as that child, it exits with conPtyHelperExitInherited when the handle value
+// named in the environment is a live pipe handle in this process, and with
+// conPtyHelperExitSkipped if the handle value itself failed to reach the child, which the
+// parent treats as a failure rather than a silent pass.
+func TestConPtyHandleInheritanceHelper(t *testing.T) {
+	if os.Getenv(conPtyHelperInvokedEnv) == "" {
+		t.Skip("helper process only, run as the child of TestCreateConPtyProcess_NoHandleInheritance")
+	}
+
+	value := os.Getenv(conPtyHelperHandleEnv)
+	if value == "" {
+		os.Exit(conPtyHelperExitSkipped)
+	}
+
+	handleValue, err := strconv.ParseUint(value, 10, 64)
+	if err != nil {
+		os.Exit(4)
+	}
+
+	// GetFileType fails when the value is not a handle of this process. A handle value may also
+	// be reused for something else, so only a pipe counts as the parent's pipe.
+	fileType, err := windows.GetFileType(windows.Handle(handleValue))
+	if err == nil && fileType == windows.FILE_TYPE_PIPE {
+		os.Exit(conPtyHelperExitInherited)
+	}
+	os.Exit(conPtyHelperExitNotInherited)
+}
+
 // BenchmarkConPtyCreation benchmarks ConPty creation performance
 func BenchmarkConPtyCreation(b *testing.B) {
 	for i := 0; i < b.N; i++ {