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
