Hi On Mon, Jan 15, 2024 at 3:26 PM Fiona Ebner <f.eb...@proxmox.com> wrote: > > 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. >
The trouble is when qemu_clipboard_update() is called without data & without a request callback set. We shouldn't allow that as we have no means to get the clipboard data then. -- Marc-André Lureau
