MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Ingo Strüwing Date:November 7 2007 5:13pm
Subject:Re: bk commit into 5.1 tree (mattiasj:1.2605) BUG#32091
View as plain text  
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 ..."?

>   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()).

...
> 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.

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.

> +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'.

> +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.

> 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.

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.

> +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;

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.

> +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.

> +
> +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".

...
> 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?

Regards
Ingo
-- 
Ingo Strüwing, Senior Software Developer
MySQL GmbH, Dachauer Str. 37, D-80335 München
Geschäftsführer: Kaj Arnö - HRB München 162140
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