From: Frédéric Lécaille Date: Wed, 1 Feb 2023 16:56:57 +0000 (+0100) Subject: MEDIUM: quic: Remove qc_conn_finalize() from the ClientHello TLS callbacks X-Git-Tag: v2.8-dev3~28 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=af25a69c8b0faa57a27dd5e9a99cbbe350ee6b07;p=thirdparty%2Fhaproxy.git MEDIUM: quic: Remove qc_conn_finalize() from the ClientHello TLS callbacks This is a bad idea to make the TLS ClientHello callback call qc_conn_finalize(). If this latter fails, this would generate a TLS alert and make the connection send packet whereas it is not functional. But qc_conn_finalize() job was to install the transport parameters sent by the QUIC listener. This installation cannot be done at any time. This must be done after having possibly negotiated the QUIC version and before sending the first Handshake packets. It seems the better moment to do that in when the Handshake TX secrets are derived. This has been found inspecting the ngtcp2 code. Calling SSL_set_quic_transport_params() too late would make the ServerHello to be sent without the transport parameters. The code for the connection update which was done from qc_conn_finalize() has been moved to quic_transport_params_store(). So, this update is done as soon as possible. Add QUIC_FL_CONN_TX_TP_RECEIVED to flag the connection as having received the peer transport parameters. Indeed this is required when the ClientHello message is splitted between packets. Add QUIC_FL_CONN_FINALIZED to protect the connection from calling qc_conn_finalize() more than one time. This latter is called only when the connection has received the transport parameters and after returning from SSL_do_hanshake() which is the function which trigger the TLS ClientHello callback call. Remove the calls to qc_conn_finalize() from from the TLS ClientHello callbacks. Must be backported to 2.6. and 2.7. --- diff --git a/include/haproxy/quic_conn-t.h b/include/haproxy/quic_conn-t.h index fa4ff54a6d..f2c7680e47 100644 --- a/include/haproxy/quic_conn-t.h +++ b/include/haproxy/quic_conn-t.h @@ -618,11 +618,14 @@ enum qc_mux_state { /* gap here */ #define QUIC_FL_CONN_HALF_OPEN_CNT_DECREMENTED (1U << 11) /* The half-open connection counter was decremented */ #define QUIC_FL_CONN_HANDSHAKE_SPEED_UP (1U << 12) /* Handshake speeding up was done */ +#define QUIC_FL_CONN_TX_TP_RECEIVED (1U << 25) /* Peer transport parameters have been received (used for the transmitting part) */ +#define QUIC_FL_CONN_FINALIZED (1U << 26) /* QUIC connection finalized (functional, ready to send/receive) */ #define QUIC_FL_CONN_NOTIFY_CLOSE (1U << 27) /* MUX notified about quic-conn imminent closure (idle-timeout or CONNECTION_CLOSE emission/reception) */ #define QUIC_FL_CONN_EXP_TIMER (1U << 28) /* timer has expired, quic-conn can be freed */ #define QUIC_FL_CONN_CLOSING (1U << 29) #define QUIC_FL_CONN_DRAINING (1U << 30) #define QUIC_FL_CONN_IMMEDIATE_CLOSE (1U << 31) + struct quic_conn { const struct quic_version *original_version; const struct quic_version *negotiated_version; diff --git a/include/haproxy/quic_conn.h b/include/haproxy/quic_conn.h index b5f25d865c..5e92afbee8 100644 --- a/include/haproxy/quic_conn.h +++ b/include/haproxy/quic_conn.h @@ -49,7 +49,6 @@ extern struct pool_head *pool_head_quic_connection_id; -int qc_conn_finalize(struct quic_conn *qc, int server); int ssl_quic_initial_ctx(struct bind_conf *bind_conf); /* Return the long packet type matching with version and */ diff --git a/src/quic_conn.c b/src/quic_conn.c index 79c20137c7..904e6699be 100644 --- a/src/quic_conn.c +++ b/src/quic_conn.c @@ -1002,6 +1002,22 @@ write: goto leave; } + if (level == ssl_encryption_handshake && qc_is_listener(qc)) { + qc->enc_params_len = + quic_transport_params_encode(qc->enc_params, + qc->enc_params + sizeof qc->enc_params, + &qc->rx.params, ver, 1); + if (!qc->enc_params_len) { + TRACE_ERROR("quic_transport_params_encode() failed", QUIC_EV_CONN_RWSEC); + goto leave; + } + + if (!SSL_set_quic_transport_params(qc->xprt_ctx->ssl, qc->enc_params, qc->enc_params_len)) { + TRACE_ERROR("SSL_set_quic_transport_params() failed", QUIC_EV_CONN_RWSEC); + goto leave; + } + } + if (level == ssl_encryption_application) { struct quic_tls_kp *prv_rx = &qc->ku.prv_rx; struct quic_tls_kp *nxt_rx = &qc->ku.nxt_rx; @@ -2216,6 +2232,41 @@ static forceinline void qc_ssl_dump_errors(struct connection *conn) int ssl_sock_get_alpn(const struct connection *conn, void *xprt_ctx, const char **str, int *len); +/* Finalize QUIC connection: + * - initialize the Initial QUIC TLS context for negotiated version, + * - derive the secrets for this context, + * - set them into the TLS stack, + * + * MUST be called after having received the remote transport parameters which + * are parsed when the TLS callback for the ClientHello message is called upon + * SSL_do_handshake() calls, not necessarily at the first time as this TLS + * message may be splitted between packets + * Return 1 if succeeded, 0 if not. + */ +static int qc_conn_finalize(struct quic_conn *qc, int server) +{ + int ret = 0; + + TRACE_ENTER(QUIC_EV_CONN_NEW, qc); + + if (qc->flags & QUIC_FL_CONN_FINALIZED) + goto finalized; + + if (qc->negotiated_version && + !qc_new_isecs(qc, &qc->negotiated_ictx, qc->negotiated_version, + qc->odcid.data, qc->odcid.len, server)) + goto out; + + /* This connection is functional (ready to send/receive) */ + qc->flags |= QUIC_FL_CONN_FINALIZED; + + finalized: + ret = 1; + out: + TRACE_LEAVE(QUIC_EV_CONN_NEW, qc); + return ret; +} + /* Provide CRYPTO data to the TLS stack found at with as length * from encryption level with as QUIC connection context. * Remaining parameter are there for debugging purposes. @@ -2252,6 +2303,16 @@ static inline int qc_provide_cdata(struct quic_enc_level *el, state = qc->state; if (state < QUIC_HS_ST_COMPLETE) { ssl_err = SSL_do_handshake(ctx->ssl); + + /* Finalize the connection as soon as possible if the peer transport parameters + * have been received. This may be useful to send packets even if this + * handshake fails. + */ + if ((qc->flags & QUIC_FL_CONN_TX_TP_RECEIVED) && !qc_conn_finalize(qc, 1)) { + TRACE_ERROR("connection finalization failed", QUIC_EV_CONN_IO_CB, qc, &state); + goto leave; + } + if (ssl_err != 1) { ssl_err = SSL_get_error(ctx->ssl, ssl_err); if (ssl_err == SSL_ERROR_WANT_READ || ssl_err == SSL_ERROR_WANT_WRITE) { @@ -5877,71 +5938,6 @@ static int qc_ssl_sess_init(struct quic_conn *qc, SSL_CTX *ssl_ctx, SSL **ssl, goto leave; } -/* Finalize QUIC connection: - * - initialize the Initial QUIC TLS context for negotiated version, - * - derive the secrets for this context, - * - encode the transport parameters to be sent, - * - set them into the TLS stack, - * - initialize ->max_ack_delay and max_idle_timeout, - * - * MUST be called after having received the remote transport parameters. - * Return 1 if succeeded, 0 if not. - */ -int qc_conn_finalize(struct quic_conn *qc, int server) -{ - int ret = 0; - struct quic_transport_params *tx_tp = &qc->tx.params; - struct quic_transport_params *rx_tp = &qc->rx.params; - const struct quic_version *ver; - - TRACE_ENTER(QUIC_EV_CONN_NEW, qc); - - if (tx_tp->version_information.negotiated_version && - tx_tp->version_information.negotiated_version != qc->original_version) { - qc->negotiated_version = - qc->tx.params.version_information.negotiated_version; - if (!qc_new_isecs(qc, &qc->negotiated_ictx, qc->negotiated_version, - qc->odcid.data, qc->odcid.len, !server)) - goto out; - - ver = qc->negotiated_version; - } - else { - ver = qc->original_version; - } - - qc->enc_params_len = - quic_transport_params_encode(qc->enc_params, - qc->enc_params + sizeof qc->enc_params, - &qc->rx.params, ver, 1); - if (!qc->enc_params_len) { - TRACE_ERROR("quic_transport_params_encode() failed", QUIC_EV_CONN_TXPKT); - goto out; - } - - if (!SSL_set_quic_transport_params(qc->xprt_ctx->ssl, qc->enc_params, qc->enc_params_len)) { - TRACE_ERROR("SSL_set_quic_transport_params() failed", QUIC_EV_CONN_TXPKT); - goto out; - } - - if (tx_tp->max_ack_delay) - qc->max_ack_delay = tx_tp->max_ack_delay; - - if (tx_tp->max_idle_timeout && rx_tp->max_idle_timeout) - qc->max_idle_timeout = - QUIC_MIN(tx_tp->max_idle_timeout, rx_tp->max_idle_timeout); - else - qc->max_idle_timeout = - QUIC_MAX(tx_tp->max_idle_timeout, rx_tp->max_idle_timeout); - - TRACE_PROTO("\nTX(remote) transp. params.", QUIC_EV_TRANSP_PARAMS, qc, tx_tp); - - ret = 1; - out: - TRACE_LEAVE(QUIC_EV_CONN_NEW, qc); - return ret; -} - /* Allocate the ssl_sock_ctx from connection . This creates the tasklet * used to process received packets. The allocated context is stored in * . diff --git a/src/quic_tp.c b/src/quic_tp.c index 31111f2ca1..85e2870ee8 100644 --- a/src/quic_tp.c +++ b/src/quic_tp.c @@ -625,6 +625,7 @@ int quic_transport_params_store(struct quic_conn *qc, int server, const unsigned char *end) { struct quic_transport_params *tx_params = &qc->tx.params; + struct quic_transport_params *rx_params = &qc->rx.params; /* initialize peer TPs to RFC default value */ quic_dflt_transport_params_cpy(tx_params); @@ -632,6 +633,23 @@ int quic_transport_params_store(struct quic_conn *qc, int server, if (!quic_transport_params_decode(tx_params, server, buf, end)) return 0; + /* Update the connection from transport parameters received */ + if (tx_params->version_information.negotiated_version && + tx_params->version_information.negotiated_version != qc->original_version) + qc->negotiated_version = + qc->tx.params.version_information.negotiated_version; + + if (tx_params->max_ack_delay) + qc->max_ack_delay = tx_params->max_ack_delay; + + if (tx_params->max_idle_timeout && rx_params->max_idle_timeout) + qc->max_idle_timeout = + QUIC_MIN(tx_params->max_idle_timeout, rx_params->max_idle_timeout); + else + qc->max_idle_timeout = + QUIC_MAX(tx_params->max_idle_timeout, rx_params->max_idle_timeout); + TRACE_PROTO("\nTX(remote) transp. params.", QUIC_EV_TRANSP_PARAMS, qc, tx_params); + return 1; } diff --git a/src/ssl_sock.c b/src/ssl_sock.c index 51d2d70464..7c3c152a57 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -2361,9 +2361,10 @@ int ssl_sock_switchctx_cbk(SSL *ssl, int *al, void *arg) } if (!quic_transport_params_store(qc, 0, extension_data, - extension_data + extension_len) || - !qc_conn_finalize(qc, 0)) + extension_data + extension_len)) goto abort; + + qc->flags |= QUIC_FL_CONN_TX_TP_RECEIVED; } #endif /* USE_QUIC */ @@ -2657,10 +2658,10 @@ int ssl_sock_switchctx_cbk(SSL *ssl, int *al, void *priv) } if (!quic_transport_params_store(qc, 0, extension_data, - extension_data + extension_len) || - !qc_conn_finalize(qc, 0)) { + extension_data + extension_len)) return SSL_TLSEXT_ERR_NOACK; - } + + qc->flags |= QUIC_FL_CONN_TX_TP_RECEIVED; } #endif /* USE_QUIC */