From: Date: July 23 2008 10:08am
Subject: Re: bzr commit into mysql-4.1 branch (holyfoot:2705) Bug#32167
List-Archive: http://lists.mysql.com/commits/50266
Message-Id: <002401c8ec9b$4ce744d0$bc15a8c0@IBMHF>
MIME-Version: 1.0
Content-Type: text/plain;
	format=flowed;
	charset="iso-8859-1";
	reply-type=original
Content-Transfer-Encoding: 8bit

Sergei,

i'd agree with the suggestions about mi_open_datafile and
MY_RESOLVE_LINK option, will recommit soon.

>> +#if defined(HAVE_REALPATH) && !defined(HAVE_purify) && 
>> !defined(HAVE_BROKEN_REALPATH)
>
> why this #ifdef ?
It existed in the old code i copied into this function, so i didn't invent 
it.
Technically i think it disables the 'realpath' call if the 'realpath' 
doesn't exists in the OS
or is broken.

Also PURIFY doesn't coexist with the 'realpath' for some reason.
It seems i saw a comment about it somewhere, but cannot remember :(

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
>