From 90fcb5c086ef618d37005b87883d27d48fd2034c Mon Sep 17 00:00:00 2001 From: Emma Stensland Date: Thu, 23 Jul 2026 12:12:26 -0600 Subject: [PATCH] Fix memory-safety and error-handling edge cases --- apps/wolfsshd/auth.c | 10 ++++ apps/wolfsshd/test/start_sshd.sh | 7 +++ examples/tpmcertserver/tpmcertclient.c | 35 +++++++----- examples/tpmcertserver/tpmcertserver.c | 23 +++++--- src/internal.c | 65 +++++++++++++++++---- src/keygen.c | 38 ++++++++++--- tests/unit.c | 78 ++++++++++++++++++++++++++ wolfssh/internal.h | 2 + 8 files changed, 219 insertions(+), 39 deletions(-) diff --git a/apps/wolfsshd/auth.c b/apps/wolfsshd/auth.c index 608346a48..2333b5005 100644 --- a/apps/wolfsshd/auth.c +++ b/apps/wolfsshd/auth.c @@ -2049,6 +2049,16 @@ static int SetDefaultUserID(WOLFSSHD_AUTH* auth) } pwInfo = getpwnam(WOLFSSH_USER_STRING(WOLFSSH_SSHD_USER)); +#ifdef WOLFSSHD_UNIT_TEST + /* Unit tests run wolfSSHD_AuthCreateUser() outside of a real daemon + * install, where the dedicated "sshd" system account may not exist. + * Fall back to the invoking user so auth-flow tests can exercise + * wolfSSHD_AuthCreateUser() without requiring that account. Never + * enabled in a production build. */ + if (pwInfo == NULL) { + pwInfo = getpwuid(getuid()); + } +#endif if (pwInfo == NULL) { /* user name not found on system */ wolfSSH_Log(WS_LOG_INFO, "[SSHD] No %s user found to use", diff --git a/apps/wolfsshd/test/start_sshd.sh b/apps/wolfsshd/test/start_sshd.sh index ad0ea204f..1c553c07a 100755 --- a/apps/wolfsshd/test/start_sshd.sh +++ b/apps/wolfsshd/test/start_sshd.sh @@ -97,6 +97,13 @@ stop_wolfsshd() { printf "Stopping SSHD, killing pid $PID\n" sudo kill $PID + # Wait for the process to actually exit so a subsequent start_wolfsshd on + # the same port doesn't race the listening socket's release (EADDRINUSE). + for i in $(seq 1 50); do + sudo kill -0 $PID 2>/dev/null || break + sleep 0.1 + done + # The temp dir is owned by the invoking user, so its root-owned key copies # can be removed without sudo. if [ -n "$SSHD_KEYDIR" ]; then diff --git a/examples/tpmcertserver/tpmcertclient.c b/examples/tpmcertserver/tpmcertclient.c index 33defea6a..1ebf76d13 100644 --- a/examples/tpmcertserver/tpmcertclient.c +++ b/examples/tpmcertserver/tpmcertclient.c @@ -35,6 +35,7 @@ #define TPMCC_USER "jill" #define TPMCC_PASSWORD "upthehill" #define TPMCC_BUF_SZ 1024 +#define TPMCC_DEFAULT_PORT 22222 static int TpmCcUserAuth(byte authType, WS_UserAuthData* authData, void* ctx) { @@ -70,23 +71,31 @@ static int TpmCcHostKeyCheck(const byte* pubKey, word32 pubKeySz, void* ctx) static int TpmCcLoadFile(const char* file, byte* buf, word32* bufSz) { int ret = 0; - FILE* f = fopen(file, "rb"); - size_t n; - int extra; + WFILE* f; + word32 fileSz; + word32 readSz; - if (f == NULL) { + if (WFOPEN(NULL, &f, file, "rb") != 0) { ret = -1; } else { - n = fread(buf, 1, *bufSz, f); - /* If the buffer filled exactly, the file may be larger than the buffer; - * reject a truncated read rather than loading a partial certificate. */ - extra = (n == (size_t)*bufSz) ? fgetc(f) : EOF; - fclose(f); - if (n == 0 || extra != EOF) + WFSEEK(NULL, f, 0, WSEEK_END); + fileSz = (word32)WFTELL(NULL, f); + WREWIND(NULL, f); + + if (fileSz == 0 || fileSz > *bufSz) { ret = -1; - else - *bufSz = (word32)n; + } + else { + readSz = (word32)WFREAD(NULL, buf, 1, fileSz, f); + if (readSz != fileSz) { + ret = -1; + } + else { + *bufSz = fileSz; + } + } + WFCLOSE(NULL, f); } return ret; @@ -97,7 +106,7 @@ int main(int argc, char* argv[]) { int ret; int i; - word16 port = 22222; + word16 port = TPMCC_DEFAULT_PORT; const char* host = "127.0.0.1"; const char* caFile = "tpm-server-cert.der"; WOLFSSH_CTX* ctx = NULL; diff --git a/examples/tpmcertserver/tpmcertserver.c b/examples/tpmcertserver/tpmcertserver.c index 473afa334..ae78ea2be 100644 --- a/examples/tpmcertserver/tpmcertserver.c +++ b/examples/tpmcertserver/tpmcertserver.c @@ -51,6 +51,7 @@ #define TPMCS_KEY_AUTH "ThisIsMyKeyAuth" #define TPMCS_CERT_MAX 2048 #define TPMCS_BUF_SZ 1024 +#define TPMCS_DEFAULT_PORT 22222 static const char* certOutFile = "tpm-server-cert.der"; @@ -142,8 +143,16 @@ static int TpmCsMakeKeyAndCert(WOLFTPM2_DEV* dev, WOLFTPM2_KEY* key, } if (rc >= 0) { - *certDerSz = (word32)rc; - rc = 0; + /* wolfTPM2_CSR_Generate_ex returns the generated DER length on + * success; sanity-check it against the output buffer capacity + * before trusting it as the certificate size. */ + if (rc == 0 || (word32)rc > *certDerSz) { + rc = BUFFER_E; + } + else { + *certDerSz = (word32)rc; + rc = 0; + } } /* The crypto callback is only needed to self-sign the certificate. Always @@ -179,15 +188,15 @@ static int TpmCsMakeKeyAndCert(WOLFTPM2_DEV* dev, WOLFTPM2_KEY* key, static int TpmCsWriteCert(const char* file, const byte* der, word32 derSz) { int ret = 0; - FILE* f = fopen(file, "wb"); + WFILE* f; - if (f == NULL) { + if (WFOPEN(NULL, &f, file, "wb") != 0) { ret = -1; } else { - if (fwrite(der, 1, derSz, f) != derSz) + if (WFWRITE(NULL, der, 1, derSz, f) != derSz) ret = -1; - fclose(f); + WFCLOSE(NULL, f); } return ret; @@ -232,7 +241,7 @@ int main(int argc, char* argv[]) { int ret; int useEcc = 1; - word16 port = 22222; + word16 port = TPMCS_DEFAULT_PORT; int i; WOLFTPM2_DEV dev; WOLFTPM2_KEY hostKey; diff --git a/src/internal.c b/src/internal.c index a5fa0ee3b..610a73eae 100644 --- a/src/internal.c +++ b/src/internal.c @@ -41,9 +41,7 @@ #endif #include #include -#ifdef WOLFSSH_CERTS - #include -#endif +#include #include #include #include @@ -6285,6 +6283,26 @@ static void FreePubKey(struct wolfSSH_sigKeyBlock *p) } +/* Returns non-zero when a wolfCrypt error code indicates a local fault + * (memory, RNG, hardware, initialisation) rather than a peer-driven + * rejection. Used by the key-agreement helpers to log local faults at + * ERROR while keeping peer-driven rejects at DEBUG to prevent a remote + * peer from flooding the error log. */ +static WS_MAYBE_UNUSED int IsLocalCryptoFault(int wcRet) +{ + switch (wcRet) { + case MEMORY_E: + case RNG_FAILURE_E: + case WC_HW_E: + case WC_INIT_E: + case BAD_MUTEX_E: + return 1; + default: + return 0; + } +} + + /* KeyAgreeDh_client * hashId - wolfCrypt hash type ID used * f - peer public key @@ -6318,7 +6336,10 @@ static int KeyAgreeDh_client(WOLFSSH* ssh, byte hashId, f, fSz); PRIVATE_KEY_LOCK(); if (ret != 0) { - WLOG(WS_LOG_ERROR, + /* Local faults (RNG, HSM, memory) are logged at ERROR; + * peer-driven rejects stay at DEBUG so a remote peer + * cannot flood the error log. */ + WLOG(IsLocalCryptoFault(ret) ? WS_LOG_ERROR : WS_LOG_DEBUG, "Generate DH shared secret failed, %d", ret); ret = WS_CRYPTO_FAILED; } @@ -6410,7 +6431,10 @@ static int KeyAgreeEcdh_client(WOLFSSH* ssh, byte hashId, key_ptr, ssh->k, &ssh->kSz); PRIVATE_KEY_LOCK(); if (ret != 0) { - WLOG(WS_LOG_ERROR, + /* Local faults (RNG, HSM, memory) are logged at ERROR; + * peer-driven rejects stay at DEBUG so a remote peer + * cannot flood the error log. */ + WLOG(IsLocalCryptoFault(ret) ? WS_LOG_ERROR : WS_LOG_DEBUG, "Generate ECC shared secret failed, %d", ret); ret = WS_CRYPTO_FAILED; } @@ -6470,7 +6494,10 @@ static int KeyAgreeCurve25519_client(WOLFSSH* ssh, byte hashId, ssh->k, &ssh->kSz, EC25519_LITTLE_ENDIAN); PRIVATE_KEY_LOCK(); if (ret != 0) { - WLOG(WS_LOG_ERROR, + /* Local faults (RNG, HSM, memory) are logged at ERROR; + * peer-driven rejects stay at DEBUG so a remote peer + * cannot flood the error log. */ + WLOG(IsLocalCryptoFault(ret) ? WS_LOG_ERROR : WS_LOG_DEBUG, "Gen curve25519 shared secret failed, %d", ret); ret = WS_CRYPTO_FAILED; } @@ -6679,7 +6706,10 @@ static int KeyAgreeEcdhMlKem_client(WOLFSSH* ssh, byte hashId, ssh->kSz += length_sharedsecret; } else { ssh->kSz = 0; - WLOG(WS_LOG_ERROR, + /* Local faults (RNG, HSM, memory) are logged at ERROR; + * peer-driven rejects stay at DEBUG so a remote peer + * cannot flood the error log. */ + WLOG(IsLocalCryptoFault(ret) ? WS_LOG_ERROR : WS_LOG_DEBUG, "Generate ECC and ML-KEM (decap) shared secret failed, %d", ret); } @@ -15922,9 +15952,18 @@ static int PrepareUserAuthRequestPassword(WOLFSSH* ssh, word32* payloadSz, if (ssh == NULL || payloadSz == NULL || authData == NULL) ret = WS_BAD_ARGUMENT; - if (ret == WS_SUCCESS) - *payloadSz += BOOLEAN_SZ + LENGTH_SZ + - authData->sf.password.passwordSz; + if (ret == WS_SUCCESS) { + word32 addSz = BOOLEAN_SZ + LENGTH_SZ; + if (MAX_PACKET_SZ < *payloadSz || + MAX_PACKET_SZ - *payloadSz < addSz || + MAX_PACKET_SZ - *payloadSz - addSz < + authData->sf.password.passwordSz) { + ret = WS_BUFFER_E; + } + else { + *payloadSz += addSz + authData->sf.password.passwordSz; + } + } return ret; } @@ -20103,6 +20142,12 @@ int wolfSSH_TestDoUserAuthBanner(WOLFSSH* ssh, byte* buf, word32 len, return DoUserAuthBanner(ssh, buf, len, idx); } +int wolfSSH_TestPrepareUserAuthRequestPassword(WOLFSSH* ssh, + word32* payloadSz, const WS_UserAuthData* authData) +{ + return PrepareUserAuthRequestPassword(ssh, payloadSz, authData); +} + int wolfSSH_TestDoChannelRequest(WOLFSSH* ssh, byte* buf, word32 len, word32* idx) { diff --git a/src/keygen.c b/src/keygen.c index 98d338ebf..5acfc268b 100644 --- a/src/keygen.c +++ b/src/keygen.c @@ -276,19 +276,33 @@ int wolfSSH_MakeMlDsaKey(byte* out, word32 outSz, word32 level) } if (ret == WS_SUCCESS) { - MlDsaKey key; +#ifdef WOLFSSH_SMALL_STACK + MlDsaKey* key; +#else + MlDsaKey keyBuf; + MlDsaKey* key = &keyBuf; +#endif int keyInit = 0; - if (wc_MlDsaKey_Init(&key, NULL, INVALID_DEVID) != 0) - ret = WS_CRYPTO_FAILED; - else { - keyInit = 1; - if (wc_MlDsaKey_SetParams(&key, wc_level) != 0) +#ifdef WOLFSSH_SMALL_STACK + key = (MlDsaKey*)WMALLOC(sizeof(MlDsaKey), NULL, DYNTYPE_PRIVKEY); + if (key == NULL) { + ret = WS_MEMORY_E; + } +#endif + + if (ret == WS_SUCCESS) { + if (wc_MlDsaKey_Init(key, NULL, INVALID_DEVID) != 0) ret = WS_CRYPTO_FAILED; + else { + keyInit = 1; + if (wc_MlDsaKey_SetParams(key, wc_level) != 0) + ret = WS_CRYPTO_FAILED; + } } if (ret == WS_SUCCESS) { - ret = wc_MlDsaKey_MakeKey(&key, &rng); + ret = wc_MlDsaKey_MakeKey(key, &rng); if (ret != 0) { WLOG(WS_LOG_DEBUG, "ML-DSA key generation failed"); ret = WS_CRYPTO_FAILED; @@ -300,7 +314,7 @@ int wolfSSH_MakeMlDsaKey(byte* out, word32 outSz, word32 level) if (ret == WS_SUCCESS) { int keySz; - keySz = wc_MlDsaKey_KeyToDer(&key, out, outSz); + keySz = wc_MlDsaKey_KeyToDer(key, out, outSz); if (keySz < 0) { WLOG(WS_LOG_DEBUG, "ML-DSA key to DER failed"); ret = WS_CRYPTO_FAILED; @@ -310,9 +324,15 @@ int wolfSSH_MakeMlDsaKey(byte* out, word32 outSz, word32 level) } if (keyInit) { - wc_MlDsaKey_Free(&key); + wc_MlDsaKey_Free(key); } +#ifdef WOLFSSH_SMALL_STACK + if (key != NULL) { + WFREE(key, NULL, DYNTYPE_PRIVKEY); + } +#endif + if (wc_FreeRng(&rng) != 0) { WLOG(WS_LOG_DEBUG, "Couldn't free RNG"); if (ret >= 0) diff --git a/tests/unit.c b/tests/unit.c index 9bda2d571..76483dad4 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -2556,6 +2556,79 @@ static int test_DoUserAuthBanner(void) return result; } +/* Verify PrepareUserAuthRequestPassword rejects with WS_BUFFER_E instead of + * wrapping *payloadSz when it would exceed MAX_PACKET_SZ. */ +static int test_PrepareUserAuthRequestPassword(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + int result = 0; + /* BOOLEAN_SZ + LENGTH_SZ, mirrors internal.c's addSz */ + const word32 addSz = 1 + 4; + + struct { + word32 startSz; + word32 passwordSz; + int expectRet; + const char* label; + } cases[] = { + { 10, 20, WS_SUCCESS, "normal small payload" }, + { 35000 - 2, 1, WS_BUFFER_E, "addSz alone overflows" }, + { 100, 34900, WS_BUFFER_E, "passwordSz overflows" }, + { 40000, 0, WS_BUFFER_E, "payloadSz already exceeds max" }, + { 35000 - addSz, 0, WS_SUCCESS, "exact fit" }, + }; + int i; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + if (ctx == NULL) + return -320; + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { + wolfSSH_CTX_free(ctx); + return -321; + } + + for (i = 0; i < (int)(sizeof(cases)/sizeof(cases[0])); i++) { + WS_UserAuthData authData; + word32 payloadSz = cases[i].startSz; + int ret; + + WMEMSET(&authData, 0, sizeof(authData)); + authData.sf.password.passwordSz = cases[i].passwordSz; + + ret = wolfSSH_TestPrepareUserAuthRequestPassword(ssh, &payloadSz, + &authData); + if (ret != cases[i].expectRet) { + printf("PrepareUserAuthRequestPassword[%s]: ret=%d, expected=%d\n", + cases[i].label, ret, cases[i].expectRet); + result = -322 - i; + break; + } + if (ret == WS_SUCCESS && + payloadSz != cases[i].startSz + addSz + cases[i].passwordSz) { + printf("PrepareUserAuthRequestPassword[%s]: payloadSz=%u, " + "expected=%u\n", cases[i].label, payloadSz, + cases[i].startSz + addSz + cases[i].passwordSz); + result = -330 - i; + break; + } + } + + if (result == 0) { + int ret = wolfSSH_TestPrepareUserAuthRequestPassword(NULL, NULL, NULL); + if (ret != WS_BAD_ARGUMENT) { + printf("PrepareUserAuthRequestPassword[null args]: ret=%d, " + "expected=%d\n", ret, WS_BAD_ARGUMENT); + result = -340; + } + } + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} + #if defined(WOLFSSH_TEST_INTERNAL) && defined(WOLFSSH_SCP) /* Verify GetScpFileMode strips setuid/setgid/sticky bits from a peer-supplied * SCP C/D record mode, matching the masking already done on the send path. @@ -9330,6 +9403,11 @@ int wolfSSH_UnitTest(int argc, char** argv) printf("DoUserAuthBanner: %s\n", (unitResult == 0 ? "SUCCESS" : "FAILED")); testResult = testResult || unitResult; + unitResult = test_PrepareUserAuthRequestPassword(); + printf("PrepareUserAuthRequestPassword: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; + unitResult = test_DoChannelRequest(); printf("DoChannelRequest: %s\n", (unitResult == 0 ? "SUCCESS" : "FAILED")); testResult = testResult || unitResult; diff --git a/wolfssh/internal.h b/wolfssh/internal.h index 9f3b9c8f7..5067f2e9d 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -1497,6 +1497,8 @@ enum WS_MessageIdLimits { WOLFSSH_API int wolfSSH_TestDoReceive(WOLFSSH* ssh); WOLFSSH_API int wolfSSH_TestDoUserAuthBanner(WOLFSSH* ssh, byte* buf, word32 len, word32* idx); + WOLFSSH_API int wolfSSH_TestPrepareUserAuthRequestPassword(WOLFSSH* ssh, + word32* payloadSz, const WS_UserAuthData* authData); WOLFSSH_API int wolfSSH_TestDoChannelRequest(WOLFSSH* ssh, byte* buf, word32 len, word32* idx); WOLFSSH_API int wolfSSH_TestDoChannelSuccess(WOLFSSH* ssh, byte* buf,