From 07bfc5a2d6ebaa1a0da406177e443948ef93f5c7 Mon Sep 17 00:00:00 2001 From: unknown Date: Thu, 29 Apr 2010 09:57:25 +0200 Subject: [PATCH 1/2] Fix buffer overflow in COM_FIELD_LIST. Fix missing bounds check in string conversion. Bump version number for security fix release. --- configure.in | 2 +- sql/sql_base.cc | 8 ++++++-- sql/sql_parse.cc | 30 ++++++++++++++++++++---------- strings/ctype-utf8.c | 4 ++++ 4 files changed, 31 insertions(+), 13 deletions(-) diff --git a/configure.in b/configure.in index 47682c34fea..45952dbeceb 100644 --- a/configure.in +++ b/configure.in @@ -7,7 +7,7 @@ AC_PREREQ(2.59) # Remember to also update version.c in ndb. # When changing major version number please also check switch statement # in mysqlbinlog::check_master_version(). -AC_INIT([MariaDB Server], [5.1.44-MariaDB], [], [mysql]) +AC_INIT([MariaDB Server], [5.1.44a-MariaDB], [], [mysql]) AC_CONFIG_SRCDIR([sql/mysqld.cc]) AC_CANONICAL_SYSTEM # USTAR format gives us the possibility to store longer path names in diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 1da17c216f2..4416ebbc45c 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -233,8 +233,12 @@ static void check_unused(void) uint create_table_def_key(THD *thd, char *key, TABLE_LIST *table_list, bool tmp_table) { - uint key_length= (uint) (strmov(strmov(key, table_list->db)+1, - table_list->table_name)-key)+1; + char *db_end= strnmov(key, table_list->db, MAX_DBKEY_LENGTH - 2); + *db_end++= '\0'; + char *table_end= strnmov(db_end, table_list->table_name, + key + MAX_DBKEY_LENGTH - 1 - db_end); + *table_end++= '\0'; + uint key_length= (uint) (table_end-key); if (tmp_table) { int4store(key + key_length, thd->server_id); diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index 9503e8a5d81..2eb6a190e63 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -1304,10 +1304,12 @@ bool dispatch_command(enum enum_server_command command, THD *thd, break; #else { - char *fields, *packet_end= packet + packet_length, *arg_end; + char *fields, *packet_end= packet + packet_length, *wildcard; /* Locked closure of all tables */ TABLE_LIST table_list; - LEX_STRING conv_name; + char db_buff[NAME_LEN+1]; + uint32 db_length; + uint dummy_errors; /* used as fields initializator */ lex_start(thd); @@ -1319,11 +1321,22 @@ bool dispatch_command(enum enum_server_command command, THD *thd, /* We have name + wildcard in packet, separated by endzero */ - arg_end= strend(packet); - thd->convert_string(&conv_name, system_charset_info, - packet, (uint) (arg_end - packet), thd->charset()); - table_list.alias= table_list.table_name= conv_name.str; - packet= arg_end + 1; + wildcard= strend(packet); + db_length= wildcard - packet; + wildcard++; + uint query_length= (uint) (packet_end - wildcard); // Don't count end \0 + if (db_length > NAME_LEN || query_length > NAME_LEN) + { + my_message(ER_UNKNOWN_COM_ERROR, ER(ER_UNKNOWN_COM_ERROR), MYF(0)); + break; + } + db_length= copy_and_convert(db_buff, sizeof(db_buff)-1, + system_charset_info, packet, db_length, + thd->charset(), &dummy_errors); + db_buff[db_length]= '\0'; + table_list.alias= table_list.table_name= db_buff; + if (!(fields= (char *) thd->memdup(wildcard, query_length + 1))) + break; if (is_schema_db(table_list.db, table_list.db_length)) { @@ -1332,9 +1345,6 @@ bool dispatch_command(enum enum_server_command command, THD *thd, table_list.schema_table= schema_table; } - uint query_length= (uint) (packet_end - packet); // Don't count end \0 - if (!(fields= (char *) thd->memdup(packet, query_length + 1))) - break; thd->set_query(fields, query_length); general_log_print(thd, command, "%s %s", table_list.table_name, fields); if (lower_case_table_names) diff --git a/strings/ctype-utf8.c b/strings/ctype-utf8.c index 91f633e45ce..a894afd71a9 100644 --- a/strings/ctype-utf8.c +++ b/strings/ctype-utf8.c @@ -4116,6 +4116,10 @@ my_wc_mb_filename(CHARSET_INFO *cs __attribute__((unused)), { int code; char hex[]= "0123456789abcdef"; + + if (s >= e) + return MY_CS_TOOSMALL; + if (wc < 128 && filename_safe_char[wc]) { *s= (uchar) wc; From fcfb218f71b7d371a10df020994fc0a618639327 Mon Sep 17 00:00:00 2001 From: unknown Date: Sun, 9 May 2010 21:30:06 +0200 Subject: [PATCH 2/2] Cherry-pick fix for Bug#53371, security hole with bypassing grants using special path in db/table names. Bump MariaDB version for security fix release. --- configure.in | 2 +- mysql-test/r/grant.result | 16 ++++++++++++++ mysql-test/t/grant.test | 25 ++++++++++++++++++++++ sql/mysql_priv.h | 2 +- sql/partition_info.cc | 4 ++-- sql/sql_parse.cc | 7 +++++- sql/sql_table.cc | 14 ++++++++++++ sql/sql_yacc.yy | 2 +- sql/table.cc | 22 ++++++++++++++++--- tests/mysql_client_test.c | 45 +++++++++++++++++++++++++++++++++++++++ 10 files changed, 130 insertions(+), 9 deletions(-) diff --git a/configure.in b/configure.in index 45952dbeceb..63ad6298191 100644 --- a/configure.in +++ b/configure.in @@ -7,7 +7,7 @@ AC_PREREQ(2.59) # Remember to also update version.c in ndb. # When changing major version number please also check switch statement # in mysqlbinlog::check_master_version(). -AC_INIT([MariaDB Server], [5.1.44a-MariaDB], [], [mysql]) +AC_INIT([MariaDB Server], [5.1.44b-MariaDB], [], [mysql]) AC_CONFIG_SRCDIR([sql/mysqld.cc]) AC_CANONICAL_SYSTEM # USTAR format gives us the possibility to store longer path names in diff --git a/mysql-test/r/grant.result b/mysql-test/r/grant.result index 8a3312da05e..b8f13723eb8 100644 --- a/mysql-test/r/grant.result +++ b/mysql-test/r/grant.result @@ -1413,3 +1413,19 @@ DROP USER 'user1'; DROP USER 'user1'@'localhost'; DROP USER 'user2'; DROP DATABASE db1; +CREATE DATABASE db1; +CREATE DATABASE db2; +GRANT SELECT ON db1.* to 'testbug'@localhost; +USE db2; +CREATE TABLE t1 (a INT); +USE test; +SELECT * FROM `../db2/tb2`; +ERROR 42S02: Table 'db1.../db2/tb2' doesn't exist +SELECT * FROM `../db2`.tb2; +ERROR 42000: SELECT command denied to user 'testbug'@'localhost' for table 'tb2' +SELECT * FROM `#mysql50#/../db2/tb2`; +ERROR 42S02: Table 'db1.#mysql50#/../db2/tb2' doesn't exist +DROP USER 'testbug'@localhost; +DROP TABLE db2.t1; +DROP DATABASE db1; +DROP DATABASE db2; diff --git a/mysql-test/t/grant.test b/mysql-test/t/grant.test index e89650c7aec..5bdb3ebe9bf 100644 --- a/mysql-test/t/grant.test +++ b/mysql-test/t/grant.test @@ -1525,5 +1525,30 @@ DROP USER 'user1'@'localhost'; DROP USER 'user2'; DROP DATABASE db1; + +# +# Bug #53371: COM_FIELD_LIST can be abused to bypass table level grants. +# + +CREATE DATABASE db1; +CREATE DATABASE db2; +GRANT SELECT ON db1.* to 'testbug'@localhost; +USE db2; +CREATE TABLE t1 (a INT); +USE test; +connect (con1,localhost,testbug,,db1); +--error ER_NO_SUCH_TABLE +SELECT * FROM `../db2/tb2`; +--error ER_TABLEACCESS_DENIED_ERROR +SELECT * FROM `../db2`.tb2; +--error ER_NO_SUCH_TABLE +SELECT * FROM `#mysql50#/../db2/tb2`; +connection default; +disconnect con1; +DROP USER 'testbug'@localhost; +DROP TABLE db2.t1; +DROP DATABASE db1; +DROP DATABASE db2; + # Wait till we reached the initial number of concurrent sessions --source include/wait_until_count_sessions.inc diff --git a/sql/mysql_priv.h b/sql/mysql_priv.h index 499d7d2fc24..0e032c857ce 100644 --- a/sql/mysql_priv.h +++ b/sql/mysql_priv.h @@ -2300,7 +2300,7 @@ void update_create_info_from_table(HA_CREATE_INFO *info, TABLE *form); int rename_file_ext(const char * from,const char * to,const char * ext); bool check_db_name(LEX_STRING *db); bool check_column_name(const char *name); -bool check_table_name(const char *name, uint length); +bool check_table_name(const char *name, uint length, bool check_for_path_chars); char *get_field(MEM_ROOT *mem, Field *field); bool get_field(MEM_ROOT *mem, Field *field, class String *res); int wild_case_compare(CHARSET_INFO *cs, const char *str,const char *wildstr); diff --git a/sql/partition_info.cc b/sql/partition_info.cc index e7d3e842903..0de8e478f03 100644 --- a/sql/partition_info.cc +++ b/sql/partition_info.cc @@ -972,7 +972,7 @@ bool partition_info::check_partition_info(THD *thd, handlerton **eng_type, part_elem->engine_type= default_engine_type; } if (check_table_name(part_elem->partition_name, - strlen(part_elem->partition_name))) + strlen(part_elem->partition_name), FALSE)) { my_error(ER_WRONG_PARTITION_NAME, MYF(0)); goto end; @@ -990,7 +990,7 @@ bool partition_info::check_partition_info(THD *thd, handlerton **eng_type, { sub_elem= sub_it++; if (check_table_name(sub_elem->partition_name, - strlen(sub_elem->partition_name))) + strlen(sub_elem->partition_name), FALSE)) { my_error(ER_WRONG_PARTITION_NAME, MYF(0)); goto end; diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index 2eb6a190e63..05ed9941441 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -1334,6 +1334,11 @@ bool dispatch_command(enum enum_server_command command, THD *thd, system_charset_info, packet, db_length, thd->charset(), &dummy_errors); db_buff[db_length]= '\0'; + if (check_table_name(db_buff, db_length, FALSE)) + { + my_error(ER_WRONG_TABLE_NAME, MYF(0), db_buff); + break; + } table_list.alias= table_list.table_name= db_buff; if (!(fields= (char *) thd->memdup(wildcard, query_length + 1))) break; @@ -6298,7 +6303,7 @@ TABLE_LIST *st_select_lex::add_table_to_list(THD *thd, DBUG_RETURN(0); // End of memory alias_str= alias ? alias->str : table->table.str; if (!test(table_options & TL_OPTION_ALIAS) && - check_table_name(table->table.str, table->table.length)) + check_table_name(table->table.str, table->table.length, FALSE)) { my_error(ER_WRONG_TABLE_NAME, MYF(0), table->table.str); DBUG_RETURN(0); diff --git a/sql/sql_table.cc b/sql/sql_table.cc index 6d20ea0d3e5..d7376318f6b 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -434,7 +434,21 @@ uint tablename_to_filename(const char *from, char *to, uint to_length) DBUG_PRINT("enter", ("from '%s'", from)); if ((length= check_n_cut_mysql50_prefix(from, to, to_length))) + { + /* + Check if the name supplied is a valid mysql 5.0 name and + make the name a zero length string if it's not. + Note that just returning zero length is not enough : + a lot of places don't check the return value and expect + a zero terminated string. + */ + if (check_table_name(to, length, TRUE)) + { + to[0]= 0; + length= 0; + } DBUG_RETURN(length); + } length= strconvert(system_charset_info, from, &my_charset_filename, to, to_length, &errors); if (check_if_legal_tablename(to) && diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy index e0f5cd9a562..a1f5547e103 100644 --- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy @@ -6149,7 +6149,7 @@ alter_list_item: { MYSQL_YYABORT; } - if (check_table_name($3->table.str,$3->table.length) || + if (check_table_name($3->table.str,$3->table.length, FALSE) || ($3->db.str && check_db_name(&$3->db))) { my_error(ER_WRONG_TABLE_NAME, MYF(0), $3->table.str); diff --git a/sql/table.cc b/sql/table.cc index 733aa3e6887..8c87dc73d7d 100644 --- a/sql/table.cc +++ b/sql/table.cc @@ -494,6 +494,19 @@ inline bool is_system_table_name(const char *name, uint length) } +/** + Check if a string contains path elements +*/ + +static inline bool has_disabled_path_chars(const char *str) +{ + for (; *str; str++) + if (*str == FN_EXTCHAR || *str == '/' || *str == '\\' || *str == '~' || *str == '@') + return TRUE; + return FALSE; +} + + /* Read table definition from a binary / text based .frm file @@ -548,7 +561,8 @@ int open_table_def(THD *thd, TABLE_SHARE *share, uint db_flags) This kind of tables must have been opened only by the my_open() above. */ - if (strchr(share->table_name.str, '@') || + if (has_disabled_path_chars(share->table_name.str) || + has_disabled_path_chars(share->db.str) || !strncmp(share->db.str, MYSQL50_TABLE_NAME_PREFIX, MYSQL50_TABLE_NAME_PREFIX_LENGTH) || !strncmp(share->table_name.str, MYSQL50_TABLE_NAME_PREFIX, @@ -2718,7 +2732,6 @@ bool check_db_name(LEX_STRING *org_name) (name_length > NAME_CHAR_LEN)); /* purecov: inspected */ } - /* Allow anything as a table name, as long as it doesn't contain an ' ' at the end @@ -2726,7 +2739,7 @@ bool check_db_name(LEX_STRING *org_name) */ -bool check_table_name(const char *name, uint length) +bool check_table_name(const char *name, uint length, bool check_for_path_chars) { uint name_length= 0; // name length in symbols const char *end= name+length; @@ -2753,6 +2766,9 @@ bool check_table_name(const char *name, uint length) continue; } } + if (check_for_path_chars && + (*name == '/' || *name == '\\' || *name == '~' || *name == FN_EXTCHAR)) + return 1; #endif name++; name_length++; diff --git a/tests/mysql_client_test.c b/tests/mysql_client_test.c index 7f5bcaff083..c5edc747085 100644 --- a/tests/mysql_client_test.c +++ b/tests/mysql_client_test.c @@ -18092,6 +18092,50 @@ static void test_bug44495() DBUG_VOID_RETURN; } +static void test_bug53371() +{ + int rc; + MYSQL_RES *result; + + myheader("test_bug53371"); + + rc= mysql_query(mysql, "DROP TABLE IF EXISTS t1"); + myquery(rc); + rc= mysql_query(mysql, "DROP DATABASE IF EXISTS bug53371"); + myquery(rc); + rc= mysql_query(mysql, "DROP USER 'testbug'@localhost"); + + rc= mysql_query(mysql, "CREATE TABLE t1 (a INT)"); + myquery(rc); + rc= mysql_query(mysql, "CREATE DATABASE bug53371"); + myquery(rc); + rc= mysql_query(mysql, "GRANT SELECT ON bug53371.* to 'testbug'@localhost"); + myquery(rc); + + rc= mysql_change_user(mysql, "testbug", NULL, "bug53371"); + myquery(rc); + + rc= mysql_query(mysql, "SHOW COLUMNS FROM client_test_db.t1"); + DIE_UNLESS(rc); + DIE_UNLESS(mysql_errno(mysql) == 1142); + + result= mysql_list_fields(mysql, "../client_test_db/t1", NULL); + DIE_IF(result); + + result= mysql_list_fields(mysql, "#mysql50#/../client_test_db/t1", NULL); + DIE_IF(result); + + rc= mysql_change_user(mysql, opt_user, opt_password, current_db); + myquery(rc); + rc= mysql_query(mysql, "DROP TABLE t1"); + myquery(rc); + rc= mysql_query(mysql, "DROP DATABASE bug53371"); + myquery(rc); + rc= mysql_query(mysql, "DROP USER 'testbug'@localhost"); + myquery(rc); +} + + /* Read and parse arguments and MySQL options from my.cnf */ @@ -18401,6 +18445,7 @@ static struct my_tests_st my_tests[]= { { "test_bug30472", test_bug30472 }, { "test_bug20023", test_bug20023 }, { "test_bug45010", test_bug45010 }, + { "test_bug53371", test_bug53371 }, { "test_bug31418", test_bug31418 }, { "test_bug31669", test_bug31669 }, { "test_bug28386", test_bug28386 },