Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

GUACAMOLE-1627: Enforce TLS1.2 #390

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

GUACAMOLE-1627: Enforce TLS1.2 #390

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions src/protocols/rdp/settings.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include <freerdp/constants.h>
#include <freerdp/settings.h>
#include <freerdp/freerdp.h>
#include <freerdp/version.h>
#include <guacamole/client.h>
#include <guacamole/string.h>
#include <guacamole/user.h>
Expand All @@ -39,6 +40,13 @@
#include <stdlib.h>
#include <string.h>

// OpenSSL TLS version constants
# define TLS1_VERSION 0x0301
# define TLS1_1_VERSION 0x0302
# define TLS1_2_VERSION 0x0303
# define TLS1_3_VERSION 0x0304
# define TLS_MAX_VERSION TLS1_3_VERSION

/* Client plugin arguments */
const char* GUAC_RDP_CLIENT_ARGS[] = {
"hostname",
Expand Down Expand Up @@ -1604,6 +1612,17 @@ 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;

// FreeRDP allows for TLS Version control starting 2.8.0
#if (defined FREERDP_VERSION_MAJOR && FREERDP_VERSION_MAJOR >=2 && defined FREERDP_VERSION_MINOR && FREERDP_VERSION_MINOR >=8 && defined FREERDP_VERSION_REVISION && FREERDP_VERSION_REVISION >=0)
// Faulty servers with partial support for TLSv1.3, like windows server 2019,
// trick FreeRDP into negotiating TLSv1.3 and then send back a RST response after initial "Client Hello" during handshake.
// Setting the min and max versions of TLS version allows us to enforce the TLS version the client(FreeRDP) chooses.
// Note that older versions of FreeRDP that relied on older versions of Openssl that didn't have TLS1.3 don't run into
// this issue as the max TLS version supported by those clients is TLS1.2.
rdp_settings->TLSMinVersion = 0;
rdp_settings->TLSMaxVersion = TLS1_2_VERSION;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than hard-coding this for 1.2 only, wouldn't it be better to add configuration options for defining min and max versions? For example, Redemption uses such configuration options.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jclc: Yes, I mentioned this in my original comment, above, and most of the conversation between @sivagudivadaz and me has been regarding the best place to put such an option or options.

I suppose we can talk about whether it makes more sense to force a single specific version or provide options for minimum and maximum versions. I'm not opposed to a version range, per se, but I wonder how valuable it will be - if you have a connection to a single server, it may not be all that useful to have a range of options, as the server is usually going to consistently support the same level, and you ought to be able to just enforce that supported level.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about using the Guacamole protocol to pass these as optional arguments? Then you could do it on a connection-by-connection basis.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's exactly what we're already talking about. The conversation and screenshot above deals with making these TLS versions configurable on a per-connection basis - if they are added as connection parameters they would be passed along with the rest of the options (hostname, port, username, password, etc.) within the Guacamole Protocol as part of establishing the connection.

#endif

#ifdef HAVE_RDPSETTINGS_ALLOWUNANOUNCEDORDERSFROMSERVER
/* Do not consider server use of unannounced orders to be a fatal error */
rdp_settings->AllowUnanouncedOrdersFromServer = TRUE;
Expand Down