diff --git a/src/tls.c b/src/tls.c index c608261217..71a7b2057e 100644 --- a/src/tls.c +++ b/src/tls.c @@ -10222,15 +10222,24 @@ static int TLSX_KeyShare_ProcessPqcHybridClient(WOLFSSL* ssl, ecc_kse->key = NULL; pqc_kse->privKey = NULL; } + else #endif + { + /* Re-sync keyShareEntry->key with ecc_kse->key. ecc_kse->key was + * aliased to keyShareEntry->key above. The inner Process*_ex + * either ran its end-of-function cleanup and set ecc_kse->key + * to NULL (so the outer pointer must also become NULL to avoid + * UAF/double-free in TLSX_KeyShare_FreeAll), or returned early + * before cleanup with ecc_kse->key still pointing at the live + * key (so the outer pointer must keep that pointer for later + * freeing). Mirroring whatever the inner left in ecc_kse->key + * handles both cases correctly. */ + keyShareEntry->key = ecc_kse->key; + } } if (ret == 0) { - keyShareEntry->key = ecc_kse->key; - ecc_kse->key = NULL; - - if ((ret == 0) && - ((ssl->arrays->preMasterSz + ssSzPqc) > ENCRYPT_LEN)) { + if ((ssl->arrays->preMasterSz + ssSzPqc) > ENCRYPT_LEN) { WOLFSSL_MSG("shared secret is too long."); ret = LENGTH_ERROR; } diff --git a/tests/api/test_tls13.c b/tests/api/test_tls13.c index ba12b7e368..08b9a5f23f 100644 --- a/tests/api/test_tls13.c +++ b/tests/api/test_tls13.c @@ -4579,6 +4579,104 @@ int test_tls13_pqc_hybrid_truncated_keyshare(void) return EXPECT_RESULT(); } +/* Test that a malformed ECDH portion in a correctly-sized PQC hybrid + * KeyShare does not leave a dangling pointer in keyShareEntry->key. + * + * The earlier truncated-keyshare test is rejected by the keLen <= ctSz + * check before TLSX_KeyShare_ProcessPqcHybridClient sets up the + * ecc_kse->key = keyShareEntry->key alias, so it does not exercise the + * dangling-pointer path. This test sends a SECP256R1MLKEM768 key_share + * whose total length is correct (65-byte ECDH point + 1088-byte ML-KEM + * ciphertext = 1153 bytes) but whose ECDH leading byte (0x05) is not a + * valid X9.63 marker. ProcessEcc_ex then fails at wc_ecc_import_x963 + * AFTER its unconditional cleanup at the end of the function frees the + * aliased key. Without the fix, the outer keyShareEntry->key still + * holds the freed pointer; wolfSSL_free -> TLSX_KeyShare_FreeAll calls + * wc_ecc_free + XFREE on it, producing a use-after-free and a double + * free that ASAN flags. */ +int test_tls13_pqc_hybrid_malformed_ecdh(void) +{ + EXPECT_DECLS; +#if defined(WOLFSSL_TLS13) && !defined(NO_WOLFSSL_CLIENT) && \ + defined(WOLFSSL_HAVE_MLKEM) && defined(WOLFSSL_PQC_HYBRIDS) && \ + !defined(WOLFSSL_NO_ML_KEM_768) && defined(HAVE_ECC) && \ + !defined(WOLFSSL_MLKEM_NO_DECAPSULATE) && \ + !defined(WOLFSSL_MLKEM_NO_MAKE_KEY) && \ + (!defined(NO_ECC256) || defined(HAVE_ALL_CURVES)) && \ + !defined(NO_ECC_SECP) + WOLFSSL_CTX *ctx = NULL; + WOLFSSL *ssl = NULL; + /* 5 (record) + 4 (HS) + 1207 (ServerHello body) = 1216 bytes. */ + static byte serverHello[1216]; + word32 i = 0; + WOLFSSL_BUFFER_INFO msg; + + XMEMSET(serverHello, 0, sizeof(serverHello)); + + /* Record: handshake, TLS 1.2 compat, length 1211 (0x04bb). */ + serverHello[i++] = 0x16; serverHello[i++] = 0x03; serverHello[i++] = 0x03; + serverHello[i++] = 0x04; serverHello[i++] = 0xbb; + /* Handshake: ServerHello (0x02), length 1207 (0x0004b7). */ + serverHello[i++] = 0x02; + serverHello[i++] = 0x00; serverHello[i++] = 0x04; serverHello[i++] = 0xb7; + /* legacy_version */ + serverHello[i++] = 0x03; serverHello[i++] = 0x03; + /* random (32 bytes) */ + XMEMSET(&serverHello[i], 0x42, 32); i += 32; + /* legacy_session_id_echo length: 0 */ + serverHello[i++] = 0x00; + /* cipher_suite: TLS_AES_128_GCM_SHA256 */ + serverHello[i++] = 0x13; serverHello[i++] = 0x01; + /* legacy_compression_method: null */ + serverHello[i++] = 0x00; + /* extensions length: 1167 (0x048f) */ + serverHello[i++] = 0x04; serverHello[i++] = 0x8f; + /* extension: supported_versions -> TLS 1.3 */ + serverHello[i++] = 0x00; serverHello[i++] = 0x2b; + serverHello[i++] = 0x00; serverHello[i++] = 0x02; + serverHello[i++] = 0x03; serverHello[i++] = 0x04; + /* extension: key_share, extension_data length 1157 (0x0485) */ + serverHello[i++] = 0x00; serverHello[i++] = 0x33; + serverHello[i++] = 0x04; serverHello[i++] = 0x85; + /* server_share.group: SECP256R1MLKEM768 (0x11eb) */ + serverHello[i++] = 0x11; serverHello[i++] = 0xeb; + /* key_exchange length: 1153 (0x0481) */ + serverHello[i++] = 0x04; serverHello[i++] = 0x81; + /* ECDH portion (65 bytes): leading 0x05 is not a valid X9.63 marker + * (valid markers: 0x04, 0x06, 0x07). The remaining 64 bytes stay zero + * from the initial XMEMSET. */ + serverHello[i++] = 0x05; + i += 64; + /* PQC portion (1088 bytes): all zero from the initial XMEMSET. */ + i += 1088; + AssertIntEQ((int)i, (int)sizeof(serverHello)); + + ExpectNotNull(ctx = wolfSSL_CTX_new(wolfTLSv1_3_client_method())); + wolfSSL_SetIORecv(ctx, PqcHybridUafRecv); + wolfSSL_SetIOSend(ctx, PqcHybridUafSend); + + ExpectNotNull(ssl = wolfSSL_new(ctx)); + + /* Match the server's offered group so this key_share is processed. */ + ExpectIntEQ(wolfSSL_UseKeyShare(ssl, WOLFSSL_SECP256R1MLKEM768), + WOLFSSL_SUCCESS); + + msg.buffer = serverHello; + msg.length = (unsigned int)sizeof(serverHello); + wolfSSL_SetIOReadCtx(ssl, &msg); + + /* Connect should fail gracefully on the malformed ECDH point. */ + ExpectIntEQ(wolfSSL_connect_TLSv13(ssl), + WC_NO_ERR_TRACE(WOLFSSL_FATAL_ERROR)); + + /* Without the fix, this triggers UAF + double-free in + * TLSX_KeyShare_FreeAll. */ + wolfSSL_free(ssl); + wolfSSL_CTX_free(ctx); +#endif + return EXPECT_RESULT(); +} + /* Test that a TLS 1.3 NewSessionTicket with a ticket shorter than ID_LEN * (32 bytes) does not cause an unsigned integer underflow / OOB read in * SetTicket. Uses a full memio handshake, then injects a crafted diff --git a/tests/api/test_tls13.h b/tests/api/test_tls13.h index c8b42fc56c..5d24f1674c 100644 --- a/tests/api/test_tls13.h +++ b/tests/api/test_tls13.h @@ -45,6 +45,7 @@ int test_tls13_unknown_ext_rejected(void); int test_tls13_cert_req_sigalgs(void); int test_tls13_derive_keys_no_key(void); int test_tls13_pqc_hybrid_truncated_keyshare(void); +int test_tls13_pqc_hybrid_malformed_ecdh(void); int test_tls13_empty_record_limit(void); int test_tls13_short_session_ticket(void); int test_tls13_early_data_0rtt_replay(void); @@ -82,6 +83,7 @@ int test_tls13_cert_with_extern_psk_sh_confirms_resumption(void); TEST_DECL_GROUP("tls13", test_tls13_cert_req_sigalgs), \ TEST_DECL_GROUP("tls13", test_tls13_derive_keys_no_key), \ TEST_DECL_GROUP("tls13", test_tls13_pqc_hybrid_truncated_keyshare), \ + TEST_DECL_GROUP("tls13", test_tls13_pqc_hybrid_malformed_ecdh), \ TEST_DECL_GROUP("tls13", test_tls13_empty_record_limit), \ TEST_DECL_GROUP("tls13", test_tls13_short_session_ticket), \ TEST_DECL_GROUP("tls13", test_tls13_early_data_0rtt_replay), \