Re: [PATCH] ui/clipboard: avoid crash upon request when clipboard peer is not initialized

[Marc-André Lureau]

Hi

On Fri, Jan 12, 2024 at 5:57 PM Fiona Ebner <f.ebner@proxmox.com> wrote:
>
> With VNC, it can be that a client sends a VNC_MSG_CLIENT_CUT_TEXT
> message before sending a VNC_MSG_CLIENT_SET_ENCODINGS message with
> VNC_ENCODING_CLIPBOARD_EXT for configuring the clipboard extension.
>
> This means that qemu_clipboard_request() can be reached (via
> vnc_client_cut_text_ext()) before vnc_server_cut_text_caps() was
> called and had the chance to initialize the clipboard peer. In that
> case, info->owner->request is NULL instead of a function and so
> attempting to call it in qemu_clipboard_request() results in a
> segfault.
>
> In particular, this can happen when using the KRDC (22.12.3) VNC
> client on Wayland.
>
> It is not enough to check in ui/vnc.c's protocol_client_msg() if the
> VNC_FEATURE_CLIPBOARD_EXT feature is enabled before handling an
> extended clipboard message with vnc_client_cut_text_ext(), because of
> the following scenario 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 feature correctly, so the check added by your patch passes), that
> clipboard info is passed to qemu_clipboard_request() and the original
> segfault still happens.
>
> Fixes: CVE-2023-6683
> Reported-by: Markus Frank <m.frank@proxmox.com>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> Tested-by: Markus Frank <m.frank@proxmox.com>
> ---
>
> This is just a minimal fix. Happy to add some warning/error to not
> hide the issue with the missing initialization completely and/or go
> for a different approach with a check somewhere in the VNC code.
>
>  ui/clipboard.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/ui/clipboard.c b/ui/clipboard.c
> index 3d14bffaf8..c13b54d2e9 100644
> --- a/ui/clipboard.c
> +++ b/ui/clipboard.c
> @@ -129,7 +129,8 @@ void qemu_clipboard_request(QemuClipboardInfo *info,
>      if (info->types[type].data ||
>          info->types[type].requested ||
>          !info->types[type].available ||
> -        !info->owner)
> +        !info->owner ||
> +        !info->owner->request)
>          return;

While that fixes the crash, I think we should handle the situation
earlier. A clipboard peer shouldn't be allowed to hold the clipboard
if it doesn't have the data available or a "request" callback set.

Iow, we should have an assert(info->owner->request != NULL) here instead.

>      info->types[type].requested = true;
> --
> 2.39.2
>
>
>


-- 
Marc-André Lureau