Commit 0f26a053 authored by Georgi Kodinov's avatar Georgi Kodinov

Bug #53371: COM_FIELD_LIST can be abused to bypass table level grants.

This is the 5.1 merge and extension of the fix.
The server was happily accepting paths in table name in all places a table
name is accepted (e.g. a SELECT). This allowed all users that have some 
privilege over some database to read all tables in all databases in all
mysql server instances that the server file system has access to.
Fixed by :
1. making sure no path elements are allowed in quoted table name when
constructing the path (note that the path symbols are still valid in table names
when they're properly escaped by the server).
2. checking the #mysql50# prefixed names the same way they're checked for
path elements in mysql-5.0.
parents 83fb8a77 6ae9c701
...@@ -1413,3 +1413,19 @@ DROP USER 'user1'; ...@@ -1413,3 +1413,19 @@ DROP USER 'user1';
DROP USER 'user1'@'localhost'; DROP USER 'user1'@'localhost';
DROP USER 'user2'; DROP USER 'user2';
DROP DATABASE db1; 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;
...@@ -1525,5 +1525,30 @@ DROP USER 'user1'@'localhost'; ...@@ -1525,5 +1525,30 @@ DROP USER 'user1'@'localhost';
DROP USER 'user2'; DROP USER 'user2';
DROP DATABASE db1; 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 # Wait till we reached the initial number of concurrent sessions
--source include/wait_until_count_sessions.inc --source include/wait_until_count_sessions.inc
...@@ -2269,7 +2269,7 @@ void update_create_info_from_table(HA_CREATE_INFO *info, TABLE *form); ...@@ -2269,7 +2269,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); int rename_file_ext(const char * from,const char * to,const char * ext);
bool check_db_name(LEX_STRING *db); bool check_db_name(LEX_STRING *db);
bool check_column_name(const char *name); 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); char *get_field(MEM_ROOT *mem, Field *field);
bool get_field(MEM_ROOT *mem, Field *field, class String *res); bool get_field(MEM_ROOT *mem, Field *field, class String *res);
int wild_case_compare(CHARSET_INFO *cs, const char *str,const char *wildstr); int wild_case_compare(CHARSET_INFO *cs, const char *str,const char *wildstr);
......
...@@ -972,7 +972,7 @@ bool partition_info::check_partition_info(THD *thd, handlerton **eng_type, ...@@ -972,7 +972,7 @@ bool partition_info::check_partition_info(THD *thd, handlerton **eng_type,
part_elem->engine_type= default_engine_type; part_elem->engine_type= default_engine_type;
} }
if (check_table_name(part_elem->partition_name, 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)); my_error(ER_WRONG_PARTITION_NAME, MYF(0));
goto end; goto end;
...@@ -990,7 +990,7 @@ bool partition_info::check_partition_info(THD *thd, handlerton **eng_type, ...@@ -990,7 +990,7 @@ bool partition_info::check_partition_info(THD *thd, handlerton **eng_type,
{ {
sub_elem= sub_it++; sub_elem= sub_it++;
if (check_table_name(sub_elem->partition_name, 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)); my_error(ER_WRONG_PARTITION_NAME, MYF(0));
goto end; goto end;
......
...@@ -1310,6 +1310,13 @@ bool dispatch_command(enum enum_server_command command, THD *thd, ...@@ -1310,6 +1310,13 @@ bool dispatch_command(enum enum_server_command command, THD *thd,
} }
thd->convert_string(&conv_name, system_charset_info, thd->convert_string(&conv_name, system_charset_info,
packet, arg_length, thd->charset()); packet, arg_length, thd->charset());
if (check_table_name(conv_name.str, conv_name.length, FALSE))
{
/* this is OK due to convert_string() null-terminating the string */
my_error(ER_WRONG_TABLE_NAME, MYF(0), conv_name.str);
break;
}
table_list.alias= table_list.table_name= conv_name.str; table_list.alias= table_list.table_name= conv_name.str;
packet= arg_end + 1; packet= arg_end + 1;
...@@ -6233,7 +6240,7 @@ TABLE_LIST *st_select_lex::add_table_to_list(THD *thd, ...@@ -6233,7 +6240,7 @@ TABLE_LIST *st_select_lex::add_table_to_list(THD *thd,
DBUG_RETURN(0); // End of memory DBUG_RETURN(0); // End of memory
alias_str= alias ? alias->str : table->table.str; alias_str= alias ? alias->str : table->table.str;
if (!test(table_options & TL_OPTION_ALIAS) && 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); my_error(ER_WRONG_TABLE_NAME, MYF(0), table->table.str);
DBUG_RETURN(0); DBUG_RETURN(0);
......
...@@ -435,7 +435,21 @@ uint tablename_to_filename(const char *from, char *to, uint to_length) ...@@ -435,7 +435,21 @@ uint tablename_to_filename(const char *from, char *to, uint to_length)
DBUG_PRINT("enter", ("from '%s'", from)); DBUG_PRINT("enter", ("from '%s'", from));
if ((length= check_n_cut_mysql50_prefix(from, to, to_length))) 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); DBUG_RETURN(length);
}
length= strconvert(system_charset_info, from, length= strconvert(system_charset_info, from,
&my_charset_filename, to, to_length, &errors); &my_charset_filename, to, to_length, &errors);
if (check_if_legal_tablename(to) && if (check_if_legal_tablename(to) &&
......
...@@ -6133,7 +6133,7 @@ alter_list_item: ...@@ -6133,7 +6133,7 @@ alter_list_item:
{ {
MYSQL_YYABORT; 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))) ($3->db.str && check_db_name(&$3->db)))
{ {
my_error(ER_WRONG_TABLE_NAME, MYF(0), $3->table.str); my_error(ER_WRONG_TABLE_NAME, MYF(0), $3->table.str);
......
...@@ -494,6 +494,26 @@ inline bool is_system_table_name(const char *name, uint length) ...@@ -494,6 +494,26 @@ 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++)
switch (*str)
{
case FN_EXTCHAR:
case '/':
case '\\':
case '~':
case '@':
return TRUE;
}
return FALSE;
}
/* /*
Read table definition from a binary / text based .frm file Read table definition from a binary / text based .frm file
...@@ -549,7 +569,8 @@ int open_table_def(THD *thd, TABLE_SHARE *share, uint db_flags) ...@@ -549,7 +569,8 @@ int open_table_def(THD *thd, TABLE_SHARE *share, uint db_flags)
This kind of tables must have been opened only by the This kind of tables must have been opened only by the
my_open() above. 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, !strncmp(share->db.str, MYSQL50_TABLE_NAME_PREFIX,
MYSQL50_TABLE_NAME_PREFIX_LENGTH) || MYSQL50_TABLE_NAME_PREFIX_LENGTH) ||
!strncmp(share->table_name.str, MYSQL50_TABLE_NAME_PREFIX, !strncmp(share->table_name.str, MYSQL50_TABLE_NAME_PREFIX,
...@@ -2711,7 +2732,6 @@ bool check_db_name(LEX_STRING *org_name) ...@@ -2711,7 +2732,6 @@ bool check_db_name(LEX_STRING *org_name)
(name_length > NAME_CHAR_LEN)); /* purecov: inspected */ (name_length > NAME_CHAR_LEN)); /* purecov: inspected */
} }
/* /*
Allow anything as a table name, as long as it doesn't contain an Allow anything as a table name, as long as it doesn't contain an
' ' at the end ' ' at the end
...@@ -2719,7 +2739,7 @@ bool check_db_name(LEX_STRING *org_name) ...@@ -2719,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 uint name_length= 0; // name length in symbols
const char *end= name+length; const char *end= name+length;
...@@ -2746,6 +2766,9 @@ bool check_table_name(const char *name, uint length) ...@@ -2746,6 +2766,9 @@ bool check_table_name(const char *name, uint length)
continue; continue;
} }
} }
if (check_for_path_chars &&
(*name == '/' || *name == '\\' || *name == '~' || *name == FN_EXTCHAR))
return 1;
#endif #endif
name++; name++;
name_length++; name_length++;
......
...@@ -18049,6 +18049,50 @@ static void test_bug44495() ...@@ -18049,6 +18049,50 @@ static void test_bug44495()
DBUG_VOID_RETURN; 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 Read and parse arguments and MySQL options from my.cnf
*/ */
...@@ -18358,6 +18402,7 @@ static struct my_tests_st my_tests[]= { ...@@ -18358,6 +18402,7 @@ static struct my_tests_st my_tests[]= {
{ "test_bug30472", test_bug30472 }, { "test_bug30472", test_bug30472 },
{ "test_bug20023", test_bug20023 }, { "test_bug20023", test_bug20023 },
{ "test_bug45010", test_bug45010 }, { "test_bug45010", test_bug45010 },
{ "test_bug53371", test_bug53371 },
{ "test_bug31418", test_bug31418 }, { "test_bug31418", test_bug31418 },
{ "test_bug31669", test_bug31669 }, { "test_bug31669", test_bug31669 },
{ "test_bug28386", test_bug28386 }, { "test_bug28386", test_bug28386 },
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment