MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Mattias Jonsson Date:November 7 2007 10:38pm
Subject:Re: bk commit into 5.1 tree (mattiasj:1.2605) BUG#32091
View as plain text  
Hi,

Thank you for the good review.

I will revise the patch after some test and clarifications.

I have some comments inline.

/Mattias

Ingo Strüwing wrote:
> Hi Mattias,
> 
> mattiasj@stripped, 06.11.2007 18:09:
> ...
>> ChangeSet@stripped, 2007-11-06 18:09:21+01:00, mattiasj@mattiasj-laptop.(none) +3
> -0
>>   Bug#32091: Security breach via directory changes
>>   
>>   Problem: the table's INDEX and DATA DIR was taken
>>     directly from the tables first partition,
>>     and opening for a rename attack similar to
>>     bug#32111 when ALTER TABLE REMOVE PARTITIONING
> 
> I seem to be unable to get the exact meaning. Do you mean:
> "the ... DIR was taken from the ... first partition. This allows for a
> rename attack ..."?
> 

Yes, that is what I mean, do you have any good proposal how to write it 
so that it is easier to understand?

for clarification:
the SQL-command explanation:
USE securedb;
CREATE TABLE t1 (..);
USE test;
CREATE TABLE t1 (..) PARTITION BY ..
   PARTITION ..
   DATA DIR 'DATADIR/securedb'
   INDEX DIR 'DATADIR/securedb'
   PARTITION...
   ...
);
/*
Now we have the partitions data and index files in the securedb-database 
directory (t1#P#p0.MYD and .MYI files)
this is possible because there is no check on the directory level (if 
the operating system allows us, then mysqld does)
*/
SHOW CREATE TABLE t1;
/*
before the patch it would return DATA/INDEX DIR for BOTH the table AND 
partition, after the patch ONLY for partitions.
Which is enough to do a overwrite attack
*/
ALTER TABLE t1 REMOVE PARTITIONING
/*
before the patch it would create a table with DATA/INDEX DIR = 
'DATADIR/mysql', but after it would create a table without DATA/INDEX 
DIR (an ordinary table in the default directory for that database).
With the patch for bug#32111 it would fail when creating the non 
partitioned table, since it introduces a check on symlinked files.
*/

>>   Solution: Silently ignore (like some other storage
>>     engines) the INDEX/DATA DIR for the table.
>>     (Only use INDEX/DATA DIR for each partitions,
>>     not for the whole table)
> 
> Ok. I agree with this. Though I would wish we could have DIRECTORY
> handling like I mentioned in the bug report. But if we go that
> direction, then it is a different task, not to be added to this bugfix.
> 
> When looking at the tests, it seems like you was able to create a table
> securedb.t1 and test.t1. This seems to say to me that the INDEX/DATA DIR
> for the table is _not_ taken from the first partition (though the code
> seems to say it is). Can you please resolve this apparent contradiction?
> If I understand correctly, the problem did only exist in ALTER TABLE
> (update_create_info()).
> 
Creating the tables is done and it after the both CREATE statements it 
would look like this:
securedb:
t1.frm <- securedb.t1
t1.MYD <- securedb.t1
t1.MYI <- securedb.t1
t1#P#p0.MYD <- test.t1
t1#P#p0.MYI <- test.t1

test (Only test.t1 files):
t1.frm
t1#P#p0.MYD -> ..../securedb/t1#P#p0.MYD
t1#P#p0.MYI -> ..../securedb/t1#P#p0.MYI
t1#P#p1.MYD
t1#P#p1.MYI
t1#P#p2.MYD
t1#P#p2.MYI
t1.par

i.e. since no file collision it works.
Since the table it self never has any data or index files (only the 
partitions do) it is not relevant here what the table's DATA/INDEX DIR 
is. But it plays the key role in the next step:

ALTER TABLE test.t1 REMOVE PARTITIONING;
This command will create t1 as an ordinary MyISAM table. without the 
patch it will do it with the DATA/INDEX DIR that is found in the first 
partition. (here 'DATADIR/securedb') and due to the bug#32111 (symlink 
security flaw) it will overwrite the t1.MYD and t1.MYI files in the 
securedb-directory.

non partitioning comment:
I believe that someone foreseen this to some extent by not allowing the 
non partitioned ALTER TABLE command to have a DATA/INDEX DIR clause 
(thus only using it in create table).

> ...
>> diff -Nrup a/mysql-test/r/partition_mgm.result
> b/mysql-test/r/partition_mgm.result
>> --- a/mysql-test/r/partition_mgm.result	2007-04-04 16:26:24 +02:00
>> +++ b/mysql-test/r/partition_mgm.result	2007-11-06 18:09:17 +01:00
>> @@ -1,4 +1,71 @@
>>  DROP TABLE IF EXISTS t1;
> 
> I wrote the below comments for my own understanding. But it might also
> be a good idea to add them as comments to the test.
> 

I will update the testcase with your proposals!

My comments are inline.
> The below test shows that a preexisting table securedb.t1 cannot be
> replaced by a user with no rights in 'securedb'. The altered table
> test.t1 remains within the test directory.
> 
The below test shows that a preexisting table securedb.t1 cannot be 
replaced by a user with no rights in 'securedb'. The altered table 
test.t1 will be altered (remove partitioning) into the test directory 
and having its partitions removed from the securedb directory.
(the partitions data files are named <tablename>#P#<partitionname>.MYD 
and will not collide with a non partitioned table's data files.)

>> +GRANT USAGE ON test.* TO eviluser@localhost;
>> +CREATE DATABASE securedb;
>> +USE securedb;
>> +CREATE TABLE t1 (a INT);
>> +insert into t1 values (0);
>> +USE test;
>> +CREATE TABLE t1 (a INT)
> ...
>> +drop table t1;
>> +drop database securedb;
> 
> The below test shows that a preexisting table securedb.t1 with a
> partition in database 'test' cannot be destroyed by a new table with the
> same partition in 'test'.
> 
The below test shows that a preexisting partition can not be destroyed 
by a new partition from another table. (Remember that a table or 
partition that uses the DATA/INDEX DIR is symlinked and thus has 2. the 
real file in the DATA/INDEX DIR and 2. a symlink in its default database 
directory pointing to the real file. So it is using/blocking 2 files (1 + 2)
>> +create database securedb;
>> +use securedb;
>> +create table t1 (a int)
>> +partition by list (a) (
> ...
>> +use test;
>> +create table t1 (a int)
>> +partition by list (a) (
> ...
>> +create table t1 (a int)
>> +partition by list (a) (
> ...
>> +drop database securedb;
>> +use test;
>> +REVOKE USAGE ON *.* FROM eviluser@localhost;
> 
> If I understand correctly, you did not try to verify that you cannot
> destroy the partitioned securedb.t1 by ALTERing test.t1. Also you did
> not show that even a root user cannot destroy securedb.t1 any more by
> creating or altering partitions of test.t1. Would be nice to see what
> happens.
> 
1. the root user is not significant here, since mysqld do not have any 
acl:s on files and directories, only at columns, tables and databases, 
which THEN maps to files and directories, which can be tampered with by 
using symlinks (am I correct?).
2. I will try a couple of ALTER .. PARTITION ... DATA/INDEX DIR and see 
what happens, if 'usable' I will add appropriate test case here.
>> diff -Nrup a/mysql-test/t/partition_mgm.test b/mysql-test/t/partition_mgm.test
>> --- a/mysql-test/t/partition_mgm.test	2007-04-04 16:26:24 +02:00
>> +++ b/mysql-test/t/partition_mgm.test	2007-11-06 18:09:17 +01:00
>> @@ -4,6 +4,85 @@ DROP TABLE IF EXISTS t1;
>>  --enable_warnings
>>  
>>  #
>> +# Bug 32091: Security breach via directory changes
>> +#
>> +
> 
> A matter of taste: I like to have all lines belonging to one bugfix
> without intermediate blank lines. So I use lines with just a "#" at the
> begin to have an optical separator. But again this is my liking. You do
> not need to follow it, if you like your style better.
> 
Noted, would probably also help automerging.
> It would improve the readability of the test result, if you had
> something like
> 
> --echo # connection default, user root
> 
> here. But that's also just a recommendation, not a must.
> 
OK, will add that
>> +GRANT USAGE ON test.* TO eviluser@localhost;
>> +CREATE DATABASE securedb;
>> +USE securedb;
>> +CREATE TABLE t1 (a INT);
>> +insert into t1 values (0);
> 
> Again, at this place an
> 
> --echo # connection con1, user eviluser
> 
> would be great. Also I like to indent other connections in the test
> file, for example:
> 
> --echo # connection default, user root
> statement
>     --echo # connection con1, user eviluser
>     connect(con1,localhost,eviluser,,);
>     statement
> --echo # connection default, user root
> connection default;
> 
I will look into the indentation
> Again this is a matter of taste. Unfortunately the indentation is lost
> in the result file, so the usefulness might be questionable.
> 
>> +connect(con1,localhost,eviluser,,);
>> +connection con1;
> 
> This is ok. But just for your information: connect(...) does implicitly
> change the connection to the new connection. The "connection con1" would
> not be necessary here.
> 
Thanks for teaching me!
>> +USE test;
>> +--replace_result $MYSQLTEST_VARDIR MYSQLTEST_VARDIR
>> +eval CREATE TABLE t1 (a INT)
>> +PARTITION BY LIST (a) (
>> + PARTITION p0 VALUES IN (0)
>> +  DATA DIRECTORY '$MYSQLTEST_VARDIR/master-data/securedb'
>> +  INDEX DIRECTORY '$MYSQLTEST_VARDIR/master-data/securedb',
>> + PARTITION p1 VALUES IN (1)
>> +  DATA DIRECTORY '$MYSQLTEST_VARDIR/master-data/test'
>> +  INDEX DIRECTORY '$MYSQLTEST_VARDIR/master-data/test',
>> + PARTITION p2 VALUES in (2)
>> +);
>> +--replace_result $MYSQLTEST_VARDIR MYSQLTEST_VARDIR
>> +ALTER TABLE t1 REMOVE PARTITIONING;
>> +insert into t1 values (1);
>> +select * from t1;
> 
> Hm. It was good that you used uppercase letters for the keywords.
> Matthias Leich recommends it and I like it too. Unfortunately we do not
> have a style guide for tests (AFAIK).
> 
> But if you really want to write in lower case, then please, please don't
> mix the style within the same test sequence. In that case convert the
> upper part to lower case too. But as I said, IMHO it is nicer to have
> all keywords in upper case and all object names (table, column,
> database, user, ...) in lower case.
> 
I will change to UPPER CASE and try to follow this style:
http://dev.mysql.com/doc/mysqltest/en/tutorial-sample-test-case.html
http://dev.mysql.com/doc/mysqltest/en/tutorial-naming-conventions.html

Hmm, maybe someone should look into the documentation style of the test 
framework (here it is in lower cases...):
http://dev.mysql.com/doc/mysqltest/en/tutorial-cleaning-up.html
>> +
>> +connection default;
>> +USE securedb;
>> +flush tables;
>> +select * from t1;
>> +use test;
>> +select * from t1;
>> +drop table t1;
>> +drop database securedb;
> 
> Another story with test cases is the selection of names.
> Though I cannot come up with something written, we want to write tests
> so that they won't destroy important user data, if accidentally run on
> an existing database.
> 
> Assume a user has a database "securedb" with important data. If he runs
> this testcase on his data directory, all his data will be deleted.
> 
> So whenever writing tests that need to do something outside of the
> "test" database, we should select database names like "mysqltest1",
> "mysqltest2". User names should also start with "mysql": "mysqluser1",
> "mysqluser2", ... In your case you can also use "mysqleviluser". The
> principle is to have "test" in the names and/or start with "mysql".
> 
Notice taken, will change the names.
> ...
>> diff -Nrup a/sql/ha_partition.cc b/sql/ha_partition.cc
>> --- a/sql/ha_partition.cc	2007-10-31 13:13:17 +01:00
>> +++ b/sql/ha_partition.cc	2007-11-06 18:09:17 +01:00
>> @@ -1599,6 +1599,7 @@ error:
>>  void ha_partition::update_create_info(HA_CREATE_INFO *create_info)
>>  {
>>    m_file[0]->update_create_info(create_info);
>> +  create_info->data_file_name= create_info->index_file_name = NULL;
>>    return;
>>  }
> 
> If I understand comments and code correctly, this is used for ALTER
> only. But there is another place, which might be guilty for setting
> these paths during CREATE:
> 
> int ha_partition::set_up_table_before_create(TABLE *tbl,
>                     const char *partition_name_with_path,
>                     HA_CREATE_INFO *info,
>                     uint part_id,
>                     partition_element *part_elem)
> {
>   ...
>   info->index_file_name= part_elem->index_file_name;
>   info->data_file_name= part_elem->data_file_name;
>   DBUG_RETURN(0);
> }
> 
> From the tests it looks like securedb.t1 and test.t1 do not clash after
> CREATE, so do I misunderstand the quoted code?
> 
Since it is not used it does not matter, but I will try to delete those 
two rows, and if nothing breaks I will include that in the patch.

> Regards
> Ingo

Thanks alot!

-- 
Mattias Jonsson, Software Engineer
MySQL AB, www.mysql.com

Are you MySQL certified?  www.mysql.com/certification
Thread
bk commit into 5.1 tree (mattiasj:1.2605) BUG#32091mattiasj6 Nov
  • Re: bk commit into 5.1 tree (mattiasj:1.2605) BUG#32091Ingo Strüwing7 Nov
    • Re: bk commit into 5.1 tree (mattiasj:1.2605) BUG#32091Mattias Jonsson7 Nov
      • Re: bk commit into 5.1 tree (mattiasj:1.2605) BUG#32091Ingo Strüwing8 Nov