Details
-
Bug
-
Status: Closed (View Workflow)
-
Critical
-
Resolution: Fixed
-
None
-
None
Description
The following test case reproduces the bug:
$ cat /tmp/test_load_file
|
100
|
200
|
300
|
|
|
$ cat mysql-test/main/ps_load.test
|
CREATE TABLE t1 (a INT);
|
LOAD DATA LOCAL INFILE '/tmp/test_load_file' INTO TABLE t1;
|
SELECT * FROM t1;
|
DROP TABLE t1;
|
If this test case is run by mtr with the --ps-protocol option it fails
main.ps_load [ fail ]
|
Test ended at 2021-02-07 22:18:19
|
|
|
CURRENT_TEST: main.ps_load
|
mysqltest: At line 2: query 'LOAD DATA LOCAL INFILE '/tmp/test_load_file' INTO TABLE t1' failed: 2000: Load data local infile forbidden
|
The above listed error message is produced by libmariadb, specifically by the function mysql_handle_local_infile() that is called by mthd_my_read_query_result. Calling trace to invocation point is below:
main()
|
run_query()
|
run_query_stmt()
|
wrap_mysql_stmt_execute()
|
mysql_stmt_execute()
|
stmt_read_execute_response()
|
mthd_my_read_query_result()
|
mysql_handle_local_infile()
|
At the place of calling the function my_set_error(conn, CR_UNKNOWN_ERROR, SQLSTATE_UNKNOWN, "Load data local infile forbidden") in the function mysql_handle_local_infile(), the data member conn->options.client_flag == CLIENT_LOCAL_FILES and the parameter can_local_file has the value false.
One level higher in the call stack the data member mysql->extension->auto_local_infile
has the value WAIT_FOR_QUERY, the expression
my_bool can_local_infile= (mysql->options.extension) &&(mysql->extension->auto_local_infile != WAIT_FOR_QUERY);
|
has the value false, as a consequences the argument value for the parameter can_local_file of the function mysql_handle_local_infile() also has the value false.
In case the original test case is run by mtr WITHOUT the option --ps-protocol, the parameter can_local_file of the function mysql_handle_local_infile() has the value true since the data member mysql->extension->auto_local_infile has the value ACCEPT_FILE_REQUEST.
The value ACCEPT_FILE_REQUEST is assigned to the the data member mysql->extension->auto_local_infile inside the function ma_simple_command:
int
|
ma_simple_command(MYSQL *mysql,enum enum_server_command command, const char *arg, |
size_t length, my_bool skipp_check, void *opt_arg) |
{
|
if ((mysql->options.client_flag & CLIENT_LOCAL_FILES) && |
mysql->options.extension && mysql->extension->auto_local_infile == WAIT_FOR_QUERY &&
|
arg && (*arg == 'l' || *arg == 'L') && |
command == COM_QUERY)
|
{
|
if (strncasecmp(arg, "load", 4) == 0) |
mysql->extension->auto_local_infile= ACCEPT_FILE_REQUEST; <<===== This is that place
|
}
|
return mysql->methods->db_command(mysql, command, arg, length, skipp_check, opt_arg); |
}
|
If the following modification be applied to the source code of the library libmariadb
--- a/libmariadb/mariadb_stmt.c
|
+++ b/libmariadb/mariadb_stmt.c
|
@@ -1713,6 +1713,17 @@ int STDCALL mysql_stmt_prepare(MYSQL_STMT *stmt, const char *query, unsigned lon
|
}
|
memset(stmt->bind, 0, sizeof(MYSQL_BIND) * stmt->field_count);
|
}
|
+ if ((mysql->options.client_flag & CLIENT_LOCAL_FILES) &&
|
+ mysql->options.extension && mysql->extension->auto_local_infile == WAIT_FOR_QUERY &&
|
+ (query[0] == 'l' || query[0] == 'L'))
|
+ {
|
+ if (strncasecmp(query, "load", 4) == 0)
|
+ mysql->extension->auto_local_infile= ACCEPT_FILE_REQUEST;
|
+ } |
then the original failure of the test main.ps_load when it is run with the option --ps-protocol be fixed.
The proposed changes in the source code of the function mysql_stmt_prepare() is obviously just a hack to temporarily fix the issue.
So, the major question is what was the reason to write so non-obvious and pretty hacked implementation for handling of statement LOAD DATA LOCAL INFILE on client side?
Attachments
Issue Links
- relates to
-
CONC-515 Implement safe LOAD DATA LOCAL INFILE method
-
- Stalled
-
Activity
Ok, I see the motivation for this approach. Could I then consider the change originally listed in this bug report
--- a/libmariadb/mariadb_stmt.c
|
+++ b/libmariadb/mariadb_stmt.c
|
@@ -1713,6 +1713,17 @@ int STDCALL mysql_stmt_prepare(MYSQL_STMT *stmt, const char *query, unsigned lon
|
}
|
memset(stmt->bind, 0, sizeof(MYSQL_BIND) * stmt->field_count);
|
}
|
+ if ((mysql->options.client_flag & CLIENT_LOCAL_FILES) &&
|
+ mysql->options.extension && mysql->extension->auto_local_infile == WAIT_FOR_QUERY &&
|
+ (query[0] == 'l' || query[0] == 'L'))
|
+ {
|
+ if (strncasecmp(query, "load", 4) == 0)
|
+ mysql->extension->auto_local_infile= ACCEPT_FILE_REQUEST;
|
+ } |
as an acceptable way to fix the bug?
This issue is linked to CONC-515: Implement safe load data local infile, which I plan to push this week (3.2-georg branch).
If you want to test MDEV-16708 use a temporary fix, and move the check for local infile from ma_simple_command to mthd_my_send_cmd, so it will work for both text and binary protocol.
shulga, would that work? A file request is sent on execute, not on prepare. And on execute you no longer have the statement text.
@@ -366,6 +380,20 @@ mthd_my_send_cmd(MYSQL *mysql,enum enum_server_command command, const char *arg,
|
{
|
NET *net= &mysql->net;
|
int result= -1;
|
+
|
+ if (command == COM_QUERY ||
|
+ command == COM_STMT_PREPARE)
|
+ {
|
+ if ((mysql->options.client_flag & CLIENT_LOCAL_FILES) &&
|
+ mysql->options.extension && mysql->extension->auto_local_infile == WAIT_FOR_QUERY &&
|
+ arg && (*arg == 'l' || *arg == 'L') )
|
+ {
|
+ if (strncasecmp(arg, "load", 4) == 0)
|
+ mysql->extension->auto_local_infile= ACCEPT_FILE_REQUEST;
|
+ }
|
+ }
|
You can see the commit 02e7d5654dd83600f3c1e27b7c61ab1f4ea59138.
LOAD DATA LOCAL is inherently insecure, the server can request the client to send any local (to the client) file and the client will oblige.
The client can disable LOAD DATA LOCAL but historically it was enabled by default.
Disabling this feature by default would break existing user applications that rely on it. Keeping it enabled — expose users that do not use it (the majority) to possible exploitation.
This heuristics is a compromise solution. It temporarily allows one "file request" packet after the client sends an 'L' statement. All users who don't use LOAD DATA will thus become not vulnerable. Users who use LOAD DATA get the window of exploitation significantly reduced.
This heuristics is not perfect. For example, it will not recognize statements like
In all cases when it does not work, the user should enable LOAD DATA LOCAL explicitly, just as she would have to do if we'd disabled LOAD DATA LOCAL by default.