RepoMirrors/haproxy
1
0
Fork 0
mirror of http://git.haproxy.org/git/haproxy.git/ synced 2025-02-22 21:56:55 +00:00
Code Issues Packages Projects Releases Wiki Activity

BUG/MEDIUM: quic: Possible use of uninitialized <odcid> variable in qc_lstnr_params_init()

Browse Source 
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.
This commit is contained in:
Frdric Lcaille 2022-08-11 17:24:38 +02:00 committed by Willy Tarreau
parent 6bdf9367fb
commit e9325e97c2
3 changed files with 31 additions and 27 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
include/haproxy/quic_tp.h
View File

@ -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.

12
src/quic_tp.c
View File

@ -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;

44
src/xprt_quic.c
View File

@ -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;