With VNC, it can be that a client sends a VNC_MSG_CLIENT_CUT_TEXT
message before sending a VNC_MSG_CLIENT_SET_ENCODINGS message with
VNC_ENCODING_CLIPBOARD_EXT for configuring the clipboard extension.

This means that qemu_clipboard_request() can be reached (via
vnc_client_cut_text_ext()) before vnc_server_cut_text_caps() was
called and had the chance to initialize the clipboard peer. In that
case, info->owner->request is NULL instead of a function and so
attempting to call it in qemu_clipboard_request() results in a
segfault.

In particular, this can happen when using the KRDC (22.12.3) VNC
client on Wayland.

Another code path leading to the same issue in
qemu_clipboard_request() is via vdagent_chr_write() and
vdagent_clipboard_recv_request() after a non-extended
VNC_MSG_CLIENT_CUT_TEXT messages with len=0 was sent.

In particular, this can happen when using the KRDC (22.12.3) VNC
client on X11.

It is not enough to check in ui/vnc.c's protocol_client_msg() if the
VNC_FEATURE_CLIPBOARD_EXT feature is enabled before handling an
extended clipboard message with vnc_client_cut_text_ext(), because of
the following scenario with two clients (say noVNC and KRDC):

The noVNC client sets the extension VNC_FEATURE_CLIPBOARD_EXT and
initializes its cbpeer.

The KRDC client does not, but triggers a vnc_client_cut_text() (note
it's not the _ext variant)). There, a new clipboard info with it as
the 'owner' is created and via qemu_clipboard_set_data() is called,
which in turn calls qemu_clipboard_update() with that info.

In qemu_clipboard_update(), the notifier for the noVNC client will be
called, i.e. vnc_clipboard_notify() and also set vs->cbinfo for the
noVNC client. The 'owner' in that clipboard info is the clipboard peer
for the KRDC client, which did not initialize the 'request' function.
That sounds correct to me, it is the owner of that clipboard info.

Then when noVNC sends a VNC_MSG_CLIENT_CUT_TEXT message (it did set
the VNC_FEATURE_CLIPBOARD_EXT feature correctly, so a check for it
passes), that clipboard info is passed to qemu_clipboard_request() and
the original segfault still happens.

Fix the issue by disallowing clipboard update if both, data is missing
and the clipboard owner's 'request' callback is not set.

Add an assert that the clipboard owner's 'request' callback is set in
qemu_clipboard_request() to have a clean error/abort should a similar
issue appear in the future.

Fixes: CVE-2023-6683
Reported-by: Markus Frank <m.frank@proxmox.com>
Suggested-by: Marc-André Lureau <marcandre.lureau@gmail.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

Changes in v2:
    * Different approach as suggested by Marc-André:
      Instead of quietly returning in qemu_clipboard_request() when no
      request callback is set, prevent clipboard update if there is
      both, no data and no request callback.

 ui/clipboard.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/ui/clipboard.c b/ui/clipboard.c
index 3d14bffaf8..d1bb7c77f2 100644
--- a/ui/clipboard.c
+++ b/ui/clipboard.c
@@ -65,12 +65,28 @@ bool qemu_clipboard_check_serial(QemuClipboardInfo *info, bool client)
 
 void qemu_clipboard_update(QemuClipboardInfo *info)
 {
+    uint32_t type;
+    bool missing_data = false;
     QemuClipboardNotify notify = {
         .type = QEMU_CLIPBOARD_UPDATE_INFO,
         .info = info,
     };
     assert(info->selection < QEMU_CLIPBOARD_SELECTION__COUNT);
 
+    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;
+    }
+
     notifier_list_notify(&clipboard_notifiers, &notify);
 
     if (cbinfo[info->selection] != info) {
@@ -132,6 +148,8 @@ void qemu_clipboard_request(QemuClipboardInfo *info,
         !info->owner)
         return;
 
+    assert(info->owner->request);
+
     info->types[type].requested = true;
     info->owner->request(info, type);
 }
-- 
2.39.2

Hi

On Wed, Jan 17, 2024 at 3:01 PM Fiona Ebner <f.ebner@proxmox.com> wrote:
>
> With VNC, it can be that a client sends a VNC_MSG_CLIENT_CUT_TEXT
> message before sending a VNC_MSG_CLIENT_SET_ENCODINGS message with
> VNC_ENCODING_CLIPBOARD_EXT for configuring the clipboard extension.
>
> This means that qemu_clipboard_request() can be reached (via
> vnc_client_cut_text_ext()) before vnc_server_cut_text_caps() was
> called and had the chance to initialize the clipboard peer. In that
> case, info->owner->request is NULL instead of a function and so
> attempting to call it in qemu_clipboard_request() results in a
> segfault.
>
> In particular, this can happen when using the KRDC (22.12.3) VNC
> client on Wayland.
>
> Another code path leading to the same issue in
> qemu_clipboard_request() is via vdagent_chr_write() and
> vdagent_clipboard_recv_request() after a non-extended
> VNC_MSG_CLIENT_CUT_TEXT messages with len=0 was sent.
>
> In particular, this can happen when using the KRDC (22.12.3) VNC
> client on X11.
>
> It is not enough to check in ui/vnc.c's protocol_client_msg() if the
> VNC_FEATURE_CLIPBOARD_EXT feature is enabled before handling an
> extended clipboard message with vnc_client_cut_text_ext(), because of
> the following scenario with two clients (say noVNC and KRDC):
>
> The noVNC client sets the extension VNC_FEATURE_CLIPBOARD_EXT and
> initializes its cbpeer.
>
> The KRDC client does not, but triggers a vnc_client_cut_text() (note
> it's not the _ext variant)). There, a new clipboard info with it as
> the 'owner' is created and via qemu_clipboard_set_data() is called,
> which in turn calls qemu_clipboard_update() with that info.
>
> In qemu_clipboard_update(), the notifier for the noVNC client will be
> called, i.e. vnc_clipboard_notify() and also set vs->cbinfo for the
> noVNC client. The 'owner' in that clipboard info is the clipboard peer
> for the KRDC client, which did not initialize the 'request' function.
> That sounds correct to me, it is the owner of that clipboard info.
>
> Then when noVNC sends a VNC_MSG_CLIENT_CUT_TEXT message (it did set
> the VNC_FEATURE_CLIPBOARD_EXT feature correctly, so a check for it
> passes), that clipboard info is passed to qemu_clipboard_request() and
> the original segfault still happens.
>
> Fix the issue by disallowing clipboard update if both, data is missing
> and the clipboard owner's 'request' callback is not set.
>
> Add an assert that the clipboard owner's 'request' callback is set in
> qemu_clipboard_request() to have a clean error/abort should a similar
> issue appear in the future.
>
> Fixes: CVE-2023-6683
> Reported-by: Markus Frank <m.frank@proxmox.com>
> Suggested-by: Marc-André Lureau <marcandre.lureau@gmail.com>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>
> Changes in v2:
>     * Different approach as suggested by Marc-André:
>       Instead of quietly returning in qemu_clipboard_request() when no
>       request callback is set, prevent clipboard update if there is
>       both, no data and no request callback.
>
>  ui/clipboard.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/ui/clipboard.c b/ui/clipboard.c
> index 3d14bffaf8..d1bb7c77f2 100644
> --- a/ui/clipboard.c
> +++ b/ui/clipboard.c
> @@ -65,12 +65,28 @@ bool qemu_clipboard_check_serial(QemuClipboardInfo *info, bool client)
>
>  void qemu_clipboard_update(QemuClipboardInfo *info)
>  {
> +    uint32_t type;
> +    bool missing_data = false;
>      QemuClipboardNotify notify = {
>          .type = QEMU_CLIPBOARD_UPDATE_INFO,
>          .info = info,
>      };
>      assert(info->selection < QEMU_CLIPBOARD_SELECTION__COUNT);
>
> +    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);
    }
}

> +
>      notifier_list_notify(&clipboard_notifiers, &notify);
>
>      if (cbinfo[info->selection] != info) {
> @@ -132,6 +148,8 @@ void qemu_clipboard_request(QemuClipboardInfo *info,
>          !info->owner)
>          return;
>
> +    assert(info->owner->request);
> +
>      info->types[type].requested = true;
>      info->owner->request(info, type);
>  }
> --
> 2.39.2
>
>

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.

Best Regards,
Fiona

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)

-- 
Marc-André Lureau

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

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

Am 17.01.24 um 13:20 schrieb 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.
> 

So have vnc_client_cut_text() call qemu_clipboard_set_data() with
data="" and size=1 in the 0-length case? I can see such a call with
noVNC as the client (which does use extended messages) when clearing the
clipboard.

But I suppose the call in vnc_client_cut_text_ext() also would need a
check before calling qemu_clipboard_set_data(). Or is there anything
guaranteeing the size read from the inflated buffer, i.e.
uint32_t tsize = read_u32(buf, 0);
which is passed to qemu_clipboard_set_data() is not zero?

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

Oh, I guess my suggestion above isn't necessary if we set
available=false when size == 0. Sorry, is this what you mean by "no
data"? Or do you mean the case where data=""? Or both?

Best Regards,
Fiona