From d66f24cfe30d26e03863a78de9fd58bb3b65ed43 Mon Sep 17 00:00:00 2001 From: Campbell Barton Date: Sun, 30 Oct 2022 15:34:02 +1100 Subject: Fix potential buffer overflow with BLI_path_slash_ensure use BLI_path_slash_ensure was appending to fixed sized buffers without a size check. --- source/blender/blenkernel/intern/appdir.c | 22 ++++++++++---------- .../blenkernel/intern/asset_library_service.cc | 2 +- .../intern/asset_library_service_test.cc | 2 +- source/blender/blenkernel/intern/pointcache.c | 6 +++--- source/blender/blenlib/BLI_path_util.h | 4 ++-- source/blender/blenlib/intern/fileops.c | 2 +- source/blender/blenlib/intern/path_util.c | 18 +++++++++------- source/blender/editors/space_buttons/buttons_ops.c | 2 +- source/blender/editors/space_file/file_ops.c | 24 +++++++++++----------- source/blender/editors/space_file/filelist.cc | 12 +++++------ source/blender/editors/space_file/filesel.c | 4 ++-- source/blender/makesrna/intern/rna_userdef.c | 2 +- source/blender/sequencer/intern/disk_cache.c | 4 ++-- 13 files changed, 54 insertions(+), 50 deletions(-) diff --git a/source/blender/blenkernel/intern/appdir.c b/source/blender/blenkernel/intern/appdir.c index 965f48d1e51..b3990b2b7fc 100644 --- a/source/blender/blenkernel/intern/appdir.c +++ b/source/blender/blenkernel/intern/appdir.c @@ -237,7 +237,7 @@ bool BKE_appdir_font_folder_default(char *dir) } #elif defined(__APPLE__) STRNCPY(test_dir, BLI_expand_tilde("~/Library/Fonts/")); - BLI_path_slash_ensure(test_dir); + BLI_path_slash_ensure(test_dir, sizeof(test_dir)); #else STRNCPY(test_dir, "/usr/share/fonts"); #endif @@ -1093,13 +1093,13 @@ void BKE_appdir_app_templates(ListBase *templates) * \param userdir: Directory specified in user preferences (may be NULL). * note that by default this is an empty string, only use when non-empty. */ -static void where_is_temp(char *tempdir, const size_t tempdir_len, const char *userdir) +static void where_is_temp(char *tempdir, const size_t tempdir_maxlen, const char *userdir) { tempdir[0] = '\0'; if (userdir && BLI_is_dir(userdir)) { - BLI_strncpy(tempdir, userdir, tempdir_len); + BLI_strncpy(tempdir, userdir, tempdir_maxlen); } if (tempdir[0] == '\0') { @@ -1116,23 +1116,23 @@ static void where_is_temp(char *tempdir, const size_t tempdir_len, const char *u for (int i = 0; i < ARRAY_SIZE(env_vars); i++) { const char *tmp = BLI_getenv(env_vars[i]); if (tmp && (tmp[0] != '\0') && BLI_is_dir(tmp)) { - BLI_strncpy(tempdir, tmp, tempdir_len); + BLI_strncpy(tempdir, tmp, tempdir_maxlen); break; } } } if (tempdir[0] == '\0') { - BLI_strncpy(tempdir, "/tmp/", tempdir_len); + BLI_strncpy(tempdir, "/tmp/", tempdir_maxlen); } else { /* add a trailing slash if needed */ - BLI_path_slash_ensure(tempdir); + BLI_path_slash_ensure(tempdir, tempdir_maxlen); } } static void tempdir_session_create(char *tempdir_session, - const size_t tempdir_session_len, + const size_t tempdir_session_maxlen, const char *tempdir) { tempdir_session[0] = '\0'; @@ -1146,9 +1146,9 @@ static void tempdir_session_create(char *tempdir_session, * #_mktemp_s also requires the last null character is included. */ const int tempdir_session_len_required = tempdir_len + session_name_len + 1; - if (tempdir_session_len_required <= tempdir_session_len) { + if (tempdir_session_len_required <= tempdir_session_maxlen) { /* No need to use path joining utility as we know the last character of #tempdir is a slash. */ - BLI_string_join(tempdir_session, tempdir_session_len, tempdir, session_name); + BLI_string_join(tempdir_session, tempdir_session_maxlen, tempdir, session_name); #ifdef WIN32 const bool needs_create = (_mktemp_s(tempdir_session, tempdir_session_len_required) == 0); #else @@ -1158,7 +1158,7 @@ static void tempdir_session_create(char *tempdir_session, BLI_dir_create_recursive(tempdir_session); } if (BLI_is_dir(tempdir_session)) { - BLI_path_slash_ensure(tempdir_session); + BLI_path_slash_ensure(tempdir_session, tempdir_session_maxlen); /* Success. */ return; } @@ -1168,7 +1168,7 @@ static void tempdir_session_create(char *tempdir_session, "Could not generate a temp file name for '%s', falling back to '%s'", tempdir_session, tempdir); - BLI_strncpy(tempdir_session, tempdir, tempdir_session_len); + BLI_strncpy(tempdir_session, tempdir, tempdir_session_maxlen); } void BKE_tempdir_init(const char *userdir) diff --git a/source/blender/blenkernel/intern/asset_library_service.cc b/source/blender/blenkernel/intern/asset_library_service.cc index a6b2b7548a2..a4f97234042 100644 --- a/source/blender/blenkernel/intern/asset_library_service.cc +++ b/source/blender/blenkernel/intern/asset_library_service.cc @@ -44,7 +44,7 @@ std::string normalize_directory_path(StringRefNull directory) char dir_normalized[PATH_MAX]; STRNCPY(dir_normalized, directory.c_str()); - BLI_path_normalize_dir(nullptr, dir_normalized); + BLI_path_normalize_dir(nullptr, dir_normalized, sizeof(dir_normalized)); return std::string(dir_normalized); } } // namespace diff --git a/source/blender/blenkernel/intern/asset_library_service_test.cc b/source/blender/blenkernel/intern/asset_library_service_test.cc index d105c5644de..7952e7ea3b0 100644 --- a/source/blender/blenkernel/intern/asset_library_service_test.cc +++ b/source/blender/blenkernel/intern/asset_library_service_test.cc @@ -118,7 +118,7 @@ TEST_F(AssetLibraryServiceTest, library_path_trailing_slashes) asset_lib_no_slash[strlen(asset_lib_no_slash) - 1] = '\0'; } - BLI_path_slash_ensure(asset_lib_with_slash); + BLI_path_slash_ensure(asset_lib_with_slash, PATH_MAX); AssetLibrary *const lib_no_slash = service->get_asset_library_on_disk(asset_lib_no_slash); diff --git a/source/blender/blenkernel/intern/pointcache.c b/source/blender/blenkernel/intern/pointcache.c index ac98bed2cf0..bb9a0f458b9 100644 --- a/source/blender/blenkernel/intern/pointcache.c +++ b/source/blender/blenkernel/intern/pointcache.c @@ -1317,7 +1317,7 @@ static int ptcache_path(PTCacheID *pid, char *dirname) BLI_path_abs(dirname, blendfilename); } - return BLI_path_slash_ensure(dirname); /* new strlen() */ + return BLI_path_slash_ensure(dirname, MAX_PTCACHE_FILE); /* new strlen() */ } if ((blendfile_path[0] != '\0') || lib) { char file[MAX_PTCACHE_PATH]; /* we don't want the dir, only the file */ @@ -1334,14 +1334,14 @@ static int ptcache_path(PTCacheID *pid, char *dirname) BLI_snprintf(dirname, MAX_PTCACHE_PATH, "//" PTCACHE_PATH "%s", file); BLI_path_abs(dirname, blendfilename); - return BLI_path_slash_ensure(dirname); /* new strlen() */ + return BLI_path_slash_ensure(dirname, MAX_PTCACHE_FILE); /* new strlen() */ } /* use the temp path. this is weak but better than not using point cache at all */ /* temporary directory is assumed to exist and ALWAYS has a trailing slash */ BLI_snprintf(dirname, MAX_PTCACHE_PATH, "%s" PTCACHE_PATH, BKE_tempdir_session()); - return BLI_path_slash_ensure(dirname); /* new strlen() */ + return BLI_path_slash_ensure(dirname, MAX_PTCACHE_FILE); /* new strlen() */ } static size_t ptcache_filepath_ext_append(PTCacheID *pid, diff --git a/source/blender/blenlib/BLI_path_util.h b/source/blender/blenlib/BLI_path_util.h index d4d2ddead71..9c661178322 100644 --- a/source/blender/blenlib/BLI_path_util.h +++ b/source/blender/blenlib/BLI_path_util.h @@ -218,7 +218,7 @@ const char *BLI_path_slash_rfind(const char *string) ATTR_NONNULL() ATTR_WARN_UN * Appends a slash to string if there isn't one there already. * Returns the new length of the string. */ -int BLI_path_slash_ensure(char *string) ATTR_NONNULL(); +int BLI_path_slash_ensure(char *string, size_t string_maxlen) ATTR_NONNULL(1); /** * Removes the last slash and everything after it to the end of string, if there is one. */ @@ -314,7 +314,7 @@ void BLI_path_normalize(const char *relabase, char *path) ATTR_NONNULL(2); * * \note Same as #BLI_path_normalize but adds a trailing slash. */ -void BLI_path_normalize_dir(const char *relabase, char *dir) ATTR_NONNULL(2); +void BLI_path_normalize_dir(const char *relabase, char *dir, size_t dir_maxlen) ATTR_NONNULL(2); /** * Make given name safe to be used in paths. diff --git a/source/blender/blenlib/intern/fileops.c b/source/blender/blenlib/intern/fileops.c index a157302e51e..005de1f85b4 100644 --- a/source/blender/blenlib/intern/fileops.c +++ b/source/blender/blenlib/intern/fileops.c @@ -401,7 +401,7 @@ static bool delete_recursive(const char *dir) /* dir listing produces dir path without trailing slash... */ BLI_strncpy(path, fl->path, sizeof(path)); - BLI_path_slash_ensure(path); + BLI_path_slash_ensure(path, sizeof(path)); if (delete_recursive(path)) { err = true; diff --git a/source/blender/blenlib/intern/path_util.c b/source/blender/blenlib/intern/path_util.c index afe8c3cc033..759a1bf0dc7 100644 --- a/source/blender/blenlib/intern/path_util.c +++ b/source/blender/blenlib/intern/path_util.c @@ -218,7 +218,7 @@ void BLI_path_normalize(const char *relabase, char *path) #endif } -void BLI_path_normalize_dir(const char *relabase, char *dir) +void BLI_path_normalize_dir(const char *relabase, char *dir, size_t dir_maxlen) { /* Would just create an unexpected "/" path, just early exit entirely. */ if (dir[0] == '\0') { @@ -226,7 +226,7 @@ void BLI_path_normalize_dir(const char *relabase, char *dir) } BLI_path_normalize(relabase, dir); - BLI_path_slash_ensure(dir); + BLI_path_slash_ensure(dir, dir_maxlen); } bool BLI_filename_make_safe_ex(char *fname, bool allow_tokens) @@ -1624,7 +1624,7 @@ bool BLI_path_contains(const char *container_path, const char *containee_path) /* Add a trailing slash to prevent same-prefix directories from matching. * e.g. "/some/path" doesn't contain "/some/path_lib". */ - BLI_path_slash_ensure(container_native); + BLI_path_slash_ensure(container_native, sizeof(container_native)); return BLI_str_startswith(containee_native, container_native); } @@ -1659,13 +1659,17 @@ const char *BLI_path_slash_rfind(const char *string) return (lfslash > lbslash) ? lfslash : lbslash; } -int BLI_path_slash_ensure(char *string) +int BLI_path_slash_ensure(char *string, size_t string_maxlen) { int len = strlen(string); + BLI_assert(len < string_maxlen); if (len == 0 || string[len - 1] != SEP) { - string[len] = SEP; - string[len + 1] = '\0'; - return len + 1; + /* Avoid unlikely buffer overflow. */ + if (len + 1 < string_maxlen) { + string[len] = SEP; + string[len + 1] = '\0'; + return len + 1; + } } return len; } diff --git a/source/blender/editors/space_buttons/buttons_ops.c b/source/blender/editors/space_buttons/buttons_ops.c index a9ce9a3d723..4013288b13b 100644 --- a/source/blender/editors/space_buttons/buttons_ops.c +++ b/source/blender/editors/space_buttons/buttons_ops.c @@ -205,7 +205,7 @@ static int file_browse_exec(bContext *C, wmOperator *op) if (BLI_is_dir(path)) { /* Do this first so '//' isn't converted to '//\' on windows. */ - BLI_path_slash_ensure(path); + BLI_path_slash_ensure(path, sizeof(path)); if (is_relative) { BLI_path_rel(path, BKE_main_blendfile_path(bmain)); str_len = strlen(path); diff --git a/source/blender/editors/space_file/file_ops.c b/source/blender/editors/space_file/file_ops.c index 20025b0bac9..c9c4704d16c 100644 --- a/source/blender/editors/space_file/file_ops.c +++ b/source/blender/editors/space_file/file_ops.c @@ -197,13 +197,13 @@ static FileSelect file_select_do(bContext *C, int selected_idx, bool do_diropen) } else if (file->redirection_path) { BLI_strncpy(params->dir, file->redirection_path, sizeof(params->dir)); - BLI_path_normalize_dir(BKE_main_blendfile_path(bmain), params->dir); - BLI_path_slash_ensure(params->dir); + BLI_path_normalize_dir(BKE_main_blendfile_path(bmain), params->dir, sizeof(params->dir)); + BLI_path_slash_ensure(params->dir, sizeof(params->dir)); } else { - BLI_path_normalize_dir(BKE_main_blendfile_path(bmain), params->dir); + BLI_path_normalize_dir(BKE_main_blendfile_path(bmain), params->dir, sizeof(params->dir)); strcat(params->dir, file->relpath); - BLI_path_slash_ensure(params->dir); + BLI_path_slash_ensure(params->dir, sizeof(params->dir)); } ED_file_change_dir(C); @@ -1095,7 +1095,7 @@ static int bookmark_select_exec(bContext *C, wmOperator *op) RNA_property_string_get(op->ptr, prop, entry); BLI_strncpy(params->dir, entry, sizeof(params->dir)); - BLI_path_normalize_dir(BKE_main_blendfile_path(bmain), params->dir); + BLI_path_normalize_dir(BKE_main_blendfile_path(bmain), params->dir, sizeof(params->dir)); ED_file_change_dir(C); WM_event_add_notifier(C, NC_SPACE | ND_SPACE_FILE_LIST, NULL); @@ -1568,8 +1568,8 @@ void file_sfile_to_operator_ex( BLI_path_join(filepath, FILE_MAX, params->dir, params->file); } else { - BLI_strncpy(filepath, params->dir, FILE_MAX - 1); - BLI_path_slash_ensure(filepath); + BLI_strncpy(filepath, params->dir, FILE_MAX); + BLI_path_slash_ensure(filepath, FILE_MAX); } if ((prop = RNA_struct_find_property(op->ptr, "relative_path"))) { @@ -1797,8 +1797,8 @@ static bool file_execute(bContext *C, SpaceFile *sfile) } else { BLI_path_normalize(BKE_main_blendfile_path(bmain), params->dir); - BLI_path_append(params->dir, sizeof(params->dir) - 1, file->relpath); - BLI_path_slash_ensure(params->dir); + BLI_path_append(params->dir, sizeof(params->dir), file->relpath); + BLI_path_slash_ensure(params->dir, sizeof(params->dir)); } ED_file_change_dir(C); } @@ -1961,7 +1961,7 @@ static int file_parent_exec(bContext *C, wmOperator *UNUSED(unused)) if (params) { if (BLI_path_parent_dir(params->dir)) { - BLI_path_normalize_dir(BKE_main_blendfile_path(bmain), params->dir); + BLI_path_normalize_dir(BKE_main_blendfile_path(bmain), params->dir, sizeof(params->dir)); ED_file_change_dir(C); if (params->recursion_level > 1) { /* Disable 'dirtree' recursion when going up in tree. */ @@ -2544,7 +2544,7 @@ void file_directory_enter_handle(bContext *C, void *UNUSED(arg_unused), void *UN } } - BLI_path_normalize_dir(BKE_main_blendfile_path(bmain), params->dir); + BLI_path_normalize_dir(BKE_main_blendfile_path(bmain), params->dir, sizeof(params->dir)); if (filelist_is_dir(sfile->files, params->dir)) { if (!STREQ(params->dir, old_dir)) { /* Avoids flickering when nothing's changed. */ @@ -2631,7 +2631,7 @@ void file_filename_enter_handle(bContext *C, void *UNUSED(arg_unused), void *arg /* if directory, open it and empty filename field */ if (filelist_is_dir(sfile->files, filepath)) { - BLI_path_normalize_dir(BKE_main_blendfile_path(bmain), filepath); + BLI_path_normalize_dir(BKE_main_blendfile_path(bmain), filepath, sizeof(filepath)); BLI_strncpy(params->dir, filepath, sizeof(params->dir)); params->file[0] = '\0'; ED_file_change_dir(C); diff --git a/source/blender/editors/space_file/filelist.cc b/source/blender/editors/space_file/filelist.cc index f177eebf6f2..3257534f94d 100644 --- a/source/blender/editors/space_file/filelist.cc +++ b/source/blender/editors/space_file/filelist.cc @@ -1208,7 +1208,7 @@ static int filelist_geticon_ex(const FileDirEntry *file, } else if (root) { BLI_path_join(fullpath, sizeof(fullpath), root, file->relpath); - BLI_path_slash_ensure(fullpath); + BLI_path_slash_ensure(fullpath, sizeof(fullpath)); } for (; tfsm; tfsm = tfsm->next) { if (STREQ(tfsm->path, target)) { @@ -1952,7 +1952,7 @@ void filelist_setdir(struct FileList *filelist, char *r_dir) const bool allow_invalid = filelist->asset_library_ref != nullptr; BLI_assert(strlen(r_dir) < FILE_MAX_LIBEXTRA); - BLI_path_normalize_dir(BKE_main_blendfile_path_from_global(), r_dir); + BLI_path_normalize_dir(BKE_main_blendfile_path_from_global(), r_dir, FILE_MAX_LIBEXTRA); const bool is_valid_path = filelist->check_dir_fn(filelist, r_dir, !allow_invalid); BLI_assert(is_valid_path || allow_invalid); UNUSED_VARS_NDEBUG(is_valid_path); @@ -2920,7 +2920,7 @@ static int filelist_readjob_list_dir(const char *root, if (BLI_file_alias_target(full_path, entry->redirection_path)) { if (BLI_is_dir(entry->redirection_path)) { entry->typeflag = FILE_TYPE_DIR; - BLI_path_slash_ensure(entry->redirection_path); + BLI_path_slash_ensure(entry->redirection_path, FILE_MAXDIR); } else { entry->typeflag = (eFileSel_File_Types)ED_path_extension_type(entry->redirection_path); @@ -3476,7 +3476,7 @@ static void filelist_readjob_recursive_dir_add_items(const bool do_lib, BLI_strncpy(dir, filelist->filelist.root, sizeof(dir)); BLI_strncpy(filter_glob, filelist->filter_data.filter_glob, sizeof(filter_glob)); - BLI_path_normalize_dir(job_params->main_name, dir); + BLI_path_normalize_dir(job_params->main_name, dir, sizeof(dir)); td_dir->dir = BLI_strdup(dir); /* Init the file indexer. */ @@ -3507,7 +3507,7 @@ static void filelist_readjob_recursive_dir_add_items(const bool do_lib, * Note that in the end, this means we 'cache' valid relative subdir once here, * this is actually better. */ BLI_strncpy(rel_subdir, subdir, sizeof(rel_subdir)); - BLI_path_normalize_dir(root, rel_subdir); + BLI_path_normalize_dir(root, rel_subdir, sizeof(rel_subdir)); BLI_path_rel(rel_subdir, root); bool is_lib = false; @@ -3555,7 +3555,7 @@ static void filelist_readjob_recursive_dir_add_items(const bool do_lib, max_recursion, is_lib, recursion_level, entry)) { /* We have a directory we want to list, add it to todo list! */ BLI_path_join(dir, sizeof(dir), root, entry->relpath); - BLI_path_normalize_dir(job_params->main_name, dir); + BLI_path_normalize_dir(job_params->main_name, dir, sizeof(dir)); td_dir = static_cast(BLI_stack_push_r(todo_dirs)); td_dir->level = recursion_level + 1; td_dir->dir = BLI_strdup(dir); diff --git a/source/blender/editors/space_file/filesel.c b/source/blender/editors/space_file/filesel.c index df7e9702e85..af2c9d4e757 100644 --- a/source/blender/editors/space_file/filesel.c +++ b/source/blender/editors/space_file/filesel.c @@ -197,7 +197,7 @@ static FileSelectParams *fileselect_ensure_updated_file_params(SpaceFile *sfile) } if (params->dir[0]) { - BLI_path_normalize_dir(blendfile_path, params->dir); + BLI_path_normalize_dir(blendfile_path, params->dir, sizeof(params->dir)); BLI_path_abs(params->dir, blendfile_path); } @@ -1187,7 +1187,7 @@ int autocomplete_directory(struct bContext *C, char *str, void *UNUSED(arg_v)) match = UI_autocomplete_end(autocpl, str); if (match == AUTOCOMPLETE_FULL_MATCH) { - BLI_path_slash_ensure(str); + BLI_path_slash_ensure(str, FILE_MAX); } } } diff --git a/source/blender/makesrna/intern/rna_userdef.c b/source/blender/makesrna/intern/rna_userdef.c index e22ae205b0a..58189ab2478 100644 --- a/source/blender/makesrna/intern/rna_userdef.c +++ b/source/blender/makesrna/intern/rna_userdef.c @@ -574,7 +574,7 @@ static void rna_Userdef_disk_cache_dir_update(Main *UNUSED(bmain), { if (U.sequencer_disk_cache_dir[0] != '\0') { BLI_path_abs(U.sequencer_disk_cache_dir, BKE_main_blendfile_path_from_global()); - BLI_path_slash_ensure(U.sequencer_disk_cache_dir); + BLI_path_slash_ensure(U.sequencer_disk_cache_dir, sizeof(U.sequencer_disk_cache_dir)); BLI_path_make_safe(U.sequencer_disk_cache_dir); } diff --git a/source/blender/sequencer/intern/disk_cache.c b/source/blender/sequencer/intern/disk_cache.c index 1f52d2ea41b..8c0ae4e055c 100644 --- a/source/blender/sequencer/intern/disk_cache.c +++ b/source/blender/sequencer/intern/disk_cache.c @@ -182,7 +182,7 @@ static void seq_disk_cache_get_files(SeqDiskCache *disk_cache, char *path) if (is_dir && !FILENAME_IS_CURRPAR(file)) { char subpath[FILE_MAX]; BLI_strncpy(subpath, fl->path, sizeof(subpath)); - BLI_path_slash_ensure(subpath); + BLI_path_slash_ensure(subpath, sizeof(sizeof(subpath))); seq_disk_cache_get_files(disk_cache, subpath); } @@ -384,7 +384,7 @@ static void seq_disk_cache_delete_invalid_files(SeqDiskCache *disk_cache, DiskCacheFile *next_file, *cache_file = disk_cache->files.first; char cache_dir[FILE_MAX]; seq_disk_cache_get_dir(disk_cache, scene, seq, cache_dir, sizeof(cache_dir)); - BLI_path_slash_ensure(cache_dir); + BLI_path_slash_ensure(cache_dir, sizeof(cache_dir)); while (cache_file) { next_file = cache_file->next; -- cgit v1.2.3