From c0db9165a31033d3c3efbb45fdc2fc471fd69c02 Mon Sep 17 00:00:00 2001 From: Emma Stensland Date: Wed, 9 Sep 2026 13:42:26 -0600 Subject: [PATCH] internal, wolfsshd: Fix code review nits --- apps/wolfsshd/wolfsshd.c | 40 ++++++++++++------- src/internal.c | 83 +++++++++------------------------------- 2 files changed, 45 insertions(+), 78 deletions(-) diff --git a/apps/wolfsshd/wolfsshd.c b/apps/wolfsshd/wolfsshd.c index 045ac5ea..9f26a9b7 100644 --- a/apps/wolfsshd/wolfsshd.c +++ b/apps/wolfsshd/wolfsshd.c @@ -4091,18 +4091,28 @@ static void wolfSSHD_ServiceCb(DWORD CtrlCode) } -static char* _convertHelper(WCHAR* in, void* heap) { - int retSz; - char* ret; - - retSz = (int)wcslen(in) * 2; - ret = (char*)WMALLOC(retSz + 1, heap, DYNTYPE_SSHD); - if (ret != NULL) { - size_t numConv = 0; - if (wcstombs_s(&numConv, ret, retSz, in, retSz) != 0) { - WFREE(ret, heap, DYNTYPE_SSHD); - ret = NULL; +/* Set *err to WS_MEMORY_E on alloc failure, or WS_FATAL_ERROR on conversion failure. */ +static char* _convertHelper(WCHAR* in, void* heap, int* err) { + char* ret = NULL; + size_t needed = 0; + + /* Query exact size needed for multi-byte ACPs. Avoids zero-size for empty args. */ + if (wcstombs_s(&needed, NULL, 0, in, 0) == 0 && needed > 0) { + ret = (char*)WMALLOC(needed, heap, DYNTYPE_SSHD); + if (ret == NULL) { + *err = WS_MEMORY_E; } + else { + size_t numConv = 0; + if (wcstombs_s(&numConv, ret, needed, in, needed - 1) != 0) { + WFREE(ret, heap, DYNTYPE_SSHD); + ret = NULL; + *err = WS_FATAL_ERROR; + } + } + } + else { + *err = WS_FATAL_ERROR; } return ret; } @@ -4192,10 +4202,14 @@ static int StartSSHD(int argc, char** argv) /* Zero first: _freeWinArgs() walks all argc slots. */ WMEMSET(argv, 0, argc * sizeof(char*)); for (z = 0; z < argc; z++) { - argv[z] = _convertHelper(cmdArgs[z], NULL); + int convErr = WS_FATAL_ERROR; + + argv[z] = _convertHelper(cmdArgs[z], NULL, &convErr); if (argv[z] == NULL) { /* mygetopt() dereferences every entry it walks. */ - ret = WS_MEMORY_E; + wolfSSH_Log(WS_LOG_ERROR, + "[SSHD] Unable to convert argument %u.", z); + ret = convErr; break; } } diff --git a/src/internal.c b/src/internal.c index 77382f50..d93d0fb0 100644 --- a/src/internal.c +++ b/src/internal.c @@ -17977,7 +17977,7 @@ static int SignHEcdsa(WOLFSSH* ssh, byte* sig, word32* sigSz, } if (ret == WS_SUCCESS) { - word32 written; + word32 written = 0; rPad = (r[0] & 0x80) ? 1 : 0; sPad = (s[0] & 0x80) ? 1 : 0; @@ -20936,44 +20936,20 @@ static int PrepareUserAuthRequestEccCert(WOLFSSH* ssh, word32* payloadSz, } else #endif /* WOLFSSH_WINDOWS_CERT_STORE */ - { - #if 0 - #ifdef WOLFSSH_AGENT - if (ssh->agentEnabled) { - word32 sz; - const byte* c = - (const byte*)authData->sf.publicKey.publicKey; - - 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); - } - else - #endif - #endif - if (authData->sf.publicKey.privateKey == NULL || - authData->sf.publicKey.privateKeySz == 0) { - /* A cert-store-only client has no in-memory key; a - * decode of the empty buffer would report a misleading - * wolfCrypt ASN error. */ - WLOG(WS_LOG_DEBUG, "PrepareUserAuthRequestEccCert: No " - "private key; the offered certificate matched no " - "cert-store slot"); - ret = WS_BAD_ARGUMENT; - } - else { - ret = wc_EccPrivateKeyDecode( - authData->sf.publicKey.privateKey, - &idx, &keySig->ks.ecc.key, - authData->sf.publicKey.privateKeySz); - } + /* No WOLFSSH_AGENT branch: only RSA certs support agent signing. */ + if (authData->sf.publicKey.privateKey == NULL || + authData->sf.publicKey.privateKeySz == 0) { + /* Avoid misleading ASN error for cert-store-only clients without in-memory key. */ + WLOG(WS_LOG_DEBUG, "PrepareUserAuthRequestEccCert: No " + "private key available for the offered ECC " + "certificate"); + ret = WS_BAD_ARGUMENT; + } + else { + ret = wc_EccPrivateKeyDecode( + authData->sf.publicKey.privateKey, + &idx, &keySig->ks.ecc.key, + authData->sf.publicKey.privateKeySz); } } @@ -21055,31 +21031,7 @@ static int BuildUserAuthRequestEccCert(WOLFSSH* ssh, WMEMCPY(checkData + i, sigStart, begin - sigStartIdx); } - #if 0 - #ifdef WOLFSSH_AGENT - if (ssh->agentEnabled) { - if (ret == WS_SUCCESS) - ret = wolfSSH_AGENT_SignRequest(ssh, checkData, checkDataSz, - sig, &sigSz, - authData->sf.publicKey.publicKey, - authData->sf.publicKey.publicKeySz, 0); - if (ret == WS_SUCCESS) { - /* begin indexes into output, whose capacity is outputSz. */ - if (outputSz <= begin || outputSz - begin < LENGTH_SZ + sigSz) { - WLOG(WS_LOG_DEBUG, "SUAR: ECDSA agent sig doesn't fit output"); - ret = WS_BUFFER_E; - } - } - if (ret == WS_SUCCESS) { - c32toa(sigSz, output + begin); - begin += LENGTH_SZ; - XMEMCPY(output + begin, sig, sigSz); - begin += sigSz; - } - } - else - #endif - #endif + /* Scope for cert-store pvtKey */ { #ifdef WOLFSSH_WINDOWS_CERT_STORE const WOLFSSH_PVT_KEY* pvtKey; @@ -21138,6 +21090,7 @@ static int BuildUserAuthRequestEccCert(WOLFSSH* ssh, } else #endif /* WOLFSSH_WINDOWS_CERT_STORE */ + /* No WOLFSSH_AGENT branch: only RSA certs support agent signing. */ { if (ret == WS_SUCCESS) { ret = wc_ecc_sign_hash(digest, digestSz, sig, &sigSz, @@ -24634,7 +24587,7 @@ static int CompositeEccSign(void* key, WC_RNG* rng, void* heap, /* RFC 5656 3.1.2: mpints with the top bit set need a zero pad. */ byte rPad = (rBuf[0] & 0x80) ? 1 : 0; byte sPad = (sBuf[0] & 0x80) ? 1 : 0; - word32 written; + word32 written = 0; if (EncodeEcdsaRsToMpints(wireSig, *wireSigSz, rBuf, rSz, rPad, sBuf, sSz, sPad, &written) != WS_SUCCESS) {