| Message ID | 20230712165547.584157-1-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] io: remove io watch if TLS channel is closed during handshake | expand |
12.07.2023 19:55, Daniel P. Berrangé wrote: > The TLS handshake make take some time to complete, during which time an > I/O watch might be registered with the main loop. If the owner of the > I/O channel invokes qio_channel_close() while the handshake is waiting > to continue the I/O watch must be removed. Failing to remove it will > later trigger the completion callback which the owner is not expecting > to receive. In the case of the VNC server, this results in a SEGV as > vnc_disconnect_start() tries to shutdown a client connection that is > already gone / NULL. > > CVE-2023-3354 > Reported-by: jiangyegen <jiangyegen@huawei.com> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > --- > > In v2: > > - Use g_clear_handle_id to set source ID to zero > > include/io/channel-tls.h | 1 + > io/channel-tls.c | 18 ++++++++++++------ > 2 files changed, 13 insertions(+), 6 deletions(-) I can't say I completely understand what's going on here, but I tried to understand it. And it looks like the patch is doing the right thing. Dunno if I can add my Reviewed-by: Michael Tokarev <mjt@tls.msk.ru> Thank you for the good work Daniel! /mjt
12.07.2023 19:55, Daniel P. Berrangé wrote: > The TLS handshake make take some time to complete, during which time an > I/O watch might be registered with the main loop. If the owner of the > I/O channel invokes qio_channel_close() while the handshake is waiting > to continue the I/O watch must be removed. Failing to remove it will > later trigger the completion callback which the owner is not expecting > to receive. In the case of the VNC server, this results in a SEGV as > vnc_disconnect_start() tries to shutdown a client connection that is > already gone / NULL. > > CVE-2023-3354 > Reported-by: jiangyegen <jiangyegen@huawei.com> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> Can we have this in 8.1 please? What's needed to get this one into 8.1? /mjt
diff --git a/include/io/channel-tls.h b/include/io/channel-tls.h index 5672479e9e..26c67f17e2 100644 --- a/include/io/channel-tls.h +++ b/include/io/channel-tls.h @@ -48,6 +48,7 @@ struct QIOChannelTLS { QIOChannel *master; QCryptoTLSSession *session; QIOChannelShutdown shutdown; + guint hs_ioc_tag; }; /** diff --git a/io/channel-tls.c b/io/channel-tls.c index 9805dd0a3f..847d5297c3 100644 --- a/io/channel-tls.c +++ b/io/channel-tls.c @@ -198,12 +198,13 @@ static void qio_channel_tls_handshake_task(QIOChannelTLS *ioc, } trace_qio_channel_tls_handshake_pending(ioc, status); - qio_channel_add_watch_full(ioc->master, - condition, - qio_channel_tls_handshake_io, - data, - NULL, - context); + ioc->hs_ioc_tag = + qio_channel_add_watch_full(ioc->master, + condition, + qio_channel_tls_handshake_io, + data, + NULL, + context); } } @@ -218,6 +219,7 @@ static gboolean qio_channel_tls_handshake_io(QIOChannel *ioc, QIOChannelTLS *tioc = QIO_CHANNEL_TLS( qio_task_get_source(task)); + tioc->hs_ioc_tag = 0; g_free(data); qio_channel_tls_handshake_task(tioc, task, context); @@ -378,6 +380,10 @@ static int qio_channel_tls_close(QIOChannel *ioc, { QIOChannelTLS *tioc = QIO_CHANNEL_TLS(ioc); + if (tioc->hs_ioc_tag) { + g_clear_handle_id(&tioc->hs_ioc_tag, g_source_remove); + } + return qio_channel_close(tioc->master, errp); }
The TLS handshake make take some time to complete, during which time an I/O watch might be registered with the main loop. If the owner of the I/O channel invokes qio_channel_close() while the handshake is waiting to continue the I/O watch must be removed. Failing to remove it will later trigger the completion callback which the owner is not expecting to receive. In the case of the VNC server, this results in a SEGV as vnc_disconnect_start() tries to shutdown a client connection that is already gone / NULL. CVE-2023-3354 Reported-by: jiangyegen <jiangyegen@huawei.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- In v2: - Use g_clear_handle_id to set source ID to zero include/io/channel-tls.h | 1 + io/channel-tls.c | 18 ++++++++++++------ 2 files changed, 13 insertions(+), 6 deletions(-)