[PATCH] QUIC: fixed post-close use-after-free
Roman Arutyunyan
arut at nginx.com
Mon May 22 12:00:52 UTC 2023
On Mon, May 22, 2023 at 03:28:24PM +0400, Sergey Kandaurov wrote:
>
> > On 21 May 2023, at 13:20, Roman Arutyunyan <arut at nginx.com> wrote:
> >
> > # HG changeset patch
> > # User Roman Arutyunyan <arut at 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).
--
Roman Arutyunyan
More information about the nginx-devel
mailing list