Bug#27471510 PERFORMANCE_SCHEMA STATUS AND VARIABLE BY THREAD ARE NOT SAFE

Patch for 8.0.

Before this fix, executing:
  SELECT * FROM performance_schema.status_by_thread
  SELECT * FROM performance_schema.variables_by_thread
under heavy load was unsafe.

Root cause 1
============

SELECT * from performance_schema.status_by_thread
inspects the status variables of running sessions.

For sessions using SSL, SSL status variables are inspected.

For example, the status variable "Ssl_cipher_list" is evaluated
by executing function show_ssl_get_cipher_list()

This function is implemented as follows:

show_ssl_get_cipher_list()
{
  if (thd->get_protocol()->get_ssl()) // (a)
  {
    ...
    SSL_get_cipher(thd->get_protocol()->get_ssl())); // (b)
    ...
  }
}

The problem is that evaluating
  thd->get_protocol()->get_ssl()
to access the underlying SSL structure is unsafe,
and subject to race conditions.

The value returned in (a) can change by the time (b)
is evaluated, for example when using prepared statements,
because the thd->get_protocol() pointer will change
during execution of PREPARE and EXECUTE.

Fix 1
=====

thd->get_protocol()->get_ssl()
is not a proper way to access SSL data for the session.

Instead, THD::m_SSL now keeps the SSL data attached to the
THD session.

THD::m_SSL is set after the SSL connection is established,
is reset upon disconnect,
and is immutable during the session execution.

Inspecting this attribute is safe when LOCK_thd_data is held,
which make table performance_schema.status_by_thread safe.

Root cause 2
============

SELECT * from performance_schema.variables_by_thread
inspects the variables of running sessions.

In particular, variable "session_track_system_variables"
is inspected.

This variable value is stored in THD::variables.track_sysvars_ptr

On the session connection,
  THD::variables.track_sysvars_ptr
is duplicated and points to allocated memory,
in this call:
    thd->session_sysvar_res_mgr.init(&thd->variables.track_sysvars_ptr,
thd->charset());

This is because Session_sysvar_resource_manager::init()
modifies the THD::variables.track_sysvars_ptr pointer itself.

On session disconnect,
  Session_sysvar_resource_manager::deinit()
free the allocated memory,
which leaves the THD::variables.track_sysvars_ptr pointer
invalid, referencing freed memory.

As soon as cleanup_variables() unlocks LOCK_thd_data,
a race condition is possible,
when using the now invalid THD::variables.track_sysvars_ptr pointer.

Fix 2
=====

In cleanup_variables(),
clear the offending pointer before freeing the underlying memory
with the call to session_sysvar_res_mgr.deinit()

This makes table performance_schema.variables_by_thread safe.

Author: marcalff

include/violite.h

@@ -420,4 +420,10 @@ struct Vio {
   Vio &operator=(Vio &&vio);
 };
 
+#ifdef HAVE_OPENSSL
+#define SSL_handle SSL *
+#else
+#define SSL_handle void *
+#endif
+
 #endif /* vio_violite_h_ */

sql/auth/sql_authentication.cc

@@ -1737,7 +1737,7 @@ static bool read_client_connect_attrs(char **ptr, size_t *max_bytes_available,
 static bool acl_check_ssl(THD *thd, const ACL_USER *acl_user) {
 #if defined(HAVE_OPENSSL)
   Vio *vio = thd->get_protocol_classic()->get_vio();
-  SSL *ssl = thd->get_protocol()->get_ssl();
+  SSL *ssl = (SSL *)vio->ssl_arg;
   X509 *cert;
 #endif /* HAVE_OPENSSL */
 
@@ -2807,7 +2807,8 @@ static void server_mpvio_initialize(THD *thd, MPVIO_EXT *mpvio,
   mpvio->auth_info.host_or_ip_length = sctx_host_or_ip.length;
 
 #if defined(HAVE_OPENSSL)
-  if (thd->get_protocol()->get_ssl())
+  Vio *vio = thd->get_protocol_classic()->get_vio();
+  if (vio->ssl_arg)
     mpvio->vio_is_encrypted = 1;
   else
 #endif /* HAVE_OPENSSL */

sql/mysqld.cc

@@ -7640,77 +7640,78 @@ static int show_ssl_ctx_get_session_cache_mode(THD *, SHOW_VAR *var, char *) {
          inside an Event.
  */
 static int show_ssl_get_version(THD *thd, SHOW_VAR *var, char *) {
+  SSL_handle ssl = thd->get_ssl();
   var->type = SHOW_CHAR;
-  if (thd->get_protocol()->get_ssl())
-    var->value =
-        const_cast<char *>(SSL_get_version(thd->get_protocol()->get_ssl()));
+  if (ssl)
+    var->value = const_cast<char *>(SSL_get_version(ssl));
   else
     var->value = (char *)"";
   return 0;
 }
 
 static int show_ssl_session_reused(THD *thd, SHOW_VAR *var, char *buff) {
+  SSL_handle ssl = thd->get_ssl();
   var->type = SHOW_LONG;
   var->value = buff;
-  if (thd->get_protocol()->get_ssl())
-    *((long *)buff) = (long)SSL_session_reused(thd->get_protocol()->get_ssl());
+  if (ssl)
+    *((long *)buff) = (long)SSL_session_reused(ssl);
   else
     *((long *)buff) = 0;
   return 0;
 }
 
 static int show_ssl_get_default_timeout(THD *thd, SHOW_VAR *var, char *buff) {
+  SSL_handle ssl = thd->get_ssl();
   var->type = SHOW_LONG;
   var->value = buff;
-  if (thd->get_protocol()->get_ssl())
-    *((long *)buff) =
-        (long)SSL_get_default_timeout(thd->get_protocol()->get_ssl());
+  if (ssl)
+    *((long *)buff) = (long)SSL_get_default_timeout(ssl);
   else
     *((long *)buff) = 0;
   return 0;
 }
 
 static int show_ssl_get_verify_mode(THD *thd, SHOW_VAR *var, char *buff) {
+  SSL_handle ssl = thd->get_ssl();
   var->type = SHOW_LONG;
   var->value = buff;
-  if (thd->get_protocol()->get_ssl())
-    *((long *)buff) = (long)SSL_get_verify_mode(thd->get_protocol()->get_ssl());
+  if (ssl)
+    *((long *)buff) = (long)SSL_get_verify_mode(ssl);
   else
     *((long *)buff) = 0;
   return 0;
 }
 
 static int show_ssl_get_verify_depth(THD *thd, SHOW_VAR *var, char *buff) {
+  SSL_handle ssl = thd->get_ssl();
   var->type = SHOW_LONG;
   var->value = buff;
-  if (thd->get_protocol()->get_ssl())
-    *((long *)buff) =
-        (long)SSL_get_verify_depth(thd->get_protocol()->get_ssl());
+  if (ssl)
+    *((long *)buff) = (long)SSL_get_verify_depth(ssl);
   else
     *((long *)buff) = 0;
   return 0;
 }
 
 static int show_ssl_get_cipher(THD *thd, SHOW_VAR *var, char *) {
+  SSL_handle ssl = thd->get_ssl();
   var->type = SHOW_CHAR;
-  if (thd->get_protocol()->get_ssl())
-    var->value =
-        const_cast<char *>(SSL_get_cipher(thd->get_protocol()->get_ssl()));
+  if (ssl)
+    var->value = const_cast<char *>(SSL_get_cipher(ssl));
   else
     var->value = (char *)"";
   return 0;
 }
 
 static int show_ssl_get_cipher_list(THD *thd, SHOW_VAR *var, char *buff) {
+  SSL_handle ssl = thd->get_ssl();
   var->type = SHOW_CHAR;
   var->value = buff;
-  if (thd->get_protocol()->get_ssl()) {
+  if (ssl) {
     int i;
     const char *p;
     char *end = buff + SHOW_VAR_FUNC_BUFF_SIZE;
-    for (i = 0; (p = SSL_get_cipher_list(thd->get_protocol()->get_ssl(), i)) &&
-                buff < end;
-         i++) {
+    for (i = 0; (p = SSL_get_cipher_list(ssl, i)) && buff < end; i++) {
       buff = my_stpnmov(buff, p, end - buff - 1);
       *buff++ = ':';
     }

sql/protocol.h

@@ -1,7 +1,7 @@
 #ifndef PROTOCOL_INCLUDED
 #define PROTOCOL_INCLUDED
 
-/* Copyright (c) 2002, 2017, Oracle and/or its affiliates. All rights reserved.
+/* Copyright (c) 2002, 2018, Oracle and/or its affiliates. All rights reserved.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License, version 2.0,
@@ -232,15 +232,6 @@ class Protocol {
                             true then the server is shutting down.
   */
   virtual int shutdown(bool server_shutdown = false) = 0;
-
-  /**
-    Returns pointer to the SSL object/struct
-
-    @return
-      @retval SSL*    The SSL struct/object
-      @retval NULL    If HAVE_OPENSSL is not defined
-  */
-  virtual SSL_handle get_ssl() = 0;
   /**
     Returns the read/writing status
 

sql/protocol_callback.cc

@@ -1,4 +1,4 @@
-/* Copyright (c) 2000, 2017, Oracle and/or its affiliates. All rights reserved.
+/* Copyright (c) 2000, 2018, Oracle and/or its affiliates. All rights reserved.
 
  This program is free software; you can redistribute it and/or modify
  it under the terms of the GNU General Public License, version 2.0,
@@ -266,12 +266,6 @@ int Protocol_callback::shutdown(bool server_shutdown) {
 */
 bool Protocol_callback::connection_alive() { return true; }
 
-/**
-  Returns SSL info. In case of Protocol_callback returns NULL, meaning
-  no SSL connection.
-*/
-SSL_handle Protocol_callback::get_ssl() { return NULL; }
-
 /**
   Should return protocol's reading/writing status. Returns 0 (idle) as it this
   is the best guess that can be made as there is no callback for

sql/protocol_callback.h

@@ -1,4 +1,4 @@
-/* Copyright (c) 2015, 2017, Oracle and/or its affiliates. All rights reserved.
+/* Copyright (c) 2015, 2018, Oracle and/or its affiliates. All rights reserved.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License, version 2.0,
@@ -302,11 +302,6 @@ class Protocol_callback : public Protocol {
   */
   virtual bool connection_alive();
 
-  /**
-    Returns SSL info
-  */
-  virtual SSL_handle get_ssl();
-
   /**
     Should return protocol's reading/writing status. Returns 0 (idle) as it
     this is the best guess that can be made as there is no callback for

sql/protocol_classic.cc

@@ -2971,14 +2971,6 @@ bool Protocol_classic::store_string_aux(const char *from, size_t length,
   return net_store_data((uchar *)from, length);
 }
 
-SSL_handle Protocol_classic::get_ssl() {
-  return
-#ifdef HAVE_OPENSSL
-      m_thd->net.vio ? (SSL *)m_thd->net.vio->ssl_arg :
-#endif
-                     NULL;
-}
-
 int Protocol_classic::shutdown(bool) {
   return m_thd->net.vio ? vio_shutdown(m_thd->net.vio) : 0;
 }

sql/protocol_classic.h

@@ -1,7 +1,7 @@
 #ifndef PROTOCOL_CLASSIC_INCLUDED
 #define PROTOCOL_CLASSIC_INCLUDED
 
-/* Copyright (c) 2002, 2017, Oracle and/or its affiliates. All rights reserved.
+/* Copyright (c) 2002, 2018, Oracle and/or its affiliates. All rights reserved.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License, version 2.0,
@@ -163,8 +163,6 @@ class Protocol_classic : public Protocol {
   uchar get_error();
   /* Set max allowed packet size */
   void set_max_packet_size(ulong max_packet_size);
-  /* Return SSL descriptor, if any */
-  SSL_handle get_ssl();
   /* Deinitialize VIO */
   virtual int shutdown(bool server_shutdown = false);
   /* Wipe NET with zeros */

sql/rpl_slave.cc

@@ -8791,15 +8791,10 @@ bool start_slave(THD *thd, LEX_SLAVE_CONNECTION *connection_param,
   mi->channel_wrlock();
 
   if (connection_param->user || connection_param->password) {
-#if defined(HAVE_OPENSSL)
-    if (!thd->get_protocol()->get_ssl())
+    if (!thd->get_ssl()) {
       push_warning(thd, Sql_condition::SL_NOTE, ER_INSECURE_PLAIN_TEXT,
                    ER_THD(thd, ER_INSECURE_PLAIN_TEXT));
-#endif
-#if !defined(HAVE_OPENSSL)
-    push_warning(thd, Sql_condition::SL_NOTE, ER_INSECURE_PLAIN_TEXT,
-                 ER_THD(thd, ER_INSECURE_PLAIN_TEXT));
-#endif
+    }
   }
 
   lock_slave_threads(mi);  // this allows us to cleanly read slave_running
@@ -9393,15 +9388,10 @@ static int change_receive_options(THD *thd, LEX_MASTER_INFO *lex_mi,
   DBUG_PRINT("info", ("master_log_pos: %lu", (ulong)mi->get_master_log_pos()));
 
   if (lex_mi->user || lex_mi->password) {
-#if defined(HAVE_OPENSSL)
-    if (!thd->get_protocol()->get_ssl())
+    if (!thd->get_ssl()) {
       push_warning(thd, Sql_condition::SL_NOTE, ER_INSECURE_PLAIN_TEXT,
                    ER_THD(thd, ER_INSECURE_PLAIN_TEXT));
-#endif
-#if !defined(HAVE_OPENSSL)
-    push_warning(thd, Sql_condition::SL_NOTE, ER_INSECURE_PLAIN_TEXT,
-                 ER_THD(thd, ER_INSECURE_PLAIN_TEXT));
-#endif
+    }
     push_warning(thd, Sql_condition::SL_NOTE, ER_INSECURE_CHANGE_MASTER,
                  ER_THD(thd, ER_INSECURE_CHANGE_MASTER));
   }

sql/sql_class.cc

@@ -1589,7 +1589,7 @@ int THD::send_explain_fields(Query_result *result) {
 }
 
 enum_vio_type THD::get_vio_type() {
-  DBUG_ENTER("shutdown_active_vio");
+  DBUG_ENTER("THD::get_vio_type");
   DBUG_RETURN(get_protocol()->connection_type());
 }
 
@@ -1599,6 +1599,7 @@ void THD::shutdown_active_vio() {
   if (active_vio) {
     vio_shutdown(active_vio);
     active_vio = 0;
+    m_SSL = NULL;
   }
   DBUG_VOID_RETURN;
 }

sql/sql_class.h

@@ -86,7 +86,8 @@
 #include "mysqld_error.h"
 #include "prealloced_array.h"
 #include "sql/auth/sql_security_ctx.h"  // Security_context
-#include "sql/discrete_interval.h"      // Discrete_interval
+#include "sql/current_thd.h"
+#include "sql/discrete_interval.h"  // Discrete_interval
 #include "sql/mdl.h"
 #include "sql/opt_costmodel.h"
 #include "sql/opt_trace_context.h"  // Opt_trace_context
@@ -1140,6 +1141,20 @@ class THD : public MDL_context_owner,
 
   Protocol *get_protocol() { return m_protocol; }
 
+  SSL_handle get_ssl() const {
+#ifndef DBUG_OFF
+    if (current_thd != this) {
+      /*
+        When inspecting this thread from monitoring,
+        the monitoring thread MUST lock LOCK_thd_data,
+        to be allowed to safely inspect SSL status variables.
+      */
+      mysql_mutex_assert_owner(&LOCK_thd_data);
+    }
+#endif
+    return m_SSL;
+  }
+
   /**
     Asserts that the protocol is of type text or binary and then
     returns the m_protocol casted to Protocol_classic. This method
@@ -1154,6 +1169,17 @@ class THD : public MDL_context_owner,
 
  private:
   Protocol *m_protocol;  // Current protocol
+  /**
+    SSL data attached to this connection.
+    This is an opaque pointer,
+    When building with SSL, this pointer is non NULL
+    only if the connection is using SSL.
+    When building without SSL, this pointer is always NULL.
+    The SSL data can be inspected to read per thread
+    status variables,
+    and this can be inspected while the thread is running.
+  */
+  SSL_handle m_SSL = {nullptr};
 
  public:
   /**
@@ -2477,9 +2503,20 @@ class THD : public MDL_context_owner,
     mysql_mutex_unlock(&LOCK_thd_data);
   }
 
+  inline void set_ssl(Vio *vio MY_ATTRIBUTE((unused))) {
+#ifdef HAVE_OPENSSL
+    mysql_mutex_lock(&LOCK_thd_data);
+    m_SSL = (SSL *)vio->ssl_arg;
+    mysql_mutex_unlock(&LOCK_thd_data);
+#else
+    m_SSL = NULL;
+#endif
+  }
+
   inline void clear_active_vio() {
     mysql_mutex_lock(&LOCK_thd_data);
     active_vio = 0;
+    m_SSL = NULL;
     mysql_mutex_unlock(&LOCK_thd_data);
   }
 

sql/sql_connect.cc

@@ -632,6 +632,14 @@ static int check_connection(THD *thd) {
     reset_host_connect_errors(thd->m_main_security_ctx.ip().str);
   }
 
+  /*
+    Now that acl_authenticate() is executed,
+    the SSL info is available.
+    Advertise it to THD, so SSL status variables
+    can be inspected.
+  */
+  thd->set_ssl(net->vio);
+
   return auth_rc;
 }
 

sql/sql_plugin.cc

@@ -2815,6 +2815,8 @@ static void cleanup_variables(THD *thd, struct System_variables *vars) {
     mysql_mutex_lock(&thd->LOCK_thd_data);
 
     plugin_var_memalloc_free(&thd->variables);
+    /* Remove references to session_sysvar_res_mgr memory before freeing it. */
+    thd->variables.track_sysvars_ptr = NULL;
     thd->session_sysvar_res_mgr.deinit();
   }
   DBUG_ASSERT(vars->table_plugin == NULL);

sql/sql_prepare.cc

@@ -209,7 +209,6 @@ class Protocol_local : public Protocol {
   virtual void end_partial_result_set();
   virtual int shutdown(bool server_shutdown = false);
   virtual bool connection_alive();
-  virtual SSL_handle get_ssl();
   virtual void start_row();
   virtual bool end_row();
   virtual void abort_row(){};
@@ -3546,8 +3545,6 @@ void Protocol_local::end_partial_result_set() {}
 
 int Protocol_local::shutdown(bool) { return 0; }
 
-SSL_handle Protocol_local::get_ssl() { return NULL; }
-
 /**
   Called between two result set rows.