[CONC-525] Incorrect handling of the statement LOAD DATA LOCAL INFILE by implementation of libmariadb Created: 2021-02-08  Updated: 2021-02-23  Resolved: 2021-02-23

Status: Closed
Project: MariaDB Connector/C
Component/s: Security
Affects Version/s: None
Fix Version/s: 3.2.0

Type: Bug Priority: Critical
Reporter: Dmitry Shulga Assignee: Georg Richter
Resolution: Fixed Votes: 0
Labels: None

Issue Links:
Relates
relates to CONC-515 Implement safe LOAD DATA LOCAL INFILE... In Review

 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?

 Comments   
Comment by Sergei Golubchik [ 2021-02-12 ]

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

/* a comment */ LOAD DATA LOCAL INFILE ...

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.

Comment by Dmitry Shulga [ 2021-02-16 ]

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?

Comment by Georg Richter [ 2021-02-16 ]

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.

Comment by Sergei Golubchik [ 2021-02-17 ]

shulga, would that work? A file request is sent on execute, not on prepare. And on execute you no longer have the statement text.

Comment by Georg Richter [ 2021-02-17 ] 

@@ -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;
 
+    }
 
+  }

Comment by Georg Richter [ 2021-02-23 ]

Fixed in rev. ca1ea5c1a285b6982edeb19e59c814a4ff4da11b

Generated at Thu Feb 08 03:05:56 UTC 2024 using Jira 8.20.16#820016-sha1:9d11dbea5f4be3d4cc21f03a88dd11d8c8687422.