From 174605c0dec6e0638a8eae9abb86e28a4d496489 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 a 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: /* This capability is set if: * * - The CLIENT knows how to send a truncated 2-byte SSLRequest * packet, containing no information other than the CLIENT_SSL flag * which is necessary to trigger the TLS handshake, and to send its * complete capability flags and other identifying information after * the TLS handshake. * - The SERVER knows how to receive this truncated 2-byte SSLRequest * packet, and to receive the client's complete capability bits * after the TLS handshake. * */ #define CLIENT_CAN_SSL_V2 (1ULL << 37) Because the client cannot safely send the SSL_V2 SSLRequest packet unless the server has advertised support for it in its (plaintext) Server Greeting packet, an active MITM could strip the CLIENT_CAN_SSL_V2 bit from that Server Greeting packet. This downgrade attack will force the client to continue exhibiting the CONC-654 vulnerability. The server is also modified to detect this case and abort the connection; this won't fix the one-time client information leakage of the CONC-654 vulnerability, but it is intended to discourage the MITM attack by making it highly visible. 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 | 14 +++++++ libmariadb | 2 +- sql/sql_acl.cc | 97 +++++++++++++++++++++++++++++++++++---------- 3 files changed, 90 insertions(+), 23 deletions(-) diff --git a/include/mysql_com.h b/include/mysql_com.h index 398bf9058df2e..41ca04df029f4 100644 --- a/include/mysql_com.h +++ b/include/mysql_com.h @@ -260,6 +260,20 @@ enum enum_indicator_type /* Client no longer needs EOF packet */ #define CLIENT_DEPRECATE_EOF (1ULL << 24) +/* This capability is set if: + * + * - The CLIENT knows how to send a truncated 2-byte SSLRequest + * packet, containing no information other than the CLIENT_SSL flag + * which is necessary to trigger the TLS handshake, and to send its + * complete capability flags and other identifying information after + * the TLS handshake. + * - The SERVER knows how to receive this truncated 2-byte SSLRequest + * packet, and to receive the client's complete capability bits + * after the TLS handshake. + * + */ +#define CLIENT_CAN_SSL_V2 (1ULL << 37) + #define CLIENT_PROGRESS_OBSOLETE (1ULL << 29) #define CLIENT_SSL_VERIFY_SERVER_CERT (1ULL << 30) /* diff --git a/libmariadb b/libmariadb index ab7a81e79e4be..8c8ac07800b84 160000 --- a/libmariadb +++ b/libmariadb @@ -1 +1 @@ -Subproject commit ab7a81e79e4be4324a2d09d19d4f5249801ef665 +Subproject commit 8c8ac07800b840d1c679953cbecbbb2423600278 diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc index f0dd2428b6432..71d42e5eb5a47 100644 --- a/sql/sql_acl.cc +++ b/sql/sql_acl.cc @@ -12229,6 +12229,7 @@ static bool send_server_handshake_packet(MPVIO_EXT *mpvio, { thd->client_capabilities |= CLIENT_SSL; thd->client_capabilities |= CLIENT_SSL_VERIFY_SERVER_CERT; + thd->client_capabilities |= CLIENT_CAN_SSL_V2; /* See parse_client_handshake_packet */ } if (data_len) @@ -12714,30 +12715,31 @@ 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) - { - 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; + ushort first_two_bytes_of_client_capabilities= uint2korr(net->read_pos); + bool pre_tls_client_packet_is_ssl_v2= (pkt_len==2 && first_two_bytes_of_client_capabilities == CLIENT_SSL); + bool pre_tls_client_packet_wants_ssl= first_two_bytes_of_client_capabilities & CLIENT_SSL; + + if (pre_tls_client_packet_wants_ssl) + { + /* Client wants to use TLS. This SSLRequest packet, sent in + * plaintext before the TLS handshake, is basically just a vestige + * that triggers the server (us) to start the TLS handshake. + * + * We ignore everything else in this pre-TLS packet, even though + * older clients send much of the same information that they will + * re-send over the TLS channel. + * + * This pre-TLS packet is untrustworthy AND if the server acts on + * its content, that FORCES the client to send more information + * in the clear. + */ - DBUG_PRINT("info", ("client capabilities: %llu", thd->client_capabilities)); - if (thd->client_capabilities & CLIENT_SSL) - { unsigned long errptr __attribute__((unused)); + if (pre_tls_client_packet_is_ssl_v2) + DBUG_PRINT("info", ("client sent SSL_V2 SSLRequest packet (2 bytes with only TLS/SSL bit set)")); + else + DBUG_PRINT("info", ("client sent old SSLRequest packet (%ld bytes including TLS/SSL bit; capabilities & 0xffff == 0x%04x)", + pkt_len, first_two_bytes_of_client_capabilities)); /* Do the SSL layering. */ if (!ssl_acceptor_fd) @@ -12753,6 +12755,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 (pkt_len == packet_error || pkt_len < NORMAL_HANDSHAKE_SIZE) @@ -12761,8 +12767,55 @@ static ulong parse_client_handshake_packet(MPVIO_EXT *mpvio, pkt_len)); DBUG_RETURN(packet_error); } + + /* Re-load the FIRST TWO BYTES of the capabilities from the packet sent over TLS. */ + 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; + } + } + bool post_tls_client_packet_indicates_ssl_v2= (client_capabilities & CLIENT_CAN_SSL_V2); + + if (pre_tls_client_packet_wants_ssl + && post_tls_client_packet_indicates_ssl_v2 + && !pre_tls_client_packet_is_ssl_v2) + { + /* 1. We told the client in our server greeting that we support the pre-TLS client packet containing only the TLS/SSL flag, + * CLIENT_CAN_SSL_V2. [Server greeting packet is sent in the clear, may be MITM'ed en route to the client.] + * 2. The client told us in its pre-TLS SSLRequest packet that it wants to use SSL. (CLIENT_SSL flag) + * 3. The client told us in its post-TLS packet that it too supports the pre-TLS client packet containing only the TLS/SSL flag, + * CLIENT_CAN_SSL_V2. [We received this information via TLS; assuming the client validated our server certificate + * to avoid a 2-sided TLS MITM, we know that this packet is authentically from the client.] + * 4. Nevertheless, the client DID NOT SEND us an SSL_V2-style SSLRequest packet. + * + * The only way this can happen is if the client is being downgraded by an active MITM attacker which + * disables the CLIENT_CAN_SSL_V2 bit in our server greeting packet. + */ + sql_print_warning("Aborting connection %lld because it is being actively MITM'ed to downgrade TLS security (attacker " + "is stripping the CLIENT_CAN_SSL_V2 bit from our server capabilities)", + thd->thread_id); + DBUG_RETURN(packet_error); } + /* 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);