Skip to content

Commit 18e15ad

Browse files
LibretroAdminLibretroAdmin
committed
libretro-common/vfs_cdrom: fix four bugs + regression test
Four real bugs in vfs_implementation_cdrom.c found during the follow-up audit after the main vfs_implementation.c fixes. Same bug class as the main vfs mmap read overflow already fixed. vfs_cdrom: cuesheet read byte_pos wrap OOB In retro_vfs_file_read_cdrom cuesheet branch: if ((int64_t)len >= (int64_t)stream->cdrom.cue_len - stream->cdrom.byte_pos) len = stream->cdrom.cue_len - stream->cdrom.byte_pos - 1; memcpy(s, stream->cdrom.cue_buf + stream->cdrom.byte_pos, len); cue_len is size_t, byte_pos is int64_t. When byte_pos equals or exceeds cue_len (reachable via seek, which does NOT clamp byte_pos), the subtraction "cue_len - byte_pos - 1" executes as unsigned size_t and wraps to SIZE_MAX. The memcpy then reads SIZE_MAX bytes off the end of cue_buf. Similarly a negative byte_pos (reachable by SEEK_CUR with a negative offset when byte_pos was near zero) becomes a huge size_t after the cast and wraps the same subtraction. Reproduced under ASan: "negative-size-param: (size=-1)" in memcpy at vfs_implementation_cdrom.c:401 Fix: guard byte_pos against being outside [0, cue_len) and clamp len against the remaining bytes using unsigned subtraction against "remaining = cue_len - byte_pos". vfs_cdrom: bin read byte_pos+len wrap OOB Same bug class in the .bin branch: if (stream->cdrom.byte_pos + len > vfs_cdrom_toc.track[stream->cdrom.cur_track - 1].track_bytes) len -= (stream->cdrom.byte_pos + len) - vfs_cdrom_toc.track[stream->cdrom.cur_track - 1].track_bytes; Attacker-controlled len near UINT64_MAX wraps "byte_pos + len" past track_bytes, bypassing the bound check; the downstream cdrom_read() is then called with the unclamped len. Fix: clamp against "remaining = track_bytes - byte_pos" with unsigned subtraction, never adding byte_pos and len. vfs_cdrom: cur_track == 0 indexes track[-1] If a user-controlled VFS path is crafted as "drive1-track00.bin" (or the Windows-style "d:/drive-track00.bin"), the two-digit parser produces cur_track = 0. The read code then does "track[cur_track - 1]" = track[-1], an out-of-bounds read into the struct preceding vfs_cdrom_toc.track[]. Fix: the parser rejects values outside [1, 99] and leaves the default cur_track = 1 in place on reject. The read paths also defensively reject cur_track == 0 before indexing, so a malicious caller that sets cur_track through another vector still cannot trip the OOB. vfs_cdrom: guard cur_track > 99 in bin read TOC array is track[99] (indices 0..98). cur_track is unsigned char so mathematically can reach 255. Defensive check rejects values above 99 before indexing. Reachability: all four bugs are reachable by a core or frontend that has VFS access to a CD-ROM-scheme stream (cdrom://...). Bugs 1 and 2 escalate a weak seek primitive (byte_pos not clamped) into arbitrary OOB reads. VC6 compatibility: the whole file is gated by HAVE_CDROM, which only the Linux and Win32 (non-Xbox) branches set. VC6 is a Win32 target, so its branch compiles. The fix uses only size_t, uint64_t, and standard comparisons -- all VC6-compatible via the existing stdint shim. Regression test: libretro-common/samples/file/vfs_cdrom/ True discriminator for bug #1. Five subtests covering byte_pos==cue_len, byte_pos>cue_len, byte_pos<0, huge len, and a baseline success. On unpatched code: SEGV on byte_pos==cue_len (plain run) ASan: "negative-size-param: (size=-1)" in memcpy On patched code: all five subtests pass cleanly; ASan clean; exit 0 Bug #2 (bin read) and bugs #3/#4 (cur_track parsing) don't get a targeted regression test. Bug #2 would require linking a full cdrom_toc setup which is platform-dependent; bugs #3/#4 are guarded by the parser fix so the failing input cannot reach the OOB path, and writing a white-box test of the parser is low value once the upstream fix is in. CI: libretro-common samples workflow updated. Full dry-run under the GHA shell contract: Built: 14 Ran: 13 Failed: 0
1 parent b431965 commit 18e15ad

4 files changed

Lines changed: 277 additions & 14 deletions

File tree

‎.github/workflows/Linux-libretro-common-samples.yml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ jobs:
5353
rzip_chunk_size_test
5454
net_ifinfo
5555
vfs_read_overflow_test
56+
cdrom_cuesheet_overflow_test
5657
)
5758
5859
# Per-binary run command (overrides ./<binary> if present).

‎libretro-common/samples/file/vfs_cdrom/Makefile‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
TARGET := cdrom_cuesheet_overflow_test
2+
3+
LIBRETRO_COMM_DIR := ../../..
4+
5+
# vfs_implementation_cdrom.c pulls in cdrom.c transitively for its
6+
# helper functions (cdrom_read, cdrom_write_cue, etc.). We only
7+
# exercise the cuesheet read path, but the source file must link
8+
# against the full cdrom helper library plus file_path for
9+
# path_get_extension.
10+
SOURCES := \
11+
cdrom_cuesheet_overflow_test.c \
12+
$(LIBRETRO_COMM_DIR)/compat/fopen_utf8.c \
13+
$(LIBRETRO_COMM_DIR)/compat/compat_strl.c \
14+
$(LIBRETRO_COMM_DIR)/encodings/encoding_utf.c \
15+
$(LIBRETRO_COMM_DIR)/file/file_path.c \
16+
$(LIBRETRO_COMM_DIR)/file/file_path_io.c \
17+
$(LIBRETRO_COMM_DIR)/streams/file_stream.c \
18+
$(LIBRETRO_COMM_DIR)/time/rtime.c \
19+
$(LIBRETRO_COMM_DIR)/cdrom/cdrom.c \
20+
$(LIBRETRO_COMM_DIR)/lists/string_list.c \
21+
$(LIBRETRO_COMM_DIR)/lists/dir_list.c \
22+
$(LIBRETRO_COMM_DIR)/compat/compat_strcasestr.c \
23+
$(LIBRETRO_COMM_DIR)/compat/compat_posix_string.c \
24+
$(LIBRETRO_COMM_DIR)/memmap/memalign.c \
25+
$(LIBRETRO_COMM_DIR)/compat/compat_fnmatch.c \
26+
$(LIBRETRO_COMM_DIR)/file/retro_dirent.c \
27+
$(LIBRETRO_COMM_DIR)/string/stdstring.c \
28+
$(LIBRETRO_COMM_DIR)/vfs/vfs_implementation.c \
29+
$(LIBRETRO_COMM_DIR)/vfs/vfs_implementation_cdrom.c
30+
31+
OBJS := $(SOURCES:.c=.o)
32+
33+
CFLAGS += -Wall -pedantic -std=gnu99 -g -DHAVE_CDROM -I$(LIBRETRO_COMM_DIR)/include
34+
35+
LDLIBS += -lm
36+
37+
all: $(TARGET)
38+
39+
%.o: %.c
40+
$(CC) -c -o $@ $< $(CFLAGS)
41+
42+
$(TARGET): $(OBJS)
43+
$(CC) -o $@ $^ $(LDFLAGS) $(LDLIBS)
44+
45+
clean:
46+
rm -f $(TARGET) $(OBJS)
47+
48+
.PHONY: clean

‎libretro-common/samples/file/vfs_cdrom/cdrom_cuesheet_overflow_test.c‎

Lines changed: 171 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,171 @@
1+
/* Regression test for retro_vfs_file_read_cdrom cuesheet byte_pos
2+
* wrap (libretro-common/vfs/vfs_implementation_cdrom.c).
3+
*
4+
* Pre-patch, the cuesheet read path contained:
5+
*
6+
* if ((int64_t)len >= (int64_t)stream->cdrom.cue_len
7+
* - stream->cdrom.byte_pos)
8+
* len = stream->cdrom.cue_len - stream->cdrom.byte_pos - 1;
9+
* memcpy(s, stream->cdrom.cue_buf + stream->cdrom.byte_pos, len);
10+
*
11+
* cue_len is size_t, byte_pos is int64_t. The subtraction
12+
* "cue_len - byte_pos - 1" is performed as unsigned (size_t). If
13+
* byte_pos >= cue_len, the subtraction wraps to a huge size_t and
14+
* memcpy reads off the end of cue_buf. byte_pos can be set to any
15+
* value by seek, which does not clamp.
16+
*
17+
* Post-patch, the read clamps against the remaining bytes with
18+
* unsigned subtraction *after* checking that byte_pos is in range,
19+
* and returns 0 (EOF) if byte_pos is out of range.
20+
*
21+
* Post-patch also returns 0 when the computed clamp would be
22+
* negative, rather than wrapping to SIZE_MAX.
23+
*
24+
* The test directly constructs a libretro_vfs_implementation_file
25+
* with a small cue_buf, sets byte_pos to a value >= cue_len, and
26+
* calls the read function. Under ASan the pre-patch behaviour
27+
* fires heap-buffer-overflow; post-patch returns 0 cleanly.
28+
*
29+
* Note: this is a Linux-only test because HAVE_CDROM is the only
30+
* configuration that compiles the cuesheet code path. The same
31+
* bug exists in the Windows branch of the same file but would
32+
* require Win32 headers to exercise directly.
33+
*/
34+
35+
#include <stdio.h>
36+
#include <stdlib.h>
37+
#include <string.h>
38+
#include <stdint.h>
39+
40+
#include <vfs/vfs.h>
41+
42+
/* Forward-declare the function we're testing. The real declaration
43+
* lives in vfs_implementation_cdrom.h which requires various
44+
* platform headers we don't want to drag in for a unit test. */
45+
int64_t retro_vfs_file_read_cdrom(libretro_vfs_implementation_file *stream,
46+
void *s, uint64_t len);
47+
48+
static int failures = 0;
49+
50+
static void test_cuesheet_byte_pos_wrap(void)
51+
{
52+
libretro_vfs_implementation_file stream;
53+
char out[64];
54+
int64_t rc;
55+
const char *fake_cue = "FILE \"track01.bin\" BINARY\nTRACK 01 MODE1/2048\n";
56+
size_t cue_size = strlen(fake_cue) + 1; /* incl NUL */
57+
58+
memset(&stream, 0, sizeof(stream));
59+
60+
stream.cdrom.cue_buf = strdup(fake_cue);
61+
if (!stream.cdrom.cue_buf)
62+
{
63+
printf("[ERROR] strdup failed\n");
64+
failures++;
65+
return;
66+
}
67+
stream.cdrom.cue_len = cue_size;
68+
69+
/* orig_path must end in ".cue" for the read function to dispatch
70+
* into the cuesheet path. The function calls path_get_extension
71+
* on it. */
72+
stream.orig_path = strdup("test.cue");
73+
if (!stream.orig_path)
74+
{
75+
free(stream.cdrom.cue_buf);
76+
printf("[ERROR] strdup failed (orig_path)\n");
77+
failures++;
78+
return;
79+
}
80+
81+
/* Baseline: read at byte_pos=0 with small len should succeed. */
82+
stream.cdrom.byte_pos = 0;
83+
rc = retro_vfs_file_read_cdrom(&stream, out, 10);
84+
if (rc < 0 || rc > 10)
85+
{
86+
printf("[ERROR] baseline read: rc=%lld, want 0..10\n",
87+
(long long)rc);
88+
failures++;
89+
}
90+
else
91+
printf("[SUCCESS] baseline cuesheet read rc=%lld\n", (long long)rc);
92+
93+
/* Attack 1: byte_pos at cue_len. Pre-patch:
94+
* cue_len - byte_pos - 1 = 0 - 1 = SIZE_MAX (size_t wrap)
95+
* len = SIZE_MAX, memcpy reads gigabytes -> OOB.
96+
* Post-patch: byte_pos >= cue_len, return 0. */
97+
stream.cdrom.byte_pos = (int64_t)cue_size;
98+
rc = retro_vfs_file_read_cdrom(&stream, out, 10);
99+
if (rc != 0)
100+
{
101+
printf("[ERROR] byte_pos==cue_len: rc=%lld, want 0\n",
102+
(long long)rc);
103+
failures++;
104+
}
105+
else
106+
printf("[SUCCESS] byte_pos==cue_len returned 0\n");
107+
108+
/* Attack 2: byte_pos past cue_len. Pre-patch: same size_t wrap. */
109+
stream.cdrom.byte_pos = (int64_t)cue_size + 100;
110+
rc = retro_vfs_file_read_cdrom(&stream, out, 10);
111+
if (rc != 0)
112+
{
113+
printf("[ERROR] byte_pos > cue_len: rc=%lld, want 0\n",
114+
(long long)rc);
115+
failures++;
116+
}
117+
else
118+
printf("[SUCCESS] byte_pos > cue_len returned 0\n");
119+
120+
/* Attack 3: negative byte_pos. Pre-patch: cast to size_t gives
121+
* SIZE_MAX-ish, then cue_len - huge wraps around into a small
122+
* positive value, then memcpy reads gigabytes off cue_buf.
123+
* Post-patch: byte_pos < 0, return 0. */
124+
stream.cdrom.byte_pos = -1;
125+
rc = retro_vfs_file_read_cdrom(&stream, out, 10);
126+
if (rc != 0)
127+
{
128+
printf("[ERROR] byte_pos < 0: rc=%lld, want 0\n", (long long)rc);
129+
failures++;
130+
}
131+
else
132+
printf("[SUCCESS] byte_pos < 0 returned 0\n");
133+
134+
/* Attack 4: huge len at legitimate byte_pos. Pre-patch: the
135+
* (int64_t)len cast makes large len negative and the bound
136+
* check behaves unpredictably. With len = UINT64_MAX the cast
137+
* is -1, and the check is "-1 >= cue_len - byte_pos" which for
138+
* small cue_len-byte_pos is false -- so the clamp is SKIPPED
139+
* and the full UINT64_MAX reaches memcpy. OOB.
140+
* Post-patch: clamp is unsigned, len is compared to unsigned
141+
* remaining, clamp always fires correctly. */
142+
stream.cdrom.byte_pos = 5;
143+
rc = retro_vfs_file_read_cdrom(&stream, out, (uint64_t)-1);
144+
{
145+
int64_t max_expected = (int64_t)cue_size - 5 - 1;
146+
if (rc < 0 || rc > max_expected)
147+
{
148+
printf("[ERROR] huge len: rc=%lld, want 0..%lld\n",
149+
(long long)rc, (long long)max_expected);
150+
failures++;
151+
}
152+
else
153+
printf("[SUCCESS] huge len clamped to rc=%lld\n", (long long)rc);
154+
}
155+
156+
free(stream.cdrom.cue_buf);
157+
free(stream.orig_path);
158+
}
159+
160+
int main(void)
161+
{
162+
test_cuesheet_byte_pos_wrap();
163+
164+
if (failures)
165+
{
166+
printf("\n%d cdrom test(s) failed\n", failures);
167+
return 1;
168+
}
169+
printf("\nAll cdrom regression tests passed.\n");
170+
return 0;
171+
}

‎libretro-common/vfs/vfs_implementation_cdrom.c‎

Lines changed: 57 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,13 @@ void retro_vfs_file_open_cdrom(
171171
if ( path[12] >= '0' && path[12] <= '9'
172172
&& path[13] >= '0' && path[13] <= '9')
173173
{
174-
stream->cdrom.cur_track = (path[12] - '0') * 10 + (path[13] - '0');
174+
unsigned parsed = (path[12] - '0') * 10 + (path[13] - '0');
175+
/* Reject track 00 -- track numbers are 1-based, and
176+
* cur_track == 0 would later index track[-1] in the
177+
* TOC array (OOB read). Leave the init value (1)
178+
* in place on reject. */
179+
if (parsed >= 1 && parsed <= 99)
180+
stream->cdrom.cur_track = parsed;
175181
#ifdef CDROM_DEBUG
176182
printf("[CDROM] Opening track %d\n", stream->cdrom.cur_track);
177183
fflush(stdout);
@@ -249,7 +255,13 @@ void retro_vfs_file_open_cdrom(
249255
if ( path[14] >= '0' && path[14] <= '9'
250256
&& path[15] >= '0' && path[15] <= '9')
251257
{
252-
stream->cdrom.cur_track = (path[14] - '0') * 10 + (path[15] - '0');
258+
unsigned parsed = (path[14] - '0') * 10 + (path[15] - '0');
259+
/* Reject track 00 -- track numbers are 1-based, and
260+
* cur_track == 0 would later index track[-1] in the
261+
* TOC array (OOB read). Leave the init value (1) in
262+
* place on reject. */
263+
if (parsed >= 1 && parsed <= 99)
264+
stream->cdrom.cur_track = parsed;
253265
#ifdef CDROM_DEBUG
254266
printf("[CDROM] Opening track %d\n", stream->cdrom.cur_track);
255267
fflush(stdout);
@@ -396,20 +408,37 @@ int64_t retro_vfs_file_read_cdrom(libretro_vfs_implementation_file *stream,
396408
&& (ext[2] == 'e' || ext[2] == 'E')
397409
&& ext[3] == '\0')
398410
{
399-
if ((int64_t)len >= (int64_t)stream->cdrom.cue_len
400-
- stream->cdrom.byte_pos)
401-
len = stream->cdrom.cue_len - stream->cdrom.byte_pos - 1;
411+
/* Guard against byte_pos being outside the cue buffer (can
412+
* happen after a seek -- seek does not clamp). Treat as
413+
* end-of-file. */
414+
if ( stream->cdrom.byte_pos < 0
415+
|| (size_t)stream->cdrom.byte_pos >= stream->cdrom.cue_len)
416+
return 0;
417+
/* Clamp len against the remaining bytes using unsigned
418+
* subtraction. Computing (byte_pos + len) and comparing
419+
* against cue_len wraps uint64_t when len is attacker-
420+
* controlled. The -1 preserves legacy behaviour of stopping
421+
* one byte before cue_len so consumers that rely on the cue
422+
* buffer being NUL-terminated don't re-read the terminator. */
423+
{
424+
size_t remaining = stream->cdrom.cue_len
425+
- (size_t)stream->cdrom.byte_pos;
426+
if (remaining > 0)
427+
remaining -= 1;
428+
if (len > (uint64_t)remaining)
429+
len = (uint64_t)remaining;
430+
}
402431
#ifdef CDROM_DEBUG
403432
printf(
404433
"[CDROM] Read: Reading %" PRIu64 " bytes from cuesheet starting at %" PRIu64 "...\n",
405434
len,
406435
stream->cdrom.byte_pos);
407436
fflush(stdout);
408437
#endif
409-
memcpy(s, stream->cdrom.cue_buf + stream->cdrom.byte_pos, len);
438+
memcpy(s, stream->cdrom.cue_buf + stream->cdrom.byte_pos, (size_t)len);
410439
stream->cdrom.byte_pos += len;
411440

412-
return len;
441+
return (int64_t)len;
413442
}
414443
else if ( (ext[0] == 'b' || ext[0] == 'B')
415444
&& (ext[1] == 'i' || ext[1] == 'I')
@@ -424,14 +453,28 @@ int64_t retro_vfs_file_read_cdrom(libretro_vfs_implementation_file *stream,
424453
unsigned char rframe = 0;
425454
size_t skip = stream->cdrom.byte_pos % 2352;
426455

427-
if (stream->cdrom.byte_pos >=
428-
vfs_cdrom_toc.track[stream->cdrom.cur_track - 1].track_bytes)
456+
/* Guard against cur_track == 0 (would index track[-1]) and
457+
* against an out-of-range byte_pos. cur_track can become
458+
* 0 via a crafted VFS path like "driveN-track00.bin" unless
459+
* the parser rejects it (see retro_vfs_file_open_cdrom). */
460+
if (stream->cdrom.cur_track == 0
461+
|| stream->cdrom.cur_track > 99)
429462
return 0;
430-
431-
if (stream->cdrom.byte_pos + len >
432-
vfs_cdrom_toc.track[stream->cdrom.cur_track - 1].track_bytes)
433-
len -= (stream->cdrom.byte_pos + len)
434-
- vfs_cdrom_toc.track[stream->cdrom.cur_track - 1].track_bytes;
463+
{
464+
size_t track_bytes =
465+
vfs_cdrom_toc.track[stream->cdrom.cur_track - 1].track_bytes;
466+
if ( stream->cdrom.byte_pos < 0
467+
|| (size_t)stream->cdrom.byte_pos >= track_bytes)
468+
return 0;
469+
/* Clamp len against remaining bytes using unsigned
470+
* subtraction. Computing (byte_pos + len) first is a
471+
* uint64_t wrap hazard when len is attacker-controlled. */
472+
{
473+
size_t remaining = track_bytes - (size_t)stream->cdrom.byte_pos;
474+
if (len > (uint64_t)remaining)
475+
len = (uint64_t)remaining;
476+
}
477+
}
435478

436479
cdrom_lba_to_msf(stream->cdrom.cur_lba, &min, &sec, &frame);
437480
cdrom_lba_to_msf(stream->cdrom.cur_lba

0 commit comments

Comments
 (0)