From fc45d4ee737b2489cb5f996d836092247f9c43e6 Mon Sep 17 00:00:00 2001 From: Daniel Lenski Date: Tue, 13 Jun 2023 15:53:53 -0700 Subject: [PATCH 1/2] Additional debug logging in sql_acl This additional logging is intended to demonstrate exactly when/where/how the switch from a plaintext TCP/IP socket to a TLS/SSL-wrapped socket occurs. The short summary is that the TLS-ification of the connection is completely entangled with the authentication process: - The mechanism for ensuring confidentiality and authenticity of the client/server communications at the TRANSPORT-layer is TLS (which literally stands for Transport Layer Security) - The APPLICATION-layer authentication mechanisms involve traffic exchanged above the transport layer, and support multiple plugins, such as the default "native" authentication plugin using usernames and passwords. - The transport-layer security mechanism is logically distinct from the application-layer authentication mechanism, but in the MariaDB server codebase these are thoroughly entangled and interdependent. This is a network-mislayering design problem with significant consequences for code complexity and flexibility. It leads to several potential and actual security vulnerabilities whereby information is improperly transmitted and accepted in plaintext prior to the TLS handshake. 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. --- sql/sql_acl.cc | 70 +++++++++++++++++++++++++++++++++++--------------- 1 file changed, 49 insertions(+), 21 deletions(-) diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc index 4c2d063b771f5..ad8bbec481456 100644 --- a/sql/sql_acl.cc +++ b/sql/sql_acl.cc @@ -12746,6 +12746,8 @@ static bool send_server_handshake_packet(MPVIO_EXT *mpvio, char scramble_buf[SCRAMBLE_LENGTH]; char *end= buff; DBUG_ENTER("send_server_handshake_packet"); + DBUG_PRINT("info", ("send_server_handshake STARTING: vio_type=%s", + safe_vio_type_name(thd->net.vio))); *end++= protocol_version; @@ -13237,10 +13239,13 @@ static ulong parse_client_handshake_packet(MPVIO_EXT *mpvio, THD *thd= mpvio->auth_info.thd; NET *net= &thd->net; char *end; + DBUG_ENTER("parse_client_handshake_packet"); DBUG_ASSERT(mpvio->status == MPVIO_EXT::FAILURE); + DBUG_PRINT("info", ("parse_client_handshake STARTING: vio_type=%s", + safe_vio_type_name(thd->net.vio))); if (pkt_len < MIN_HANDSHAKE_SIZE) - return packet_error; + DBUG_RETURN(packet_error); /* Protocol buffer is guaranteed to always end with \0. (see my_net_read()) @@ -13253,7 +13258,7 @@ static ulong parse_client_handshake_packet(MPVIO_EXT *mpvio, if (client_capabilities & CLIENT_PROTOCOL_41) { if (pkt_len < 32) - return packet_error; + DBUG_RETURN(packet_error); client_capabilities|= ((ulong) uint2korr(net->read_pos+2)) << 16; if (!(client_capabilities & CLIENT_MYSQL)) { @@ -13275,7 +13280,7 @@ static ulong parse_client_handshake_packet(MPVIO_EXT *mpvio, /* Do the SSL layering. */ if (!ssl_acceptor_fd) - return packet_error; + DBUG_RETURN(packet_error); DBUG_PRINT("info", ("IO layer change in progress...")); mysql_rwlock_rdlock(&LOCK_ssl_refresh); @@ -13286,16 +13291,19 @@ static ulong parse_client_handshake_packet(MPVIO_EXT *mpvio, if(ssl_ret) { DBUG_PRINT("error", ("Failed to accept new SSL connection")); - return packet_error; + DBUG_RETURN(packet_error); } + DBUG_PRINT("info", ("Immediately following IO layer change: vio_type=%s", + safe_vio_type_name(thd->net.vio))); + 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)) { DBUG_PRINT("error", ("Failed to read user information (pkt_len= %lu)", pkt_len)); - return packet_error; + DBUG_RETURN(packet_error); } } @@ -13304,19 +13312,19 @@ static ulong parse_client_handshake_packet(MPVIO_EXT *mpvio, thd->max_client_packet_length= uint4korr(net->read_pos+4); DBUG_PRINT("info", ("client_character_set: %d", (uint) net->read_pos[8])); if (thd_init_client_charset(thd, (uint) net->read_pos[8])) - return packet_error; + DBUG_RETURN(packet_error); end= (char*) net->read_pos+32; } else { if (pkt_len < 5) - return packet_error; + DBUG_RETURN(packet_error); thd->max_client_packet_length= uint3korr(net->read_pos+2); end= (char*) net->read_pos+5; } if (end >= (char*) net->read_pos+ pkt_len +2) - return packet_error; + DBUG_RETURN(packet_error); if (thd->client_capabilities & CLIENT_IGNORE_SPACE) thd->variables.sql_mode|= MODE_IGNORE_SPACE; @@ -13324,7 +13332,7 @@ static ulong parse_client_handshake_packet(MPVIO_EXT *mpvio, thd->variables.net_wait_timeout= thd->variables.net_interactive_timeout; if (end >= (char*) net->read_pos+ pkt_len +2) - return packet_error; + DBUG_RETURN(packet_error); if ((thd->client_capabilities & CLIENT_TRANSACTIONS) && opt_using_transactions) @@ -13359,7 +13367,7 @@ static ulong parse_client_handshake_packet(MPVIO_EXT *mpvio, len= safe_net_field_length_ll((uchar**)&passwd, net->read_pos + pkt_len - (uchar*)passwd); if (len > pkt_len) - return packet_error; + DBUG_RETURN(packet_error); } passwd_len= (size_t)len; @@ -13368,7 +13376,7 @@ static ulong parse_client_handshake_packet(MPVIO_EXT *mpvio, if (passwd == NULL || passwd + passwd_len + MY_TEST(db) > (char*) net->read_pos + pkt_len) - return packet_error; + DBUG_RETURN(packet_error); /* strlen() can't be easily deleted without changing protocol */ db_len= safe_strlen(db); @@ -13383,7 +13391,7 @@ static ulong parse_client_handshake_packet(MPVIO_EXT *mpvio, if (unlikely(thd->copy_with_error(system_charset_info, (LEX_STRING*) &mpvio->db, thd->charset(), db, db_len))) - return packet_error; + DBUG_RETURN(packet_error); user_len= copy_and_convert(user_buff, sizeof(user_buff) - 1, system_charset_info, user, user_len, @@ -13410,7 +13418,7 @@ static ulong parse_client_handshake_packet(MPVIO_EXT *mpvio, my_free((char*) sctx->user); if (!(sctx->user= my_strndup(user, user_len, MYF(MY_WME)))) - return packet_error; /* The error is set by my_strdup(). */ + DBUG_RETURN(packet_error); /* The error is set by my_strdup(). */ /* @@ -13424,12 +13432,12 @@ static ulong parse_client_handshake_packet(MPVIO_EXT *mpvio, { // if mysqld's been started with --skip-grant-tables option mpvio->status= MPVIO_EXT::SUCCESS; - return packet_error; + DBUG_RETURN(packet_error); } thd->password= passwd_len > 0; if (find_mpvio_user(mpvio)) - return packet_error; + DBUG_RETURN(packet_error); if ((thd->client_capabilities & CLIENT_PLUGIN_AUTH) && (client_plugin < (char *)net->read_pos + pkt_len)) @@ -13458,7 +13466,7 @@ static ulong parse_client_handshake_packet(MPVIO_EXT *mpvio, if ((thd->client_capabilities & CLIENT_CONNECT_ATTRS) && read_client_connect_attrs(&next_field, ((char *)net->read_pos) + pkt_len, mpvio->auth_info.thd->charset())) - return packet_error; + DBUG_RETURN(packet_error); /* if the acl_user needs a different plugin to authenticate @@ -13475,7 +13483,7 @@ static ulong parse_client_handshake_packet(MPVIO_EXT *mpvio, mpvio->cached_client_reply.pkt_len= (uint)passwd_len; mpvio->cached_client_reply.plugin= client_plugin; mpvio->status= MPVIO_EXT::RESTART; - return packet_error; + DBUG_RETURN(packet_error); } /* @@ -13494,16 +13502,17 @@ static ulong parse_client_handshake_packet(MPVIO_EXT *mpvio, if (send_plugin_request_packet(mpvio, (uchar*) mpvio->cached_server_packet.pkt, mpvio->cached_server_packet.pkt_len)) - return packet_error; + DBUG_RETURN(packet_error); passwd_len= my_net_read(&thd->net); passwd= (char*)thd->net.read_pos; } *buff= (uchar*) passwd; - return (ulong)passwd_len; + DBUG_RETURN((ulong)passwd_len); #else - return 0; + DBUG_ENTER("parse_client_handshake_packet (EMPTY STUB)"); + DBUG_RETURN(0); #endif } @@ -13524,6 +13533,8 @@ static int server_mpvio_write_packet(MYSQL_PLUGIN_VIO *param, MPVIO_EXT *mpvio= (MPVIO_EXT *) param; int res; DBUG_ENTER("server_mpvio_write_packet"); + DBUG_PRINT("info", ("server_mpvio_write_packet STARTING: vio_type=%s", + safe_vio_type_name(mpvio->auth_info.thd->net.vio))); /* reset cached_client_reply */ mpvio->cached_client_reply.pkt= 0; @@ -13570,6 +13581,9 @@ static int server_mpvio_read_packet(MYSQL_PLUGIN_VIO *param, uchar **buf) MYSQL_SERVER_AUTH_INFO * const ai= &mpvio->auth_info; ulong pkt_len; DBUG_ENTER("server_mpvio_read_packet"); + DBUG_PRINT("info", ("server_mpvio_read_packet STARTING: vio_type=%s", + safe_vio_type_name(mpvio->auth_info.thd->net.vio))); + if (mpvio->status == MPVIO_EXT::RESTART) { const char *client_auth_plugin= @@ -13647,6 +13661,8 @@ static int server_mpvio_read_packet(MYSQL_PLUGIN_VIO *param, uchar **buf) ai->auth_string_length= (ulong) mpvio->acl_user->auth[mpvio->curr_auth].salt.length; strmake_buf(ai->authenticated_as, mpvio->acl_user->user.str); + DBUG_PRINT("info", ("server_mpvio_read_packet FINISHING: vio_type=%s", + safe_vio_type_name(mpvio->auth_info.thd->net.vio))); DBUG_RETURN((int)pkt_len); err: @@ -13655,6 +13671,8 @@ static int server_mpvio_read_packet(MYSQL_PLUGIN_VIO *param, uchar **buf) if (!ai->thd->is_error()) my_error(ER_HANDSHAKE_ERROR, MYF(0)); } + DBUG_PRINT("info", ("server_mpvio_read_packet FINISHING WITH ERROR: vio_type=%s", + safe_vio_type_name(mpvio->auth_info.thd->net.vio))); DBUG_RETURN(-1); } @@ -13786,18 +13804,26 @@ static int do_auth_once(THD *thd, const LEX_CSTRING *auth_plugin_name, bool unlock_plugin= false; plugin_ref plugin= get_auth_plugin(thd, *auth_plugin_name, &unlock_plugin); + DBUG_ENTER("do_auth_once"); + DBUG_PRINT("info", ("do_auth_once STARTING: vio_type=%s", + safe_vio_type_name(thd->net.vio))); mpvio->plugin= plugin; mpvio->auth_info.user_name= NULL; if (plugin) { st_mysql_auth *info= (st_mysql_auth *) plugin_decl(plugin)->info; + + DBUG_PRINT("info", ("auth_plugin_name: %s, interface_version: %d", + auth_plugin_name->str, info->interface_version)); + switch (info->interface_version >> 8) { case 0x02: res= info->authenticate_user(mpvio, &mpvio->auth_info); break; case 0x01: { + DBUG_PRINT("info", ("Using compat downgrade for authentication")); MYSQL_SERVER_AUTH_INFO_0x0100 compat; compat.downgrade(&mpvio->auth_info); res= info->authenticate_user(mpvio, (MYSQL_SERVER_AUTH_INFO *)&compat); @@ -13820,7 +13846,9 @@ static int do_auth_once(THD *thd, const LEX_CSTRING *auth_plugin_name, res= CR_ERROR; } - return res; + DBUG_PRINT("info", ("do_auth_once FINISHING: vio_type=%s", + safe_vio_type_name(thd->net.vio))); + DBUG_RETURN(res); } enum PASSWD_ERROR_ACTION From 2eda50cb4ffa59009f204d1fa245d55fe13726ab Mon Sep 17 00:00:00 2001 From: Daniel Lenski Date: Thu, 29 Jun 2023 17:40:31 -0700 Subject: [PATCH 2/2] [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 2e51f67b662a1..c20190b55eb25 100644 --- a/include/mysql_com.h +++ b/include/mysql_com.h @@ -274,6 +274,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_OBSOLETE (1ULL << 30) /* diff --git a/libmariadb b/libmariadb index 3393fe35d3787..5eec3aa08c15e 160000 --- a/libmariadb +++ b/libmariadb @@ -1 +1 @@ -Subproject commit 3393fe35d378744e12636766931cf5109cc6c2e5 +Subproject commit 5eec3aa08c15e7a20a9d90e283b67bade12c170b diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc index ad8bbec481456..0d7d6ad82a1bc 100644 --- a/sql/sql_acl.cc +++ b/sql/sql_acl.cc @@ -12761,6 +12761,7 @@ static bool send_server_handshake_packet(MPVIO_EXT *mpvio, if (ssl_acceptor_fd) { thd->client_capabilities |= CLIENT_SSL; + thd->client_capabilities |= CLIENT_CAN_SSL_V2; /* See parse_client_handshake_packet */ } if (data_len) @@ -13253,30 +13254,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) @@ -13297,6 +13299,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)) @@ -13305,8 +13311,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);