commit 89e48ff92f3ac7645a4d90fc352b88aba8af802a
author Amaury Denoyelle <adenoyelle@haproxy.com> Wed Apr 19 10:04:41 2023 +0200
committer Amaury Denoyelle <adenoyelle@haproxy.com> Wed Apr 19 10:18:58 2023 +0200
tree 576cb5c29794fa81b49535d3302d361e6ad59053
parent 5003ac7fe9b3709daa624d087bb1cc2ee93a8a3a

BUG/MEDIUM: quic: prevent crash on Retry sending

The following commit introduced a regression :
  commit 1a5cc19cecfa84bfd0fdd7b9d5d20899cce40662
  MINOR: quic: adjust Rx packet type parsing

Since this commit, qv variable was left to NULL as version is stored
directly in quic_rx_packet instance. In most cases, this only causes
traces to skip version printing. However, qv is dereferenced when
sending a Retry which causes a segfault.

To fix this, simply remove qv variable and use pkt->version instead,
both for traces and send_retry() invocation.

This bug was detected thanks to QUIC interop runner. It can easily be
reproduced by using quic-force-retry on the bind line.

This must be backported up to 2.7.

src/quic_conn.c

diff --git a/src/quic_conn.c b/src/quic_conn.c
index b6f1467..02e4044 100644
--- a/src/quic_conn.c
+++ b/src/quic_conn.c
@@ -6867,7 +6867,6 @@
 	const unsigned char *beg = buf;
 	struct proxy *prx;
 	struct quic_counters *prx_counters;
-	const struct quic_version *qv = NULL;
 
 	TRACE_ENTER(QUIC_EV_CONN_LPKT);
 
@@ -6952,7 +6951,7 @@
 			if (!quic_dec_int(&token_len, (const unsigned char **)&buf, end) ||
 				end - buf < token_len) {
 				TRACE_PROTO("Packet dropped",
-				            QUIC_EV_CONN_LPKT, NULL, NULL, NULL, qv);
+				            QUIC_EV_CONN_LPKT, NULL, NULL, NULL, pkt->version);
 				goto drop;
 			}
 
@@ -6962,10 +6961,10 @@
 			if (global.cluster_secret && !token_len) {
 				if (l->bind_conf->options & BC_O_QUIC_FORCE_RETRY) {
 					TRACE_PROTO("Initial without token, sending retry",
-					            QUIC_EV_CONN_LPKT, NULL, NULL, NULL, qv);
-					if (send_retry(l->rx.fd, &dgram->saddr, pkt, qv)) {
+					            QUIC_EV_CONN_LPKT, NULL, NULL, NULL, pkt->version);
+					if (send_retry(l->rx.fd, &dgram->saddr, pkt, pkt->version)) {
 						TRACE_PROTO("Error during Retry generation",
-						            QUIC_EV_CONN_LPKT, NULL, NULL, NULL, qv);
+						            QUIC_EV_CONN_LPKT, NULL, NULL, NULL, pkt->version);
 						goto drop_silent;
 					}
 
@@ -6978,7 +6977,7 @@
 				 * cluster secret.
 				 */
 				TRACE_PROTO("Packet dropped", QUIC_EV_CONN_LPKT,
-				            NULL, NULL, NULL, qv);
+				            NULL, NULL, NULL, pkt->version);
 				goto drop;
 			}
 
@@ -6989,7 +6988,7 @@
 		else if (pkt->type != QUIC_PACKET_TYPE_0RTT) {
 			if (pkt->dcid.len != QUIC_HAP_CID_LEN) {
 				TRACE_PROTO("Packet dropped",
-				            QUIC_EV_CONN_LPKT, NULL, NULL, NULL, qv);
+				            QUIC_EV_CONN_LPKT, NULL, NULL, NULL, pkt->version);
 				goto drop;
 			}
 		}
@@ -6997,7 +6996,7 @@
 		if (!quic_dec_int(&len, (const unsigned char **)&buf, end) ||
 			end - buf < len) {
 			TRACE_PROTO("Packet dropped",
-			            QUIC_EV_CONN_LPKT, NULL, NULL, NULL, qv);
+			            QUIC_EV_CONN_LPKT, NULL, NULL, NULL, pkt->version);
 			goto drop;
 		}
 
@@ -7056,7 +7055,7 @@
 		pkt->len = end - beg;
 	}
 
-	TRACE_PROTO("RX pkt parsed", QUIC_EV_CONN_LPKT, NULL, pkt, NULL, qv);
+	TRACE_PROTO("RX pkt parsed", QUIC_EV_CONN_LPKT, NULL, pkt, NULL, pkt->version);
 	TRACE_LEAVE(QUIC_EV_CONN_LPKT);
 	return 0;
 
@@ -7065,7 +7064,7 @@
  drop_silent:
 	if (!pkt->len)
 		pkt->len = end - beg;
-	TRACE_PROTO("RX pkt parsing failed", QUIC_EV_CONN_LPKT, NULL, pkt, NULL, qv);
+	TRACE_PROTO("RX pkt parsing failed", QUIC_EV_CONN_LPKT, NULL, pkt, NULL, pkt->version);
 	TRACE_LEAVE(QUIC_EV_CONN_LPKT);
 	return -1;
 }