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

[Sergei Golubchik]

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