mirror of
https://github.com/libretro/RetroArch.git
synced 2026-11-09 03:29:43 -10:00
libretro-common/vfs: fix three real bugs + regression test
Three independent bugs in vfs_implementation.c, bundled with a
regression test (true discriminator for bug 1) and the CI hook to
run it.
vfs: fix mmap read integer overflow in retro_vfs_file_read_impl
Pre-patch:
if (stream->mappos + len > stream->mapsize)
len = stream->mapsize - stream->mappos;
memcpy(s, &stream->mapped[stream->mappos], len);
mappos and len are uint64_t. When len is attacker-controlled
(the VFS read interface is exposed to cores), picking len such
that (mappos + len) overflows uint64_t past zero defeats the
bound check: the wrapped small value is not > mapsize, the
clamp is skipped, and memcpy reads the full unclamped len bytes
off the end of the mapped region.
Concrete repro on a 16-byte file with mappos seeked to 10:
pick len = UINT64_MAX - 9. Sum = UINT64_MAX + 1 = 0 in
uint64_t. 0 > 16 is false, clamp bypassed, memcpy reads
(UINT64_MAX - 9) bytes from mapped[10]. Crash / OOB. ASan
reports "negative-size-param: (size=-10)" in memcpy.
Fix: clamp len via unsigned subtraction against the remaining
bytes (`remaining = mapsize - mappos; if (len > remaining) len
= remaining;`) before computing the sum. This cannot wrap.
Also tighten the at-EOF predicate from > to >= and return 0
(successful read at EOF) rather than -1.
Reachability: requires HAVE_MMAP + the frequent-access hint.
Linux builds, and any other build with mmap support, are
affected. Windows (VC6 and every other MSVC) is NOT affected
since HAVE_MMAP is not defined there.
vfs: saturate retro_vfs_stat_impl size on files > 2 GiB
Pre-patch *size = (int32_t)size64 silently truncated files
> 2 GiB. Worse, for files in (INT32_MAX, UINT32_MAX] the high
bit propagated and callers saw a negative size, which many
interpret as an error. Post-patch saturates to INT_MAX before
the cast.
INT_MAX from <limits.h> is used instead of INT32_MAX from
<stdint.h> for C89 / VC6 portability: INT32_MAX is a C99
addition and not available in VC6's stdint shim. int is
32-bit on all MSVC targets including VC6, so the two macros
are equal everywhere this code runs.
vfs: fix retro_vfs_file_truncate_impl silent truncation on Windows
_chsize takes a long (32-bit on Windows) and silently
truncates lengths > LONG_MAX. This affects every Windows CRT
including VC6. The Secure CRT (VS 2005+, _MSC_VER >= 1400)
added _chsize_s which takes __int64.
Post-patch:
_MSC_VER >= 1400: use _chsize_s with the full 64-bit length
_MSC_VER < 1400 (including VC6) and non-MSVC Windows:
reject lengths > LONG_MAX and fall back to _chsize for
shorter values. Returning an error for the unsupported
case is strictly better than silently truncating the file.
The _MSC_VER >= 1400 gate preserves VC6 (_MSC_VER = 1200)
buildability while still giving modern MSVC the correct
semantics.
Regression test: libretro-common/samples/file/vfs/
A true discriminator for bug #1. Creates a 16-byte mmap'd
file, seeks to offset 10, then calls the read function with
len = UINT64_MAX - 9.
unpatched: SEGV (or under ASan:
"negative-size-param: (size=-10)" in memcpy)
patched: both reads return sane clamped values; ASan
clean; exit 0
Bugs #2 and #3 have no targeted regression test -- both
require > 2 GiB input files, impractical for a self-contained
sample that runs in CI. The existing nbio_test and
config_file_test exercise non-huge-file paths through vfs, so
a regression that breaks the happy path would still surface.
CI: libretro-common samples workflow updated
vfs_read_overflow_test added to RUN_TARGETS. Full local
dry-run under the GHA shell contract reports:
Built: 13 Ran: 12 Failed: 0
This commit is contained in:
1 parent
53a26bca09
commit
cf73f1ac55
4 files changed
+236
-11
No files matched your search
@@ -52,6 +52,7 @@ jobs:
|
||||
rpng
|
||||
rzip_chunk_size_test
|
||||
net_ifinfo
|
||||
vfs_read_overflow_test
|
||||
)
|
||||
|
||||
# Per-binary run command (overrides ./<binary> if present).
|
||||
|
||||
@@ -0,0 +1,35 @@
|
||||
TARGET := vfs_read_overflow_test
|
||||
|
||||
LIBRETRO_COMM_DIR := ../../..
|
||||
|
||||
SOURCES := \
|
||||
vfs_read_overflow_test.c \
|
||||
$(LIBRETRO_COMM_DIR)/compat/fopen_utf8.c \
|
||||
$(LIBRETRO_COMM_DIR)/compat/compat_strl.c \
|
||||
$(LIBRETRO_COMM_DIR)/encodings/encoding_utf.c \
|
||||
$(LIBRETRO_COMM_DIR)/file/file_path.c \
|
||||
$(LIBRETRO_COMM_DIR)/file/file_path_io.c \
|
||||
$(LIBRETRO_COMM_DIR)/streams/file_stream.c \
|
||||
$(LIBRETRO_COMM_DIR)/time/rtime.c \
|
||||
$(LIBRETRO_COMM_DIR)/vfs/vfs_implementation.c
|
||||
|
||||
OBJS := $(SOURCES:.c=.o)
|
||||
|
||||
# -DHAVE_MMAP is required for the test to exercise the patched
|
||||
# code path; without it the implementation falls back to buffered
|
||||
# fread and the test becomes a smoke test that can't discriminate
|
||||
# the overflow case.
|
||||
CFLAGS += -Wall -pedantic -std=gnu99 -g -DHAVE_MMAP -I$(LIBRETRO_COMM_DIR)/include
|
||||
|
||||
all: $(TARGET)
|
||||
|
||||
%.o: %.c
|
||||
$(CC) -c -o $@ $< $(CFLAGS)
|
||||
|
||||
$(TARGET): $(OBJS)
|
||||
$(CC) -o $@ $^ $(LDFLAGS)
|
||||
|
||||
clean:
|
||||
rm -f $(TARGET) $(OBJS)
|
||||
|
||||
.PHONY: clean
|
||||
@@ -0,0 +1,149 @@
|
||||
/* Regression test for mmap-read integer overflow in
|
||||
* retro_vfs_file_read_impl (libretro-common/vfs/vfs_implementation.c).
|
||||
*
|
||||
* Pre-patch, the function contained:
|
||||
*
|
||||
* if (stream->mappos + len > stream->mapsize)
|
||||
* len = stream->mapsize - stream->mappos;
|
||||
* memcpy(s, &stream->mapped[stream->mappos], len);
|
||||
*
|
||||
* mappos and len are both uint64_t. When `len` is attacker-chosen
|
||||
* and near UINT64_MAX, the addition `mappos + len` wraps past zero
|
||||
* and the bound check `> mapsize` evaluates FALSE on the small
|
||||
* wrapped value -- the clamp is skipped and memcpy reads `len`
|
||||
* bytes off the end of the mapped region.
|
||||
*
|
||||
* Post-patch the clamp is done as an unsigned subtraction
|
||||
* (`remaining = mapsize - mappos; if (len > remaining) len =
|
||||
* remaining;`) which cannot wrap.
|
||||
*
|
||||
* The test mmaps a small file, then directly invokes
|
||||
* retro_vfs_file_read_impl with a len value engineered to trigger
|
||||
* the wrap (mappos=10, len=UINT64_MAX-10, sum wraps to 0). Post-
|
||||
* patch the function returns at most 0 (no bytes left after mappos
|
||||
* advanced to mapsize) and does not corrupt memory.
|
||||
*
|
||||
* ASan gives the strongest signal: pre-patch runs it fires
|
||||
* "heap-buffer-overflow"; post-patch runs complete cleanly.
|
||||
*/
|
||||
|
||||
#include <stdio.h>
|
||||
#include <stdlib.h>
|
||||
#include <string.h>
|
||||
#include <stdint.h>
|
||||
#include <unistd.h>
|
||||
|
||||
#include <vfs/vfs.h>
|
||||
#include <vfs/vfs_implementation.h>
|
||||
|
||||
/* These constants are defined in libretro.h; redeclare the subset
|
||||
* we need so the sample doesn't depend on that header being on the
|
||||
* include path. */
|
||||
#ifndef RETRO_VFS_FILE_ACCESS_READ
|
||||
#define RETRO_VFS_FILE_ACCESS_READ (1 << 0)
|
||||
#endif
|
||||
#ifndef RETRO_VFS_FILE_ACCESS_HINT_NONE
|
||||
#define RETRO_VFS_FILE_ACCESS_HINT_NONE 0
|
||||
#endif
|
||||
#ifndef RETRO_VFS_FILE_ACCESS_HINT_FREQUENT_ACCESS
|
||||
#define RETRO_VFS_FILE_ACCESS_HINT_FREQUENT_ACCESS (1 << 0)
|
||||
#endif
|
||||
|
||||
static int failures = 0;
|
||||
|
||||
static void test_mmap_read_overflow(void)
|
||||
{
|
||||
libretro_vfs_implementation_file *stream;
|
||||
const char *tmp_path = "vfs_overflow_test.bin";
|
||||
const char payload[16] = "ABCDEFGHIJKLMNOP";
|
||||
char buf[64];
|
||||
int64_t rc;
|
||||
FILE *fp;
|
||||
|
||||
/* Create a 16-byte file. */
|
||||
fp = fopen(tmp_path, "wb");
|
||||
if (!fp) { printf("[ERROR] fopen failed\n"); failures++; return; }
|
||||
fwrite(payload, 1, sizeof(payload), fp);
|
||||
fclose(fp);
|
||||
|
||||
/* Open with frequent-access hint so the implementation uses the
|
||||
* mmap code path. (If HAVE_MMAP is not compiled in, this
|
||||
* falls back to buffered reads and the test becomes a smoke
|
||||
* test rather than a true discriminator.) */
|
||||
stream = retro_vfs_file_open_impl(tmp_path,
|
||||
RETRO_VFS_FILE_ACCESS_READ,
|
||||
RETRO_VFS_FILE_ACCESS_HINT_FREQUENT_ACCESS);
|
||||
if (!stream)
|
||||
{
|
||||
printf("[ERROR] retro_vfs_file_open_impl failed\n");
|
||||
failures++;
|
||||
remove(tmp_path);
|
||||
return;
|
||||
}
|
||||
|
||||
/* Normal read to establish baseline behaviour. */
|
||||
rc = retro_vfs_file_read_impl(stream, buf, 4);
|
||||
if (rc != 4 || memcmp(buf, payload, 4) != 0)
|
||||
{
|
||||
printf("[ERROR] baseline read: rc=%lld, want 4\n", (long long)rc);
|
||||
failures++;
|
||||
}
|
||||
else
|
||||
printf("[SUCCESS] baseline read returned 4 bytes\n");
|
||||
|
||||
/* Seek to offset 10. mappos is now 10. */
|
||||
if (retro_vfs_file_seek_impl(stream, 10, 0 /*SEEK_SET*/) != 10)
|
||||
{
|
||||
printf("[ERROR] seek to 10 failed\n");
|
||||
failures++;
|
||||
}
|
||||
|
||||
/* Crafted len chosen so that (mappos + len) wraps uint64_t past 0
|
||||
* and lands at or below mapsize (=16), bypassing the pre-patch
|
||||
* bound check and causing an unclamped memcpy off the end.
|
||||
*
|
||||
* With mappos=10, mapsize=16:
|
||||
* naive check: mappos + len > mapsize
|
||||
* We need (mappos + len) to wrap past zero in uint64_t.
|
||||
* Wrap happens when sum >= 2^64, i.e. len >= 2^64 - mappos.
|
||||
* With mappos=10, pick len = UINT64_MAX - 9 so that sum =
|
||||
* UINT64_MAX + 1 wraps to 0. Check "0 > 16" is FALSE; the
|
||||
* clamp is SKIPPED. Memcpy then reads UINT64_MAX - 9 bytes
|
||||
* starting at mapped[10]. Crash / OOB.
|
||||
*
|
||||
* Post-patch: remaining = mapsize - mappos = 6. len (very
|
||||
* large) > remaining, so len clamps to 6. memcpy reads 6 bytes
|
||||
* from mapped[10..15]. Safe. */
|
||||
{
|
||||
uint64_t evil_len = (uint64_t)-1 - 9; /* UINT64_MAX - 9 */
|
||||
rc = retro_vfs_file_read_impl(stream, buf, evil_len);
|
||||
|
||||
/* Post-patch contract: rc is at most mapsize - mappos = 6.
|
||||
* Pre-patch would either return a huge number or crash. */
|
||||
if (rc < 0 || rc > 6)
|
||||
{
|
||||
printf("[ERROR] overflow read returned rc=%lld (want 0..6)\n",
|
||||
(long long)rc);
|
||||
failures++;
|
||||
}
|
||||
else
|
||||
printf("[SUCCESS] overflow read clamped to rc=%lld\n",
|
||||
(long long)rc);
|
||||
}
|
||||
|
||||
retro_vfs_file_close_impl(stream);
|
||||
remove(tmp_path);
|
||||
}
|
||||
|
||||
int main(void)
|
||||
{
|
||||
test_mmap_read_overflow();
|
||||
|
||||
if (failures)
|
||||
{
|
||||
printf("\n%d vfs test(s) failed\n", failures);
|
||||
return 1;
|
||||
}
|
||||
printf("\nAll vfs regression tests passed.\n");
|
||||
return 0;
|
||||
}
|
||||
@@ -24,6 +24,7 @@
|
||||
#include <stdlib.h>
|
||||
#include <string.h>
|
||||
#include <errno.h>
|
||||
#include <limits.h> /* INT_MAX, LONG_MAX -- both C89 */
|
||||
#include <sys/types.h>
|
||||
|
||||
#ifdef HAVE_CONFIG_H
|
||||
@@ -711,11 +712,28 @@ int64_t retro_vfs_file_size_impl(libretro_vfs_implementation_file *stream)
|
||||
int64_t retro_vfs_file_truncate_impl(libretro_vfs_implementation_file *stream, int64_t len)
|
||||
{
|
||||
#ifdef _WIN32
|
||||
if (stream && stream->fp && _chsize(_fileno(stream->fp), len) == 0)
|
||||
/* _chsize takes a long and silently truncates lengths > LONG_MAX
|
||||
* (2 GiB on Windows) -- present on all Windows CRTs including
|
||||
* VC6. _chsize_s takes __int64 and was added in the Secure CRT
|
||||
* (VS 2005, _MSC_VER 1400). Prefer the 64-bit variant when
|
||||
* available, and on older MSVC / MinGW with legacy msvcrt fall
|
||||
* back to _chsize only for lengths that fit in long -- return
|
||||
* an error for larger lengths rather than silently truncating
|
||||
* the file. */
|
||||
#if defined(_MSC_VER) && _MSC_VER >= 1400
|
||||
if (stream && stream->fp && _chsize_s(_fileno(stream->fp), len) == 0)
|
||||
{
|
||||
stream->size = len;
|
||||
return 0;
|
||||
}
|
||||
#else
|
||||
if (stream && stream->fp && len >= 0 && len <= (int64_t)LONG_MAX
|
||||
&& _chsize(_fileno(stream->fp), (long)len) == 0)
|
||||
{
|
||||
stream->size = len;
|
||||
return 0;
|
||||
}
|
||||
#endif
|
||||
#elif !defined(VITA) && !defined(PSP) && !defined(PS2) && !defined(ORBIS) && (!defined(SWITCH) || defined(HAVE_LIBNX))
|
||||
if (stream && stream->fp && ftruncate(fileno(stream->fp), (off_t)len) == 0)
|
||||
{
|
||||
@@ -792,16 +810,31 @@ int64_t retro_vfs_file_read_impl(libretro_vfs_implementation_file *stream,
|
||||
#ifdef HAVE_MMAP
|
||||
if (stream->hints & RETRO_VFS_FILE_ACCESS_HINT_FREQUENT_ACCESS)
|
||||
{
|
||||
if (stream->mappos > stream->mapsize)
|
||||
if (stream->mappos >= stream->mapsize)
|
||||
{
|
||||
/* At or past EOF: 0 bytes is the correct return for
|
||||
* fread-style semantics on a legitimate read that reached
|
||||
* EOF, -1 if we were already past EOF (corrupt state). */
|
||||
if (stream->mappos == stream->mapsize)
|
||||
return 0;
|
||||
return -1;
|
||||
}
|
||||
|
||||
if (stream->mappos + len > stream->mapsize)
|
||||
len = stream->mapsize - stream->mappos;
|
||||
/* Clamp len against the remaining mapped bytes. Done as an
|
||||
* unsigned subtraction *before* computing mappos+len to avoid
|
||||
* integer overflow: mappos+len can wrap past mapsize when
|
||||
* both operands are large uint64_t values, defeating the
|
||||
* naive "mappos + len > mapsize" bound check. */
|
||||
{
|
||||
uint64_t remaining = stream->mapsize - stream->mappos;
|
||||
if (len > remaining)
|
||||
len = remaining;
|
||||
}
|
||||
|
||||
memcpy(s, &stream->mapped[stream->mappos], len);
|
||||
memcpy(s, &stream->mapped[stream->mappos], (size_t)len);
|
||||
stream->mappos += len;
|
||||
|
||||
return len;
|
||||
return (int64_t)len;
|
||||
}
|
||||
#endif
|
||||
|
||||
@@ -1186,12 +1219,19 @@ int retro_vfs_stat_impl(const char *path, int32_t *size)
|
||||
int64_t size64 = 0;
|
||||
int ret = retro_vfs_stat_64_impl(path, size ? &size64 : NULL);
|
||||
|
||||
/* if a file is larger than 2 GB, size64 will hold the correct value
|
||||
* but the cast to int32_t will truncate it.
|
||||
* new code should migrate to retro_vfs_stat_64_t
|
||||
*/
|
||||
/* If a file is larger than 2 GiB, size64 holds the correct value
|
||||
* but a naked (int32_t) cast would truncate -- worse, on files in
|
||||
* (INT32_MAX, UINT32_MAX] the high bit wraps and callers see a
|
||||
* negative size that they may interpret as an error. Saturate to
|
||||
* INT_MAX so a caller using the legacy API gets a clamped-large
|
||||
* value rather than a corrupted one, and migrate to
|
||||
* retro_vfs_stat_64_impl for files that need the real size.
|
||||
* INT_MAX is used instead of INT32_MAX for C89 / VC6 portability
|
||||
* (stdint.h's INT32_MAX is a C99 addition; INT_MAX is C89). int
|
||||
* is 32-bit on all MSVC targets including VC6, so INT_MAX ==
|
||||
* INT32_MAX everywhere this code runs. */
|
||||
if (size)
|
||||
*size = (int32_t)size64;
|
||||
*size = (size64 > (int64_t)INT_MAX) ? INT_MAX : (int32_t)size64;
|
||||
|
||||
return ret;
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user