Skip to content
Permalink
Browse files
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
  • Loading branch information
@lawrinn @sanja-byelkin
lawrinn authored and sanja-byelkin committed Oct 29, 2020
1 parent 6cb8868 commit 4b854d4
Show file tree
Hide file tree
Showing 2 changed files with 336 additions and 12 deletions.
199 sql/sql_prepare.cc
View file
@@ -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

0 comments on commit 4b854d4

Please sign in to comment.