Security: heap- and stack-overflow fixes across image, audio, IO and cheevos paths

Fixes eight memory-safety bugs sharing a small set of root causes:
uint32_t multiplications feeding size_t allocations, ftell() and
filestream_get_size() returning -1 silently coerced to size_t,
attacker-controlled filenames used as filesystem paths, and
per-channel-count assumptions in audio decode.

* rjpeg, rpng (formats/jpeg/rjpeg.c, formats/png/rpng.c)
  Width*height*sizeof(uint32_t) wraps in uint32 on 32-bit hosts
  (3DS, Vita, PSP, Wii, Wii U, older Android, 32-bit Windows),
  the malloc returns an undersized buffer, and the per-row decode
  writes multi-GiB off the end.  Add (size_t) casts at every
  pixel-buffer malloc site so the multiplication is overflow-safe
  on 64-bit.  Add a 0x4000 dimension cap matching rbmp.c, rtga.c
  and rwebp.c, gated on 32-bit only -- on 64-bit the size_t
  arithmetic plus the existing 4 GiB output guard in
  rpng_iterate_image are sufficient, and a hard cap would reject
  legitimate large images (IrfanView and friends routinely open
  tens-of-thousands-pixel JPEGs and PNGs).

* image_transfer Gekko path (formats/image_transfer.c)
  Same uint32 multiplication primitive in the post-decode tile
  conversion on Wii.  Add (size_t) casts.  The 32-bit rpng/rjpeg
  cap closes the primitive at the source on Wii, but the casts
  are kept for defence in depth.

* audio_mixer FLAC (audio/audio_mixer.c)
  audio_mixer_mix_flac asks drflac for AUDIO_MIXER_TEMP_BUFFER/2
  frames into a 32 KiB stack buffer.  drflac writes
  frame_count*channel_count floats, so 8-channel FLAC writes
  128 KiB into the 32 KiB buffer -- a 96 KiB stack overflow
  reaching saved-RIP territory.  Reject non-stereo FLAC at
  audio_mixer_play_flac time.  Mono was already producing wrong
  audio per the existing comment in mix_flac; a proper per-
  channel-aware fix is left for a separate change.

* cheevos badge_name (cheevos/cheevos_client.c)
  rcheevos_client_download_badge interpolates the server-supplied
  badge_name into a filesystem path and writes the HTTP body
  there.  A malicious or MITM'd retroachievements.org could send
  badge_name = "../../../../etc/cron.d/evil" and write attacker-
  controlled bytes (with a forced .png suffix) anywhere on disk.
  Real badge names are numeric IDs optionally suffixed with
  "_lock"; reject anything outside [A-Za-z0-9_-].

* cheevos JSON-override loader (cheevos/cheevos_client.c)
  ftell()'s long return was assigned to size_t _len, so an ftell
  error (-1) became SIZE_MAX; malloc(_len + 1) wrapped to
  malloc(0) and contents[_len] = 0 corrupted the heap.  Capture
  into long, check for the negative sentinel.  Debug-only path
  (CHEEVOS_JSON_OVERRIDE).

* rxml (formats/xml/rxml.c)
  Same pattern, file-scale: filestream_get_size returns -1 on
  error and silently flowed into (size_t)(len + 1) as 0 on 64-bit
  (malloc(0) -> tiny block) or as a wrapped value on 32-bit
  (undersized buffer).  Either way memory_buffer[len] = '\0'
  wrote far out of bounds.  Reject negative sizes and any size
  that wouldn't fit in size_t.  Reachable via .xml shader presets
  and Logiqx .dat database files.

* filestream_read_file (streams/file_stream.c)
  The existing size check was
    if ((int64_t)(uint64_t)(size + 1) != (size + 1)) goto error;
  -- tautological for any positive int64_t, never trips.  On
  32-bit hosts any file > ~4 GiB silently truncates through the
  size_t cast on the malloc and the subsequent filestream_read
  overruns the heap.  Replace with an explicit size_t-fit check.

* test/formats/test_rpng.c (new)
  Six libcheck cases covering rpng_process_ihdr.  The two that
  exercise the 0x4000 cap (0x4001 and 30000^2) are gated on 32-
  bit hosts only since on 64-bit those dimensions are legitimate
  after this change.  The remaining four (accept-at-limit,
  uint32-max-reject, accept-small, zero-rejected) run on every
  platform.  Wired into Makefile.test alongside the existing
  string/utils/hash/lists/queues suites; builds with -Werror
  under the same TEST_UNIT_CFLAGS the other tests use; pulls in
  trans_stream*.c + -lz for the zlib backend reference rpng.c
  links to.
This commit is contained in:
U-DESKTOP-SPFP6AQ\twistedtechre committed 2026-04-27 21:46:16 +02:00
1 parent 90b96a3039
commit 7335b37dc7
9 files changed
+177 -14

No files matched your search

+49 -1
View File
@@ -294,6 +294,7 @@ void rcheevos_client_http_load_response(const rc_api_request_t* request,
rc_client_server_callback_t callback, void* callback_data)
{
char* contents;
long ftell_ret;
size_t _len = 0;
FILE* file = fopen(CHEEVOS_JSON_OVERRIDE, "rb");
@@ -311,9 +312,23 @@ void rcheevos_client_http_load_response(const rc_api_request_t* request,
}
fseek(file, 0, SEEK_END);
_len = ftell(file);
ftell_ret = ftell(file);
fseek(file, 0, SEEK_SET);
/* ftell can return -1 on stream error. Pre-patch _len was
* declared size_t and assigned directly from ftell, so a
* negative return became (size_t)-1 == SIZE_MAX, malloc
* (SIZE_MAX + 1) wrapped to malloc(0) returning a tiny
* non-NULL block, and the subsequent contents[_len] = 0
* write at offset SIZE_MAX corrupted the heap. */
if (ftell_ret < 0)
{
fclose(file);
callback(NULL, 0, callback_data);
return;
}
_len = (size_t)ftell_ret;
contents = (char*)malloc(_len + 1);
/* NULL-check the malloc: without this the contents[_len] = 0
* write below and the fread into contents NULL-deref. */
@@ -489,6 +504,39 @@ bool rcheevos_client_download_badge(rc_client_download_queue_t* queue,
fill_pathname_slash(badge_fullpath, sizeof(badge_fullpath));
badge_fullname = badge_fullpath + strlen(badge_fullpath);
badge_fullname_size = sizeof(badge_fullpath) - (badge_fullname - badge_fullpath);
/* badge_name is supplied by the achievement server (or, on a
* compromised TLS path, an attacker-controlled MITM).
* fill_pathname_slash ensures we are anchored under the
* badges directory, but if badge_name contains '..', '/', '\\'
* or other path-component separators the resulting filesystem
* write escapes that directory. Validate that badge_name is
* a single safe filename component (alphanumerics plus '_' and
* '-' suffixed by '_lock' on the lock variant); the underscore
* is enough because real badge names from the server are
* numeric IDs ("12345") or numeric IDs with a "_lock" suffix.
* Reject anything else rather than synthesising a sanitised
* version, since a bogus badge name from the server is itself
* a signal that something is wrong. */
{
const char *p;
bool ok = (badge_name && *badge_name);
for (p = badge_name; ok && *p; p++)
{
char c = *p;
if (!( (c >= '0' && c <= '9')
|| (c >= 'a' && c <= 'z')
|| (c >= 'A' && c <= 'Z')
|| c == '_' || c == '-'))
ok = false;
}
if (!ok)
{
CHEEVOS_LOG(RCHEEVOS_TAG "Rejecting badge with unsafe name.\n");
return false;
}
}
snprintf(badge_fullname, badge_fullname_size, "%s" FILE_PATH_PNG_EXTENSION, badge_name);
if (path_is_valid(badge_fullpath))
+12 -1
View File
@@ -23,6 +23,12 @@ TEST_HASH_SRC = test/hash/test_hash.c hash/lrc_hash.c \
streams/file_stream.c vfs/vfs_implementation.c file/file_path.c \
compat/compat_strl.c time/rtime.c string/stdstring.c encodings/encoding_utf.c
TEST_RPNG = test/formats/test_rpng
TEST_RPNG_SRC = test/formats/test_rpng.c formats/png/rpng.c \
streams/trans_stream.c streams/trans_stream_zlib.c \
streams/trans_stream_pipe.c
TEST_RPNG_LIBS = -lz
all:
# Build and execute tests in order, to avoid coverage file collision
# string
@@ -45,12 +51,17 @@ all:
$(CC) $(TEST_UNIT_CFLAGS) $(TEST_GENERIC_QUEUE_SRC) -o $(TEST_GENERIC_QUEUE)
$(TEST_GENERIC_QUEUE)
lcov -c -d . -o `dirname $(TEST_GENERIC_QUEUE)`/coverage.info
# rpng
$(CC) $(TEST_UNIT_CFLAGS) $(TEST_RPNG_SRC) $(TEST_RPNG_LIBS) -o $(TEST_RPNG)
$(TEST_RPNG)
lcov -c -d . -o `dirname $(TEST_RPNG)`/coverage.info
lcov -o test/coverage.info \
-a test/utils/coverage.info \
-a test/string/coverage.info \
-a test/lists/coverage.info \
-a test/queues/coverage.info
-a test/queues/coverage.info \
-a test/formats/coverage.info
genhtml -o test/coverage/ test/coverage.info
clean:
+19
View File
@@ -723,6 +723,25 @@ static bool audio_mixer_play_flac(
if (!dr_flac)
return false;
/* The downstream mixer (audio_mixer_mix_flac) requests
* AUDIO_MIXER_TEMP_BUFFER / 2 frames into a stack buffer
* sized AUDIO_MIXER_TEMP_BUFFER floats. drflac writes
* frame_count * channel_count floats, so this only fits
* exactly for stereo. Mono fits but the downstream
* accounting is wrong (per existing comment); >2 channels
* overflows the stack buffer (e.g. 8-channel FLAC writes
* 4 * AUDIO_MIXER_TEMP_BUFFER floats = 4x the buffer).
* Reject anything that isn't stereo here rather than risk a
* stack overflow during mix. Mono should be fixed
* separately by adjusting the mixer's per-channel
* accounting. */
if (dr_flac->channels != 2)
{
drflac_close(dr_flac);
return false;
}
if (dr_flac->sampleRate != s_rate)
{
ratio = (double)s_rate / (double)(dr_flac->sampleRate);
+16 -2
View File
@@ -299,12 +299,26 @@ int image_transfer_process(
unsigned tmp_pitch, width2, i;
const uint16_t *src = NULL;
uint16_t *dst = NULL;
void *tmp = malloc((*width) * (*height) * sizeof(uint32_t));
/* (size_t) casts on width and height: pre-patch the uint32
* multiplication width * height * 4 wrapped on 32-bit Wii
* (Gekko is a 32-bit PowerPC) for any image with
* width*height > 2^30, the malloc returned an undersized
* buffer, and the memcpy below ran off the end. This file
* is reached only after rpng/rjpeg has already accepted the
* image; on 32-bit (which is where this matters) those
* decoders cap dimensions at 0x4000 which closes the
* primitive at the source. The casts here keep the
* arithmetic safe regardless of upstream caps and on any
* platform where image_transfer.c is compiled, including
* future 64-bit Wii-class targets. */
void *tmp = malloc(
(size_t)(*width) * (size_t)(*height) * sizeof(uint32_t));
if (!tmp)
return IMAGE_PROCESS_ERROR;
memcpy(tmp, *buf, (*width) * (*height) * sizeof(uint32_t));
memcpy(tmp, *buf,
(size_t)(*width) * (size_t)(*height) * sizeof(uint32_t));
tmp_pitch = ((*width) * sizeof(uint32_t)) >> 1;
*width &= ~3;
+24 -2
View File
@@ -2583,6 +2583,24 @@ static int rjpeg_process_frame_header(rjpeg_jpeg *z, int scan)
if (s->img_x == 0)
return 0;
/* On 32-bit hosts the (size_t) casts on the malloc sites below
* are not enough by themselves: the per-component arena and
* iter_output buffers can still demand multi-hundred-MB
* allocations that fragment or fail the address space, even
* with overflow-safe arithmetic. Cap dimensions on 32-bit
* (matching the cap rbmp.c, rtga.c and rwebp.c apply) to
* preserve the previous host-resource ceiling there.
*
* On 64-bit hosts the SIZE_MAX/4 guard further down in this
* function plus the (size_t) casts make the allocations safe
* regardless of dimensions, and a desktop user loading a
* large legitimate JPEG (cf. IrfanView, image-pipeline tools)
* is a real use case. Do not cap there. */
#if SIZE_MAX <= 0xFFFFFFFFu
if (s->img_x > 0x4000u || s->img_y > 0x4000u)
return 0;
#endif
c = rjpeg_get8(s);
/* JFIF requires */
@@ -4149,7 +4167,11 @@ static int rjpeg_iterate_init_resample(rjpeg_t *rjpeg)
r->resample = j->resample_row_hv_2_kernel;
}
rjpeg->iter_output = (uint8_t *)malloc(4 * j->img_x * j->img_y);
/* (size_t) casts: img_x and img_y are uint32_t. Even with the
* 0x4000 cap above this only matters on 32-bit hosts, but the
* cast makes the expression robust against future cap changes. */
rjpeg->iter_output = (uint8_t *)malloc(
(size_t)4 * (size_t)j->img_x * (size_t)j->img_y);
if (!rjpeg->iter_output)
return 0;
@@ -4653,7 +4675,7 @@ int rjpeg_process_image(rjpeg_t *rjpeg, void **buf_data,
/* Allocate output buffer */
proc->output = (uint8_t *) malloc(
proc->n * j->img_x * j->img_y);
(size_t)proc->n * (size_t)j->img_x * (size_t)j->img_y);
if (!proc->output)
{
rjpeg_process_free(proc);
+36 -5
View File
@@ -658,6 +658,28 @@ static bool rpng_process_ihdr(struct png_ihdr *ihdr)
return false;
}
/* On 32-bit hosts the per-row decode mallocs cannot fit much
* more than 1 GiB of decoded RGBA, and an undersized malloc
* combined with attacker-controlled dimensions has historically
* been the heap-overflow primitive prompting the 0x4000 caps
* in rbmp.c, rtga.c and rwebp.c. Keep the tight cap there.
*
* On 64-bit the (size_t) casts in rpng_reverse_filter_init and
* the final allocator make the per-row arithmetic overflow-safe
* regardless of dimensions, and the 4 GiB output guard further
* down in rpng_iterate_image already rejects images whose
* decoded buffer cannot be addressed. Loading a 30000x30000
* RGBA image on a desktop with the RAM to spare is a legitimate
* use case (cf. IrfanView), so do not impose the 0x4000 cap
* there. */
#if SIZE_MAX <= 0xFFFFFFFFu
if (ihdr->width > 0x4000u || ihdr->height > 0x4000u)
{
fprintf(stderr, "[RPNG] Error in line %d.\n", __LINE__);
return false;
}
#endif
#ifdef RPNG_TEST
fprintf(stderr, "IHDR: (%u x %u), bpc = %u, palette = %s, color = %s, alpha = %s, adam7 = %s.\n",
ihdr->width, ihdr->height,
@@ -696,6 +718,15 @@ static bool rpng_process_ihdr(struct png_ihdr *ihdr)
return false;
}
/* See the matching comment in the RPNG_TEST/DEBUG variant
* above. Cap only on 32-bit; 64-bit lets the (size_t)
* widening + 4 GiB output guard handle large legitimate
* images. */
#if SIZE_MAX <= 0xFFFFFFFFu
if (ihdr->width > 0x4000u || ihdr->height > 0x4000u)
return false;
#endif
return true;
}
#endif
@@ -1063,7 +1094,7 @@ static int rpng_reverse_filter_init(const struct png_ihdr *ihdr,
rpng_passes[pngp->pass_pos].stride_y - 1) / rpng_passes[pngp->pass_pos].stride_y;
if (!(pngp->data = (uint32_t*)malloc(
pngp->pass_width * pngp->pass_height * sizeof(uint32_t))))
(size_t)pngp->pass_width * (size_t)pngp->pass_height * sizeof(uint32_t))))
return -1;
pngp->ihdr = *ihdr;
@@ -1371,11 +1402,11 @@ end:
#ifdef GEKKO
/* We often use these in textures, make sure
* they're 32-byte aligned */
*data = (uint32_t*)memalign(32, rpng->ihdr.width *
rpng->ihdr.height * sizeof(uint32_t));
*data = (uint32_t*)memalign(32, (size_t)rpng->ihdr.width *
(size_t)rpng->ihdr.height * sizeof(uint32_t));
#else
*data = (uint32_t*)malloc(rpng->ihdr.width *
rpng->ihdr.height * sizeof(uint32_t));
*data = (uint32_t*)malloc((size_t)rpng->ihdr.width *
(size_t)rpng->ihdr.height * sizeof(uint32_t));
#endif
if (!*data)
goto false_end;
+9
View File
@@ -21,6 +21,7 @@
*/
#include <string.h>
#include <stdint.h>
#include <boolean.h>
#include <streams/file_stream.h>
@@ -101,6 +102,14 @@ rxml_document_t *rxml_load_document(const char *path)
return NULL;
len = filestream_get_size(file);
/* filestream_get_size returns -1 on error. Pre-patch this
* flowed through (size_t)(len + 1) as malloc(0) on 64-bit
* (returning a tiny non-NULL block) or as a wrapped value on
* 32-bit; either way memory_buffer[len] = '\0' wrote far
* out-of-bounds. Reject negative sizes and any size that
* would not fit in size_t on this platform. */
if (len < 0 || (uint64_t)len >= (uint64_t)((size_t)-1))
goto error;
memory_buffer = (char*)malloc((size_t)(len + 1));
if (!memory_buffer)
goto error;
+10 -2
View File
@@ -629,9 +629,17 @@ int64_t filestream_read_file(const char *path, void **buf, int64_t *len)
if ((content_buf_size = filestream_get_size(file)) < 0)
goto error;
if (!(content_buf = malloc((size_t)(content_buf_size + 1))))
/* Reject sizes that would not survive the cast to size_t for
* the malloc below. Pre-patch the only check here was a
* tautological '(int64_t)(uint64_t)X != X' (which is false
* for any positive int64_t), and on 32-bit hosts any file
* larger than ~4 GiB silently truncated through (size_t),
* the malloc was undersized, and the filestream_read below
* overran it. */
if ((uint64_t)content_buf_size + 1 > (uint64_t)((size_t)-1))
goto error;
if ((int64_t)(uint64_t)(content_buf_size + 1) != (content_buf_size + 1))
if (!(content_buf = malloc((size_t)(content_buf_size + 1))))
goto error;
if ((ret = filestream_read(file, content_buf, (int64_t)content_buf_size)) <
+2 -1
View File
@@ -7508,7 +7508,8 @@ bool MainWindow::addDirectoryFilesToList(QProgressDialog *dialog,
* Don't just extend this to add all files
* in a ZIP, because we might hit something like
* MAME/FBA where only the archives themselves
/* Only append inner file reference if filter inside archives is enabled */
* Only append inner file reference if filter
* inside archives is enabled */
if (playlistDialog->filterInArchive())
{
pathArray = (QString(pathData)