Hi,

On Thu, Sep 07, 2023 at 07:13:54PM +0400, Sergey Kandaurov wrote:
> # HG changeset patch
> # User Sergey Kandaurov <pluknet@nginx.com>
> # Date 1694041352 -14400
> # Thu Sep 07 03:02:32 2023 +0400
> # Node ID 02c86eac80c907adb7790c79ac6892afabcee5f4
> # Parent be1862a28fd8575a88475215ccfce995e392dfab
> QUIC: added check to prevent packet output with discarded keys.
>
> In addition to triggering alert, it ensures that such packets won't be sent.
>
> With the previous change that marks server keys as discarded by zeroing the
> key lengh, it is now an error to send packets with discarded keys. OpenSSL
> based stacks tolerate such behaviour because key length isn't used in packet
> protection, but BoringSSL will raise the UNSUPPORTED_KEY_SIZE cipher error.
> It won't be possible to use discarded keys with reused crypto contexts.
>
> diff --git a/src/event/quic/ngx_event_quic_output.c b/src/event/quic/ngx_event_quic_output.c
> --- a/src/event/quic/ngx_event_quic_output.c
> +++ b/src/event/quic/ngx_event_quic_output.c
> @@ -519,6 +519,21 @@ ngx_quic_output_packet(ngx_connection_t
>
> qc = ngx_quic_get_connection(c);
>
> + if (!ngx_quic_keys_available(qc->keys, ctx->level, 1)) {
> + ngx_log_error(NGX_LOG_ALERT, c->log, 0, "quic %s write keys discarded",
> + ngx_quic_level_name(ctx->level));
> +
> + while (!ngx_queue_empty(&ctx->frames)) {
> + q = ngx_queue_head(&ctx->frames);
> + ngx_queue_remove(q);
> +
> + f = ngx_queue_data(q, ngx_quic_frame_t, queue);
> + ngx_quic_free_frame(c, f);
> + }
> +
> + return 0;
> + }
> +
> ngx_quic_init_packet(c, ctx, &pkt, qc->path);
>
> min_payload = ngx_quic_payload_size(&pkt, min);
> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel

In ngx_quic_create_datagrams(), before calling ngx_quic_output_packet(),
ngx_quic_generate_ack() generates ACK frames for the current level.
Maybe it's better to add this block before generating ACKs? Otherwise we
generate an ACK and then immediately remove it. This technically would require
a similar block in ngx_quic_create_segments(), although the application level
should normally never be in a discarded key situation.

It's true however that the next patch partially solves the issue by not
generating the last handshake ACK. That however does not seem to be a complete
solution since there could be previous ACKs. In my opinion, this patch
provides a better and more general solution than the next one.

--
Roman Arutyunyan
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel