Hi, Vladislav!
On Jan 04, Vladislav Vaintroub wrote:
> revision-id: 82d7e2094c8 (mariadb-10.4.1-41-g82d7e2094c8)
> parent(s): c4ec6bb69ec
> author: Vladislav Vaintroub <wlad(a)mariadb.com>
> committer: Vladislav Vaintroub <wlad(a)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(a)mariadb.org
