Skip to content

Commit

Permalink
[misc] fix possible buffer overflows in _snprintf()
Browse files Browse the repository at this point in the history
* _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.
  • Loading branch information
pbatard committed Apr 17, 2024
1 parent 92ac1c7 commit 513c5f4
Show file tree
Hide file tree
Showing 5 changed files with 27 additions and 29 deletions.
31 changes: 15 additions & 16 deletions src/iso.c
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down Expand Up @@ -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];
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
}
Expand Down Expand Up @@ -1238,7 +1237,7 @@ BOOL ExtractISO(const char* src_iso, const char* dest_dir, BOOL scan)
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);
Expand Down Expand Up @@ -1311,11 +1310,11 @@ BOOL ExtractISO(const char* src_iso, const char* dest_dir, BOOL scan)
// 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) {
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -1599,13 +1598,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 + 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;
}
Expand Down
4 changes: 2 additions & 2 deletions src/parser.c
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/*
* Rufus: The Reliable USB Formatting Utility
* Elementary Unicode compliant find/replace parser
* Copyright © 2012-2023 Pete Batard <[email protected]>
* Copyright © 2012-2024 Pete Batard <[email protected]>
*
* 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
Expand Down Expand Up @@ -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++;
}
Expand Down
4 changes: 2 additions & 2 deletions src/rufus.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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, "\\");
}
Expand Down
7 changes: 3 additions & 4 deletions src/rufus.h
Original file line number Diff line number Diff line change
Expand Up @@ -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)?"<NULL>":str1), ((str2==NULL)?"<NULL>":str2))
#define safe_strstr(str1, str2) strstr(((str1==NULL)?"<NULL>":str1), ((str2==NULL)?"<NULL>":str2))
Expand All @@ -164,7 +163,7 @@
#define safe_strnicmp(str1, str2, count) _strnicmp(((str1==NULL)?"<NULL>":str1), ((str2==NULL)?"<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))
Expand Down
10 changes: 5 additions & 5 deletions src/rufus.rc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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"
Expand Down

2 comments on commit 513c5f4

@gashasbi
Copy link

@gashasbi gashasbi commented on 513c5f4 Apr 17, 2024 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pbatard
Copy link
Owner Author

@pbatard pbatard commented on 513c5f4 Apr 17, 2024 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.