From 907e73da1bb239603d180ca1367688d10e48eba7 Mon Sep 17 00:00:00 2001 From: Daniel Lenski Date: Thu, 29 Jun 2023 17:20:04 -0700 Subject: [PATCH] [CONC-654] Stop leaking client identifying information to the server before the TLS handshake MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The server implementation here was incorrect as well, unnecessarily reading—and TRUSTING—client identifying information sent before the TLS handshake. That's in MDEV-31585. As a result of the server's mishandling of this information, it's not possible for the client to fix this in a way that's backwards-compatible with old servers. We rely on the server sending a capability bit to indicate that the server-side bug has been fixed: /* Server does not mishandle information sent in the plaintext * login request packet sent prior to the TLS handshake. As a result, the * client can safely send an empty/dummy packet contianing no * identifying information. Indicates that MDEV-31585 has been fixed. * Since ??.?. */ #define MARIADB_CLIENT_CAN_SEND_DUMMY_HANDSHAKE_PACKET (1ULL << 37) All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc. --- include/mariadb_com.h | 7 ++++++ plugins/auth/my_auth.c | 57 +++++++++++++++++++++++++++++------------- 2 files changed, 47 insertions(+), 17 deletions(-) diff --git a/include/mariadb_com.h b/include/mariadb_com.h index 58f44c4db..3e3f9c3a5 100644 --- a/include/mariadb_com.h +++ b/include/mariadb_com.h @@ -176,6 +176,13 @@ enum enum_server_command #define MARIADB_CLIENT_EXTENDED_METADATA (1ULL << 35) /* Do not resend metadata for prepared statements, since 10.6*/ #define MARIADB_CLIENT_CACHE_METADATA (1ULL << 36) +/* Server does not grievously mishandle information sent in the plaintext + * login request packet sent prior to the TLS handshake. As a result, the + * client can safely send an empty/dummy packet contianing no + * identifying information. Indicates that MDEV-31585 has been fixed. + * Since ??.?. + */ +#define MARIADB_CLIENT_CAN_SEND_DUMMY_HANDSHAKE_PACKET (1ULL << 37) #define IS_MARIADB_EXTENDED_SERVER(mysql)\ (!(mysql->server_capabilities & CLIENT_MYSQL)) diff --git a/plugins/auth/my_auth.c b/plugins/auth/my_auth.c index c65729a16..4bf8efa3f 100644 --- a/plugins/auth/my_auth.c +++ b/plugins/auth/my_auth.c @@ -209,6 +209,11 @@ static int send_client_reply_packet(MCPVIO_EXT *mpvio, size_t conn_attr_len= (mysql->options.extension) ? mysql->options.extension->connect_attrs_len : 0; +#if defined(HAVE_TLS) && !defined(EMBEDDED_LIBRARY) + bool server_allows_dummy_packet= + mysql->extension->mariadb_server_capabilities & (MARIADB_CLIENT_CAN_SEND_DUMMY_HANDSHAKE_PACKET >> 32); +#endif + /* see end= buff+32 below, fixed size of the packet is 32 bytes */ buff= malloc(33 + USERNAME_LENGTH + data_len + NAME_LEN + NAME_LEN + conn_attr_len + 9); end= buff; @@ -320,26 +325,44 @@ static int send_client_reply_packet(MCPVIO_EXT *mpvio, if (mysql->options.use_ssl && (mysql->client_flag & CLIENT_SSL)) { - /* - Send UNENCRYPTED "Login Request" packet with mysql->client_flag - and max_packet_size, but no username; without this, the server - does not know we want to switch to SSL/TLS - - FIXME: Sending this packet is a very very VERY bad idea. It - contains the client's preferred charset and flags in plaintext; - this can be used for fingerprinting the client software version, - and probable geographic location. - - This offers a glaring opportunity for pervasive attackers to - easily target, intercept, and exploit the client-server - connection (e.g. "MITM all connections from known-vulnerable - client versions originating from countries X, Y, and Z"). - */ - if (ma_net_write(net, (unsigned char *)buff, (size_t) (end-buff)) || ma_net_flush(net)) + int ret; + if (server_allows_dummy_packet) + { + /* + The server has been disemborked so that it doesn't inadvisably + rely on information send in the pre-TLS-handshake packet. + Send it a dummy packet that contains NOTHING except for the + 2-byte client capabilities with the CLIENT_SSL bit. + */ + unsigned char dummy[2]; + int2store(dummy, CLIENT_SSL); + ret= (ma_net_write(net, dummy, sizeof(dummy)) || ma_net_flush(net)); + } + else + { + /* + Send UNENCRYPTED "Login Request" packet with mysql->client_flag + and max_packet_size, but no username; without this, the server + does not know we want to switch to SSL/TLS + + FIXME: Sending this packet is a very very VERY bad idea. It + contains the client's preferred charset and flags in plaintext; + this can be used for fingerprinting the client software version, + and probable geographic location. + + This offers a glaring opportunity for pervasive attackers to + easily target, intercept, and exploit the client-server + connection (e.g. "MITM all connections from known-vulnerable + client versions originating from countries X, Y, and Z"). + */ + ret= (ma_net_write(net, (unsigned char *)buff, (size_t) (end-buff)) || ma_net_flush(net)); + } + + if (ret) { my_set_error(mysql, CR_SERVER_LOST, SQLSTATE_UNKNOWN, ER(CR_SERVER_LOST_EXTENDED), - "sending connection information to server", + "sending dummy client reply packet to server", errno); goto error; }