Am 15.01.24 um 12:15 schrieb Marc-André Lureau: > Hi > > On Mon, Jan 15, 2024 at 2:45 PM Fiona Ebner <f.eb...@proxmox.com> wrote: >> >> Am 14.01.24 um 14:51 schrieb Marc-André Lureau: >>>> >>>> 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. >>> >> >> Where should initialization of the cbpeer happen so that we are >> guaranteed to do it also for clients that do not set the >> VNC_FEATURE_CLIPBOARD_EXT feature? Can the vnc_clipboard_request >> function be re-used for clients without that feature or will it be >> necessary to add some kind of "dummy" request callback for those clients? > > qemu_clipboard_update() shouldn't accept info as current clipboard if > the owner doesn't have the data available or a "request" callback set. > This should also be assert() somehow and handled earlier. >
The request callback is only initialized in vnc_server_cut_text_caps() when the VNC_FEATURE_CLIPBOARD_EXT is enabled. AFAIU, it's perfectly fine for clients to use the clipboard with non-extended messages and qemu_clipboard_update() should (and currently does) accept those. > In vnc_client_cut_text_ext() we could detect that situation, but with > Daniel's "[PATCH] ui: reject extended clipboard message if not > activated", this shouldn't happen anymore iiuc. > Daniel's patch doesn't change the behavior for non-extended messages. The problem can still happen with two VNC clients. This is the scenario described in the lower half of my commit message (and why Daniel mentions in his patch that it's not sufficient to fix the CVE). In short: client A does not set the VNC_FEATURE_CLIPBOARD_EXT feature and then uses a non-extended VNC_MSG_CLIENT_CUT_TEXT message. This leads to vnc_client_cut_text() being called and setting the clipboard info referencing that client. But here, no request callback is initialized, that only happens in vnc_server_cut_text_caps() when the VNC_FEATURE_CLIPBOARD_EXT is enabled. When client B does set the VNC_FEATURE_CLIPBOARD_EXT feature and does send an extended VNC_MSG_CLIENT_CUT_TEXT message, the request callback will be attempted but it isn't set. Best Regards, Fiona
