> On 22 May 2023, at 16:00, Roman Arutyunyan <arut@nginx.com> wrote:
>
> On Mon, May 22, 2023 at 03:28:24PM +0400, Sergey Kandaurov wrote:
>>
>>> On 21 May 2023, at 13:20, Roman Arutyunyan <arut@nginx.com> wrote:
>>>
>>> # HG changeset patch
>>> # User Roman Arutyunyan <arut@nginx.com>
>>> # Date 1684659758 -14400
>>> # Sun May 21 13:02:38 2023 +0400
>>> # Node ID 321b69f47a0a9874a063e464d13706339a706bb8
>>> # Parent 235d482ef6bc8c40a956b2413865d42c94e0fc05
>>> QUIC: fixed post-close use-after-free.
>>>
>>> Previously, ngx_quic_close_connection() could be called in a way that QUIC
>>> connection was accessed after the call. In most cases the connection is not
>>> closed right away, but close timeout is scheduled. However, it's not always
>>> the case. Also, if the close process started earlier for a different reason,
>>> calling ngx_quic_close_connection() may actually close the connection. The
>>> connection object should not be accessed after that. Particularly, accessing
>>> c->pool can result in null pointer dereference.
>>
>> The last sentence doesn't seem correct.
>> For that, c->pool should be NULL on destroy; not sure if we do this.
>
> Indeed. Removed this sentence.
>
>>> Now, when possible, return statement is added to eliminate post-close connection
>>> object access. In other places ngx_quic_close_connection() is substituted with
>>> posting close event.
>>>
>>> Prodded by Coverity (CID 1530402).
>>>
>>> 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
>>> @@ -844,7 +844,7 @@ ngx_quic_handle_packet(ngx_connection_t
>>> "quic stateless reset packet detected");
>>>
>>> qc->draining = 1;
>>> - ngx_quic_close_connection(c, NGX_OK);
>>> + ngx_post_event(&qc->close, &ngx_posted_events);
>>>
>>> return NGX_OK;
>>> }
>>
>> Stateless reset mimics a short packet and uses an entire datagram.
>> We handle short packets accordingly, expecting no further packets.
>> So there should be no issues with synchronous connection close,
>> except perhaps that we return NGX_OK here for seemingly no reason.
>> This theoretically may involve additional actions leading to post-close
>> use-after-free.
>
> We can return NGX_DONE, but it's still not good for calling functions to
> reference connection that's already freed. In current code this will
> result in logging packet handling result in a freed c->log. But even if
> we fix this, having a freed connection will one day lead to a problem.
>
>>> @@ -1390,7 +1390,7 @@ ngx_quic_handle_frames(ngx_connection_t
>>>
>>> if (do_close) {
>>> qc->draining = 1;
>>> - ngx_quic_close_connection(c, NGX_OK);
>>> + ngx_post_event(&qc->close, &ngx_posted_events);
>>> }
>>>
>>> if (pkt->path != qc->path && nonprobing) {
>>
>> Not sure if this one is practicable (but I don't object).
>> To make this call close connection immediately, qc->closing
>> should be already set but close timer unset. To speculate,
>> ngx_quic_close_connection() could be previously called with
>> some other rc, and ngx_quic_close_streams() returned NGX_AGAIN.
>
> Yes, that's unlikely, but possible.
>
>>> 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
>>> @@ -806,6 +806,7 @@ void ngx_quic_lost_handler(ngx_event_t *
>>>
>>> if (ngx_quic_detect_lost(c, NULL) != NGX_OK) {
>>> ngx_quic_close_connection(c, NGX_ERROR);
>>> + return;
>>> }
>>>
>>> ngx_quic_connstate_dbg(c);
>>> diff --git a/src/event/quic/ngx_event_quic_streams.c b/src/event/quic/ngx_event_quic_streams.c
>>> --- a/src/event/quic/ngx_event_quic_streams.c
>>> +++ b/src/event/quic/ngx_event_quic_streams.c
>>> @@ -1084,7 +1084,8 @@ ngx_quic_stream_cleanup_handler(void *da
>>> {
>>> ngx_connection_t *c = data;
>>>
>>> - ngx_quic_stream_t *qs;
>>> + ngx_quic_stream_t *qs;
>>> + ngx_quic_connection_t *qc;
>>>
>>> qs = c->quic;
>>>
>>> @@ -1092,16 +1093,23 @@ ngx_quic_stream_cleanup_handler(void *da
>>> "quic stream id:0x%xL cleanup", qs->id);
>>>
>>> if (ngx_quic_shutdown_stream(c, NGX_RDWR_SHUTDOWN) != NGX_OK) {
>>> - ngx_quic_close_connection(c, NGX_ERROR);
>>> - return;
>>> + goto failed;
>>> }
>>>
>>> qs->connection = NULL;
>>>
>>> if (ngx_quic_close_stream(qs) != NGX_OK) {
>>> - ngx_quic_close_connection(c, NGX_ERROR);
>>> - return;
>>> + goto failed;
>>> }
>>> +
>>> + return;
>>> +
>>> +failed:
>>> +
>>> + qc = ngx_quic_get_connection(qs->parent);
>>> + qc->error = NGX_QUIC_ERR_INTERNAL_ERROR;
>>> +
>>> + ngx_post_event(&qc->close, &ngx_posted_events);
>>> }
>>>
>>>
>>
>> Another problem here is that ngx_quic_close_connection() was
>> called with stream connection, with unpleasant consequences.
>> You might want to reflect this in the changelog.
>
> Thanks for noticing this. The full commit log:
>
> QUIC: fixed post-close use-after-free.
>
> Previously, ngx_quic_close_connection() could be called in a way that QUIC
> connection was accessed after the call. In most cases the connection is not
> closed right away, but close timeout is scheduled. However, it's not always
> the case. Also, if the close process started earlier for a different reason,
> calling ngx_quic_close_connection() may actually close the connection. The
> connection object should not be accessed after that.
>
> Now, when possible, return statement is added to eliminate post-close connection
> object access. In other places ngx_quic_close_connection() is substituted with
> posting close event.
>
> Also, the new way of closing connection in ngx_quic_stream_cleanup_handler()
> fixes another problem in this function. Previously it passed stream connection
> instead of QUIC connection to ngx_quic_close_connection(). This could result
> in incomplete connection shutdown. One consequence of that could be that QUIC
> streams were freed without shutting down their application contexts. This could
> result in another use-after-free.
>
> Found by Coverity (CID 1530402).
>

Good for me.

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