Hi, Vladislav! On Jan 04, Vladislav Vaintroub wrote:

revision-id: 82d7e2094c8 (mariadb-10.4.1-41-g82d7e2094c8) parent(s): c4ec6bb69ec author: Vladislav Vaintroub <wlad@mariadb.com> committer: Vladislav Vaintroub <wlad@mariadb.com> 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@mariadb.org