From 4cba8388968b70fe20e290221dc421c717051fdd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Mon, 15 Jan 2024 09:51:19 +0000 Subject: [PATCH 1/7] ui: reject extended clipboard message if not activated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The extended clipboard message protocol requires that the client activate the extension by requesting a psuedo encoding. If this is not done, then any extended clipboard messages from the client should be considered invalid and the client dropped. Signed-off-by: Daniel P. Berrangé Reviewed-by: Marc-André Lureau Message-Id: <20240115095119.654271-1-berrange@redhat.com> --- ui/vnc.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ui/vnc.c b/ui/vnc.c index 3db87fd89c..af20d24534 100644 --- a/ui/vnc.c +++ b/ui/vnc.c @@ -2445,6 +2445,11 @@ static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len) } if (read_s32(data, 4) < 0) { + if (!vnc_has_feature(vs, VNC_FEATURE_CLIPBOARD_EXT)) { + error_report("vnc: extended clipboard message while disabled"); + vnc_client_error(vs); + break; + } if (dlen < 4) { error_report("vnc: malformed payload (header less than 4 bytes)" " in extended clipboard pseudo-encoding."); From 405484b29f6548c7b86549b0f961b906337aa68a Mon Sep 17 00:00:00 2001 From: Fiona Ebner Date: Wed, 24 Jan 2024 11:57:48 +0100 Subject: [PATCH 2/7] ui/clipboard: mark type as not available when there is no data MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With VNC, a client can send a non-extended VNC_MSG_CLIENT_CUT_TEXT message with len=0. In qemu_clipboard_set_data(), the clipboard info will be updated setting data to NULL (because g_memdup(data, size) returns NULL when size is 0). If the client does not set the VNC_ENCODING_CLIPBOARD_EXT feature when setting up the encodings, then the 'request' callback for the clipboard peer is not initialized. Later, because data is NULL, qemu_clipboard_request() can be reached via vdagent_chr_write() and vdagent_clipboard_recv_request() and there, the clipboard owner's 'request' callback will be attempted to be called, but that is a NULL pointer. In particular, this can happen when using the KRDC (22.12.3) VNC client. Another scenario leading to the same issue is 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 handling updates with size 0 differently. In particular, mark in the clipboard info that the type is not available. While at it, switch to g_memdup2(), because g_memdup() is deprecated. Cc: qemu-stable@nongnu.org Fixes: CVE-2023-6683 Reported-by: Markus Frank Suggested-by: Marc-André Lureau Signed-off-by: Fiona Ebner Reviewed-by: Marc-André Lureau Tested-by: Markus Frank Message-ID: <20240124105749.204610-1-f.ebner@proxmox.com> --- ui/clipboard.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/ui/clipboard.c b/ui/clipboard.c index 3d14bffaf8..b3f6fa3c9e 100644 --- a/ui/clipboard.c +++ b/ui/clipboard.c @@ -163,9 +163,15 @@ void qemu_clipboard_set_data(QemuClipboardPeer *peer, } g_free(info->types[type].data); - info->types[type].data = g_memdup(data, size); - info->types[type].size = size; - info->types[type].available = true; + if (size) { + info->types[type].data = g_memdup2(data, size); + info->types[type].size = size; + info->types[type].available = true; + } else { + info->types[type].data = NULL; + info->types[type].size = 0; + info->types[type].available = false; + } if (update) { qemu_clipboard_update(info); From 9c416582611b7495bdddb4c5456c7acb64b78938 Mon Sep 17 00:00:00 2001 From: Fiona Ebner Date: Wed, 24 Jan 2024 11:57:49 +0100 Subject: [PATCH 3/7] ui/clipboard: add asserts for update and request MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Should an issue like CVE-2023-6683 ever appear again in the future, it will be more obvious which assumption was violated. Suggested-by: Marc-André Lureau Signed-off-by: Fiona Ebner Reviewed-by: Marc-André Lureau Message-ID: <20240124105749.204610-2-f.ebner@proxmox.com> --- ui/clipboard.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/ui/clipboard.c b/ui/clipboard.c index b3f6fa3c9e..4264884a6c 100644 --- a/ui/clipboard.c +++ b/ui/clipboard.c @@ -65,12 +65,24 @@ bool qemu_clipboard_check_serial(QemuClipboardInfo *info, bool client) void qemu_clipboard_update(QemuClipboardInfo *info) { + uint32_t type; QemuClipboardNotify notify = { .type = QEMU_CLIPBOARD_UPDATE_INFO, .info = info, }; assert(info->selection < QEMU_CLIPBOARD_SELECTION__COUNT); + 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 +144,8 @@ void qemu_clipboard_request(QemuClipboardInfo *info, !info->owner) return; + assert(info->owner->request); + info->types[type].requested = true; info->owner->request(info, type); } From 95b08fee8f68d284a5028d37fd28be7a70c8e92b Mon Sep 17 00:00:00 2001 From: Tianlan Zhou Date: Thu, 8 Feb 2024 01:20:25 +0800 Subject: [PATCH 4/7] ui/console: Fix console resize with placeholder surface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In `qemu_console_resize()`, the old surface of the console is keeped if the new console size is the same as the old one. If the old surface is a placeholder, and the new size of console is the same as the placeholder surface (640*480), the surface won't be replace. In this situation, the surface's `QEMU_PLACEHOLDER_FLAG` flag is still set, so the console won't be displayed in SDL display mode. This patch fixes this problem by forcing a new surface if the old one is a placeholder. Signed-off-by: Tianlan Zhou Reviewed-by: Marc-André Lureau Message-ID: <20240207172024.8-1-bobby825@126.com> --- ui/console.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ui/console.c b/ui/console.c index 7db921e3b7..832055675c 100644 --- a/ui/console.c +++ b/ui/console.c @@ -1577,7 +1577,7 @@ void qemu_console_resize(QemuConsole *s, int width, int height) assert(QEMU_IS_GRAPHIC_CONSOLE(s)); if ((s->scanout.kind != SCANOUT_SURFACE || - (surface && surface->flags & QEMU_ALLOCATED_FLAG)) && + (surface && !is_buffer_shared(surface) && !is_placeholder(surface))) && qemu_console_get_width(s, -1) == width && qemu_console_get_height(s, -1) == height) { return; From d67611907590a1e6c998b7c5a5cb4394acf84329 Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Wed, 14 Feb 2024 23:03:56 +0900 Subject: [PATCH 5/7] audio: Depend on dbus_display1_dep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit dbusaudio needs dbus_display1_dep. Fixes: 739362d4205c ("audio: add "dbus" audio backend") Signed-off-by: Akihiko Odaki Reviewed-by: Marc-André Lureau Message-Id: <20240214-dbus-v7-1-7eff29f04c34@daynix.com> --- audio/meson.build | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/audio/meson.build b/audio/meson.build index c8f658611f..608f35e6af 100644 --- a/audio/meson.build +++ b/audio/meson.build @@ -30,7 +30,8 @@ endforeach if dbus_display module_ss = ss.source_set() - module_ss.add(when: [gio, pixman], if_true: files('dbusaudio.c')) + module_ss.add(when: [gio, dbus_display1_dep, pixman], + if_true: files('dbusaudio.c')) audio_modules += {'dbus': module_ss} endif From 7aee57df930da2cf6361c5183aff96468ae4027d Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Wed, 14 Feb 2024 23:03:57 +0900 Subject: [PATCH 6/7] meson: Explicitly specify dbus-display1.h dependency MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Explicitly specify dbus-display1.h as a dependency so that files depending on it will not get compiled too early. Fixes: 1222070e7728 ("meson: ensure dbus-display generated code is built before other units") Signed-off-by: Akihiko Odaki Reviewed-by: Marc-André Lureau Message-Id: <20240214-dbus-v7-2-7eff29f04c34@daynix.com> --- ui/meson.build | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ui/meson.build b/ui/meson.build index 376e0d771b..0b7e2b6f6b 100644 --- a/ui/meson.build +++ b/ui/meson.build @@ -91,7 +91,7 @@ if dbus_display '--c-namespace', 'QemuDBus', '--generate-c-code', '@BASENAME@']) dbus_display1_lib = static_library('dbus-display1', dbus_display1, dependencies: gio) - dbus_display1_dep = declare_dependency(link_with: dbus_display1_lib, include_directories: include_directories('.')) + dbus_display1_dep = declare_dependency(link_with: dbus_display1_lib, sources: dbus_display1[0]) dbus_ss.add(when: [gio, dbus_display1_dep], if_true: [files( 'dbus-chardev.c', From 186acfbaf7f325833702f50f75ef5116dc29e233 Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Wed, 14 Feb 2024 23:03:58 +0900 Subject: [PATCH 7/7] tests/qtest: Depend on dbus_display1_dep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It ensures dbus-display1.c will not be recompiled. Signed-off-by: Akihiko Odaki Reviewed-by: Marc-André Lureau Message-Id: <20240214-dbus-v7-3-7eff29f04c34@daynix.com> --- tests/qtest/meson.build | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build index 2b89e8634b..430d49b409 100644 --- a/tests/qtest/meson.build +++ b/tests/qtest/meson.build @@ -344,7 +344,7 @@ if vnc.found() endif if dbus_display - qtests += {'dbus-display-test': [dbus_display1, gio]} + qtests += {'dbus-display-test': [dbus_display1_dep, gio]} endif qtest_executables = {}