From 92cfb2e0520415223d2c0703cbe6ab440c29caba Mon Sep 17 00:00:00 2001 From: John Safranek Date: Fri, 11 Jul 2025 09:24:10 -0700 Subject: [PATCH 1/3] Coverity: Side effect in assertion 1. Modify AssertNotNull() to use the same pattern as AssertNull(). The macro assigns the pointer to a local variable and that is checked. Fixes CIDs: 537020 573008 573010 573011 573013 573014 573015 573016 573017 573018 573022 573023 573024 --- tests/api.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/tests/api.c b/tests/api.c index cc78f8019..ad40bc92f 100644 --- a/tests/api.c +++ b/tests/api.c @@ -85,12 +85,15 @@ char* myoptarg = NULL; #define AssertTrue(x) Assert( (x), ("%s is true", #x), (#x " => FALSE")) #define AssertFalse(x) Assert(!(x), ("%s is false", #x), (#x " => TRUE")) -#define AssertNotNull(x) Assert( (x), ("%s is not null", #x), (#x " => NULL")) + +#define AssertNotNull(x) do { \ + PEDANTIC_EXTENSION void* _isNotNull = (void*)(x); \ + Assert(_isNotNull, ("%s is not null", #x), (#x " => NULL")); \ +} while (0) #define AssertNull(x) do { \ - PEDANTIC_EXTENSION void* _x = (void*)(x); \ - \ - Assert(!_x, ("%s is null", #x), (#x " => %p", _x)); \ + PEDANTIC_EXTENSION void* _isNull = (void*)(x); \ + Assert(!_isNull, ("%s is null", #x), (#x " => %p", _isNull)); \ } while(0) #define AssertInt(x, y, op, er) do { \ From 538ae1531083a280f85709dc48f06056379a86d1 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Fri, 11 Jul 2025 12:02:08 -0700 Subject: [PATCH 2/3] Coverity: Resource leak 1. Fix some resource leaks during error conditions where a socket or a file descriptor doesn't get closed in all error cases. 2. In wolfSSH_SFTP_RecvOpen(), initialize the file descriptor. 3. For 572902, the error case resource leaks are fixed. There's still an issue to resolve for storing the FD for use later. Fixes CIDs: 572856 572902* 573012 573019 573021 573076 --- apps/wolfsshd/wolfsshd.c | 5 +++-- src/wolfsftp.c | 25 +++++++++++++++++++------ tests/api.c | 20 ++++++++++++++++++++ tests/auth.c | 26 ++++++++++++++++++++++---- 4 files changed, 64 insertions(+), 12 deletions(-) diff --git a/apps/wolfsshd/wolfsshd.c b/apps/wolfsshd/wolfsshd.c index 1a6b76354..8b235f0ab 100644 --- a/apps/wolfsshd/wolfsshd.c +++ b/apps/wolfsshd/wolfsshd.c @@ -245,6 +245,8 @@ static byte* getBufferFromFile(const char* fileName, word32* bufSz, void* heap) word32 fileSz; word32 readSz; + WOLFSSH_UNUSED(heap); + if (fileName == NULL) return NULL; if (WFOPEN(NULL, &file, fileName, "rb") != 0) @@ -263,10 +265,9 @@ static byte* getBufferFromFile(const char* fileName, word32* bufSz, void* heap) } if (bufSz) *bufSz = readSz; - WFCLOSE(NULL, file); } + WFCLOSE(NULL, file); - (void)heap; return buf; } #endif /* NO_FILESYSTEM */ diff --git a/src/wolfsftp.c b/src/wolfsftp.c index cf434cda7..bfebc165c 100644 --- a/src/wolfsftp.c +++ b/src/wolfsftp.c @@ -1997,6 +1997,12 @@ int wolfSSH_SFTP_RecvOpen(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz) WLOG(WS_LOG_SFTP, "Receiving WOLFSSH_FTP_OPEN"); + #ifdef MICROCHIP_MPLAB_HARMONY + fd = WBADFILE; + #else + fd = -1; + #endif + if (sizeof(WFD) > WOLFSSH_MAX_HANDLE) { WLOG(WS_LOG_SFTP, "Handle size is too large"); return WS_FATAL_ERROR; @@ -2083,7 +2089,6 @@ int wolfSSH_SFTP_RecvOpen(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz) } #ifdef MICROCHIP_MPLAB_HARMONY - fd = WBADFILE; { WFILE* f = &fd; if (WFOPEN(ssh->fs, &f, dir, m) != WS_SUCCESS) { @@ -2111,7 +2116,8 @@ int wolfSSH_SFTP_RecvOpen(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz) WLOG(WS_LOG_SFTP, "Unable to store handle"); res = ier; if (wolfSSH_SFTP_CreateStatus(ssh, WOLFSSH_FTP_FAILURE, reqId, res, - "English", NULL, &outSz) != WS_SIZE_ONLY) { + "English", NULL, &outSz) != WS_SIZE_ONLY) { + WCLOSE(ssh->fs, fd); return WS_FATAL_ERROR; } ret = WS_FATAL_ERROR; @@ -2119,14 +2125,18 @@ int wolfSSH_SFTP_RecvOpen(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz) } #endif - /* create packet */ - out = (byte*)WMALLOC(outSz, ssh->ctx->heap, DYNTYPE_BUFFER); - if (out == NULL) { - return WS_MEMORY_E; + if (ret == WS_SUCCESS) { + /* create packet */ + out = (byte*)WMALLOC(outSz, ssh->ctx->heap, DYNTYPE_BUFFER); + if (out == NULL) { + WCLOSE(ssh->fs, fd); + return WS_MEMORY_E; + } } if (ret == WS_SUCCESS) { if (SFTP_CreatePacket(ssh, WOLFSSH_FTP_HANDLE, out, outSz, (byte*)&fd, sizeof(WFD)) != WS_SUCCESS) { + WCLOSE(ssh->fs, fd); return WS_FATAL_ERROR; } } @@ -2134,6 +2144,9 @@ int wolfSSH_SFTP_RecvOpen(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz) if (wolfSSH_SFTP_CreateStatus(ssh, WOLFSSH_FTP_FAILURE, reqId, res, "English", out, &outSz) != WS_SUCCESS) { WFREE(out, ssh->ctx->heap, DYNTYPE_BUFFER); + if (fd >= 0) { + WCLOSE(ssh->fs, fd); + } return WS_FATAL_ERROR; } } diff --git a/tests/api.c b/tests/api.c index ad40bc92f..3f060618e 100644 --- a/tests/api.c +++ b/tests/api.c @@ -927,8 +927,17 @@ static void sftp_client_connect(WOLFSSH_CTX** ctx, WOLFSSH** ssh, int port) build_addr(&clientAddr, host, port); tcp_socket(&sockFd, ((struct sockaddr_in *)&clientAddr)->sin_family); + if (sockFd < 0) { + wolfSSH_free(*ssh); + wolfSSH_CTX_free(*ctx); + *ctx = NULL; + *ssh = NULL; + return; + } + ret = connect(sockFd, (const struct sockaddr *)&clientAddr, clientAddrSz); if (ret != 0){ + WCLOSESOCKET(sockFd); wolfSSH_free(*ssh); wolfSSH_CTX_free(*ctx); *ctx = NULL; @@ -945,6 +954,7 @@ static void sftp_client_connect(WOLFSSH_CTX** ctx, WOLFSSH** ssh, int port) ret = wolfSSH_SFTP_connect(*ssh); if (ret != WS_SUCCESS){ + WCLOSESOCKET(sockFd); wolfSSH_free(*ssh); wolfSSH_CTX_free(*ctx); *ctx = NULL; @@ -1611,8 +1621,17 @@ static void keyboard_client_connect(WOLFSSH_CTX** ctx, WOLFSSH** ssh, int port) build_addr(&clientAddr, host, port); tcp_socket(&sockFd, ((struct sockaddr_in *)&clientAddr)->sin_family); + if (sockFd < 0) { + wolfSSH_free(*ssh); + wolfSSH_CTX_free(*ctx); + *ctx = NULL; + *ssh = NULL; + return; + } + ret = connect(sockFd, (const struct sockaddr *)&clientAddr, clientAddrSz); if (ret != 0){ + WCLOSESOCKET(sockFd); wolfSSH_free(*ssh); wolfSSH_CTX_free(*ctx); *ctx = NULL; @@ -1628,6 +1647,7 @@ static void keyboard_client_connect(WOLFSSH_CTX** ctx, WOLFSSH** ssh, int port) ret = wolfSSH_connect(*ssh); if (ret != WS_SUCCESS){ + WCLOSESOCKET(sockFd); wolfSSH_free(*ssh); wolfSSH_CTX_free(*ctx); *ctx = NULL; diff --git a/tests/auth.c b/tests/auth.c index cf57a3c16..ac3a5a545 100644 --- a/tests/auth.c +++ b/tests/auth.c @@ -443,15 +443,33 @@ static int basic_client_connect(WOLFSSH_CTX** ctx, WOLFSSH** ssh, int port) wolfSSH_CTX_free(*ctx); *ctx = NULL; *ssh = NULL; + WCLOSESOCKET(sockFd); return ret; } ret = wolfSSH_SetUsername(*ssh, username); - if (ret == WS_SUCCESS) - ret = wolfSSH_set_fd(*ssh, (int)sockFd); + if (ret != WS_SUCCESS) { + wolfSSH_free(*ssh); + wolfSSH_CTX_free(*ctx); + *ctx = NULL; + *ssh = NULL; + WCLOSESOCKET(sockFd); + fprintf(stderr, "line= %d\n", __LINE__); + return ret; + } + + ret = wolfSSH_set_fd(*ssh, (int)sockFd); + if (ret != WS_SUCCESS) { + fprintf(stderr, "line= %d\n", __LINE__); + wolfSSH_free(*ssh); + wolfSSH_CTX_free(*ctx); + *ctx = NULL; + *ssh = NULL; + WCLOSESOCKET(sockFd); + return ret; + } - if (ret == WS_SUCCESS) - ret = wolfSSH_connect(*ssh); + ret = wolfSSH_connect(*ssh); return ret; } From 441f975ed6a1dd1097f7de30cd4cead85c1ea91d Mon Sep 17 00:00:00 2001 From: John Safranek Date: Fri, 11 Jul 2025 14:50:22 -0700 Subject: [PATCH 3/3] Coverity: 'Constant' variable guards dead code 1. Remove the default password from the wolfSSH client app main function. It isn't set to anything. (It was originally an example client option.) 2. Remove handling the default password from the wolfSSH client app's user authentication callback. 3. Set the password size directly. 4. Changed a void typecast to WOLFSSH_UNUSED. Fixes CID: 572898 --- apps/wolfssh/common.c | 45 ++++++++++++++++++------------------------ apps/wolfssh/wolfssh.c | 4 ---- 2 files changed, 19 insertions(+), 30 deletions(-) diff --git a/apps/wolfssh/common.c b/apps/wolfssh/common.c index 5d5a90d33..bdb80cff5 100644 --- a/apps/wolfssh/common.c +++ b/apps/wolfssh/common.c @@ -499,6 +499,8 @@ int ClientUserAuth(byte authType, { int ret = WOLFSSH_USERAUTH_SUCCESS; + WOLFSSH_UNUSED(ctx); + #ifdef DEBUG_WOLFSSH /* inspect supported types from server */ printf("Server supports:\n"); @@ -538,37 +540,28 @@ int ClientUserAuth(byte authType, ret = WOLFSSH_USERAUTH_SUCCESS; } else if (authType == WOLFSSH_USERAUTH_PASSWORD) { - const char* defaultPassword = (const char*)ctx; - word32 passwordSz = 0; - - if (defaultPassword != NULL) { - passwordSz = (word32)strlen(defaultPassword); - WMEMCPY(userPassword, defaultPassword, passwordSz); + printf("Password: "); + fflush(stdout); + ClientSetEcho(0); + if (fgets((char*)userPassword, sizeof(userPassword), stdin) == NULL) { + fprintf(stderr, "Getting password failed.\n"); + ret = WOLFSSH_USERAUTH_FAILURE; } else { - printf("Password: "); - fflush(stdout); - ClientSetEcho(0); - if (fgets((char*)userPassword, sizeof(userPassword), stdin) == NULL) { - fprintf(stderr, "Getting password failed.\n"); - ret = WOLFSSH_USERAUTH_FAILURE; - } - else { - char* c = strpbrk((char*)userPassword, "\r\n"); - if (c != NULL) - *c = '\0'; - } - passwordSz = (word32)strlen((const char*)userPassword); - ClientSetEcho(1); - #ifdef USE_WINDOWS_API - printf("\r\n"); - #endif - fflush(stdout); + char* c = strpbrk((char*)userPassword, "\r\n"); + if (c != NULL) + *c = '\0'; } + ClientSetEcho(1); + #ifdef USE_WINDOWS_API + printf("\r\n"); + #endif + fflush(stdout); if (ret == WOLFSSH_USERAUTH_SUCCESS) { authData->sf.password.password = userPassword; - authData->sf.password.passwordSz = passwordSz; + authData->sf.password.passwordSz = + (word32)strlen((const char*)userPassword); } } @@ -743,7 +736,7 @@ int ClientLoadCA(WOLFSSH_CTX* ctx, const char* caCert) WFREE(der, NULL, 0); } #else - (void)ctx; + WOLFSSH_UNUSED(ctx); fprintf(stderr, "Support for certificates not compiled in."); ret = WS_NOT_COMPILED; #endif diff --git a/apps/wolfssh/wolfssh.c b/apps/wolfssh/wolfssh.c index d85bc0246..dc2d83bc9 100644 --- a/apps/wolfssh/wolfssh.c +++ b/apps/wolfssh/wolfssh.c @@ -900,7 +900,6 @@ static THREAD_RETURN WOLFSSH_THREAD wolfSSH_Client(void* args) SOCKADDR_IN_T clientAddr; socklen_t clientAddrSz = sizeof(clientAddr); int ret = 0; - const char* password = NULL; byte keepOpen = 1; #ifdef USE_WINDOWS_API byte rawMode = 0; @@ -976,9 +975,6 @@ static THREAD_RETURN WOLFSSH_THREAD wolfSSH_Client(void* args) wolfSSH_SetGlobalReqCtx(ssh, &ssh); /* dummy ctx */ #endif - if (password != NULL) - wolfSSH_SetUserAuthCtx(ssh, (void*)password); - #ifdef WOLFSSH_AGENT if (useAgent) { WMEMSET(&agentCbCtx, 0, sizeof(agentCbCtx));