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:48 PM Fiona Ebner <f.ebner@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.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.
> >
>
> 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