Hi On Mon, Jan 15, 2024 at 3:48 PM Fiona Ebner <f.eb...@proxmox.com> wrote: > > Am 15.01.24 um 12:33 schrieb Marc-André Lureau: > > 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. > > > > In the above scenario, I'm pretty sure there is data when > qemu_clipboard_update() is called. Just no request callback. If we'd > reject this, won't that break clients that do not set the > VNC_FEATURE_CLIPBOARD_EXT feature and only use non-extended > VNC_MSG_CLIENT_CUT_TEXT messages?
If "data" is already set, then qemu_clipboard_request() returns. Inverting the condition I suggested above: it's allowed to be the clipboard owner if either "data" is set, or a request callback is set. -- Marc-André Lureau