Magnus Svensson wrote:
> fre 2007-10-19 klockan 12:32 -0300 skrev Davi Arnaut:
>> Below is the list of changes that have just been committed into a local
>> 5.0 repository of davi. When davi does a push these changes will
>> be propagated to the main repository and, within 24 hours after the
>> push, to the public repository.
>> For information on how to access the public repository
>> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
>>
>> ChangeSet@stripped, 2007-10-19 12:32:33-03:00, davi@stripped +2 -0
>> Bug#31669 Buffer overflow in mysql_change_user()
>>
>> The problem is that when copying the supplied username and
>> database, no bounds checking is performed on the fixed-length
>> buffer. A sufficiently large (> 512) user string can easily
>> cause stack corruption. Since this API can be used from PHP
>> and other programs, this is a serious problem.
>>
>> The solution is to increase the buffer size to the accepted
>> size in similar functions and perform bounds checking when
>> copying the username and database.
>>
>> libmysql/libmysql.c@stripped, 2007-10-19 12:32:30-03:00, davi@stripped +5 -3
>> Increase the buffer size and perform bounds checking when copying
>> the supplied arguments.
>>
>> tests/mysql_client_test.c@stripped, 2007-10-19 12:32:30-03:00, davi@stripped
> +31 -0
>> Add test case for Bug#31669
>>
>> diff -Nrup a/libmysql/libmysql.c b/libmysql/libmysql.c
>> --- a/libmysql/libmysql.c 2007-10-17 14:22:40 -03:00
>> +++ b/libmysql/libmysql.c 2007-10-19 12:32:30 -03:00
>> @@ -706,7 +706,8 @@ int cli_read_change_user_result(MYSQL *m
>> my_bool STDCALL mysql_change_user(MYSQL *mysql, const char *user,
>> const char *passwd, const char *db)
>> {
>> - char buff[512],*end=buff;
>> + char buff[USERNAME_LENGTH+SCRAMBLED_PASSWORD_CHAR_LENGTH+NAME_LEN+100];
>
> magnus: Hmm, how large does that get? Looking at
> 'client_mysql_real_connect' we use "NAME_LEN+USER_NAME_LENGTH+100" and
> maybe that is actually enough?
I prefer to explicitly count the scrambled password length.
> Isn't the +100 supposed to take care of
> allocating some extra space for "password and stuff" ;)
>
> Please think a little more about size of this buffer. Maybe we should
> reserve the proper size for "db" that we also put in there? Then we can
> probably skip the +100 "magic constant".
I think I should have skipped it.. we won't copy more then
USERNAME_LENGTH + SCRAMBLED_PASSWORD_CHAR_LENGTH + NAME_LEN anyway.
Checking just in case:
USERNAME_LENGTH is 16.
SCRAMBLED_PASSWORD_CHAR_LENGTH is 20*2+1.
NAME_LEN is 64.
NUL's + 2.
Well, dunno where that magic +100 comes from. I think we can safely just
use: USERNAME_LENGTH + SCRAMBLED_PASSWORD_CHAR_LENGTH + NAME_LEN + 2
>> + char *end=buff;
> magnus: ^ space between = and buff
Ok.
>> int rc;
>> DBUG_ENTER("mysql_change_user");
>>
>> @@ -716,7 +717,8 @@ my_bool STDCALL mysql_change_user(MYSQL
>> passwd="";
>>
>> /* Store user into the buffer */
>> - end=strmov(end,user)+1;
>> + strmake(end, user, USERNAME_LENGTH);
>> + end= strend(end) + 1;
>
> magnus: "strmake() returns pointer to closing null" according to
> comment in strings/strmake.c - I guess you could skip the strend call as
> you do in the next call to 'strmake'
Ok.
>>
>> /* write scrambled password according to server capabilities */
>> if (passwd[0])
>> @@ -736,7 +738,7 @@ my_bool STDCALL mysql_change_user(MYSQL
>> else
>> *end++= '\0'; /* empty password */
>> /* Add database if needed */
>> - end= strmov(end, db ? db : "") + 1;
>> + end= strmake(end, db ? db : "", NAME_LEN) + 1;
>
> magnus: ok.
>
>>
>> /* Write authentication package */
>> simple_command(mysql,COM_CHANGE_USER, buff,(ulong) (end-buff),1);
>> diff -Nrup a/tests/mysql_client_test.c b/tests/mysql_client_test.c
>> --- a/tests/mysql_client_test.c 2007-09-14 23:45:13 -03:00
>> +++ b/tests/mysql_client_test.c 2007-10-19 12:32:30 -03:00
>> @@ -15857,6 +15857,36 @@ static void test_bug29306()
>> DBUG_VOID_RETURN;
>> }
>>
>> +
>> +/**
>> + Bug#31669 Buffer overflow in mysql_change_user()
>> +*/
>> +
>> +static void test_bug31669()
>> +{
>> + int rc;
>> + char buff[4096];
>> +
>> + DBUG_ENTER("test_bug31669");
>> + myheader("test_bug31669");
>> +
>> + rc= mysql_change_user(mysql, NULL, NULL, NULL);
>> + DIE_UNLESS(rc);
>> +
>> + rc= mysql_change_user(mysql, "", "", "");
>> + DIE_UNLESS(rc);
>> +
>> + memset(buff, 'a', sizeof(buff));
>> +
>> + rc= mysql_change_user(mysql, buff, buff, buff);
>> + DIE_UNLESS(rc);
>> +
>> + rc = mysql_change_user(mysql, opt_user, opt_password, current_db);
>> + DIE_UNLESS(!rc);
>
> magnus: Maybe add a test that checks the function succeeds when using
> the max allowed value of user, password and db?
Will do.
Thanks,
--
Davi Arnaut, Software Engineer
MySQL Inc, www.mysql.com
Are you MySQL certified? www.mysql.com/certification
