Skip to content

Commit f3bf03e

Browse files
committed
libretro-common/formats/json/rjson: integer overflow hardening + test
Audit of rjson.c (1569 lines; the JSON parser + writer used by configs, manifests, RetroAchievements, cloud sync, etc.). Six integer-overflow/underflow hardenings in the parser and writer, plus a regression test. None are remotely exploitable in the typical usage pattern -- they require inputs or outputs approaching or exceeding 2 GiB -- but each is a real correctness bug that can bite on 32-bit targets or on pathological locally-generated JSON. rjson: cap _rjson_grow_string doubling The growing string buffer doubled its capacity unconditionally: size_t new_string_cap = json->string_cap * 2; Once string_cap exceeds SIZE_MAX / 2, the doubling wraps to 0 or a tiny value. realloc() with that argument is implementation- defined: glibc returns NULL (benign, caller errors out); some other libcs return a valid 0-byte pointer. The following pushchar then writes at string[0], which is past the newly shrunk allocation. Heap overflow. Reachable on 32-bit with a ~2 GiB JSON string token (and correspondingly much harder on 64-bit). Fix: cap new_string_cap at _RJSON_MAX_SIZE (256 MiB) and fail cleanly when the growth would exceed it. rjson: clamp rjson_open_user io_block_size The public entry point accepted any int as the input block size and allocated sizeof(rjson_t) - sizeof(input_buf) + io_block_size. A small io_block_size (including 0 or negative) underallocated input_buf to fewer bytes than the fixed struct expected, and the first read in _rjson_io_input ran off the end. The two internal callers (rjson_open_stream, rjson_open_rfile) already bound io_block_size to [1024, 4096], so this isn't reachable via them. It is reachable from any consumer that calls rjson_open_user directly with attacker-influenced sizing, so clamp it here for defense-in-depth: io_block_size floored at 16, ceilinged at _RJSON_MAX_SIZE. rjsonwriter: size_t arithmetic in _rjsonwriter_memory_io new_cap = writer->buf_num + (is_append ? len : 0) + 512; All operands are signed int. For a writer generating >2 GiB of output (local database dumps, long JSON-encoded state blobs) the sum overflows INT_MAX (UB). Post-overflow the comparison "new_cap > buf_cap" misbehaves, the realloc is skipped, and the subsequent memcpy in the same function writes past the existing allocation. Fix: redo the math in size_t, overflow-check against _RJSON_MAX_SIZE, set error_text and return 0 on failure. The realloc call then receives a sane positive int. rjsonwriter: size_t arithmetic in rjsonwriter_raw Same class of bug: if (writer->buf_num + len > writer->buf_cap) rjsonwriter_flush(writer); Signed-int sum overflows for large writes and the flush is skipped, letting the downstream memcpy blow past the buffer. Also: nothing guarded against a negative len. A caller passing len < 0 pre-patch advanced buf_num by a negative amount via a later "buf_num += len" and the next write corrupted the ring. Fix: reject negative len at the top, redo the bounds comparison in size_t. rjsonwriter: size_t arithmetic in rjsonwriter_rawf Same class. newcap = buf_num + need + 1 could overflow for a single oversized formatted value. Same fix pattern. Regression test: libretro-common/samples/formats/json/rjson_test.c Six subtests, three of which are true discriminators: 1. parser happy path (nested object + array) 2. long-string parse (forces _rjson_grow_string growth past the inline buffer) -- smoke for patch #1 3. malformed inputs: 12 variants (truncated objects, dangling surrogates, bad numbers, ...) must NOT crash. Smoke for general robustness. 4. writer happy path 5. rjsonwriter_raw with len = -99 -- DIRECT DISCRIMINATOR for the negative-len fix. Pre-patch under ASan this fires: AddressSanitizer: negative-size-param: (size=-99) in memcpy at rjson.c:1392 (rjsonwriter_raw) Post-patch the call is a no-op and the output is exactly the expected 6-byte "AAABBB". 6. rjson_open_user with tiny io_block_size (-10..8) -- DIRECT DISCRIMINATOR for the floor fix. Pre-patch io_block_size=0 allocated zero bytes for input_buf and the first read was OOB (UB at C level, crashes on some libcs). Post-patch all values floor to 16 and the open+read+free cycle is clean. Built and ran under -Wall -pedantic -std=gnu99: All 6 subtests pass on patched code. ASan -O0 (patched): clean, exit 0. ASan -O0 (unpatched, for the negative-len subtest): "negative-size-param: (size=-99)" in memcpy -> immediate abort. CI: libretro-common samples workflow updated. rjson_test added to RUN_TARGETS. Full local dry-run under the GHA shell contract: Built: 15 Ran: 15 Failed: 0 VC6 compatibility: rjson.c is compiled on all platforms. The fix uses only size_t / ssize_t / int and the existing stdint shim; <limits.h> (C89) is now explicitly included for INT_MAX. _RJSON_MAX_SIZE is a size_t-cast literal, valid in C89.
1 parent eb86df0 commit f3bf03e

4 files changed

Lines changed: 401 additions & 5 deletions

File tree

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ jobs:
5555
vfs_read_overflow_test
5656
cdrom_cuesheet_overflow_test
5757
http_parse_test
58+
rjson_test
5859
)
5960
6061
# Per-binary run command (overrides ./<binary> if present).

libretro-common/formats/json/rjson.c

Lines changed: 72 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,19 @@
2525
#include <stdio.h> /* snprintf, vsnprintf */
2626
#include <stdarg.h> /* va_list */
2727
#include <string.h> /* memcpy */
28-
#include <stdint.h> /* int64_t */
28+
#include <stdint.h> /* int64_t, SIZE_MAX */
2929
#include <stdlib.h> /* malloc, realloc, atof, atoi */
30+
#include <limits.h> /* INT_MAX */
31+
32+
/* Ceiling for the parser's growing string buffer and the writer's
33+
* in-memory buffer. Reached in practice only by pathologically large
34+
* (and almost certainly malformed) JSON inputs on a 32-bit system,
35+
* but having an explicit cap avoids undefined behaviour in the
36+
* signed-int arithmetic of the writer and the size_t doubling in the
37+
* parser wrapping past SIZE_MAX / 2. Callers with legitimate giant
38+
* payloads should not be using these APIs in the first place --
39+
* stream in chunks. */
40+
#define _RJSON_MAX_SIZE ((size_t)256 * 1024 * 1024)
3041

3142
#include <formats/rjson.h>
3243
#include <compat/posix_string.h>
@@ -139,7 +150,23 @@ static bool _rjson_io_input(rjson_t *json)
139150
static bool _rjson_grow_string(rjson_t *json)
140151
{
141152
char *string;
142-
size_t new_string_cap = json->string_cap * 2;
153+
size_t new_string_cap;
154+
/* Bound the doubling so string_cap * 2 can never wrap past
155+
* SIZE_MAX. Pre-patch a pathological input on 32-bit (string_cap
156+
* nearing 2 GiB) doubled to 0 and realloc() returned either NULL
157+
* (benign) or a 0-byte pointer (heap overflow on next pushchar).
158+
* Post-patch we fail cleanly before the arithmetic wrap. */
159+
if (json->string_cap > _RJSON_MAX_SIZE / 2)
160+
{
161+
if (json->string_cap >= _RJSON_MAX_SIZE)
162+
{
163+
_rjson_error(json, "string token too large");
164+
return false;
165+
}
166+
new_string_cap = _RJSON_MAX_SIZE;
167+
}
168+
else
169+
new_string_cap = json->string_cap * 2;
143170
if (json->string != json->inline_string)
144171
string = (char*)realloc(json->string, new_string_cap);
145172
else if ((string = (char*)malloc(new_string_cap)) != NULL)
@@ -915,7 +942,16 @@ void _rjson_setup(rjson_t *json, rjson_io_t io, void *user_data, int input_len)
915942

916943
rjson_t *rjson_open_user(rjson_io_t io, void *user_data, int io_block_size)
917944
{
918-
rjson_t* json = (rjson_t*)malloc(
945+
rjson_t* json;
946+
/* Clamp io_block_size against negative / tiny / oversized values.
947+
* The two internal callers (rjson_open_stream / rjson_open_rfile)
948+
* already bound this, but this function is public and can be
949+
* reached directly. */
950+
if (io_block_size < 16)
951+
io_block_size = 16;
952+
else if ((size_t)io_block_size > _RJSON_MAX_SIZE)
953+
io_block_size = (int)_RJSON_MAX_SIZE;
954+
json = (rjson_t*)malloc(
919955
sizeof(rjson_t) - sizeof(((rjson_t*)0)->input_buf) + io_block_size);
920956
if (json) _rjson_setup(json, io, user_data, io_block_size);
921957
return json;
@@ -1290,7 +1326,24 @@ static int _rjsonwriter_memory_io(const void* buf, int len, void *user)
12901326
{
12911327
rjsonwriter_t *writer = (rjsonwriter_t *)user;
12921328
bool is_append = (buf != writer->buf);
1293-
int new_cap = writer->buf_num + (is_append ? len : 0) + 512;
1329+
size_t append_len = (is_append ? (size_t)len : 0);
1330+
size_t target;
1331+
int new_cap;
1332+
/* Detect the int-overflow pre-patch, where buf_num + len + 512
1333+
* wrapped past INT_MAX and new_cap went negative. The subsequent
1334+
* "new_cap > buf_cap" comparison then misbehaved and memcpy wrote
1335+
* past the existing allocation on a post-overflow buffer that
1336+
* hadn't been grown. Do the arithmetic in size_t and cap at
1337+
* _RJSON_MAX_SIZE (which also fits comfortably in int). */
1338+
if (len < 0 || (size_t)writer->buf_num > _RJSON_MAX_SIZE - append_len
1339+
- 512)
1340+
{
1341+
if (!writer->error_text)
1342+
writer->error_text = "output buffer too large";
1343+
return 0;
1344+
}
1345+
target = (size_t)writer->buf_num + append_len + 512;
1346+
new_cap = (int)target;
12941347
if (!writer->final_flush && (is_append || new_cap > writer->buf_cap))
12951348
{
12961349
bool can_realloc = (writer->buf != writer->inline_buf);
@@ -1376,7 +1429,13 @@ const char *rjsonwriter_get_error(rjsonwriter_t *writer)
13761429

13771430
void rjsonwriter_raw(rjsonwriter_t *writer, const char *buf, int len)
13781431
{
1379-
if (writer->buf_num + len > writer->buf_cap)
1432+
/* Guard against negative len and against buf_num+len signed
1433+
* overflow. A caller that somehow passes a huge len would
1434+
* pre-patch compute a negative sum, skip the flush, then memcpy
1435+
* past the existing buffer below. */
1436+
if (len < 0)
1437+
return;
1438+
if ((size_t)writer->buf_num + (size_t)len > (size_t)writer->buf_cap)
13801439
rjsonwriter_flush(writer);
13811440
if (len == 1)
13821441
{
@@ -1424,6 +1483,14 @@ void rjsonwriter_rawf(rjsonwriter_t *writer, const char *fmt, ...)
14241483
return;
14251484
}
14261485
rjsonwriter_flush(writer);
1486+
/* Guard signed-int overflow on newcap: buf_num + need + 1 could
1487+
* wrap past INT_MAX for a single very large formatted value. */
1488+
if ((size_t)writer->buf_num + (size_t)need + 1 > _RJSON_MAX_SIZE)
1489+
{
1490+
if (!writer->error_text)
1491+
writer->error_text = "output buffer too large";
1492+
return;
1493+
}
14271494
if (writer->buf_num + need >= writer->buf_cap)
14281495
{
14291496
int newcap = writer->buf_num + need + 1;
Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
TARGET := rjson_test
2+
3+
LIBRETRO_COMM_DIR := ../../..
4+
5+
SOURCES := \
6+
rjson_test.c \
7+
$(LIBRETRO_COMM_DIR)/formats/json/rjson.c \
8+
$(LIBRETRO_COMM_DIR)/compat/fopen_utf8.c \
9+
$(LIBRETRO_COMM_DIR)/compat/compat_strl.c \
10+
$(LIBRETRO_COMM_DIR)/compat/compat_posix_string.c \
11+
$(LIBRETRO_COMM_DIR)/encodings/encoding_utf.c \
12+
$(LIBRETRO_COMM_DIR)/encodings/encoding_crc32.c \
13+
$(LIBRETRO_COMM_DIR)/file/file_path.c \
14+
$(LIBRETRO_COMM_DIR)/file/file_path_io.c \
15+
$(LIBRETRO_COMM_DIR)/streams/file_stream.c \
16+
$(LIBRETRO_COMM_DIR)/streams/interface_stream.c \
17+
$(LIBRETRO_COMM_DIR)/streams/memory_stream.c \
18+
$(LIBRETRO_COMM_DIR)/streams/trans_stream.c \
19+
$(LIBRETRO_COMM_DIR)/streams/trans_stream_pipe.c \
20+
$(LIBRETRO_COMM_DIR)/streams/trans_stream_zlib.c \
21+
$(LIBRETRO_COMM_DIR)/time/rtime.c \
22+
$(LIBRETRO_COMM_DIR)/vfs/vfs_implementation.c
23+
24+
OBJS := $(SOURCES:.c=.o)
25+
26+
CFLAGS += -Wall -pedantic -std=gnu99 -g -I$(LIBRETRO_COMM_DIR)/include
27+
LDLIBS += -lz
28+
29+
all: $(TARGET)
30+
31+
%.o: %.c
32+
$(CC) -c -o $@ $< $(CFLAGS)
33+
34+
$(TARGET): $(OBJS)
35+
$(CC) -o $@ $^ $(LDFLAGS) $(LDLIBS)
36+
37+
clean:
38+
rm -f $(TARGET) $(OBJS)
39+
40+
.PHONY: clean

0 commit comments

Comments
 (0)