MDEV-38431: [10.6] auth switch with long password corrupts database name

[rophy]

• The Jira issue number for this PR is: MDEV-38431

Description

Problem: When a client connects with CLIENT_PLUGIN_AUTH_LENENC_CLIENT_DATA capability and a password >= 251 bytes, the server incorrectly calculates the database name pointer, causing "Unknown database" errors with garbage characters.

Root cause: In parse_client_handshake_packet() (sql/sql_acl.cc), the database
pointer was calculated as db + passwd_len + 1, which assumes a 1-byte password length prefix. However, when password >= 251 bytes, LENENC encoding uses a 3-byte prefix (0xFC + 2 bytes). The passwd pointer has already been advanced past this prefix by safe_net_field_length_ll(), but db has not.

Before fix:

ERROR 1049 (42000): Unknown database 'a'

(where 'a' is a garbage character from the password buffer)

After fix:

db
mdev38431_db

(connects to the correct database)

Side effects: None expected. The fix only affects the specific code path when CLIENT_PLUGIN_AUTH_LENENC_CLIENT_DATA is set
and password >= 251 bytes.

How can this PR be tested?

Run the included MTR regression test:

./mysql-test/mtr --suite=plugins mdev38431

The test verifies:

1. Short password (7 bytes) - baseline test
2. Long password (260 bytes) - triggers 3-byte LENENC encoding
3. Very long password (500 bytes) - additional boundary test

Basing the PR against the correct MariaDB version

• This is a new feature and the PR is
  based against the latest MariaDB development branch
• This is a bug fix and the PR is based against the earliest branch in which the
  bug can be reproduced
  Bug reproduced on 10.6, 10.11, 11.4, and 11.8. PR is based on 10.6.

Backward compatibility

No backward compatibility concerns:

• No on-disk format changes
• No behavior changes for existing valid use cases
• Users can upgrade seamlessly

[CLAassistant]

All committers have signed the CLA.

[gkodinov]

Thank you for your contribution! This is a preliminary review. Once addressed, I will solicit a final review from the developers.

I'd also verify (and add a test for) the COM_CHANGE_USER. It looks like it's missing handling for CLIENT_PLUGIN_AUTH_LENENC_CLIENT_DATA when dealing with the password.

[rophy]

Regarding COM_CHANGE_USER and CLIENT_PLUGIN_AUTH_LENENC_CLIENT_DATA:

The MDEV-38431 bug requires user, LENENC-encoded password (>250 bytes), and database to be in the same packet.

COM_CHANGE_USER with auth plugin switch is NOT affected because the password is sent in a separate packet after auth switch negotiation. The database pointer is calculated from the initial packet before the password arrives.

I verified this by testing COM_CHANGE_USER with 256+ byte cleartext passwords - they work correctly.

[gkodinov]

Regarding COM_CHANGE_USER and CLIENT_PLUGIN_AUTH_LENENC_CLIENT_DATA:

The MDEV-38431 bug requires user, LENENC-encoded password (>250 bytes), and database to be in the same packet.

COM_CHANGE_USER with auth plugin switch is NOT affected because the password is sent in a separate packet after auth switch negotiation. The database pointer is calculated from the initial packet before the password arrives.

I verified this by testing COM_CHANGE_USER with 256+ byte cleartext passwords - they work correctly.

I've tried the following fragment (replace the data as you see fit) :

        MYSQL *conn;
        static char *opt_default_auth = (char *)"mysql_clear_password";
        static uint opt_protocol =  MYSQL_PROTOCOL_TCP;
        conn = mysql_init(NULL);
        if (conn == NULL)
        {
                fprintf(stderr, "mysql_init() failed\n");
                return 1;
        }

        mysql_options(conn, MYSQL_PLUGIN_DIR, "/Users/gkodinov/dev/server-main/bld/libmariadb/");
        mysql_options(conn, MYSQL_DEFAULT_AUTH, opt_default_auth);
        mysql_options(conn, MYSQL_OPT_PROTOCOL, &opt_protocol);
        if (mysql_real_connect(conn, "localhost", "shortuser", "secret",
                               "mdev38431_db", 20202, "/Users/gkodinov/dev/server-main/bld/x.sock", 0) == NULL)
        {
                fprintf(stderr, "mysql_real_connect() failed\n");
                mysql_close(conn);
                return 1;
        }
        if (mysql_change_user(conn, "longuser",                              "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
                              "mdev38431_db"))
        {
                fprintf(stderr, "mysql_change_user() failed\n");
                mysql_close(conn);
                return 1;
        }

And I get an DBUG_ASSERT calling mysql_change_user() in the above as in:

Assertion failed: (data_len <= 255), function send_change_user_packet, file my_auth.c, line 233.
Abort trap: 6

The callstack of this is:

libsystem_kernel.dylib!__pthread_kill (Unknown Source:0)
libsystem_pthread.dylib!pthread_kill (Unknown Source:0)
libsystem_c.dylib!abort (Unknown Source:0)
libsystem_c.dylib!__assert_rtn (Unknown Source:0)
libmariadb.3.dylib!send_change_user_packet (/Users/gkodinov/dev/server-main/libmariadb/plugins/auth/my_auth.c:233)
libmariadb.3.dylib!client_mpvio_write_packet (/Users/gkodinov/dev/server-main/libmariadb/plugins/auth/my_auth.c:639)
mysql_clear_password.so!clear_password_auth_client (/Users/gkodinov/dev/server-main/libmariadb/plugins/auth/mariadb_cleartext.c:51)
libmariadb.3.dylib!run_plugin_auth (/Users/gkodinov/dev/server-main/libmariadb/plugins/auth/my_auth.c:801)
libmariadb.3.dylib!mysql_change_user (/Users/gkodinov/dev/server-main/libmariadb/libmariadb/mariadb_lib.c:2278)
client!main (/Users/gkodinov/dev/client/client.cc:37)
start (Unknown Source:0)

the fragment in send_change_user_packet is:

if (!data_len)
    *end++= 0;
  else
  {
    if (mysql->client_flag & CLIENT_SECURE_CONNECTION)
    {
      DBUG_ASSERT(data_len <= 255);
      if (data_len > 255)
      {
        my_set_error(mysql, CR_MALFORMED_PACKET, SQLSTATE_UNKNOWN, 0);
        goto error;
      }
      *end++= data_len;
    }

So no, mysql_change_user() doesn't work with longer passwords and it's not a separate packet.
Check e.g. https://dev.mysql.com/doc/dev/mysql-server/latest/page_protocol_com_change_user.html

This however seems to be a separate issue, as the limitation was there to start with. I'd wait for the final review and discuss this with the assigned reviewer. But my 2 cents is that you need to make send_change_user_packet() do similarly to send_client_reply_packet() in that respect. But this will produce backward compatibility issues with the protocol since the server (parse_com_change_user() to be specific) expects a single byte length. So a new client (that might send a longer string) talking to an old server will still fail.

The way I see it there are the following options:

• disallow passwords longer than 251.
• add a new capability flag (an addon to CLIENT_PLUGIN_AUTH_LENENC_CLIENT_DATA) to say that COM_CHANGE_USER produces and expects length encoded passwords.
• fix com_change_user to produce and expect length encoded passwords when CLIENT_PLUGIN_AUTH_LENENC_CLIENT_DATA and be ready to live with the new client talking to an old server and failing to authenticate instead of the error/assert it produces now on mysql_change_user().

I will leave it to the final reviewer to pick one. But I'd vote for the last one.

[gkodinov]

please fix the test failure on windows and add a test for mysql_change_user().

[rophy]

please fix the test failure on windows and add a test for mysql_change_user().

Added two tests to cover mysql_change_user() with short and long password.

Note that both tests PASSED. Somehow the code path of "long password within single packet" could not be triggered.

Update: figured out how to reproduced the bug. Added a test which is commented out since it currently fails.

[rophy]

Added the fix for mysql_change_user() at both server side and libmariadb, enabled the failing test

mariadb-corporation/mariadb-connector-c#298

[rophy]

hi, as the mysql_change_user requires fixing client sdks, I'd propose that we focus the scope of this PR on auth_switch

[vuvova]

Yes, absolutely, thank you.

I wanted to suggest that, but didn't want to be too picky. It's mainly important to have COM_CHANGE_USER changes in a separate commit (and they were), because after you push PRs won't matter anymore, the history only contains commits, not PRs. Separate PR is better too, but not as critically important.

[vuvova]

in your last commit "fix db pointer for old protocol without CLIENT_SECURE_CONNECTION" you only fixed parse_com_change_user_packet(), but not parse_client_handshake_packet().

I expected the opposite, that you fix parse_client_handshake_packet() and move everything related to COM_CHANGE_USER to a new PR.

The fix itself is ok, although I suggest to move ER_UNKNOWN_COM_ERROR check down so that it would apply in other cases too. Like, for example, if (db > net->read_pos + pkt_len) { ... }

[vuvova]

looks good, thanks. Would you mind squashing all commits into one? Then I'll merge it.

[rophy]

Done