quic: fix NULL txl dereference in qtx_resize_txe
Fixes: 1957148384 "QUIC Record Layer (Refactor and TX Side)"
Reviewed-by: Saša Nedvědický <sashan@openssl.org>
Reviewed-by: Paul Dale <paul.dale@oracle.com>
Reviewed-by: Eugene Syromiatnikov <esyr@openssl.org>
MergeDate: Sat Apr 11 20:55:10 2026
(Merged from https://github.com/openssl/openssl/pull/30474)
This commit is contained in:
Nikolas Gauder
Nikola Pajkovsky
parent
4d487eb1ac
commit
16892155e1
2 changed files with 109 additions and 21 deletions
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue