From 7550af84a9c6c94aeb2fdf77e83ebee83b8aa620 Mon Sep 17 00:00:00 2001 From: Daniel Lenski Date: Thu, 29 Jun 2023 17:40:31 -0700 Subject: [PATCH] [MDEV-31585] Stop trusting or relying on client identifying information sent prior to the TLS handshake MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The server has heretofore improperly mishandled—and TRUSTED—information sent in the plaintext login request packet sent prior to the TLS handshake. As a result of this, the client is *forced* to send excessive and exploitable identifying information in the pre-TLS-handshake plaintext login packet. That client-side vulnerability is CONC-654. This modifies the server to stop relying on any of the information in the pre-TLS-handshake plaintext login packet EXCEPT for the single bit that tells it that a TLS handshake will follow. It furthermore adds an "extended capability" bit to the server greeting packet, which informs the client that it is safe to send a bare-bones dummy packet containing ONLY the instruction that a TLS handshake will follow: /* 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/mysql_com.h | 3 +++ libmariadb | 2 +- sql/sql_acl.cc | 61 +++++++++++++++++++++++++++++---------------- 3 files changed, 44 insertions(+), 22 deletions(-) diff --git a/include/mysql_com.h b/include/mysql_com.h index b0e96caddf700..ec7c3af54c650 100644 --- a/include/mysql_com.h +++ b/include/mysql_com.h @@ -288,6 +288,9 @@ enum enum_indicator_type /* Do not resend metadata for prepared statements, since 10.6*/ #define MARIADB_CLIENT_CACHE_METADATA (1ULL << 36) +/* Server doesn't improperly read the pre-TLS dummy packet beyond the initial 2 bytes */ +#define CLIENT_CAN_SEND_DUMMY_HANDSHAKE_PACKET (1ULL << 37) + #ifdef HAVE_COMPRESS #define CAN_CLIENT_COMPRESS CLIENT_COMPRESS #else diff --git a/libmariadb b/libmariadb index 5af90f00ffeda..23cbf7cc72ba5 160000 --- a/libmariadb +++ b/libmariadb @@ -1 +1 @@ -Subproject commit 5af90f00ffeda64795e23753c14d601cce5d02ca +Subproject commit 23cbf7cc72ba56c93a99b6462a6774b939f8918f diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc index 436cd1e976fdc..d18b05155aa98 100644 --- a/sql/sql_acl.cc +++ b/sql/sql_acl.cc @@ -13259,6 +13259,7 @@ static bool send_server_handshake_packet(MPVIO_EXT *mpvio, thd->client_capabilities|= CLIENT_TRANSACTIONS; thd->client_capabilities|= CAN_CLIENT_COMPRESS; + thd->client_capabilities|= CLIENT_CAN_SEND_DUMMY_HANDSHAKE_PACKET; if (ssl_acceptor_fd) { @@ -13752,30 +13753,19 @@ static ulong parse_client_handshake_packet(MPVIO_EXT *mpvio, */ DBUG_ASSERT(net->read_pos[pkt_len] == 0); - ulonglong client_capabilities= uint2korr(net->read_pos); - compile_time_assert(sizeof(client_capabilities) >= 8); - if (client_capabilities & CLIENT_PROTOCOL_41) + ushort first_two_bytes_of_client_capabilities= uint2korr(net->read_pos); + if (first_two_bytes_of_client_capabilities & CLIENT_SSL) { - if (pkt_len < 32) - DBUG_RETURN(packet_error); - client_capabilities|= ((ulong) uint2korr(net->read_pos+2)) << 16; - if (!(client_capabilities & CLIENT_MYSQL)) - { - // it is client with mariadb extensions - ulonglong ext_client_capabilities= - (((ulonglong)uint4korr(net->read_pos + 28)) << 32); - client_capabilities|= ext_client_capabilities; - } - } - - /* Disable those bits which are not supported by the client. */ - compile_time_assert(sizeof(thd->client_capabilities) >= 8); - thd->client_capabilities&= client_capabilities; + /* Client wants to use TLS. This packet is really just a + * dummy which gets sent before the TLS handshake, in order + * to trigger the server (us) to start the TLS handshake. + * + * We should ignore everything else in this packet until + * after the TLS handshake. + */ - DBUG_PRINT("info", ("client capabilities: %llu", thd->client_capabilities)); - if (thd->client_capabilities & CLIENT_SSL) - { unsigned long errptr __attribute__((unused)); + DBUG_PRINT("info", ("client capabilities have TLS/SSL bit set")); /* Do the SSL layering. */ if (!ssl_acceptor_fd) @@ -13796,6 +13786,10 @@ static ulong parse_client_handshake_packet(MPVIO_EXT *mpvio, DBUG_PRINT("info", ("Immediately following IO layer change: vio_type=%s", safe_vio_type_name(thd->net.vio))); + /* Now we are using TLS. The client will resend its REAL + * handshake packet, containing complete credentials and + * capability information. + */ DBUG_PRINT("info", ("Reading user information over SSL layer")); pkt_len= my_net_read(net); if (unlikely(pkt_len == packet_error || pkt_len < NORMAL_HANDSHAKE_SIZE)) @@ -13804,8 +13798,33 @@ static ulong parse_client_handshake_packet(MPVIO_EXT *mpvio, pkt_len)); DBUG_RETURN(packet_error); } + + /* Re-load the FIRST TWO BYTES of the client handshake packet */ + first_two_bytes_of_client_capabilities = uint2korr(net->read_pos); } + ulonglong client_capabilities= (ulonglong) first_two_bytes_of_client_capabilities; + compile_time_assert(sizeof(client_capabilities) >= 8); + + DBUG_PRINT("info", ("client capabilities: %llu", thd->client_capabilities)); + if (client_capabilities & CLIENT_PROTOCOL_41) + { + if (pkt_len < 32) + DBUG_RETURN(packet_error); + client_capabilities|= ((ulong) uint2korr(net->read_pos+2)) << 16; + if (!(client_capabilities & CLIENT_MYSQL)) + { + // it is client with mariadb extensions + ulonglong ext_client_capabilities= + (((ulonglong)uint4korr(net->read_pos + 28)) << 32); + client_capabilities|= ext_client_capabilities; + } + } + + /* Disable those bits which are not supported by the client. */ + compile_time_assert(sizeof(thd->client_capabilities) >= 8); + thd->client_capabilities&= client_capabilities; + if (client_capabilities & CLIENT_PROTOCOL_41) { thd->max_client_packet_length= uint4korr(net->read_pos+4);