Re: [Maria-developers] ca4ef7185da: MDEV-9245 password "reuse prevention" validation plugin
Hi, Oleksandr! you forgot to check in the result file On Sep 07, Oleksandr Byelkin wrote:
revision-id: ca4ef7185da (mariadb-10.6.1-79-gca4ef7185da) parent(s): 271afbc88e4 author: Oleksandr Byelkin committer: Oleksandr Byelkin timestamp: 2021-09-07 14:35:05 +0200 message:
MDEV-9245 password "reuse prevention" validation plugin
diff --git a/plugin/reuse_password_check/CMakeLists.txt b/plugin/reuse_password_check/CMakeLists.txt --- /dev/null +++ b/plugin/reuse_password_check/CMakeLists.txt @@ -0,0 +1,4 @@ + +ADD_DEFINITIONS(-DMYSQL_SERVER)
as I wrote to HF, this should not be needed.
+ +MYSQL_ADD_PLUGIN(reuse_password_check reuse_password_check.c MODULE_ONLY)
Unless you have a reason why this must be "MODULE_ONLY", please remove MODULE_ONLY and try to compile and test with -DPLUGIN_REUSE_PASSWORD_CHECK=STATIC also, I'd suggest "password_reuse_check" not "reuse_password_check".
diff --git a/plugin/reuse_password_check/reuse_password_check.c b/plugin/reuse_password_check/reuse_password_check.c --- /dev/null +++ b/plugin/reuse_password_check/reuse_password_check.c @@ -0,0 +1,236 @@ +/* Copyright (c) 2021, Oleksandr Byelkin and MariaDB + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; version 2 of the License. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program; if not, write to the Free Software + Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1335 USA */ + +#include
+#include +#include +#include +#include
only plugin_password_validation.h should be needed here.
+ + +#include
this might be needed all right
+#include
but this isn't needed either
+ +#define HISTORY_DB_NAME "reuse_password_check_history" + +#define SQL_BUFF_LEN 2048 + +// 0 - unlimited, otherwise number of days to check +static unsigned interval= 0; + +// helping string for bin_to_hex512 +static char digits[]= "0123456789ABCDEF"; + + +/** + Convert string of 512 bits (64 bytes) to hex representation + + @param to pointer to the result puffer + (should be at least 64*2 bytes) + @param str pointer to 512 bits (64 bytes string) +*/ + +static void bin_to_hex512(char *to, const unsigned char *str) +{ + const unsigned char *str_end= str + (512/8); + for (; str != str_end; ++str) + { + *to++= digits[((uchar) *str) >> 4]; + *to++= digits[((uchar) *str) & 0x0F]; + } +} + + +/** + Send SQL error as ER_UNKNOWN_ERROR for information + + @param mysql Connection handler +*/ + +static void report_sql_error(MYSQL *mysql) +{ + my_printf_error(ER_UNKNOWN_ERROR, "[%d] %s", ME_WARNING, + mysql_errno(mysql), mysql_error(mysql));
may be include plugin name here?
+} + + +/** + Create the history of passwords table for this plugin. + + @param mysql Connection handler + + @retval 1 - Error + @retval 0 - OK +*/ + +static int create_table(MYSQL *mysql) +{ + if (mysql_real_query(mysql, + // 512/8 = 64 + STRING_WITH_LEN("CREATE TABLE mysql." HISTORY_DB_NAME + " ( hash binary(64)," + " time timestamp,"
make it "default current_timestamp", just to be explicit. even though it's implicitly default current_timestamp already.
+ " primary key (hash), index tm (time) )" + " ENGINE=Aria"))) + { + report_sql_error(mysql); + return 1; + } + return 0; +} + + +/** + Run this query and create table if needed + + @param mysql Connection handler + @param query The query to run + @param len length of the query text + + @retval 1 - Error + @retval 0 - OK +*/ + +static int run_query_with_table_creation(MYSQL *mysql, const char *query, + size_t len) +{ + if (unlikely(mysql_real_query(mysql, query, len))) + { + unsigned int rc= mysql_errno(mysql); + if (rc != ER_NO_SUCH_TABLE) + { + // suppress this error in case of try to add the same password twice + if (rc != ER_DUP_ENTRY) + report_sql_error(mysql); + return 1; + } + if (create_table(mysql)) + return 1; + if (unlikely(mysql_real_query(mysql, query, len))) + { + report_sql_error(mysql); + return 1; + } + } + return 0; +} + + +/** + Password validator + + @param username User name (part of whole login name) + @param password Password to validate + @param hostname Host name (part of whole login name) + + @retval 1 - Password is not OK or an error happened + @retval 0 - Password is OK +*/ + +static int validate(const MYSQL_CONST_LEX_STRING *username, + const MYSQL_CONST_LEX_STRING *password, + const MYSQL_CONST_LEX_STRING *hostname) +{ + MYSQL *mysql= NULL; + size_t key_len= username->length + password->length + hostname->length; + size_t buff_len= (key_len > SQL_BUFF_LEN ? key_len : SQL_BUFF_LEN); + size_t len; + char *buff= malloc(buff_len); + unsigned char hash[512/8]; + char escaped_hash[512/8*2 + 1]; + if (!buff) + return 1; + + mysql= mysql_init(NULL); + if (!mysql) + { + free(buff); + return 1; + } + + memcpy(buff, hostname->str, hostname->length); + memcpy(buff + hostname->length, username->str, username->length); + memcpy(buff + hostname->length + username->length, password->str, + password->length); + buff[key_len]= 0; + bzero(hash, sizeof(hash)); + my_sha512(hash, buff, key_len); + if (mysql_real_connect_local(mysql, NULL, NULL, NULL, 0) == NULL) + goto sql_error; + + if (interval) + { + // trim the table + len= snprintf(buff, buff_len, + "DELETE FROM mysql." HISTORY_DB_NAME + " WHERE time < DATE_SUB(NOW(), interval %d day)", + interval); + if (unlikely(run_query_with_table_creation(mysql, buff, len))) + goto sql_error; + } + + bin_to_hex512(escaped_hash, hash); + escaped_hash[512/8*2]= '\0'; + len= snprintf(buff, buff_len, + "INSERT INTO mysql." HISTORY_DB_NAME "(hash) " + "values (x'%s')", + escaped_hash); + if (unlikely(run_query_with_table_creation(mysql, buff, len))) + goto sql_error; + + free(buff); + mysql_close(mysql); + return 0; // OK + +sql_error: + free(buff); + if (mysql) + mysql_close(mysql); + return 1; // Error +} + +static MYSQL_SYSVAR_UINT(interval, interval, PLUGIN_VAR_RQCMDARG, + "How old (in days) passwords to check (0 means unlimited)", NULL, NULL,
better "Password history retention period in days (0 means unlimited)"
+ 0, 0, 365*100, 1); + + +static struct st_mysql_sys_var* sysvars[]= { + MYSQL_SYSVAR(interval), + NULL +}; + +static struct st_mariadb_password_validation info= +{ + MariaDB_PASSWORD_VALIDATION_INTERFACE_VERSION, + validate +}; + +maria_declare_plugin(simple_password_check)
^^^ must be the same name as in CMakeLists.txt, otherwise static linking won't work.
+{ + MariaDB_PASSWORD_VALIDATION_PLUGIN, + &info, + "reuse_password_check", + "Oleksandr Byelkin", + "Reusage of password check",
Better "Password reuse check" or, as it's a description, "Prevent password reuse"
+ PLUGIN_LICENSE_GPL, + NULL, + NULL, + 0x0100, + NULL, + sysvars, + "1.0", + MariaDB_PLUGIN_MATURITY_ALPHA +} +maria_declare_plugin_end;
diff --git a/mysql-test/suite/plugins/t/reuse_password_check.test b/mysql-test/suite/plugins/t/reuse_password_check.test --- /dev/null +++ b/mysql-test/suite/plugins/t/reuse_password_check.test @@ -0,0 +1,58 @@ +--source include/not_embedded.inc + +if (!$REUSE_PASSWORD_CHECK_SO) { + skip No REUSE_PASSWORD_CHECK plugin; +} + +install soname "reuse_password_check"; + +set @save_reuse_password_check_interval= @@reuse_password_check_interval; + +set global reuse_password_check_interval= 0; + +--echo # Default value (sould be unlimited i.e. 0) +SHOW GLOBAL VARIABLES like "reuse_password_check%"; + +--echo # insert user +grant select on *.* to user_name@localhost identified by 'test_pwd';
what about sql mode that doesn't allow to create users with grant? I don't see where you've disabled that.
+ +#--error ER_NOT_VALID_PASSWORD +#grant select on *.* to user_name@localhost identified by 'test_pwd'; +#show warnings;
why is that commented out?
+ +--error ER_CANNOT_USER +alter user user_name@localhost identified by 'test_pwd'; +show warnings; + +# Plugin does not work for it +#--error ER_NOT_VALID_PASSWORD +#SET PASSWORD FOR user_name@localhost = PASSWORD('test_pwd'); + +--echo # check exparation + +set global reuse_password_check_interval= 10; + +--error ER_CANNOT_USER +alter user user_name@localhost identified by 'test_pwd'; +show warnings; +select hex(hash) from mysql.reuse_password_check_history; + +--echo # emulate old password +update mysql.reuse_password_check_history set time= date_sub(now(), interval +11 day); + +alter user user_name@localhost identified by 'test_pwd'; +show warnings; + +drop user user_name@localhost; + +show create table mysql.reuse_password_check_history; +select count(*) from mysql.reuse_password_check_history; + +drop table mysql.reuse_password_check_history; + +set @save_reuse_password_check_interval= @@reuse_password_check_interval; + +set global reuse_password_check_interval= @save_reuse_password_check_interval; + +uninstall plugin reuse_password_check;
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi, Serg
On Tue, Sep 7, 2021 at 7:22 PM Sergei Golubchik
Hi, Oleksandr!
you forgot to check in the result file
On Sep 07, Oleksandr Byelkin wrote:
revision-id: ca4ef7185da (mariadb-10.6.1-79-gca4ef7185da) parent(s): 271afbc88e4 author: Oleksandr Byelkin committer: Oleksandr Byelkin timestamp: 2021-09-07 14:35:05 +0200 message:
MDEV-9245 password "reuse prevention" validation plugin
diff --git a/plugin/reuse_password_check/CMakeLists.txt b/plugin/reuse_password_check/CMakeLists.txt --- /dev/null +++ b/plugin/reuse_password_check/CMakeLists.txt @@ -0,0 +1,4 @@ + +ADD_DEFINITIONS(-DMYSQL_SERVER)
as I wrote to HF, this should not be needed.
So far it is needed, otherwise I got this: plugin/password_reuse_check/password_reuse_check.c: In function ‘validate’: plugin/password_reuse_check/password_reuse_check.c:170:7: error: implicit declaration of function ‘mysql_real_connect_local’; did you mean ‘mysql_real_connect_cont’? [-Werror=implicit-function-declaration] 170 | if (mysql_real_connect_local(mysql, NULL, NULL, NULL, 0) == NULL) | ^~~~~~~~~~~~~~~~~~~~~~~~ | mysql_real_connect_cont plugin/password_reuse_check/password_reuse_check.c:170:60: error: comparison between pointer and integer [-Werror] 170 | if (mysql_real_connect_local(mysql, NULL, NULL, NULL, 0) == NULL) | ^~ cc1: all warnings being treated as errors [1145/1618] Building CXX object plugin...ocket.dir/handlersocket/database.cpp.o ninja: build stopped: subcommand failed.
+ +MYSQL_ADD_PLUGIN(reuse_password_check reuse_password_check.c MODULE_ONLY)
Unless you have a reason why this must be "MODULE_ONLY", please remove MODULE_ONLY and try to compile and test with -DPLUGIN_REUSE_PASSWORD_CHECK=STATIC
OK
also, I'd suggest "password_reuse_check" not "reuse_password_check".
OK
diff --git a/plugin/reuse_password_check/reuse_password_check.c b/plugin/reuse_password_check/reuse_password_check.c --- /dev/null +++ b/plugin/reuse_password_check/reuse_password_check.c @@ -0,0 +1,236 @@ +/* Copyright (c) 2021, Oleksandr Byelkin and MariaDB + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; version 2 of the License. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program; if not, write to the Free Software + Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1335 USA */ + +#include
+#include +#include +#include +#include only plugin_password_validation.h should be needed here.
OK, I removed unlikely() and added only needed defines with comment for what it is needed
+ + +#include
this might be needed all right
+#include
but this isn't needed either
OK
+ +#define HISTORY_DB_NAME "reuse_password_check_history" + +#define SQL_BUFF_LEN 2048 + +// 0 - unlimited, otherwise number of days to check +static unsigned interval= 0; + +// helping string for bin_to_hex512 +static char digits[]= "0123456789ABCDEF"; + + +/** + Convert string of 512 bits (64 bytes) to hex representation + + @param to pointer to the result puffer + (should be at least 64*2 bytes) + @param str pointer to 512 bits (64 bytes string) +*/ + +static void bin_to_hex512(char *to, const unsigned char *str) +{ + const unsigned char *str_end= str + (512/8); + for (; str != str_end; ++str) + { + *to++= digits[((uchar) *str) >> 4]; + *to++= digits[((uchar) *str) & 0x0F]; + } +} + + +/** + Send SQL error as ER_UNKNOWN_ERROR for information + + @param mysql Connection handler +*/ + +static void report_sql_error(MYSQL *mysql) +{ + my_printf_error(ER_UNKNOWN_ERROR, "[%d] %s", ME_WARNING, + mysql_errno(mysql), mysql_error(mysql));
may be include plugin name here?
done
+} + + +/** + Create the history of passwords table for this plugin. + + @param mysql Connection handler + + @retval 1 - Error + @retval 0 - OK +*/ + +static int create_table(MYSQL *mysql) +{ + if (mysql_real_query(mysql, + // 512/8 = 64 + STRING_WITH_LEN("CREATE TABLE mysql." HISTORY_DB_NAME + " ( hash binary(64)," + " time timestamp,"
make it "default current_timestamp", just to be explicit. even though it's implicitly default current_timestamp already.
done
+ " primary key (hash), index tm (time) )" + " ENGINE=Aria"))) + { + report_sql_error(mysql); + return 1; + } + return 0; +} + + +/** + Run this query and create table if needed + + @param mysql Connection handler + @param query The query to run + @param len length of the query text + + @retval 1 - Error + @retval 0 - OK +*/ + +static int run_query_with_table_creation(MYSQL *mysql, const char *query, + size_t len) +{ + if (unlikely(mysql_real_query(mysql, query, len))) + { + unsigned int rc= mysql_errno(mysql); + if (rc != ER_NO_SUCH_TABLE) + { + // suppress this error in case of try to add the same password twice + if (rc != ER_DUP_ENTRY) + report_sql_error(mysql); + return 1; + } + if (create_table(mysql)) + return 1; + if (unlikely(mysql_real_query(mysql, query, len))) + { + report_sql_error(mysql); + return 1; + } + } + return 0; +} + + +/** + Password validator + + @param username User name (part of whole login name) + @param password Password to validate + @param hostname Host name (part of whole login name) + + @retval 1 - Password is not OK or an error happened + @retval 0 - Password is OK +*/ + +static int validate(const MYSQL_CONST_LEX_STRING *username, + const MYSQL_CONST_LEX_STRING *password, + const MYSQL_CONST_LEX_STRING *hostname) +{ + MYSQL *mysql= NULL; + size_t key_len= username->length + password->length + hostname->length; + size_t buff_len= (key_len > SQL_BUFF_LEN ? key_len : SQL_BUFF_LEN); + size_t len; + char *buff= malloc(buff_len); + unsigned char hash[512/8]; + char escaped_hash[512/8*2 + 1]; + if (!buff) + return 1; + + mysql= mysql_init(NULL); + if (!mysql) + { + free(buff); + return 1; + } + + memcpy(buff, hostname->str, hostname->length); + memcpy(buff + hostname->length, username->str, username->length); + memcpy(buff + hostname->length + username->length, password->str, + password->length); + buff[key_len]= 0; + bzero(hash, sizeof(hash)); + my_sha512(hash, buff, key_len); + if (mysql_real_connect_local(mysql, NULL, NULL, NULL, 0) == NULL) + goto sql_error; + + if (interval) + { + // trim the table + len= snprintf(buff, buff_len, + "DELETE FROM mysql." HISTORY_DB_NAME + " WHERE time < DATE_SUB(NOW(), interval %d day)", + interval); + if (unlikely(run_query_with_table_creation(mysql, buff, len))) + goto sql_error; + } + + bin_to_hex512(escaped_hash, hash); + escaped_hash[512/8*2]= '\0'; + len= snprintf(buff, buff_len, + "INSERT INTO mysql." HISTORY_DB_NAME "(hash) " + "values (x'%s')", + escaped_hash); + if (unlikely(run_query_with_table_creation(mysql, buff, len))) + goto sql_error; + + free(buff); + mysql_close(mysql); + return 0; // OK + +sql_error: + free(buff); + if (mysql) + mysql_close(mysql); + return 1; // Error +} + +static MYSQL_SYSVAR_UINT(interval, interval, PLUGIN_VAR_RQCMDARG, + "How old (in days) passwords to check (0 means unlimited)", NULL, NULL,
better "Password history retention period in days (0 means unlimited)"
OK
+ 0, 0, 365*100, 1); + + +static struct st_mysql_sys_var* sysvars[]= { + MYSQL_SYSVAR(interval), + NULL +}; + +static struct st_mariadb_password_validation info= +{ + MariaDB_PASSWORD_VALIDATION_INTERFACE_VERSION, + validate +}; + +maria_declare_plugin(simple_password_check)
^^^ must be the same name as in CMakeLists.txt, otherwise static linking won't work.
fixed
+{ + MariaDB_PASSWORD_VALIDATION_PLUGIN, + &info, + "reuse_password_check", + "Oleksandr Byelkin", + "Reusage of password check",
Better "Password reuse check" or, as it's a description, "Prevent password reuse"
done
+ PLUGIN_LICENSE_GPL, + NULL, + NULL, + 0x0100, + NULL, + sysvars, + "1.0", + MariaDB_PLUGIN_MATURITY_ALPHA +} +maria_declare_plugin_end;
diff --git a/mysql-test/suite/plugins/t/reuse_password_check.test b/mysql-test/suite/plugins/t/reuse_password_check.test --- /dev/null +++ b/mysql-test/suite/plugins/t/reuse_password_check.test @@ -0,0 +1,58 @@ +--source include/not_embedded.inc + +if (!$REUSE_PASSWORD_CHECK_SO) { + skip No REUSE_PASSWORD_CHECK plugin; +} + +install soname "reuse_password_check"; + +set @save_reuse_password_check_interval= @@reuse_password_check_interval; + +set global reuse_password_check_interval= 0; + +--echo # Default value (sould be unlimited i.e. 0) +SHOW GLOBAL VARIABLES like "reuse_password_check%"; + +--echo # insert user +grant select on *.* to user_name@localhost identified by 'test_pwd';
what about sql mode that doesn't allow to create users with grant? I don't see where you've disabled that.
good question; I have no idea why it works, now I am trying to figure it out.
+ +#--error ER_NOT_VALID_PASSWORD +#grant select on *.* to user_name@localhost identified by 'test_pwd'; +#show warnings;
why is that commented out?
fixed
+ +--error ER_CANNOT_USER +alter user user_name@localhost identified by 'test_pwd'; +show warnings; + +# Plugin does not work for it +#--error ER_NOT_VALID_PASSWORD +#SET PASSWORD FOR user_name@localhost = PASSWORD('test_pwd'); + +--echo # check exparation + +set global reuse_password_check_interval= 10; + +--error ER_CANNOT_USER +alter user user_name@localhost identified by 'test_pwd'; +show warnings; +select hex(hash) from mysql.reuse_password_check_history; + +--echo # emulate old password +update mysql.reuse_password_check_history set time= date_sub(now(), interval +11 day); + +alter user user_name@localhost identified by 'test_pwd'; +show warnings; + +drop user user_name@localhost; + +show create table mysql.reuse_password_check_history; +select count(*) from mysql.reuse_password_check_history; + +drop table mysql.reuse_password_check_history; + +set @save_reuse_password_check_interval= @@reuse_password_check_interval; + +set global reuse_password_check_interval= @save_reuse_password_check_interval; + +uninstall plugin reuse_password_check;
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
participants (2)
-
Oleksandr Byelkin -
Sergei Golubchik