BUG/MEDIUM: quic: Possible use of uninitialized <odcid> variable in qc_lstnr_params_init()
When receiving a token into a client Initial packet without a cluster secret defined
by configuration, the <odcid> variable used to parse the ODCID from the token
could be used without having been initialized. Such a packet must be dropped. So
the sufficient part of this patch is this check:
+ }
+ else if (!global.cluster_secret && token_len) {
+ /* Impossible case: a token was received without configured
+ * cluster secret.
+ */
+ TRACE_PROTO("Packet dropped", QUIC_EV_CONN_LPKT,
+ NULL, NULL, NULL, qv);
+ goto drop;
}
Take the opportunity of this patch to rework and make it more readable this part
of code where such a packet must be dropped removing the <check_token> variable.
When an ODCID is parsed from a token, new <token_odcid> new pointer variable
is set to the address of the parsed ODCID. This way, is not set but used it will
make crash haproxy. This was not always the case with an uninitialized local
variable.
Adapt the API to used such a pointer variable: <token> boolean variable is removed
from qc_lstnr_params_init() prototype.
This must be backported to 2.6.
diff --git a/src/quic_tp.c b/src/quic_tp.c
index f83ed86..449f94e 100644
--- a/src/quic_tp.c
+++ b/src/quic_tp.c
@@ -634,8 +634,8 @@
/* QUIC server (or haproxy listener) only function.
* Initialize the local transport parameters <rx_params> from <listener_params>
* coming from configuration and Initial packet information (destintation
- * connection ID, source connection ID, original destination connection ID,
- * and if a token was present denoted by <token> boolean value.
+ * connection ID, source connection ID, original destination connection ID from
+ * client token.
* Returns 1 if succeeded, 0 if not.
*/
int qc_lstnr_params_init(struct quic_conn *qc,
@@ -643,7 +643,7 @@
const unsigned char *stateless_reset_token,
const unsigned char *dcid, size_t dcidlen,
const unsigned char *scid, size_t scidlen,
- const unsigned char *odcid, size_t odcidlen, int token)
+ const unsigned char *token_odcid, size_t token_odcidlen)
{
struct quic_transport_params *rx_params = &qc->rx.params;
struct tp_cid *odcid_param = &rx_params->original_destination_connection_id;
@@ -654,9 +654,9 @@
memcpy(rx_params->stateless_reset_token, stateless_reset_token,
sizeof rx_params->stateless_reset_token);
/* Copy original_destination_connection_id transport parameter. */
- if (token) {
- memcpy(odcid_param->data, odcid, odcidlen);
- odcid_param->len = odcidlen;
+ if (token_odcid) {
+ memcpy(odcid_param->data, token_odcid, token_odcidlen);
+ odcid_param->len = token_odcidlen;
/* Copy retry_source_connection_id transport parameter. */
memcpy(rx_params->retry_source_connection_id.data, dcid, dcidlen);
rx_params->retry_source_connection_id.len = dcidlen;
diff --git a/src/xprt_quic.c b/src/xprt_quic.c
index 3d9f704..83a1aac 100644
--- a/src/xprt_quic.c
+++ b/src/xprt_quic.c
@@ -4600,7 +4600,7 @@
*/
static struct quic_conn *qc_new_conn(const struct quic_version *qv, int ipv4,
struct quic_cid *dcid, struct quic_cid *scid,
- const struct quic_cid *odcid,
+ const struct quic_cid *token_odcid,
struct sockaddr_storage *saddr,
int server, int token, void *owner)
{
@@ -4726,7 +4726,7 @@
icid->stateless_reset_token,
dcid->data, dcid->len,
qc->scid.data, qc->scid.len,
- odcid->data, odcid->len, token))
+ token_odcid->data, token_odcid->len))
goto err;
if (qc_conn_alloc_ssl_ctx(qc) ||
@@ -5774,7 +5774,7 @@
if (long_header) {
uint64_t len;
struct quic_cid odcid;
- int check_token = 0;
+ struct quic_cid *token_odcid = NULL; // ODCID received from client token
TRACE_PROTO("long header packet received", QUIC_EV_CONN_LPKT, qc);
if (!quic_packet_read_long_header(&buf, end, pkt)) {
@@ -5838,25 +5838,28 @@
/* TODO Retry should be automatically activated if
* suspect network usage is detected.
*/
- if (global.cluster_secret) {
- if (!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)) {
- TRACE_PROTO("Error during Retry generation",
- QUIC_EV_CONN_LPKT, NULL, NULL, NULL, qv);
- goto err;
- }
-
- HA_ATOMIC_INC(&prx_counters->retry_sent);
+ 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)) {
+ TRACE_PROTO("Error during Retry generation",
+ QUIC_EV_CONN_LPKT, NULL, NULL, NULL, qv);
goto err;
}
- }
- else {
- check_token = 1;
+
+ HA_ATOMIC_INC(&prx_counters->retry_sent);
+ goto err;
}
}
+ else if (!global.cluster_secret && token_len) {
+ /* Impossible case: a token was received without configured
+ * cluster secret.
+ */
+ TRACE_PROTO("Packet dropped", QUIC_EV_CONN_LPKT,
+ NULL, NULL, NULL, qv);
+ goto drop;
+ }
pkt->token = buf;
pkt->token_len = token_len;
@@ -5883,7 +5886,7 @@
goto drop_no_conn;
qc = retrieve_qc_conn_from_cid(pkt, l, &dgram->saddr);
- if (check_token && pkt->token) {
+ if (global.cluster_secret && pkt->token_len) {
if (*pkt->token == QUIC_TOKEN_FMT_RETRY) {
const struct quic_version *ver = qc ? qc->original_version : qv;
if (!quic_retry_token_check(pkt->token, pkt->token_len, ver, &odcid,
@@ -5897,6 +5900,7 @@
goto drop;
}
+ token_odcid = &odcid;
HA_ATOMIC_INC(&prx_counters->retry_validated);
}
else {
@@ -5943,7 +5947,7 @@
pkt->saddr = dgram->saddr;
ipv4 = dgram->saddr.ss_family == AF_INET;
- qc = qc_new_conn(qv, ipv4, &pkt->dcid, &pkt->scid, &odcid,
+ qc = qc_new_conn(qv, ipv4, &pkt->dcid, &pkt->scid, token_odcid,
&pkt->saddr, 1, !!pkt->token_len, l);
if (qc == NULL)
goto drop;