Uploaded image for project: 'MariaDB Server'
  1. MariaDB Server
  2. MDEV-24207

recognise mysql forms of invalid password for mysql_native_password

Details

    Description

      MySQL-5.7 and 8.0 have invalid passwords of forms:

      mysql-5.7

      mysql> show create user `mysql.sys`@localhost;
      +------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
      | CREATE USER for mysql.sys@localhost                                                                                                                                          |
      +------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
      | CREATE USER 'mysql.sys'@'localhost' IDENTIFIED WITH 'mysql_native_password' AS '*THISISNOTAVALIDPASSWORDTHATCANBEUSEDHERE' REQUIRE NONE PASSWORD EXPIRE DEFAULT ACCOUNT LOCK |
      +------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
      1 row in set (0.00 sec)
       
      mysql> CREATE USER 'mysql.XXX'@'localhost' IDENTIFIED WITH 'mysql_native_password' AS '*THISISNOTAVALIDPASSWORDTHATCANBEUSEDHERE' REQUIRE NONE PASSWORD EXPIRE DEFAULT ACCOUNT LOCK;
      Query OK, 0 rows affected (0.00 sec)
       
      mysql> CREATE USER 'mysql.YYY'@'localhost' IDENTIFIED WITH 'mysql_native_password' AS '*WALKISNOTAVALIDPASSWORDTHATCANBEUSEDFOOD' REQUIRE NONE PASSWORD EXPIRE DEFAULT ACCOUNT LOCK;
      Query OK, 0 rows affected (0.00 sec)
       
      mysql> CREATE USER 'mysql.XXX'@'localhost' IDENTIFIED WITH 'mysql_native_password' AS 'maybe this can be anything' REQUIRE NONE PASSWORD EXPIRE DEFAULT ACCOUNT LOCK;
      ERROR 1827 (HY000): The password hash doesn't have the expected format. Check if the correct password algorithm is being used with the PASSWORD() function.
       
      mysql>  CREATE USER 'mysql.ZZZZ'@'localhost' IDENTIFIED WITH 'mysql_native_password' AS '!WALKISNOTAVALIDPASSWORDTHATCANBEUSEDFOOD' REQUIRE NONE PASSWORD EXPIRE DEFAULT ACCOUNT LOCK;
      ERROR 1827 (HY000): The password hash doesn't have the expected format. Check if the correct password algorithm is being used with the PASSWORD() function.
      mysql>  CREATE USER 'mysql.ZZZZ'@'localhost' IDENTIFIED WITH 'mysql_native_password' AS '*WALKISNOTAVALIDPASSWORDTHATCANBEUSEDfood' REQUIRE NONE PASSWORD EXPIRE DEFAULT ACCOUNT LOCK;
      Query OK, 0 rows affected (0.00 sec)
      

      So begins with '*' and has the right length is the compatibility to aim for if supporting the mysql-5.7 syntax/definitions. The use of non-hex characters however make it invalid.

      Attachments

        Issue Links

          Activity

            danblack Daniel Black added a comment -

            bb-10.4-danielblack-MDEV-24207-accept-m57-forms-of-invalid-password is there for review.

            The goal is to provide transportability of MYSQL native password's invalid forms while ensuring that the invalid password really is invalid.

            danblack Daniel Black added a comment - bb-10.4-danielblack- MDEV-24207 -accept-m57-forms-of-invalid-password is there for review. The goal is to provide transportability of MYSQL native password's invalid forms while ensuring that the invalid password really is invalid.
            danblack Daniel Black added a comment -

            cvicentiu can you have a review of this one please.

            Is removing compatibility of 11111111111111111111111111111111111111111 and insisting on *1111111111111111111111111111111111111111 too much of a change?

            danblack Daniel Black added a comment - cvicentiu can you have a review of this one please. Is removing compatibility of 11111111111111111111111111111111111111111 and insisting on *1111111111111111111111111111111111111111 too much of a change?
            cvicentiu Vicențiu Ciorbaru added a comment - - edited

            danblack After carefully looking at this, here is my assessment.

            1. The main goal of this patch is to prevent MariaDB's native_password_plugin from "parsing" the hex (or non hex) authentication_string. Due to how the current code is written, we convert any string (within native_password_get_salt) that has the appropriate length to a "binary" representation, that can potentially match a real password.

            More specifically,
            "*THISISNOTAVALIDPASSWORDTHATCANBEUSEDHERE" produces the same results as "*d13c3c78dafa52d9bce09bdd1adcb7befced1ebe".

            2. Please reword the commit message to highlight this fact first. It took a long time to understand why the check was necessary and I had to read the review of MDEV-22974 to understand the problem in more detail.
            Also, the commit message has the MDEV-22974 and the following title is not related to MDEV-22974, nor to MDEV-24207's title.

            3. I see no reason to require the specific * character at the start. It is a breaking change for users in a stable version so it's a definite *no* there.

            4. I like checking for non-hex characters and if any are found, shortcut and set the password to be "invalid password". However I think the while loop is very hard to read. Here's an alternative that I suggest to use, replace the whole if-else clause with this:

              const char *hash_end= hash + hash_length;
              for (const char *c= hash + 1; c < hash_end; c++)
              {
                /* If any non-hex characters are found, mark the password as invalid. */
                if (!(*c >= '0' && *c <= '9') &&
                    !(*c >= 'A' && *c <= 'F') &&
                    !(*c >= 'a' && *c <= 'f'))
                {
                  memcpy(out, invalid_password, sizeof(invalid_password));
                  *out_length= sizeof(invalid_password);
                  return 0;
                }
              }
            

            With points 1-4, the patch fixes both the "invalid-password-could-match-to-a-real-password" problem and also is compatible with MySQL's invalid password definition. I think that covers all cases.

            Assuming buildbot doesn't highlight any problems, I'm ok with pushing the patch with the review changes.

            cvicentiu Vicențiu Ciorbaru added a comment - - edited danblack After carefully looking at this, here is my assessment. 1. The main goal of this patch is to prevent MariaDB's native_password_plugin from "parsing" the hex (or non hex) authentication_string. Due to how the current code is written, we convert any string (within native_password_get_salt) that has the appropriate length to a "binary" representation, that can potentially match a real password. More specifically, "*THISISNOTAVALIDPASSWORDTHATCANBEUSEDHERE" produces the same results as "*d13c3c78dafa52d9bce09bdd1adcb7befced1ebe" . 2. Please reword the commit message to highlight this fact first. It took a long time to understand why the check was necessary and I had to read the review of MDEV-22974 to understand the problem in more detail. Also, the commit message has the MDEV-22974 and the following title is not related to MDEV-22974 , nor to MDEV-24207 's title. 3. I see no reason to require the specific * character at the start. It is a breaking change for users in a stable version so it's a definite * no * there. 4. I like checking for non-hex characters and if any are found, shortcut and set the password to be "invalid password". However I think the while loop is very hard to read. Here's an alternative that I suggest to use, replace the whole if-else clause with this: const char *hash_end= hash + hash_length; for ( const char *c= hash + 1; c < hash_end; c++) { /* If any non-hex characters are found, mark the password as invalid. */ if (!(*c >= '0' && *c <= '9' ) && !(*c >= 'A' && *c <= 'F' ) && !(*c >= 'a' && *c <= 'f' )) { memcpy (out, invalid_password, sizeof (invalid_password)); *out_length= sizeof (invalid_password); return 0; } } With points 1-4, the patch fixes both the "invalid-password-could-match-to-a-real-password" problem and also is compatible with MySQL's invalid password definition. I think that covers all cases. Assuming buildbot doesn't highlight any problems, I'm ok with pushing the patch with the review changes.
            cvicentiu Vicențiu Ciorbaru added a comment - - edited

            And actually, one more idea that came to mind, why not also check hash[0] to see if it is in fact the "*" character. If it is not, memcpy the invalid_password directly. This will also allow for the following use-cases:

            To quickly "disable" a user's password, change the "*" to "!". To "re-enable" the password, change the "!" to "*".

            cvicentiu Vicențiu Ciorbaru added a comment - - edited And actually, one more idea that came to mind, why not also check hash[0] to see if it is in fact the "*" character. If it is not, memcpy the invalid_password directly. This will also allow for the following use-cases: To quickly "disable" a user's password, change the "*" to "!". To "re-enable" the password, change the "!" to "*".

            cvicentiu, I presume, you meant "if hash[0] is not /[a-z0-9*]/

            serg Sergei Golubchik added a comment - cvicentiu , I presume, you meant "if hash[0] is not /[a-z0-9*]/
            danblack Daniel Black added a comment -

            On account locking via password 10.4 has `ALTER USER xx ACCOUNT LOCK/ UNLOCK` so no need to add another mechanism.

            Given there's no current restriction on the first character I guess its breaking an compatibility to give it any meaning.

            Thanks for the other review comments cvicentiu, I'll take those in the rework.

            serg I don't follow https://github.com/MariaDB/server/commit/778cb28230b42d1abdc388c3d5e4d2807f4ea198#r45005968 comment literally or objective wise. The password length is the main criteria for it being invalid (per MDEV-22974).

            danblack Daniel Black added a comment - On account locking via password 10.4 has `ALTER USER xx ACCOUNT LOCK/ UNLOCK` so no need to add another mechanism. Given there's no current restriction on the first character I guess its breaking an compatibility to give it any meaning. Thanks for the other review comments cvicentiu , I'll take those in the rework. serg I don't follow https://github.com/MariaDB/server/commit/778cb28230b42d1abdc388c3d5e4d2807f4ea198#r45005968 comment literally or objective wise. The password length is the main criteria for it being invalid (per MDEV-22974 ).

            People

              danblack Daniel Black
              danblack Daniel Black
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Git Integration

                  Error rendering 'com.xiplink.jira.git.jira_git_plugin:git-issue-webpanel'. Please contact your Jira administrators.