Skip to content
Permalink
Browse files Browse the repository at this point in the history
fs: Don't try to delete the file when copying. It could cause a secur…
…ity issue if the file exists and doesn't allow other's to read/write. delete could allow someone to create the file and have access to the data.
  • Loading branch information
user-none committed Jul 12, 2018
1 parent f82091a commit db124b8
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 30 deletions.
52 changes: 25 additions & 27 deletions base/fs/m_fs.c
Expand Up @@ -101,6 +101,15 @@ static M_bool M_fs_isfileintodir(const char *p1, const char *p2, char **new_p2)
return M_TRUE;
}

/* Used by copy and move to determine if we can write to the given path
* based on a file already existing there or not.
*
* access is used to determine existence because we don't want to overwrite
* if there already is a file. This is not guaranteed because if there is
* a race condition where a file is created after this check it will be
* overwritten. Not much we can do about that. It shouldn't pose a security
* issue since this is more of a request than a requirement.
*/
static M_bool M_fs_check_overwrite_allowed(const char *p1, const char *p2, M_uint32 mode)
{
M_fs_info_t *info = NULL;
Expand Down Expand Up @@ -129,8 +138,7 @@ static M_bool M_fs_check_overwrite_allowed(const char *p1, const char *p2, M_uin

if (type != M_FS_TYPE_DIR) {
/* File exists at path. */
if (M_fs_perms_can_access(p2, M_FS_PERMS_MODE_NONE) == M_FS_ERROR_SUCCESS)
{
if (M_fs_perms_can_access(p2, M_FS_PERMS_MODE_NONE) == M_FS_ERROR_SUCCESS) {
ret = M_FALSE;
goto done;
}
Expand Down Expand Up @@ -209,19 +217,6 @@ static M_fs_error_t M_fs_copy_file(const char *path_old, const char *path_new, M
size_t offset;
M_fs_error_t res;

/* We're going to create/open/truncate the new file, then as we read the contents from the old file we'll write it
* to new file. */
if (M_fs_perms_can_access(path_new, M_FS_PERMS_MODE_NONE) == M_FS_ERROR_SUCCESS) {
/* Try to delete the file since we'll be overwrite it. This is so when we create the file we create it without
* any permissions and to ensure that anything that has the file already open won't be able to read the new
* contents we're writing to the file or be able to change the perms. There is an unavoidable race condition
* between deleting and creating the file where someone could create the file and have access. However,
* depending on the OS they may have access even if the file is created with no perms... */
res = M_fs_delete(path_new, M_FALSE, NULL, M_FS_PROGRESS_NOEXTRA);
if (res != M_FS_ERROR_SUCCESS) {
return res;
}
}
/* Open the old file */
res = M_fs_file_open(&fd_old, path_old, M_FS_BUF_SIZE, M_FS_FILE_MODE_READ|M_FS_FILE_MODE_NOCREATE, NULL);
if (res != M_FS_ERROR_SUCCESS) {
Expand All @@ -236,6 +231,9 @@ static M_fs_error_t M_fs_copy_file(const char *path_old, const char *path_new, M
}
perms = M_fs_info_get_perms(info);
}

/* We're going to create/open/truncate the new file, then as we read the contents from the old file we'll write it
* to new file. */
res = M_fs_file_open(&fd_new, path_new, M_FS_BUF_SIZE, M_FS_FILE_MODE_WRITE|M_FS_FILE_MODE_OVERWRITE, perms);
M_fs_info_destroy(info);
if (res != M_FS_ERROR_SUCCESS) {
Expand Down Expand Up @@ -333,7 +331,7 @@ M_fs_error_t M_fs_move(const char *path_old, const char *path_new, M_uint32 mode
}

/* Normalize the old path and do basic checks that it exists. We'll leave really checking that the old path
* existing to rename because any check we perform may not be true when rename is called. */
* existing to rename because any check we perform may not be true when rename is called. */
res = M_fs_path_norm(&norm_path_old, path_old, M_FS_PATH_NORM_RESALL, M_FS_SYSTEM_AUTO);
if (res != M_FS_ERROR_SUCCESS) {
M_free(norm_path_new);
Expand All @@ -351,7 +349,7 @@ M_fs_error_t M_fs_move(const char *path_old, const char *path_new, M_uint32 mode
return res;
}

/* There is a race condition where the path could not exist but be created between the exists check and calling
/* There is a race condition where the path could not exist but be created between the exists check and calling
* rename to move the file but there isn't much we can do in this case. copy will delete and the file so this
* situation won't cause an error. */
if (!M_fs_check_overwrite_allowed(norm_path_old, norm_path_new, mode)) {
Expand Down Expand Up @@ -399,15 +397,15 @@ M_fs_error_t M_fs_move(const char *path_old, const char *path_new, M_uint32 mode
res = M_fs_delete(norm_path_old, M_TRUE, NULL, M_FS_PROGRESS_NOEXTRA);
} else {
/* Failure - Delete the new files that were copied but only if we are not overwriting. We don't
* want to remove any existing files (especially if the dest is a dir). */
* want to remove any existing files (especially if the dest is a dir). */
if (!(mode & M_FS_FILE_MODE_OVERWRITE)) {
M_fs_delete(norm_path_new, M_TRUE, NULL, M_FS_PROGRESS_NOEXTRA);
}
res = M_FS_ERROR_GENERIC;
}
} else {
/* Call the cb with the result of the move whether it was a success for fail. We call the cb only if the
* result of the move is not M_FS_ERROR_NOT_SAMEDEV because the copy operation will call the cb for us. */
* result of the move is not M_FS_ERROR_NOT_SAMEDEV because the copy operation will call the cb for us. */
if (cb) {
M_fs_progress_set_result(progress, res);
if (!cb(progress)) {
Expand Down Expand Up @@ -465,7 +463,7 @@ M_fs_error_t M_fs_copy(const char *path_old, const char *path_new, M_uint32 mode
}

/* Normalize the old path and do basic checks that it exists. We'll leave really checking that the old path
* existing to rename because any check we perform may not be true when rename is called. */
* existing to rename because any check we perform may not be true when rename is called. */
res = M_fs_path_norm(&norm_path_old, path_old, M_FS_PATH_NORM_RESALL, M_FS_SYSTEM_AUTO);
if (res != M_FS_ERROR_SUCCESS) {
M_free(norm_path_new);
Expand All @@ -485,7 +483,7 @@ M_fs_error_t M_fs_copy(const char *path_old, const char *path_new, M_uint32 mode

type = M_fs_info_get_type(info);

/* There is a race condition where the path could not exist but be created between the exists check and calling
/* There is a race condition where the path could not exist but be created between the exists check and calling
* rename to move the file but there isn't much we can do in this case. copy will delete and the file so this
* situation won't cause an error. */
if (!M_fs_check_overwrite_allowed(norm_path_old, norm_path_new, mode)) {
Expand All @@ -497,7 +495,7 @@ M_fs_error_t M_fs_copy(const char *path_old, const char *path_new, M_uint32 mode

entries = M_fs_dir_entries_create();
/* No need to destroy info because it's now owned by entries and will be destroyed when entries is destroyed.
* M_FS_DIR_WALK_FILTER_READ_INFO_BASIC doesn't actually get the perms it's just there to ensure the info is
* M_FS_DIR_WALK_FILTER_READ_INFO_BASIC doesn't actually get the perms it's just there to ensure the info is
* stored in the entry. */
M_fs_dir_entries_insert(entries, M_fs_dir_walk_fill_entry(norm_path_new, NULL, type, info, M_FS_DIR_WALK_FILTER_READ_INFO_BASIC));
if (type == M_FS_TYPE_DIR) {
Expand All @@ -523,7 +521,7 @@ M_fs_error_t M_fs_copy(const char *path_old, const char *path_new, M_uint32 mode

type = M_fs_dir_entry_get_type(entry);
/* The total isn't the total number of files but the total number of operations.
* Making dirs and symlinks is one operation and copying a file will be split into
* Making dirs and symlinks is one operation and copying a file will be split into
* multiple operations. Copying uses the M_FS_BUF_SIZE to read and write in
* chunks. We determine how many chunks will be needed to read the entire file and
* use that for the number of operations for the file. */
Expand Down Expand Up @@ -600,7 +598,7 @@ M_fs_error_t M_fs_copy(const char *path_old, const char *path_new, M_uint32 mode
}

/* Delete the file(s) if it could not be copied properly, but only if we are not overwriting.
* If we're overwriting then there could be other files in that location (especially if it's a dir). */
* If we're overwriting then there could be other files in that location (especially if it's a dir). */
if (res != M_FS_ERROR_SUCCESS && !(mode & M_FS_FILE_MODE_OVERWRITE)) {
M_fs_delete(path_new, M_TRUE, NULL, M_FS_PROGRESS_NOEXTRA);
}
Expand Down Expand Up @@ -659,7 +657,7 @@ M_fs_error_t M_fs_delete(const char *path, M_bool remove_children, M_fs_progress
entries = M_fs_dir_entries_create();

/* Recursive directory deletion isn't intuitive. We have to generate a list of files and delete the list.
* We cannot delete as walk because not all file systems support that operation. The walk; delete; behavior
* We cannot delete as walk because not all file systems support that operation. The walk; delete; behavior
* is undefined in Posix and HFS is known to skip files if the directory contents is modifies as the
* directory is being walked. */
if (type == M_FS_TYPE_DIR && remove_children) {
Expand All @@ -671,7 +669,7 @@ M_fs_error_t M_fs_delete(const char *path, M_bool remove_children, M_fs_progress
}

/* Add the original path to the list of entries. This may be the only entry in the list. We need to add
* it after a potential walk because we can't delete a directory that isn't empty.
* it after a potential walk because we can't delete a directory that isn't empty.
* Note:
* - The info will be owned by the entry and destroyed when it is destroyed.
* - The basic info param doesn't get the info in this case. it's set so the info is stored in the entry. */
Expand All @@ -680,7 +678,7 @@ M_fs_error_t M_fs_delete(const char *path, M_bool remove_children, M_fs_progress
len = M_fs_dir_entries_len(entries);
if (cb) {
/* Create the progress. The same progress will be used for the entire operation. It will be updated with
* new info as necessary. */
* new info as necessary. */
progress = M_fs_progress_create();

/* Get the total size of all files to be deleted if using the progress cb and size totals is set. */
Expand Down
22 changes: 19 additions & 3 deletions base/fs/m_fs_path.c
Expand Up @@ -254,7 +254,7 @@ char *M_fs_path_join_parts(const M_list_str_t *path, M_fs_system_t sys_type)
sys_type = M_fs_path_get_system_type(sys_type);

/* Remove any empty parts (except for the first part which denotes an abs path on Unix
* or a UNC path on Windows). */
* or a UNC path on Windows). */
parts = M_list_str_duplicate(path);
for (i=len-1; i>0; i--) {
part = M_list_str_at(parts, i);
Expand Down Expand Up @@ -536,7 +536,7 @@ M_bool M_fs_path_ishidden(const char *path, M_fs_info_t *info)
}

/* Hidden. Check if the first character of the last part of the path. Either the file or directory name itself
* starts with a '.'. */
* starts with a '.'. */
path_parts = M_fs_path_componentize_path(path, M_FS_SYSTEM_UNIX);
len = M_list_str_len(path_parts);
if (len > 0) {
Expand Down Expand Up @@ -601,7 +601,23 @@ char *M_fs_path_tmpdir(M_fs_system_t sys_type)
d = M_fs_path_mac_tmpdir();
#else
const char *const_temp;
/* Try Unix env var. */
/* Unix doens't have a fancy function to get the standard
* temporary directory an application can use. Instead there
* is a convoluted set of possible paths that could be used.
*
* We're going to go though each one in a priority order and
* verify if we can read and write the directory. If so then
* that's the one that will be used. We are fine using access
* here because it doesn't matter if the path ends up being
* changed out from underneath us later on. When it's used
* at that time it will fail. Right now we just want to get
* a path that can be tried. */

/* Try Unix env vars.
*
* This is not ideal but a valid way to set the temporary directory
* for a user. Per Single Unix Specification 4 and probably other things.
*/
# ifdef HAVE_SECURE_GETENV
const_temp = secure_getenv("TMPDIR");
# else
Expand Down

0 comments on commit db124b8

Please sign in to comment.