maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Vladislav!

On Jan 04, Vladislav Vaintroub wrote:
> revision-id: 82d7e2094c8 (mariadb-10.4.1-41-g82d7e2094c8)
> parent(s): c4ec6bb69ec
> author: Vladislav Vaintroub <wlad@xxxxxxxxxxx>
> committer: Vladislav Vaintroub <wlad@xxxxxxxxxxx>
> timestamp: 2019-01-04 10:42:05 +0100
> message:
> 
> MDEV-7598 Lock user after too many password errors

COM_CHANGE_USER has its own blocking, that works differently.
See change_user_notembedded test.

1. Does it also increment acl_user->password_errors counter now?
2. Should COM_CHANGE_USER be changed to behave more like connect?

even the answer to the second question is "no", please, add tests for
change_user to document the behavior from the first question.

> diff --git a/mysql-test/main/max_password_errors.test b/mysql-test/main/max_password_errors.test
> new file mode 100644
> index 00000000000..fbb9e699b49
> --- /dev/null
> +++ b/mysql-test/main/max_password_errors.test
> @@ -0,0 +1,46 @@
> +--source include/not_embedded.inc
> +set @old_max_password_errors=@@max_password_errors;
> +set global max_password_errors=2;
> +create user u identified by 'good_pass';
> +
> +# Test that user is blocked after 'max_password_errors' bad passwords
> +--replace_result $MASTER_MYSOCK MASTER_SOCKET $MASTER_MYPORT MASTER_PORT
> +error ER_ACCESS_DENIED_ERROR;
> +connect(con1, localhost, u, bas_pass);
> +--replace_result $MASTER_MYSOCK MASTER_SOCKET $MASTER_MYPORT MASTER_PORT
> +error ER_ACCESS_DENIED_ERROR;
> +connect (con1, localhost, u, bad_pass);
> +--replace_result $MASTER_MYSOCK MASTER_SOCKET $MASTER_MYPORT MASTER_PORT
> +error ER_USER_IS_BLOCKED;
> +connect(con1, localhost, u, good_pass);

add another bad_pass connect here, please, to show what error one will
get (ER_ACCESS_DENIED_ERROR or ER_USER_IS_BLOCKED).

> +# Test that FLUSH PRIVILEGES clears the error
> +FLUSH PRIVILEGES;
> +connect (con1, localhost, u, good_pass);
> +disconnect con1;
> diff --git a/mysql-test/main/max_password_errors_auth_named_pipe.opt b/mysql-test/main/max_password_errors_auth_named_pipe.opt

I'd better put the test into the plugins suite. That's where all similar
tests are.

> diff --git a/mysql-test/main/max_password_errors_auth_socket.opt b/mysql-test/main/max_password_errors_auth_socket.opt

same

> diff --git a/mysql-test/main/max_password_errors_auth_socket.test b/mysql-test/main/max_password_errors_auth_socket.test
> new file mode 100644
> index 00000000000..4f31927b02d
> --- /dev/null
> +++ b/mysql-test/main/max_password_errors_auth_socket.test
> @@ -0,0 +1,20 @@
> +# Tests that max_password_errors has no effect on login errors with
> +# passwordless plugins (Windows version / auth_named_pipe)
> +
> +--source include/not_embedded.inc
> +--source include/have_unix_socket.inc
> +
> +set @old_max_password_errors=@@max_password_errors;
> +create user nosuchuser identified with 'unix_socket';

Technically, you need to test that $USER != nosuchuser.
Otherwise someone someday will surely run it from a nosuchuser account :)

May be just include have_unix_socket.inc after you created nosuchuser,
it should do the trick.

> diff --git a/mysql-test/main/mysqld--help.result b/mysql-test/main/mysqld--help.result
> index 8faf332a7dd..3255927d4a6 100644
> --- a/mysql-test/main/mysqld--help.result
> +++ b/mysql-test/main/mysqld--help.result
> @@ -546,6 +546,11 @@ The following specify which files/extra groups are read (specified before remain
>   The maximum BLOB length to send to server from
>   mysql_send_long_data API. Deprecated option; use
>   max_allowed_packet instead.
> + --max-password-errors=# 
> + If there is more than this number of failed connect
> + attempts due to invalid password, user will be blocked
> + from further connections until FLUSH_PRIVILEGES.Value 0
> + disables user blocking

1. Space before "Value"
2. Why 0 disables blocking? max_connect_errors doesn't have a special 0
   value.

>   --max-prepared-stmt-count=# 
>   Maximum number of prepared statements in the server
>   --max-recursive-iterations[=#] 
> @@ -1518,6 +1523,7 @@ max-heap-table-size 16777216
>  max-join-size 18446744073709551615
>  max-length-for-sort-data 1024
>  max-long-data-size 16777216
> +max-password-errors 0
>  max-prepared-stmt-count 16382
>  max-recursive-iterations 18446744073709551615

see? max-recursive-iterations doesn't have a special 0 value,
it simply uses maxint. So does max-join-size.

>  max-relay-log-size 1073741824
> diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc
> index 85b54009219..95f92688677 100644
> --- a/sql/sql_acl.cc
> +++ b/sql/sql_acl.cc
> @@ -12323,6 +12324,23 @@ static bool send_plugin_request_packet(MPVIO_EXT *mpvio,
>  }
>  
>  #ifndef NO_EMBEDDED_ACCESS_CHECKS
> +
> +/**
> +  Safeguard to avoid blocking the root, when max_password_errors
> +  limit is reached.
> +
> +  Currently, we allow password errors for superuser on localhost.
> +
> +  @return true, if password errors should be ignored, and user should not be locked.
> +*/
> +static bool ignore_max_password_errors(const ACL_USER *acl_user)
> +{
> + const char *host= acl_user->host.hostname;
> + return (acl_user->access & SUPER_ACL)
> +   && (!strcasecmp(host, "localhost") ||
> +       !strcmp(host, "127.0.0.1") ||
> +       !strcmp(host, "::1"));
> +}

I don't like special cases :(

May be after implementing `unix_socket OR mysql_native_password`
feature, this special case could be removed?

>  /**
>     Finds acl entry in user database for authentication purposes.
>  
> @@ -12341,6 +12359,21 @@ static bool find_mpvio_user(MPVIO_EXT *mpvio)
>    mysql_mutex_lock(&acl_cache->lock);
>  
>    ACL_USER *user= find_user_or_anon(sctx->host, sctx->user, sctx->ip);
> +
> +  if (user && max_password_errors && user->password_errors >= max_password_errors)
> +  {
> +    if (ignore_max_password_errors(user))
> +      user->password_errors= 0;

add a comment here, like // skip a second mutex lock in handle_password_errors()

> +    else
> +    {
> +      mysql_mutex_unlock(&acl_cache->lock);
> +      my_error(ER_USER_IS_BLOCKED, MYF(0));
> +      general_log_print(mpvio->auth_info.thd, COM_CONNECT,
> +        ER_THD(mpvio->auth_info.thd, ER_USER_IS_BLOCKED));
> +      DBUG_RETURN(1);
> +    }
> +  }
> +
>    if (user)
>      mpvio->acl_user= user->copy(mpvio->auth_info.thd->mem_root);
>  
> @@ -13305,6 +13370,8 @@ bool acl_authenticate(THD *thd, uint com_change_user_pkt_len)
>        break;
>      case CR_AUTH_USER_CREDENTIALS:
>        errors.m_authentication= 1;
> +      if (thd->password && max_password_errors)

add && !mpvio->make_it_fail here (see how make_it_fail is set).

> +        handle_password_errors(acl_user->user.str, acl_user->host.hostname, PASSWD_ERROR_INCREMENT);
>        break;
>      case CR_ERROR:
>      default:

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx