List:Commits« Previous MessageNext Message »
From:Davi Arnaut Date:December 24 2010 10:31am
Subject:Re: bzr commit into mysql-5.1-bugteam branch (Dmitry.Shulga:3512)
Bug#56976
View as plain text  
Hi Dmitry,

On 12/16/10 2:52 PM, Dmitry Shulga wrote:
> #At file:///Users/shulga/projects/mysql/5.1-bugteam-bug56976/ based on
> revid:mats.kindahl@stripped
>
>   3512 Dmitry Shulga	2010-12-16
>        Fixed bug#56976 - Severe Denial Of Service in prepared statements.

Some comments below.

>        The problem is that it doesn't check size of concatenated string
>        against some limit when reading long data from client.
>
>        The solution is to add check for size of result string against
>        value of max_allowed_packet constant.
>       @ sql/item.cc
>          Item_param::set_longdata - added check for size of concatenated result
>          against value of max_allowed_packet constant.
>       @ sql/item.h
>          added third argument of type THD* to declaration of set_longdata().
>       @ sql/net_serv.cc
>          Fixed an error that had been added by patch for bug#42503.
>          This change will fix bug#58887.

Please make it a separate patch. In this separate patch, you should 
properly cite Bug#58887 so the patch gets associated with the bug report.

>      modified:
>        sql/item.cc
>        sql/item.h
>        sql/net_serv.cc
>        sql/sql_prepare.cc
> === modified file 'sql/item.cc'
> --- a/sql/item.cc	2010-11-18 13:11:18 +0000
> +++ b/sql/item.cc	2010-12-16 16:52:09 +0000
> @@ -2738,7 +2738,7 @@ bool Item_param::set_str(const char *str
>   }
>
>
> -bool Item_param::set_longdata(const char *str, ulong length)
> +bool Item_param::set_longdata(const char *str, ulong length, THD *thd)

Pass THD as the first argument.

>   {
>     DBUG_ENTER("Item_param::set_longdata");
>
> @@ -2751,6 +2751,9 @@ bool Item_param::set_longdata(const char
>       (here), and first have to concatenate all pieces together,
>       write query to the binary log and only then perform conversion.
>     */
> +  if (str_value.length() + length > thd->variables.max_allowed_packet)
> +    DBUG_RETURN(TRUE);

This will yield a ER_OUTOFMEMORY, but I think it is more appropriate to 
set the error to ER_TOO_LONG_STRING. One problem is that error handling 
is done in the caller, mysql_stmt_get_longdata. I think to implement 
this properly we need to set the PS error fields (state, last_errno, 
last_error etc) based from values on the diagnostics area.

>     if (str_value.append(str, length,&my_charset_bin))
>       DBUG_RETURN(TRUE);
>     state= LONG_DATA_VALUE;
>

[..]

>
> === modified file 'sql/net_serv.cc'
> --- a/sql/net_serv.cc	2010-09-16 10:24:27 +0000
> +++ b/sql/net_serv.cc	2010-12-16 16:52:09 +0000
> @@ -170,17 +170,7 @@ my_bool net_realloc(NET *net, size_t len
>     DBUG_ENTER("net_realloc");
>     DBUG_PRINT("enter",("length: %lu", (ulong) length));
>
> -  /*
> -    When compression is off, net->where_b is always 0.
> -    With compression turned on, net->where_b may indicate
> -    that we still have a piece of the previous logical
> -    packet in the buffer, unprocessed. Take it into account
> -    when checking that max_allowed_packet is not exceeded.
> -    This ensures that the client treats max_allowed_packet
> -    limit identically, regardless of compression being on
> -    or off.
> -  */
> -  if (length>= (net->max_packet_size + net->where_b))
> +  if (length>= net->max_packet_size)

This is a separate bug report, please make a separate patch.

>     {
>       DBUG_PRINT("error", ("Packet too large. Max size: %lu",
>                            net->max_packet_size));
>
> === modified file 'sql/sql_prepare.cc'
> --- a/sql/sql_prepare.cc	2010-11-03 10:24:47 +0000
> +++ b/sql/sql_prepare.cc	2010-12-16 16:52:09 +0000
> @@ -2800,9 +2800,9 @@ void mysql_stmt_get_longdata(THD *thd, c
>     param= stmt->param_array[param_number];
>
>   #ifndef EMBEDDED_LIBRARY
> -  if (param->set_longdata(packet, (ulong) (packet_end - packet)))
> +  if (param->set_longdata(packet, (ulong) (packet_end - packet), thd))
>   #else
> -  if (param->set_longdata(thd->extra_data, thd->extra_length))
> +  if (param->set_longdata(thd->extra_data, thd->extra_length, thd))
>   #endif
>     {

Please, also add a appropriate test case. Take a look at:

	tests/mysql_client_test.c

Regards,

Davi
Thread
bzr commit into mysql-5.1-bugteam branch (Dmitry.Shulga:3512) Bug#56976Dmitry Shulga16 Dec
  • Re: bzr commit into mysql-5.1-bugteam branch (Dmitry.Shulga:3512)Bug#56976Davi Arnaut24 Dec