From: Daniel P. Berrangé <berrange@redhat.com> Currently when using VNC the "password" flag turns on password based authentication. The actual password has to be provided separately via the monitor. This introduces a "password-secret" option which lets the password be provided up front. $QEMU --object secret,id=vncsec0,file=passwd.txt \ --vnc localhost:0,password-secret=vncsec0 Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> Message-Id: <20210311114343.439820-2-berrange@redhat.com> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- ui/vnc.c | 23 ++++++++++++++++++++++- qemu-options.hx | 5 +++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/ui/vnc.c b/ui/vnc.c index 310abc937812..e8e3426a65c4 100644 --- a/ui/vnc.c +++ b/ui/vnc.c @@ -48,6 +48,7 @@ #include "crypto/tlscredsanon.h" #include "crypto/tlscredsx509.h" #include "crypto/random.h" +#include "crypto/secret_common.h" #include "qom/object_interfaces.h" #include "qemu/cutils.h" #include "qemu/help_option.h" @@ -3459,6 +3460,9 @@ static QemuOptsList qemu_vnc_opts = { },{ .name = "password", .type = QEMU_OPT_BOOL, + },{ + .name = "password-secret", + .type = QEMU_OPT_STRING, },{ .name = "reverse", .type = QEMU_OPT_BOOL, @@ -3931,6 +3935,7 @@ void vnc_display_open(const char *id, Error **errp) int lock_key_sync = 1; int key_delay_ms; const char *audiodev; + const char *passwordSecret; if (!vd) { error_setg(errp, "VNC display not active"); @@ -3948,7 +3953,23 @@ void vnc_display_open(const char *id, Error **errp) goto fail; } - password = qemu_opt_get_bool(opts, "password", false); + + passwordSecret = qemu_opt_get(opts, "password-secret"); + if (passwordSecret) { + if (qemu_opt_get(opts, "password")) { + error_setg(errp, + "'password' flag is redundant with 'password-secret'"); + goto fail; + } + vd->password = qcrypto_secret_lookup_as_utf8(passwordSecret, + errp); + if (!vd->password) { + goto fail; + } + password = true; + } else { + password = qemu_opt_get_bool(opts, "password", false); + } if (password) { if (fips_get_state()) { error_setg(errp, diff --git a/qemu-options.hx b/qemu-options.hx index 622d3bfa5a7d..357fc4596ecc 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -2165,6 +2165,11 @@ SRST time to allow <protocol> password to expire immediately or never expire. + ``password-secret=<secret-id>`` + Require that password based authentication is used for client + connections, using the password provided by the ``secret`` + object identified by ``secret-id``. + ``tls-creds=ID`` Provides the ID of a set of TLS credentials to use to secure the VNC server. They will apply to both the normal VNC server socket -- 2.29.2

From: Daniel P. Berrangé <berrange@redhat.com> With the new "password-secret" option, there is no reason to use the old inecure "password" option with -spice, so it can be deprecated. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> Message-Id: <20210311114343.439820-4-berrange@redhat.com> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- ui/spice-core.c | 2 ++ docs/system/deprecated.rst | 8 ++++++++ qemu-options.hx | 4 ++++ 3 files changed, 14 insertions(+) diff --git a/ui/spice-core.c b/ui/spice-core.c index b9d8aced91df..272d19b0c152 100644 --- a/ui/spice-core.c +++ b/ui/spice-core.c @@ -686,6 +686,8 @@ static void qemu_spice_init(void) } else { str = qemu_opt_get(opts, "password"); if (str) { + warn_report("'password' option is deprecated and insecure, " + "use 'password-secret' instead"); password = g_strdup(str); } } diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst index 5e3a31c12361..8cba672a7b4a 100644 --- a/docs/system/deprecated.rst +++ b/docs/system/deprecated.rst @@ -174,6 +174,14 @@ Input parameters that take a size value should only use a size suffix the value is hexadecimal. That is, '0x20M' is deprecated, and should be written either as '32M' or as '0x2000000'. +``-spice password=string`` (since 6.0) +'''''''''''''''''''''''''''''''''''''' + +This option is insecure because the SPICE password remains visible in +the process listing. This is replaced by the new ``password-secret`` +option which lets the password be securely provided on the command +line using a ``secret`` object instance. + QEMU Machine Protocol (QMP) commands ------------------------------------ diff --git a/qemu-options.hx b/qemu-options.hx index a98f8e84a29d..4da3f4f48c03 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -1928,6 +1928,10 @@ SRST ``password=<string>`` Set the password you need to authenticate. + This option is deprecated and insecure because it leaves the + password visible in the process listing. Use ``password-secret`` + instead. + ``password-secret=<secret-id>`` Set the ID of the ``secret`` object containing the password you need to authenticate. -- 2.29.2

From: Akihiko Odaki <akihiko.odaki@gmail.com> ui/cocoa used to call exit immediately after calling qemu_system_shutdown_request, which prevents QEMU from actually perfoming system shutdown. Just sleep forever, and wait QEMU to call exit and kill the Cocoa thread. Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com> Message-Id: <20210219111652.20623-1-akihiko.odaki@gmail.com> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- ui/cocoa.m | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/ui/cocoa.m b/ui/cocoa.m index a7848ae0a30e..9da0e884b712 100644 --- a/ui/cocoa.m +++ b/ui/cocoa.m @@ -1115,7 +1115,13 @@ QemuCocoaView *cocoaView; COCOA_DEBUG("QemuCocoaAppController: applicationWillTerminate\n"); qemu_system_shutdown_request(SHUTDOWN_CAUSE_HOST_UI); - exit(0); + + /* + * Sleep here, because returning will cause OSX to kill us + * immediately; the QEMU main loop will handle the shutdown + * request and terminate the process. + */ + [NSThread sleepForTimeInterval:INFINITY]; } - (BOOL)applicationShouldTerminateAfterLastWindowClosed:(NSApplication *)theApplication -- 2.29.2

From: Daniel P. Berrangé <berrange@redhat.com> We plan framebuffer update rects based on the VNC server surface. If the client doesn't support desktop resize, then the client bounds may differ from the server surface bounds. VNC clients may become upset if we then send an update message outside the bounds of the client desktop. This takes the approach of clamping the rectangles from the worker thread immediately before sending them. This may sometimes results in sending a framebuffer update message with zero rectangles. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Message-Id: <20210311182957.486939-3-berrange@redhat.com> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- ui/vnc-jobs.c | 44 ++++++++++++++++++++++++++++++++++++++++---- ui/trace-events | 5 +++++ 2 files changed, 45 insertions(+), 4 deletions(-) diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c index dbbfbefe5619..4562bf89288e 100644 --- a/ui/vnc-jobs.c +++ b/ui/vnc-jobs.c @@ -32,6 +32,7 @@ #include "qemu/sockets.h" #include "qemu/main-loop.h" #include "block/aio.h" +#include "trace.h" /* * Locking: @@ -94,6 +95,8 @@ int vnc_job_add_rect(VncJob *job, int x, int y, int w, int h) { VncRectEntry *entry = g_new0(VncRectEntry, 1); + trace_vnc_job_add_rect(job->vs, job, x, y, w, h); + entry->rect.x = x; entry->rect.y = y; entry->rect.w = w; @@ -190,6 +193,8 @@ static void vnc_async_encoding_start(VncState *orig, VncState *local) local->zlib = orig->zlib; local->hextile = orig->hextile; local->zrle = orig->zrle; + local->client_width = orig->client_width; + local->client_height = orig->client_height; } static void vnc_async_encoding_end(VncState *orig, VncState *local) @@ -202,6 +207,34 @@ static void vnc_async_encoding_end(VncState *orig, VncState *local) orig->lossy_rect = local->lossy_rect; } +static bool vnc_worker_clamp_rect(VncState *vs, VncJob *job, VncRect *rect) +{ + trace_vnc_job_clamp_rect(vs, job, rect->x, rect->y, rect->w, rect->h); + + if (rect->x >= vs->client_width) { + goto discard; + } + rect->w = MIN(vs->client_width - rect->x, rect->w); + if (rect->w == 0) { + goto discard; + } + + if (rect->y >= vs->client_height) { + goto discard; + } + rect->h = MIN(vs->client_height - rect->y, rect->h); + if (rect->h == 0) { + goto discard; + } + + trace_vnc_job_clamped_rect(vs, job, rect->x, rect->y, rect->w, rect->h); + return true; + + discard: + trace_vnc_job_discard_rect(vs, job, rect->x, rect->y, rect->w, rect->h); + return false; +} + static int vnc_worker_thread_loop(VncJobQueue *queue) { VncJob *job; @@ -260,14 +293,17 @@ static int vnc_worker_thread_loop(VncJobQueue *queue) goto disconnected; } - n = vnc_send_framebuffer_update(&vs, entry->rect.x, entry->rect.y, - entry->rect.w, entry->rect.h); + if (vnc_worker_clamp_rect(&vs, job, &entry->rect)) { + n = vnc_send_framebuffer_update(&vs, entry->rect.x, entry->rect.y, + entry->rect.w, entry->rect.h); - if (n >= 0) { - n_rectangles += n; + if (n >= 0) { + n_rectangles += n; + } } g_free(entry); } + trace_vnc_job_nrects(&vs, job, n_rectangles); vnc_unlock_display(job->vs->vd); /* Put n_rectangles at the beginning of the message */ diff --git a/ui/trace-events b/ui/trace-events index bd8f8a9d186a..3838ae2d849a 100644 --- a/ui/trace-events +++ b/ui/trace-events @@ -59,6 +59,11 @@ vnc_client_throttle_audio(void *state, void *ioc, size_t offset) "VNC client thr vnc_client_unthrottle_forced(void *state, void *ioc) "VNC client unthrottle forced offset state=%p ioc=%p" vnc_client_unthrottle_incremental(void *state, void *ioc, size_t offset) "VNC client unthrottle incremental state=%p ioc=%p offset=%zu" vnc_client_output_limit(void *state, void *ioc, size_t offset, size_t threshold) "VNC client output limit state=%p ioc=%p offset=%zu threshold=%zu" +vnc_job_add_rect(void *state, void *job, int x, int y, int w, int h) "VNC add rect state=%p job=%p offset=%d,%d size=%dx%d" +vnc_job_discard_rect(void *state, void *job, int x, int y, int w, int h) "VNC job discard rect state=%p job=%p offset=%d,%d size=%dx%d" +vnc_job_clamp_rect(void *state, void *job, int x, int y, int w, int h) "VNC job clamp rect state=%p job=%p offset=%d,%d size=%dx%d" +vnc_job_clamped_rect(void *state, void *job, int x, int y, int w, int h) "VNC job clamp rect state=%p job=%p offset=%d,%d size=%dx%d" +vnc_job_nrects(void *state, void *job, int nrects) "VNC job state=%p job=%p nrects=%d" vnc_auth_init(void *display, int websock, int auth, int subauth) "VNC auth init state=%p websock=%d auth=%d subauth=%d" vnc_auth_start(void *state, int method) "VNC client auth start state=%p method=%d" vnc_auth_pass(void *state, int method) "VNC client auth passed state=%p method=%d" -- 2.29.2

From: Daniel P. Berrangé <berrange@redhat.com> A long time ago the VNC server code had some memory corruption fixes done in: commit bea60dd7679364493a0d7f5b54316c767cf894ef Author: Peter Lieven <pl@kamp.de> Date: Mon Jun 30 10:57:51 2014 +0200 ui/vnc: fix potential memory corruption issues One of the implications of the fix was that the VNC server would have a thin black bad down the right hand side if the guest desktop width was not a multiple of 16. In practice this was a non-issue since the VNC server was always honouring a guest specified resolution and guests essentially always pick from a small set of sane resolutions likely in real world hardware. We recently introduced support for the extended desktop resize extension and as a result the VNC client has ability to specify an arbitrary desktop size and the guest OS may well honour it exactly. As a result we no longer have any guarantee that the width will be a multiple of 16, and so when resizing the desktop we have a 93% chance of getting the black bar on the right hand size. The VNC server maintains three different desktop dimensions 1. The guest surface 2. The server surface 3. The client desktop The requirement for the width to be a multiple of 16 only applies to item 2, the server surface, for the purpose of doing dirty bitmap tracking. Normally we will set the client desktop size to always match the server surface size, but that's not a strict requirement. In order to cope with clients that don't support the desktop size encoding, we already allow for the client desktop to be a different size that the server surface. Thus we can trivially eliminate the black bar, but setting the client desktop size to be the un-rounded server surface size - the so called "true width". Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Message-Id: <20210311182957.486939-5-berrange@redhat.com> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- ui/vnc.h | 1 + ui/vnc.c | 23 +++++++++++++++++++---- ui/trace-events | 2 ++ 3 files changed, 22 insertions(+), 4 deletions(-) diff --git a/ui/vnc.h b/ui/vnc.h index 116463d5f099..d4f3e1555809 100644 --- a/ui/vnc.h +++ b/ui/vnc.h @@ -164,6 +164,7 @@ struct VncDisplay struct VncSurface guest; /* guest visible surface (aka ds->surface) */ pixman_image_t *server; /* vnc server surface */ + int true_width; /* server surface width before rounding up */ const char *id; QTAILQ_ENTRY(VncDisplay) next; diff --git a/ui/vnc.c b/ui/vnc.c index 8c9890b3cdc4..9c004a11f495 100644 --- a/ui/vnc.c +++ b/ui/vnc.c @@ -608,6 +608,11 @@ static int vnc_width(VncDisplay *vd) VNC_DIRTY_PIXELS_PER_BIT)); } +static int vnc_true_width(VncDisplay *vd) +{ + return MIN(VNC_MAX_WIDTH, surface_width(vd->ds)); +} + static int vnc_height(VncDisplay *vd) { return MIN(VNC_MAX_HEIGHT, surface_height(vd->ds)); @@ -691,16 +696,16 @@ static void vnc_desktop_resize(VncState *vs) !vnc_has_feature(vs, VNC_FEATURE_RESIZE_EXT))) { return; } - if (vs->client_width == pixman_image_get_width(vs->vd->server) && + if (vs->client_width == vs->vd->true_width && vs->client_height == pixman_image_get_height(vs->vd->server)) { return; } - assert(pixman_image_get_width(vs->vd->server) < 65536 && - pixman_image_get_width(vs->vd->server) >= 0); + assert(vs->vd->true_width < 65536 && + vs->vd->true_width >= 0); assert(pixman_image_get_height(vs->vd->server) < 65536 && pixman_image_get_height(vs->vd->server) >= 0); - vs->client_width = pixman_image_get_width(vs->vd->server); + vs->client_width = vs->vd->true_width; vs->client_height = pixman_image_get_height(vs->vd->server); if (vnc_has_feature(vs, VNC_FEATURE_RESIZE_EXT)) { @@ -774,6 +779,7 @@ static void vnc_update_server_surface(VncDisplay *vd) width = vnc_width(vd); height = vnc_height(vd); + vd->true_width = vnc_true_width(vd); vd->server = pixman_image_create_bits(VNC_SERVER_FB_FORMAT, width, height, NULL, 0); @@ -809,13 +815,22 @@ static void vnc_dpy_switch(DisplayChangeListener *dcl, vd->guest.fb = pixman_image_ref(surface->image); vd->guest.format = surface->format; + if (pageflip) { + trace_vnc_server_dpy_pageflip(vd, + surface_width(surface), + surface_height(surface), + surface_format(surface)); vnc_set_area_dirty(vd->guest.dirty, vd, 0, 0, surface_width(surface), surface_height(surface)); return; } + trace_vnc_server_dpy_recreate(vd, + surface_width(surface), + surface_height(surface), + surface_format(surface)); /* server surface */ vnc_update_server_surface(vd); diff --git a/ui/trace-events b/ui/trace-events index 3838ae2d849a..5d1da6f23668 100644 --- a/ui/trace-events +++ b/ui/trace-events @@ -59,6 +59,8 @@ vnc_client_throttle_audio(void *state, void *ioc, size_t offset) "VNC client thr vnc_client_unthrottle_forced(void *state, void *ioc) "VNC client unthrottle forced offset state=%p ioc=%p" vnc_client_unthrottle_incremental(void *state, void *ioc, size_t offset) "VNC client unthrottle incremental state=%p ioc=%p offset=%zu" vnc_client_output_limit(void *state, void *ioc, size_t offset, size_t threshold) "VNC client output limit state=%p ioc=%p offset=%zu threshold=%zu" +vnc_server_dpy_pageflip(void *dpy, int w, int h, int fmt) "VNC server dpy pageflip dpy=%p size=%dx%d fmt=%d" +vnc_server_dpy_recreate(void *dpy, int w, int h, int fmt) "VNC server dpy recreate dpy=%p size=%dx%d fmt=%d" vnc_job_add_rect(void *state, void *job, int x, int y, int w, int h) "VNC add rect state=%p job=%p offset=%d,%d size=%dx%d" vnc_job_discard_rect(void *state, void *job, int x, int y, int w, int h) "VNC job discard rect state=%p job=%p offset=%d,%d size=%dx%d" vnc_job_clamp_rect(void *state, void *job, int x, int y, int w, int h) "VNC job clamp rect state=%p job=%p offset=%d,%d size=%dx%d" -- 2.29.2

From: Akihiko Odaki <akihiko.odaki@gmail.com> Based-on: <20210310042348.21931-1-akihiko.odaki@gmail.com> Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com> Message-Id: <20210312133212.3131-1-akihiko.odaki@gmail.com> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- ui/cocoa.m | 38 +++++++++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/ui/cocoa.m b/ui/cocoa.m index 9da0e884b712..37e1fb52eb4d 100644 --- a/ui/cocoa.m +++ b/ui/cocoa.m @@ -690,7 +690,43 @@ QemuCocoaView *cocoaView; NSPoint p = [self screenLocationOfEvent:event]; NSUInteger modifiers = [event modifierFlags]; - // emulate caps lock keydown and keyup + /* + * Check -[NSEvent modifierFlags] here. + * + * There is a NSEventType for an event notifying the change of + * -[NSEvent modifierFlags], NSEventTypeFlagsChanged but these operations + * are performed for any events because a modifier state may change while + * the application is inactive (i.e. no events fire) and we don't want to + * wait for another modifier state change to detect such a change. + * + * NSEventModifierFlagCapsLock requires a special treatment. The other flags + * are handled in similar manners. + * + * NSEventModifierFlagCapsLock + * --------------------------- + * + * If CapsLock state is changed, "up" and "down" events will be fired in + * sequence, effectively updates CapsLock state on the guest. + * + * The other flags + * --------------- + * + * If a flag is not set, fire "up" events for all keys which correspond to + * the flag. Note that "down" events are not fired here because the flags + * checked here do not tell what exact keys are down. + * + * If one of the keys corresponding to a flag is down, we rely on + * -[NSEvent keyCode] of an event whose -[NSEvent type] is + * NSEventTypeFlagsChanged to know the exact key which is down, which has + * the following two downsides: + * - It does not work when the application is inactive as described above. + * - It malfactions *after* the modifier state is changed while the + * application is inactive. It is because -[NSEvent keyCode] does not tell + * if the key is up or down, and requires to infer the current state from + * the previous state. It is still possible to fix such a malfanction by + * completely leaving your hands from the keyboard, which hopefully makes + * this implementation usable enough. + */ if (!!(modifiers & NSEventModifierFlagCapsLock) != qkbd_state_modifier_get(kbd, QKBD_MOD_CAPSLOCK)) { qkbd_state_key_event(kbd, Q_KEY_CODE_CAPS_LOCK, true); -- 2.29.2