BUG/MEDIUM: quic: Possible use of uninitialized <odcid> variable in qc_lstnr_params_i...

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/include/haproxy/quic_tp.h b/include/haproxy/quic_tp.h
index 192460217bb31f1c321c69cfa823b66115484347..d6b7acc2523aa233f20bdbe76b0a1fbc681b12d3 100644 (file)
--- a/include/haproxy/quic_tp.h
+++ b/include/haproxy/quic_tp.h
@@ -25,7 +25,7 @@ int qc_lstnr_params_init(struct quic_conn *qc,
                          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);
 
 /* Dump <cid> transport parameter connection ID value if present (non null length).
  * Used only for debugging purposes.

diff --git a/src/quic_tp.c b/src/quic_tp.c
index f83ed86811d4bcf1c68d6215a78a0ea9117dee1f..449f94e331fdfccafd208c3f0e8eba2192f642e4 100644 (file)
--- a/src/quic_tp.c
+++ b/src/quic_tp.c
@@ -634,8 +634,8 @@ int quic_transport_params_store(struct quic_conn *qc, int server,
 /* 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 @@ int qc_lstnr_params_init(struct quic_conn *qc,
                          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 @@ int qc_lstnr_params_init(struct quic_conn *qc,
        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 3d9f704f1e54efab95c2e7063f725df0420809b2..83a1aacaaed4e63116942a2483f8d6c8f7cc6dde 100644 (file)
--- a/src/xprt_quic.c
+++ b/src/xprt_quic.c
@@ -4600,7 +4600,7 @@ static int parse_retry_token(struct quic_conn *qc,
  */
 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 @@ static struct quic_conn *qc_new_conn(const struct quic_version *qv, int ipv4,
                                            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 @@ static void qc_lstnr_pkt_rcv(unsigned char *buf, const unsigned char *end,
        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,24 +5838,27 @@ static void qc_lstnr_pkt_rcv(unsigned char *buf, const unsigned char *end,
                        /* 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;
                                        }
+
+                                       HA_ATOMIC_INC(&prx_counters->retry_sent);
+                                       goto err;
                                }
-                               else {
-                                       check_token = 1;
-                               }
+                       }
+                       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;
@@ -5883,7 +5886,7 @@ static void qc_lstnr_pkt_rcv(unsigned char *buf, const unsigned char *end,
                        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 @@ static void qc_lstnr_pkt_rcv(unsigned char *buf, const unsigned char *end,
                                        goto drop;
                                }
 
+                               token_odcid = &odcid;
                                HA_ATOMIC_INC(&prx_counters->retry_validated);
                        }
                        else {
@@ -5943,7 +5947,7 @@ static void qc_lstnr_pkt_rcv(unsigned char *buf, const unsigned char *end,
                        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;