Re: [PATCH v2] ui/clipboard: ensure data is available or request callback is set upon update

[Marc-André Lureau]

Hi

On Wed, Jan 17, 2024 at 3:56 PM Fiona Ebner <f.ebner@proxmox.com> wrote:
>
> Am 17.01.24 um 12:33 schrieb Marc-André Lureau:
> > Hi
> >
> > On Wed, Jan 17, 2024 at 3:30 PM Fiona Ebner <f.ebner@proxmox.com> wrote:
> >>
> >> Am 17.01.24 um 12:11 schrieb Marc-André Lureau:
> >>> Hi
> >>>
> >>> On Wed, Jan 17, 2024 at 3:01 PM Fiona Ebner <f.ebner@proxmox.com> wrote:
> >>>>
> >>>> +    for (type = 0; type < QEMU_CLIPBOARD_TYPE__COUNT && !missing_data; 
> >>>> type++) {
> >>>> +        if (!info->types[type].data) {
> >>>> +            missing_data = true;
> >>>> +        }
> >>>> +    }
> >>>> +    /*
> >>>> +     * If data is missing, the clipboard owner's 'request' callback 
> >>>> needs to be
> >>>> +     * set. Otherwise, there is no way to get the clipboard data and
> >>>> +     * qemu_clipboard_request() cannot be called.
> >>>> +     */
> >>>> +    if (missing_data && info->owner && !info->owner->request) {
> >>>> +        return;
> >>>> +    }
> >>>
> >>> It needs to check whether the type is "available". If not data is
> >>> provided, owner should be set as well, it should assert() that.
> >>>
> >>> That should do the job:
> >>>
> >>> for (type = 0; type < QEMU_CLIPBOARD_TYPE__COUNT; type++) {
> >>>     /*
> >>>      * If data is missing, the clipboard owner's 'request' callback needs 
> >>> to
> >>>      * be set. Otherwise, there is no way to get the clipboard data and
> >>>      * qemu_clipboard_request() cannot be called.
> >>>      */
> >>>     if (info->types[type].available && !info->types[type].data) {
> >>>         assert(info->owner && info->owner->request);
> >>>     }
> >>> }
> >>>
> >>
> >> Okay, thanks! But we can't assert, because that doesn't resolve the CVE
> >> as it would still crash. The VNC client might not have the
> >> VNC_FEATURE_CLIPBOARD_EXT feature, and the request callback is currently
> >> only set in that case. But we can return instead of assert to just avoid
> >> clipboard update. I'll send a v3.
> >
> > If it doesn't have VNC_FEATURE_CLIPBOARD_EXT, it shouldn't update the
> > clipboard without data. (ClientCutText/ServerCutText always have data,
> > even if 0-length)
> >
>
> But a buggy client should not be able to crash QEMU. With a
> VNC_MSG_CLIENT_CUT_TEXT message, when read_s32(data, 4) == 0,
> vnc_client_cut_text() is called with zero length. Is that supposed to

Indeed. That case should be handled at the VNC server code level.. A
0-length text should set clipboard to "", not NULL.

With Ext, there is an explicit description of data ending with \0:
According to 
https://github.com/rfbproto/rfbproto/blob/master/rfbproto.rst#user-content-7727extended-clipboard-pseudo-encoding
"The text must be followed by a terminating null even though the
length is also explicitly given."

But we should still handle 0-length data cases.

> happen? The branch for an extended message is only taken when
> read_s32(data, 4) < 0 and Daniel's patch fixes that branch.
>
> I noticed in qemu_clipboard_set_data():
>
> > info->types[type].data = g_memdup(data, size);
>
> the g_memdup call will return NULL when size == 0 even if data is
> non-NULL. Is that the actual problem in the above scenario?

Hmm, qemu_clipboard_set_data() shouldn't allow data == NULL, or size == 0.

When data is requested, the "peer" will call
qemu_clipboard_set_data(). But I can't see a good way for the peer to
return with no data, it should probably update the clipboard info with
available=false.

What do you think?

-- 
Marc-André Lureau