From: Frédéric Lécaille Date: Mon, 3 Jul 2023 08:28:33 +0000 (+0200) Subject: BUG/MINOR: quic: Possible leak when allocating an encryption level X-Git-Tag: v2.9-dev2~91 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=0e53cb07a5101969eddf26825a19411917139685;p=thirdparty%2Fhaproxy.git BUG/MINOR: quic: Possible leak when allocating an encryption level This bug was reported by GH #2200 (coverity issue) as follows: *** CID 1516590: Resource leaks (RESOURCE_LEAK) /src/quic_tls.c: 159 in quic_conn_enc_level_init() 153 154 LIST_APPEND(&qc->qel_list, &qel->list); 155 *el = qel; 156 ret = 1; 157 leave: 158 TRACE_LEAVE(QUIC_EV_CONN_CLOSE, qc); >>> CID 1516590: Resource leaks (RESOURCE_LEAK) >>> Variable "qel" going out of scope leaks the storage it points to. 159 return ret; 160 } 161 162 /* Uninitialize QUIC encryption level. Never fails. */ 163 void quic_conn_enc_level_uninit(struct quic_conn *qc, struct quic_enc_level *qel) 164 { This bug was introduced by this commit which has foolishly assumed the encryption level memory would be released after quic_conn_enc_level_init() has failed. This is no more possible because this object is dynamic and no more a static member of the QUIC connection object. Anyway, this patch modifies quic_conn_enc_level_init() to ensure this is no more leak when quic_conn_enc_level_init() fails calling quic_conn_enc_level_uninit() in case of memory allocation error. quic_conn_enc_level_uninit() code was moved without modification only to be defined before quic_conn_enc_level_init() There is no need to backport this. --- diff --git a/src/quic_tls.c b/src/quic_tls.c index cb4c37fb24..f80e486e6a 100644 --- a/src/quic_tls.c +++ b/src/quic_tls.c @@ -100,6 +100,25 @@ void quic_tls_secret_hexdump(struct buffer *buf, chunk_appendf(buf, "%02x", secret[i]); } +/* Uninitialize QUIC encryption level. Never fails. */ +void quic_conn_enc_level_uninit(struct quic_conn *qc, struct quic_enc_level *qel) +{ + int i; + + TRACE_ENTER(QUIC_EV_CONN_CLOSE, qc); + + for (i = 0; i < qel->tx.crypto.nb_buf; i++) { + if (qel->tx.crypto.bufs[i]) { + pool_free(pool_head_quic_crypto_buf, qel->tx.crypto.bufs[i]); + qel->tx.crypto.bufs[i] = NULL; + } + } + ha_free(&qel->tx.crypto.bufs); + quic_cstream_free(qel->cstream); + + TRACE_LEAVE(QUIC_EV_CONN_CLOSE, qc); +} + /* Initialize QUIC TLS encryption level with as level for QUIC * connection allocating everything needed. * @@ -120,6 +139,9 @@ static int quic_conn_enc_level_init(struct quic_conn *qc, if (!qel) goto leave; + qel->tx.crypto.bufs = NULL; + qel->tx.crypto.nb_buf = 0; + qel->cstream = NULL; qel->pktns = pktns; qel->level = level; quic_tls_ctx_reset(&qel->tls_ctx); @@ -131,11 +153,12 @@ static int quic_conn_enc_level_init(struct quic_conn *qc, /* TODO: use a pool */ qel->tx.crypto.bufs = malloc(sizeof *qel->tx.crypto.bufs); if (!qel->tx.crypto.bufs) - goto leave; + goto err; qel->tx.crypto.bufs[0] = pool_alloc(pool_head_quic_crypto_buf); if (!qel->tx.crypto.bufs[0]) - goto leave; + goto err; + qel->tx.crypto.bufs[0]->sz = 0; qel->tx.crypto.nb_buf = 1; @@ -148,7 +171,7 @@ static int quic_conn_enc_level_init(struct quic_conn *qc, else { qel->cstream = quic_cstream_new(qc); if (!qel->cstream) - goto leave; + goto err; } LIST_APPEND(&qc->qel_list, &qel->list); @@ -157,25 +180,11 @@ static int quic_conn_enc_level_init(struct quic_conn *qc, leave: TRACE_LEAVE(QUIC_EV_CONN_CLOSE, qc); return ret; -} -/* Uninitialize QUIC encryption level. Never fails. */ -void quic_conn_enc_level_uninit(struct quic_conn *qc, struct quic_enc_level *qel) -{ - int i; - - TRACE_ENTER(QUIC_EV_CONN_CLOSE, qc); - - for (i = 0; i < qel->tx.crypto.nb_buf; i++) { - if (qel->tx.crypto.bufs[i]) { - pool_free(pool_head_quic_crypto_buf, qel->tx.crypto.bufs[i]); - qel->tx.crypto.bufs[i] = NULL; - } - } - ha_free(&qel->tx.crypto.bufs); - quic_cstream_free(qel->cstream); - - TRACE_LEAVE(QUIC_EV_CONN_CLOSE, qc); + err: + quic_conn_enc_level_uninit(qc, qel); + pool_free(pool_head_quic_enc_level, qel); + goto leave; } /* Allocate a QUIC TLS encryption with as TLS stack encryption to be