Re: [PATCH] io: remove io watch if TLS channel is closed during handshake

[Daniel P . Berrangé]

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 :|