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? 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".
> + char *end=buff;
magnus: ^ space between = and buff
> 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'
>
> /* 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?
> +
> + DBUG_VOID_RETURN;
> +}
> +
> /*
> Read and parse arguments and MySQL options from my.cnf
> */
> @@ -16149,6 +16179,7 @@ static struct my_tests_st my_tests[]= {
> { "test_bug27592", test_bug27592 },
> { "test_bug29948", test_bug29948 },
> { "test_bug29306", test_bug29306 },
> + { "test_bug31669", test_bug31669 },
> { 0, 0 }
> };
>
>
--
Magnus Svensson
Senior Software Engineer
msvensson@stripped
+46709164491
