MySQL Bugs: #102266: mysql_change_user fails due to COM_CHANGE

Bug #102266	mysql_change_user fails due to COM_CHANGE_USER incorrectly parsed
Submitted:	18 Jan 2021 0:44	Modified:	26 May 2021 12:28
Reporter:	Rene' Cannao'	Email Updates:
Status:	Closed	Impact on me:	None
Category:	MySQL Server: C API (client library)	Severity:	S3 (Non-critical)
Version:	5.6 5.7, 5.7.32 , 8.0	OS:	Any
Assigned to:		CPU Architecture:	Any
Tags:	Contribution

Description:
If a client connects to a MySQL server specifying a collation that the server doesn't know, MySQL server silently switch the collation (and even charset).
This is described and reported in http://bugs.mysql.com/102265 .

Although, if the client then tries to run mysql_change_user(), the server returns an "Access denied" error , even specifying that no password was specified.

This behavior is conflicting with the behavior described in http://bugs.mysql.com/102265 .
In fact MySQL server allows to connect, but then doesn't allow to run CHANGE_USER .
I suggest both mysql_real_connect() and mysql_change_user() should return an error.

How to repeat:
Compile the following code against libmysqlclient version 8.0 and run it against a MySQL server running 5.7 configured with default charset latin1:

$ cat change_user_bug.c 
#include <stdio.h>
#include <mysql.h>

int main() {
  MYSQL mysql;
  mysql_init(&mysql);
  fprintf(stderr, "Setting charset utf8mb4\n");
  mysql_options(&mysql, MYSQL_SET_CHARSET_NAME, "utf8mb4");
  if (!mysql_real_connect(&mysql,"127.0.0.1","sbtest","sbtest","information_schema",3306,NULL,0)) {
    fprintf(stderr, "Failed to connect to database: Error: %s\n",
    mysql_error(&mysql));
	return 1;
  }
  fprintf(stderr, "Server version: %s\n", mysql.server_version);
  const char * query = "SHOW VARIABLES LIKE '%coll%'";
  fprintf(stderr, "Running query: %s\n", query);
  if (mysql_query(&mysql, query)) {
    fprintf(stderr, "Failed to run query: Error: %s\n",
    mysql_error(&mysql));
	return 1;
  }
  MYSQL_RES *result = mysql_store_result(&mysql);
  MYSQL_ROW row;
  fprintf(stderr, "Result:\n");
  while ((row = mysql_fetch_row(result))) {
    fprintf(stderr, "%s = %s\n", row[0], row[1] ? row[1] : "NULL");
  }
  if (mysql_change_user(&mysql, "sbtest","sbtest","information_schema")) {
    fprintf(stderr, "Failed to change user.  Error: %s\n",
    mysql_error(&mysql));
	return 1;
  }
  fprintf(stderr, "All good\n");
  return 0;
}

The code will explicitly set charset to utf8mb4, connect to the backend, retrieve some of its session variable related to collation, then try to run mysql_change_user() .

This is the output if compiled against libmysqlclient older than 8.0 (where the default collation for utf8mb4 is utf8mb4_general_ci) :

./change_user_bug 
Setting charset utf8mb4
Server version: 5.7.32-0ubuntu0.18.04.1-log
Running query: SHOW VARIABLES LIKE '%coll%'
Result:
collation_connection = utf8mb4_general_ci
collation_database = utf8_general_ci
collation_server = latin1_swedish_ci
All good

This is the output if compiled against libmysqlclient version 8.0 (where the default collation for utf8mb4 is utf8mb4_0900_ai_ci , and unknown to MySQL 5.7) :

./change_user_bug 
Setting charset utf8mb4
Server version: 5.7.32-0ubuntu0.18.04.1-log
Running query: SHOW VARIABLES LIKE '%coll%'
Result:
collation_connection = latin1_swedish_ci
collation_database = utf8_general_ci
collation_server = latin1_swedish_ci
Failed to change user.  Error: Access denied for user 'sbtest'@'localhost' (using password: NO)

Suggested fix:
The error message should not be "Access denied" with "(using password: NO)": a password was used.
"Access denied" seems correct, but it should explain that the reason is unknown collation.

Hello Rene,

Thank you for the report and test case.
Verified as described.

Regards,
Umesh

After a lot of debugging, I found the root cause of this and I can say the title is actually wrong: mysql_change_user() doesn't fail because of unknown collation, but because the COM_CHANGE_USER is incorrectly parsed in function parse_com_change_user_packet().

In 5.7 , ptr is not increased by 2 (the size of the field where the collation is specified):
https://github.com/mysql/mysql-server/blob/5.7/sql/auth/sql_authentication.cc#L1111-L1113

While in 8.0 ptr is correctly increase by 2:
https://github.com/mysql/mysql-server/blob/8.0/sql/auth/sql_authentication.cc#L2191-L2197

For further reference, this bug was fixed in 8.0.1 almost 4 years ago (March 2017) but it never made it into 5.7

Also, please keep in mind that also the implementation in MySQL 8.0 has a bug:
ptr is increased by 2 (the size of the collation) *only* if CLIENT_PLUGIN_AUTH capability is set.
This implementation is not correct, because charset/collation can be sent also if CLIENT_PLUGIN_AUTH is not set.

This can be verified in client.cc file, where charset/collation number is sent:
https://github.com/mysql/mysql-server/blob/8.0/sql-common/client.cc#L3970-L3973

Collation number can be sent even if CLIENT_PLUGIN_AUTH is not set, in fact the check for CLIENT_PLUGIN_AUTH is few lines after:
https://github.com/mysql/mysql-server/blob/8.0/sql-common/client.cc#L3975-L3976

I am renaming this bug from "mysql_change_user fails with unknown collation" to "mysql_change_user fails due to COM_CHANGE_USER incorrectly parsed" .

Since there is a parsing error also in 8.0 , I am adding 8.0 to the list of affected versions

New suggested fix:

At minimum, port this code from 8.0 to 5.7:
https://github.com/mysql/mysql-server/blob/8.0/sql/auth/sql_authentication.cc#L2193-L2197

    /*
      ptr needs to be updated to point to correct position so that
      connection attributes are read properly.
    */
    ptr = ptr + 2 + strlen(client_plugin) + 1;

A more complete fix should be to evaluate if a collation number is sent even if CLIENT_PLUGIN_AUTH is not set, even if probably there are no clients out there that do not have CLIENT_PLUGIN_AUTH capabilities.

my suggested fix for 8.0.34:
diff --git a/sql/auth/sql_authentication.cc b/sql/auth/sql_authentication.cc
index c7c34cc14ab..415b1c7dbc0 100644
--- a/sql/auth/sql_authentication.cc
+++ b/sql/auth/sql_authentication.cc
@@ -2152,6 +2152,7 @@ static bool parse_com_change_user_packet(THD *thd, MPVIO_EXT *mpvio,
   if (ptr + 1 < end) {
     if (mpvio->charset_adapter->init_client_charset(uint2korr(ptr)))
       return true;
+    ptr = ptr + 2;
   }
 
   /* Convert database and user names to utf8 */
@@ -2189,17 +2190,17 @@ static bool parse_com_change_user_packet(THD *thd, MPVIO_EXT *mpvio,
 
   const char *client_plugin;
   if (protocol->has_client_capability(CLIENT_PLUGIN_AUTH)) {
-    client_plugin = ptr + 2;
-    /*
-      ptr needs to be updated to point to correct position so that
-      connection attributes are read properly.
-    */
-    ptr = ptr + 2 + strlen(client_plugin) + 1;
-
+    client_plugin = ptr;
     if (client_plugin >= end) {
       my_error(ER_UNKNOWN_COM_ERROR, MYF(0));
       return true;
     }
+
+    /*
+      ptr needs to be updated to point to correct position so that
+      connection attributes are read properly.
+    */
+    ptr = ptr + strlen(client_plugin) + 1;
   } else
     client_plugin = Cached_authentication_plugins::get_plugin_name(
         PLUGIN_MYSQL_NATIVE_PASSWORD);

patch base on 8.0.25

Attachment: patch8025.diff (application/octet-stream, text), 1.23 KiB.

patch base on 8.0.25 (*) I confirm the code being submitted is offered under the terms of the OCA, and that I am authorized to contribute it.

Contribution: patch8025.diff (application/octet-stream, text), 1.23 KiB.

Bug #103733 marked as duplicate of this one

Posted by developer:
 
Fixed in 5.7.35.

The mysql_change_user() C API function did not properly parse the
COM_CHANGE_USER packet, which could result in silent failure to
process optional query attributes that may have been supplied prior
to the mysql_change_user() call. Thanks for René Cannaò for the
contribution.