maria-developers team mailing list archive

[Daniel Black <daniel@xxxxxxxxxxx>]

On Thu, Oct 22, 2020 at 8:25 PM Sergei Golubchik <serg@xxxxxxxxxxx> 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/9a478b11de26fb43f8a7df4253f80d549cd41ab1

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 '*'))