MDEV-19838 Wrong direxec param data caused crash

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

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.
 */
@@ -1003,11 +1006,73 @@ 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
+
+  @retval true  this parameter is wrong
+  @retval false this parameter is OK
+*/
+
+static 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:
+  case MYSQL_TYPE_NEWDATE:
+  break;
+  /*
+    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:
+  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 *read_pos= *data;
-  const uint signed_bit= 1 << 15;
+
   DBUG_ENTER("set_conversion_functions");
   /*
      First execute or types altered by the client, setup the
@@ -1020,12 +1085,17 @@ 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);
     setup_one_conversion_function(thd, *it, (uchar) (typecode & 0xff));
     (*it)->sync_clones();
   }
@@ -1035,7 +1105,7 @@ static bool set_conversion_functions(Prepared_statement *stmt,
 
 
 static bool setup_conversion_functions(Prepared_statement *stmt,
-                                       uchar **data, uchar *data_end,
+                                       uchar **data,
                                        bool bulk_protocol= 0)
 {
   /* skip null bits */
@@ -1048,7 +1118,7 @@ static bool setup_conversion_functions(Prepared_statement *stmt,
   if (*read_pos++) //types supplied / first execute
   {
     *data= read_pos;
-    bool res= set_conversion_functions(stmt, data, data_end);
+    bool res= set_conversion_functions(stmt, data);
     DBUG_RETURN(res);
   }
   *data= read_pos;
@@ -3159,11 +3229,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 */
 
@@ -3219,6 +3298,84 @@ 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
+  @param read_bytes      need to read types (only with bulk_op)
+
+  @retval true  this parameter is wrong
+  @retval false this parameter is OK
+*/
+
+static bool
+stmt_execute_packet_sanity_check(Prepared_statement *stmt,
+                                 uchar *packet, uchar *packet_end,
+                                 bool bulk_op, bool direct_exec,
+                                 bool read_types)
+{
+
+  DBUG_ASSERT((!read_types) || (read_types && bulk_op));
+  if (stmt->param_count > 0)
+  {
+    uint packet_length= static_cast<uint>(packet_end - packet);
+    uint null_bitmap_bytes= (bulk_op ? 0 : (stmt->param_count + 7)/8);
+    uint min_len_for_param_count = null_bitmap_bytes
+                                 + (bulk_op ? 0 : 1); /* sent types byte */
+
+    if (!bulk_op && packet_length >= min_len_for_param_count)
+    {
+      if ((read_types= 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;
+        }
+      }
+    }
+
+    if (read_types)
+    {
+      /* 2 bytes per parameter of the type and flags */
+      min_len_for_param_count+= 2*stmt->param_count;
+    }
+    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
@@ -3258,6 +3415,24 @@ 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 required 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, read_types))
+  {
+    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)
@@ -4168,7 +4343,7 @@ Prepared_statement::set_parameters(String *expanded_query,
   {
 #ifndef EMBEDDED_LIBRARY
     uchar *null_array= packet;
-    res= (setup_conversion_functions(this, &packet, packet_end) ||
+    res= (setup_conversion_functions(this, &packet) ||
           set_params(this, null_array, packet, packet_end, expanded_query));
 #else
     /*
@@ -4400,7 +4575,7 @@ Prepared_statement::execute_bulk_loop(String *expanded_query,
 
 #ifndef EMBEDDED_LIBRARY
   if (read_types &&
-      set_conversion_functions(this, &packet, packet_end))
+      set_conversion_functions(this, &packet))
 #else
   // bulk parameters are not supported for embedded, so it will an error
 #endif

tests/mysql_client_test.c

@@ -19897,6 +19897,152 @@ static void test_ps_params_in_ctes()
 }
 
 
+#ifndef EMBEDDED_LIBRARY
+#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);
+}
+#endif // EMBEDDED_LIBRARY
+
 static struct my_tests_st my_tests[]= {
   { "disable_query_logs", disable_query_logs },
   { "test_view_sp_list_fields", test_view_sp_list_fields },
@@ -20182,6 +20328,9 @@ static struct my_tests_st my_tests[]= {
   { "test_bulk_replace", test_bulk_replace },
 #endif
   { "test_ps_params_in_ctes", test_ps_params_in_ctes },
+#ifndef EMBEDDED_LIBRARY
+  { "test_mdev19838", test_mdev19838 },
+#endif
   { 0, 0 }
 };