[PATCH] QUIC: posted generating TLS Key Update next keys

[Sergey Kandaurov]

> On 21 Aug 2023, at 20:17, Sergey Kandaurov <pluknet at nginx.com> wrote:
> 
> # HG changeset patch
> # User Sergey Kandaurov <pluknet at nginx.com>
> # Date 1692634530 -14400
> #      Mon Aug 21 20:15:30 2023 +0400
> # Node ID 4215a1d1426c23a06df6229b986d36e838594a91
> # Parent  44536076405cf79ebdd82a6a0ab27bb3aed86b04
> QUIC: posted generating TLS Key Update next keys.
> 
> Since at least f9fbeb4ee0de, which TLS Key Update support predates,
> queued data output is deferred to a posted push handler.  To address
> timing signals, generating next keys is posted after handling frames,
> to run after posted push handler.
> 
> diff --git a/src/event/quic/ngx_event_quic.c b/src/event/quic/ngx_event_quic.c
> --- a/src/event/quic/ngx_event_quic.c
> +++ b/src/event/quic/ngx_event_quic.c
> @@ -283,6 +283,10 @@ ngx_quic_new_connection(ngx_connection_t
>     qc->path_validation.data = c;
>     qc->path_validation.handler = ngx_quic_path_handler;
> 
> +    qc->key_update.log = c->log;
> +    qc->key_update.data = c;
> +    qc->key_update.handler = ngx_quic_keys_update;
> +
>     qc->conf = conf;
> 
>     if (ngx_quic_init_transport_params(&qc->tp, conf) != NGX_OK) {

A posted event should be removed on connection close, as otherwise
posting the event may produce use-after-free, which will happen on
ngx_quic_push_handler -> ngx_quic_output error path.
The patch to address this (to be folded to the changeset):

diff --git a/src/event/quic/ngx_event_quic.c b/src/event/quic/ngx_event_quic.c
--- a/src/event/quic/ngx_event_quic.c
+++ b/src/event/quic/ngx_event_quic.c
@@ -566,6 +566,10 @@ ngx_quic_close_connection(ngx_connection
         ngx_delete_posted_event(&qc->push);
     }
 
+    if (qc->key_update.posted) {
+        ngx_delete_posted_event(&qc->key_update);
+    }
+
     if (qc->close.timer_set) {
         return;
     }

> @@ -1055,7 +1059,9 @@ ngx_quic_handle_payload(ngx_connection_t
>         return rc;
>     }
> 
> -    return ngx_quic_keys_update(c, qc->keys);
> +    ngx_post_event(&qc->key_update, &ngx_posted_events);
> +
> +    return NGX_OK;
> }
> 
> 
> diff --git a/src/event/quic/ngx_event_quic_connection.h b/src/event/quic/ngx_event_quic_connection.h
> --- a/src/event/quic/ngx_event_quic_connection.h
> +++ b/src/event/quic/ngx_event_quic_connection.h
> @@ -230,6 +230,8 @@ struct ngx_quic_connection_s {
>     ngx_event_t                       pto;
>     ngx_event_t                       close;
>     ngx_event_t                       path_validation;
> +    ngx_event_t                       key_update;
> +
>     ngx_msec_t                        last_cc;
> 
>     ngx_msec_t                        first_rtt;
> diff --git a/src/event/quic/ngx_event_quic_protection.c b/src/event/quic/ngx_event_quic_protection.c
> --- a/src/event/quic/ngx_event_quic_protection.c
> +++ b/src/event/quic/ngx_event_quic_protection.c
> @@ -700,13 +700,20 @@ ngx_quic_keys_switch(ngx_connection_t *c
> }
> 
> 
> -ngx_int_t
> -ngx_quic_keys_update(ngx_connection_t *c, ngx_quic_keys_t *keys)
> +void
> +ngx_quic_keys_update(ngx_event_t *ev)
> {
> -    ngx_uint_t           i;
> -    ngx_quic_hkdf_t      seq[6];
> -    ngx_quic_ciphers_t   ciphers;
> -    ngx_quic_secrets_t  *current, *next;
> +    ngx_uint_t              i;
> +    ngx_quic_hkdf_t         seq[6];
> +    ngx_quic_keys_t        *keys;
> +    ngx_connection_t       *c;
> +    ngx_quic_ciphers_t      ciphers;
> +    ngx_quic_secrets_t     *current, *next;
> +    ngx_quic_connection_t  *qc;
> +
> +    c = ev->data;
> +    qc = ngx_quic_get_connection(c);
> +    keys = qc->keys;
> 
>     current = &keys->secrets[ssl_encryption_application];
>     next = &keys->next_key;
> @@ -716,7 +723,7 @@ ngx_quic_keys_update(ngx_connection_t *c
>     if (ngx_quic_ciphers(keys->cipher, &ciphers, ssl_encryption_application)
>         == NGX_ERROR)
>     {
> -        return NGX_ERROR;
> +        goto failed;
>     }
> 
>     next->client.secret.len = current->client.secret.len;
> @@ -744,11 +751,15 @@ ngx_quic_keys_update(ngx_connection_t *c
> 
>     for (i = 0; i < (sizeof(seq) / sizeof(seq[0])); i++) {
>         if (ngx_quic_hkdf_expand(&seq[i], ciphers.d, c->log) != NGX_OK) {
> -            return NGX_ERROR;
> +            goto failed;
>         }
>     }
> 
> -    return NGX_OK;
> +    return;
> +
> +failed:
> +
> +    ngx_quic_close_connection(c, NGX_ERROR);
> }
> 
> 
> diff --git a/src/event/quic/ngx_event_quic_protection.h b/src/event/quic/ngx_event_quic_protection.h
> --- a/src/event/quic/ngx_event_quic_protection.h
> +++ b/src/event/quic/ngx_event_quic_protection.h
> @@ -99,7 +99,7 @@ ngx_uint_t ngx_quic_keys_available(ngx_q
> void ngx_quic_keys_discard(ngx_quic_keys_t *keys,
>     enum ssl_encryption_level_t level);
> void ngx_quic_keys_switch(ngx_connection_t *c, ngx_quic_keys_t *keys);
> -ngx_int_t ngx_quic_keys_update(ngx_connection_t *c, ngx_quic_keys_t *keys);
> +void ngx_quic_keys_update(ngx_event_t *ev);
> ngx_int_t ngx_quic_encrypt(ngx_quic_header_t *pkt, ngx_str_t *res);
> ngx_int_t ngx_quic_decrypt(ngx_quic_header_t *pkt, uint64_t *largest_pn);
> void ngx_quic_compute_nonce(u_char *nonce, size_t len, uint64_t pn);
> diff --git a/src/event/quic/ngx_event_quic_ssl.c b/src/event/quic/ngx_event_quic_ssl.c
> --- a/src/event/quic/ngx_event_quic_ssl.c
> +++ b/src/event/quic/ngx_event_quic_ssl.c
> @@ -482,9 +482,7 @@ ngx_quic_crypto_input(ngx_connection_t *
>      * Generating next keys before a key update is received.
>      */
> 
> -    if (ngx_quic_keys_update(c, qc->keys) != NGX_OK) {
> -        return NGX_ERROR;
> -    }
> +    ngx_post_event(&qc->key_update, &ngx_posted_events);
> 
>     /*
>      * RFC 9001, 4.9.2.  Discarding Handshake Keys

-- 
Sergey Kandaurov