Re: [Maria-developers] 7807d9b6939: MDEV-22974: mysql_native_password make "invalid" valid
Hi, Daniel! On Oct 22, Daniel Black wrote:
revision-id: 7807d9b6939 (mariadb-10.5.4-62-g7807d9b6939) parent(s): 054f10365c4 author: Daniel Black <daniel@mariadb.org> committer: Daniel Black <daniel@mariadb.org> timestamp: 2020-07-16 17:06:36 +1000 message:
MDEV-22974: mysql_native_password make "invalid" valid
Per b9f3f06857ac, mysql_system_tables_data.sql creates a mysql_native_password with a salted hash of "invalid" so that `set password` will detect a native password can be pplied:.
"applied"
SHOW CREATE USER; dilignently uses this value in its output generating the SQL:
MariaDB [(none)]> show create user;
+---------------------------------------------------------------------------------------------------+ | CREATE USER for dan@localhost | +---------------------------------------------------------------------------------------------------+ | CREATE USER `dan`@`localhost` IDENTIFIED VIA mysql_native_password USING 'invalid' OR unix_socket | +---------------------------------------------------------------------------------------------------+
Attempting to execute this before this patch resutls in:
MariaDB [(none)]> CREATE USER `dan2`@`localhost` IDENTIFIED VIA mysql_native_password USING 'invalid' OR unix_socket; ERROR 1372 (HY000): Password hash should be a 41-digit hexadecimal number
As such deep the the implementation of mysql_native_password we make "invalid" valid (pun intended) such that the above create user will succeed.
native_password_get_salt is only called in the context of set_user_salt, so all setting of native passwords to hashed content of 'invalid', quite literally create an invalid password.
So other forms of "invalid" are valid SQL in creating invalid passwords:
MariaDB [(none)]> set password = 'invalid'; Query OK, 0 rows affected (0.001 sec)
MariaDB [(none)]> alter user dan@localhost IDENTIFIED BY PASSWORD 'invalid'; Query OK, 0 rows affected (0.000 sec)
diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc index d94016b7815..3cd7a67ae1a 100644 --- a/sql/sql_acl.cc +++ b/sql/sql_acl.cc @@ -14539,6 +14539,12 @@ static int native_password_get_salt(const char *hash, size_t hash_length,
if (hash_length != SCRAMBLED_PASSWORD_CHAR_LENGTH) { + if (hash_length == 7 && strcmp(hash, "invalid") == 0) + { + memcpy(out, "invalid", 7); + *out_length= 7; + return 0; + }
okay. After you said ASAN, I think I can see why this could be problematic. You can, of course, pad it to SCRAMBLED_PASSWORD_CHAR_LENGTH, but then you'll create a *valid* scramble that would correspond to some actual password. One option would be to allow "invalid" literal in set_user_auth(), before even any plugin checks. But I'm unsure of the implications.
my_error(ER_PASSWD_LENGTH, MYF(0), SCRAMBLED_PASSWORD_CHAR_LENGTH); return 1; }
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
On Thu, Oct 22, 2020 at 8:25 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Daniel!
On Oct 22, Daniel Black wrote:
SHOW CREATE USER; dilignently uses this value in its output generating the SQL:
MariaDB [(none)]> show create user;
+---------------------------------------------------------------------------------------------------+ | CREATE USER for dan@localhost | +---------------------------------------------------------------------------------------------------+ | CREATE USER `dan`@`localhost` IDENTIFIED VIA mysql_native_password USING 'invalid' OR unix_socket | +---------------------------------------------------------------------------------------------------+
Attempting to execute this before this patch resutls in:
MariaDB [(none)]> CREATE USER `dan2`@`localhost` IDENTIFIED VIA mysql_native_password USING 'invalid' OR unix_socket; ERROR 1372 (HY000): Password hash should be a 41-digit hexadecimal number
As such deep the the implementation of mysql_native_password we make "invalid" valid (pun intended) such that the above create user will succeed.
native_password_get_salt is only called in the context of set_user_salt, so all setting of native passwords to hashed content of 'invalid', quite literally create an invalid password.
So other forms of "invalid" are valid SQL in creating invalid passwords:
MariaDB [(none)]> set password = 'invalid'; Query OK, 0 rows affected (0.001 sec)
MariaDB [(none)]> alter user dan@localhost IDENTIFIED BY PASSWORD 'invalid'; Query OK, 0 rows affected (0.000 sec)
diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc index d94016b7815..3cd7a67ae1a 100644 --- a/sql/sql_acl.cc +++ b/sql/sql_acl.cc @@ -14539,6 +14539,12 @@ static int native_password_get_salt(const char *hash, size_t hash_length,
if (hash_length != SCRAMBLED_PASSWORD_CHAR_LENGTH) { + if (hash_length == 7 && strcmp(hash, "invalid") == 0) + { + memcpy(out, "invalid", 7); + *out_length= 7; + return 0; + }
okay. After you said ASAN, I think I can see why this could be problematic.
You can, of course, pad it to SCRAMBLED_PASSWORD_CHAR_LENGTH, but then you'll create a *valid* scramble that would correspond to some actual password.
yikes.
One option would be to allow "invalid" literal in set_user_auth(), before even any plugin checks. But I'm unsure of the implications.
Updated: bb-10.4-danielblack-MDEV-22974-mysql_native_password-make-invalid-valid with 2 commits: first addresses this review: https://github.com/MariaDB/server/commit/9a478b11de26fb43f8a7df4253f80d549cd... In native_password_get_salt we set a scramble of an invalid length. We check the length in native_password_authenticate before even looking at the contents of the scrambled password. This keeps the implementation confined to the native_auth implementation. second - d5ddbdcf61218a0a45228d2f22fe4dd77a7fe7c8 accepts the mysql invalid password forms (and becomes slightly more strict on our invalid forms (those not beginning with '*'))
Hi, Daniel! On Oct 26, Daniel Black wrote:
On Thu, Oct 22, 2020 at 8:25 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Daniel!
On Oct 22, Daniel Black wrote:
@@ -14539,6 +14539,12 @@ static int native_password_get_salt(const char *hash, size_t hash_length,
if (hash_length != SCRAMBLED_PASSWORD_CHAR_LENGTH) { + if (hash_length == 7 && strcmp(hash, "invalid") == 0) + { + memcpy(out, "invalid", 7); + *out_length= 7; + return 0; + }
okay. After you said ASAN, I think I can see why this could be problematic.
Updated:
I don't see why you did it that complex with invalid_password and everything. It seems you could've fixed the ASAN error from your first patch with just @@ -14498,7 +14498,7 @@ static int native_password_authenticate(MYSQL_PLUGIN_VI> info->password_used= PASSWORD_USED_YES; if (pkt_len == SCRAMBLE_LENGTH) { - if (!info->auth_string_length) + if (info->auth_string_length != SCRAMBLE_LENGTH) DBUG_RETURN(CR_AUTH_USER_CREDENTIALS); if (check_scramble(pkt, thd->scramble, (uchar*)info->auth_string)) Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
That works for me. Pushed first patch with above simple fix. On Sat, Oct 31, 2020 at 4:45 AM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Daniel!
On Oct 26, Daniel Black wrote:
On Thu, Oct 22, 2020 at 8:25 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Daniel!
On Oct 22, Daniel Black wrote:
@@ -14539,6 +14539,12 @@ static int native_password_get_salt(const char *hash, size_t hash_length,
if (hash_length != SCRAMBLED_PASSWORD_CHAR_LENGTH) { + if (hash_length == 7 && strcmp(hash, "invalid") == 0) + { + memcpy(out, "invalid", 7); + *out_length= 7; + return 0; + }
okay. After you said ASAN, I think I can see why this could be problematic.
Updated:
I don't see why you did it that complex with invalid_password and everything. It seems you could've fixed the ASAN error from your first patch with just
@@ -14498,7 +14498,7 @@ static int native_password_authenticate(MYSQL_PLUGIN_VI> info->password_used= PASSWORD_USED_YES; if (pkt_len == SCRAMBLE_LENGTH) { - if (!info->auth_string_length) + if (info->auth_string_length != SCRAMBLE_LENGTH) DBUG_RETURN(CR_AUTH_USER_CREDENTIALS);
if (check_scramble(pkt, thd->scramble, (uchar*)info->auth_string))
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
participants (2)
-
Daniel Black -
Sergei Golubchik