From 1e30e732e2d5d88a460777a8d2244ff6a406ca90 Mon Sep 17 00:00:00 2001 From: Yosuke Shimizu Date: Mon, 28 Sep 2026 11:05:08 +0900 Subject: [PATCH] wolfsshd: bound the shell child reap and flush the held backlog - The pty master is closed before the reap. The reap polls waitpid() with WNOHANG for WOLFSSHD_CHILD_REAP_TRIES x WOLFSSHD_CHILD_REAP_WAIT_US, then sends SIGKILL; a failed kill() is logged and ends the wait. - ChildSig is installed before forkpty() instead of after it. - An EIO read from the pty master ends the child's output instead of the loop. - A held backlog is sent with SHELL_FlushOut() before the pipe drain; the drain is skipped when the backlog is not sent. - New sshd_channel_close_test.sh (paramiko) closes the channel while output is held, for an exec command and a pty shell, and checks that the command and the connection both end; it also checks exit status 3 from an exec and an interactive shell. run_all runs it; sshd-test.yml and code-coverage.yml install python3-paramiko. --- .github/workflows/code-coverage.yml | 4 +- .github/workflows/sshd-test.yml | 7 + apps/wolfsshd/test/run_all_sshd_tests.sh | 1 + apps/wolfsshd/test/sshd_channel_close_test.sh | 163 ++++++++++++++++++ apps/wolfsshd/wolfsshd.c | 60 ++++++- 5 files changed, 227 insertions(+), 8 deletions(-) create mode 100755 apps/wolfsshd/test/sshd_channel_close_test.sh diff --git a/.github/workflows/code-coverage.yml b/.github/workflows/code-coverage.yml index 627aedde5..278c2d9b5 100644 --- a/.github/workflows/code-coverage.yml +++ b/.github/workflows/code-coverage.yml @@ -55,10 +55,10 @@ jobs: uses: actions/checkout@v6 # clang 18 is the min: -fcoverage-mcdc does not exist before it. - - name: Install clang and LLVM coverage tools + - name: Install clang, LLVM coverage tools and paramiko run: | sudo apt-get update - sudo apt-get install -y clang-18 llvm-18 libclang-rt-18-dev + sudo apt-get install -y clang-18 llvm-18 libclang-rt-18-dev python3-paramiko - name: Download wolfSSL uses: actions/download-artifact@v8 diff --git a/.github/workflows/sshd-test.yml b/.github/workflows/sshd-test.yml index 0469ce559..09be02051 100644 --- a/.github/workflows/sshd-test.yml +++ b/.github/workflows/sshd-test.yml @@ -118,6 +118,13 @@ jobs: working-directory: ./wolfssh/ run: make check + # For sshd_channel_close_test.sh. The suite runs as root, which does not + # see a per-user pip install, so use the distro package. + - name: Install paramiko + run: | + sudo apt-get -y update + sudo apt-get -y install python3-paramiko + - name: Run wolfSSHd tests working-directory: ./wolfssh/apps/wolfsshd/test run: | diff --git a/apps/wolfsshd/test/run_all_sshd_tests.sh b/apps/wolfsshd/test/run_all_sshd_tests.sh index e7279144c..98e4c95d3 100755 --- a/apps/wolfsshd/test/run_all_sshd_tests.sh +++ b/apps/wolfsshd/test/run_all_sshd_tests.sh @@ -15,6 +15,7 @@ test_cases=( "sshd_term_close_test.sh" "sshd_stdin_eof_test.sh" "sshd_stdin_stall_test.sh" + "sshd_channel_close_test.sh" "ssh_kex_algos.sh" ) diff --git a/apps/wolfsshd/test/sshd_channel_close_test.sh b/apps/wolfsshd/test/sshd_channel_close_test.sh new file mode 100755 index 000000000..ac15e83df --- /dev/null +++ b/apps/wolfsshd/test/sshd_channel_close_test.sh @@ -0,0 +1,163 @@ +#!/bin/bash +# Closing a session channel while wolfsshd holds its output must not leave the +# connection process waiting on the command. The exec command traps INT and +# HUP, so only the reap's SIGKILL ends it; a pty shell must exit on the hangup. +# The exit status must still reach the client, from an exec and from a shell. +# +# Needs paramiko: the OpenSSH client never closes the channel early; it keeps +# reading and discards what it cannot write. +HOST="$1" +PORT="$2" +USER_NAME="${3:-`whoami`}" + +command -v python3 >/dev/null 2>&1 || exit 77 +python3 -c "import paramiko" >/dev/null 2>&1 || exit 77 +command -v timeout >/dev/null 2>&1 || exit 77 + +# The command's pid is checked on this machine. +case "$HOST" in + 127.0.0.1|localhost|::1) ;; + *) exit 77 ;; +esac + +KEYDIR=`mktemp -d` || exit 1 +MARKDIR=`mktemp -d` || exit 1 +trap 'rm -rf "$KEYDIR" "$MARKDIR"' EXIT +# Private to the login user, who writes the marker. +chown "$USER_NAME" "$MARKDIR" || exit 77 +cp ../../../keys/hansel-key-ecc.pem "$KEYDIR/id_ecdsa" || exit 1 +chmod 600 "$KEYDIR/id_ecdsa" + +timeout 60 python3 -u - "$HOST" "$PORT" "$USER_NAME" "$KEYDIR/id_ecdsa" \ + "$MARKDIR/exited" <<'EOF' +import os +import re +import sys +import time + +import paramiko + +host, port, user, key = sys.argv[1], int(sys.argv[2]), sys.argv[3], sys.argv[4] +marker = sys.argv[5] +LIMIT = 5 +WAIT = 20 # for any one read or exit status + + +def alive(pid): + try: + os.kill(pid, 0) + except ProcessLookupError: + return False + except PermissionError: + pass + return True + + +def connect(): + client = paramiko.SSHClient() + client.set_missing_host_key_policy(paramiko.AutoAddPolicy()) + client.connect(host, port=port, username=user, + pkey=paramiko.ECDSAKey.from_private_key_file(key), + look_for_keys=False, allow_agent=False, timeout=10) + return client + + +def run_case(name, shell): + client = connect() + chan = client.get_transport().open_session() + chan.settimeout(WAIT) + if shell: + chan.get_pty() + chan.invoke_shell() + # The marker is a builtin redirection on the exit path only. + chan.send("trap 'exit' HUP; trap ': > %s' EXIT; echo PID=$$\n" + % marker) + else: + chan.exec_command('echo PID=$$; trap "" INT HUP; ' + 'seq 1 10000000; sleep 30') + + # The pty echoes the typed line with a literal $$, which does not match, + # and the newline keeps a pid split across two reads from matching. + out = b"" + match = None + while match is None: + data = chan.recv(4096) + if not data: + break + out += data + match = re.search(rb"PID=(\d+)\r?\n", out) + if match is None: + print("%s: no pid from the command" % name) + client.close() + return False + pid = int(match.group(1)) + if shell: + chan.send("seq 1 10000000\n") + + # Stop reading until the window fills and wolfsshd holds output, then + # close the channel with the transport still up. + time.sleep(1) + chan.close() + + start = time.time() + while alive(pid) and time.time() - start < LIMIT: + time.sleep(0.1) + # The connection process ends too, and closes the connection. + transport = client.get_transport() + while transport.is_active() and time.time() - start < LIMIT: + time.sleep(0.1) + closed = not transport.is_active() + client.close() + + if alive(pid): + print("%s: command %d still running %ds after the channel close" % + (name, pid, LIMIT)) + return False + print("%s: command %d gone %.1fs after the channel close" % + (name, pid, time.time() - start)) + if not closed: + print("%s: connection still open %ds after the channel close" % + (name, LIMIT)) + return False + # SIGKILL cannot run the trap, so the marker means the shell exited itself. + if shell and not os.path.exists(marker): + print("%s: killed instead of exiting on the hangup" % name) + return False + return True + + +def run_status(name, shell): + client = connect() + chan = client.get_transport().open_session() + if shell: + # An exec takes the pipe path even with a pty; a shell reads the pty. + chan.get_pty() + chan.invoke_shell() + chan.send("exit 3\n") + else: + chan.exec_command("exit 3") + start = time.time() + while not chan.exit_status_ready() and time.time() - start < WAIT: + time.sleep(0.05) + if not chan.exit_status_ready(): + print("%s: no exit status within %ds" % (name, WAIT)) + client.close() + return False + status = chan.recv_exit_status() + client.close() + print("%s: exit status %d, expected 3" % (name, status)) + return status == 3 + + +try: + connect().close() +except (paramiko.SSHException, OSError) as e: + print("cannot connect with paramiko: %s" % e) + sys.exit(77) + +ok = run_case("exec", False) +ok = run_case("pty shell", True) and ok +ok = run_status("exec status", False) and ok +ok = run_status("shell status", True) and ok +sys.exit(0 if ok else 1) +EOF diff --git a/apps/wolfsshd/wolfsshd.c b/apps/wolfsshd/wolfsshd.c index d56c7a20b..e99494e1f 100644 --- a/apps/wolfsshd/wolfsshd.c +++ b/apps/wolfsshd/wolfsshd.c @@ -2672,6 +2672,12 @@ static int SHELL_SetNonBlocking(int fd) #ifndef WOLFSSHD_SHELL_FLUSH_WAIT_US #define WOLFSSHD_SHELL_FLUSH_WAIT_US 50000 #endif +#ifndef WOLFSSHD_CHILD_REAP_TRIES + #define WOLFSSHD_CHILD_REAP_TRIES 10 +#endif +#ifndef WOLFSSHD_CHILD_REAP_WAIT_US + #define WOLFSSHD_CHILD_REAP_WAIT_US 50000 +#endif /* Send out the last of a shell's output once the child has been reaped. The * SSH socket is non blocking, so retry a bounded number of times on a full @@ -2784,6 +2790,7 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, * destructive, so what a short write leaves is * carried to the next pass. */ int childInIdx = 0; /* How much of those the child has taken. */ + int drainPipes = 1; backlog.len = 0; backlog.state = SHELL_SEND_READY; @@ -2862,6 +2869,8 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, } } + /* Before the fork, so a child that exits at once clears ChildRunning */ + signal(SIGCHLD, ChildSig); ChildRunning = 1; childPid = forkpty(&childFd, NULL, NULL, NULL); if (childPid < 0) { @@ -3039,7 +3048,6 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, return WS_FATAL_ERROR; } - signal(SIGCHLD, ChildSig); signal(SIGINT, SIG_DFL); rc = tcgetattr(childFd, &tios); @@ -3487,7 +3495,10 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, /* Treat a 0 return as EOF so the loop can shut down. */ if (cnt_r < 0) { int err = errno; - if (err != EINTR && err != EAGAIN + if (err == EIO) { + stdoutEmpty = 1; /* no process has the terminal open */ + } + else if (err != EINTR && err != EAGAIN && err != EWOULDBLOCK) { break; } @@ -3535,14 +3546,39 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, } } + /* Close the pty master so the shell gets SIGHUP and can exit on its own */ + if (childFd >= 0) { + close(childFd); + childFd = -1; + } + /* get return value of child process */ { int waitStatus; + int tries = 0; do { - rc = waitpid(childPid, &waitStatus, 0); + rc = waitpid(childPid, &waitStatus, WNOHANG); + if (rc == 0 && tries < WOLFSSHD_CHILD_REAP_TRIES) { + usleep(WOLFSSHD_CHILD_REAP_WAIT_US); + tries++; + } + else if (rc == 0) { + /* Child did not exit in time; force it with SIGKILL. */ + if (kill(childPid, SIGKILL) == 0) { + rc = waitpid(childPid, &waitStatus, 0); + } + else { + int err = errno; + + wolfSSH_Log(WS_LOG_ERROR, "[SSHD] Unable to kill child " + "process %d, errno %d", (int)childPid, err); + errno = err; + rc = -1; + } + } /* if the waitpid experienced an interrupt then try again */ - } while (rc < 0 && errno == EINTR); + } while (rc == 0 || (rc < 0 && errno == EINTR)); if (rc < 0) { wolfSSH_Log(WS_LOG_ERROR, "[SSHD] Issue waiting for child's exit " @@ -3557,13 +3593,25 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, } } + /* A break can leave unsent shell output in shellBuffer; send it before + * the pipe drain reads into shellBuffer. */ + if (backlog.len > 0) { + if (backlog.state == SHELL_SEND_NEVER + || SHELL_FlushOut(ssh, sshFd, shellChannelId, shellBuffer, + backlog.len, backlog.ext) != 0) { + /* later pipe output would leave a gap in the stream */ + drainPipes = 0; + } + backlog.len = 0; + } + /* check for any left over data in pipes then close them */ if (!ptyReq || forcedCmd) { int readSz; /* when the pipe can not be made non blocking skip the drain, a * blocking read here could hang the connection process */ - if (SHELL_SetNonBlocking(stdoutPipe[0]) == 0) { + if (drainPipes && SHELL_SetNonBlocking(stdoutPipe[0]) == 0) { readSz = (int)read(stdoutPipe[0], shellBuffer, sizeof shellBuffer); if (readSz > 0) { SHELL_FlushOut(ssh, sshFd, shellChannelId, shellBuffer, readSz, @@ -3571,7 +3619,7 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, } } - if (SHELL_SetNonBlocking(stderrPipe[0]) == 0) { + if (drainPipes && SHELL_SetNonBlocking(stderrPipe[0]) == 0) { readSz = (int)read(stderrPipe[0], shellBuffer, sizeof shellBuffer); if (readSz > 0) { SHELL_FlushOut(ssh, sshFd, shellChannelId, shellBuffer, readSz,