Break backwards compatibility to overcome TLS vulnerabilities, but al…

…low clients to opt-out The CONC-648 and CONC-654 vulnerabilities cannot be addressed on the client side in a way that is fully backwards-compatible with existing servers: 1. CONC-648: Per Sergei Golubchik's research (https://jira.mariadb.org/browse/CONC-648?focusedCommentId=263341#comment-263341), there are numerous legitimate errors which servers sometimes send prior to the TLS handshake. 2. CONC-654: There is a server bug which must be fixed (https://jira.mariadb.org/browse/MDEV-31585) in order for the client and server to be able to establish a correctly-configured session with the client-side vulnerability fixed. At the same time, if we allow the client to continue silently downgrading itself (= "exhibiting these vulnerabilities") in the name of backwards-compatibility, end users will have no motivation to upgrade. Instead, we should change the client to avoid these vulnerabilities unconditionally. At the same time, add a special client option `backwards-compatible-insecure-ssl` (alias `backwards-compatible-insecure-tls`) to allow the client to continue implementing the old, insecure behavior... but with noisy warnings every time this behavior is required. 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.
dlenski · Jul 7, 2023 · 60a356a · 60a356a
1 parent 5cb97d6
commit 60a356a
Show file tree

Hide file tree

Showing 3 changed files with 51 additions and 8 deletions.
diff --git a/include/mysql.h b/include/mysql.h
@@ -223,6 +223,7 @@ extern const char *SQLSTATE_UNKNOWN;
     MYSQL_OPT_MAX_ALLOWED_PACKET,
     MYSQL_OPT_NET_BUFFER_LENGTH,
     MYSQL_OPT_TLS_VERSION,
+    MYSQL_OPT_BACKWARDS_COMPATIBLE_INSECURE_SSL,
 
     /* MariaDB specific */
     MYSQL_PROGRESS_CALLBACK=5999,
@@ -331,6 +332,7 @@ struct st_mysql_options {
     char *shared_memory_base_name;
     unsigned long max_allowed_packet;
     my_bool use_ssl;				/* if to use SSL or not */
+    my_bool backwards_compatible_insecure_ssl;
     my_bool compress,named_pipe;
     my_bool reconnect, unused_1, unused_2, unused_3;
     enum mysql_option methods_to_use;

diff --git a/libmariadb/mariadb_lib.c b/libmariadb/mariadb_lib.c
@@ -680,6 +680,7 @@ struct st_default_options mariadb_defaults[] =
   {{MYSQL_SERVER_PUBLIC_KEY}, MARIADB_OPTION_STR, "server-public-key"},
   {{MYSQL_OPT_BIND}, MARIADB_OPTION_STR, "bind-address"},
   {{MYSQL_OPT_SSL_ENFORCE}, MARIADB_OPTION_BOOL, "ssl-enforce"},
+  {{MYSQL_OPT_BACKWARDS_COMPATIBLE_INSECURE_SSL}, MARIADB_OPTION_BOOL, "backwards-compatible-insecure-ssl"},
   {{MARIADB_OPT_RESTRICTED_AUTH}, MARIADB_OPTION_STR, "restricted-auth"},
   {{.option_func=parse_connection_string}, MARIADB_OPTION_FUNC, "connection"},
   /* Aliases */
@@ -699,6 +700,7 @@ struct st_default_options mariadb_defaults[] =
   {{MARIADB_OPT_TLS_PASSPHRASE}, MARIADB_OPTION_STR, "tls-passphrase"},
   {{MYSQL_OPT_SSL_ENFORCE}, MARIADB_OPTION_BOOL, "tls-enforce"},
   {{MYSQL_OPT_SSL_VERIFY_SERVER_CERT}, MARIADB_OPTION_BOOL,"tls-verify-peer"},
+  {{MYSQL_OPT_BACKWARDS_COMPATIBLE_INSECURE_SSL}, MARIADB_OPTION_BOOL, "backwards-compatible-insecure-tls"},
   {{0}, 0, NULL}
 };
 
@@ -1788,15 +1790,33 @@ MYSQL *mthd_my_real_connect(MYSQL *mysql, const char *host, const char *user,
   if ((pkt_length=ma_net_safe_read(mysql)) == packet_error)
   {
     if (mysql->options.use_ssl)
-      my_set_error(mysql, CR_CONNECTION_ERROR, SQLSTATE_UNKNOWN,
-                   "Received error packet before completion of TLS handshake. "
-                   "Suppressing its details so that the client cannot vary its behavior "
-                   "based on this UNTRUSTED input.");
+    {
+      if (!mysql->options.backwards_compatible_insecure_ssl)
+        my_set_error(mysql, CR_CONNECTION_ERROR, SQLSTATE_UNKNOWN,
+                     "Received error packet before completion of TLS handshake. "
+                     "Suppressing its details so that the client cannot vary its behavior "
+                     "based on this UNTRUSTED input.");
+      else
+      {
+        /* The server has sent an error packet before completion of the TLS handshake.
+         * The client wishes to use SSL, so it SHOULD NOT trust this packet, because
+         * it is trivial for a MITM attacker to inject or interfere with such packets.
+         *
+         * However, the client has explicitly opted in to accept servers which use
+         * TLS/SSL in a buggy and insecure way for backwards-compatibility, so we allow
+         * this to go through and use the error set by ma_net_safe_read(), with a warning.
+         */
+        fprintf(stderr,
+                "WARNING: Received error packet sent by server before completion of TLS\n"
+                "    handshake. This is insecure, but we allow it because client is configured\n"
+                "    with backwards-compatible-insecure-ssl.\n");
+      }
+    }
     else if (mysql->net.last_errno == CR_SERVER_LOST)
       my_set_error(mysql, CR_SERVER_LOST, SQLSTATE_UNKNOWN,
-                 ER(CR_SERVER_LOST_EXTENDED),
-                 "handshake: reading initial communication packet",
-                 errno);
+                   ER(CR_SERVER_LOST_EXTENDED),
+                   "handshake: reading initial communication packet",
+                   errno);
 
     goto error;
   }
@@ -1932,6 +1952,23 @@ MYSQL *mthd_my_real_connect(MYSQL *mysql, const char *host, const char *user,
                      "Client requires TLS/SSL, but the server does not support it");
     goto error;
   }
+  if (mysql->options.use_ssl &&
+      !(mysql->extension->mariadb_server_capabilities & (MARIADB_CLIENT_CAN_SEND_DUMMY_HANDSHAKE_PACKET >> 32)))
+  {
+    /* Unless the client has explicitly opted in to (backwards compatible insecure TLS/SSL),
+     * which leaks excessive information prior to the TLS handshake, then we should abort.
+     */
+    if (!mysql->options.backwards_compatible_insecure_ssl)
+    {
+      SET_CLIENT_ERROR(mysql, CR_SSL_CONNECTION_ERROR, SQLSTATE_UNKNOWN,
+                       "Client requires secure TLS/SSL, but server only supports old, buggy, insecure TLS/SSL. Specify backwards-compatible-insecure-ssl to override.");
+      goto error;
+    }
+    else
+      fprintf(stderr,
+              "WARNING: Server only supports old, buggy, insecure TLS/SSL, and client has\n"
+              "  opted to use this (backwards-compatible-insecure-ssl set)\n");
+  }
 
   /* Set character set */
   if (mysql->options.charset_name)
@@ -3560,6 +3597,9 @@ mysql_optionsv(MYSQL *mysql,enum mysql_option option, ...)
   case MYSQL_OPT_SSL_ENFORCE:
     mysql->options.use_ssl= (*(my_bool *)arg1);
     break;
+  case MYSQL_OPT_BACKWARDS_COMPATIBLE_INSECURE_SSL:
+    mysql->options.backwards_compatible_insecure_ssl= (*(my_bool *)arg1);
+    break;
   case MYSQL_OPT_SSL_VERIFY_SERVER_CERT:
     if (*(my_bool *)arg1)
       mysql->options.client_flag |= CLIENT_SSL_VERIFY_SERVER_CERT;

diff --git a/plugins/auth/my_auth.c b/plugins/auth/my_auth.c
@@ -326,7 +326,8 @@ static int send_client_reply_packet(MCPVIO_EXT *mpvio,
       (mysql->client_flag & CLIENT_SSL))
   {
     int ret;
-    if (server_allows_dummy_packet)
+    if (server_allows_dummy_packet
+        || !mysql->options.backwards_compatible_insecure_ssl)
     {
       /*
         The server has been disemborked so that it doesn't inadvisably