I verified that the current waveplayer.c still allocates str as FILEMGR_FILE_NAME_SIZE + 20 and then formats the filename into it using unbounded sprintf(). ([GitHub]1) With a 40-character filename, even "Playing file (1/1): " + filename + NUL requires 61 bytes, already exceeding a 60-byte buffer. The missing termination from the earlier strncpy() can make the condition worse.
Buffer overflow in GetFileInfo() when displaying long WAV filenames
Describe the set-up
- Board: STM32F769I-DISCO / STM32F769I-Discovery
- Example:
Projects/STM32F769I-Discovery/Applications/Audio/Audio_playback_and_record
- Affected revision:
c2ecfd2d
- Affected file:
Src/waveplayer.c
- Affected function:
GetFileInfo()
The issue is independent of a specific compiler and is caused by an unchecked formatted write to a fixed-size stack buffer.
Describe the bug
GetFileInfo() uses sprintf() to construct a display string containing the selected WAV filename:
uint8_t str[FILEMGR_FILE_NAME_SIZE + 20];
sprintf((char *)str, "Playing file (%d/%d): %s",
file_idx + 1, FileList.ptr,
(char *)FileList.file[file_idx].name);
FILEMGR_FILE_NAME_SIZE is 40, so str is 60 bytes.
However, the fixed text alone requires 20 bytes even for single-digit indices:
A 40-character filename therefore requires at least:
20-byte prefix + 40-byte filename + 1-byte NUL = 61 bytes
This exceeds the 60-byte destination buffer. Larger values of file_idx or FileList.ptr increase the required size further.
In addition, during file enumeration the filename is copied using:
strncpy((char *)FileList.file[FileList.ptr].name,
(char *)fn, FILEMGR_FILE_NAME_SIZE);
If the source filename is at least FILEMGR_FILE_NAME_SIZE bytes long, strncpy() does not append a terminating NUL byte. The later %s conversion in sprintf() can therefore also read beyond the filename buffer until an unrelated NUL byte is encountered.
As a result, a sufficiently long filename on the removable media can cause an out-of-bounds read followed by a stack buffer overflow in GetFileInfo().
How To Reproduce
-
Build and run the STM32F769I-Discovery Audio_playback_and_record application normally.
-
Place a valid WAV file on the removable storage with a filename close to the maximum length accepted by the file manager, for example a 40-character filename.
-
Insert the storage and allow the application to enumerate the WAV files.
-
Select the long-named WAV file for playback.
-
GetFileInfo() executes:
sprintf((char *)str, "Playing file (%d/%d): %s",
file_idx + 1, FileList.ptr,
(char *)FileList.file[file_idx].name);
The resulting string exceeds the size of str, causing memory beyond the stack buffer to be overwritten. If the filename was copied without a terminating NUL, %s may additionally read beyond the filename field.
Depending on the surrounding stack layout, the issue may result in memory corruption or a crash.
Additional context
The issue can be addressed by both ensuring that stored filenames are always NUL-terminated and using a bounded formatting function.
For example, the display operation can be changed to:
snprintf((char *)str, sizeof(str),
"Playing file (%d/%d): %s",
file_idx + 1, FileList.ptr,
(char *)FileList.file[file_idx].name);
The filename copy should also reserve space for and explicitly add a terminator, for example:
strncpy((char *)FileList.file[FileList.ptr].name,
(char *)fn,
FILEMGR_FILE_NAME_SIZE - 1);
FileList.file[FileList.ptr].name[FILEMGR_FILE_NAME_SIZE - 1] = '\0';
Using both changes prevents the unterminated-source read and bounds the formatted write into str.
I verified that the current
waveplayer.cstill allocatesstrasFILEMGR_FILE_NAME_SIZE + 20and then formats the filename into it using unboundedsprintf(). ([GitHub]1) With a 40-character filename, even"Playing file (1/1): "+ filename + NUL requires 61 bytes, already exceeding a 60-byte buffer. The missing termination from the earlierstrncpy()can make the condition worse.Buffer overflow in
GetFileInfo()when displaying long WAV filenamesDescribe the set-up
Projects/STM32F769I-Discovery/Applications/Audio/Audio_playback_and_recordc2ecfd2dSrc/waveplayer.cGetFileInfo()The issue is independent of a specific compiler and is caused by an unchecked formatted write to a fixed-size stack buffer.
Describe the bug
GetFileInfo()usessprintf()to construct a display string containing the selected WAV filename:FILEMGR_FILE_NAME_SIZEis 40, sostris 60 bytes.However, the fixed text alone requires 20 bytes even for single-digit indices:
A 40-character filename therefore requires at least:
This exceeds the 60-byte destination buffer. Larger values of
file_idxorFileList.ptrincrease the required size further.In addition, during file enumeration the filename is copied using:
If the source filename is at least
FILEMGR_FILE_NAME_SIZEbytes long,strncpy()does not append a terminating NUL byte. The later%sconversion insprintf()can therefore also read beyond the filename buffer until an unrelated NUL byte is encountered.As a result, a sufficiently long filename on the removable media can cause an out-of-bounds read followed by a stack buffer overflow in
GetFileInfo().How To Reproduce
Build and run the STM32F769I-Discovery
Audio_playback_and_recordapplication normally.Place a valid WAV file on the removable storage with a filename close to the maximum length accepted by the file manager, for example a 40-character filename.
Insert the storage and allow the application to enumerate the WAV files.
Select the long-named WAV file for playback.
GetFileInfo()executes:The resulting string exceeds the size of
str, causing memory beyond the stack buffer to be overwritten. If the filename was copied without a terminating NUL,%smay additionally read beyond the filename field.Depending on the surrounding stack layout, the issue may result in memory corruption or a crash.
Additional context
The issue can be addressed by both ensuring that stored filenames are always NUL-terminated and using a bounded formatting function.
For example, the display operation can be changed to:
The filename copy should also reserve space for and explicitly add a terminator, for example:
Using both changes prevents the unterminated-source read and bounds the formatted write into
str.