Fix PQC hybrid KeyShare pointer sanity.

Also add tests for it and SetTicket
This commit is contained in:
Anthony Hu
2026-03-27 14:33:41 -04:00
parent 5b1d2d795c
commit 46f6320380
3 changed files with 189 additions and 1 deletions
+12
View File
@@ -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);
+172
View File
@@ -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();
}
+5 -1
View File
@@ -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 */