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

[Fiona Ebner]

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
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?

Best Regards,
Fiona