From 801ac0701d0c2ded5a64b36daeb6c29cec327e28 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Mon, 9 Mar 2026 10:13:29 -0700 Subject: [PATCH 1/6] Non-constant-time password hash comparison In wolfSSHd, the comparisons of the password hash and public keys were using memcmp(). Changed to use ConstantCompare(). Affected functions: CheckPasswordHashUnix, CheckPublicKeyUnix. Issue: F-53 --- apps/wolfsshd/auth.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/apps/wolfsshd/auth.c b/apps/wolfsshd/auth.c index 1b13d6af1..2fe00049b 100644 --- a/apps/wolfsshd/auth.c +++ b/apps/wolfsshd/auth.c @@ -338,7 +338,8 @@ static int CheckPasswordHashUnix(const char* input, char* stored) if (storedSz == 0 || stored[0] == '*' || hashedInputSz == 0 || hashedInput[0] == '*' || hashedInputSz != storedSz || - WMEMCMP(hashedInput, stored, storedSz) != 0) { + ConstantCompare((const byte*)hashedInput, + (const byte*)stored, storedSz) != 0) { ret = WSSHD_AUTH_FAILURE; } } @@ -656,7 +657,7 @@ static int CheckPublicKeyUnix(const char* name, if (rc == WS_SUCCESS) { rc = wc_Hash(WC_HASH_TYPE_SHA256, caKey, caKeySz, fingerprint, WC_SHA256_DIGEST_SIZE); - if (rc == 0 && WMEMCMP(fingerprint, pubKeyCtx->caKey, + if (rc == 0 && ConstantCompare(fingerprint, pubKeyCtx->caKey, WC_SHA256_DIGEST_SIZE) == 0) { foundKey = 1; break; From a8ad9f0197dc77ef9e3fdd6633e637d2e267b6b4 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Mon, 9 Mar 2026 10:19:59 -0700 Subject: [PATCH 2/6] DoIgnore Missing Payload Bounds Validation The DoIgnore() function was not bounds checking the ignore message. Changed it to use the GetSkip() function which does bounds checking and skips the current blob. Updated GetSkip() to allow 0 length blobs to skip. Affected function: DoIgnore. Issue: F-410 --- src/internal.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/src/internal.c b/src/internal.c index e40878584..d5fb04a43 100644 --- a/src/internal.c +++ b/src/internal.c @@ -3575,12 +3575,12 @@ int GetSkip(const byte* buf, word32 len, word32* idx) int result; word32 sz; - result = GetUint32(&sz, buf, len, idx); + result = GetSize(&sz, buf, len, idx); if (result == WS_SUCCESS) { result = WS_BUFFER_E; - if (*idx < len && sz <= len - *idx) { + if (*idx <= len && sz <= len - *idx) { *idx += sz; result = WS_SUCCESS; } @@ -6320,18 +6320,8 @@ static int DoKexDhGexGroup(WOLFSSH* ssh, static int DoIgnore(WOLFSSH* ssh, byte* buf, word32 len, word32* idx) { - word32 dataSz; - word32 begin = *idx; - WOLFSSH_UNUSED(ssh); - WOLFSSH_UNUSED(len); - - ato32(buf + begin, &dataSz); - begin += LENGTH_SZ + dataSz; - - *idx = begin; - - return WS_SUCCESS; + return GetSkip(buf, len, idx); } static int DoRequestSuccess(WOLFSSH *ssh, byte *buf, word32 len, word32 *idx) From 9cb60cdadf0e423498d47504e3d35bc5e93b187d Mon Sep 17 00:00:00 2001 From: John Safranek Date: Mon, 9 Mar 2026 11:10:29 -0700 Subject: [PATCH 3/6] DoUserAuthRequestPassword Missing Bounds Check Replace the original message parsing functions with the GetStringRef() function, which does better bounds checking. Affected function: DoUserAuthRequestPassword. Issue: F-411 --- src/internal.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/internal.c b/src/internal.c index d5fb04a43..297a3181b 100644 --- a/src/internal.c +++ b/src/internal.c @@ -6890,20 +6890,14 @@ static int DoUserAuthRequestPassword(WOLFSSH* ssh, WS_UserAuthData* authData, } if (ret == WS_SUCCESS) - ret = GetUint32(&pw->passwordSz, buf, len, &begin); + ret = GetStringRef(&pw->passwordSz, &pw->password, buf, len, &begin); if (ret == WS_SUCCESS) { - pw->password = buf + begin; - begin += pw->passwordSz; - if (pw->hasNewPassword) { /* Skip the password change. Maybe error out since we aren't * supporting password changes at this time. */ - ret = GetUint32(&pw->newPasswordSz, buf, len, &begin); - if (ret == WS_SUCCESS) { - pw->newPassword = buf + begin; - begin += pw->newPasswordSz; - } + ret = GetStringRef(&pw->newPasswordSz, &pw->newPassword, + buf, len, &begin); } else { pw->newPassword = NULL; From 6638c01bbb8764aee71231680e84932ba7d3432f Mon Sep 17 00:00:00 2001 From: John Safranek Date: Mon, 9 Mar 2026 13:46:54 -0700 Subject: [PATCH 4/6] DoServiceRequest Missing Bounds Check Replace the original message parsing functions with the GetString() function, which does better bounds checking. Affected functions: DoServiceRequest, DoServiceAccept. Issue: F-524, F-525 --- src/internal.c | 52 ++++++++++++++++---------------------------------- 1 file changed, 16 insertions(+), 36 deletions(-) diff --git a/src/internal.c b/src/internal.c index 297a3181b..4d3676957 100644 --- a/src/internal.c +++ b/src/internal.c @@ -6523,56 +6523,36 @@ static int DoDisconnect(WOLFSSH* ssh, byte* buf, word32 len, word32* idx) static int DoServiceRequest(WOLFSSH* ssh, byte* buf, word32 len, word32* idx) { - word32 begin = *idx; - word32 nameSz; - char serviceName[WOLFSSH_MAX_NAMESZ]; - - WOLFSSH_UNUSED(len); + char name[WOLFSSH_MAX_NAMESZ+1]; + word32 nameSz = sizeof(name); + int ret; - ato32(buf + begin, &nameSz); - begin += LENGTH_SZ; + ret = GetString(name, &nameSz, buf, len, idx); - if (begin + nameSz > len || nameSz >= WOLFSSH_MAX_NAMESZ) { - return WS_BUFFER_E; + if (ret == WS_SUCCESS) { + WLOG(WS_LOG_DEBUG, "Requesting service: %s", name); + ssh->clientState = CLIENT_USERAUTH_REQUEST_DONE; } - WMEMCPY(serviceName, buf + begin, nameSz); - begin += nameSz; - serviceName[nameSz] = 0; - - *idx = begin; - - WLOG(WS_LOG_DEBUG, "Requesting service: %s", serviceName); - ssh->clientState = CLIENT_USERAUTH_REQUEST_DONE; - - return WS_SUCCESS; + return ret; } static int DoServiceAccept(WOLFSSH* ssh, byte* buf, word32 len, word32* idx) { - word32 begin = *idx; - word32 nameSz; - char serviceName[WOLFSSH_MAX_NAMESZ]; + char name[WOLFSSH_MAX_NAMESZ+1]; + word32 nameSz = sizeof(name); + int ret; - ato32(buf + begin, &nameSz); - begin += LENGTH_SZ; + ret = GetString(name, &nameSz, buf, len, idx); - if (begin + nameSz > len || nameSz >= WOLFSSH_MAX_NAMESZ) { - return WS_BUFFER_E; + if (ret == WS_SUCCESS) { + WLOG(WS_LOG_DEBUG, "Accepted service: %s", name); + ssh->serverState = SERVER_USERAUTH_REQUEST_DONE; } - WMEMCPY(serviceName, buf + begin, nameSz); - begin += nameSz; - serviceName[nameSz] = 0; - - *idx = begin; - - WLOG(WS_LOG_DEBUG, "Accepted service: %s", serviceName); - ssh->serverState = SERVER_USERAUTH_REQUEST_DONE; - - return WS_SUCCESS; + return ret; } From 6a3e97d12b78a674c05f7253eb842930cbfdebaa Mon Sep 17 00:00:00 2001 From: John Safranek Date: Mon, 9 Mar 2026 14:06:22 -0700 Subject: [PATCH 5/6] PrepareUserAuthRequestEcc Missing Bounds Checks For agent ECC public key parsing, replaced parsing the data by hand with the GetSkip() and GetStringRef() functions which do bounds checking. Affected function: PrepareUserAuthRequestEcc. Issue: F-526 --- src/internal.c | 37 +++++++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/src/internal.c b/src/internal.c index 4d3676957..ca18a6e96 100644 --- a/src/internal.c +++ b/src/internal.c @@ -14400,19 +14400,32 @@ static int PrepareUserAuthRequestEcc(WOLFSSH* ssh, word32* payloadSz, word32 idx = 0; #ifdef WOLFSSH_AGENT if (ssh->agentEnabled) { - word32 sz; - const byte* c = (const byte*)authData->sf.publicKey.publicKey; + const byte* publicKey = NULL; + word32 publicKeySz; - ato32(c + idx, &sz); - idx += LENGTH_SZ + sz; - ato32(c + idx, &sz); - idx += LENGTH_SZ + sz; - ato32(c + idx, &sz); - idx += LENGTH_SZ; - c += idx; - idx = 0; - - ret = wc_ecc_import_x963(c, sz, &keySig->ks.ecc.key); + ret = GetSkip((const byte*)authData->sf.publicKey.publicKey, + authData->sf.publicKey.publicKeySz, &idx); + if (ret == WS_SUCCESS) { + ret = GetSkip((const byte*)authData->sf.publicKey.publicKey, + authData->sf.publicKey.publicKeySz, &idx); + } + if (ret == WS_SUCCESS) { + ret = GetStringRef(&publicKeySz, &publicKey, + (const byte*)authData->sf.publicKey.publicKey, + authData->sf.publicKey.publicKeySz, &idx); + } + if (ret == WS_SUCCESS) { + ret = wc_ecc_import_x963(publicKey, publicKeySz, + &keySig->ks.ecc.key); + } + if (ret != 0) { + WLOG(WS_LOG_ERROR, + "wc_ecc_import_x963 failed, ret = %d", ret); + ret = WS_ECC_E; + } + else { + ret = WS_SUCCESS; + } } else #endif From 331048d701f78ef3dcdec50188ab7ed3070ffdfb Mon Sep 17 00:00:00 2001 From: John Safranek Date: Mon, 9 Mar 2026 14:16:57 -0700 Subject: [PATCH 6/6] Unsigned variable compared < 0 When filling the screen with spaces, the code was subtracting two unsigned numbers and checking if they were negative. Changed to use a comparison and adjust the subtraction as appropriate, then did the rest of the size expansion. If the second point is before the first, set the fill length to 0. Affected function: wolfSSH_ClearScreen. Issue: F-48 --- src/wolfterm.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/wolfterm.c b/src/wolfterm.c index 5fd23bf8f..63e69d679 100644 --- a/src/wolfterm.c +++ b/src/wolfterm.c @@ -115,14 +115,15 @@ static void wolfSSH_ClearScreen(WOLFSSH_HANDLE handle, word32 x1, word32 y1, wor start.Y = y1; /* get number of cells */ - if (y1 == y2) { /* on same line so is x2 - x1 */ - fill = x2 - x1; + if (y2 == y1) { /* on same line so is x2 - x1 */ + fill = (x2 >= x1) ? (x2 - x1) : 0; } - else { /* | y1 - y2 | * maxX - x1 + x2 */ - fill = y1 - y2; - if (fill < 0) - fill += fill * 2; - fill = fill * maxX - x1 + x2; + /* (y2 - y1) * maxX - x1 + x2 */ + else if (y2 > y1) { + fill = (y2 - y1) * maxX - x1 + x2; + } + else { + fill = 0; } FillConsoleOutputCharacterA(handle, ' ', fill, start, &w);