Skip to content

Commit

Permalink
[misc] harden usage of uprintf()
Browse files Browse the repository at this point in the history
* Passing a non-formatting buffer as first parameter of uprintf() can lead
  to an exception if this buffer happens to contain a '%' character, so
  usage of uprintf() with string buffers that may contain '%' should be
  sanitized.
* Also drop the _uprintf/_uprintfs aliases as they are no longer required.
  • Loading branch information
pbatard committed Apr 20, 2023
1 parent 1a3a155 commit fffd4d1
Show file tree
Hide file tree
Showing 12 changed files with 37 additions and 31 deletions.
4 changes: 2 additions & 2 deletions src/ext2fs/com_err.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@ struct error_table {
struct et_list;

/* For use with Rufus */
extern void _uprintf(const char *format, ...);
extern void uprintf(const char *format, ...);
#define VA_ARGS(...) , ##__VA_ARGS__
#define com_err(src, err, fmt, ...) _uprintf("%s: [%08X] " # fmt, src?src:"ext2fs", err - EXT2_ET_BASE VA_ARGS(__VA_ARGS__))
#define com_err(src, err, fmt, ...) uprintf("%s: [%08X] " # fmt, src?src:"ext2fs", err - EXT2_ET_BASE VA_ARGS(__VA_ARGS__))

extern char const *error_message (long);
extern void (*com_err_hook) (const char *, long, const char *, va_list);
Expand Down
2 changes: 1 addition & 1 deletion src/format.c
Original file line number Diff line number Diff line change
Expand Up @@ -1290,7 +1290,7 @@ static BOOL WriteDrive(HANDLE hPhysicalDrive, BOOL bZeroDrive)
}
assert((uintptr_t)sec_buf % SelectedDrive.SectorSize == 0);
sec_buf_pos = 0;
bled_init(_uprintf, NULL, sector_write, update_progress, NULL, &FormatStatus);
bled_init(uprintf, NULL, sector_write, update_progress, NULL, &FormatStatus);
bled_ret = bled_uncompress_with_handles(hSourceImage, hPhysicalDrive, img_report.compression_type);
bled_exit();
uprintfs("\r\n");
Expand Down
7 changes: 4 additions & 3 deletions src/localization.c
Original file line number Diff line number Diff line change
Expand Up @@ -513,7 +513,7 @@ void PrintStatusInfo(BOOL info, BOOL debug, unsigned int duration, int msg_id, .
va_start(args, msg_id);
safe_vsnprintf(msg_cur, MSG_LEN, format, args);
va_end(args);
msg_cur[MSG_LEN-1] = '\0';
msg_cur[MSG_LEN - 1] = '\0';

if ((duration != 0) || (!bStatusTimerArmed))
OutputMessage(info, msg_cur);
Expand All @@ -534,8 +534,9 @@ void PrintStatusInfo(BOOL info, BOOL debug, unsigned int duration, int msg_id, .
va_start(args, msg_id);
safe_vsnprintf(buf, MSG_LEN, format, args);
va_end(args);
buf[MSG_LEN-1] = '\0';
uprintf(buf);
buf[MSG_LEN - 1] = '\0';
// buf may(?) containt a '%' so don't feed it as a naked format string
uprintf("%s", buf);
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/net.c
Original file line number Diff line number Diff line change
Expand Up @@ -945,7 +945,7 @@ static DWORD WINAPI DownloadISOThread(LPVOID param)
free(sig);
uprintf("Download signature is valid ✓");
uncompressed_size = *((uint64_t*)&compressed[5]);
if ((uncompressed_size < 1 * MB) && (bled_init(_uprintf, NULL, NULL, NULL, NULL, &FormatStatus) >= 0)) {
if ((uncompressed_size < 1 * MB) && (bled_init(uprintf, NULL, NULL, NULL, NULL, &FormatStatus) >= 0)) {
fido_script = malloc((size_t)uncompressed_size);
size = bled_uncompress_from_buffer_to_buffer(compressed, dwCompressedSize, fido_script, (size_t)uncompressed_size, BLED_COMPRESSION_LZMA);
bled_exit();
Expand Down
3 changes: 2 additions & 1 deletion src/process.c
Original file line number Diff line number Diff line change
Expand Up @@ -493,7 +493,8 @@ static DWORD WINAPI SearchProcessThread(LPVOID param)
// If we're switching process and found a match, print it
if (bFound) {
static_sprintf (tmp, "● [%06u] %s (%s)", (uint32_t)pid[cur_pid], cmdline, access_rights_str[access_rights & 0x7]);
vuprintf(tmp);
// tmp may contain a '%' so don't feed it as a naked format string
vuprintf("%s", tmp);
StrArrayAdd(&BlockingProcess, tmp, TRUE);
bFound = FALSE;
access_rights = 0;
Expand Down
9 changes: 6 additions & 3 deletions src/rufus.c
Original file line number Diff line number Diff line change
Expand Up @@ -2255,7 +2255,8 @@ DWORD CheckDriveAccess(DWORD dwTimeOut, BOOL bPrompt)
proceed = FALSE;
uprintf("Found potentially blocking process(es) against %s:", &PhysicalPath[4]);
for (j = 0; j < BlockingProcess.Index; j++)
uprintf(BlockingProcess.String[j]);
// BlockingProcess.String[j] may contain a '%' so don't feed it as a naked format string
uprintf("%s", BlockingProcess.String[j]);
}
}

Expand All @@ -2275,7 +2276,8 @@ DWORD CheckDriveAccess(DWORD dwTimeOut, BOOL bPrompt)
proceed = FALSE;
uprintf("Found potentially blocking process(es) against %s", drive_name);
for (j = 0; j < BlockingProcess.Index; j++)
uprintf(BlockingProcess.String[j]);
// BlockingProcess.String[j] may contain a '%' so don't feed it as a naked format string
uprintf("%s", BlockingProcess.String[j]);
}
}
}
Expand Down Expand Up @@ -3547,7 +3549,8 @@ int WINAPI WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR lpCmdLine
if (list_params) {
uprintf("Command line arguments:");
for (i = 1; i < argc; i++)
uprintf(argv[i]);
// argv[i] may contain a '%' so don't feed it as a naked format string.
uprintf("%s", argv[i]);
}
}
} else {
Expand Down
16 changes: 7 additions & 9 deletions src/rufus.h
Original file line number Diff line number Diff line change
Expand Up @@ -172,20 +172,18 @@ static __inline void static_repchr(char* p, char s, char r) {
#define to_unix_path(str) static_repchr(str, '\\', '/')
#define to_windows_path(str) static_repchr(str, '/', '\\')

extern void _uprintf(const char *format, ...);
extern void _uprintfs(const char *str);
#define uprintf(...) _uprintf(__VA_ARGS__)
#define uprintfs(s) _uprintfs(s)
#define vuprintf(...) do { if (verbose) _uprintf(__VA_ARGS__); } while(0)
#define vvuprintf(...) do { if (verbose > 1) _uprintf(__VA_ARGS__); } while(0)
#define suprintf(...) do { if (!bSilent) _uprintf(__VA_ARGS__); } while(0)
#define uuprintf(...) do { if (usb_debug) _uprintf(__VA_ARGS__); } while(0)
extern void uprintf(const char *format, ...);
extern void uprintfs(const char *str);
#define vuprintf(...) do { if (verbose) uprintf(__VA_ARGS__); } while(0)
#define vvuprintf(...) do { if (verbose > 1) uprintf(__VA_ARGS__); } while(0)
#define suprintf(...) do { if (!bSilent) uprintf(__VA_ARGS__); } while(0)
#define uuprintf(...) do { if (usb_debug) uprintf(__VA_ARGS__); } while(0)
#define ubprintf(...) do { safe_sprintf(&ubuffer[ubuffer_pos], UBUFFER_SIZE - ubuffer_pos - 4, __VA_ARGS__); \
ubuffer_pos = strlen(ubuffer); ubuffer[ubuffer_pos++] = '\r'; ubuffer[ubuffer_pos++] = '\n'; \
ubuffer[ubuffer_pos] = 0; } while(0)
#define ubflush() do { if (ubuffer_pos) uprintf("%s", ubuffer); ubuffer_pos = 0; } while(0)
#ifdef _DEBUG
#define duprintf(...) _uprintf(__VA_ARGS__)
#define duprintf uprintf
#else
#define duprintf(...)
#endif
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 3.23.2025"
CAPTION "Rufus 3.23.2026"
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 @@ -392,8 +392,8 @@ END
//

VS_VERSION_INFO VERSIONINFO
FILEVERSION 3,23,2025,0
PRODUCTVERSION 3,23,2025,0
FILEVERSION 3,23,2026,0
PRODUCTVERSION 3,23,2026,0
FILEFLAGSMASK 0x3fL
#ifdef _DEBUG
FILEFLAGS 0x1L
Expand All @@ -411,13 +411,13 @@ BEGIN
VALUE "Comments", "https://rufus.ie"
VALUE "CompanyName", "Akeo Consulting"
VALUE "FileDescription", "Rufus"
VALUE "FileVersion", "3.23.2025"
VALUE "FileVersion", "3.23.2026"
VALUE "InternalName", "Rufus"
VALUE "LegalCopyright", "© 2011-2023 Pete Batard (GPL v3)"
VALUE "LegalTrademarks", "https://www.gnu.org/licenses/gpl-3.0.html"
VALUE "OriginalFilename", "rufus-3.23.exe"
VALUE "ProductName", "Rufus"
VALUE "ProductVersion", "3.23.2025"
VALUE "ProductVersion", "3.23.2026"
END
END
BLOCK "VarFileInfo"
Expand Down
4 changes: 2 additions & 2 deletions src/stdfn.c
Original file line number Diff line number Diff line change
Expand Up @@ -762,8 +762,8 @@ DWORD RunCommand(const char* cmd, const char* dir, BOOL log)
output = malloc(dwAvail + 1);
if ((output != NULL) && (ReadFile(hOutputRead, output, dwAvail, &dwRead, NULL)) && (dwRead != 0)) {
output[dwAvail] = 0;
// coverity[tainted_string]
uprintf(output);
// output may contain a '%' so don't feed it as a naked format string
uprintf("%s", output);
}
free(output);
}
Expand Down
6 changes: 3 additions & 3 deletions src/stdio.c
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
/*
* Rufus: The Reliable USB Formatting Utility
* Standard User I/O Routines (logging, status, error, etc.)
* Copyright © 2011-2023 Pete Batard <[email protected]>
* Copyright © 2020 Mattiwatti <[email protected]>
* Copyright © 2011-2021 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 @@ -46,7 +46,7 @@ HWND hStatus;
size_t ubuffer_pos = 0;
char ubuffer[UBUFFER_SIZE]; // Buffer for ubpushf() messages we don't log right away

void _uprintf(const char *format, ...)
void uprintf(const char *format, ...)
{
static char buf[4096];
char* p = buf;
Expand Down Expand Up @@ -82,7 +82,7 @@ void _uprintf(const char *format, ...)
free(wbuf);
}

void _uprintfs(const char* str)
void uprintfs(const char* str)
{
wchar_t* wstr;
wstr = utf8_to_wchar(str);
Expand Down
2 changes: 1 addition & 1 deletion src/vhd.c
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ static BOOL IsCompressedBootableImage(const char* path)
if (buf == NULL)
return FALSE;
FormatStatus = 0;
bled_init(_uprintf, NULL, NULL, NULL, NULL, &FormatStatus);
bled_init(uprintf, NULL, NULL, NULL, NULL, &FormatStatus);
dc = bled_uncompress_to_buffer(path, (char*)buf, MBR_SIZE, file_assoc[i].type);
bled_exit();
if (dc != MBR_SIZE) {
Expand Down
3 changes: 3 additions & 0 deletions src/wue.c
Original file line number Diff line number Diff line change
Expand Up @@ -673,6 +673,8 @@ BOOL SetupWinToGo(DWORD DriveIndex, const char* drive_name, BOOL use_esp)
static_sprintf(cmd, "%s\\bcdboot.exe %s\\Windows /v /f %s /s %s", sysnative_dir, drive_name,
HAS_BOOTMGR_BIOS(img_report) ? (HAS_BOOTMGR_EFI(img_report) ? "ALL" : "BIOS") : "UEFI",
(use_esp) ? ms_efi : drive_name);
// I don't believe we can ever have a stray '%' in cmd, but just in case...
assert(strchr(cmd, '%') == NULL);
uprintf(cmd);
if (RunCommand(cmd, sysnative_dir, usb_debug) != 0) {
// Try to continue... but report a failure
Expand All @@ -698,6 +700,7 @@ BOOL SetupWinToGo(DWORD DriveIndex, const char* drive_name, BOOL use_esp)
uprintf("Disabling use of the Windows Recovery Environment using command:");
static_sprintf(cmd, "%s\\bcdedit.exe /store %s\\EFI\\Microsoft\\Boot\\BCD /set {default} recoveryenabled no",
sysnative_dir, (use_esp) ? ms_efi : drive_name);
assert(strchr(cmd, '%') == NULL);
uprintf(cmd);
RunCommand(cmd, sysnative_dir, usb_debug);

Expand Down

0 comments on commit fffd4d1

Please sign in to comment.