[PATCH v2 0/3] io: fix crash in VNC websock server when client quits early

[PATCH v2 0/3] io: fix crash in VNC websock server when client quits early

[Daniel P. Berrangé]

See patch 3 for the description of the problem and reproducer

Changes in v2:

 - Improve finalizer robustness of TLS source
 - Keep cleanup in finalizer of websock, just augment
   it in the close method
 - Fix resetting of hs_ioc_tag value when callback
   is complete
 - Add CVE assignemnt in 3rd patch

Daniel P. Berrangé (3):
  io: release active GSource in TLS channel finalizer
  io: move websock resource release to close method
  io: fix use after free in websocket handshake code

 include/io/channel-websock.h |  3 ++-
 io/channel-tls.c             | 10 ++++++++++
 io/channel-websock.c         | 33 ++++++++++++++++++++++++++-------
 3 files changed, 38 insertions(+), 8 deletions(-)

-- 
2.50.1

[PATCH v2 1/3] io: release active GSource in TLS channel finalizer

[Daniel P. Berrangé]

While code is supposed to call qio_channel_close() before releasing the
last reference on an QIOChannel, this is not guaranteed. QIOChannelFile
and QIOChannelSocket both cleanup resources in their finalizer if the
close operation was missed.

This ensures the TLS channel will do the same failsafe cleanup.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 io/channel-tls.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/io/channel-tls.c b/io/channel-tls.c
index 7135896f79..bb460ca7e9 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -342,6 +342,16 @@ static void qio_channel_tls_finalize(Object *obj)
 {
     QIOChannelTLS *ioc = QIO_CHANNEL_TLS(obj);
 
+    if (ioc->hs_ioc_tag) {
+        trace_qio_channel_tls_handshake_cancel(ioc);
+        g_clear_handle_id(&ioc->hs_ioc_tag, g_source_remove);
+    }
+
+    if (ioc->bye_ioc_tag) {
+        trace_qio_channel_tls_bye_cancel(ioc);
+        g_clear_handle_id(&ioc->bye_ioc_tag, g_source_remove);
+    }
+
     object_unref(OBJECT(ioc->master));
     qcrypto_tls_session_free(ioc->session);
 }
-- 
2.50.1

[PATCH v2 2/3] io: move websock resource release to close method

[Daniel P. Berrangé]

The QIOChannelWebsock object releases all its resources in the
finalize callback. This is later than desired, as callers expect
to be able to call qio_channel_close() to fully close a channel
and release resources related to I/O.

The logic in the finalize method is at most a failsafe to handle
cases where a consumer forgets to call qio_channel_close.

This adds equivalent logic to the close method to release the
resources, using g_clear_handle_id/g_clear_pointer to be robust
against repeated invokations. The finalize method is tweaked
so that the GSource is removed before releasing the underlying
channel.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 io/channel-websock.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/io/channel-websock.c b/io/channel-websock.c
index 0a8c5c4712..a50a160e18 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -922,13 +922,13 @@ static void qio_channel_websock_finalize(Object *obj)
     buffer_free(&ioc->encinput);
     buffer_free(&ioc->encoutput);
     buffer_free(&ioc->rawinput);
-    object_unref(OBJECT(ioc->master));
     if (ioc->io_tag) {
         g_source_remove(ioc->io_tag);
     }
     if (ioc->io_err) {
         error_free(ioc->io_err);
     }
+    object_unref(OBJECT(ioc->master));
 }
 
 
@@ -1218,6 +1218,15 @@ static int qio_channel_websock_close(QIOChannel *ioc,
     QIOChannelWebsock *wioc = QIO_CHANNEL_WEBSOCK(ioc);
 
     trace_qio_channel_websock_close(ioc);
+    buffer_free(&wioc->encinput);
+    buffer_free(&wioc->encoutput);
+    buffer_free(&wioc->rawinput);
+    if (wioc->io_tag) {
+        g_clear_handle_id(&wioc->io_tag, g_source_remove);
+    }
+    if (wioc->io_err) {
+        g_clear_pointer(&wioc->io_err, error_free);
+    }
     return qio_channel_close(wioc->master, errp);
 }
 
-- 
2.50.1

[PATCH v2 3/3] io: fix use after free in websocket handshake code

[Daniel P. Berrangé]

If the QIOChannelWebsock object is freed while it is waiting to
complete a handshake, a GSource is leaked. This can lead to the
callback firing later on and triggering a use-after-free in the
use of the channel. This was observed in the VNC server with the
following trace from valgrind:

==2523108== Invalid read of size 4
==2523108==    at 0x4054A24: vnc_disconnect_start (vnc.c:1296)
==2523108==    by 0x4054A24: vnc_client_error (vnc.c:1392)
==2523108==    by 0x4068A09: vncws_handshake_done (vnc-ws.c:105)
==2523108==    by 0x44863B4: qio_task_complete (task.c:197)
==2523108==    by 0x448343D: qio_channel_websock_handshake_io (channel-websock.c:588)
==2523108==    by 0x6EDB862: UnknownInlinedFun (gmain.c:3398)
==2523108==    by 0x6EDB862: g_main_context_dispatch_unlocked.lto_priv.0 (gmain.c:4249)
==2523108==    by 0x6EDBAE4: g_main_context_dispatch (gmain.c:4237)
==2523108==    by 0x45EC79F: glib_pollfds_poll (main-loop.c:287)
==2523108==    by 0x45EC79F: os_host_main_loop_wait (main-loop.c:310)
==2523108==    by 0x45EC79F: main_loop_wait (main-loop.c:589)
==2523108==    by 0x423A56D: qemu_main_loop (runstate.c:835)
==2523108==    by 0x454F300: qemu_default_main (main.c:37)
==2523108==    by 0x73D6574: (below main) (libc_start_call_main.h:58)
==2523108==  Address 0x57a6e0dc is 28 bytes inside a block of size 103,608 free'd
==2523108==    at 0x5F2FE43: free (vg_replace_malloc.c:989)
==2523108==    by 0x6EDC444: g_free (gmem.c:208)
==2523108==    by 0x4053F23: vnc_update_client (vnc.c:1153)
==2523108==    by 0x4053F23: vnc_refresh (vnc.c:3225)
==2523108==    by 0x4042881: dpy_refresh (console.c:880)
==2523108==    by 0x4042881: gui_update (console.c:90)
==2523108==    by 0x45EFA1B: timerlist_run_timers.part.0 (qemu-timer.c:562)
==2523108==    by 0x45EFC8F: timerlist_run_timers (qemu-timer.c:495)
==2523108==    by 0x45EFC8F: qemu_clock_run_timers (qemu-timer.c:576)
==2523108==    by 0x45EFC8F: qemu_clock_run_all_timers (qemu-timer.c:663)
==2523108==    by 0x45EC765: main_loop_wait (main-loop.c:600)
==2523108==    by 0x423A56D: qemu_main_loop (runstate.c:835)
==2523108==    by 0x454F300: qemu_default_main (main.c:37)
==2523108==    by 0x73D6574: (below main) (libc_start_call_main.h:58)
==2523108==  Block was alloc'd at
==2523108==    at 0x5F343F3: calloc (vg_replace_malloc.c:1675)
==2523108==    by 0x6EE2F81: g_malloc0 (gmem.c:133)
==2523108==    by 0x4057DA3: vnc_connect (vnc.c:3245)
==2523108==    by 0x448591B: qio_net_listener_channel_func (net-listener.c:54)
==2523108==    by 0x6EDB862: UnknownInlinedFun (gmain.c:3398)
==2523108==    by 0x6EDB862: g_main_context_dispatch_unlocked.lto_priv.0 (gmain.c:4249)
==2523108==    by 0x6EDBAE4: g_main_context_dispatch (gmain.c:4237)
==2523108==    by 0x45EC79F: glib_pollfds_poll (main-loop.c:287)
==2523108==    by 0x45EC79F: os_host_main_loop_wait (main-loop.c:310)
==2523108==    by 0x45EC79F: main_loop_wait (main-loop.c:589)
==2523108==    by 0x423A56D: qemu_main_loop (runstate.c:835)
==2523108==    by 0x454F300: qemu_default_main (main.c:37)
==2523108==    by 0x73D6574: (below main) (libc_start_call_main.h:58)
==2523108==

The above can be reproduced by launching QEMU with

  $ qemu-system-x86_64 -vnc localhost:0,websocket=5700

and then repeatedly running:

  for i in {1..100}; do
     (echo -n "GET / HTTP/1.1" && sleep 0.05) | nc -w 1 localhost 5700 &
  done

CVE-2025-11234
Reported-by: Grant Millar | Cylo <rid@cylo.io>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/io/channel-websock.h |  3 ++-
 io/channel-websock.c         | 22 ++++++++++++++++------
 2 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/include/io/channel-websock.h b/include/io/channel-websock.h
index e180827c57..6700cf8946 100644
--- a/include/io/channel-websock.h
+++ b/include/io/channel-websock.h
@@ -61,7 +61,8 @@ struct QIOChannelWebsock {
     size_t payload_remain;
     size_t pong_remain;
     QIOChannelWebsockMask mask;
-    guint io_tag;
+    guint hs_io_tag; /* tracking handshake task */
+    guint io_tag; /* tracking watch task */
     Error *io_err;
     gboolean io_eof;
     uint8_t opcode;
diff --git a/io/channel-websock.c b/io/channel-websock.c
index a50a160e18..cb4dafdebb 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -545,6 +545,7 @@ static gboolean qio_channel_websock_handshake_send(QIOChannel *ioc,
         trace_qio_channel_websock_handshake_fail(ioc, error_get_pretty(err));
         qio_task_set_error(task, err);
         qio_task_complete(task);
+        wioc->hs_io_tag = 0;
         return FALSE;
     }
 
@@ -560,6 +561,7 @@ static gboolean qio_channel_websock_handshake_send(QIOChannel *ioc,
             trace_qio_channel_websock_handshake_complete(ioc);
             qio_task_complete(task);
         }
+        wioc->hs_io_tag = 0;
         return FALSE;
     }
     trace_qio_channel_websock_handshake_pending(ioc, G_IO_OUT);
@@ -586,6 +588,7 @@ static gboolean qio_channel_websock_handshake_io(QIOChannel *ioc,
         trace_qio_channel_websock_handshake_fail(ioc, error_get_pretty(err));
         qio_task_set_error(task, err);
         qio_task_complete(task);
+        wioc->hs_io_tag = 0;
         return FALSE;
     }
     if (ret == 0) {
@@ -597,7 +600,7 @@ static gboolean qio_channel_websock_handshake_io(QIOChannel *ioc,
     error_propagate(&wioc->io_err, err);
 
     trace_qio_channel_websock_handshake_reply(ioc);
-    qio_channel_add_watch(
+    wioc->hs_io_tag = qio_channel_add_watch(
         wioc->master,
         G_IO_OUT,
         qio_channel_websock_handshake_send,
@@ -907,11 +910,12 @@ void qio_channel_websock_handshake(QIOChannelWebsock *ioc,
 
     trace_qio_channel_websock_handshake_start(ioc);
     trace_qio_channel_websock_handshake_pending(ioc, G_IO_IN);
-    qio_channel_add_watch(ioc->master,
-                          G_IO_IN,
-                          qio_channel_websock_handshake_io,
-                          task,
-                          NULL);
+    ioc->hs_io_tag = qio_channel_add_watch(
+        ioc->master,
+        G_IO_IN,
+        qio_channel_websock_handshake_io,
+        task,
+        NULL);
 }
 
 
@@ -922,6 +926,9 @@ static void qio_channel_websock_finalize(Object *obj)
     buffer_free(&ioc->encinput);
     buffer_free(&ioc->encoutput);
     buffer_free(&ioc->rawinput);
+    if (ioc->hs_io_tag) {
+        g_source_remove(ioc->hs_io_tag);
+    }
     if (ioc->io_tag) {
         g_source_remove(ioc->io_tag);
     }
@@ -1221,6 +1228,9 @@ static int qio_channel_websock_close(QIOChannel *ioc,
     buffer_free(&wioc->encinput);
     buffer_free(&wioc->encoutput);
     buffer_free(&wioc->rawinput);
+    if (wioc->hs_io_tag) {
+        g_clear_handle_id(&wioc->hs_io_tag, g_source_remove);
+    }
     if (wioc->io_tag) {
         g_clear_handle_id(&wioc->io_tag, g_source_remove);
     }
-- 
2.50.1

Re: [PATCH v2 1/3] io: release active GSource in TLS channel finalizer

[Eric Blake]

On Fri, Oct 03, 2025 at 04:02:43PM +0100, Daniel P. Berrangé wrote:
> While code is supposed to call qio_channel_close() before releasing the
> last reference on an QIOChannel, this is not guaranteed. QIOChannelFile
> and QIOChannelSocket both cleanup resources in their finalizer if the
> close operation was missed.
> 
> This ensures the TLS channel will do the same failsafe cleanup.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  io/channel-tls.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/io/channel-tls.c b/io/channel-tls.c
> index 7135896f79..bb460ca7e9 100644
> --- a/io/channel-tls.c
> +++ b/io/channel-tls.c
> @@ -342,6 +342,16 @@ static void qio_channel_tls_finalize(Object *obj)
>  {
>      QIOChannelTLS *ioc = QIO_CHANNEL_TLS(obj);
>  
> +    if (ioc->hs_ioc_tag) {
> +        trace_qio_channel_tls_handshake_cancel(ioc);
> +        g_clear_handle_id(&ioc->hs_ioc_tag, g_source_remove);
> +    }
> +
> +    if (ioc->bye_ioc_tag) {
> +        trace_qio_channel_tls_bye_cancel(ioc);
> +        g_clear_handle_id(&ioc->bye_ioc_tag, g_source_remove);
> +    }
> +
>      object_unref(OBJECT(ioc->master));
>      qcrypto_tls_session_free(ioc->session);
>  }
> -- 
> 2.50.1
> 
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH v2 2/3] io: move websock resource release to close method

[Eric Blake]

On Fri, Oct 03, 2025 at 04:02:44PM +0100, Daniel P. Berrangé wrote:
> The QIOChannelWebsock object releases all its resources in the
> finalize callback. This is later than desired, as callers expect
> to be able to call qio_channel_close() to fully close a channel
> and release resources related to I/O.
> 
> The logic in the finalize method is at most a failsafe to handle
> cases where a consumer forgets to call qio_channel_close.
> 
> This adds equivalent logic to the close method to release the
> resources, using g_clear_handle_id/g_clear_pointer to be robust
> against repeated invokations. The finalize method is tweaked

invocations

> so that the GSource is removed before releasing the underlying
> channel.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  io/channel-websock.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/io/channel-websock.c b/io/channel-websock.c
> index 0a8c5c4712..a50a160e18 100644
> --- a/io/channel-websock.c
> +++ b/io/channel-websock.c
> @@ -922,13 +922,13 @@ static void qio_channel_websock_finalize(Object *obj)
>      buffer_free(&ioc->encinput);
>      buffer_free(&ioc->encoutput);
>      buffer_free(&ioc->rawinput);
> -    object_unref(OBJECT(ioc->master));
>      if (ioc->io_tag) {
>          g_source_remove(ioc->io_tag);
>      }
>      if (ioc->io_err) {
>          error_free(ioc->io_err);
>      }
> +    object_unref(OBJECT(ioc->master));
>  }
>  
>  
> @@ -1218,6 +1218,15 @@ static int qio_channel_websock_close(QIOChannel *ioc,
>      QIOChannelWebsock *wioc = QIO_CHANNEL_WEBSOCK(ioc);
>  
>      trace_qio_channel_websock_close(ioc);
> +    buffer_free(&wioc->encinput);
> +    buffer_free(&wioc->encoutput);
> +    buffer_free(&wioc->rawinput);
> +    if (wioc->io_tag) {
> +        g_clear_handle_id(&wioc->io_tag, g_source_remove);
> +    }
> +    if (wioc->io_err) {
> +        g_clear_pointer(&wioc->io_err, error_free);
> +    }
>      return qio_channel_close(wioc->master, errp);
>  }
>  
> -- 
> 2.50.1
> 
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH v2 3/3] io: fix use after free in websocket handshake code

[Eric Blake]

On Fri, Oct 03, 2025 at 04:02:45PM +0100, Daniel P. Berrangé wrote:
> If the QIOChannelWebsock object is freed while it is waiting to
> complete a handshake, a GSource is leaked. This can lead to the
> callback firing later on and triggering a use-after-free in the
> use of the channel. This was observed in the VNC server with the
> following trace from valgrind:
> 
> ==2523108== Invalid read of size 4
> ==2523108==    at 0x4054A24: vnc_disconnect_start (vnc.c:1296)

> 
> The above can be reproduced by launching QEMU with
> 
>   $ qemu-system-x86_64 -vnc localhost:0,websocket=5700
> 
> and then repeatedly running:
> 
>   for i in {1..100}; do
>      (echo -n "GET / HTTP/1.1" && sleep 0.05) | nc -w 1 localhost 5700 &
>   done
> 
> CVE-2025-11234
> Reported-by: Grant Millar | Cylo <rid@cylo.io>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  include/io/channel-websock.h |  3 ++-
>  io/channel-websock.c         | 22 ++++++++++++++++------
>  2 files changed, 18 insertions(+), 7 deletions(-)

> 
> diff --git a/include/io/channel-websock.h b/include/io/channel-websock.h
> index e180827c57..6700cf8946 100644
> --- a/include/io/channel-websock.h
> +++ b/include/io/channel-websock.h
> @@ -61,7 +61,8 @@ struct QIOChannelWebsock {
>      size_t payload_remain;
>      size_t pong_remain;
>      QIOChannelWebsockMask mask;
> -    guint io_tag;
> +    guint hs_io_tag; /* tracking handshake task */
> +    guint io_tag; /* tracking watch task */

The comment helps.  It might be possible to use a longer name like
watch_io_tag as the counterpart to hs_io_tag for symmetry, but that's
more code churn, so I'm happy to keep the names you have here.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org