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

[Marc-André Lureau]

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>


-- 
Marc-André Lureau