From 3b0abe376ef3477c32cf9907e68c9c4dafdaf865 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Mon, 24 Feb 2020 16:31:28 -0800 Subject: [PATCH 1/3] GUACAMOLE-962: Request relaxed RDP order checks if supported by FreeRDP. --- configure.ac | 15 +++++++++++++++ src/protocols/rdp/settings.c | 5 +++++ 2 files changed, 20 insertions(+) diff --git a/configure.ac b/configure.ac index dba5f715..24050b9e 100644 --- a/configure.ac +++ b/configure.ac @@ -722,6 +722,21 @@ then fi +# Support for receiving unannounced orders from the RDP server +if test "x${have_freerdp2}" = "xyes" +then + AC_CHECK_MEMBERS([rdpSettings.AllowUnanouncedOrdersFromServer],, + [AC_MSG_WARN([ + -------------------------------------------- + This version of FreeRDP does not support relaxed order checks. RDP servers + that send orders that the client did not announce as supported (such as the + VirtualBox RDP server) will likely not be usable. + + See: https://issues.apache.org/jira/browse/GUACAMOLE-962 + --------------------------------------------])], + [[#include ]]) +fi + # Restore CPPFLAGS, removing FreeRDP-specific options needed for testing CPPFLAGS="$OLDCPPFLAGS" diff --git a/src/protocols/rdp/settings.c b/src/protocols/rdp/settings.c index fe2cf672..864e6427 100644 --- a/src/protocols/rdp/settings.c +++ b/src/protocols/rdp/settings.c @@ -1356,5 +1356,10 @@ void guac_rdp_push_settings(guac_client* client, rdp_settings->OrderSupport[NEG_FAST_INDEX_INDEX] = !guac_settings->disable_glyph_caching; rdp_settings->OrderSupport[NEG_FAST_GLYPH_INDEX] = !guac_settings->disable_glyph_caching; +#ifdef HAVE_RDPSETTINGS_ALLOWUNANOUNCEDORDERSFROMSERVER + /* Do not consider server use of unannounced orders to be a fatal error */ + rdp_settings->AllowUnanouncedOrdersFromServer = TRUE; +#endif + } From a80cd8db06ab3f7fbf049b03670d83389dee155d Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Mon, 24 Feb 2020 16:32:15 -0800 Subject: [PATCH 2/3] GUACAMOLE-962: Restore OpaqueRect and PatBlt handlers. This commit effectively reverts commit 9855d875c794e9517567e89ad13acccd7e7e03d0. With relaxed order checks enabled, FreeRDP will indeed invoke the OpaqueRect and PatBlt handlers (even though we do not announce support for those orders) as long as handlers are provided. --- src/protocols/rdp/gdi.c | 95 +++++++++++++++++++++++++++++++++++++++++ src/protocols/rdp/gdi.h | 39 +++++++++++++++++ src/protocols/rdp/rdp.c | 2 + 3 files changed, 136 insertions(+) diff --git a/src/protocols/rdp/gdi.c b/src/protocols/rdp/gdi.c index feb6505d..1cbee144 100644 --- a/src/protocols/rdp/gdi.c +++ b/src/protocols/rdp/gdi.c @@ -18,6 +18,7 @@ */ #include "bitmap.h" +#include "color.h" #include "common/display.h" #include "common/surface.h" #include "rdp.h" @@ -139,6 +140,76 @@ BOOL guac_rdp_gdi_dstblt(rdpContext* context, const DSTBLT_ORDER* dstblt) { } +BOOL guac_rdp_gdi_patblt(rdpContext* context, PATBLT_ORDER* patblt) { + + /* + * Note that this is not a full implementation of PATBLT. This is a + * fallback implementation which only renders a solid block of background + * color using the specified ROP3 operation, ignoring whatever brush + * was actually specified. + * + * As libguac-client-rdp explicitly tells the server not to send PATBLT, + * well-behaved RDP servers will not use this operation at all, while + * others will at least have a fallback. + */ + + /* Get client and current layer */ + guac_client* client = ((rdp_freerdp_context*) context)->client; + guac_common_surface* current_surface = + ((guac_rdp_client*) client->data)->current_surface; + + int x = patblt->nLeftRect; + int y = patblt->nTopRect; + int w = patblt->nWidth; + int h = patblt->nHeight; + + /* + * Warn that rendering is a fallback, as the server should not be sending + * this order. + */ + guac_client_log(client, GUAC_LOG_INFO, "Using fallback PATBLT (server is ignoring " + "negotiated client capabilities)"); + + /* Render rectangle based on ROP */ + switch (patblt->bRop) { + + /* If blackness, send black rectangle */ + case 0x00: + guac_common_surface_set(current_surface, x, y, w, h, + 0x00, 0x00, 0x00, 0xFF); + break; + + /* If NOP, do nothing */ + case 0xAA: + break; + + /* If operation is just a copy, send foreground only */ + case 0xCC: + case 0xF0: + guac_common_surface_set(current_surface, x, y, w, h, + (patblt->foreColor >> 16) & 0xFF, + (patblt->foreColor >> 8 ) & 0xFF, + (patblt->foreColor ) & 0xFF, + 0xFF); + break; + + /* If whiteness, send white rectangle */ + case 0xFF: + guac_common_surface_set(current_surface, x, y, w, h, + 0xFF, 0xFF, 0xFF, 0xFF); + break; + + /* Otherwise, invert entire rect */ + default: + guac_common_surface_transfer(current_surface, x, y, w, h, + GUAC_TRANSFER_BINARY_NDEST, current_surface, x, y); + + } + + return TRUE; + +} + BOOL guac_rdp_gdi_scrblt(rdpContext* context, const SCRBLT_ORDER* scrblt) { guac_client* client = ((rdp_freerdp_context*) context)->client; @@ -256,6 +327,30 @@ BOOL guac_rdp_gdi_memblt(rdpContext* context, MEMBLT_ORDER* memblt) { } +BOOL guac_rdp_gdi_opaquerect(rdpContext* context, const OPAQUE_RECT_ORDER* opaque_rect) { + + /* Get client data */ + guac_client* client = ((rdp_freerdp_context*) context)->client; + + UINT32 color = guac_rdp_convert_color(context, opaque_rect->color); + + guac_common_surface* current_surface = ((guac_rdp_client*) client->data)->current_surface; + + int x = opaque_rect->nLeftRect; + int y = opaque_rect->nTopRect; + int w = opaque_rect->nWidth; + int h = opaque_rect->nHeight; + + guac_common_surface_set(current_surface, x, y, w, h, + (color >> 16) & 0xFF, + (color >> 8 ) & 0xFF, + (color ) & 0xFF, + 0xFF); + + return TRUE; + +} + BOOL guac_rdp_gdi_set_bounds(rdpContext* context, const rdpBounds* bounds) { guac_client* client = ((rdp_freerdp_context*) context)->client; diff --git a/src/protocols/rdp/gdi.h b/src/protocols/rdp/gdi.h index 9ad5713c..76735aa5 100644 --- a/src/protocols/rdp/gdi.h +++ b/src/protocols/rdp/gdi.h @@ -62,6 +62,25 @@ guac_composite_mode guac_rdp_rop3_transfer_function(guac_client* client, */ BOOL guac_rdp_gdi_dstblt(rdpContext* context, const DSTBLT_ORDER* dstblt); +/** + * Handler for the PatBlt Primary Drawing Order. A PatBlt Primary Drawing Order + * paints a rectangle of image data, a brush pattern, and a three-way raster + * operation which considers the source data, the destination, AND the brush + * pattern. See: + * + * https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpegdi/bd4bf5e7-b988-45f9-8201-3b22cc9aeeb8 + * + * @param context + * The rdpContext associated with the current RDP session. + * + * @param patblt + * The PATBLT update to handle. + * + * @return + * TRUE if successful, FALSE otherwise. + */ +BOOL guac_rdp_gdi_patblt(rdpContext* context, PATBLT_ORDER* patblt); + /** * Handler for the ScrBlt Primary Drawing Order. A ScrBlt Primary Drawing Order * paints a rectangle of image data using a raster operation which considers @@ -98,6 +117,26 @@ BOOL guac_rdp_gdi_scrblt(rdpContext* context, const SCRBLT_ORDER* scrblt); */ BOOL guac_rdp_gdi_memblt(rdpContext* context, MEMBLT_ORDER* memblt); +/** + * Handler for the OpaqueRect Primary Drawing Order. An OpaqueRect Primary + * Drawing Order draws an opaque rectangle of a single solid color. Note that + * support for OpaqueRect cannot be claimed without also supporting PatBlt, as + * both use the same negotiation order number. See: + * + * https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpegdi/1eead7aa-ac63-411a-9f8c-b1b227526877 + * + * @param context + * The rdpContext associated with the current RDP session. + * + * @param opaque_rect + * The OPAQUE RECT update to handle. + * + * @return + * TRUE if successful, FALSE otherwise. + */ +BOOL guac_rdp_gdi_opaquerect(rdpContext* context, + const OPAQUE_RECT_ORDER* opaque_rect); + /** * Handler called prior to calling the handlers for specific updates when * those updates are clipped by a bounding rectangle. This is not a true RDP diff --git a/src/protocols/rdp/rdp.c b/src/protocols/rdp/rdp.c index d79af533..9388d0cd 100644 --- a/src/protocols/rdp/rdp.c +++ b/src/protocols/rdp/rdp.c @@ -179,8 +179,10 @@ BOOL rdp_freerdp_pre_connect(freerdp* instance) { rdpPrimaryUpdate* primary = instance->update->primary; primary->DstBlt = guac_rdp_gdi_dstblt; + primary->PatBlt = guac_rdp_gdi_patblt; primary->ScrBlt = guac_rdp_gdi_scrblt; primary->MemBlt = guac_rdp_gdi_memblt; + primary->OpaqueRect = guac_rdp_gdi_opaquerect; pointer_cache_register_callbacks(instance->update); glyph_cache_register_callbacks(instance->update); From 8ea9b14a806f63dfa123838d2e82bed70340ab02 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Mon, 24 Feb 2020 17:55:06 -0800 Subject: [PATCH 3/3] GUACAMOLE-818: Break SFTP directory JSON at blob boundaries. Do not skip entries. The intent of the previous version of the SFTP directory listing code was to break the JSON transfer at blob boundaries, waiting for an ack before sending the next blob, however the ordering of the "blob_written" and directory read checks could result in a directory entry being skipped at the boundary of each blob. The proper order would be to check the "blob_written" flag first, however the "blob_written" flag is unnecessary. It's simpler and more correct to just break out of the loop once the desired blob has been flushed. --- src/common-ssh/sftp.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/common-ssh/sftp.c b/src/common-ssh/sftp.c index b05409b5..d57a1826 100644 --- a/src/common-ssh/sftp.c +++ b/src/common-ssh/sftp.c @@ -586,7 +586,6 @@ static int guac_common_ssh_sftp_ls_ack_handler(guac_user* user, guac_stream* stream, char* message, guac_protocol_status status) { int bytes_read; - int blob_written = 0; char filename[GUAC_COMMON_SSH_SFTP_MAX_PATH]; LIBSSH2_SFTP_ATTRIBUTES attributes; @@ -608,8 +607,7 @@ static int guac_common_ssh_sftp_ls_ack_handler(guac_user* user, /* While directory entries remain */ while ((bytes_read = libssh2_sftp_readdir(list_state->directory, - filename, sizeof(filename), &attributes)) > 0 - && !blob_written) { + filename, sizeof(filename), &attributes)) > 0) { char absolute_path[GUAC_COMMON_SSH_SFTP_MAX_PATH]; @@ -639,9 +637,10 @@ static int guac_common_ssh_sftp_ls_ack_handler(guac_user* user, else mimetype = "application/octet-stream"; - /* Write entry */ - blob_written |= guac_common_json_write_property(user, stream, - &list_state->json_state, absolute_path, mimetype); + /* Write entry, waiting for next ack if a blob is written */ + if (guac_common_json_write_property(user, stream, + &list_state->json_state, absolute_path, mimetype)) + break; }