From 5dd5d70506611ea68c7103fcf97512e3114fffae Mon Sep 17 00:00:00 2001 From: Kristofer Pettersson Date: Mon, 3 May 2010 18:14:39 +0200 Subject: [PATCH] Bug#50373 --secure-file-priv="" Iterative patch improvement. Previously committed patch caused wrong result on Windows. The previous patch also broke secure_file_priv for symlinks since not all file paths which must be compared against this variable are normalized using the same norm. The server variable opt_secure_file_priv wasn't normalized properly and caused the operations LOAD DATA INFILE .. INTO TABLE .. and SELECT load_file(..) to do different interpretations of the --secure-file-priv option. The patch moves code to the server initialization routines so that the path always is normalized once and only once. It was also intended that setting the option to an empty string should be equal to lifting all previously set restrictions. This is also fixed by this patch. mysql-test/r/loaddata.result: * Removed test code which will currently break the much used --mem feature of mtr. mysql-test/t/loaddata.test: * Removed test code which will currently break the much used --mem feature of mtr. sql/item_strfunc.cc: * Replaced string comparing code on opt_secure_file_priv with an interface which guarantees that both file paths are normalized using the same norm on all platforms. sql/mysql_priv.h: * Added signature for is_secure_file_path() sql/mysqld.cc: * New function for checking if a path compatible with the secure path restriction. * Added initialization of the opt_secure_file_priv variable. sql/sql_class.cc: * Replaced string comparing code on opt_secure_file_priv with an interface which guarantees that both file paths are normalized using the same norm on all platforms. sql/sql_load.cc: * Replaced string comparing code on opt_secure_file_priv with an interface which guarantees that both file paths are normalized using the same norm on all platforms. --- mysql-test/r/loaddata.result | 6 ----- mysql-test/t/loaddata.test | 14 +++++++---- sql/item_strfunc.cc | 3 +-- sql/mysql_priv.h | 6 +++++ sql/mysqld.cc | 46 ++++++++++++++++++++++++++++++++---- sql/sql_class.cc | 3 +-- sql/sql_load.cc | 11 ++++----- 7 files changed, 64 insertions(+), 25 deletions(-) diff --git a/mysql-test/r/loaddata.result b/mysql-test/r/loaddata.result index 8246cb40538..b3487d376a1 100644 --- a/mysql-test/r/loaddata.result +++ b/mysql-test/r/loaddata.result @@ -202,12 +202,6 @@ select * from t1; a b c 10 NULL Ten 15 NULL Fifteen -show variables like "secure_file_pri%"; -Variable_name Value -secure_file_priv MYSQLTEST_VARDIR -select @@secure_file_priv; -@@secure_file_priv -MYSQLTEST_VARDIR set @@secure_file_priv= 0; ERROR HY000: Variable 'secure_file_priv' is a read only variable truncate table t1; diff --git a/mysql-test/t/loaddata.test b/mysql-test/t/loaddata.test index e5f0a1d7eba..126bd5c8838 100644 --- a/mysql-test/t/loaddata.test +++ b/mysql-test/t/loaddata.test @@ -153,10 +153,16 @@ select * from t1; # # It should not be possible to load from a file outside of vardir ---replace_result $MYSQLTEST_VARDIR MYSQLTEST_VARDIR -show variables like "secure_file_pri%"; ---replace_result $MYSQLTEST_VARDIR MYSQLTEST_VARDIR -select @@secure_file_priv; +## The following lines were disabled because of patch for +## bug 50373. MYSQLTEST_VARDIR doesn't rewrite symlinks +## to real paths, but this is done for secure_file_priv. +## Because of this the result can't be replaced if the +## test suite runs with the --mem option which creates +## symlinks to the ramdisk. +#--replace_result $MYSQLTEST_VARDIR MYSQLTEST_VARDIR +#show variables like "secure_file_pri%"; +#--replace_result $MYSQLTEST_VARDIR MYSQLTEST_VARDIR +#select @@secure_file_priv; --error 1238 set @@secure_file_priv= 0; diff --git a/sql/item_strfunc.cc b/sql/item_strfunc.cc index b53172d631a..5d56b0a621a 100644 --- a/sql/item_strfunc.cc +++ b/sql/item_strfunc.cc @@ -2959,8 +2959,7 @@ String *Item_load_file::val_str(String *str) MY_RELATIVE_PATH | MY_UNPACK_FILENAME); /* Read only allowed from within dir specified by secure_file_priv */ - if (opt_secure_file_priv && - strncmp(opt_secure_file_priv, path, strlen(opt_secure_file_priv))) + if (!is_secure_file_path(path)) goto err; if (!my_stat(path, &stat_info, MYF(0))) diff --git a/sql/mysql_priv.h b/sql/mysql_priv.h index 56175d069c5..f410f6dbcc2 100644 --- a/sql/mysql_priv.h +++ b/sql/mysql_priv.h @@ -1832,6 +1832,12 @@ void sql_perror(const char *message); bool fn_format_relative_to_data_home(char * to, const char *name, const char *dir, const char *extension); +/** + Test a file path to determine if the path is compatible with the secure file + path restriction. +*/ +bool is_secure_file_path(char *path); + #ifdef MYSQL_SERVER File open_binlog(IO_CACHE *log, const char *log_file_name, const char **errmsg); diff --git a/sql/mysqld.cc b/sql/mysqld.cc index 0d62c5953fc..24614737a59 100644 --- a/sql/mysqld.cc +++ b/sql/mysqld.cc @@ -8776,6 +8776,45 @@ fn_format_relative_to_data_home(char * to, const char *name, } +/** + Test a file path to determine if the path is compatible with the secure file + path restriction. + + @param path null terminated character string + + @return + @retval TRUE The path is secure + @retval FALSE The path isn't secure +*/ + +bool is_secure_file_path(char *path) +{ + char buff1[FN_REFLEN], buff2[FN_REFLEN]; + /* + All paths are secure if opt_secure_file_path is 0 + */ + if (!opt_secure_file_priv) + return TRUE; + + if (my_realpath(buff1, path, 0)) + { + /* + The supplied file path might have been a file and not a directory. + */ + int length= (int)dirname_length(path); + if (length >= FN_REFLEN) + return FALSE; + memcpy(buff2, path, length); + buff2[length]= '\0'; + if (length == 0 || my_realpath(buff1, buff2, 0)) + return FALSE; + } + convert_dirname(buff2, buff1, NullS); + if (strncmp(opt_secure_file_priv, buff2, strlen(opt_secure_file_priv))) + return FALSE; + return TRUE; +} + static int fix_paths(void) { char buff[FN_REFLEN],*pos; @@ -8843,14 +8882,13 @@ static int fix_paths(void) } else { - convert_dirname(buff, opt_secure_file_priv, NullS); - char *secure_file_real_path= (char *)my_malloc(FN_REFLEN, MYF(MY_FAE)); - if (secure_file_real_path == 0 || - my_realpath(secure_file_real_path, buff, 0)) + if (my_realpath(buff, opt_secure_file_priv, 0)) { sql_print_warning("Failed to normalize the argument for --secure-file-priv."); return 1; } + char *secure_file_real_path= (char *)my_malloc(FN_REFLEN, MYF(MY_FAE)); + convert_dirname(secure_file_real_path, buff, NullS); my_free(opt_secure_file_priv, MYF(0)); opt_secure_file_priv= secure_file_real_path; } diff --git a/sql/sql_class.cc b/sql/sql_class.cc index bf5af7141c0..2633f03f2d6 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -1831,8 +1831,7 @@ static File create_file(THD *thd, char *path, sql_exchange *exchange, else (void) fn_format(path, exchange->file_name, mysql_real_data_home, "", option); - if (opt_secure_file_priv && - strncmp(opt_secure_file_priv, path, strlen(opt_secure_file_priv))) + if (!is_secure_file_path(path)) { /* Write only allowed to dir or subdir specified by secure_file_priv */ my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0), "--secure-file-priv"); diff --git a/sql/sql_load.cc b/sql/sql_load.cc index 71cf118f577..e121f69dfdf 100644 --- a/sql/sql_load.cc +++ b/sql/sql_load.cc @@ -348,14 +348,11 @@ int mysql_load(THD *thd,sql_exchange *ex,TABLE_LIST *table_list, DBUG_ASSERT(FALSE); #endif } - else if (opt_secure_file_priv) + else if (!is_secure_file_path(name)) { - if (strncmp(opt_secure_file_priv, name, strlen(opt_secure_file_priv))) - { - /* Read only allowed from within dir specified by secure_file_priv */ - my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0), "--secure-file-priv"); - DBUG_RETURN(TRUE); - } + /* Read only allowed from within dir specified by secure_file_priv */ + my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0), "--secure-file-priv"); + DBUG_RETURN(TRUE); } }