From 3543ef0625f4f85366cce3fc998ac0f45846a1a8 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Fri, 30 Oct 2026 10:36:23 -0700 Subject: [PATCH] Free only the KEX keys that were initialized SendKexDhReply and the ECDH and Curve25519 KeyAgree functions now track which keys were initialized and free only those. A host-key format with no loaded key, or a failed small-stack allocation, no longer hands never-initialized storage to the wolfCrypt free. - sigKeyBlockFull gains keyInit, set by SendKexGetSigningKey - per-key Inited flags in the client and server KeyAgree functions - regress: no matching host key frees nothing (WOLF_CRYPTO_CB_FREE) - unit: server KeyAgree survives an allocation failure (small stack) Issue: F-11675, F-11939 --- src/internal.c | 72 ++++++++++++++++++---- tests/regress.c | 57 ++++++++++++++++++ tests/unit.c | 147 +++++++++++++++++++++++++++++++++++++++++++++ wolfssh/internal.h | 4 ++ 4 files changed, 268 insertions(+), 12 deletions(-) diff --git a/src/internal.c b/src/internal.c index f142c0db9..f6b3fc23e 100644 --- a/src/internal.c +++ b/src/internal.c @@ -8522,6 +8522,7 @@ static int KeyAgreeEcdh_client(WOLFSSH* ssh, byte hashId, { int ret = WS_SUCCESS; int primeId = ECC_CURVE_INVALID; + int keyInited = 0; ecc_key *key_ptr = NULL; #ifndef WOLFSSH_SMALL_STACK ecc_key key_s; @@ -8539,8 +8540,11 @@ static int KeyAgreeEcdh_client(WOLFSSH* ssh, byte hashId, #else /* ! WOLFSSH_SMALL_STACK */ key_ptr = &key_s; #endif /* WOLFSSH_SMALL_STACK */ - if (ret == 0) + if (ret == 0) { ret = wc_ecc_init(key_ptr); + if (ret == 0) + keyInited = 1; + } /* Enforce the negotiated curve on the peer point, matching the server. */ if (ret == 0) { primeId = wcPrimeForId(ssh->handshake->kexId); @@ -8563,7 +8567,8 @@ static int KeyAgreeEcdh_client(WOLFSSH* ssh, byte hashId, ret = WS_CRYPTO_FAILED; } } - wc_ecc_free(key_ptr); + if (keyInited) + wc_ecc_free(key_ptr); #ifdef WOLFSSH_SMALL_STACK if (key_ptr) { WFREE(key_ptr, ssh->ctx->heap, DYNTYPE_PRIVKEY); @@ -8595,6 +8600,7 @@ static int KeyAgreeCurve25519_client(WOLFSSH* ssh, byte hashId, #ifndef WOLFSSH_NO_CURVE25519_SHA256 { int ret; + int pubInited = 0; curve25519_key pub; WLOG(WS_LOG_DEBUG, "Entering KeyAgreeCurve25519_client()"); @@ -8602,6 +8608,7 @@ static int KeyAgreeCurve25519_client(WOLFSSH* ssh, byte hashId, ret = wc_curve25519_init(&pub); if (ret == 0) { + pubInited = 1; ret = wc_curve25519_check_public(f, fSz, EC25519_LITTLE_ENDIAN); } @@ -8627,7 +8634,8 @@ static int KeyAgreeCurve25519_client(WOLFSSH* ssh, byte hashId, } } - wc_curve25519_free(&pub); + if (pubInited) + wc_curve25519_free(&pub); wc_curve25519_free(&ssh->handshake->privKey.curve25519); WLOG(WS_LOG_DEBUG, "Leaving KeyAgreeCurve25519_client(), ret = %d", ret); @@ -16038,6 +16046,7 @@ int SendKexInit(WOLFSSH* ssh) struct wolfSSH_sigKeyBlockFull { byte pubKeyId; /* handshake->pubKeyId */ byte pubKeyFmtId; + byte keyInit; /* sk key initialized, so it needs a free */ word32 sz; const char *pubKeyName; /* IdToName(handshake->pubKeyId) */ word32 pubKeyNameSz; @@ -16446,6 +16455,8 @@ static int SendKexGetSigningKey(WOLFSSH* ssh, sigKeyBlock_ptr->sk.rsa.nSz = (word32)sizeof(sigKeyBlock_ptr->sk.rsa.n); ret = wc_InitRsaKey(&sigKeyBlock_ptr->sk.rsa.key, heap); + if (ret == 0) + sigKeyBlock_ptr->keyInit = 1; if (ret == 0 && certDer != NULL && certDerSz > 0) { byte* pubKeyDer = NULL; @@ -16482,6 +16493,8 @@ static int SendKexGetSigningKey(WOLFSSH* ssh, sigKeyBlock_ptr->sk.rsa.nSz = (word32)sizeof(sigKeyBlock_ptr->sk.rsa.n); ret = wc_InitRsaKey(&sigKeyBlock_ptr->sk.rsa.key, heap); + if (ret == 0) + sigKeyBlock_ptr->keyInit = 1; #ifdef WOLFSSH_TPM if (ret == 0 && ssh->ctx->privateKey[keyIdx].isTpm) { /* No private key in RAM; take the public key from the TPM. */ @@ -16601,6 +16614,8 @@ static int SendKexGetSigningKey(WOLFSSH* ssh, (word32)sizeof(sigKeyBlock_ptr->sk.ecc.q); ret = wc_ecc_init_ex(&sigKeyBlock_ptr->sk.ecc.key, heap, INVALID_DEVID); + if (ret == 0) + sigKeyBlock_ptr->keyInit = 1; scratch = 0; #ifdef WOLFSSH_WINDOWS_CERT_STORE /* A slot is never both cert-store and TPM backed; testing the @@ -16736,6 +16751,8 @@ static int SendKexGetSigningKey(WOLFSSH* ssh, sigKeyBlock_ptr->sk.ed.qSz = sizeof(sigKeyBlock_ptr->sk.ed.q); ret = wc_ed25519_init(&sigKeyBlock_ptr->sk.ed.key); + if (ret == 0) + sigKeyBlock_ptr->keyInit = 1; scratch = 0; if (ret == 0) @@ -16827,6 +16844,8 @@ static int SendKexGetSigningKey(WOLFSSH* ssh, ret = wc_MlDsaKey_Init(&sigKeyBlock_ptr->sk.mldsa.key, heap, INVALID_DEVID); + if (ret == 0) + sigKeyBlock_ptr->keyInit = 1; if (ret == 0) ret = wc_MlDsaKey_SetParams(&sigKeyBlock_ptr->sk.mldsa.key, level); @@ -16943,6 +16962,7 @@ static int SendKexGetSigningKey(WOLFSSH* ssh, * rebuild via export/import; others copy flat safely. */ sigKeyBlock_ptr->sk.mldsa_composite.base.mldsa = keySig->ks.mldsa_composite.mldsa; + sigKeyBlock_ptr->keyInit = 1; wc_MlDsaKey_Free(&keySig->ks.mldsa_composite.mldsa); { const CompositeTradOps* tradOps = @@ -17528,6 +17548,8 @@ static int KeyAgreeEcdh_server(WOLFSSH* ssh, byte hashId, byte* f, word32* fSz) ecc_key privKey[1]; #endif int primeId; + int pubKeyInited = 0; + int privKeyInited = 0; WLOG(WS_LOG_DEBUG, "Entering KeyAgreeEcdh_server()"); WOLFSSH_UNUSED(hashId); @@ -17536,10 +17558,16 @@ static int KeyAgreeEcdh_server(WOLFSSH* ssh, byte hashId, byte* f, word32* fSz) if (primeId == ECC_CURVE_INVALID) ret = WS_INVALID_PRIME_CURVE; - if (ret == 0) + if (ret == 0) { ret = wc_ecc_init_ex(pubKey, heap, INVALID_DEVID); - if (ret == 0) + if (ret == 0) + pubKeyInited = 1; + } + if (ret == 0) { ret = wc_ecc_init_ex(privKey, heap, INVALID_DEVID); + if (ret == 0) + privKeyInited = 1; + } #ifdef HAVE_WC_ECC_SET_RNG if (ret == 0) ret = wc_ecc_set_rng(privKey, ssh->rng); @@ -17564,8 +17592,10 @@ static int KeyAgreeEcdh_server(WOLFSSH* ssh, byte hashId, byte* f, word32* fSz) ssh->k, &ssh->kSz); PRIVATE_KEY_LOCK(); } - wc_ecc_free(privKey); - wc_ecc_free(pubKey); + if (privKeyInited) + wc_ecc_free(privKey); + if (pubKeyInited) + wc_ecc_free(pubKey); #ifdef WOLFSSH_SMALL_STACK WFREE(pubKey, heap, DYNTYPE_PUBKEY); WFREE(privKey, heap, DYNTYPE_PRIVKEY); @@ -17609,14 +17639,22 @@ static int KeyAgreeCurve25519_server(WOLFSSH* ssh, byte hashId, #else curve25519_key pubKey[1], privKey[1]; #endif + int pubKeyInited = 0; + int privKeyInited = 0; WLOG(WS_LOG_DEBUG, "Entering KeyAgreeCurve25519_server()"); WOLFSSH_UNUSED(hashId); - if (ret == 0) + if (ret == 0) { ret = wc_curve25519_init_ex(pubKey, heap, INVALID_DEVID); - if (ret == 0) + if (ret == 0) + pubKeyInited = 1; + } + if (ret == 0) { ret = wc_curve25519_init_ex(privKey, heap, INVALID_DEVID); + if (ret == 0) + privKeyInited = 1; + } if (ret == 0) ret = wc_curve25519_check_public(ssh->handshake->e, ssh->handshake->eSz, EC25519_LITTLE_ENDIAN); @@ -17641,8 +17679,10 @@ static int KeyAgreeCurve25519_server(WOLFSSH* ssh, byte hashId, ssh->k, &ssh->kSz, EC25519_LITTLE_ENDIAN); PRIVATE_KEY_LOCK(); } - wc_curve25519_free(privKey); - wc_curve25519_free(pubKey); + if (privKeyInited) + wc_curve25519_free(privKey); + if (pubKeyInited) + wc_curve25519_free(pubKey); #ifdef WOLFSSH_SMALL_STACK WFREE(pubKey, heap, DYNTYPE_PUBKEY); WFREE(privKey, heap, DYNTYPE_PRIVKEY); @@ -19132,7 +19172,7 @@ int SendKexDhReply(WOLFSSH* ssh) /* Note: this free chain uses pubKeyFmtId, but it must structurally mirror * the initialization switch in SendKexGetSigningKey which dispatches on pubKeyFmtId. */ - if (sigKeyBlock_ptr != NULL) { + if (sigKeyBlock_ptr != NULL && sigKeyBlock_ptr->keyInit) { if (sigKeyBlock_ptr->pubKeyFmtId == ID_SSH_RSA #ifdef WOLFSSH_CERTS || sigKeyBlock_ptr->pubKeyFmtId == ID_X509V3_SSH_RSA @@ -26851,6 +26891,14 @@ int wolfSSH_TestKeyAgreeEcdh_client(WOLFSSH* ssh, byte hashId, } #endif /* !WOLFSSH_NO_ECDH */ +#ifndef WOLFSSH_NO_CURVE25519_SHA256 +int wolfSSH_TestKeyAgreeCurve25519_server(WOLFSSH* ssh, byte hashId, + byte* f, word32* fSz) +{ + return KeyAgreeCurve25519_server(ssh, hashId, f, fSz); +} +#endif /* !WOLFSSH_NO_CURVE25519_SHA256 */ + #if !defined(WOLFSSH_NO_NISTP256_MLKEM768_SHA256) || \ !defined(WOLFSSH_NO_NISTP384_MLKEM1024_SHA384) || \ !defined(WOLFSSH_NO_CURVE25519_MLKEM768_SHA256) diff --git a/tests/regress.c b/tests/regress.c index ad9b79a94..541c6843c 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -43,6 +43,9 @@ #include #include +#ifdef WOLF_CRYPTO_CB + #include +#endif #include #include #include @@ -2136,6 +2139,56 @@ static void TestKexDhReplyRejectsEccSigNameDowngrade(void) } #endif +#if !defined(WOLFSSH_NO_RSA_SHA2_256) && \ + !defined(WOLFSSH_NO_ECDSA_SHA2_NISTP256) +#if defined(WOLF_CRYPTO_CB) && defined(WOLF_CRYPTO_CB_FREE) +/* Device 0 is the devId a zeroed, never-initialized key carries, so a + * wolfCrypt free of one is dispatched here. */ +#define REGRESS_ZERO_DEVID 0 +static int noHostKeyRsaFrees; + +static int NoHostKeyFreeCb(int devId, wc_CryptoInfo* info, void* ctx) +{ + (void)devId; + (void)ctx; + if (info != NULL && info->algo_type == WC_ALGO_TYPE_FREE + && info->free.algo == WC_ALGO_TYPE_PK + && info->free.type == WC_PK_TYPE_RSA) { + noHostKeyRsaFrees++; + } + return CRYPTOCB_UNAVAILABLE; +} +#endif + +/* The server negotiates rsa-sha2-256 but only has an ECDSA host key, so + * SendKexDhReply finds no signing key. It must fail without freeing the + * RSA key it never initialized. */ +static void TestKexDhReplyNoMatchingHostKey(void) +{ + KexReplyHarness harness; + KexReplyRunResult result; + + InitKexReplyHarness(&harness, "rsa-sha2-256", REGRESS_SERVER_KEY_ECC_PATH, + 0, NULL); +#if defined(WOLF_CRYPTO_CB) && defined(WOLF_CRYPTO_CB_FREE) + noHostKeyRsaFrees = 0; + AssertIntEQ(wc_CryptoCb_RegisterDevice(REGRESS_ZERO_DEVID, + NoHostKeyFreeCb, NULL), 0); +#endif + RunKexReplyHandshake(&harness, &result); +#if defined(WOLF_CRYPTO_CB) && defined(WOLF_CRYPTO_CB_FREE) + wc_CryptoCb_UnRegisterDevice(REGRESS_ZERO_DEVID); + AssertIntEQ(noHostKeyRsaFrees, 0); +#endif + + AssertFalse(result.serverSuccess); + AssertIntEQ(result.serverErr, WS_INVALID_ALGO_ID); + AssertFalse(result.clientSuccess); + + FreeKexReplyHarness(&harness); +} +#endif + #ifndef WOLFSSH_NO_ED25519 static void TestKexDhReplyRejectsEd25519SigNameDowngrade(void) { @@ -22172,6 +22225,10 @@ int main(int argc, char** argv) #ifndef WOLFSSH_NO_ECDSA_SHA2_NISTP256 TestKexDhReplyRejectsEccSigNameDowngrade(); #endif + #if !defined(WOLFSSH_NO_RSA_SHA2_256) && \ + !defined(WOLFSSH_NO_ECDSA_SHA2_NISTP256) + TestKexDhReplyNoMatchingHostKey(); + #endif #ifndef WOLFSSH_NO_ED25519 TestKexDhReplyRejectsEd25519SigNameDowngrade(); #endif diff --git a/tests/unit.c b/tests/unit.c index ffc3517d6..b3c42f9bf 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -21233,6 +21233,142 @@ static int test_KeyAgreeEcdh_client_rejectsOffCurvePoint(void) } #endif /* !WOLFSSH_NO_ECDH && !WOLFSSH_NO_ECDH_SHA2_NISTP256 */ +#if defined(WOLFSSH_SMALL_STACK) && \ + defined(WOLFSSH_TEST_CAPTURING_ALLOCATOR) && \ + ((!defined(WOLFSSH_NO_ECDH) && \ + !defined(WOLFSSH_NO_ECDH_SHA2_NISTP256)) || \ + !defined(WOLFSSH_NO_CURVE25519_SHA256)) +/* Fault-injecting allocator: the failAt'th allocation after install + * returns NULL, and every other one is filled with 0xCC so storage that + * was never initialized looks like garbage, not zeros. The other one of + * the first two allocations is never initialized; the free checks it is + * still all 0xCC, so a wolfCrypt free that touched it (the Curve25519 + * free zeroes the key) is caught. */ +#define FAIL_NTH_PROBE_SZ 2 +static int failAllocAt = 0; +static int failAllocCount = 0; +static void* failNthPtr[FAIL_NTH_PROBE_SZ]; +static size_t failNthSz[FAIL_NTH_PROBE_SZ]; +static int failAllocTouched = 0; + +static void* FailNthMalloc(size_t size) +{ + void* p; + + if (++failAllocCount == failAllocAt) + return NULL; + p = malloc(size); + if (p != NULL) + WMEMSET(p, 0xCC, size); + if (failAllocCount <= FAIL_NTH_PROBE_SZ) { + failNthPtr[failAllocCount - 1] = p; + failNthSz[failAllocCount - 1] = size; + } + return p; +} + +static void FailNthFree(void* ptr) +{ + size_t i, j; + + for (i = 0; ptr != NULL && i < FAIL_NTH_PROBE_SZ; i++) { + if (ptr == failNthPtr[i]) { + for (j = 0; j < failNthSz[i]; j++) { + if (((byte*)ptr)[j] != 0xCC) { + failAllocTouched = 1; + break; + } + } + failNthPtr[i] = NULL; + } + } + free(ptr); +} + +static void* FailNthRealloc(void* ptr, size_t size) +{ + return realloc(ptr, size); +} + +typedef int (*KeyAgreeServerFn)(WOLFSSH* ssh, byte hashId, + byte* f, word32* fSz); + +/* Verify the server ECDH and Curve25519 KeyAgree functions survive a + * failed allocation of either temporary key under WOLFSSH_SMALL_STACK. + * When one allocation fails, the other key was allocated but never + * initialized; it must not be handed to the wolfCrypt free, so it must + * reach WFREE still holding the allocator's fill. */ +static int test_KeyAgree_server_allocFailCleanup(void) +{ + static const struct { + byte kexId; + KeyAgreeServerFn fn; + } cases[] = { +#if !defined(WOLFSSH_NO_ECDH) && !defined(WOLFSSH_NO_ECDH_SHA2_NISTP256) + { ID_ECDH_SHA2_NISTP256, wolfSSH_TestKeyAgreeEcdh_server }, +#endif +#ifndef WOLFSSH_NO_CURVE25519_SHA256 + { ID_CURVE25519_SHA256, wolfSSH_TestKeyAgreeCurve25519_server }, +#endif + }; + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + byte f[MAX_KEX_KEY_SZ]; + word32 fSz; + word32 i; + int failAt; + int ret; + int result = 0; + wolfSSL_Malloc_cb prevMf = NULL; + wolfSSL_Free_cb prevFf = NULL; + wolfSSL_Realloc_cb prevRf = NULL; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + if (ctx == NULL) + return -790; + ssh = wolfSSH_new(ctx); + if (ssh == NULL || ssh->handshake == NULL) { + result = -791; + goto out; + } + + wolfSSL_GetAllocators(&prevMf, &prevFf, &prevRf); + for (i = 0; i < (word32)(sizeof(cases) / sizeof(cases[0])); i++) { + /* The two temporary keys are the first two allocations. */ + for (failAt = 1; failAt <= 2; failAt++) { + ssh->handshake->kexId = cases[i].kexId; + fSz = (word32)sizeof(f); + failAllocAt = failAt; + failAllocCount = 0; + failAllocTouched = 0; + WMEMSET(failNthPtr, 0, sizeof(failNthPtr)); + if (wolfSSL_SetAllocators(FailNthMalloc, FailNthFree, + FailNthRealloc) != 0) { + result = -792; + goto out; + } + ret = cases[i].fn(ssh, WC_HASH_TYPE_SHA256, f, &fSz); + wolfSSL_SetAllocators(prevMf, prevFf, prevRf); + if (ret != WS_MEMORY_E) { + result = -793; + goto out; + } + if (failAllocTouched) { + result = -794; + goto out; + } + } + } + +out: + if (ssh != NULL) + wolfSSH_free(ssh); + if (ctx != NULL) + wolfSSH_CTX_free(ctx); + return result; +} +#endif /* WOLFSSH_SMALL_STACK && WOLFSSH_TEST_CAPTURING_ALLOCATOR && ... */ + #if !defined(WOLFSSH_NO_NISTP256_MLKEM768_SHA256) || \ !defined(WOLFSSH_NO_NISTP384_MLKEM1024_SHA384) || \ !defined(WOLFSSH_NO_CURVE25519_MLKEM768_SHA256) @@ -27006,6 +27142,17 @@ int wolfSSH_UnitTest(int argc, char** argv) testResult = testResult || unitResult; #endif /* !WOLFSSH_NO_ECDH && !WOLFSSH_NO_ECDH_SHA2_NISTP256 */ +#if defined(WOLFSSH_SMALL_STACK) && \ + defined(WOLFSSH_TEST_CAPTURING_ALLOCATOR) && \ + ((!defined(WOLFSSH_NO_ECDH) && \ + !defined(WOLFSSH_NO_ECDH_SHA2_NISTP256)) || \ + !defined(WOLFSSH_NO_CURVE25519_SHA256)) + unitResult = test_KeyAgree_server_allocFailCleanup(); + printf("KeyAgree_server_allocFailCleanup: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; +#endif + #if !defined(WOLFSSH_NO_NISTP256_MLKEM768_SHA256) || \ !defined(WOLFSSH_NO_NISTP384_MLKEM1024_SHA384) || \ !defined(WOLFSSH_NO_CURVE25519_MLKEM768_SHA256) diff --git a/wolfssh/internal.h b/wolfssh/internal.h index d9c550995..692ee416b 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -2209,6 +2209,10 @@ enum WS_MessageIdLimits { WOLFSSH_API int wolfSSH_TestKeyAgreeEcdh_client(WOLFSSH* ssh, byte hashId, const byte* f, word32 fSz); #endif /* !WOLFSSH_NO_ECDH */ +#ifndef WOLFSSH_NO_CURVE25519_SHA256 + WOLFSSH_API int wolfSSH_TestKeyAgreeCurve25519_server(WOLFSSH* ssh, + byte hashId, byte* f, word32* fSz); +#endif /* !WOLFSSH_NO_CURVE25519_SHA256 */ #if !defined(WOLFSSH_NO_NISTP256_MLKEM768_SHA256) || \ !defined(WOLFSSH_NO_NISTP384_MLKEM1024_SHA384) || \ !defined(WOLFSSH_NO_CURVE25519_MLKEM768_SHA256)