From cc83fc3543feeae08252dbbca57489bcc73f5b16 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Mon, 14 Jul 2025 15:37:13 -0700 Subject: [PATCH 1/5] Coverity: String not null terminated 1. Swap out strdup() for a malloc() and memcpy(). Then nul terminate the string before tokenizing. Fixes CID: 572834 --- src/ssh.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/ssh.c b/src/ssh.c index 33aff33ac..fb13d134b 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -1658,9 +1658,13 @@ static int DoSshPubKey(const byte* in, word32 inSz, byte** out, /* SSH format is: type AAAABASE64ENCODEDKEYDATA comment + + allocate a copy to tokenize, add a null terminator. */ - c = WSTRDUP((const char*)in, heap, DYNTYPE_STRING); + c = (char*)WMALLOC(inSz + 1, heap, DYNTYPE_STRING); if (c != NULL) { + WMEMCPY(c, in, inSz); + c[inSz-1] = 0; type = WSTRTOK(c, " \n", &last); key = WSTRTOK(NULL, " \n", &last); } From f24c3c262c35c016a6cd1d31b3b1070ab0eb5b79 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Mon, 14 Jul 2025 15:46:03 -0700 Subject: [PATCH 2/5] Coverity: Dereference before null check 1. When cleaning the path, check that the path pointer is not null before using it. 2. Move the strlen of path. 3. Remove the second check of path, and just loop over it. Fixed CID: 572847 --- examples/sftpclient/sftpclient.c | 82 +++++++++++++++++--------------- 1 file changed, 43 insertions(+), 39 deletions(-) diff --git a/examples/sftpclient/sftpclient.c b/examples/sftpclient/sftpclient.c index c1baba675..d4b97099a 100644 --- a/examples/sftpclient/sftpclient.c +++ b/examples/sftpclient/sftpclient.c @@ -249,9 +249,15 @@ static void sig_handler(const int sig) static void clean_path(char* path) { int i; - long sz = (long)WSTRLEN(path); + long sz; byte found; + if (path == NULL) { + return; + } + + sz = (long)WSTRLEN(path); + /* remove any double '/' chars */ for (i = 0; i < sz; i++) { if (path[i] == '/' && path[i+1] == '/') { @@ -272,51 +278,49 @@ static void clean_path(char* path) } } - if (path != NULL) { - /* go through path until no cases are found */ - do { - int prIdx = 0; /* begin of cut */ - int enIdx = 0; /* end of cut */ - sz = (long)WSTRLEN(path); - - found = 0; - for (i = 0; i < sz; i++) { - if (path[i] == '/') { - int z; - - /* if next two chars are .. then delete */ - if (path[i+1] == '.' && path[i+2] == '.') { - enIdx = i + 3; - - /* start at one char before / and retrace path */ - for (z = i - 1; z > 0; z--) { - if (path[z] == '/') { - prIdx = z; - break; - } + /* go through path until no cases are found */ + do { + int prIdx = 0; /* begin of cut */ + int enIdx = 0; /* end of cut */ + sz = (long)WSTRLEN(path); + + found = 0; + for (i = 0; i < sz; i++) { + if (path[i] == '/') { + int z; + + /* if next two chars are .. then delete */ + if (path[i+1] == '.' && path[i+2] == '.') { + enIdx = i + 3; + + /* start at one char before / and retrace path */ + for (z = i - 1; z > 0; z--) { + if (path[z] == '/') { + prIdx = z; + break; } + } - /* cut out .. and previous */ - WMEMMOVE(path + prIdx, path + enIdx, sz - enIdx); - path[sz - (enIdx - prIdx)] = '\0'; - - if (enIdx == sz) { - path[prIdx] = '\0'; - } + /* cut out .. and previous */ + WMEMMOVE(path + prIdx, path + enIdx, sz - enIdx); + path[sz - (enIdx - prIdx)] = '\0'; - /* case of at / */ - if (WSTRLEN(path) == 0) { - path[0] = '/'; - path[1] = '\0'; - } + if (enIdx == sz) { + path[prIdx] = '\0'; + } - found = 1; - break; + /* case of at / */ + if (WSTRLEN(path) == 0) { + path[0] = '/'; + path[1] = '\0'; } + + found = 1; + break; } } - } while (found); - } + } + } while (found); } #define WS_MAX_EXAMPLE_RW 1024 From 4700799fccc68a33e49b4fdfe176a727199d6222 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Mon, 14 Jul 2025 15:48:30 -0700 Subject: [PATCH 3/5] Coverity: Dereference before null check 1. After getting the user's pw info, don't check that the shell value is null. We've already use it at that point. Fixes CID: 572919 --- apps/wolfsshd/wolfsshd.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/apps/wolfsshd/wolfsshd.c b/apps/wolfsshd/wolfsshd.c index 8b235f0ab..0b1eb1e69 100644 --- a/apps/wolfsshd/wolfsshd.c +++ b/apps/wolfsshd/wolfsshd.c @@ -1347,20 +1347,18 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, setenv("LOGNAME", pPasswd->pw_name, 1); setenv("SHELL", pPasswd->pw_shell, 1); - if (pPasswd->pw_shell) { - if (WSTRLEN(pPasswd->pw_shell) < sizeof(shell)) { - char* cursor; - char* start; - - WSTRNCPY(shell, pPasswd->pw_shell, sizeof(shell)); - cursor = shell; - do { - start = cursor; - *cursor = '-'; - cursor = WSTRCHR(start, '/'); - } while (cursor && *cursor != '\0'); - args[0] = start; - } + if (WSTRLEN(pPasswd->pw_shell) < sizeof(shell)) { + char* cursor; + char* start; + + WSTRNCPY(shell, pPasswd->pw_shell, sizeof(shell)); + cursor = shell; + do { + start = cursor; + *cursor = '-'; + cursor = WSTRCHR(start, '/'); + } while (cursor && *cursor != '\0'); + args[0] = start; } rc = chdir(pPasswd->pw_dir); From c641294fc3342703dff681eb68e4cdea5a50560a Mon Sep 17 00:00:00 2001 From: John Safranek Date: Mon, 14 Jul 2025 15:54:01 -0700 Subject: [PATCH 4/5] Coverity: Buffer not null terminated 1. When copying the shell, leave a byte free in the dest buffer. Fill it with a null. Fixes CID: 572891 --- apps/wolfsshd/wolfsshd.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/apps/wolfsshd/wolfsshd.c b/apps/wolfsshd/wolfsshd.c index 0b1eb1e69..a6eb6816e 100644 --- a/apps/wolfsshd/wolfsshd.c +++ b/apps/wolfsshd/wolfsshd.c @@ -1351,7 +1351,8 @@ static int SHELL_Subsystem(WOLFSSHD_CONNECTION* conn, WOLFSSH* ssh, char* cursor; char* start; - WSTRNCPY(shell, pPasswd->pw_shell, sizeof(shell)); + WSTRNCPY(shell, pPasswd->pw_shell, sizeof(shell)-1); + shell[sizeof(shell)-1] = 0; cursor = shell; do { start = cursor; From 903bbc7fb2f48f780677ce90b2924ee2f201ed05 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Mon, 21 Jul 2025 09:36:35 -0700 Subject: [PATCH 5/5] Coverity: Argument cannot be negative 1. Due to not checking the result of fseek(), it is possible to try to malloc() -1 bytes of storage. Checking the return from fseek() and erring if negative. 2. Changing the check between the result of fseek() and fread() to match signedness. Adding some casting, as at that point the fseek() result is always positive. Fixes CIDs: 573009 572928 572868 --- apps/wolfssh/common.c | 10 +++++----- apps/wolfsshd/wolfsshd.c | 10 +++++++--- examples/client/common.c | 13 ++++++------- tests/api.c | 32 ++++++++++++++++++++++---------- 4 files changed, 40 insertions(+), 25 deletions(-) diff --git a/apps/wolfssh/common.c b/apps/wolfssh/common.c index bdb80cff5..283708cc1 100644 --- a/apps/wolfssh/common.c +++ b/apps/wolfssh/common.c @@ -76,7 +76,7 @@ static int load_der_file(const char* filename, byte** out, word32* outSz) { WFILE* file; byte* in; - word32 inSz; + long inSz; int ret; if (filename == NULL || out == NULL || outSz == NULL) @@ -90,10 +90,10 @@ static int load_der_file(const char* filename, byte** out, word32* outSz) WFCLOSE(NULL, file); return -1; } - inSz = (word32)WFTELL(NULL, file); + inSz = WFTELL(NULL, file); WREWIND(NULL, file); - if (inSz == 0) { + if (inSz <= 0) { WFCLOSE(NULL, file); return -1; } @@ -105,7 +105,7 @@ static int load_der_file(const char* filename, byte** out, word32* outSz) } ret = (int)WFREAD(NULL, in, 1, inSz, file); - if (ret <= 0 || (word32)ret != inSz) { + if (ret <= 0 || ret != inSz) { ret = -1; WFREE(in, NULL, 0); in = 0; @@ -115,7 +115,7 @@ static int load_der_file(const char* filename, byte** out, word32* outSz) ret = 0; *out = in; - *outSz = inSz; + *outSz = (word32)inSz; WFCLOSE(NULL, file); diff --git a/apps/wolfsshd/wolfsshd.c b/apps/wolfsshd/wolfsshd.c index a6eb6816e..de6b14da2 100644 --- a/apps/wolfsshd/wolfsshd.c +++ b/apps/wolfsshd/wolfsshd.c @@ -242,7 +242,7 @@ static byte* getBufferFromFile(const char* fileName, word32* bufSz, void* heap) { FILE* file; byte* buf = NULL; - word32 fileSz; + long fileSz; word32 readSz; WOLFSSH_UNUSED(heap); @@ -252,13 +252,17 @@ static byte* getBufferFromFile(const char* fileName, word32* bufSz, void* heap) if (WFOPEN(NULL, &file, fileName, "rb") != 0) return NULL; WFSEEK(NULL, file, 0, WSEEK_END); - fileSz = (word32)WFTELL(NULL, file); + fileSz = WFTELL(NULL, file); + if (fileSz < 0) { + WFCLOSE(NULL, file); + return NULL; + } WREWIND(NULL, file); buf = (byte*)WMALLOC(fileSz + 1, heap, DYNTYPE_SSHD); if (buf != NULL) { readSz = (word32)WFREAD(NULL, buf, 1, fileSz, file); - if (readSz < fileSz) { + if (readSz < (size_t)fileSz) { WFCLOSE(NULL, file); WFREE(buf, heap, DYNTYPE_SSHD); return NULL; diff --git a/examples/client/common.c b/examples/client/common.c index 12f02e52e..92682955d 100644 --- a/examples/client/common.c +++ b/examples/client/common.c @@ -262,7 +262,7 @@ static int load_der_file(const char* filename, byte** out, word32* outSz, { WFILE* file; byte* in; - word32 inSz; + long inSz; int ret; if (filename == NULL || out == NULL || outSz == NULL) @@ -276,13 +276,12 @@ static int load_der_file(const char* filename, byte** out, word32* outSz, WFCLOSE(NULL, file); return -1; } - inSz = (word32)WFTELL(NULL, file); - WREWIND(NULL, file); - - if (inSz == 0) { + inSz = WFTELL(NULL, file); + if (inSz <= 0) { WFCLOSE(NULL, file); return -1; } + WREWIND(NULL, file); in = (byte*)WMALLOC(inSz, heap, 0); if (in == NULL) { @@ -291,7 +290,7 @@ static int load_der_file(const char* filename, byte** out, word32* outSz, } ret = (int)WFREAD(NULL, in, 1, inSz, file); - if (ret <= 0 || (word32)ret != inSz) { + if (ret <= 0 || ret != inSz) { ret = -1; WFREE(in, heap, 0); in = 0; @@ -301,7 +300,7 @@ static int load_der_file(const char* filename, byte** out, word32* outSz, ret = 0; *out = in; - *outSz = inSz; + *outSz = (word32)inSz; WFCLOSE(NULL, file); diff --git a/tests/api.c b/tests/api.c index 3f060618e..2bef34998 100644 --- a/tests/api.c +++ b/tests/api.c @@ -504,26 +504,38 @@ static int load_file(const char* filename, byte** buf, word32* bufSz) } if (ret == 0) { - fseek(f, 0, XSEEK_END); - *bufSz = (word32)ftell(f); - rewind(f); + ret = fseek(f, 0, XSEEK_END); + if (ret < 0) + ret = -3; + } + + if (ret == 0) { + long sz = ftell(f); + if (sz < 0) + ret = -4; + else + *bufSz = (word32)sz; } if (ret == 0) { + rewind(f); *buf = (byte*)malloc(*bufSz); if (*buf == NULL) - ret = -3; + ret = -5; } if (ret == 0) { - int readSz; - readSz = (int)fread(*buf, 1, *bufSz, f); - if (readSz < (int)*bufSz) - ret = -4; + size_t readSz; + readSz = fread(*buf, 1, *bufSz, f); + if (readSz < *bufSz) + ret = -6; } - if (f != NULL) - fclose(f); + if (f != NULL) { + ret = fclose(f); + if (ret < 0) + ret = -7; + } return ret; }