From 9536bd657b2c6d3228009189c7c17a028342b8f5 Mon Sep 17 00:00:00 2001 From: Georgi Kodinov Date: Wed, 25 Mar 2009 15:37:21 +0200 Subject: [PATCH 1/2] Bug#43748: crash when non-super user tries to kill the replication threads (Pushing for Azundris) We allow security-contexts with NULL users (for system-threads and for unauthenticated users). If a non-SUPER-user tried to KILL such a thread, we tried to compare the user-fields to see whether they owned that thread. Comparing against NULL was not a good idea. If KILLer does not have SUPER-privilege, we specifically check whether both KILLer and KILLee have a non-NULL user before testing for string- equality. If either is NULL, we reject the KILL. mysql-test/r/rpl_temporary.result: Try to have a non-SUPER user KILL a system thread. mysql-test/t/rpl_temporary.test: Try to have a non-SUPER user KILL a system thread. sql/sql_parse.cc: Make sure security contexts of both KILLer *and* KILLee are non-NULL before testing for string-equality! --- mysql-test/r/rpl_temporary.result | 18 ++++++++++++++++ mysql-test/t/rpl_temporary.test | 36 +++++++++++++++++++++++++++++++ sql/sql_parse.cc | 21 +++++++++++++++++- 3 files changed, 74 insertions(+), 1 deletion(-) diff --git a/mysql-test/r/rpl_temporary.result b/mysql-test/r/rpl_temporary.result index 15c069ab68d..5eefced7564 100644 --- a/mysql-test/r/rpl_temporary.result +++ b/mysql-test/r/rpl_temporary.result @@ -4,6 +4,24 @@ reset master; reset slave; drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9; start slave; +FLUSH PRIVILEGES; +drop table if exists t999; +create temporary table t999( +id int, +user char(255), +host char(255), +db char(255), +Command char(255), +time int, +State char(255), +info char(255) +); +LOAD DATA INFILE "./tmp/bl_dump_thread_id" into table t999; +drop table t999; +GRANT USAGE ON *.* TO user43748@localhost; +KILL `select id from information_schema.processlist where command='Binlog Dump'`; +ERROR HY000: You are not owner of thread `select id from information_schema.processlist where command='Binlog Dump'` +DROP USER user43748@localhost; reset master; SET @save_select_limit=@@session.sql_select_limit; SET @@session.sql_select_limit=10, @@session.pseudo_thread_id=100; diff --git a/mysql-test/t/rpl_temporary.test b/mysql-test/t/rpl_temporary.test index 516f3a026c9..366fdf00c56 100644 --- a/mysql-test/t/rpl_temporary.test +++ b/mysql-test/t/rpl_temporary.test @@ -3,6 +3,42 @@ source include/add_anonymous_users.inc; source include/master-slave.inc; +# +# Bug#43748: crash when non-super user tries to kill the replication threads +# + +--connection master +save_master_pos; + +--connection slave +sync_with_master; + +--connection slave +FLUSH PRIVILEGES; + +# in 5.0, we need to do some hocus pocus to get a system-thread ID (-> $id) +--source include/get_binlog_dump_thread_id.inc + +# make a non-privileged user on slave. try to KILL system-thread as her. +GRANT USAGE ON *.* TO user43748@localhost; + +--connect (mysqltest_2_con,localhost,user43748,,test,$SLAVE_MYPORT,) +--connection mysqltest_2_con + +--replace_result $id "`select id from information_schema.processlist where command='Binlog Dump'`" +--error ER_KILL_DENIED_ERROR +eval KILL $id; + +--disconnect mysqltest_2_con + +--connection slave + +DROP USER user43748@localhost; + +--connection master + + + # Clean up old slave's binlogs. # The slave is started with --log-slave-updates # and this test does SHOW BINLOG EVENTS on the slave's diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index 2297283c92d..33adcfe3342 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -7386,8 +7386,27 @@ void kill_one_thread(THD *thd, ulong id, bool only_kill_query) VOID(pthread_mutex_unlock(&LOCK_thread_count)); if (tmp) { + + /* + If we're SUPER, we can KILL anything, including system-threads. + No further checks. + + thd..user could in theory be NULL while we're still in + "unauthenticated" state. This is more a theoretical case. + + tmp..user will be NULL for system threads (cf Bug#43748). + We need to check so Jane Random User doesn't crash the server + when trying to kill a) system threads or b) unauthenticated + users' threads. + + If user of both killer and killee are non-null, proceed with + slayage if both are string-equal. + */ + if ((thd->security_ctx->master_access & SUPER_ACL) || - !strcmp(thd->security_ctx->user, tmp->security_ctx->user)) + ((thd->security_ctx->user != NULL) && + (tmp->security_ctx->user != NULL) && + !strcmp(thd->security_ctx->user, tmp->security_ctx->user))) { tmp->awake(only_kill_query ? THD::KILL_QUERY : THD::KILL_CONNECTION); error=0; From e46c139dd81081aceb27902ee4b632904cae292b Mon Sep 17 00:00:00 2001 From: "Tatiana A. Nurnberg" Date: Wed, 25 Mar 2009 17:10:27 +0100 Subject: [PATCH 2/2] Bug#43748: crash when non-super user tries to kill the replication threads Fine-tuning. Broke out comparison into method by suggestion of Davi. Clarified comments. Reverting test-case which I find too brittle; proper test case in 5.1+. --- mysql-test/r/rpl_temporary.result | 18 ---------------- mysql-test/t/rpl_temporary.test | 36 ------------------------------- sql/sql_class.cc | 7 ++++++ sql/sql_class.h | 1 + sql/sql_parse.cc | 17 +++++++-------- 5 files changed, 16 insertions(+), 63 deletions(-) diff --git a/mysql-test/r/rpl_temporary.result b/mysql-test/r/rpl_temporary.result index 5eefced7564..15c069ab68d 100644 --- a/mysql-test/r/rpl_temporary.result +++ b/mysql-test/r/rpl_temporary.result @@ -4,24 +4,6 @@ reset master; reset slave; drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9; start slave; -FLUSH PRIVILEGES; -drop table if exists t999; -create temporary table t999( -id int, -user char(255), -host char(255), -db char(255), -Command char(255), -time int, -State char(255), -info char(255) -); -LOAD DATA INFILE "./tmp/bl_dump_thread_id" into table t999; -drop table t999; -GRANT USAGE ON *.* TO user43748@localhost; -KILL `select id from information_schema.processlist where command='Binlog Dump'`; -ERROR HY000: You are not owner of thread `select id from information_schema.processlist where command='Binlog Dump'` -DROP USER user43748@localhost; reset master; SET @save_select_limit=@@session.sql_select_limit; SET @@session.sql_select_limit=10, @@session.pseudo_thread_id=100; diff --git a/mysql-test/t/rpl_temporary.test b/mysql-test/t/rpl_temporary.test index 366fdf00c56..516f3a026c9 100644 --- a/mysql-test/t/rpl_temporary.test +++ b/mysql-test/t/rpl_temporary.test @@ -3,42 +3,6 @@ source include/add_anonymous_users.inc; source include/master-slave.inc; -# -# Bug#43748: crash when non-super user tries to kill the replication threads -# - ---connection master -save_master_pos; - ---connection slave -sync_with_master; - ---connection slave -FLUSH PRIVILEGES; - -# in 5.0, we need to do some hocus pocus to get a system-thread ID (-> $id) ---source include/get_binlog_dump_thread_id.inc - -# make a non-privileged user on slave. try to KILL system-thread as her. -GRANT USAGE ON *.* TO user43748@localhost; - ---connect (mysqltest_2_con,localhost,user43748,,test,$SLAVE_MYPORT,) ---connection mysqltest_2_con - ---replace_result $id "`select id from information_schema.processlist where command='Binlog Dump'`" ---error ER_KILL_DENIED_ERROR -eval KILL $id; - ---disconnect mysqltest_2_con - ---connection slave - -DROP USER user43748@localhost; - ---connection master - - - # Clean up old slave's binlogs. # The slave is started with --log-slave-updates # and this test does SHOW BINLOG EVENTS on the slave's diff --git a/sql/sql_class.cc b/sql/sql_class.cc index 74bc669a049..aa796d6acbd 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -2144,6 +2144,13 @@ void Security_context::skip_grants() } +bool Security_context::user_matches(Security_context *them) +{ + return ((user != NULL) && (them->user != NULL) && + !strcmp(user, them->user)); +} + + /**************************************************************************** Handling of open and locked tables states. diff --git a/sql/sql_class.h b/sql/sql_class.h index 3e3dfcd08fa..47dbac0f17b 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -985,6 +985,7 @@ public: { return (*priv_host ? priv_host : (char *)"%"); } + bool user_matches(Security_context *); }; diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index 33adcfe3342..c2d789b30b5 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -7391,22 +7391,21 @@ void kill_one_thread(THD *thd, ulong id, bool only_kill_query) If we're SUPER, we can KILL anything, including system-threads. No further checks. - thd..user could in theory be NULL while we're still in - "unauthenticated" state. This is more a theoretical case. + KILLer: thd->security_ctx->user could in theory be NULL while + we're still in "unauthenticated" state. This is a theoretical + case (the code suggests this could happen, so we play it safe). - tmp..user will be NULL for system threads (cf Bug#43748). + KILLee: tmp->security_ctx->user will be NULL for system threads. We need to check so Jane Random User doesn't crash the server - when trying to kill a) system threads or b) unauthenticated - users' threads. + when trying to kill a) system threads or b) unauthenticated users' + threads (Bug#43748). - If user of both killer and killee are non-null, proceed with + If user of both killer and killee are non-NULL, proceed with slayage if both are string-equal. */ if ((thd->security_ctx->master_access & SUPER_ACL) || - ((thd->security_ctx->user != NULL) && - (tmp->security_ctx->user != NULL) && - !strcmp(thd->security_ctx->user, tmp->security_ctx->user))) + thd->security_ctx->user_matches(tmp->security_ctx)) { tmp->awake(only_kill_query ? THD::KILL_QUERY : THD::KILL_CONNECTION); error=0;