MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Ingo Strüwing Date:November 8 2007 9:49am
Subject:Re: bk commit into 5.1 tree (mattiasj:1.2605) BUG#32091
View as plain text  
Hi Mattias,

Mattias Jonsson, 07.11.2007 23:38:
...
> Ingo Strüwing wrote:
...
>> 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?

My problems resulted from a "felt" discrepancy in tenses of your sentence:
"the ... DIR ... was taken ..., and opening ..."
The first part is past tense, while the second part is ongoing activity
in the present tense. This confused me. If you had written the second
part in past tense too, it would be clearer (in my feeling):
"the ... DIR ... was taken ..., and opened ..."
To make it even more clear that the second part is a consequence of the
first part, I would add a "thus":
"the ... DIR ... was taken ..., and thus opened ..."
The same could be reached by something similar to my question:
"the ... DIR ... was taken ... . This allowed ..."
The advantage here is that we have shorter sentences, which is good in
most cases.

> 
> for clarification:
> the SQL-command explanation:
...

Thank you for the thorough explanations. I think I understood what happened.
I was just unsure about that sentence in the changeset comment.

>> ...
>>> diff -Nrup a/mysql-test/r/partition_mgm.result
...
> 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)

After being confused again, I do now guess that you wanted to write 1.
and 2. instead of 2. and 2.

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

Yes, I believe so. But what I wanted to say: You tried to destroy a
table of the almighty root user as 'eviluser' with reduced rights. What
I would like to see is that the root user itself cannot destroy its own
table just by tampering with the DIRECTORY clauses. I agree that it is
almost obvious, but a test could destroy even the smallest doubt. I do
also agree that this does not address the security problem of this bug,
but it would be nice to have it anyway. However I do not insist in this
test, I just wanted to clarify my thoughts.

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

Thanks. I agree that don't need to add the tests if they are not
'usable' (I believe I understand what you mean). :-)

...
> I will change to UPPER CASE and try to follow this style:

Thanks!

> http://dev.mysql.com/doc/mysqltest/en/tutorial-sample-test-case.html
> http://dev.mysql.com/doc/mysqltest/en/tutorial-naming-conventions.html

Hm. I think the latter deserves extensions for user names and database
names and perhaps other objects. I'll notice the docs team.

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

Agree.

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

Thanks.

So we seem to be in agreement. The commenting and test case issues do
not require another review by me. So I say:

OK to push from me after the agreed changes.

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