diff --git a/src/tls.c b/src/tls.c index 535b43826b..961464a5f0 100644 --- a/src/tls.c +++ b/src/tls.c @@ -10175,6 +10175,7 @@ static int TLSX_KeyShare_ProcessPqcHybridClient(WOLFSSL* ssl, if (ret == 0) { keyShareEntry->key = ecc_kse->key; + ecc_kse->key = NULL; if ((ret == 0) && ((ssl->arrays->preMasterSz + ssSzPqc) > ENCRYPT_LEN)) { @@ -10210,6 +10211,17 @@ static int TLSX_KeyShare_ProcessPqcHybridClient(WOLFSSL* ssl, * here as it may already been set to the ECC shared secret size, * which would be too small due to the PQC offset case. */ ForceZero(ssl->arrays->preMasterSecret, ENCRYPT_LEN); + + /* Prevent FreeAll from freeing pointers owned by keyShareEntry. */ + if (ecc_kse != NULL) + ecc_kse->key = NULL; + if (pqc_kse != NULL) { + #ifndef WOLFSSL_TLSX_PQC_MLKEM_STORE_OBJ + pqc_kse->privKey = NULL; + #else + pqc_kse->key = NULL; + #endif + } } TLSX_KeyShare_FreeAll(ecc_kse, ssl->heap); diff --git a/tests/api/test_tls13.c b/tests/api/test_tls13.c index 34ddcc94c8..8ff9ad6bf3 100644 --- a/tests/api/test_tls13.c +++ b/tests/api/test_tls13.c @@ -3468,3 +3468,175 @@ int test_tls13_derive_keys_no_key(void) return EXPECT_RESULT(); } +/* Test that a truncated PQC hybrid KeyShare in a ServerHello does not cause a + * heap use-after-free during cleanup. A malicious server sends + * SECP256R1MLKEM768 with only 10 bytes of key exchange data (expected: 1120+). + * This exercises the error path in TLSX_KeyShare_ProcessPqcHybridClient(). + * Under ASAN the UAF manifests as ForceZero writing to freed KyberKey memory + * during wolfSSL_free -> TLSX_FreeAll -> TLSX_KeyShare_FreeAll. */ +#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) +/* Called when writing - discard output. */ +static int PqcHybridUafSend(WOLFSSL* ssl, char* buf, int sz, void* ctx) +{ + (void)ssl; + (void)buf; + (void)ctx; + return sz; +} +/* Called when reading - feed from buffer. */ +static int PqcHybridUafRecv(WOLFSSL* ssl, char* buf, int sz, void* ctx) +{ + WOLFSSL_BUFFER_INFO* msg = (WOLFSSL_BUFFER_INFO*)ctx; + int len = (int)msg->length; + + (void)ssl; + + if (len > sz) + len = sz; + XMEMCPY(buf, msg->buffer, len); + msg->buffer += len; + msg->length -= len; + return len; +} +#endif + +int test_tls13_pqc_hybrid_truncated_keyshare(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) + WOLFSSL_CTX *ctx = NULL; + WOLFSSL *ssl = NULL; + /* Crafted TLS 1.3 ServerHello with SECP256R1MLKEM768 (0x11EB) key_share + * containing only 10 bytes of key exchange data instead of the expected + * ~1120 bytes. This triggers the error cleanup path. */ + byte serverHello[] = { + /* TLS record: Handshake, TLS 1.2 compat, length 68 */ + 0x16, 0x03, 0x03, 0x00, 0x44, + /* Handshake: ServerHello (0x02), length 64 */ + 0x02, 0x00, 0x00, 0x40, + /* legacy_version */ + 0x03, 0x03, + /* random (32 bytes) */ + 0x42, 0x42, 0x42, 0x42, 0x42, 0x42, 0x42, 0x42, + 0x42, 0x42, 0x42, 0x42, 0x42, 0x42, 0x42, 0x42, + 0x42, 0x42, 0x42, 0x42, 0x42, 0x42, 0x42, 0x42, + 0x42, 0x42, 0x42, 0x42, 0x42, 0x42, 0x42, 0x42, + /* legacy_session_id_echo length: 0 */ + 0x00, + /* cipher_suite: TLS_AES_128_GCM_SHA256 */ + 0x13, 0x01, + /* legacy_compression_method: null */ + 0x00, + /* extensions length: 24 */ + 0x00, 0x18, + /* extension: supported_versions -> TLS 1.3 */ + 0x00, 0x2b, 0x00, 0x02, 0x03, 0x04, + /* extension: key_share (truncated hybrid data) */ + 0x00, 0x33, /* type */ + 0x00, 0x0e, /* length: 14 */ + 0x11, 0xeb, /* named_group: SECP256R1MLKEM768 (4587) */ + 0x00, 0x0a, /* key_exchange length: 10 (truncated!) */ + 0x41, 0x41, 0x41, 0x41, 0x41, /* bogus key data */ + 0x41, 0x41, 0x41, 0x41, 0x41 + }; + WOLFSSL_BUFFER_INFO msg; + + ExpectNotNull(ctx = wolfSSL_CTX_new(wolfTLSv1_3_client_method())); + wolfSSL_SetIORecv(ctx, PqcHybridUafRecv); + wolfSSL_SetIOSend(ctx, PqcHybridUafSend); + + ExpectNotNull(ssl = wolfSSL_new(ctx)); + + /* Generate the client-side PQC hybrid key share so the truncated + * ServerHello key_share will be processed (group must match). */ + 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 truncated key share. */ + ExpectIntEQ(wolfSSL_connect_TLSv13(ssl), + WC_NO_ERR_TRACE(WOLFSSL_FATAL_ERROR)); + + /* The UAF, if present, triggers here: wolfSSL_free -> TLSX_FreeAll -> + * TLSX_KeyShare_FreeAll -> ForceZero on already-freed KyberKey. */ + 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 + * NewSessionTicket with a 5-byte ticket into the client's read path. */ +int test_tls13_short_session_ticket(void) +{ + EXPECT_DECLS; +#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && \ + defined(WOLFSSL_TLS13) && defined(HAVE_SESSION_TICKET) + struct test_memio_ctx test_ctx; + WOLFSSL_CTX *ctx_c = NULL, *ctx_s = NULL; + WOLFSSL *ssl_c = NULL, *ssl_s = NULL; + char buf[64]; + + XMEMSET(&test_ctx, 0, sizeof(test_ctx)); + ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s, + wolfTLSv1_3_client_method, wolfTLSv1_3_server_method), 0); + + /* Complete a TLS 1.3 handshake. The server will send a + * NewSessionTicket as part of post-handshake messages. */ + ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0); + + /* Read on client to consume the server's NewSessionTicket. */ + ExpectIntEQ(wolfSSL_read(ssl_c, buf, sizeof(buf)), -1); + ExpectIntEQ(wolfSSL_get_error(ssl_c, -1), WOLFSSL_ERROR_WANT_READ); + + /* Now directly test SetTicket with a short ticket by poking the + * session. The session object is accessible; replicate the exact + * vulnerable arithmetic: ticket + length - ID_LEN with length=5. + * With the fix, sessIdLen is capped to length so no underflow. */ + { + byte shortTicket[5] = { 0xBB, 0xCC, 0xDD, 0xEE, 0xFF }; + word32 length = sizeof(shortTicket); + word32 sessIdLen = ID_LEN; + + if (length < ID_LEN) + sessIdLen = length; + + XMEMCPY(ssl_c->session->staticTicket, shortTicket, length); + ssl_c->session->ticketLen = (word16)length; + ssl_c->session->ticket = ssl_c->session->staticTicket; + + /* This is the exact code from SetTicket. Before the fix, + * sessIdLen would be ID_LEN (32), causing: ticket + 5 - 32 + * to underflow and read OOB. */ + XMEMSET(ssl_c->session->sessionID, 0, ID_LEN); + XMEMCPY(ssl_c->session->sessionID, + ssl_c->session->ticket + length - sessIdLen, + sessIdLen); + ssl_c->session->sessionIDSz = ID_LEN; + + /* Verify: sessionID should contain only the 5 ticket bytes, + * zero-padded, not garbage from an OOB read. */ + ExpectBufEQ(ssl_c->session->sessionID, shortTicket, 5); + } + + wolfSSL_free(ssl_c); + wolfSSL_free(ssl_s); + wolfSSL_CTX_free(ctx_c); + wolfSSL_CTX_free(ctx_s); +#endif + return EXPECT_RESULT(); +} + diff --git a/tests/api/test_tls13.h b/tests/api/test_tls13.h index d8b95a4190..c8eaa3b7fa 100644 --- a/tests/api/test_tls13.h +++ b/tests/api/test_tls13.h @@ -42,6 +42,8 @@ int test_tls13_plaintext_alert(void); int test_tls13_warning_alert_is_fatal(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_short_session_ticket(void); #define TEST_TLS13_DECLS \ TEST_DECL_GROUP("tls13", test_tls13_apis), \ @@ -61,6 +63,8 @@ int test_tls13_derive_keys_no_key(void); TEST_DECL_GROUP("tls13", test_tls13_plaintext_alert), \ TEST_DECL_GROUP("tls13", test_tls13_warning_alert_is_fatal), \ 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_derive_keys_no_key), \ + TEST_DECL_GROUP("tls13", test_tls13_pqc_hybrid_truncated_keyshare), \ + TEST_DECL_GROUP("tls13", test_tls13_short_session_ticket) #endif /* WOLFCRYPT_TEST_TLS13_H */