Re: bzr commit into mysql-4.1 branch (holyfoot:2705) Bug#32167

[Alexey Botchkov]

(continuation of the IRC discussion)
>> +   mi_open_datafile(info,share,NULL,-1))
>
> Why you didn't use real name here ?
>
> On the other hand, if you'd move the check from mi_open down to
> mi_open_datafile(), it would cover all cases automatically.
>
moving the check to the mi_open_datafile didn't work well for me
as we don't store the original path in the 'share' structure.
share->data_file_name contains 'loaded' symlink so i can't use
my_is_symlink() to identify if we need to check the 'realpath'.
Storing the 'original' name of the symlink in the share->data_file_name 
didn't work
well also, particularly with the 'SHOW CREATE FILE'.
So now i plan to keep the check in the mi_open() code and just fixe the
mi_open_datafile call you noted.

Do you have a different idea?

Regards
HF


----- Original Message ----- 
From: "Sergei Golubchik" <serg@stripped>
To: "Alexey Botchkov" <holyfoot@stripped>
Cc: <commits@stripped>
Sent: Monday, July 21, 2008 4:28 PM
Subject: Re: bzr commit into mysql-4.1 branch (holyfoot:2705) Bug#32167


> Hi!
>
> On Jul 18, Alexey Botchkov wrote:
>> #At file:///home/hf/work/mysql_common/32167/
>>
>>  2705 Alexey Botchkov 2008-07-18
>>       bug#32167 another privilege bypass with DATA/INDEX DIRECTORY.
>>       test_if_data_home_dir fixed to look into the real path.
>>       Checks added to mi_optn for symlinks into data home directory.
>
> A couple of questions, see below.
> Otherwise good.
>
>> === modified file 'myisam/mi_check.c'
>> --- a/myisam/mi_check.c 2007-12-13 10:38:01 +0000
>> +++ b/myisam/mi_check.c 2008-07-18 12:02:39 +0000
>> @@ -1610,7 +1610,7 @@ err:
>>      DATA_TMP_EXT, share->base.raid_chunks,
>>      (param->testflag & T_BACKUP_DATA ?
>>       MYF(MY_REDEL_MAKE_BACKUP): MYF(0))) ||
>> -   mi_open_datafile(info,share,-1))
>> +   mi_open_datafile(info,share,NULL,-1))
>>  got_error=1;
>>      }
>>    }
>> @@ -2390,7 +2390,7 @@ err:
>>      DATA_TMP_EXT, share->base.raid_chunks,
>>      (param->testflag & T_BACKUP_DATA ?
>>       MYF(MY_REDEL_MAKE_BACKUP): MYF(0))) ||
>> -   mi_open_datafile(info,share,-1))
>> +   mi_open_datafile(info,share,NULL,-1))
>>  got_error=1;
>>      }
>>    }
>> @@ -2927,7 +2927,7 @@ err:
>>      DATA_TMP_EXT, share->base.raid_chunks,
>>      (param->testflag & T_BACKUP_DATA ?
>>       MYF(MY_REDEL_MAKE_BACKUP): MYF(0))) ||
>> -   mi_open_datafile(info,share,-1))
>> +   mi_open_datafile(info,share,NULL,-1))
>
> Why you didn't use real name here ?
>
> On the other hand, if you'd move the check from mi_open down to
> mi_open_datafile(), it would cover all cases automatically.
>
>> === modified file 'mysys/my_symlink.c'
>> --- a/mysys/my_symlink.c 2005-01-18 01:49:39 +0000
>> +++ b/mysys/my_symlink.c 2008-07-18 12:02:39 +0000
>> @@ -107,17 +107,26 @@ int my_symlink(const char *content, cons
>>  #define BUFF_LEN FN_LEN
>>  #endif
>>
>> +int my_is_symlink(const char *filename __attribute__((unused)))
>> +{
>> +#if defined(HAVE_REALPATH) && !defined(HAVE_purify) && 
>> !defined(HAVE_BROKEN_REALPATH)
>
> why this #ifdef ?
>
>> +  struct stat stat_buff;
>> +  return !lstat(filename, &stat_buff) &&
> S_ISLNK(stat_buff.st_mode);
>> +#else
>> +  return 0;
>> +#endif /*HAVE_REALPATH*/
>> +}
>> +
>>  int my_realpath(char *to, const char *filename,
>>  myf MyFlags __attribute__((unused)))
>>  {
>>  #if defined(HAVE_REALPATH) && !defined(HAVE_purify) && 
>> !defined(HAVE_BROKEN_REALPATH)
>>    int result=0;
>>    char buff[BUFF_LEN];
>> -  struct stat stat_buff;
>>    DBUG_ENTER("my_realpath");
>>
>> -  if (!(MyFlags & MY_RESOLVE_LINK) ||
>> -      (!lstat(filename,&stat_buff) && S_ISLNK(stat_buff.st_mode)))
>> +  if (!(MyFlags & MY_RESOLVE_LINK) || my_is_symlink(filename))
>
> perhaps, it's best to remove MY_RESOLVE_LINK mode altogether. It's
> misleading and insecure - as one can get an unresoled symlinked path if
> the symlink is somewhere in the middle, not at the end.
>
> I could imagine it was introduced as a optimization, but it's wrong.
>
> Just remove this if() completely and call realpath unconditionally.
>
> Regards / Mit vielen Grüssen,
> Sergei
>
> -- 
>   __  ___     ___ ____  __
>  /  |/  /_ __/ __/ __ \/ /   Sergei Golubchik <serg@stripped>
> / /|_/ / // /\ \/ /_/ / /__  Principal Software Engineer/Server Architect
> /_/  /_/\_, /___/\___\_\___/  Sun Microsystems GmbH, HRB München 161028
>       <___/                  Sonnenallee 1, 85551 Kirchheim-Heimstetten
> Geschäftsführer: Thomas Schroeder, Wolfgang Engels, Dr. Roland Boemer
> Vorsitzender des Aufsichtsrates: Martin Häring
>
> -- 
> MySQL Code Commits Mailing List
> For list archives: http://lists.mysql.com/commits
> To unsubscribe:    http://lists.mysql.com/commits?unsub=1
>