MDEV-19838 Wrong direxec param data caused crashbb-10.3-mdev-19838

In case of direct execution(stmtid=-1, mariadb_stmt_execute_direct in C
API) application is in control of how many parameters client sends to
the server. In case this number is not equal to actual query parameters
number, the server may start to interprete packet data incorrectly, e.g.
starting from the size of null bitmap. And that could cause it to crash
at some point. The commit introduces some additional COM_STMT_EXECUTE
packet sanity checks:
- checking that "types sent" byte is set, and the value is equal to 1.
  if it's not direct execution, then that value is 0 or 1.
- checking that parameter type value is a valid type, and parameter
  flags value is 0 or only "unsigned" bit is set
- added more checks that read does not go beyond the end of the packet

2 files changed, 288 insertions, 8 deletions

diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc
index b3c5700a0b7..33695e4599b 100644
--- a/sql/sql_prepare.cc
+++ b/sql/sql_prepare.cc
@@ -123,6 +123,9 @@ When one supplies long data for a placeholder:
 #include "transaction.h"                        // trans_rollback_implicit
 #include "wsrep_mysqld.h"
 
+/* Constants defining bits in parameter type flags. Flags are read from high byte of short value */
+static const uint parameter_flag_unsigned = 128U << 8;
+
 /**
   A result class used to send cursor rows using the binary protocol.
 */
@@ -949,11 +952,67 @@ static bool insert_bulk_params(Prepared_statement *stmt,
   DBUG_RETURN(0);
 }
 
-static bool set_conversion_functions(Prepared_statement *stmt,
-                                     uchar **data, uchar *data_end)
+/**
+  Checking if parameter type and flags are valid
+  @param typecode - ushort value with type in low byte, and flags in high byte
+*/
+bool
+parameter_type_sanity_check(ushort typecode)
+{
+  /* Checking if type in lower byte is valid */
+  switch (typecode & 0xff) {
+  case MYSQL_TYPE_DECIMAL:
+  case MYSQL_TYPE_NEWDECIMAL:
+  case MYSQL_TYPE_TINY:
+  case MYSQL_TYPE_SHORT:
+  case MYSQL_TYPE_LONG:
+  case MYSQL_TYPE_LONGLONG:
+  case MYSQL_TYPE_INT24:
+  case MYSQL_TYPE_YEAR:
+  case MYSQL_TYPE_BIT:
+  case MYSQL_TYPE_FLOAT:
+  case MYSQL_TYPE_DOUBLE:
+  case MYSQL_TYPE_NULL:
+  case MYSQL_TYPE_VARCHAR:
+  case MYSQL_TYPE_TINY_BLOB:
+  case MYSQL_TYPE_MEDIUM_BLOB:
+  case MYSQL_TYPE_LONG_BLOB:
+  case MYSQL_TYPE_BLOB:
+  case MYSQL_TYPE_VAR_STRING:
+  case MYSQL_TYPE_STRING:
+  case MYSQL_TYPE_ENUM:
+  case MYSQL_TYPE_SET:
+  case MYSQL_TYPE_GEOMETRY:
+  case MYSQL_TYPE_TIMESTAMP:
+  case MYSQL_TYPE_DATE:
+  case MYSQL_TYPE_TIME:
+  case MYSQL_TYPE_DATETIME:
+  // This types normally cannot be sent by client, so maybe it'd be better to treat them like an error here
+  case MYSQL_TYPE_TIMESTAMP2:
+  case MYSQL_TYPE_TIME2:
+  case MYSQL_TYPE_DATETIME2:
+  case MYSQL_TYPE_NEWDATE:
+  case MYSQL_TYPE_VARCHAR_COMPRESSED:
+  case MYSQL_TYPE_BLOB_COMPRESSED:
+    break;
+  default:
+    return true;
+  };
+ 
+  // In Flags in high byte only unsigned bit may be set
+  if (typecode & ((~parameter_flag_unsigned) & 0x0000ff00))
+  {
+    return true;
+  }
+  return false;
+}
+
+static bool 
+set_conversion_functions(Prepared_statement *stmt,
+                         uchar **data, uchar *data_end)
 {
   uchar *read_pos= *data;
-  const uint signed_bit= 1 << 15;
+  
   DBUG_ENTER("set_conversion_functions");
   /*
      First execute or types altered by the client, setup the
@@ -966,12 +1025,15 @@ static bool set_conversion_functions(Prepared_statement *stmt,
   {
     ushort typecode;
 
-    if (read_pos >= data_end)
-      DBUG_RETURN(1);
-
+    /* stmt_execute_packet_sanity_check has already verified, that there are enough data in the packet for data types */
     typecode= sint2korr(read_pos);
     read_pos+= 2;
-    (**it).unsigned_flag= MY_TEST(typecode & signed_bit);
+
+    if (parameter_type_sanity_check(typecode))
+    {
+      DBUG_RETURN(1);
+    }
+    (**it).unsigned_flag= MY_TEST(typecode & parameter_flag_unsigned);
     (*it)->setup_conversion(thd, (uchar) (typecode & 0xff));
     (*it)->sync_clones();
   }
@@ -3126,11 +3188,20 @@ static void mysql_stmt_execute_common(THD *thd,
 
 void mysqld_stmt_execute(THD *thd, char *packet_arg, uint packet_length)
 {
+  const uint packet_min_lenght= 9;
   uchar *packet= (uchar*)packet_arg; // GCC 4.0.1 workaround
+
+  DBUG_ENTER("mysqld_stmt_execute");
+
+  if (packet_length < packet_min_lenght)
+  {
+    my_error(ER_MALFORMED_PACKET, MYF(0), 0,
+      "", "mysqld_stmt_execute");
+    DBUG_VOID_RETURN;
+  }
   ulong stmt_id= uint4korr(packet);
   ulong flags= (ulong) packet[4];
   uchar *packet_end= packet + packet_length;
-  DBUG_ENTER("mysqld_stmt_execute");
 
   packet+= 9;                               /* stmt_id + 5 bytes of flags */
 
@@ -3186,7 +3257,54 @@ void mysqld_stmt_bulk_execute(THD *thd, char *packet_arg, uint packet_length)
   DBUG_VOID_RETURN;
 }
 
+/**
+Additional packet checks for direct execution
+
+@param thd             THD handle
+@param stmt            prepared statement being directly executed
+@param paket           packet with parameters to bind
+@param packet_end      pointer to the byte after parameters end
+@param bulk_op         is it bulk operation
+@param direct_exec     is it direct execution
+*/
+bool stmt_execute_packet_sanity_check(Prepared_statement *stmt, uchar *packet, uchar *packet_end, bool bulk_op, bool direct_exec)
+{
+  if (stmt->param_count > 0)
+  {
+    uint packet_length= packet_end - packet;
+    uint null_bitmap_bytes= (uint)(bulk_op ? 0 : (stmt->param_count + 7)/8);
+    uint min_len_for_param_count = null_bitmap_bytes
+                                 + 1;               /* sent types byte */
+
+    if (packet_length >= min_len_for_param_count)
+    {
+      if (!bulk_op && packet[null_bitmap_bytes])
+      {
+        /* Should be 0 or 1. If the byte is not 1, that could mean,
+           e.g. that we read incorrect byte due to incorrect number of sent parameters for direct execution
+           (i.e. null bitmap is shorter or longer, than it should be) */
+        if (packet[null_bitmap_bytes] != '\1')
+        {
+          return true;
+        }
+        min_len_for_param_count+= 2*stmt->param_count; /* 2 bytes per parameter of the type and flags */
+      }
+      else /* If types are not sent, there is nothing to do here. But for direct execution types should always be sent */
+      {
+        return direct_exec;
+      }
+    }
 
+    /* If true, the packet is guaranteed too short for the number of parameters in the PS */
+    return (packet_length < min_len_for_param_count);
+  }
+  else
+  {
+    /* If there is no parameters, this should be normally already end of the packet. If it's not - then error */
+    return (packet_end > packet);
+  }
+  return false;
+}
 /**
   Common part of prepared statement execution
 
@@ -3225,6 +3343,20 @@ static void mysql_stmt_execute_common(THD *thd,
              llstr(stmt_id, llbuf), "mysqld_stmt_execute");
     DBUG_VOID_RETURN;
   }
+
+  /*
+  * In case of direct execution application decides how many parameters to send.
+  * Thus extra checks are reuqired to prevent crashes caused by incorrect interpretation of
+  * the packet data. Plus there can be always a broken evil client.
+  */
+  if (stmt_execute_packet_sanity_check(stmt, packet, packet_end, bulk_op, stmt_id == LAST_STMT_ID))
+  {
+    char llbuf[22];
+    my_error(ER_MALFORMED_PACKET, MYF(0), static_cast<int>(sizeof(llbuf)),
+             llstr(stmt_id, llbuf), "mysqld_stmt_execute");
+    DBUG_VOID_RETURN;
+  }
+
   stmt->read_types= read_types;
 
 #if defined(ENABLED_PROFILING)
diff --git a/tests/mysql_client_test.c b/tests/mysql_client_test.c
index edad6453102..0ed18a05bed 100644
--- a/tests/mysql_client_test.c
+++ b/tests/mysql_client_test.c
@@ -36,6 +36,9 @@
 #endif
 
 #include "mysql_client_fw.c"
+#ifndef MARIADB_STMT_BULK_SUPPORTED
+#include "mariadb_stmt.h"
+#endif // !MARIADB_STMT_BULK_SUPPORTED
 #ifndef _WIN32
 #include <arpa/inet.h>
 #endif
@@ -20860,6 +20863,150 @@ static void test_explain_meta()
 }
 
 
+#define MDEV19838_MAX_PARAM_COUNT 32
+#define MDEV19838_FIELDS_COUNT 17
+static void test_mdev19838()
+{
+  int rc;
+  MYSQL_BIND bind[MDEV19838_MAX_PARAM_COUNT];
+  unsigned int i, paramCount = 1;
+  char charvalue[] = "012345678901234567890123456789012345";
+  MYSQL_STMT *stmt;
+
+  myheader("test_mdev19838");
+
+  rc = mysql_query(mysql, "CREATE TABLE mdev19838("
+    	  "f1  char(36),"
+    	  "f2  char(36),"
+    	  "f3  char(36),"
+    	  "f4  char(36),"
+    	  "f5  char(36),"
+    	  "f6  char(36),"
+    	  "f7  char(36),"
+    	  "f8  char(36),"
+    	  "f9  char(36),"
+    	  "f10 char(36),"
+    	  "f11 char(36),"
+    	  "f12 char(36),"
+    	  "f13 char(36),"
+    	  "f14 char(36),"
+    	  "f15 char(36),"
+    	  "f16 char(36),"
+    	  "f17 char(36)"
+    ")");
+  myquery(rc);
+
+  stmt = mysql_stmt_init(mysql);
+  check_stmt(stmt);
+
+  memset(bind, 0, sizeof(bind));
+
+  for (i = 0; i < MDEV19838_MAX_PARAM_COUNT; ++i)
+  {
+    bind[i].buffer = charvalue;
+    bind[i].buffer_type = MYSQL_TYPE_STRING;
+    bind[i].buffer_length = strlen(charvalue) + 1;
+    bind[i].length = &bind[i].length_value;
+    bind[i].length_value = bind[i].buffer_length - 1;
+  }
+
+  for (paramCount = 1; paramCount < MDEV19838_FIELDS_COUNT; ++paramCount)
+  {
+    mysql_stmt_attr_set(stmt, STMT_ATTR_PREBIND_PARAMS, &paramCount);
+
+    rc = mysql_stmt_bind_param(stmt, bind);
+    check_execute(stmt, rc);
+
+    rc = mariadb_stmt_execute_direct(stmt, "INSERT INTO mdev19838"
+      "(f1, f2, f3, f4, f5, f6, f7, f8, f9, f10, f11, f12, f13, f14, f15, f16, f17)"
+      " VALUES "
+      "(?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)", -1);
+
+    /* Expecting an error */
+    DIE_UNLESS(rc != 0);
+
+    mysql_stmt_close(stmt);
+    stmt = mysql_stmt_init(mysql);
+    check_stmt(stmt);
+  }
+
+  paramCount = 0;
+  mysql_stmt_attr_set(stmt, STMT_ATTR_PREBIND_PARAMS, &paramCount);
+  rc = mariadb_stmt_execute_direct(stmt, "INSERT INTO mdev19838(f1)"
+    " VALUES (?)", -1);
+  /* Expecting an error */
+  DIE_UNLESS(rc != 0);
+  mysql_stmt_close(stmt);
+
+  stmt = mysql_stmt_init(mysql);
+  check_stmt(stmt);
+  /* Correct number of parameters */
+  paramCount = MDEV19838_FIELDS_COUNT;
+  mysql_stmt_attr_set(stmt, STMT_ATTR_PREBIND_PARAMS, &paramCount);
+  mysql_stmt_bind_param(stmt, bind);
+
+  rc = mariadb_stmt_execute_direct(stmt, "INSERT INTO mdev19838"
+    "(f1, f2, f3, f4, f5, f6, f7, f8, f9, f10, f11, f12, f13, f14, f15, f16, f17)"
+    " VALUES "
+    "(?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)", -1);
+  check_execute(stmt, rc);
+
+  /* MYSQL_TYPE_TINY = 1. This parameter byte can be read as "parameters send" flag byte.
+     Checking that wrong packet is still detected */
+  bind[0].buffer_type = MYSQL_TYPE_TINY;
+  bind[0].length_value = 1;
+  bind[0].buffer_length = 1;
+
+  for (paramCount = 8; paramCount > 0; --paramCount)
+  {
+    mysql_stmt_close(stmt);
+    stmt = mysql_stmt_init(mysql);
+    check_stmt(stmt);
+
+    mysql_stmt_attr_set(stmt, STMT_ATTR_PREBIND_PARAMS, &paramCount);
+
+    rc = mysql_stmt_bind_param(stmt, bind);
+    check_execute(stmt, rc);
+
+    rc = mariadb_stmt_execute_direct(stmt, "INSERT INTO mdev19838"
+      "(f1, f2, f3, f4, f5, f6, f7, f8, f9)"
+      " VALUES "
+      "(?, ?, ?, ?, ?, ?, ?, ?, ?)", -1);
+
+    /* Expecting an error */
+    DIE_UNLESS(rc != 0);
+  }
+
+  /* Test of query w/out parameters, with parameter sent and not sent */
+  for (paramCount = MDEV19838_MAX_PARAM_COUNT; paramCount != (unsigned int)-1; --paramCount)
+  {
+    mysql_stmt_close(stmt);
+    stmt = mysql_stmt_init(mysql);
+    check_stmt(stmt);
+
+    mysql_stmt_attr_set(stmt, STMT_ATTR_PREBIND_PARAMS, &paramCount);
+
+    if (paramCount > 0)
+    {
+      rc = mysql_stmt_bind_param(stmt, bind);
+      check_execute(stmt, rc);
+    }
+
+    rc = mariadb_stmt_execute_direct(stmt, "INSERT INTO mdev19838"
+      "(f1)"
+      " VALUES "
+      "(0x1111111111111111)", -1);
+
+    /* Expecting an error if parameters are sent */
+    DIE_UNLESS(rc != 0 || paramCount == 0);
+  }
+
+  mysql_stmt_close(stmt);
+
+  rc = mysql_query(mysql, "drop table mdev19838");
+  myquery(rc);
+}
+
 static struct my_tests_st my_tests[]= {
   { "disable_query_logs", disable_query_logs },
   { "test_view_sp_list_fields", test_view_sp_list_fields },
@@ -21152,6 +21299,7 @@ static struct my_tests_st my_tests[]= {
 #endif
   { "test_ps_params_in_ctes", test_ps_params_in_ctes },
   { "test_explain_meta", test_explain_meta },
+  { "test_mdev19838", test_mdev19838 },
   { 0, 0 }
 };