Sergey Kandaurov
October 13, 2023 11:10AM
> On 18 Sep 2023, at 11:08, Roman Arutyunyan <arut@nginx.com> wrote:
>
> 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);
>
> 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 patch is a catch all measure against using non-existing keys,
which should normally not happen, hence the alert log level.
Frames are removed, because there's no more sense to keep them
in the queue (and to avoid repetitive alerts).

The actual fix is the next patch in the series.

> This technically would require
> a similar block in ngx_quic_create_segments(),

This creates complexity for no reason, I would rather not.

> although the application level
> should normally never be in a discarded key situation.

Agree.
Although discarding all available keys is added in this series as part of
connection tear down, it should not affect ngx_quic_create_segments().

>
> It's true however that the next patch partially solves the issue by not
> generating the last handshake ACK.

Indeed, that patch appears incomplete, I will remove it.
What you suggest about ngx_quic_generate_ack() seems to be
the right direction, see below.

> That however does not seem to be a complete
> solution since there could be previous ACKs.

There should not be previous ACKs sitting in ctx->frames because
Initial and Handshake packets involved in establishing TLS handshake
are acknowledged immediately.

Though, I see other ways to generate ACKs, which my patch didn't
address. For example, client may send out of order packets, or
multiple packets with holes in packets numbers, resulting in too
many ACK ranges. In both cases, ACKs are generated directly in
ngx_quic_ack_packet(), without posting a push event. So, I added
keys availability check to ngx_quic_ack_packet() as a common place.

# HG changeset patch
# User Sergey Kandaurov <pluknet@nginx.com>
# Date 1697110950 -14400
# Thu Oct 12 15:42:30 2023 +0400
# Node ID 4586b12e6a73811dc60fa78e6c0a82af36ea2957
# Parent 96af5e3a2a3a4e6b11f0dc084c9a60d836de70c4
QUIC: prevented generating ACK frames with discarded keys.

Previously it was possible to generate ACK frames using formally discarded
protection keys, in particular, when acknowledging a client Handshake packet
used to complete the TLS handshake and to discard handshake protection keys.
As it happens late in packet processing, it could be possible to generate ACK
frames after the keys were already discarded.

ACK frames are generated from ngx_quic_ack_packet(), either using a posted
push event, which envolves ngx_quic_generate_ack() as a part of the final
packet assembling, or directly in ngx_quic_ack_packet(), such as when there
is no room to add a new ACK range or when the received packet is out of order.
The added keys availability check is used to avoid generating late ACK frames
in both cases.

diff --git a/src/event/quic/ngx_event_quic_ack.c b/src/event/quic/ngx_event_quic_ack.c
--- a/src/event/quic/ngx_event_quic_ack.c
+++ b/src/event/quic/ngx_event_quic_ack.c
@@ -907,6 +907,10 @@ ngx_quic_ack_packet(ngx_connection_t *c,
" nranges:%ui", pkt->pn, (int64_t) ctx->largest_range,
ctx->first_range, ctx->nranges);

+ if (!ngx_quic_keys_available(qc->keys, ctx->level, 1)) {
+ return NGX_OK;
+ }
+
prev_pending = ctx->pending_ack;

if (pkt->need_ack) {

--
Sergey Kandaurov
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Subject Author Views Posted

[PATCH 0 of 8] [quic] reusing crypto contexts, and more

Sergey Kandaurov 454 September 07, 2023 11:18AM

[PATCH 1 of 8] QUIC: split keys availability checks to read and write sides

Sergey Kandaurov 74 September 07, 2023 11:18AM

Re: [PATCH 1 of 8] QUIC: split keys availability checks to read and write sides

Roman Arutyunyan 72 September 21, 2023 09:30AM

[PATCH 2 of 8] QUIC: added check to prevent packet output with discarded keys

Sergey Kandaurov 68 September 07, 2023 11:18AM

Re: [PATCH 2 of 8] QUIC: added check to prevent packet output with discarded keys

Roman Arutyunyan 76 September 18, 2023 03:10AM

Re: [PATCH 2 of 8] QUIC: added check to prevent packet output with discarded keys

Sergey Kandaurov 85 October 13, 2023 11:10AM

[PATCH 3 of 8] QUIC: prevented output of ACK frame when discarding handshake keys

Sergey Kandaurov 68 September 07, 2023 11:18AM

[PATCH 4 of 8] QUIC: renamed protection functions

Sergey Kandaurov 66 September 07, 2023 11:18AM

Re: [PATCH 4 of 8] QUIC: renamed protection functions

Roman Arutyunyan 76 September 21, 2023 09:32AM

[PATCH 5 of 8] QUIC: reusing crypto contexts for packet protection

Sergey Kandaurov 73 September 07, 2023 11:18AM

Re: [PATCH 5 of 8] QUIC: reusing crypto contexts for packet protection

Roman Arutyunyan 81 September 19, 2023 09:54AM

Re: [PATCH 5 of 8] QUIC: reusing crypto contexts for packet protection

Sergey Kandaurov 69 October 13, 2023 11:14AM

Re: [PATCH 5 of 8] QUIC: reusing crypto contexts for packet protection

Sergey Kandaurov 69 October 17, 2023 06:40AM

Re: [PATCH 5 of 8] QUIC: reusing crypto contexts for packet protection

Sergey Kandaurov 85 October 23, 2023 06:38PM

[PATCH 6 of 8] QUIC: reusing crypto contexts for header protection

Sergey Kandaurov 69 September 07, 2023 11:18AM

Re: [PATCH 6 of 8] QUIC: reusing crypto contexts for header protection

Roman Arutyunyan 66 September 20, 2023 08:14AM

Re: [PATCH 6 of 8] QUIC: reusing crypto contexts for header protection

Sergey Kandaurov 63 October 13, 2023 11:14AM

[PATCH 7 of 8] QUIC: cleaned up now unused ngx_quic_ciphers() calls

Sergey Kandaurov 72 September 07, 2023 11:18AM

Re: [PATCH 7 of 8] QUIC: cleaned up now unused ngx_quic_ciphers() calls

Roman Arutyunyan 83 September 20, 2023 08:28AM

Re: [PATCH 7 of 8] QUIC: cleaned up now unused ngx_quic_ciphers() calls

Sergey Kandaurov 74 October 13, 2023 11:16AM

[PATCH 8 of 8] QUIC: explicitly zero out unused keying material

Sergey Kandaurov 67 September 07, 2023 11:18AM

Re: [PATCH 8 of 8] QUIC: explicitly zero out unused keying material

Roman Arutyunyan 70 September 21, 2023 09:30AM

Re: [PATCH 8 of 8] QUIC: explicitly zero out unused keying material

Sergey Kandaurov 62 October 13, 2023 11:16AM

[PATCH 00 of 11] [quic] reusing crypto contexts, and more #2

Sergey Kandaurov 68 October 18, 2023 11:28AM

[PATCH 01 of 11] QUIC: split keys availability checks to read and write sides

Sergey Kandaurov 66 October 18, 2023 11:28AM

[PATCH 02 of 11] QUIC: added safety belt to prevent using discarded keys

Sergey Kandaurov 66 October 18, 2023 11:28AM

[PATCH 03 of 11] QUIC: prevented generating ACK frames with discarded keys

Sergey Kandaurov 65 October 18, 2023 11:28AM

[PATCH 04 of 11] QUIC: renamed protection functions

Sergey Kandaurov 61 October 18, 2023 11:28AM

[PATCH 05 of 11] QUIC: reusing crypto contexts for packet protection

Sergey Kandaurov 65 October 18, 2023 11:28AM

[PATCH 06 of 11] QUIC: common code for crypto open and seal operations

Sergey Kandaurov 64 October 18, 2023 11:28AM

[PATCH 07 of 11] QUIC: reusing crypto contexts for header protection

Sergey Kandaurov 62 October 18, 2023 11:30AM

[PATCH 08 of 11] QUIC: cleaned up now unused ngx_quic_ciphers() calls

Sergey Kandaurov 62 October 18, 2023 11:30AM

[PATCH 09 of 11] QUIC: simplified ngx_quic_ciphers() API

Sergey Kandaurov 58 October 18, 2023 11:30AM

[PATCH 10 of 11] QUIC: removed key field from ngx_quic_secret_t

Sergey Kandaurov 64 October 18, 2023 11:30AM

[PATCH 11 of 11] QUIC: explicitly zero out unused keying material

Sergey Kandaurov 62 October 18, 2023 11:38AM

Re: [PATCH 00 of 11] [quic] reusing crypto contexts, and more #2

Roman Arutyunyan 68 October 20, 2023 03:28AM



Sorry, you do not have permission to post/reply in this forum.

Online Users

Guests: 254
Record Number of Users: 8 on April 13, 2023
Record Number of Guests: 421 on December 02, 2018
Powered by nginx      Powered by FreeBSD      PHP Powered      Powered by MariaDB      ipv6 ready