Skip to content

Commit 45699e2

Browse files
LibretroAdminLibretroAdmin
committed
test/libretro-db: regression test for rmsgpack length-field guards + CI
Adds libretro-db/samples/rmsgpack/rmsgpack_overflow_test, a self-contained regression test for the three preceding commits (b701dbb, ee37153, a22e6fc). Seven cases: - STR32 with len 0xFFFFFFFE on a 5-byte stream (rejected) - MAP32 with len 0x10000000 on a 5-byte stream (rejected) - ARRAY32 with len 0xFFFFFFFF on a 5-byte stream (rejected) - Truncated ARRAY16 claiming 10 entries with 5 available (rejected) - Valid fixmap of 2 entries with valid contents (accepted) - Valid ARRAY16 of 4 fixints (accepted) - Valid maximal STR8 (len 0xFF) with that many bytes (accepted) Verified to discriminate pre/post-patch: against b701dbb~1's rmsgpack.c the STR32 case segfaults under tight memory limits (ulimit -v 524288: malloc(0xFFFFFFFF) returns NULL, the unchecked *pbuff = ... is dereferenced); against the patched code all seven cases pass. The test follows the existing samples/ pattern (plain C, asserts manually, exit nonzero on failure) rather than libcheck because that's what the existing CI harness in .github/workflows/Linux-libretro-common-samples.yml understands. Uses intfstream_open_memory so the test needs no filesystem fixtures and the size-bound path fires (memory streams report a known size). Adds a sibling workflow Linux-libretro-db-samples.yml that walks libretro-db/samples/, builds every Makefile-rooted directory, and runs targets named in its RUN_TARGETS allowlist. The workflow is a near-verbatim copy of the libretro-common-samples one with the working-directory changed and an empty initial RUN_TARGETS allow- list except for rmsgpack_overflow_test; new tests added under libretro-db/samples/ in the future just need their Makefile target appended to that allowlist. Tested locally: the workflow's bash logic builds the test, runs it, and the test reports all 7 SUCCESS lines and exits 0. Build takes ~6 seconds on Ubuntu 24 with system gcc and zlib1g-dev.
1 parent 0555b23 commit 45699e2

5 files changed

Lines changed: 507 additions & 2 deletions

File tree

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

Lines changed: 151 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,151 @@
1+
name: CI Linux libretro-db samples
2+
3+
on:
4+
push:
5+
branches:
6+
- master
7+
pull_request:
8+
branches:
9+
- master
10+
workflow_dispatch:
11+
12+
permissions:
13+
contents: read
14+
15+
env:
16+
ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSION: true
17+
18+
jobs:
19+
samples:
20+
name: Build and run libretro-db/samples
21+
runs-on: ubuntu-latest
22+
timeout-minutes: 15
23+
24+
steps:
25+
- name: Install dependencies
26+
run: |
27+
sudo apt-get update -y
28+
sudo apt-get install -y build-essential zlib1g-dev
29+
30+
- name: Checkout
31+
uses: actions/checkout@v3
32+
33+
- name: Build and run samples
34+
shell: bash
35+
working-directory: libretro-db/samples
36+
run: |
37+
set -u
38+
set -o pipefail
39+
40+
# Samples whose binary, when invoked with no arguments, runs a
41+
# self-contained test and exits 0 on success / non-zero on
42+
# failure. These are built AND executed.
43+
declare -a RUN_TARGETS=(
44+
rmsgpack_overflow_test
45+
)
46+
47+
# Per-binary run command (overrides ./<binary> if present).
48+
declare -A RUN_ENV=()
49+
50+
# Samples that are build-only.
51+
declare -a BUILD_ONLY_DIRS=(
52+
)
53+
54+
# Samples that are currently broken at build time on a stock
55+
# Ubuntu host and are therefore neither built nor run.
56+
declare -a SKIP_DIRS=(
57+
)
58+
59+
is_in() {
60+
local needle=$1; shift
61+
local h
62+
for h in "$@"; do [[ "$h" == "$needle" ]] && return 0; done
63+
return 1
64+
}
65+
66+
fails=0
67+
builds=0
68+
runs=0
69+
70+
# Collect all Makefile directories (one or two levels deep).
71+
mapfile -t MKDIRS < <(find . -name Makefile -printf '%h\n' | sort)
72+
73+
printf '\n==> %d sample directories found\n' "${#MKDIRS[@]}"
74+
for d in "${MKDIRS[@]}"; do printf ' %s\n' "${d#./}"; done
75+
printf '\n'
76+
77+
for d in "${MKDIRS[@]}"; do
78+
rel=${d#./}
79+
printf '========================================\n'
80+
printf '[%s] %s\n' "$(is_in "$rel" "${SKIP_DIRS[@]}" && echo skip || echo build)" "$rel"
81+
printf '========================================\n'
82+
83+
if is_in "$rel" "${SKIP_DIRS[@]}"; then
84+
printf '[skip] %s is on the skip list\n\n' "$rel"
85+
continue
86+
fi
87+
88+
# Build
89+
if ! ( cd "$d" && make clean all ); then
90+
printf '\n::error title=Build failed::%s failed to build\n' "$rel"
91+
fails=$((fails+1))
92+
continue
93+
fi
94+
builds=$((builds+1))
95+
96+
# Skip run for build-only dirs
97+
if is_in "$rel" "${BUILD_ONLY_DIRS[@]}"; then
98+
printf '[skip-run] %s (build-only list)\n\n' "$rel"
99+
continue
100+
fi
101+
102+
# Extract targets from Makefile. Handles:
103+
# TARGET := foo
104+
# TARGETS = a b c
105+
# TARGET_TEST := foo_test
106+
# TARGET_TEST2 := foo_test2
107+
mapfile -t targets < <(
108+
grep -hE '^(TARGET|TARGETS|TARGET_TEST[0-9]*)[[:space:]]*[:?]?=' "$d/Makefile" \
109+
| sed -E 's/^[^=]*=[[:space:]]*//' \
110+
| tr -s ' \t' '\n' \
111+
| grep -v '^$' \
112+
| sort -u
113+
)
114+
115+
for t in "${targets[@]}"; do
116+
if ! is_in "$t" "${RUN_TARGETS[@]}"; then
117+
printf '[skip-run] %s/%s (not in run allowlist)\n' "$rel" "$t"
118+
continue
119+
fi
120+
121+
bin="$d/$t"
122+
if [[ ! -x "$bin" ]]; then
123+
printf '::error title=Missing binary::%s was in the run allowlist but %s does not exist after build\n' "$t" "$bin"
124+
fails=$((fails+1))
125+
continue
126+
fi
127+
128+
extra_env=${RUN_ENV[$t]:-}
129+
130+
printf '\n[run] %s\n' "$bin"
131+
if ( cd "$d" && env $extra_env timeout 60 "./$t" ); then
132+
printf '[pass] %s\n\n' "$t"
133+
runs=$((runs+1))
134+
else
135+
rc=$?
136+
printf '\n::error title=Test failed::%s exited with status %d\n' "$t" "$rc"
137+
fails=$((fails+1))
138+
fi
139+
done
140+
done
141+
142+
printf '========================================\n'
143+
printf 'Summary\n'
144+
printf '========================================\n'
145+
printf ' Built: %d\n' "$builds"
146+
printf ' Ran: %d\n' "$runs"
147+
printf ' Failed: %d\n' "$fails"
148+
149+
if [[ $fails -gt 0 ]]; then
150+
exit 1
151+
fi

‎libretro-db/libretrodb.c‎

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -196,6 +196,7 @@ int libretrodb_open(const char *path, libretrodb_t *db, bool write)
196196
{
197197
libretrodb_header_t header;
198198
libretrodb_metadata_t md;
199+
int64_t file_size;
199200
unsigned mode = write ? RETRO_VFS_FILE_ACCESS_READ_WRITE | RETRO_VFS_FILE_ACCESS_UPDATE_EXISTING : RETRO_VFS_FILE_ACCESS_READ;
200201
intfstream_t *fd = intfstream_open_file(path, mode, RETRO_VFS_FILE_ACCESS_HINT_NONE);
201202
db->can_write = write;
@@ -215,8 +216,26 @@ int libretrodb_open(const char *path, libretrodb_t *db, bool write)
215216
goto error;
216217

217218
header.metadata_offset = swap_if_little64(header.metadata_offset);
218-
intfstream_seek(fd, (ssize_t)header.metadata_offset,
219-
RETRO_VFS_SEEK_POSITION_START);
219+
220+
/* Pre-patch the metadata_offset field was an attacker-
221+
* controlled uint64 from the .rdb header, cast to ssize_t and
222+
* fed straight to intfstream_seek without any bounds check.
223+
* On 32-bit that cast truncates; on 64-bit a value past EOF
224+
* left the stream in a state where the subsequent
225+
* rmsgpack_dom_read_into either failed cleanly or, depending
226+
* on the VFS implementation's seek-past-EOF semantics, read
227+
* stale buffered bytes.
228+
*
229+
* Reject metadata_offset that doesn't fit in the actual file
230+
* (must leave room for at least the 1-byte fixmap header). */
231+
file_size = intfstream_get_size(fd);
232+
if (file_size < 0)
233+
goto error;
234+
if (header.metadata_offset >= (uint64_t)file_size)
235+
goto error;
236+
if (intfstream_seek(fd, (ssize_t)header.metadata_offset,
237+
RETRO_VFS_SEEK_POSITION_START) < 0)
238+
goto error;
220239

221240
if (rmsgpack_dom_read_into(fd, "count", &md.count, NULL) < 0)
222241
goto error;

‎libretro-db/rmsgpack.c‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -387,11 +387,44 @@ static int rmsgpack_read_buff(intfstream_t *fd, size_t size, char **pbuff, uint6
387387
{
388388
ssize_t read_len;
389389
uint64_t tmp_len = 0;
390+
int64_t remaining;
391+
int64_t here;
392+
int64_t total;
390393

391394
if (rmsgpack_read_uint(fd, &tmp_len, size) == -1)
392395
return -1;
393396

397+
/* Pre-patch tmp_len was an attacker-controlled uint64 fed
398+
* directly into malloc((size_t)(tmp_len + 1)). Three
399+
* problems:
400+
* - tmp_len = UINT64_MAX wraps to malloc(0) returning a
401+
* small block; the subsequent intfstream_read with
402+
* (size_t)UINT64_MAX bytes would heap-overflow on any
403+
* stream that could deliver them.
404+
* - tmp_len = 0xFFFFFFFE forces a ~4 GiB malloc on 64-bit
405+
* (which Linux overcommit happily grants) for a 5-byte
406+
* STR32 input, OOM-killing the process. On 32-bit the
407+
* (size_t) cast truncates and creates a heap overflow.
408+
* - Even when malloc returns NULL the code dereferenced
409+
* *pbuff at line 396 and (*pbuff)[read_len] at line 404.
410+
*
411+
* Fix: bound tmp_len against the remaining bytes in the
412+
* stream (a buffer can't legitimately claim more bytes than
413+
* are left to read), reject the SIZE_MAX edge to avoid the
414+
* +1 wrap, and NULL-check the malloc. */
415+
here = intfstream_tell(fd);
416+
total = intfstream_get_size(fd);
417+
if (here < 0 || total < 0 || here > total)
418+
return -1;
419+
remaining = total - here;
420+
if (tmp_len > (uint64_t)remaining)
421+
return -1;
422+
if (tmp_len >= (uint64_t)((size_t)-1))
423+
return -1;
424+
394425
*pbuff = (char *)malloc((size_t)(tmp_len + 1) * sizeof(char));
426+
if (!*pbuff)
427+
return -1;
395428

396429
if ((read_len = intfstream_read(fd, *pbuff, (size_t)tmp_len)) == -1)
397430
{
@@ -412,6 +445,29 @@ static int rmsgpack_read_map(intfstream_t *fd, uint32_t len,
412445
{
413446
int rv;
414447
unsigned i;
448+
int64_t here, total;
449+
450+
/* Pre-patch len was an attacker-controlled uint32 from the
451+
* file (MAP16 or MAP32 header) handed straight to the callback
452+
* which calloc'd 'len * sizeof(rmsgpack_dom_pair)' (~80 bytes
453+
* per pair). A 5-byte MAP32 header '0xdf 0x10 0x00 0x00 0x00'
454+
* (len = 2^28) demanded a ~21 GiB calloc, OOM-killing the
455+
* process. Even where calloc succeeded, the subsequent
456+
* iteration recursed into rmsgpack_read len times, doubling
457+
* the dom reader stack on each recursion until it ran out.
458+
*
459+
* A map cannot legitimately claim more entries than the
460+
* stream has bytes left to encode them: the smallest possible
461+
* key+value pair is 2 bytes (one type byte each). Reject
462+
* len > remaining_bytes / 2. */
463+
here = intfstream_tell(fd);
464+
total = intfstream_get_size(fd);
465+
if (here >= 0 && total >= 0 && here <= total)
466+
{
467+
uint64_t remaining = (uint64_t)(total - here);
468+
if ((uint64_t)len > remaining / 2u)
469+
return -1;
470+
}
415471

416472
if ( ( callbacks->read_map_start)
417473
&& (rv = callbacks->read_map_start(len, data)) < 0)
@@ -433,6 +489,20 @@ static int rmsgpack_read_array(intfstream_t *fd, uint32_t len,
433489
{
434490
int rv;
435491
unsigned i;
492+
int64_t here, total;
493+
494+
/* Same primitive as rmsgpack_read_map above. Smallest
495+
* possible array element is 1 byte (a fixint), so len cannot
496+
* legitimately exceed the number of bytes left in the
497+
* stream. */
498+
here = intfstream_tell(fd);
499+
total = intfstream_get_size(fd);
500+
if (here >= 0 && total >= 0 && here <= total)
501+
{
502+
uint64_t remaining = (uint64_t)(total - here);
503+
if ((uint64_t)len > remaining)
504+
return -1;
505+
}
436506

437507
if ( ( callbacks->read_array_start)
438508
&& (rv = callbacks->read_array_start(len, data)) < 0)

‎libretro-db/samples/rmsgpack/Makefile‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
TARGET := rmsgpack_overflow_test
2+
3+
LIBRETRODB_DIR := ../..
4+
LIBRETRO_COMM_DIR := ../../../libretro-common
5+
6+
SOURCES := \
7+
rmsgpack_overflow_test.c \
8+
$(LIBRETRODB_DIR)/rmsgpack.c \
9+
$(LIBRETRODB_DIR)/rmsgpack_dom.c \
10+
$(LIBRETRO_COMM_DIR)/streams/interface_stream.c \
11+
$(LIBRETRO_COMM_DIR)/streams/file_stream.c \
12+
$(LIBRETRO_COMM_DIR)/streams/memory_stream.c \
13+
$(LIBRETRO_COMM_DIR)/compat/compat_strl.c \
14+
$(LIBRETRO_COMM_DIR)/compat/fopen_utf8.c \
15+
$(LIBRETRO_COMM_DIR)/encodings/encoding_utf.c \
16+
$(LIBRETRO_COMM_DIR)/encodings/encoding_crc32.c \
17+
$(LIBRETRO_COMM_DIR)/file/file_path.c \
18+
$(LIBRETRO_COMM_DIR)/file/file_path_io.c \
19+
$(LIBRETRO_COMM_DIR)/string/stdstring.c \
20+
$(LIBRETRO_COMM_DIR)/time/rtime.c \
21+
$(LIBRETRO_COMM_DIR)/vfs/vfs_implementation.c
22+
23+
OBJS := $(SOURCES:.c=.o)
24+
25+
CFLAGS += -Wall -pedantic -std=gnu99 -g \
26+
-I$(LIBRETRO_COMM_DIR)/include \
27+
-I$(LIBRETRODB_DIR)
28+
LDFLAGS +=
29+
30+
all: $(TARGET)
31+
32+
%.o: %.c
33+
$(CC) -c -o $@ $< $(CFLAGS)
34+
35+
$(TARGET): $(OBJS)
36+
$(CC) -o $@ $^ $(LDFLAGS)
37+
38+
clean:
39+
rm -f $(TARGET) $(OBJS)
40+
41+
.PHONY: clean

0 commit comments

Comments
 (0)