From ab3622e6729f823a019013fd7ad11658cdb23179 Mon Sep 17 00:00:00 2001 From: JacobBarthelmeh Date: Mon, 17 Feb 2025 10:07:39 -0700 Subject: [PATCH 1/4] fix for FD_SET call on pipes and handling of channel EOF --- apps/wolfsshd/wolfsshd.c | 53 ++++++++++++++++++++++++++-------------- 1 file changed, 35 insertions(+), 18 deletions(-) diff --git a/apps/wolfsshd/wolfsshd.c b/apps/wolfsshd/wolfsshd.c index 1d4fb898..4794f3e6 100644 --- a/apps/wolfsshd/wolfsshd.c +++ b/apps/wolfsshd/wolfsshd.c @@ -816,7 +816,8 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, BOOL ret; word32 shellChannelId = 0; #ifndef EXAMPLE_BUFFER_SZ -#define EXAMPLE_BUFFER_SZ 4096 + /* default to try and read max packet size */ + #define EXAMPLE_BUFFER_SZ 32768 #endif byte shellBuffer[EXAMPLE_BUFFER_SZ]; int cnt_r, cnt_w; @@ -1166,7 +1167,8 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, pid_t childPid; #ifndef EXAMPLE_BUFFER_SZ - #define EXAMPLE_BUFFER_SZ 4096 + /* default to try and read max packet size */ + #define EXAMPLE_BUFFER_SZ 32768 #endif #ifndef MAX_IDLE_COUNT #define MAX_IDLE_COUNT 2 @@ -1431,23 +1433,23 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, FD_SET(sshFd, &writeFds); } - /* select on stdout/stderr pipes with forced commands */ - if (forcedCmd) { - FD_SET(stdoutPipe[0], &readFds); - if (stdoutPipe[0] > maxFd) - maxFd = stdoutPipe[0]; - - FD_SET(stderrPipe[0], &readFds); - if (stderrPipe[0] > maxFd) - maxFd = stderrPipe[0]; - } - else { - FD_SET(childFd, &readFds); - if (childFd > maxFd) - maxFd = childFd; - } - if (wolfSSH_stream_peek(ssh, tmp, 1) <= 0) { + /* select on stdout/stderr pipes with forced commands */ + if (forcedCmd) { + FD_SET(stdoutPipe[0], &readFds); + if (stdoutPipe[0] > maxFd) + maxFd = stdoutPipe[0]; + + FD_SET(stderrPipe[0], &readFds); + if (stderrPipe[0] > maxFd) + maxFd = stderrPipe[0]; + } + else { + FD_SET(childFd, &readFds); + if (childFd > maxFd) + maxFd = childFd; + } + rc = select((int)maxFd + 1, &readFds, &writeFds, NULL, NULL); if (rc == -1) break; @@ -1503,6 +1505,21 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, break; } } + + /* did the channel just receive an EOF? */ + if (cnt_r == 0) { + int eof; + WOLFSSH_CHANNEL* current; + + current = wolfSSH_ChannelFind(ssh, lastChannel, + WS_CHANNEL_ID_SELF); + eof = wolfSSH_ChannelGetEof(current); + if (eof) { + /* SSH is done, kill off child process */ + kill(childPid, SIGKILL); + break; + } + } } /* if the window was previously full, try resending the data */ From bbe3bac2d5eed43767f67add60db08db334dcaed Mon Sep 17 00:00:00 2001 From: JacobBarthelmeh Date: Tue, 18 Feb 2025 09:49:20 -0700 Subject: [PATCH 2/4] add regression test and use a better macro name --- .github/workflows/sshd-test.yml | 19 +++++++++++++++++++ apps/wolfsshd/wolfsshd.c | 23 ++++++++++++++--------- 2 files changed, 33 insertions(+), 9 deletions(-) diff --git a/.github/workflows/sshd-test.yml b/.github/workflows/sshd-test.yml index b1f70541..3fbe3daf 100644 --- a/.github/workflows/sshd-test.yml +++ b/.github/workflows/sshd-test.yml @@ -108,6 +108,25 @@ jobs: ./configure --enable-all LDFLAGS="-L${{ github.workspace }}/build-dir/lib" CPPFLAGS="-I${{ github.workspace }}/build-dir/include -DWOLFSSH_NO_FPKI -DWOLFSSH_NO_SFTP_TIMEOUT -DWOLFSSH_MAX_SFTP_RW=4000000 -DMAX_PATH_SZ=120" --enable-static --disable-shared && make sudo timeout --preserve-status -s 2 5 valgrind --error-exitcode=1 --leak-check=full ./apps/wolfsshd/wolfsshd -D -f sshd_config -h ./keys/server-key.pem -d -p 22222 + # regression test, check that cat command does not hang + - name: Test cat command for hanging + working-directory: ./wolfssh/ + timeout-minutes: 1 + run: | + touch sshd_config.txt + echo "AuthorizedKeysFile $PWD/authorized_keys_test" >> sshd_config.txt + cat ./keys/hansel-*.pub > authorized_keys_test + sed -i.bak "s/hansel/$USER/" ./authorized_keys_test + ./configure --enable-all LDFLAGS="-L${{ github.workspace }}/build-dir/lib" CPPFLAGS="-I${{ github.workspace }}/build-dir/include -DWOLFSSH_NO_FPKI -DWOLFSSH_NO_SFTP_TIMEOUT -DWOLFSSH_MAX_SFTP_RW=4000000 -DMAX_PATH_SZ=120" --enable-static --disable-shared && make + sudo ./apps/wolfsshd/wolfsshd -f sshd_config.txt -h ./keys/server-key.pem -p 22225 + chmod 600 ./keys/hansel-key-rsa.pem + tail -c 50000 /dev/urandom > test + while ! nc -z 127.0.0.1 22225; do echo "waiting for wolfSSHd"; sleep 0.2; done + cat test | ssh -vvv -T -i ./keys/hansel-key-rsa.pem -oStrictHostKeyChecking=no 127.0.0.1 -p 22225 'cat > test-file' + diff test ~/test-file + sudo pkill wolfsshd + + - name: configure with debug working-directory: ./wolfssh/ run : | diff --git a/apps/wolfsshd/wolfsshd.c b/apps/wolfsshd/wolfsshd.c index 4794f3e6..3d2aeb1c 100644 --- a/apps/wolfsshd/wolfsshd.c +++ b/apps/wolfsshd/wolfsshd.c @@ -57,6 +57,11 @@ #define WOLFSSHD_TIMEOUT 1 #endif +#ifdef EXAMPLE_BUFFER_SZ + #warning use WOLFSSHD_SHELL_BUFFER_SZ instead of EXAMPLE_BUFFER_SZ + #define WOLFSSHD_SHELL_BUFFER_SZ EXAMPLE_BUFFER_SZ +#endif + #if defined(WOLFSSH_SHELL) && !defined(_WIN32) #ifdef HAVE_PTY_H #include @@ -815,11 +820,11 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, { BOOL ret; word32 shellChannelId = 0; -#ifndef EXAMPLE_BUFFER_SZ +#ifndef WOLFSSHD_SHELL_BUFFER_SZ /* default to try and read max packet size */ - #define EXAMPLE_BUFFER_SZ 32768 + #define WOLFSSHD_SHELL_BUFFER_SZ 32768 #endif - byte shellBuffer[EXAMPLE_BUFFER_SZ]; + byte shellBuffer[WOLFSSHD_SHELL_BUFFER_SZ]; int cnt_r, cnt_w; HANDLE ptyIn = NULL, ptyOut = NULL; HANDLE cnslIn = NULL, cnslOut = NULL; @@ -1106,9 +1111,9 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, } if (readPending) { - WMEMSET(shellBuffer, 0, EXAMPLE_BUFFER_SZ); + WMEMSET(shellBuffer, 0, WOLFSSHD_SHELL_BUFFER_SZ); - if (ReadFile(ptyOut, shellBuffer, EXAMPLE_BUFFER_SZ, &cnt_r, + if (ReadFile(ptyOut, shellBuffer, WOLFSSHD_SHELL_BUFFER_SZ, &cnt_r, NULL) != TRUE) { wolfSSH_Log(WS_LOG_INFO, "[SSHD] Error reading from pipe for console"); @@ -1166,15 +1171,15 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, int stdoutPipe[2], stderrPipe[2]; pid_t childPid; -#ifndef EXAMPLE_BUFFER_SZ +#ifndef WOLFSSHD_SHELL_BUFFER_SZ /* default to try and read max packet size */ - #define EXAMPLE_BUFFER_SZ 32768 + #define WOLFSSHD_SHELL_BUFFER_SZ 32768 #endif #ifndef MAX_IDLE_COUNT #define MAX_IDLE_COUNT 2 #endif - byte shellBuffer[EXAMPLE_BUFFER_SZ]; - byte channelBuffer[EXAMPLE_BUFFER_SZ]; + byte shellBuffer[WOLFSSHD_SHELL_BUFFER_SZ]; + byte channelBuffer[WOLFSSHD_SHELL_BUFFER_SZ]; char* forcedCmd; int windowFull = 0; /* Contains size of bytes from shellBuffer that did * not get passed on to wolfSSH yet. This happens From 60cd4bca73dfe6dbc011b5b17534dacb86580f30 Mon Sep 17 00:00:00 2001 From: JacobBarthelmeh Date: Wed, 19 Feb 2025 10:32:08 -0700 Subject: [PATCH 3/4] redirect stdin with forced command --- apps/wolfsshd/wolfsshd.c | 42 ++++++++++++++++++++++++++++++++++------ 1 file changed, 36 insertions(+), 6 deletions(-) diff --git a/apps/wolfsshd/wolfsshd.c b/apps/wolfsshd/wolfsshd.c index 3d2aeb1c..c76826f5 100644 --- a/apps/wolfsshd/wolfsshd.c +++ b/apps/wolfsshd/wolfsshd.c @@ -1169,6 +1169,7 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, int rc; WS_SOCKET_T childFd = 0; int stdoutPipe[2], stderrPipe[2]; + int stdinPipe[2]; pid_t childPid; #ifndef WOLFSSHD_SHELL_BUFFER_SZ @@ -1193,6 +1194,8 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, stdoutPipe[1] = -1; stderrPipe[0] = -1; stderrPipe[1] = -1; + stdinPipe[0] = -1; + stdinPipe[1] = -1; forcedCmd = wolfSSHD_ConfigGetForcedCmd(usrConf); @@ -1223,10 +1226,18 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, } if (pipe(stderrPipe) != 0) { close(stdoutPipe[0]); - close(stderrPipe[1]); + close(stdoutPipe[1]); wolfSSH_Log(WS_LOG_ERROR, "[SSHD] Issue creating stderr pipe"); return WS_FATAL_ERROR; } + if (pipe(stdinPipe) != 0) { + close(stdoutPipe[0]); + close(stdoutPipe[1]); + close(stderrPipe[0]); + close(stderrPipe[1]); + wolfSSH_Log(WS_LOG_ERROR, "[SSHD] Issue creating stdin pipe"); + return WS_FATAL_ERROR; + } } ChildRunning = 1; @@ -1250,8 +1261,20 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, if (forcedCmd) { close(stdoutPipe[0]); close(stderrPipe[0]); + close(stdinPipe[1]); stdoutPipe[0] = -1; stderrPipe[0] = -1; + stdinPipe[1] = -1; + + if (dup2(stdinPipe[0], STDIN_FILENO) == -1) { + wolfSSH_Log(WS_LOG_ERROR, + "[SSHD] Error redirecting stdin pipe"); + if (wolfSSHD_AuthReducePermissions(conn->auth) != WS_SUCCESS) { + exit(1); + } + + return WS_FATAL_ERROR; + } if (dup2(stdoutPipe[1], STDOUT_FILENO) == -1) { wolfSSH_Log(WS_LOG_ERROR, "[SSHD] Error redirecting stdout pipe"); @@ -1361,6 +1384,7 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, ret = execv(cmd, (char**)args); close(stdoutPipe[1]); close(stderrPipe[1]); + close(stdinPipe[1]); } else { ret = execv(cmd, (char**)args); @@ -1418,6 +1442,7 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, if (forcedCmd) { close(stdoutPipe[1]); close(stderrPipe[1]); + close(stdinPipe[0]); } while (ChildRunning || windowFull || !stdoutEmpty || peerConnected) { @@ -1485,8 +1510,9 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, sizeof channelBuffer); if (cnt_r <= 0) break; - cnt_w = (int)write(childFd, - channelBuffer, cnt_r); + + cnt_w = (int)write(stdinPipe[1], channelBuffer, + cnt_r); if (cnt_w <= 0) break; } @@ -1520,9 +1546,9 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, WS_CHANNEL_ID_SELF); eof = wolfSSH_ChannelGetEof(current); if (eof) { - /* SSH is done, kill off child process */ - kill(childPid, SIGKILL); - break; + /* SSH is done, close stdin pipe to child process */ + close(stdinPipe[1]); + stdinPipe[1] = -1; } } } @@ -1705,8 +1731,12 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, if (readSz > 0) { wolfSSH_extended_data_send(ssh, shellBuffer, readSz); } + close(stdoutPipe[0]); close(stderrPipe[0]); + if (stdinPipe[1] != -1) { + close(stdinPipe[1]); + } } (void)conn; From 385921393665269cd0bdc5350aaaa4cdaf222fe1 Mon Sep 17 00:00:00 2001 From: JacobBarthelmeh Date: Wed, 19 Feb 2025 12:00:38 -0700 Subject: [PATCH 4/4] use childFd when not handling a forced command --- apps/wolfsshd/wolfsshd.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/apps/wolfsshd/wolfsshd.c b/apps/wolfsshd/wolfsshd.c index c76826f5..1a6b7635 100644 --- a/apps/wolfsshd/wolfsshd.c +++ b/apps/wolfsshd/wolfsshd.c @@ -1511,8 +1511,14 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, if (cnt_r <= 0) break; - cnt_w = (int)write(stdinPipe[1], channelBuffer, - cnt_r); + if (forcedCmd) { + cnt_w = (int)write(stdinPipe[1], channelBuffer, + cnt_r); + } + else { + cnt_w = (int)write(childFd, channelBuffer, + cnt_r); + } if (cnt_w <= 0) break; } @@ -1545,7 +1551,7 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, current = wolfSSH_ChannelFind(ssh, lastChannel, WS_CHANNEL_ID_SELF); eof = wolfSSH_ChannelGetEof(current); - if (eof) { + if (eof && forcedCmd) { /* SSH is done, close stdin pipe to child process */ close(stdinPipe[1]); stdinPipe[1] = -1;