MySQL Bugs: #43748: crash when non-super user tries to kill the replication threads

Bug #43748	crash when non-super user tries to kill the replication threads
Submitted:	19 Mar 12:20	Modified:	13 Apr 21:27
Reporter:	Shane Bester
Status:	Closed
Category:	Server: General	Severity:	S1 (Critical)
Version:	4.1.24, 5.0.78, 5.1.32, 6.0.9	OS:	Any
Assigned to:	Tatjana A. Nuernberg	Target Version:	5.0.80,5.1.34
Tags:	DoS, KILL, kill_one_thread
Triage:	Triaged: D1 (Critical)

Description:
we're seeing crashes in kill_one_thread() when somebody runs KILL <id> under high load

(gdb) bt
#0 0x00000034aed09737 in pthread_kill 
#1 0x000000000072848b in write_core ()
#2 0x00000000005aa10f in handle_segfault ()
#3 <signal handler called>
#4 0x00000000005c4136 in kill_one_thread ()
#5 0x00000000005bf043 in mysql_execute_command ()
#6 0x00000000005c77b9 in mysql_parse ()
#7 0x00000000005bbd97 in dispatch_command ()
#8 0x00000000005b9bd3 in handle_one_connection ()
#9 0x00000034aed0610a in start_thread () 
#10 0x00000034ad8c68c3 in clone () 

I will attach an analysis of the assembler and C code, to tell exactly which line crashed
in kill_one_thread().

How to repeat:
under high load, kill a connection

Suggested fix:
no testcase yet.

how to repeat:

setup a slave which runs..
create yourself a user on the slave: 
grant usage on *.* to 'shane'@'127.0.0.1';
run mysql -ushane -h127.0.0.1
kill the replication thread using kill <id>

you can know the replication thread id by logging in as root and checking,
since show processlist as a normal user will not show it.

The cause is that kill_one_thread forgets to check tmp->slave_thread is true...
therefore strcmp is passed a null pointer here:

if ((thd->security_ctx->master_access & SUPER_ACL) ||
	!strcmp(thd->security_ctx->user, tmp->security_ctx->user))

A workaround is to GRANT the SUPER privilege to any users who need to do manual 
KILL statements.

Security_context defined in sql/sql_class.h, 963ff in 5.0:
"user - user of the client, set to NULL until the user has been read from the
connection"

I haven't analyzed yet whether the connection will already show up in SHOW PROCESSLIST at
that point etc., that is, if we could in theory race this from a 2nd connexion (ie by
predicting the next ID and KILLing that). Unless we have some special mojo in place for
that, we can safely say that NULL is a legal (and to a degree, to be expected) value for
user *throughout*, even outside of rpl concerns.

Anyway, for rpl, we do the standard Security_context::init() which NULLs the relevant
fields (and then a Security_context::skip_grants() that inits the "*other* fields" :).
sql/slave.cc::init_slave_thread()@2909ff

Crash would occur in sql/sql_parse.cc::kill_one_thread()@7344 as reported.

What KILLs do we allow here, both SYSTEM_THREAD_SLAVE_SQL and SYSTEM_THREAD_SLAVE_IO?
What's the dealio with other (non-slave) system-threads, such as
SYSTEM_THREAD_DELAYED_INSERT?

The docs have this to say:

"If you have the PROCESS privilege, you can see all threads. If you have the SUPER
privilege, you can kill all threads and statements.

...

An INSERT DELAYED thread quickly flushes (inserts) all rows it has in memory and then
terminates"

So by that taken, not only is anything KILLable (though it might not always be a good
idea, say in the middle of REPAIR/OPTIMIZE TABLE); but DELAYED is named specifically.

http://dev.mysql.com/doc/refman/5.0/en/kill.html

we definitely expect NULL-values for user, in rpl-context and outside:

sql/sql_show.cc::mysqld_list_processes()@1351:

        thd_info->user= thd->strdup(tmp_sctx->user ? tmp_sctx->user :
                                    (tmp->system_thread ?
                                     "system user" : "unauthenticated user"));

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/70359

2771 Georgi Kodinov	2009-03-25
      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!

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/70377

2772 Tatiana A. Nurnberg	2009-03-25
      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+.

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/70391

2842 Tatiana A. Nurnberg	2009-03-25 [merge]
      Bug#43748: crash when non-super user tries to kill the replication threads
      
      manual merge. also adds test specific to 5.1+
     @ mysql-test/suite/rpl/r/rpl_temporary.result
        show that a non-privileged user trying to
        kill system-threads no longer crashes the
        server. test in 5.1+ only.
     @ mysql-test/suite/rpl/t/rpl_temporary.test
        show that a non-privileged user trying to
        kill system-threads no longer crashes the
        server. test in 5.1+ only.
     @ sql/sql_class.cc
        manual merge
     @ sql/sql_class.h
        manual merge
     @ sql/sql_parse.cc
        manual merge

queued for 5.0.80, 5.1.34, 6.0.11 in pe-stage

queued for 5.0.80, 5.1.34, 6.0.11 in pe-stage

Pushed into 5.0.80 (revid:joro@sun.com-20090327142516-55gumdxj39z6eijj) (version source
revid:ramil@mysql.com-20090325175042-v226810cljvzr5yx) (merge vers: 5.0.80) (pib:6)

Pushed into 5.1.34 (revid:joro@sun.com-20090327143448-wuuuycetc562ty6o) (version source
revid:joro@sun.com-20090327121550-v894szaf6dbk0aki) (merge vers: 5.1.34) (pib:6)

Noted in 5.0.80, 5.1.34 changelogs.

An attempt by a user who did not have the SUPER privilege to kill a
system thread could cause a server crash.

Setting report to NDI pending push into 6.0.x.

Pushed into 6.0.11-alpha (revid:alik@sun.com-20090413084402-snnrocwzktcl88ny) (version
source revid:joro@sun.com-20090327121728-u9hp8huig35gnddy) (merge vers: 6.0.11-alpha)
(pib:6)

Noted in 6.0.11 changelog.

Pushed into 5.1.34-ndb-6.2.18 (revid:jonas@mysql.com-20090508185236-p9b3as7qyauybefl)
(version source revid:jonas@mysql.com-20090508185236-p9b3as7qyauybefl) (merge vers:
5.1.34-ndb-6.2.18) (pib:6)

Pushed into 5.1.34-ndb-6.3.25 (revid:jonas@mysql.com-20090509063138-1u3q3v09wnn2txyt)
(version source revid:jonas@mysql.com-20090509063138-1u3q3v09wnn2txyt) (merge vers:
5.1.34-ndb-6.3.25) (pib:6)

Pushed into 5.1.34-ndb-7.0.6 (revid:jonas@mysql.com-20090509154927-im9a7g846c6u1hzc)
(version source revid:jonas@mysql.com-20090509154927-im9a7g846c6u1hzc) (merge vers:
5.1.34-ndb-7.0.6) (pib:6)

Noted in 5.0.74sp1 changelog.