From: Nikolas Gauder Date: Tue, 17 Mar 2026 19:29:28 +0000 (+0100) Subject: quic: fix NULL txl dereference in qtx_resize_txe X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=16892155e15340ba9725d01cd9593726376fd995;p=thirdparty%2Fopenssl.git quic: fix NULL txl dereference in qtx_resize_txe Fixes: 1957148384c7 "QUIC Record Layer (Refactor and TX Side)" Reviewed-by: Saša Nedvědický Reviewed-by: Paul Dale Reviewed-by: Eugene Syromiatnikov MergeDate: Sat Apr 11 20:55:10 2026 (Merged from https://github.com/openssl/openssl/pull/30474) --- diff --git a/ssl/quic/quic_record_tx.c b/ssl/quic/quic_record_tx.c index 8714437232..0dbdb1491a 100644 --- a/ssl/quic/quic_record_tx.c +++ b/ssl/quic/quic_record_tx.c @@ -269,9 +269,9 @@ static TXE *qtx_ensure_free_txe(OSSL_QTX *qtx, size_t alloc_len) * of the TXE might change; the new address is returned, or NULL on failure, in * which case the original TXE remains valid. */ -static TXE *qtx_resize_txe(OSSL_QTX *qtx, TXE_LIST *txl, TXE *txe, size_t n) +static TXE *qtx_resize_txe(OSSL_QTX *qtx, TXE *txe, size_t n) { - TXE *txe2, *p; + TXE *txe2; /* Should never happen. */ if (txe == NULL) @@ -280,27 +280,21 @@ static TXE *qtx_resize_txe(OSSL_QTX *qtx, TXE_LIST *txl, TXE *txe, size_t n) if (n >= SIZE_MAX - sizeof(TXE)) return NULL; - /* Remove the item from the list to avoid accessing freed memory */ - p = ossl_list_txe_prev(txe); - ossl_list_txe_remove(txl, txe); + /* + * to resize txe, the caller must detach it from the list first, + * fail if txe is still attached. + */ + if (!ossl_assert(ossl_list_txe_prev(txe) == NULL + && ossl_list_txe_next(txe) == NULL)) + return NULL; /* * NOTE: We do not clear old memory, although it does contain decrypted * data. */ txe2 = OPENSSL_realloc(txe, sizeof(TXE) + n); - if (txe2 == NULL) { - if (p == NULL) - ossl_list_txe_insert_head(txl, txe); - else - ossl_list_txe_insert_after(txl, p, txe); + if (txe2 == NULL) return NULL; - } - - if (p == NULL) - ossl_list_txe_insert_head(txl, txe2); - else - ossl_list_txe_insert_after(txl, p, txe2); if (qtx->cons == txe) qtx->cons = txe2; @@ -313,13 +307,12 @@ static TXE *qtx_resize_txe(OSSL_QTX *qtx, TXE_LIST *txl, TXE *txe, size_t n) * Ensure the data buffer attached to an TXE is at least n bytes in size. * Returns NULL on failure. */ -static TXE *qtx_reserve_txe(OSSL_QTX *qtx, TXE_LIST *txl, - TXE *txe, size_t n) +static TXE *qtx_reserve_txe(OSSL_QTX *qtx, TXE *txe, size_t n) { if (txe->alloc_len >= n) return txe; - return qtx_resize_txe(qtx, txl, txe, n); + return qtx_resize_txe(qtx, txe, n); } /* Move a TXE from pending to free. */ @@ -840,6 +833,16 @@ int ossl_qtx_write_pkt(OSSL_QTX *qtx, const OSSL_QTX_PKT *pkt) * serialize/encrypt the packet. We always encrypt packets as soon as * our caller gives them to us, which relieves the caller of any need to * keep the plaintext around. + * + * the txe can have three distinct states: + * - attached to free list + * - attached to tx list + * - detached. + * + * if txe is detached (not member of free/tx list), then it is kept + * in qtx->cons. The qtx_ensure_cons() here either returns the txe + * from free list or existing ->cons txe. The txe we obtain here + * is detached. */ txe = qtx_ensure_cons(qtx); if (txe == NULL) @@ -849,9 +852,20 @@ int ossl_qtx_write_pkt(OSSL_QTX *qtx, const OSSL_QTX_PKT *pkt) * Ensure TXE has at least MDPL bytes allocated. This should only be * possible if the MDPL has increased. */ - txe = qtx_reserve_txe(qtx, NULL, txe, qtx->mdpl); - if (txe == NULL) + txe = qtx_reserve_txe(qtx, txe, qtx->mdpl); + if (txe == NULL) { + /* + * realloc of txe failed. however it is still kept in ->cons, + * no memory leak. + * The question is what we should do here to handle error, + * is doing `return 0` enough? or shall we discard ->cons and + * put it back to free list? + * or just stop coalescing the packet and dispatch it to network + * right now so the next packet tx can start from fresh? + * I think this is the problem for another day. + */ return 0; + } if (!was_coalescing) { /* Set addresses in TXE. */ @@ -878,6 +892,11 @@ int ossl_qtx_write_pkt(OSSL_QTX *qtx, const OSSL_QTX_PKT *pkt) /* * We failed due to insufficient length, so end the current * datagram and try again. + * + * the ossl_qtx_finish_dgram() also puts the txe (-.cons) to + * tx list, so ->cons becomes attached again. The function also + * sets ->cons to NULL so the next loop iteration starts with + * fresh txe (which is also safe to resize). */ ossl_qtx_finish_dgram(qtx); was_coalescing = 0; diff --git a/test/quicapitest.c b/test/quicapitest.c index 84a5c9566f..0d79ceb7f2 100644 --- a/test/quicapitest.c +++ b/test/quicapitest.c @@ -19,6 +19,7 @@ #include "testutil.h" #include "testutil/output.h" #include "../ssl/ssl_local.h" +#include "../ssl/quic/quic_channel_local.h" #include "internal/quic_error.h" static OSSL_LIB_CTX *libctx = NULL; @@ -3517,6 +3518,73 @@ err: #endif } +static int test_quic_resize_txe(void) +{ + SSL_CTX *cctx = NULL; + SSL *clientquic = NULL; + QUIC_TSERVER *qtserv = NULL; + QUIC_CHANNEL *ch = NULL; + unsigned char msg[] = "resize test"; + unsigned char buf[sizeof(msg)]; + size_t numbytes = 0; + int ret = 0; + + if (!TEST_ptr(cctx = SSL_CTX_new_ex(libctx, NULL, OSSL_QUIC_client_method()))) + goto end; + + if (!TEST_true(qtest_create_quic_objects(libctx, cctx, NULL, + cert, privkey, 0, + &qtserv, &clientquic, + NULL, NULL))) + goto end; + + if (!TEST_true(qtest_create_quic_connection(qtserv, clientquic))) + goto end; + + /* + * Client writes first to open stream 0 (client-initiated bidirectional). + * The server must see the stream before it can write back on it. + */ + if (!TEST_true(SSL_write_ex(clientquic, msg, sizeof(msg), &numbytes)) + || !TEST_size_t_eq(numbytes, sizeof(msg))) + goto end; + + ossl_quic_tserver_tick(qtserv); + if (!TEST_true(ossl_quic_tserver_read(qtserv, 0, buf, sizeof(buf), + &numbytes))) + goto end; + + /* + * Increase the server's QTX MDPL above the initial allocation size + * (QUIC_MIN_INITIAL_DGRAM_LEN = 1200). All TXEs in the free list have + * alloc_len = 1200, so the next write will trigger qtx_resize_txe. + */ + ch = ossl_quic_tserver_get_channel(qtserv); + if (!TEST_true(ossl_qtx_set_mdpl(ch->qtx, + QUIC_MIN_INITIAL_DGRAM_LEN + 250))) + goto end; + + /* Trigger a server write: exercises qtx_resize_txe via qtx_reserve_txe */ + if (!TEST_true(ossl_quic_tserver_write(qtserv, 0, + msg, sizeof(msg), &numbytes)) + || !TEST_size_t_eq(numbytes, sizeof(msg))) + goto end; + + ossl_quic_tserver_tick(qtserv); + SSL_handle_events(clientquic); + + if (!TEST_true(SSL_read_ex(clientquic, buf, sizeof(buf), &numbytes)) + || !TEST_mem_eq(buf, numbytes, msg, sizeof(msg))) + goto end; + + ret = 1; +end: + ossl_quic_tserver_free(qtserv); + SSL_free(clientquic); + SSL_CTX_free(cctx); + return ret; +} + /***********************************************************************************/ OPT_TEST_DECLARE_USAGE("provider config certsdir datadir\n") @@ -3625,6 +3693,7 @@ int setup_tests(void) ADD_TEST(test_quic_peer_addr_v6); ADD_TEST(test_quic_peer_addr_v4); ADD_TEST(test_ech); + ADD_TEST(test_quic_resize_txe); return 1; err: