* [PATCH] io: remove io watch if TLS channel is closed during handshake
@ 2023-07-05 18:17 Daniel P. Berrangé
2023-07-07 9:30 ` Marc-André Lureau
0 siblings, 1 reply; 4+ messages in thread
From: Daniel P. Berrangé @ 2023-07-05 18:17 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-stable, Daniel P. Berrangé, Mauro Matteo Cascella,
jiangyegen
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>
---
include/io/channel-tls.h | 1 +
io/channel-tls.c | 18 ++++++++++++------
2 files changed, 13 insertions(+), 6 deletions(-)
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..e327e6a5c2 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_source_remove(tioc->hs_ioc_tag);
+ }
+
return qio_channel_close(tioc->master, errp);
}
--
2.41.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] io: remove io watch if TLS channel is closed during handshake
2023-07-05 18:17 [PATCH] io: remove io watch if TLS channel is closed during handshake Daniel P. Berrangé
@ 2023-07-07 9:30 ` Marc-André Lureau
2023-07-07 9:38 ` Daniel P. Berrangé
0 siblings, 1 reply; 4+ messages in thread
From: Marc-André Lureau @ 2023-07-07 9:30 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, qemu-stable, Mauro Matteo Cascella, jiangyegen
[-- Attachment #1: Type: text/plain, Size: 3012 bytes --]
On Wed, Jul 5, 2023 at 10:20 PM Daniel P. Berrangé <berrange@redhat.com>
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>
>
> ---
> include/io/channel-tls.h | 1 +
> io/channel-tls.c | 18 ++++++++++++------
> 2 files changed, 13 insertions(+), 6 deletions(-)
>
> 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..e327e6a5c2 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_source_remove(tioc->hs_ioc_tag);
>
set it to 0 ?
or
g_clear_handle_id(&tios->hs_ioc_tag, g_source_remove);
> + }
> +
> return qio_channel_close(tioc->master, errp);
> }
>
> --
> 2.41.0
>
>
>
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 4343 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] io: remove io watch if TLS channel is closed during handshake
2023-07-07 9:30 ` Marc-André Lureau
@ 2023-07-07 9:38 ` Daniel P. Berrangé
2023-07-12 9:15 ` Michael Tokarev
0 siblings, 1 reply; 4+ messages in thread
From: Daniel P. Berrangé @ 2023-07-07 9:38 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, qemu-stable, Mauro Matteo Cascella, jiangyegen
On Fri, Jul 07, 2023 at 01:30:16PM +0400, Marc-André Lureau wrote:
> On Wed, Jul 5, 2023 at 10:20 PM Daniel P. Berrangé <berrange@redhat.com>
> 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>
> >
>
>
>
> > ---
> > include/io/channel-tls.h | 1 +
> > io/channel-tls.c | 18 ++++++++++++------
> > 2 files changed, 13 insertions(+), 6 deletions(-)
> >
> > 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..e327e6a5c2 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_source_remove(tioc->hs_ioc_tag);
> >
>
> set it to 0 ?
> or
> g_clear_handle_id(&tios->hs_ioc_tag, g_source_remove);
Yes, close can be called mutliple times, so we must set to zero.
> > + }
> > +
> > return qio_channel_close(tioc->master, errp);
> > }
> >
> > --
> > 2.41.0
> >
> >
> >
>
> --
> Marc-André Lureau
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] io: remove io watch if TLS channel is closed during handshake
2023-07-07 9:38 ` Daniel P. Berrangé
@ 2023-07-12 9:15 ` Michael Tokarev
0 siblings, 0 replies; 4+ messages in thread
From: Michael Tokarev @ 2023-07-12 9:15 UTC (permalink / raw)
To: Daniel P. Berrangé, Marc-André Lureau
Cc: qemu-devel, qemu-stable, Mauro Matteo Cascella, jiangyegen,
qemu-stable
07.07.2023 12:38, Daniel P. Berrangé wrote:
> On Fri, Jul 07, 2023 at 01:30:16PM +0400, Marc-André Lureau wrote:
...>>> @@ -378,6 +380,10 @@ static int qio_channel_tls_close(QIOChannel *ioc,
>>> {
>>> QIOChannelTLS *tioc = QIO_CHANNEL_TLS(ioc);
>>>
>>> + if (tioc->hs_ioc_tag) {
>>> + g_source_remove(tioc->hs_ioc_tag);
>>>
>>
>> set it to 0 ?
>> or
>> g_clear_handle_id(&tios->hs_ioc_tag, g_source_remove);
>
> Yes, close can be called mutliple times, so we must set to zero.
Ping, for qemu 8.1 and -stable ?
Thanks,
/mjt
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-07-12 9:17 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-05 18:17 [PATCH] io: remove io watch if TLS channel is closed during handshake Daniel P. Berrangé
2023-07-07 9:30 ` Marc-André Lureau
2023-07-07 9:38 ` Daniel P. Berrangé
2023-07-12 9:15 ` Michael Tokarev
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).