From 513c5f44a51845f0cd305a602453ac86c6395d21 Mon Sep 17 00:00:00 2001 From: Pete Batard Date: Wed, 17 Apr 2024 17:19:03 +0100 Subject: [PATCH] [misc] fix possible buffer overflows in _snprintf() * _snprintf() is not always guaranteed to NUL terminate a string which could lead to buffer overflows in iso_extract_files() and iso_extract_files(). * Fix this by switching to using the more secure _snprintf_s(). * Vulnerability discovered and reported by Mansour Gashasbi (@gashasbi). * For good measure, we also switch to the strncat_s() where possible and also use memmove() instead of memcpy()/strcpy() as the behaviour of the latter on overlapping memory regions is undefined. * Also fix some additional MinGW warnings regarding casts and nb_blocks. --- src/iso.c | 31 +++++++++++++++---------------- src/parser.c | 4 ++-- src/rufus.c | 4 ++-- src/rufus.h | 7 +++---- src/rufus.rc | 10 +++++----- 5 files changed, 27 insertions(+), 29 deletions(-) diff --git a/src/iso.c b/src/iso.c index 752c35d9..a68ba07a 100644 --- a/src/iso.c +++ b/src/iso.c @@ -615,10 +615,9 @@ static int udf_extract_files(udf_t *p_udf, udf_dirent_t *p_udf_dirent, const cha uprintf("Error allocating file name"); goto out; } - length = _snprintf(psz_fullpath, length, "%s%s/%s", psz_extract_dir, psz_path, psz_basename); - if (length < 0) { + length = _snprintf_s(psz_fullpath, length, _TRUNCATE, "%s%s/%s", psz_extract_dir, psz_path, psz_basename); + if (length < 0) goto out; - } if (S_ISLNK(udf_get_posix_filemode(p_udf_dirent))) img_report.has_symlinks = SYMLINKS_UDF; if (udf_is_dir(p_udf_dirent)) { @@ -757,7 +756,7 @@ static int iso_extract_files(iso9660_t* p_iso, const char *psz_path) return 1; } - length = _snprintf(psz_fullpath, sizeof(psz_fullpath), "%s%s/", psz_extract_dir, psz_path); + length = _snprintf_s(psz_fullpath, sizeof(psz_fullpath), _TRUNCATE, "%s%s/", psz_extract_dir, psz_path); if (length < 0) goto out; psz_basename = &psz_fullpath[length]; @@ -1079,7 +1078,7 @@ BOOL ExtractISO(const char* src_iso, const char* dest_dir, BOOL scan) const char* basedir[] = { "i386", "amd64", "minint" }; const char* tmp_sif = ".\\txtsetup.sif~"; int k, r = 1; - char* tmp, * buf, * ext, * spacing = " "; + char *tmp, *buf = NULL, *ext, *spacing = " "; char path[MAX_PATH], path2[16]; uint16_t sl_version; size_t i, j, size, sl_index = 0; @@ -1131,7 +1130,7 @@ BOOL ExtractISO(const char* src_iso, const char* dest_dir, BOOL scan) if (fd_md5sum == NULL) uprintf("WARNING: Could not create '%s'", md5sum_name[0]); } else { - md5sum_size = ReadISOFileToBuffer(src_iso, md5sum_name[0], &md5sum_data); + md5sum_size = ReadISOFileToBuffer(src_iso, md5sum_name[0], (uint8_t**)&md5sum_data); md5sum_pos = md5sum_data; } } @@ -1238,7 +1237,7 @@ out: char isolinux_tmp[MAX_PATH]; static_sprintf(isolinux_tmp, "%sisolinux.tmp", temp_dir); size = (size_t)ExtractISOFile(src_iso, isolinux_path.String[i], isolinux_tmp, FILE_ATTRIBUTE_NORMAL); - if ((size == 0) || (read_file(isolinux_tmp, &buf) != size)) { + if ((size == 0) || (read_file(isolinux_tmp, (uint8_t**)&buf) != size)) { uprintf(" Could not access %s", isolinux_path.String[i]); } else { sl_version = GetSyslinuxVersion(buf, size, &ext); @@ -1311,11 +1310,11 @@ out: // coverity[swapped_arguments] if (GetTempFileNameU(temp_dir, APPLICATION_NAME, 0, path) != 0) { size = (size_t)ExtractISOFile(src_iso, grub_path, path, FILE_ATTRIBUTE_NORMAL); - if ((size == 0) || (read_file(path, &buf) != size)) + if ((size == 0) || (read_file(path, (uint8_t**)&buf) != size)) uprintf(" Could not read Grub version from '%s'", grub_path); else GetGrubVersion(buf, size); - free(buf); + safe_free(buf); DeleteFileU(path); } if (img_report.grub2_version[0] == 0) { @@ -1539,7 +1538,7 @@ uint32_t ReadISOFileToBuffer(const char* iso, const char* iso_file, uint8_t** bu { ssize_t read_size; int64_t file_length; - uint32_t ret = 0, nb_blocks; + uint32_t ret = 0, nblocks; iso9660_t* p_iso = NULL; udf_t* p_udf = NULL; udf_dirent_t *p_udf_root = NULL, *p_udf_file = NULL; @@ -1566,13 +1565,13 @@ uint32_t ReadISOFileToBuffer(const char* iso, const char* iso_file, uint8_t** bu uprintf("Only files smaller than 4 GB are supported"); goto out; } - nb_blocks = (uint32_t)((file_length + UDF_BLOCKSIZE - 1) / UDF_BLOCKSIZE); - *buf = malloc(nb_blocks * UDF_BLOCKSIZE + 1); + nblocks = (uint32_t)((file_length + UDF_BLOCKSIZE - 1) / UDF_BLOCKSIZE); + *buf = malloc(nblocks * UDF_BLOCKSIZE + 1); if (*buf == NULL) { uprintf("Could not allocate buffer for file %s", iso_file); goto out; } - read_size = udf_read_block(p_udf_file, *buf, nb_blocks); + read_size = udf_read_block(p_udf_file, *buf, nblocks); if (read_size < 0 || read_size != file_length) { uprintf("Error reading UDF file %s", iso_file); goto out; @@ -1599,13 +1598,13 @@ try_iso: uprintf("Only files smaller than 4 GB are supported"); goto out; } - nb_blocks = (uint32_t)((file_length + ISO_BLOCKSIZE - 1) / ISO_BLOCKSIZE); - *buf = malloc(nb_blocks * ISO_BLOCKSIZE + 1); + nblocks = (uint32_t)((file_length + ISO_BLOCKSIZE - 1) / ISO_BLOCKSIZE); + *buf = malloc(nblocks * ISO_BLOCKSIZE + 1); if (*buf == NULL) { uprintf("Could not allocate buffer for file %s", iso_file); goto out; } - if (iso9660_iso_seek_read(p_iso, *buf, p_statbuf->lsn, nb_blocks) != nb_blocks * ISO_BLOCKSIZE) { + if (iso9660_iso_seek_read(p_iso, *buf, p_statbuf->lsn, nblocks) != nblocks * ISO_BLOCKSIZE) { uprintf("Error reading ISO file %s", iso_file); goto out; } diff --git a/src/parser.c b/src/parser.c index 8cd1ccb1..5970b543 100644 --- a/src/parser.c +++ b/src/parser.c @@ -1,7 +1,7 @@ /* * Rufus: The Reliable USB Formatting Utility * Elementary Unicode compliant find/replace parser - * Copyright © 2012-2023 Pete Batard + * Copyright © 2012-2024 Pete Batard * * 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 @@ -91,7 +91,7 @@ static loc_cmd* get_loc_cmd(char c, char* line) { // locate ending quote while ((line[i] != 0) && ((line[i] != '"') || ((line[i] == '"') && (line[i-1] == '\\')))) { if ((line[i] == '"') && (line[i-1] == '\\')) { - strcpy(&line[i-1], &line[i]); + memmove(&line[i-1], &line[i], strlen(&line[i]) + 1); } else { i++; } diff --git a/src/rufus.c b/src/rufus.c index 0fbbc32e..e3f38c8b 100755 --- a/src/rufus.c +++ b/src/rufus.c @@ -3258,7 +3258,7 @@ int WINAPI WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR lpCmdLine static_strcpy(cur_dir, ".\\"); } else { // Need to remove the '\\?\' prefix and reappend the trailing '\' - strcpy(cur_dir, &cur_dir[4]); + static_strcpy(cur_dir, &cur_dir[4]); static_strcat(cur_dir, "\\"); } safe_closehandle(hFile); @@ -3283,7 +3283,7 @@ int WINAPI WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR lpCmdLine static_strcpy(temp_dir, cur_dir); } else { // Need to remove the '\\?\' prefix or else we'll get issues with the Fido icon - strcpy(temp_dir, &temp_dir[4]); + static_strcpy(temp_dir, &temp_dir[4]); // And me must re-append the '\' that gets removed by GetFinalPathNameByHandle() static_strcat(temp_dir, "\\"); } diff --git a/src/rufus.h b/src/rufus.h index 24532db4..685fe0f9 100644 --- a/src/rufus.h +++ b/src/rufus.h @@ -150,12 +150,11 @@ #define safe_free(p) do {free((void*)p); p = NULL;} while(0) #define safe_mm_free(p) do {_mm_free((void*)p); p = NULL;} while(0) #define safe_min(a, b) min((size_t)(a), (size_t)(b)) -#define safe_strcp(dst, dst_max, src, count) do {memcpy(dst, src, safe_min(count, dst_max)); \ +#define safe_strcp(dst, dst_max, src, count) do {memmove(dst, src, safe_min(count, dst_max)); \ ((char*)(dst))[safe_min(count, dst_max)-1] = 0;} while(0) #define safe_strcpy(dst, dst_max, src) safe_strcp(dst, dst_max, src, safe_strlen(src)+1) #define static_strcpy(dst, src) safe_strcpy(dst, sizeof(dst), src) -#define safe_strncat(dst, dst_max, src, count) strncat(dst, src, safe_min(count, (dst_max) - safe_strlen(dst) - 1)) -#define safe_strcat(dst, dst_max, src) safe_strncat(dst, dst_max, src, safe_strlen(src)+1) +#define safe_strcat(dst, dst_max, src) strncat_s(dst, dst_max, src, _TRUNCATE) #define static_strcat(dst, src) safe_strcat(dst, sizeof(dst), src) #define safe_strcmp(str1, str2) strcmp(((str1==NULL)?"":str1), ((str2==NULL)?"":str2)) #define safe_strstr(str1, str2) strstr(((str1==NULL)?"":str1), ((str2==NULL)?"":str2)) @@ -164,7 +163,7 @@ #define safe_strnicmp(str1, str2, count) _strnicmp(((str1==NULL)?"":str1), ((str2==NULL)?"":str2), count) #define safe_closehandle(h) do {if ((h != INVALID_HANDLE_VALUE) && (h != NULL)) {CloseHandle(h); h = INVALID_HANDLE_VALUE;}} while(0) #define safe_release_dc(hDlg, hDC) do {if ((hDC != INVALID_HANDLE_VALUE) && (hDC != NULL)) {ReleaseDC(hDlg, hDC); hDC = NULL;}} while(0) -#define safe_sprintf(dst, count, ...) do {_snprintf(dst, count, __VA_ARGS__); (dst)[(count)-1] = 0; } while(0) +#define safe_sprintf(dst, count, ...) do {_snprintf_s(dst, count, _TRUNCATE, __VA_ARGS__); (dst)[(count)-1] = 0; } while(0) #define static_sprintf(dst, ...) safe_sprintf(dst, sizeof(dst), __VA_ARGS__) #define safe_atoi(str) ((((char*)(str))==NULL)?0:atoi(str)) #define safe_strlen(str) ((((char*)(str))==NULL)?0:strlen(str)) diff --git a/src/rufus.rc b/src/rufus.rc index 6cd976d3..8ec32d0d 100644 --- a/src/rufus.rc +++ b/src/rufus.rc @@ -33,7 +33,7 @@ LANGUAGE LANG_NEUTRAL, SUBLANG_NEUTRAL IDD_DIALOG DIALOGEX 12, 12, 232, 326 STYLE DS_SETFONT | DS_MODALFRAME | DS_CENTER | WS_MINIMIZEBOX | WS_POPUP | WS_CAPTION | WS_SYSMENU EXSTYLE WS_EX_ACCEPTFILES -CAPTION "Rufus 4.5.2129" +CAPTION "Rufus 4.5.2130" FONT 9, "Segoe UI Symbol", 400, 0, 0x0 BEGIN LTEXT "Drive Properties",IDS_DRIVE_PROPERTIES_TXT,8,6,53,12,NOT WS_GROUP @@ -397,8 +397,8 @@ END // VS_VERSION_INFO VERSIONINFO - FILEVERSION 4,5,2129,0 - PRODUCTVERSION 4,5,2129,0 + FILEVERSION 4,5,2130,0 + PRODUCTVERSION 4,5,2130,0 FILEFLAGSMASK 0x3fL #ifdef _DEBUG FILEFLAGS 0x1L @@ -416,13 +416,13 @@ BEGIN VALUE "Comments", "https://rufus.ie" VALUE "CompanyName", "Akeo Consulting" VALUE "FileDescription", "Rufus" - VALUE "FileVersion", "4.5.2129" + VALUE "FileVersion", "4.5.2130" VALUE "InternalName", "Rufus" VALUE "LegalCopyright", "� 2011-2024 Pete Batard (GPL v3)" VALUE "LegalTrademarks", "https://www.gnu.org/licenses/gpl-3.0.html" VALUE "OriginalFilename", "rufus-4.5.exe" VALUE "ProductName", "Rufus" - VALUE "ProductVersion", "4.5.2129" + VALUE "ProductVersion", "4.5.2130" END END BLOCK "VarFileInfo"