| Message ID | 20240117110109.287430-1-f.ebner@proxmox.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] ui/clipboard: ensure data is available or request callback is set upon update | expand |
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, ¬ify); > > 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)
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?
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
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, ¬ify); 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); }
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(+)