From e9a10d66b79c9fc0723dd35c1001d133e796efff Mon Sep 17 00:00:00 2001 From: Nick Couchman Date: Sat, 9 Mar 2019 17:56:55 -0500 Subject: [PATCH 1/7] GUACAMOLE-414: Add pthread lock and callbacks for TLS write locking. --- src/protocols/vnc/vnc.c | 64 ++++++++++++++++++++++++++++++++++++++++- src/protocols/vnc/vnc.h | 5 ++++ 2 files changed, 68 insertions(+), 1 deletion(-) diff --git a/src/protocols/vnc/vnc.c b/src/protocols/vnc/vnc.c index d9f9dbbb..d13826d1 100644 --- a/src/protocols/vnc/vnc.c +++ b/src/protocols/vnc/vnc.c @@ -55,6 +55,62 @@ char* GUAC_VNC_CLIENT_KEY = "GUAC_VNC"; +/** + * A callback function that is called by the VNC library prior to writing + * data to a TLS-encrypted socket. This returns the rfbBool FALSE value + * if there's an error locking the mutex, or rfbBool TRUE otherwise. + * + * @param rfb_client + * The rfbClient for which to lock the TLS mutex. + * + * @returns + * rfbBool FALSE if an error occurs locking the mutex, otherwise + * TRUE. + */ +static rfbBool guac_vnc_lock_write_to_tls(rfbClient* rfb_client) { + + // Retrieve the Guacamole data structures + guac_client* gc = rfbClientGetClientData(rfb_client, GUAC_VNC_CLIENT_KEY); + guac_vnc_client* vnc_client = (guac_vnc_client*) gc->data; + + // Lock write access + int retval = pthread_mutex_lock(&(vnc_client->tls_lock)); + if (retval) { + guac_client_log(gc, GUAC_LOG_ERROR, "Error locking TLS write mutex: %d", retval); + return FALSE; + } + return TRUE; + +} + +/** + * A callback function for use by the VNC library that is called once + * the client is finished writing to a TLS-encrypted socket. A rfbBool + * FALSE value is returned if an error occurs unlocking the mutex, + * otherwise TRUE is returned. + * + * @param rfb_client + * The rfbClient for which to unlock the TLS mutex. + * + * @returns + * rfbBool FALSE if an error occurs unlocking the mutex, otherwise + * TRUE. + */ +static rfbBool guac_vnc_unlock_write_to_tls(rfbClient* rfb_client) { + + // Retrieve the Guacamole data structures + guac_client* gc = rfbClientGetClientData(rfb_client, GUAC_VNC_CLIENT_KEY); + guac_vnc_client* vnc_client = (guac_vnc_client*) gc->data; + + // Unlock write access + int retval = pthread_mutex_unlock(&(vnc_client->tls_lock)); + if (retval) { + guac_client_log(gc, GUAC_LOG_ERROR, "Error unlocking TLS write mutex: %d", retval); + return FALSE; + } + return TRUE; +} + rfbClient* guac_vnc_get_client(guac_client* client) { rfbClient* rfb_client = rfbGetClient(8, 3, 4); /* 32-bpp client */ @@ -68,6 +124,10 @@ rfbClient* guac_vnc_get_client(guac_client* client) { rfb_client->GotFrameBufferUpdate = guac_vnc_update; rfb_client->GotCopyRect = guac_vnc_copyrect; + /* TLS Locking and Unlocking */ + rfb_client->LockWriteToTLS = guac_vnc_lock_write_to_tls; + rfb_client->UnlockWriteToTLS = guac_vnc_unlock_write_to_tls; + /* Do not handle clipboard and local cursor if read-only */ if (vnc_settings->read_only == 0) { @@ -182,6 +242,9 @@ void* guac_vnc_client_thread(void* data) { rfbClientLog = guac_vnc_client_log_info; rfbClientErr = guac_vnc_client_log_error; + /* Initialize the write lock */ + pthread_mutex_init(&(vnc_client->tls_lock), NULL); + /* Attempt connection */ rfbClient* rfb_client = guac_vnc_get_client(client); int retries_remaining = settings->retries; @@ -403,4 +466,3 @@ void* guac_vnc_client_thread(void* data) { return NULL; } - diff --git a/src/protocols/vnc/vnc.h b/src/protocols/vnc/vnc.h index ce2d20af..31e78461 100644 --- a/src/protocols/vnc/vnc.h +++ b/src/protocols/vnc/vnc.h @@ -55,6 +55,11 @@ typedef struct guac_vnc_client { */ pthread_t client_thread; + /** + * The TLS mutex lock for the client. + */ + pthread_mutex_t tls_lock; + /** * The underlying VNC client. */ From c90c057e1276cc92f6143eafc6f5aef9bdbd5ca5 Mon Sep 17 00:00:00 2001 From: Nick Couchman Date: Sat, 9 Mar 2019 21:39:56 -0500 Subject: [PATCH 2/7] GUACAMOLE-414: Add version checks for TLS locking. --- src/protocols/vnc/vnc.c | 7 +++++++ src/protocols/vnc/vnc.h | 3 +++ 2 files changed, 10 insertions(+) diff --git a/src/protocols/vnc/vnc.c b/src/protocols/vnc/vnc.c index d13826d1..54e88dca 100644 --- a/src/protocols/vnc/vnc.c +++ b/src/protocols/vnc/vnc.c @@ -47,6 +47,7 @@ #include #include #include +#include #include #include @@ -55,6 +56,7 @@ char* GUAC_VNC_CLIENT_KEY = "GUAC_VNC"; +#if LIBVNCSERVER_VERSION_MAJOR >=0 && LIBVNCSERVER_VERSION_MINOR >= 9 && LIBVNCSERVER_VERSION_PATCHLEVEL >= 11 /** * A callback function that is called by the VNC library prior to writing * data to a TLS-encrypted socket. This returns the rfbBool FALSE value @@ -110,6 +112,7 @@ static rfbBool guac_vnc_unlock_write_to_tls(rfbClient* rfb_client) { } return TRUE; } +#endif rfbClient* guac_vnc_get_client(guac_client* client) { @@ -124,9 +127,11 @@ rfbClient* guac_vnc_get_client(guac_client* client) { rfb_client->GotFrameBufferUpdate = guac_vnc_update; rfb_client->GotCopyRect = guac_vnc_copyrect; +#if LIBVNCSERVER_VERSION_MAJOR >=0 && LIBVNCSERVER_VERSION_MINOR >= 9 && LIBVNCSERVER_VERSION_PATCHLEVEL >= 11 /* TLS Locking and Unlocking */ rfb_client->LockWriteToTLS = guac_vnc_lock_write_to_tls; rfb_client->UnlockWriteToTLS = guac_vnc_unlock_write_to_tls; +#endif /* Do not handle clipboard and local cursor if read-only */ if (vnc_settings->read_only == 0) { @@ -242,8 +247,10 @@ void* guac_vnc_client_thread(void* data) { rfbClientLog = guac_vnc_client_log_info; rfbClientErr = guac_vnc_client_log_error; +#if LIBVNCSERVER_VERSION_MAJOR >=0 && LIBVNCSERVER_VERSION_MINOR >= 9 && LIBVNCSERVER_VERSION_PATCHLEVEL >= 11 /* Initialize the write lock */ pthread_mutex_init(&(vnc_client->tls_lock), NULL); +#endif /* Attempt connection */ rfbClient* rfb_client = guac_vnc_get_client(client); diff --git a/src/protocols/vnc/vnc.h b/src/protocols/vnc/vnc.h index 31e78461..de9b3239 100644 --- a/src/protocols/vnc/vnc.h +++ b/src/protocols/vnc/vnc.h @@ -32,6 +32,7 @@ #include #include #include +#include #ifdef ENABLE_PULSE #include "pulse/pulse.h" @@ -55,10 +56,12 @@ typedef struct guac_vnc_client { */ pthread_t client_thread; +#if LIBVNCSERVER_VERSION_MAJOR >=0 && LIBVNCSERVER_VERSION_MINOR >= 9 && LIBVNCSERVER_VERSION_PATCHLEVEL >= 11 /** * The TLS mutex lock for the client. */ pthread_mutex_t tls_lock; +#endif /** * The underlying VNC client. From df4c93b3e8e25fde155c07d07ba8972a7455ff54 Mon Sep 17 00:00:00 2001 From: Nick Couchman Date: Sun, 10 Mar 2019 15:22:49 -0400 Subject: [PATCH 3/7] GUACAMOLE-414: Use configure checks for finding TLS locking support. --- configure.ac | 27 +++++++++++++++++++++++++++ src/protocols/vnc/vnc.c | 6 +++--- src/protocols/vnc/vnc.h | 2 +- 3 files changed, 31 insertions(+), 4 deletions(-) diff --git a/configure.ac b/configure.ac index 0acb27b1..85a0559e 100644 --- a/configure.ac +++ b/configure.ac @@ -512,6 +512,33 @@ then fi +# +# TLS Locking Support within libVNCServer +# + +if test "x${have_libvncserver}" = "xyes" +then + + have_vnc_tls_locking=yes + AC_CHECK_MEMBERS([rfbClient.LockWriteToTLS, rfbClient.UnlockWriteToTLS], + [], [have_vnc_tls_locking=no], + [[#include ]]) + + if test "x${have_vnc_tls_locking}" = "xno" + then + AC_MSG_WARN([ + -------------------------------------------- + This version of libvncclient lacks support + for TLS locking. VNC connections that use + TLS may experience instability as documented + in GUACAMOLE-414]) + else + AC_DEFINE([ENABLE_VNC_TLS_LOCKING],, + [Whether support for TLS locking within VNC is enabled.]) + fi + +fi + # # FreeRDP # diff --git a/src/protocols/vnc/vnc.c b/src/protocols/vnc/vnc.c index 54e88dca..932a917d 100644 --- a/src/protocols/vnc/vnc.c +++ b/src/protocols/vnc/vnc.c @@ -56,7 +56,7 @@ char* GUAC_VNC_CLIENT_KEY = "GUAC_VNC"; -#if LIBVNCSERVER_VERSION_MAJOR >=0 && LIBVNCSERVER_VERSION_MINOR >= 9 && LIBVNCSERVER_VERSION_PATCHLEVEL >= 11 +#ifdef ENABLE_VNC_TLS_LOCKING /** * A callback function that is called by the VNC library prior to writing * data to a TLS-encrypted socket. This returns the rfbBool FALSE value @@ -127,7 +127,7 @@ rfbClient* guac_vnc_get_client(guac_client* client) { rfb_client->GotFrameBufferUpdate = guac_vnc_update; rfb_client->GotCopyRect = guac_vnc_copyrect; -#if LIBVNCSERVER_VERSION_MAJOR >=0 && LIBVNCSERVER_VERSION_MINOR >= 9 && LIBVNCSERVER_VERSION_PATCHLEVEL >= 11 +#ifdef ENABLE_VNC_TLS_LOCKING /* TLS Locking and Unlocking */ rfb_client->LockWriteToTLS = guac_vnc_lock_write_to_tls; rfb_client->UnlockWriteToTLS = guac_vnc_unlock_write_to_tls; @@ -247,7 +247,7 @@ void* guac_vnc_client_thread(void* data) { rfbClientLog = guac_vnc_client_log_info; rfbClientErr = guac_vnc_client_log_error; -#if LIBVNCSERVER_VERSION_MAJOR >=0 && LIBVNCSERVER_VERSION_MINOR >= 9 && LIBVNCSERVER_VERSION_PATCHLEVEL >= 11 +#ifdef ENABLE_VNC_TLS_LOCKING /* Initialize the write lock */ pthread_mutex_init(&(vnc_client->tls_lock), NULL); #endif diff --git a/src/protocols/vnc/vnc.h b/src/protocols/vnc/vnc.h index de9b3239..7e927585 100644 --- a/src/protocols/vnc/vnc.h +++ b/src/protocols/vnc/vnc.h @@ -56,7 +56,7 @@ typedef struct guac_vnc_client { */ pthread_t client_thread; -#if LIBVNCSERVER_VERSION_MAJOR >=0 && LIBVNCSERVER_VERSION_MINOR >= 9 && LIBVNCSERVER_VERSION_PATCHLEVEL >= 11 +#ifdef ENABLE_VNC_TLS_LOCKING /** * The TLS mutex lock for the client. */ From 36817f37741bdc7e5ce6e9ad2e5e7bc044720588 Mon Sep 17 00:00:00 2001 From: Nick Couchman Date: Sun, 10 Mar 2019 17:33:14 -0400 Subject: [PATCH 4/7] GUACAMOLE-414: Clean up style and move mutex init to client allocation. --- src/protocols/vnc/client.c | 11 +++++++++++ src/protocols/vnc/vnc.c | 14 ++++---------- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/src/protocols/vnc/client.c b/src/protocols/vnc/client.c index 9cc85a18..cee1d4d5 100644 --- a/src/protocols/vnc/client.c +++ b/src/protocols/vnc/client.c @@ -36,6 +36,7 @@ #include +#include #include #include @@ -48,6 +49,11 @@ int guac_client_init(guac_client* client) { guac_vnc_client* vnc_client = calloc(1, sizeof(guac_vnc_client)); client->data = vnc_client; +#ifdef ENABLE_VNC_TLS_LOCKING + /* Initialize the write lock */ + pthread_mutex_init(&(vnc_client->tls_lock), NULL); +#endif + /* Init clipboard */ vnc_client->clipboard = guac_common_clipboard_alloc(GUAC_VNC_CLIPBOARD_MAX_LENGTH); @@ -125,6 +131,11 @@ int guac_vnc_client_free_handler(guac_client* client) { if (settings != NULL) guac_vnc_settings_free(settings); +#ifdef ENABLE_VNC_TLS_LOCKING + /* Clean up TLS lock mutex. */ + pthread_mutex_destroy(&(vnc_client->tls_lock)); +#endif + /* Free generic data struct */ free(client->data); diff --git a/src/protocols/vnc/vnc.c b/src/protocols/vnc/vnc.c index 932a917d..c68f6d3b 100644 --- a/src/protocols/vnc/vnc.c +++ b/src/protocols/vnc/vnc.c @@ -47,7 +47,6 @@ #include #include #include -#include #include #include @@ -71,11 +70,11 @@ char* GUAC_VNC_CLIENT_KEY = "GUAC_VNC"; */ static rfbBool guac_vnc_lock_write_to_tls(rfbClient* rfb_client) { - // Retrieve the Guacamole data structures + /* Retrieve the Guacamole data structures */ guac_client* gc = rfbClientGetClientData(rfb_client, GUAC_VNC_CLIENT_KEY); guac_vnc_client* vnc_client = (guac_vnc_client*) gc->data; - // Lock write access + /* Lock write access */ int retval = pthread_mutex_lock(&(vnc_client->tls_lock)); if (retval) { guac_client_log(gc, GUAC_LOG_ERROR, "Error locking TLS write mutex: %d", retval); @@ -100,11 +99,11 @@ static rfbBool guac_vnc_lock_write_to_tls(rfbClient* rfb_client) { */ static rfbBool guac_vnc_unlock_write_to_tls(rfbClient* rfb_client) { - // Retrieve the Guacamole data structures + /* Retrieve the Guacamole data structures */ guac_client* gc = rfbClientGetClientData(rfb_client, GUAC_VNC_CLIENT_KEY); guac_vnc_client* vnc_client = (guac_vnc_client*) gc->data; - // Unlock write access + /* Unlock write access */ int retval = pthread_mutex_unlock(&(vnc_client->tls_lock)); if (retval) { guac_client_log(gc, GUAC_LOG_ERROR, "Error unlocking TLS write mutex: %d", retval); @@ -247,11 +246,6 @@ void* guac_vnc_client_thread(void* data) { rfbClientLog = guac_vnc_client_log_info; rfbClientErr = guac_vnc_client_log_error; -#ifdef ENABLE_VNC_TLS_LOCKING - /* Initialize the write lock */ - pthread_mutex_init(&(vnc_client->tls_lock), NULL); -#endif - /* Attempt connection */ rfbClient* rfb_client = guac_vnc_get_client(client); int retries_remaining = settings->retries; From bfc6c1e6e0e9852725bb9d4bdd49b7f5ac4536d6 Mon Sep 17 00:00:00 2001 From: Nick Couchman Date: Sun, 10 Mar 2019 17:40:34 -0400 Subject: [PATCH 5/7] GUACAMOLE-414: Convert errors to strings from ptread_mutex_lock and unlock. --- src/protocols/vnc/vnc.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/protocols/vnc/vnc.c b/src/protocols/vnc/vnc.c index c68f6d3b..66de2634 100644 --- a/src/protocols/vnc/vnc.c +++ b/src/protocols/vnc/vnc.c @@ -77,7 +77,8 @@ static rfbBool guac_vnc_lock_write_to_tls(rfbClient* rfb_client) { /* Lock write access */ int retval = pthread_mutex_lock(&(vnc_client->tls_lock)); if (retval) { - guac_client_log(gc, GUAC_LOG_ERROR, "Error locking TLS write mutex: %d", retval); + guac_client_log(gc, GUAC_LOG_ERROR, "Error locking TLS write mutex: %s", + strerror(retval)); return FALSE; } return TRUE; @@ -106,7 +107,8 @@ static rfbBool guac_vnc_unlock_write_to_tls(rfbClient* rfb_client) { /* Unlock write access */ int retval = pthread_mutex_unlock(&(vnc_client->tls_lock)); if (retval) { - guac_client_log(gc, GUAC_LOG_ERROR, "Error unlocking TLS write mutex: %d", retval); + guac_client_log(gc, GUAC_LOG_ERROR, "Error unlocking TLS write mutex: %d", + strerror(retval)); return FALSE; } return TRUE; From a6f2ab9d93fe1e5b6a4aa0b5551119965c86fba8 Mon Sep 17 00:00:00 2001 From: Nick Couchman Date: Sun, 10 Mar 2019 17:41:45 -0400 Subject: [PATCH 6/7] GUACAMOLE-414: Use correct formatting for string from strerror. --- src/protocols/vnc/vnc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/protocols/vnc/vnc.c b/src/protocols/vnc/vnc.c index 66de2634..a3ae9b12 100644 --- a/src/protocols/vnc/vnc.c +++ b/src/protocols/vnc/vnc.c @@ -107,7 +107,7 @@ static rfbBool guac_vnc_unlock_write_to_tls(rfbClient* rfb_client) { /* Unlock write access */ int retval = pthread_mutex_unlock(&(vnc_client->tls_lock)); if (retval) { - guac_client_log(gc, GUAC_LOG_ERROR, "Error unlocking TLS write mutex: %d", + guac_client_log(gc, GUAC_LOG_ERROR, "Error unlocking TLS write mutex: %s", strerror(retval)); return FALSE; } From a4521208ba03f7e5939203c46c71bd2b6ffbffb6 Mon Sep 17 00:00:00 2001 From: Nick Couchman Date: Sun, 24 Mar 2019 15:09:58 -0400 Subject: [PATCH 7/7] GUACAMOLE-414: Remove unnecessary rfbconfig include. --- src/protocols/vnc/vnc.h | 1 - 1 file changed, 1 deletion(-) diff --git a/src/protocols/vnc/vnc.h b/src/protocols/vnc/vnc.h index 7e927585..7c189cfb 100644 --- a/src/protocols/vnc/vnc.h +++ b/src/protocols/vnc/vnc.h @@ -32,7 +32,6 @@ #include #include #include -#include #ifdef ENABLE_PULSE #include "pulse/pulse.h"