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

[Marc-André Lureau]

Hi

On Mon, Jan 15, 2024 at 3:26 PM Fiona Ebner <f.ebner@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.ebner@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