Skip to content

Commit

Permalink
Crude "auto-load-data-local-infile" mode
Browse files Browse the repository at this point in the history
Disable LOAD DATA LOCAL INFILE suport by default and
auto-enable it for the duration of one query, if the query
string starts with the word "load". In all other cases the application
should enable LOAD DATA LOCAL INFILE support explicitly.
  • Loading branch information
vuvova committed Jan 27, 2019
1 parent 21f9037 commit 2175bfc
Show file tree
Hide file tree
Showing 9 changed files with 90 additions and 11 deletions.
10 changes: 8 additions & 2 deletions CMakeLists.txt
Expand Up @@ -256,9 +256,15 @@ IF(HAVE_GGDB3)
SET(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -ggdb3")
ENDIF()

OPTION(ENABLED_LOCAL_INFILE
"If we should should enable LOAD DATA LOCAL by default" ${IF_WIN})
SET(ENABLED_LOCAL_INFILE "AUTO" CACHE STRING "If we should should enable LOAD DATA LOCAL by default (OFF/ON/AUTO)")
MARK_AS_ADVANCED(ENABLED_LOCAL_INFILE)
IF (ENABLED_LOCAL_INFILE MATCHES "^(0|FALSE)$")
SET(ENABLED_LOCAL_INFILE OFF)
ELSEIF(ENABLED_LOCAL_INFILE MATCHES "^(1|TRUE)$")
SET(ENABLED_LOCAL_INFILE ON)
ELSEIF (NOT ENABLED_LOCAL_INFILE MATCHES "^(ON|OFF|AUTO)$")
MESSAGE(FATAL_ERROR "ENABLED_LOCAL_INFILE must be one of OFF, ON, AUTO")
ENDIF()

OPTION(WITH_FAST_MUTEXES "Compile with fast mutexes" OFF)
MARK_AS_ADVANCED(WITH_FAST_MUTEXES)
Expand Down
2 changes: 0 additions & 2 deletions client/mysqltest.cc
Expand Up @@ -6045,7 +6045,6 @@ void do_connect(struct st_command *command)
#endif
if (opt_compress || con_compress)
mysql_options(con_slot->mysql, MYSQL_OPT_COMPRESS, NullS);
mysql_options(con_slot->mysql, MYSQL_OPT_LOCAL_INFILE, 0);
mysql_options(con_slot->mysql, MYSQL_SET_CHARSET_NAME,
charset_info->csname);
if (opt_charsets_dir)
Expand Down Expand Up @@ -9110,7 +9109,6 @@ int main(int argc, char **argv)
(void *) &opt_connect_timeout);
if (opt_compress)
mysql_options(con->mysql,MYSQL_OPT_COMPRESS,NullS);
mysql_options(con->mysql, MYSQL_OPT_LOCAL_INFILE, 0);
mysql_options(con->mysql, MYSQL_SET_CHARSET_NAME,
charset_info->csname);
if (opt_charsets_dir)
Expand Down
1 change: 0 additions & 1 deletion cmake/build_configurations/mysql_release.cmake
Expand Up @@ -94,7 +94,6 @@ IF(FEATURE_SET)
ENDFOREACH()
ENDIF()

OPTION(ENABLED_LOCAL_INFILE "" ON)
IF(RPM)
SET(WITH_SSL system CACHE STRING "")
SET(WITH_ZLIB system CACHE STRING "")
Expand Down
6 changes: 5 additions & 1 deletion config.h.cmake
Expand Up @@ -530,7 +530,11 @@
/*
MySQL features
*/
#cmakedefine ENABLED_LOCAL_INFILE 1
#define LOCAL_INFILE_MODE_OFF 0
#define LOCAL_INFILE_MODE_ON 1
#define LOCAL_INFILE_MODE_AUTO 2
#define ENABLED_LOCAL_INFILE LOCAL_INFILE_MODE_@ENABLED_LOCAL_INFILE@

#cmakedefine ENABLED_PROFILING 1
#cmakedefine EXTRA_DEBUG 1
#cmakedefine BACKUP_TEST 1
Expand Down
2 changes: 1 addition & 1 deletion include/mysql.h
Expand Up @@ -274,7 +274,7 @@ typedef struct st_mysql

/* session-wide random string */
char scramble[SCRAMBLE_LENGTH+1];
my_bool unused1;
my_bool auto_local_infile;
void *unused2, *unused3, *unused4, *unused5;

LIST *stmts; /* list of all statements */
Expand Down
2 changes: 1 addition & 1 deletion include/mysql.h.pp
Expand Up @@ -341,7 +341,7 @@
my_bool free_me;
my_bool reconnect;
char scramble[20 +1];
my_bool unused1;
my_bool auto_local_infile;
void *unused2, *unused3, *unused4, *unused5;
LIST *stmts;
const struct st_mysql_methods *methods;
Expand Down
26 changes: 26 additions & 0 deletions mysql-test/r/mysql.result
Expand Up @@ -587,3 +587,29 @@ a
2
drop table "a1\""b1";
set sql_mode=default;
create table t1 (a text);
select count(*) from t1;
count(*)
41
truncate table t1;
select count(*) from t1;
count(*)
41
truncate table t1;
select count(*) from t1;
count(*)
0
truncate table t1;
select count(*) from t1;
count(*)
0
truncate table t1;
select count(*) from t1;
count(*)
41
truncate table t1;
select count(*) from t1;
count(*)
0
truncate table t1;
drop table t1;
22 changes: 22 additions & 0 deletions mysql-test/t/mysql.test
Expand Up @@ -656,3 +656,25 @@ show create table "a1\""b1";
select * from "a1\""b1";
drop table "a1\""b1";
set sql_mode=default;

#
# mysql --local-infile
#
--let $ldli = load data local infile '$MYSQLTEST_VARDIR/tmp/bug.sql' into table test.t1;
create table t1 (a text);
--exec $MYSQL -e "$ldli"
select count(*) from t1; truncate table t1;
--exec $MYSQL --enable-local-infile -e "$ldli"
select count(*) from t1; truncate table t1;
--error 1
--exec $MYSQL --disable-local-infile -e "$ldli"
select count(*) from t1; truncate table t1;
--error 1
--exec $MYSQL -e "/*q*/$ldli"
select count(*) from t1; truncate table t1;
--exec $MYSQL --enable-local-infile -e "/*q*/$ldli"
select count(*) from t1; truncate table t1;
--error 1
--exec $MYSQL --disable-local-infile -e "/*q*/$ldli"
select count(*) from t1; truncate table t1;
drop table t1;
30 changes: 27 additions & 3 deletions sql-common/client.c
Expand Up @@ -115,6 +115,12 @@ my_bool net_flush(NET *net);
#include <my_context.h>
#include <mysql_async.h>

typedef enum {
ALWAYS_ACCEPT, /* heuristics is disabled, use CLIENT_LOCAL_FILES */
WAIT_FOR_QUERY, /* heuristics is enabled, not sending files */
ACCEPT_FILE_REQUEST /* heuristics is enabled, ready to send a file */
} auto_local_infile_state;

#define native_password_plugin_name "mysql_native_password"
#define old_password_plugin_name "mysql_old_password"

Expand Down Expand Up @@ -1765,8 +1771,10 @@ mysql_init(MYSQL *mysql)
--enable-local-infile
*/

#if defined(ENABLED_LOCAL_INFILE) && !defined(MYSQL_SERVER)
#if ENABLED_LOCAL_INFILE && !defined(MYSQL_SERVER)
mysql->options.client_flag|= CLIENT_LOCAL_FILES;
mysql->auto_local_infile= ENABLED_LOCAL_INFILE == LOCAL_INFILE_MODE_AUTO
? WAIT_FOR_QUERY : ALWAYS_ACCEPT;
#endif

#ifdef HAVE_SMEM
Expand Down Expand Up @@ -3951,8 +3959,14 @@ static my_bool cli_read_query_result(MYSQL *mysql)
ulong field_count;
MYSQL_DATA *fields;
ulong length;
#ifdef MYSQL_CLIENT
my_bool can_local_infile= mysql->auto_local_infile != WAIT_FOR_QUERY;
#endif
DBUG_ENTER("cli_read_query_result");

if (mysql->auto_local_infile == ACCEPT_FILE_REQUEST)
mysql->auto_local_infile= WAIT_FOR_QUERY;

if ((length = cli_safe_read(mysql)) == packet_error)
DBUG_RETURN(1);
free_old_query(mysql); /* Free old result */
Expand Down Expand Up @@ -3989,7 +4003,8 @@ static my_bool cli_read_query_result(MYSQL *mysql)
{
int error;

if (!(mysql->options.client_flag & CLIENT_LOCAL_FILES))
if (!(mysql->options.client_flag & CLIENT_LOCAL_FILES) ||
!can_local_infile)
{
set_mysql_error(mysql, CR_MALFORMED_PACKET, unknown_sqlstate);
DBUG_RETURN(1);
Expand Down Expand Up @@ -4027,6 +4042,13 @@ int STDCALL
mysql_send_query(MYSQL* mysql, const char* query, ulong length)
{
DBUG_ENTER("mysql_send_query");
if (mysql->options.client_flag & CLIENT_LOCAL_FILES &&
mysql->auto_local_infile == WAIT_FOR_QUERY &&
(*query == 'l' || *query == 'L'))
{
if (strncasecmp(query, STRING_WITH_LEN("load")) == 0)
mysql->auto_local_infile= ACCEPT_FILE_REQUEST;
}
DBUG_RETURN(simple_command(mysql, COM_QUERY, (uchar*) query, length, 1));
}

Expand Down Expand Up @@ -4241,10 +4263,12 @@ mysql_options(MYSQL *mysql,enum mysql_option option, const void *arg)
mysql->options.protocol=MYSQL_PROTOCOL_PIPE; /* Force named pipe */
break;
case MYSQL_OPT_LOCAL_INFILE: /* Allow LOAD DATA LOCAL ?*/
if (!arg || test(*(uint*) arg))
if (!arg || *(uint*) arg)
mysql->options.client_flag|= CLIENT_LOCAL_FILES;
else
mysql->options.client_flag&= ~CLIENT_LOCAL_FILES;
mysql->auto_local_infile= arg && *(uint*)arg == LOCAL_INFILE_MODE_AUTO
? WAIT_FOR_QUERY : ALWAYS_ACCEPT;
break;
case MYSQL_INIT_COMMAND:
add_init_command(&mysql->options,arg);
Expand Down

3 comments on commit 2175bfc

@orlitzky
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this still allow a malicious server to request the wrong (or simply, an additional) file in response to a "load" query?

@vuvova
Copy link
Member Author

@vuvova vuvova commented on 2175bfc Jan 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, it does allow to request a wrong file. This is not a bullet-proof solution, but a compromise between doing nothing and disabling CLIENT_LOCAL_FILES by default (and breaking many thousands of installations that use it). This fix is almost like disabling by default while not breaking almost any existing application. At least it didn't break any our LOAD DATA LOCAL tests.

A bullet-proof solution would mean an SQL parser in the client library. It needs to parse comments correctly (e.g. /*foo*/load/*bar*/data and even /*!50607 load data*/), it needs to handle all possible string escapings and encodings to be able to extract the file name correctly. So we consider it an overkill, a performance price that users should not pay, taking into account that almost all statements are not LOAD DATA LOCAL.

Security-conscious applications can still disable CLIENT_LOCAL_FILES as before. And if an application sends queries that start from a comment (like /*foo*/load) it can fully enable CLIENT_LOCAL_FILES as before.

By the way, no, a malicious server cannot request an additional file. After this fix a client library will accept only one file request, and only directly after "load..." query.

@orlitzky
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thank you for the detailed response.

Please sign in to comment.