Hi On Fri, Jan 12, 2024 at 5:57 PM Fiona Ebner <f.eb...@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.fr...@proxmox.com> > Signed-off-by: Fiona Ebner <f.eb...@proxmox.com> > Tested-by: Markus Frank <m.fr...@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