diff mbox series

[v3,1/2] ui/clipboard: mark type as not available when there is no data

Message ID 20240124105749.204610-1-f.ebner@proxmox.com
State New
Headers show
Series [v3,1/2] ui/clipboard: mark type as not available when there is no data | expand

Commit Message

Fiona Ebner Jan. 24, 2024, 10:57 a.m. UTC
With VNC, a client can send a non-extended VNC_MSG_CLIENT_CUT_TEXT
message with len=0. In qemu_clipboard_set_data(), the clipboard info
will be updated setting data to NULL (because g_memdup(data, size)
returns NULL when size is 0). If the client does not set the
VNC_ENCODING_CLIPBOARD_EXT feature when setting up the encodings, then
the 'request' callback for the clipboard peer is not initialized.
Later, because data is NULL, qemu_clipboard_request() can be reached
via vdagent_chr_write() and vdagent_clipboard_recv_request() and
there, the clipboard owner's 'request' callback will be attempted to
be called, but that is a NULL pointer.

In particular, this can happen when using the KRDC (22.12.3) VNC
client.

Another scenario leading to the same issue is with two clients (say
noVNC and KRDC):

The noVNC client sets the extension VNC_FEATURE_CLIPBOARD_EXT and
initializes its cbpeer.

The KRDC client does not, but triggers a vnc_client_cut_text() (note
it's not the _ext variant)). There, a new clipboard info with it as
the 'owner' is created and via qemu_clipboard_set_data() is called,
which in turn calls qemu_clipboard_update() with that info.

In qemu_clipboard_update(), the notifier for the noVNC client will be
called, i.e. vnc_clipboard_notify() and also set vs->cbinfo for the
noVNC client. The 'owner' in that clipboard info is the clipboard peer
for the KRDC client, which did not initialize the 'request' function.
That sounds correct to me, it is the owner of that clipboard info.

Then when noVNC sends a VNC_MSG_CLIENT_CUT_TEXT message (it did set
the VNC_FEATURE_CLIPBOARD_EXT feature correctly, so a check for it
passes), that clipboard info is passed to qemu_clipboard_request() and
the original segfault still happens.

Fix the issue by handling updates with size 0 differently. In
particular, mark in the clipboard info that the type is not available.

While at it, switch to g_memdup2(), because g_memdup() is deprecated.

Cc: qemu-stable@nongnu.org
Fixes: CVE-2023-6683
Reported-by: Markus Frank <m.frank@proxmox.com>
Suggested-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

Changes in v3:
    * Yet another new appraoch, setting available to false when
      no data is passed in when updating.
    * Update commit message to focus on the fact that non-extended
      VNC_MSG_CLIENT_CUT_TEXT messages with len=0 are problematic.

 ui/clipboard.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Comments

Marc-André Lureau Jan. 24, 2024, 11:05 a.m. UTC | #1
On Wed, Jan 24, 2024 at 2:59 PM Fiona Ebner <f.ebner@proxmox.com> wrote:
>
> With VNC, a client can send a non-extended VNC_MSG_CLIENT_CUT_TEXT
> message with len=0. In qemu_clipboard_set_data(), the clipboard info
> will be updated setting data to NULL (because g_memdup(data, size)
> returns NULL when size is 0). If the client does not set the
> VNC_ENCODING_CLIPBOARD_EXT feature when setting up the encodings, then
> the 'request' callback for the clipboard peer is not initialized.
> Later, because data is NULL, qemu_clipboard_request() can be reached
> via vdagent_chr_write() and vdagent_clipboard_recv_request() and
> there, the clipboard owner's 'request' callback will be attempted to
> be called, but that is a NULL pointer.
>
> In particular, this can happen when using the KRDC (22.12.3) VNC
> client.
>
> Another scenario leading to the same issue is with two clients (say
> noVNC and KRDC):
>
> The noVNC client sets the extension VNC_FEATURE_CLIPBOARD_EXT and
> initializes its cbpeer.
>
> The KRDC client does not, but triggers a vnc_client_cut_text() (note
> it's not the _ext variant)). There, a new clipboard info with it as
> the 'owner' is created and via qemu_clipboard_set_data() is called,
> which in turn calls qemu_clipboard_update() with that info.
>
> In qemu_clipboard_update(), the notifier for the noVNC client will be
> called, i.e. vnc_clipboard_notify() and also set vs->cbinfo for the
> noVNC client. The 'owner' in that clipboard info is the clipboard peer
> for the KRDC client, which did not initialize the 'request' function.
> That sounds correct to me, it is the owner of that clipboard info.
>
> Then when noVNC sends a VNC_MSG_CLIENT_CUT_TEXT message (it did set
> the VNC_FEATURE_CLIPBOARD_EXT feature correctly, so a check for it
> passes), that clipboard info is passed to qemu_clipboard_request() and
> the original segfault still happens.
>
> Fix the issue by handling updates with size 0 differently. In
> particular, mark in the clipboard info that the type is not available.
>
> While at it, switch to g_memdup2(), because g_memdup() is deprecated.
>
> Cc: qemu-stable@nongnu.org
> Fixes: CVE-2023-6683
> Reported-by: Markus Frank <m.frank@proxmox.com>
> Suggested-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>
> Changes in v3:
>     * Yet another new appraoch, setting available to false when
>       no data is passed in when updating.
>     * Update commit message to focus on the fact that non-extended
>       VNC_MSG_CLIENT_CUT_TEXT messages with len=0 are problematic.
>
>  ui/clipboard.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/ui/clipboard.c b/ui/clipboard.c
> index 3d14bffaf8..b3f6fa3c9e 100644
> --- a/ui/clipboard.c
> +++ b/ui/clipboard.c
> @@ -163,9 +163,15 @@ void qemu_clipboard_set_data(QemuClipboardPeer *peer,
>      }
>
>      g_free(info->types[type].data);
> -    info->types[type].data = g_memdup(data, size);
> -    info->types[type].size = size;
> -    info->types[type].available = true;
> +    if (size) {
> +        info->types[type].data = g_memdup2(data, size);
> +        info->types[type].size = size;
> +        info->types[type].available = true;
> +    } else {
> +        info->types[type].data = NULL;
> +        info->types[type].size = 0;
> +        info->types[type].available = false;
> +    }
>
>      if (update) {
>          qemu_clipboard_update(info);
> --
> 2.39.2
>
>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Markus Frank Jan. 24, 2024, 2:11 p.m. UTC | #2
Tested the two paths that could lead to this segfault.
KRDC and noVNC running simultaneously on wayland and both running on Xorg when the clipboard is cleared.
Everything worked fine and no segfault happens.

On  2024-01-24 11:57, Fiona Ebner wrote:
> With VNC, a client can send a non-extended VNC_MSG_CLIENT_CUT_TEXT
> message with len=0. In qemu_clipboard_set_data(), the clipboard info
> will be updated setting data to NULL (because g_memdup(data, size)
> returns NULL when size is 0). If the client does not set the
> VNC_ENCODING_CLIPBOARD_EXT feature when setting up the encodings, then
> the 'request' callback for the clipboard peer is not initialized.
> Later, because data is NULL, qemu_clipboard_request() can be reached
> via vdagent_chr_write() and vdagent_clipboard_recv_request() and
> there, the clipboard owner's 'request' callback will be attempted to
> be called, but that is a NULL pointer.
> 
> In particular, this can happen when using the KRDC (22.12.3) VNC
> client.
> 
> Another scenario leading to the same issue is with two clients (say
> noVNC and KRDC):
> 
> The noVNC client sets the extension VNC_FEATURE_CLIPBOARD_EXT and
> initializes its cbpeer.
> 
> The KRDC client does not, but triggers a vnc_client_cut_text() (note
> it's not the _ext variant)). There, a new clipboard info with it as
> the 'owner' is created and via qemu_clipboard_set_data() is called,
> which in turn calls qemu_clipboard_update() with that info.
> 
> In qemu_clipboard_update(), the notifier for the noVNC client will be
> called, i.e. vnc_clipboard_notify() and also set vs->cbinfo for the
> noVNC client. The 'owner' in that clipboard info is the clipboard peer
> for the KRDC client, which did not initialize the 'request' function.
> That sounds correct to me, it is the owner of that clipboard info.
> 
> Then when noVNC sends a VNC_MSG_CLIENT_CUT_TEXT message (it did set
> the VNC_FEATURE_CLIPBOARD_EXT feature correctly, so a check for it
> passes), that clipboard info is passed to qemu_clipboard_request() and
> the original segfault still happens.
> 
> Fix the issue by handling updates with size 0 differently. In
> particular, mark in the clipboard info that the type is not available.
> 
> While at it, switch to g_memdup2(), because g_memdup() is deprecated.
> 
> Cc: qemu-stable@nongnu.org
> Fixes: CVE-2023-6683
> Reported-by: Markus Frank <m.frank@proxmox.com>
> Suggested-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
Tested-by: Markus Frank <m.frank@proxmox.com>
> ---
> 
> Changes in v3:
>      * Yet another new appraoch, setting available to false when
>        no data is passed in when updating.
>      * Update commit message to focus on the fact that non-extended
>        VNC_MSG_CLIENT_CUT_TEXT messages with len=0 are problematic.
> 
>   ui/clipboard.c | 12 +++++++++---
>   1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/ui/clipboard.c b/ui/clipboard.c
> index 3d14bffaf8..b3f6fa3c9e 100644
> --- a/ui/clipboard.c
> +++ b/ui/clipboard.c
> @@ -163,9 +163,15 @@ void qemu_clipboard_set_data(QemuClipboardPeer *peer,
>       }
>   
>       g_free(info->types[type].data);
> -    info->types[type].data = g_memdup(data, size);
> -    info->types[type].size = size;
> -    info->types[type].available = true;
> +    if (size) {
> +        info->types[type].data = g_memdup2(data, size);
> +        info->types[type].size = size;
> +        info->types[type].available = true;
> +    } else {
> +        info->types[type].data = NULL;
> +        info->types[type].size = 0;
> +        info->types[type].available = false;
> +    }
>   
>       if (update) {
>           qemu_clipboard_update(info);
diff mbox series

Patch

diff --git a/ui/clipboard.c b/ui/clipboard.c
index 3d14bffaf8..b3f6fa3c9e 100644
--- a/ui/clipboard.c
+++ b/ui/clipboard.c
@@ -163,9 +163,15 @@  void qemu_clipboard_set_data(QemuClipboardPeer *peer,
     }
 
     g_free(info->types[type].data);
-    info->types[type].data = g_memdup(data, size);
-    info->types[type].size = size;
-    info->types[type].available = true;
+    if (size) {
+        info->types[type].data = g_memdup2(data, size);
+        info->types[type].size = size;
+        info->types[type].available = true;
+    } else {
+        info->types[type].data = NULL;
+        info->types[type].size = 0;
+        info->types[type].available = false;
+    }
 
     if (update) {
         qemu_clipboard_update(info);