Bug#24388746: PRIVILEGE ESCALATION AND RACE CONDITION USING CREATE TABLE

During REPAIR TABLE of a MyISAM table, a temporary data file (.TMD)
is created. When repair finishes, this file is renamed to the original
.MYD file. The problem was that during this rename, we copied the
stats from the old file to the new file with chmod/chown. If a user
managed to replace the temporary file before chmod/chown was executed,
it was possible to get an arbitrary file with the privileges of the
mysql user.

This patch fixes the problem by not copying stats from the old
file to the new file. This is not needed as the new file was
created with the correct stats. This fix only changes server
behavior - external utilities such as myisamchk still does
chmod/chown.

No test case provided since the problem involves synchronization
with file system operations.

3 files changed, 60 insertions, 23 deletions

diff --git a/storage/myisam/ha_myisam.cc b/storage/myisam/ha_myisam.cc
index 602a0ae6cc1..21cbef32188 100644
--- a/storage/myisam/ha_myisam.cc
+++ b/storage/myisam/ha_myisam.cc
@@ -1,5 +1,5 @@
 /*
-   Copyright (c) 2000, 2012, Oracle and/or its affiliates. All rights reserved.
+   Copyright (c) 2000, 2016, Oracle and/or its affiliates. All rights reserved.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -1091,24 +1091,36 @@ int ha_myisam::repair(THD *thd, MI_CHECK &param, bool do_optimize)
         /* TODO: respect myisam_repair_threads variable */
         my_snprintf(buf, 40, "Repair with %d threads", my_count_bits(key_map));
         thd_proc_info(thd, buf);
+        /*
+          The new file is created with the right stats, so we can skip
+          copying file stats from old to new.
+        */
         error = mi_repair_parallel(&param, file, fixed_name,
-            param.testflag & T_QUICK);
+                                   param.testflag & T_QUICK, TRUE);
         thd_proc_info(thd, "Repair done"); // to reset proc_info, as
                                       // it was pointing to local buffer
       }
       else
       {
         thd_proc_info(thd, "Repair by sorting");
+        /*
+          The new file is created with the right stats, so we can skip
+          copying file stats from old to new.
+        */
         error = mi_repair_by_sort(&param, file, fixed_name,
-            param.testflag & T_QUICK);
+                                  param.testflag & T_QUICK, TRUE);
       }
     }
     else
     {
       thd_proc_info(thd, "Repair with keycache");
       param.testflag &= ~T_REP_BY_SORT;
+      /*
+        The new file is created with the right stats, so we can skip
+        copying file stats from old to new.
+      */
       error=  mi_repair(&param, file, fixed_name,
-			param.testflag & T_QUICK);
+			param.testflag & T_QUICK, TRUE);
     }
 #ifdef HAVE_MMAP
     if (remap)
@@ -1124,7 +1136,11 @@ int ha_myisam::repair(THD *thd, MI_CHECK &param, bool do_optimize)
     {
       optimize_done=1;
       thd_proc_info(thd, "Sorting index");
-      error=mi_sort_index(&param,file,fixed_name);
+      /*
+        The new file is created with the right stats, so we can skip
+        copying file stats from old to new.
+      */
+      error=mi_sort_index(&param,file,fixed_name, TRUE);
     }
     if (!statistics_done && (local_testflag & T_STATISTICS))
     {
diff --git a/storage/myisam/mi_check.c b/storage/myisam/mi_check.c
index ba1f975549a..fe0d4c9c30b 100644
--- a/storage/myisam/mi_check.c
+++ b/storage/myisam/mi_check.c
@@ -1,5 +1,5 @@
 /*
-   Copyright (c) 2000, 2015, Oracle and/or its affiliates. All rights reserved.
+   Copyright (c) 2000, 2016, Oracle and/or its affiliates. All rights reserved.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -1512,7 +1512,7 @@ static int mi_drop_all_indexes(MI_CHECK *param, MI_INFO *info, my_bool force)
 	/* Save new datafile-name in temp_filename */
 
 int mi_repair(MI_CHECK *param, register MI_INFO *info,
-	      char * name, int rep_quick)
+	      char * name, int rep_quick, my_bool no_copy_stat)
 {
   int error,got_error;
   ha_rows start_records,new_header_length;
@@ -1726,6 +1726,11 @@ err:
     /* Replace the actual file with the temporary file */
     if (new_file >= 0)
     {
+      myf flags= 0;
+      if (param->testflag & T_BACKUP_DATA)
+        flags |= MY_REDEL_MAKE_BACKUP;
+      if (no_copy_stat)
+        flags |= MY_REDEL_NO_COPY_STAT;
       mysql_file_close(new_file, MYF(0));
       info->dfile=new_file= -1;
       /*
@@ -1744,8 +1749,7 @@ err:
         info->s->file_map= NULL;
       }
       if (change_to_newfile(share->data_file_name, MI_NAME_DEXT, DATA_TMP_EXT,
-			    (param->testflag & T_BACKUP_DATA ?
-			     MYF(MY_REDEL_MAKE_BACKUP): MYF(0))) ||
+                            flags) ||
 	  mi_open_datafile(info,share,name,-1))
 	got_error=1;
 
@@ -1933,7 +1937,8 @@ int flush_blocks(MI_CHECK *param, KEY_CACHE *key_cache, File file)
 
 	/* Sort index for more efficent reads */
 
-int mi_sort_index(MI_CHECK *param, register MI_INFO *info, char * name)
+int mi_sort_index(MI_CHECK *param, register MI_INFO *info, char * name,
+                  my_bool no_copy_stat)
 {
   reg2 uint key;
   reg1 MI_KEYDEF *keyinfo;
@@ -2004,7 +2009,7 @@ int mi_sort_index(MI_CHECK *param, register MI_INFO *info, char * name)
   share->kfile = -1;
   (void) mysql_file_close(new_file, MYF(MY_WME));
   if (change_to_newfile(share->index_file_name, MI_NAME_IEXT, INDEX_TMP_EXT,
-			MYF(0)) ||
+			no_copy_stat ? MYF(MY_REDEL_NO_COPY_STAT) : MYF(0)) ||
       mi_open_keyfile(share))
     goto err2;
   info->lock_type= F_UNLCK;			/* Force mi_readinfo to lock */
@@ -2209,6 +2214,8 @@ err:
     info		MyISAM handler to repair
     name		Name of table (for warnings)
     rep_quick		set to <> 0 if we should not change data file
+    no_copy_stat        Don't copy file stats from old to new file,
+                        assume that new file was created with correct stats
 
   RESULT
     0	ok
@@ -2216,7 +2223,7 @@ err:
 */
 
 int mi_repair_by_sort(MI_CHECK *param, register MI_INFO *info,
-		      const char * name, int rep_quick)
+		      const char * name, int rep_quick, my_bool no_copy_stat)
 {
   int got_error;
   uint i;
@@ -2543,11 +2550,15 @@ err:
     /* Replace the actual file with the temporary file */
     if (new_file >= 0)
     {
+      myf flags= 0;
+      if (param->testflag & T_BACKUP_DATA)
+        flags |= MY_REDEL_MAKE_BACKUP;
+      if (no_copy_stat)
+        flags |= MY_REDEL_NO_COPY_STAT;
       mysql_file_close(new_file, MYF(0));
       info->dfile=new_file= -1;
       if (change_to_newfile(share->data_file_name,MI_NAME_DEXT, DATA_TMP_EXT,
-			    (param->testflag & T_BACKUP_DATA ?
-			     MYF(MY_REDEL_MAKE_BACKUP): MYF(0))) ||
+                            flags) ||
 	  mi_open_datafile(info,share,name,-1))
 	got_error=1;
     }
@@ -2595,6 +2606,8 @@ err:
     info		MyISAM handler to repair
     name		Name of table (for warnings)
     rep_quick		set to <> 0 if we should not change data file
+    no_copy_stat        Don't copy file stats from old to new file,
+                        assume that new file was created with correct stats
 
   DESCRIPTION
     Same as mi_repair_by_sort but do it multithreaded
@@ -2629,7 +2642,7 @@ err:
 */
 
 int mi_repair_parallel(MI_CHECK *param, register MI_INFO *info,
-			const char * name, int rep_quick)
+                       const char * name, int rep_quick, my_bool no_copy_stat)
 {
   int got_error;
   uint i,key, total_key_length, istep;
@@ -3076,11 +3089,15 @@ err:
     /* Replace the actual file with the temporary file */
     if (new_file >= 0)
     {
+      myf flags= 0;
+      if (param->testflag & T_BACKUP_DATA)
+        flags |= MY_REDEL_MAKE_BACKUP;
+      if (no_copy_stat)
+        flags |= MY_REDEL_NO_COPY_STAT;
       mysql_file_close(new_file, MYF(0));
       info->dfile=new_file= -1;
       if (change_to_newfile(share->data_file_name, MI_NAME_DEXT, DATA_TMP_EXT,
-			    (param->testflag & T_BACKUP_DATA ?
-			     MYF(MY_REDEL_MAKE_BACKUP): MYF(0))) ||
+			    flags) ||
 	  mi_open_datafile(info,share,name,-1))
 	got_error=1;
     }
diff --git a/storage/myisam/myisamchk.c b/storage/myisam/myisamchk.c
index 8606bd7c748..9360a054872 100644
--- a/storage/myisam/myisamchk.c
+++ b/storage/myisam/myisamchk.c
@@ -1,4 +1,4 @@
-/* Copyright (c) 2000, 2012, Oracle and/or its affiliates. All rights reserved.
+/* Copyright (c) 2000, 2016, Oracle and/or its affiliates. All rights reserved.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -993,14 +993,18 @@ static int myisamchk(MI_CHECK *param, char * filename)
 				info->s->state.key_map,
 				param->force_sort))
 	{
+          /*
+            The new file might not be created with the right stats depending
+            on how myisamchk is run, so we must copy file stats from old to new.
+          */
           if (param->testflag & T_REP_BY_SORT)
-            error=mi_repair_by_sort(param,info,filename,rep_quick);
+            error= mi_repair_by_sort(param, info, filename, rep_quick, FALSE);
           else
-            error=mi_repair_parallel(param,info,filename,rep_quick);
+            error= mi_repair_parallel(param, info, filename, rep_quick, FALSE);
 	  state_updated=1;
 	}
 	else if (param->testflag & T_REP_ANY)
-	  error=mi_repair(param, info,filename,rep_quick);
+	  error= mi_repair(param, info, filename, rep_quick, FALSE);
       }
       if (!error && param->testflag & T_SORT_RECORDS)
       {
@@ -1040,12 +1044,12 @@ static int myisamchk(MI_CHECK *param, char * filename)
 	  {
 	    if (param->verbose)
 	      puts("Table had a compressed index;  We must now recreate the index");
-	    error=mi_repair_by_sort(param,info,filename,1);
+	    error= mi_repair_by_sort(param, info, filename, 1, FALSE);
 	  }
 	}
       }
       if (!error && param->testflag & T_SORT_INDEX)
-	error=mi_sort_index(param,info,filename);
+	error= mi_sort_index(param, info, filename, FALSE);
       if (!error)
 	share->state.changed&= ~(STATE_CHANGED | STATE_CRASHED |
 				 STATE_CRASHED_ON_REPAIR);