[MDEV-31585] Stop trusting or relying on client identifying information sent prior to the TLS handshake

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.

Author: dlenski

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

libmariadb

@@ -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);