| Bug #43747 | BACKUP with backref path succeeds on invalid backupdir | ||
|---|---|---|---|
| Submitted: | 19 Mar 2009 10:40 | Modified: | 29 Jun 2009 14:40 |
| Reporter: | Ingo Strüwing | Email Updates: | |
| Status: | Closed | Impact on me: | |
| Category: | MySQL Server: Backup | Severity: | S3 (Non-critical) |
| Version: | 6.0 | OS: | Any |
| Assigned to: | Ingo Strüwing | CPU Architecture: | Any |
[19 Mar 2009 10:40]
Ingo Strüwing
[20 Mar 2009 16:49]
Chuck Bell
Verified as described. The suggested fixes would enhance the operation of backup and the backupdir.
[20 Mar 2009 17:01]
Ingo Strüwing
Suggested triage values: Defect: Medium. This is a serious problem, but only for exceptional use of backupdir. Workaround: None. The described behavior cannot be influenced. Impact: Substantial. I expect backup to be used a lot. The backupdir behavior could confuse quite a few people.
[20 Mar 2009 21:08]
Bugs System
A patch for this bug has been committed. After review, it may be pushed to the relevant source trees for release in the next version. You can access the patch from: http://lists.mysql.com/commits/69964 2803 Ingo Struewing 2009-03-20 Bug#43747 - BACKUP with backref path succeeds on invalid backupdir The server variable "backupdir" behaved obscure. If it was set to some existent non-directory file system object, one could not use it for backup or restore, unless the path name used as the backup image file parameter started with ../ or was an absolute path. However, if backupdir references a non-existent object, backup and restore fails, regardless, what the backup image file is, even if it is an absolute path (see Bug 43536). The problem is fixed so that backupdir is not accepted as a usable path, based on its mere existence, but it must also be a directory. The new condition is pretty simple: If backupdir is not an existent directory, backup and restore fail. Period. An absolute path as the backup image file parameter does not help (see Bug 43536). If backupdir is an existent directory, the combination of backupdir and backup image file parameter works as expected. If backupdir is a symbolic link, the decision is based on the target of the link (the link is followed). @ include/my_dir.h Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Added declaration for test_if_directory(). @ mysql-test/suite/backup/r/backup_backupdir.result Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Added test result. @ mysql-test/suite/backup/t/backup_backupdir.test Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Added tests. @ mysys/my_lib.c Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Added function test_if_directory(). @ sql/backup/kernel.cc Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Added a call to test_if_directory() to the check of backupdir. @ sql/set_var.cc Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Added calls to test_if_directory() to the checks of backupdir.
[24 Mar 2009 23:55]
Chuck Bell
Setting back to in progress until next patch is ready.
[27 Mar 2009 16:34]
Ingo Strüwing
If this will be fixed, please note that some tests need to have their workarounds removed. The tests are not pushed yet. They are developed for WL#4518 (Online Backup: Test backupdir variable). The affected test cases are: backup_backupdir, backup_backupdir_not_windows, and backup_backupdir_sync.
[5 Apr 2009 10:32]
Ingo Strüwing
If you agree that we want to have the check for a directory, then I'll come up with a solution that should also fix Bug#43767 (Wrong path if backupdir is symlink and backref path used): At first we let the backupdir variable value go through mf_format and make it a clean path with symlink resolution. The result should not contain a trailing slash any more. Based on this conversion we can safely do the checks, and for Bug#43767, appending the BACKUP/RESTORE image file argument will give a path relative to the target of a symlink. So both problems should be fixed in one go. Can I have approval for this approach from both reviewers?
[6 Apr 2009 15:27]
Chuck Bell
The approach is acceptable. Please proceed.
[14 Apr 2009 10:46]
Jørgen Løland
I concur - the approach is acceptable.
[24 Apr 2009 14:54]
Ingo Strüwing
I apologize that the implementation of this fix took so long and ended in a big patch. The main problem was that MySQL did not have functions to handle path names in a way I want them to be handled. I did first try to use my_realpath() for all path resolutions. I ended up with incomparable paths in the test cases. If $MYSQL_VARDIR is a symbolic link into a memory file system (e.g. mysql-test-run --mem), the resulting paths started with a path into the memfs. I did not find a way to load such resolved path into a variable for a mysqltest --replace_result statement. I could get at unresolved paths only. Another problem was that my_realpath() does no cleanup of the path if the target object does not exist. That is, it does not remove unneeded ./ or ../ path elements. Also, my_realpath() never resolves or cleans the path on Windows. Using cleanup_dirname() after every my_realpath() makes clean paths in cases of non-existent target objects and on Windows, but it does not take symbolic links into account (see Bug#43767 - Wrong path if backupdir is symlink and backref path used). Assume a symbolic link "symlink" to "/target/place". cleanup_dirname("symlink/../myfile") results in "myfile", while normal file system operations like "touch symlink/../myfile" would work on "/target/myfile". This means that cleanup_dirname() constructs paths based on the symlink itself, while normal file system operations interpret paths based on symlink targets. A correct cleanup in this case would mean to retain the ../ after the symlink. So I say that cleanup_dirname() produces correct results for platforms without symbolic links only. My next attempt was to leave path names unresolved. I constructed result paths by just concatenating subpaths. A relative pathname for the backupdir variable was concatenated with datadir. A relative path name for the BACKUP/RESTORE argument was concatenated with backupdir. This approach worked out well on Linux and Windows. I was just stopped by a failing test case backup_securebackup. A test succeeded, which should have failed. The reason was that "securepath/" compared with "securepath/../../myfile" pretended to be inside "securepath". At this point, I tried to use my_realpath() just for the the two paths in this comparison. This worked on Linux, though it was a fault that my_realpath() does not clean paths to non-existent objects. The comparison could still succeed and RESTORE was stopped by the non-existent file only. However, BACKUP could succeed anyway. I stripped off the last path element (the base name) of the backup/restore file path for the comparison. This fixed the BACKUP problem by stopping it on a non-existent directory. The fault turned into a blemish. The wrong error message was ugly, but I would have accepted it, if it would only work on Windows. As explained above, on Windows, my_realpath() does never clean the path. So the full security flaw was retained. At this point I decided that MySQL needs a function that cleans a path from "./", "../", respects symbolic links, cleans the path even if the target object does not exist, and works on Windows too. I used cleanup_dirname() as a base. Since I want to clean all sort of paths, not only directories, I called it safe_cleanup_pathname(). I got rid of the questionable feature of resolving "/~/" (tilde in the middle of a path name wipes out the leading part and replaces it by the home directory). I got rid of replacing "./" at path begin by the current directory. I do not want to turn a relative path into an absolute path. (A path starting with "~/" is already an absolute path if the home directory is an absolute path; if not, the resulting path is relative anyway.) Since the new function name contains "safe", I added a parameter for the size of the result buffer instead of relying it to be at least FN_REFLEN big. The nice side effect is that the path lengths for backupdir, secure_backup_file_priv, and the backup image file name are not limited by MySQL any more. Another useful extension is the variable length argument list, which allows the concatenation of path names in one go with the cleanup. This required to rename the function to safe_cleanup_cat_path(). I added this function into the comparison with the secure_backup_file_priv variable, and it was successful. Hence I did also use it after every path concatenation, which I mentioned above. This made beautiful, clean path names. But I had to fix some tests. Now I started to check the test coverage. I added a few tests, which we should already have before ;-). But anyway, it turned out, that a unittest for safe_cleanup_cat_path() was required to check all cases. Finally I had to add some test case fixes for Windows, and fix the unittest because of "\" path delimiters. After everything was perfect, I noticed that it may not be a good idea to make backupdir and secure_backup_file_priv absolute paths when they are set. If the file system objects change, symlinks become directories and/or vice versa, the cleanup based on existing objects may be wrong when the variables are used. So I restructured the code to store the variable values unmodified, and just make clean, absolute paths whne they are used. After that I had to fix the tests again. This was the adventure. But it will go on with reviews, which may require much more effort than average. Note that this patch does also fix Bug#43767 (Wrong path if backupdir is symlink and backref path used).
[24 Apr 2009 14:58]
Bugs System
A patch for this bug has been committed. After review, it may be pushed to the relevant source trees for release in the next version. You can access the patch from: http://lists.mysql.com/commits/72789 2797 Ingo Struewing 2009-04-24 Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used A backupdir variable value like 'existent_file.txt' and a backup image file name like '../backup_file.bak' allowed for a successful BACKUP and RESTORE (Bug #43747). It had been decided that we do not want to allow BACKUP or RESTORE to succeed, if backupdir is set to an invalid path (compare Bug 43536 - BACKUP with absolute path fails on invalid backupdir). A backupdir variable value like 'symlink_to_dir1_dir2' and a backup image file name like '../backup_file.bak' located the file in the wrong directory. If "symlink_to_dir1_dir2 -> dir1/dir2", the used file was just "backup_file.bak". Cleanup of "../" was done based on the symlink itself, not based on its target (dir2). Normal file system operations interpret a path of "symlink_to_dir1_dir2/../backup_file.bak" as "dir1/backup_file.bak". The main problem was that MySQL did not have a path handling function, which cleans a path while respecting symbolic links, works even if the path name references a non-existent file system object, and works on Windows too. For more details see the explanation in the bug report #43747. Another problem was that the backupdir variable was checked for an existent and writable file system object only. It was not required that this object needs to be a directory. The main part of the patch is the new function safe_cleanup_cat_path() in mf_pack.c. Another important new function is test_if_directory() in my_lib.c. It allows to verify that backupdir is not just a writable path, but also a directory. Yet another new function is cut_print_path() in mf_format.c. It supports the creation of good error messages. Many error message templates have limited space for path names, e.g. %.64s. Often the last part of a path name is more informative for error messages than its begin. This function returns a cut path with a prepended "... ", if the path is longer than the length limit. Yet another new function is is_usable_directory() in set_var.cc. It supports checking of variable values when they are set. To avoid frequent use of strlen() I added length variables to the seldom or never changed variables home_dir, mysql_real_data_home, and opt_secure_backup_file_priv. Finally I used these functions in the backup kernel to build an absolute, clean path for the backupdir to check it for being a writable directory and to prepend it to a relative backup image file path. Additionally I used the functions on opt_secure_backup_file_priv before comparing it against the resulting backup image file path. To have a good test coverage for the new non-trivial function safe_cleanup_cat_path(), I made a unittest for it. @ include/my_dir.h Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Added declaration for test_if_directory(). @ include/my_sys.h Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Added declarations for home_dir_len, cut_print_path(), and cleanup_cat_path(). @ mysql-test/suite/backup/r/backup_backupdir.result Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Updated test result. @ mysql-test/suite/backup/r/backup_backupdir_not_windows.result Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used New test result. @ mysql-test/suite/backup/r/backup_securebackup.result Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Updated test result. @ mysql-test/suite/backup/t/backup_backupdir.test Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Added proper cleanup for backupdir. Added SELECTs for backupdir to show its changes. Removed tests for limited length of the variable. Made the test for a "really long string" better checkable. Added new tests for the reported bug. Added tests for special variable values (NULL and DEFAULT). @ mysql-test/suite/backup/t/backup_backupdir_not_windows.test Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used New test case for Bug#43767. @ mysql-test/suite/backup/t/backup_securebackup-master.opt Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Added a --backupdir option for coverage testing. @ mysql-test/suite/backup/t/backup_securebackup.test Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Added proper cleanup for backupdir. Fixed wrong tests (bup_sfp5.bak, bup_sfp1.bak). Added a printout for the backupdir option. @ mysys/mf_format.c Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Added definition of new function cut_print_path(). @ mysys/mf_pack.c Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Added definition of new function safe_cleanup_cat_path(). @ mysys/my_init.c Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Added setup for home_dir_len. @ mysys/my_lib.c Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Added definition of new function test_if_directory(). @ mysys/my_static.c Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Added declaration of home_dir_len. @ sql/backup/kernel.cc Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Moved check of backupdir from execute_backup_command() to Backup_restore_ctx::prepare_path(). Changed the latter to use safe_cleanup_cat_path() and check_backupdir(). @ sql/backup/stream.cc Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Changed Stream::test_secure_file_priv_access() to use safe_cleanup_cat_path() on opt_secure_backup_file_priv. @ sql/mysql_priv.h Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Added declarations of mysql_real_data_home_len and opt_secure_backup_file_priv_len. @ sql/mysqld.cc Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Added definitions and setup of mysql_real_data_home_len and opt_secure_backup_file_priv_len. Moved check of backupdir value from mysqld_get_one_option() to fix_paths(). Used is_usable_directory() on backupdir and opt_secure_backup_file_priv. @ sql/set_var.cc Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Removed double declarations. Removed sys_check_backupdir() altogether. All checks are done in sys_update_backupdir() now. Changed make_default_backupdir() to default_backupdir(), which allocates the value. Added definition of new function check_backupdir(). Added definition of new function is_usable_directory(). Changed sys_update_backupdir() to use is_usable_directory(). @ sql/set_var.h Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Added declarations for is_usable_directory(), check_backupdir(), and default_backupdir(). @ sql/share/errmsg.txt Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Changed ER_BACKUP_BACKUPDIR to the more genaral usable ER_NOT_RW_DIR. @ unittest/mysys/CMakeLists.txt Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Added safe_cleanup_cat_path-t. @ unittest/mysys/Makefile.am Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Added safe_cleanup_cat_path-t. @ unittest/mysys/safe_cleanup_cat_path-t.c Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used New unit test for safe_cleanup_cat_path().
[28 Apr 2009 17:06]
Chuck Bell
Patch approved. Well done.
[29 Apr 2009 9:11]
Jørgen Løland
Good to push
[1 May 2009 12:15]
Ingo Strüwing
Queued to 6.0-backup.
[3 Jun 2009 7:09]
Jørgen Løland
Merged to azalea June 2
[29 Jun 2009 14:40]
Paul DuBois
No changelog entry needed. Not in any released version.
[6 Jan 2010 17:55]
Bugs System
A patch for this bug has been committed. After review, it may be pushed to the relevant source trees for release in the next version. You can access the patch from: http://lists.mysql.com/commits/96163 3035 Ingo Struewing 2010-01-06 WL#5101 - MySQL Backup back port Merged revid:ingo.struewing@sun.com-20090424145553-xmtiynisc12yf3wl Merged revid:ingo.struewing@sun.com-20090519094845-d1a0tsqervmfwp57 Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used A backupdir variable value like 'existent_file.txt' and a backup image file name like '../backup_file.bak' allowed for a successful BACKUP and RESTORE (Bug #43747). It had been decided that we do not want to allow BACKUP or RESTORE to succeed, if backupdir is set to an invalid path (compare Bug 43536 - BACKUP with absolute path fails on invalid backupdir). A backupdir variable value like 'symlink_to_dir1_dir2' and a backup image file name like '../backup_file.bak' located the file in the wrong directory. If "symlink_to_dir1_dir2 -> dir1/dir2", the used file was just "backup_file.bak". Cleanup of "../" was done based on the symlink itself, not based on its target (dir2). Normal file system operations interpret a path of "symlink_to_dir1_dir2/../backup_file.bak" as "dir1/backup_file.bak". The main problem was that MySQL did not have a path handling function, which cleans a path while respecting symbolic links, works even if the path name references a non-existent file system object, and works on Windows too. For more details see the explanation in the bug report #43747. Another problem was that the backupdir variable was checked for an existent and writable file system object only. It was not required that this object needs to be a directory. The main part of the patch is the new function safe_cleanup_cat_path() in mf_pack.c. Another important new function is test_if_directory() in my_lib.c. It allows to verify that backupdir is not just a writable path, but also a directory. Yet another new function is cut_print_path() in mf_format.c. It supports the creation of good error messages. Many error message templates have limited space for path names, e.g. %.64s. Often the last part of a path name is more informative for error messages than its begin. This function returns a cut path with a prepended "... ", if the path is longer than the length limit. Yet another new function is is_usable_directory() in set_var.cc. It supports checking of variable values when they are set. To avoid frequent use of strlen() I added length variables to the seldom or never changed variables home_dir, mysql_real_data_home, and opt_secure_backup_file_priv. Finally I used these functions in the backup kernel to build an absolute, clean path for the backupdir to check it for being a writable directory and to prepend it to a relative backup image file path. Additionally I used the functions on opt_secure_backup_file_priv before comparing it against the resulting backup image file path. To have a good test coverage for the new non-trivial function safe_cleanup_cat_path(), I made a unittest for it. --- Bug#43767 - Wrong path if backupdir is symlink and backref path used Addendum fix to Bug#43747 (BACKUP with backref path succeeds on invalid backupdir) and Bug#43767. The former fix checked every path element for being a symlink. Only then it was prevented to let ../ remove this path element. But the correct solution is to prevent ../ from removing all path elements except of real (non-symlinked) directories. A path like dir/non-existent-file.txt/../existent-file.txt must not be cleaned up to dir/existent-file.txt. Normal (operating system) file operations don't do that either. ".." is a valid path element with its back-referencing semantics in an existent directory only. However, on Windows, we need to do the clean-ups irrespectively of the file system object. Windows system services do the clean-up even after path elements that refer to non-existent objects. In the above example, dir/existent-file.txt is used. safe_cleanup_cat_path(), its unittest, and the regression tests do now respect the platform difference. original changeset: 2599.146.1 original changeset: 2599.148.1 @ include/my_sys.h WL#5101 - MySQL Backup back port Removed a MY_FLAG value clash. @ libmysql/CMakeLists.txt WL#5101 - MySQL Backup back port Added mysys as include directory. Required for my_rnd.c. @ unittest/mysys/CMakeLists.txt WL#5101 - MySQL Backup back port Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Added safe_cleanup_cat_path-t. @ unittest/mysys/Makefile.am WL#5101 - MySQL Backup back port Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Added safe_cleanup_cat_path-t. @ unittest/mysys/safe_cleanup_cat_path-t.c WL#5101 - MySQL Backup back port Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used New unit test for safe_cleanup_cat_path().
