mirror of
https://github.com/libretro/RetroArch.git
synced 2026-11-09 03:29:43 -10:00
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:
1 parent
90b96a3039
commit
7335b37dc7
9 files changed
+177
-14
No files matched your search
@@ -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))
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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)) <
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in new issue
Block a user