diff --git a/src/protocols/vnc/argv.h b/src/protocols/vnc/argv.h index 677778206..894b58d43 100644 --- a/src/protocols/vnc/argv.h +++ b/src/protocols/vnc/argv.h @@ -28,6 +28,12 @@ /** * Handles a received argument value from a Guacamole "argv" instruction, * updating the given connection parameter. + * + * As noted in the user.c file, care should be taken when updating this + * callback to make sure that arguments are handled correctly when + * a connection is marked as read-only, and to make sure that any + * usage of this callback for non-owner users of a connection does + * not have unintended security implications. */ guac_argv_callback guac_vnc_argv_callback; diff --git a/src/protocols/vnc/user.c b/src/protocols/vnc/user.c index 74204f624..42bae34aa 100644 --- a/src/protocols/vnc/user.c +++ b/src/protocols/vnc/user.c @@ -85,10 +85,6 @@ int guac_vnc_user_join_handler(guac_user* user, int argc, char** argv) { if (!settings->disable_paste) user->clipboard_handler = guac_vnc_clipboard_handler; - /* Updates to connection parameters if we own the connection */ - if (user->owner) - user->argv_handler = guac_argv_handler; - #ifdef ENABLE_COMMON_SSH /* Set generic (non-filesystem) file upload handler */ if (settings->enable_sftp && !settings->sftp_disable_upload) @@ -97,6 +93,26 @@ int guac_vnc_user_join_handler(guac_user* user, int argc, char** argv) { } + /** + * Update connection parameters if we own the connection. + * + * Note that the argv handler is called *regardless* of whether + * or not the connection is read-only, as this allows authentication + * to be prompted and processed even if the owner cannot send + * input to the remote session. In the future, if other argv handling + * is added to the VNC protocol, checks may need to be done within + * the argv handler to verify that read-only connections remain + * read-only. + * + * Also, this is only handled for the owner - if the argv handler + * is expanded to include non-owner users in the future, special + * care will need to be taken to make sure that the arguments + * processed by the handler do not have unintended security + * implications for non-owner users. + */ + if (user->owner) + user->argv_handler = guac_argv_handler; + return 0; }