diff --git a/apps/wolfssh/common.c b/apps/wolfssh/common.c index 283708cc..e8e347a4 100644 --- a/apps/wolfssh/common.c +++ b/apps/wolfssh/common.c @@ -45,10 +45,12 @@ static byte* userPublicKey = userPublicKeyBuf; static const byte* userPublicKeyType = NULL; static byte userPassword[256]; static const byte* userPrivateKeyType = NULL; +static byte userPublicKeyAlloc = 0; static word32 userPublicKeySz = 0; static byte pubKeyLoaded = 0; /* was a public key loaded */ static byte userPrivateKeyBuf[1191]; static byte* userPrivateKey = userPrivateKeyBuf; +static byte userPrivateKeyAlloc = 0; static word32 userPublicKeyTypeSz = 0; static word32 userPrivateKeySz = sizeof(userPrivateKeyBuf); static word32 userPrivateKeyTypeSz = 0; @@ -670,6 +672,13 @@ int ClientUseCert(const char* certName) userPublicKeyType = publicKeyType; userPublicKeyTypeSz = (word32)WSTRLEN((const char*)publicKeyType); pubKeyLoaded = 1; + userPublicKeyAlloc = 1; + } + else { + userPublicKey = userPublicKeyBuf; + userPublicKeySz = 0; + userPublicKeyType = NULL; + userPublicKeyAlloc = 0; } #else fprintf(stderr, "Certificate support not compiled in"); @@ -687,12 +696,22 @@ int ClientSetPrivateKey(const char* privKeyName) { int ret; + userPrivateKeyAlloc = 0; userPrivateKey = NULL; /* create new buffer based on parsed input */ ret = wolfSSH_ReadKey_file(privKeyName, (byte**)&userPrivateKey, &userPrivateKeySz, (const byte**)&userPrivateKeyType, &userPrivateKeyTypeSz, &isPrivate, NULL); + if (ret == 0) { + userPrivateKeyAlloc = 1; + } + else { + userPrivateKey = userPrivateKeyBuf; + userPrivateKeySz = sizeof(userPrivateKeyBuf); + userPrivateKeyType = NULL; + } + return ret; } @@ -703,6 +722,7 @@ int ClientUsePubKey(const char* pubKeyName) { int ret; + userPublicKeyAlloc = 0; userPublicKey = NULL; /* create new buffer based on parsed input */ ret = wolfSSH_ReadKey_file(pubKeyName, &userPublicKey, &userPublicKeySz, @@ -711,6 +731,11 @@ int ClientUsePubKey(const char* pubKeyName) if (ret == 0) { pubKeyLoaded = 1; + userPublicKeyAlloc = 1; + } + else { + userPublicKey = userPublicKeyBuf; + userPublicKeySz = 0; } return ret; @@ -747,11 +772,17 @@ int ClientLoadCA(WOLFSSH_CTX* ctx, const char* caCert) void ClientFreeBuffers(void) { - if (userPublicKey != userPublicKeyBuf) { + if (userPublicKeyAlloc && userPublicKey != NULL) { WFREE(userPublicKey, NULL, DYNTYPE_PRIVKEY); + userPublicKey = userPublicKeyBuf; + userPublicKeySz = 0; + userPublicKeyAlloc = 0; } - if (userPrivateKey != userPrivateKeyBuf) { + if (userPrivateKeyAlloc && userPrivateKey != NULL) { WFREE(userPrivateKey, NULL, DYNTYPE_PRIVKEY); + userPrivateKey = userPrivateKeyBuf; + userPrivateKeySz = sizeof(userPrivateKeyBuf); + userPrivateKeyAlloc = 0; } } diff --git a/apps/wolfssh/wolfssh.c b/apps/wolfssh/wolfssh.c index dc2d83bc..ca405d41 100644 --- a/apps/wolfssh/wolfssh.c +++ b/apps/wolfssh/wolfssh.c @@ -220,6 +220,7 @@ typedef struct thread_args { wolfSSL_Mutex lock; byte rawMode; byte quit; + int readError; } thread_args; #ifdef _POSIX_THREADS @@ -390,6 +391,7 @@ static THREAD_RET readPeer(void* in) int bufSz = sizeof(buf); thread_args* args = (thread_args*)in; int ret = 0; + int stop = 0; int fd = wolfSSH_get_fd(args->ssh); word32 bytes; #ifdef USE_WINDOWS_API @@ -398,11 +400,6 @@ static THREAD_RET readPeer(void* in) fd_set readSet; fd_set errSet; - FD_ZERO(&readSet); - FD_ZERO(&errSet); - FD_SET(fd, &readSet); - FD_SET(fd, &errSet); - #ifdef USE_WINDOWS_API if (args->rawMode == 0) { DWORD wrd; @@ -431,9 +428,13 @@ static THREAD_RET readPeer(void* in) #endif while (ret >= 0) { - #if defined(WOLFSSH_TERM) && defined(USE_WINDOWS_API) +#if defined(WOLFSSH_TERM) && defined(USE_WINDOWS_API) (void)windowMonitor(args); - #endif +#endif + FD_ZERO(&readSet); + FD_ZERO(&errSet); + FD_SET(fd, &readSet); + FD_SET(fd, &errSet); bytes = select(fd + 1, &readSet, NULL, &errSet, NULL); wc_LockMutex(&args->lock); @@ -458,18 +459,18 @@ static THREAD_RET readPeer(void* in) } while (ret > 0); } else if (ret <= 0) { - if (ret == WS_FATAL_ERROR) { - ret = wolfSSH_get_error(args->ssh); - if (ret == WS_WANT_READ) { - continue; - } - #ifdef WOLFSSH_AGENT - else if (ret == WS_CHAN_RXD) { - byte agentBuf[512]; - int rxd, txd; - word32 channel = 0; + int err = (ret == WS_FATAL_ERROR) ? + wolfSSH_get_error(args->ssh) : ret; + if (err == WS_WANT_READ) { + bytes = 0; + } +#ifdef WOLFSSH_AGENT + else if (err == WS_CHAN_RXD) { + byte agentBuf[512]; + int rxd, txd; + word32 channel = 0; - wolfSSH_GetLastRxId(args->ssh, &channel); + wolfSSH_GetLastRxId(args->ssh, &channel); rxd = wolfSSH_ChannelIdRead(args->ssh, channel, agentBuf, sizeof(agentBuf)); if (rxd > 4) { @@ -495,9 +496,17 @@ static THREAD_RET readPeer(void* in) WMEMSET(agentBuf, 0, sizeof(agentBuf)); continue; } - #endif /* WOLFSSH_AGENT */ +#endif /* WOLFSSH_AGENT */ + else if (err == WS_CBIO_ERR_CONN_CLOSE || + err == WS_SOCKET_ERROR_E || + err == WS_MSGID_NOT_ALLOWED_E) { + args->readError = err; + ret = err; + stop = 1; + bytes = 0; } - else if (ret != WS_EOF) { + else if (err != WS_EOF) { + wc_UnLockMutex(&args->lock); err_sys("Stream read failed."); } } @@ -517,12 +526,16 @@ static THREAD_RET readPeer(void* in) } #endif } - ret = wolfSSH_stream_peek(args->ssh, buf, bufSz); - if (ret <= 0) { - bytes = 0; /* read it all */ + if (!stop) { + ret = wolfSSH_stream_peek(args->ssh, buf, bufSz); + if (ret <= 0) { + bytes = 0; /* read it all */ + } } } wc_UnLockMutex(&args->lock); + if (stop) + break; } #if !defined(WOLFSSH_NO_ECC) && defined(FP_ECC) && defined(HAVE_THREAD_LS) wc_ecc_fp_free(); /* free per thread cache */ @@ -791,7 +804,8 @@ static int config_parse_command_line(struct config* config, if (found != NULL) { *found = '\0'; if (config->user) { - free(config->user); + WFREE(config->user, NULL, 0); + config->user = NULL; } sz = WSTRLEN(cursor); config->user = (char*)WMALLOC(sz + 1, NULL, 0); @@ -818,7 +832,7 @@ static int config_parse_command_line(struct config* config, strcpy(config->hostname, cursor); } - free(dest); + WFREE(dest, NULL, 0); myoptind++; } @@ -874,18 +888,23 @@ static int config_cleanup(struct config* config) { if (config->user) { WFREE(config->user, NULL, 0); + config->user = NULL; } if (config->hostname) { WFREE(config->hostname, NULL, 0); + config->hostname = NULL; } if (config->keyFile) { WFREE(config->keyFile, NULL, 0); + config->keyFile = NULL; } if (config->pubKeyFile) { WFREE(config->pubKeyFile, NULL, 0); + config->pubKeyFile = NULL; } if (config->command) { WFREE(config->command, NULL, 0); + config->command = NULL; } return 0; @@ -900,6 +919,7 @@ static THREAD_RETURN WOLFSSH_THREAD wolfSSH_Client(void* args) SOCKADDR_IN_T clientAddr; socklen_t clientAddrSz = sizeof(clientAddr); int ret = 0; + int ioErr = 0; byte keepOpen = 1; #ifdef USE_WINDOWS_API byte rawMode = 0; @@ -1037,6 +1057,7 @@ static THREAD_RETURN WOLFSSH_THREAD wolfSSH_Client(void* args) wc_InitMutex(&arg.lock); arg.ssh = ssh; + arg.readError = 0; #ifdef WOLFSSH_TERM arg.quit = 0; #if (defined(__OSX__) || defined(__APPLE__)) @@ -1082,12 +1103,14 @@ static THREAD_RETURN WOLFSSH_THREAD wolfSSH_Client(void* args) sem_destroy(&windowSem); #endif #endif /* WOLFSSH_TERM */ + ioErr = arg.readError; #elif defined(_MSC_VER) thread_args arg; HANDLE thread[2]; arg.ssh = ssh; arg.rawMode = rawMode; + arg.readError = 0; wc_InitMutex(&arg.lock); if (config.command) { @@ -1107,6 +1130,7 @@ static THREAD_RETURN WOLFSSH_THREAD wolfSSH_Client(void* args) WaitForSingleObject(thread[1], INFINITE); CloseHandle(thread[0]); CloseHandle(thread[1]); + ioErr = arg.readError; #else err_sys("No threading to use"); #endif @@ -1139,10 +1163,13 @@ static THREAD_RETURN WOLFSSH_THREAD wolfSSH_Client(void* args) #if defined(WOLFSSH_TERM) || defined(WOLFSSH_SHELL) ((func_args*)args)->return_code = wolfSSH_GetExitStatus(ssh); #endif + if (ioErr != 0 && ((func_args*)args)->return_code == 0) { + ((func_args*)args)->return_code = 1; + } wolfSSH_free(ssh); wolfSSH_CTX_free(ctx); - if (ret != WS_SUCCESS && ret != WS_SOCKET_ERROR_E) { + if ((ret != WS_SUCCESS && ret != WS_SOCKET_ERROR_E) || ioErr != 0) { WLOG(WS_LOG_DEBUG, "Closing client stream failed"); #if defined(WOLFSSH_TERM) || defined(WOLFSSH_SHELL) /* override return value, do not want to return success if connection diff --git a/src/internal.c b/src/internal.c index dfadcede..5a162f3c 100644 --- a/src/internal.c +++ b/src/internal.c @@ -691,6 +691,8 @@ INLINE static int IsMessageAllowedClient(WOLFSSH *ssh, byte msg) /* Error if expecting a specific message and didn't receive. */ if (ssh->handshake && ssh->handshake->expectMsgId != MSGID_NONE) { + /* The explicit expectMsgId check supersedes the old + * IsMessageAllowedKeying() stub for rekey filtering. */ if (msg != ssh->handshake->expectMsgId) { WLOG(WS_LOG_DEBUG, "Message ID %u not the expected message %u", diff --git a/tests/include.am b/tests/include.am index a9aed41c..644b4af5 100644 --- a/tests/include.am +++ b/tests/include.am @@ -45,7 +45,8 @@ tests_kex_test_CPPFLAGS = -DNO_MAIN_DRIVER $(AM_CPPFLAGS) tests_kex_test_LDADD = src/libwolfssh.la tests_kex_test_DEPENDENCIES = src/libwolfssh.la -tests_regress_test_SOURCES = tests/regress.c +tests_regress_test_SOURCES = tests/regress.c apps/wolfssh/common.c \ + apps/wolfssh/common.h tests_regress_test_CPPFLAGS = -DNO_MAIN_DRIVER -DWOLFSSH_TEST_INTERNAL $(AM_CPPFLAGS) tests_regress_test_LDADD = src/libwolfssh_test.la tests_regress_test_DEPENDENCIES = src/libwolfssh_test.la diff --git a/tests/regress.c b/tests/regress.c index fbd6032e..0fe4218f 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -19,10 +19,13 @@ #include #include #include +#include +#include #include #include #include +#include "apps/wolfssh/common.h" #ifndef WOLFSSH_NO_ABORT #define WABORT() abort() @@ -362,6 +365,44 @@ static void TestKexInitRejectedWhenKeying(WOLFSSH* ssh) AssertFalse(allowed); } +/* Ensure client buffer cleanup tolerates multiple invocations after allocs. */ +static void TestClientBuffersIdempotent(void) +{ + int ret; + + ret = ClientUsePubKey("keys/gretel-key-rsa.pub"); + AssertIntEQ(ret, 0); + ret = ClientSetPrivateKey("keys/gretel-key-rsa.pem"); + AssertIntEQ(ret, 0); + + ClientFreeBuffers(); + /* Should be safe to call again without double free. */ + ClientFreeBuffers(); +} + +/* Simulate Ctrl+D (stdin EOF) during password prompt; expect failure but no crash. */ +static void TestPasswordEofNoCrash(void) +{ + WS_UserAuthData auth; + int savedStdin, devNull, ret; + + WMEMSET(&auth, 0, sizeof(auth)); + + savedStdin = dup(STDIN_FILENO); + devNull = open("/dev/null", O_RDONLY); + AssertTrue(devNull >= 0); + AssertTrue(dup2(devNull, STDIN_FILENO) >= 0); + + ret = ClientUserAuth(WOLFSSH_USERAUTH_PASSWORD, &auth, NULL); + AssertIntEQ(ret, WOLFSSH_USERAUTH_FAILURE); + + close(devNull); + dup2(savedStdin, STDIN_FILENO); + close(savedStdin); + + ClientFreeBuffers(); +} + int main(int argc, char** argv) { @@ -390,6 +431,12 @@ int main(int argc, char** argv) TestChannelBlockedBeforeAuth(ssh); TestChannelAllowedAfterAuth(ssh); TestKexInitRejectedWhenKeying(ssh); + TestClientBuffersIdempotent(); + TestPasswordEofNoCrash(); + + /* TODO: add app-level regressions that simulate stdin EOF/password + * prompts and mid-session socket closes once the test harness can + * drive the wolfssh client without real sockets/tty. */ ResetSession(ssh); wolfSSH_free(ssh);