[pull] master from libretro:master#973
Merged
pull[bot] merged 4 commits intoAlexandre1er:masterfrom Apr 28, 2026
Merged
Conversation
…t attacker-controlled inputs
bps_apply_patch (tasks/task_patch.c) implemented BPS patch
application without bounds checks on any of the per-command
reads or writes. The action loop:
while (bps.modify_offset < bps.modify_length - 12)
{
size_t _len = bps_decode(&bps); /* attacker-chosen */
unsigned mode = _len & 3;
_len = (_len >> 2) + 1;
switch (mode)
{
case TARGET_READ:
while (_len--) {
uint8_t data = bps_read(&bps);
bps.target_data[bps.output_offset++] = data; /* OOB write */
}
case SOURCE_READ:
while (_len--) {
uint8_t data = bps.source_data[bps.output_offset]; /* OOB read */
bps.target_data[bps.output_offset++] = data;
}
case SOURCE_COPY:
bps.source_offset += offset; /* unbounded signed offset */
while (_len--) {
uint8_t data = bps.source_data[bps.source_offset++]; /* OOB read */
bps.target_data[bps.output_offset++] = data;
}
case TARGET_COPY:
bps.target_offset += offset;
while (_len--) {
uint8_t data = bps.target_data[bps.target_offset++];
bps.target_data[bps.output_offset++] = data;
}
}
}
A malicious .bps could:
- Write attacker-chosen bytes far past the malloc'd
target_data buffer (heap-buffer-overflow WRITE) via any
of the four modes.
- Read past source_data via SOURCE_COPY's unbounded signed
offset. Negative offsets underflow source_offset to a
huge size_t; positive offsets push it past
source_length. The leaked bytes flow into target_data
and the patch checksum, exposing heap contents.
- Read past modify_data via TARGET_READ when _len exceeds
the remaining patch bytes. The action-loop guard
confirms only "at least one command's worth of bytes",
not the full read range.
The size prelude (modify_source_size / modify_target_size /
modify_markup_size, decoded by bps_decode immediately after
the "BPS1" header) was also unchecked:
- modify_target_size declared as size_t silently truncated
the uint64 bps_decode result on 32-bit hosts. A value of
0x100000000 wraps to 0; the subsequent malloc(0) returns
a tiny allocation while bps.target_length is set to the
raw uint64. The next target_data[output_offset++] write
OOB by the truncated delta.
- modify_markup_size was used as a loop bound consuming
bytes from modify_data via bps_read. A value above the
remaining patch bytes ran the read loop off the end of
modify_data.
Reachability: user has soft-patching enabled (the default
when a .bps file with the same basename as the loaded ROM is
present) and loads a ROM whose .bps was supplied by an
attacker. Same threat class as CDFS / CHD / BSV file-load
bugs.
Fix:
1. Capture the three size-prelude values as raw uint64,
reject anything above SIZE_MAX (so the size_t cast is
guaranteed lossless), and reject markup_size that would
drive bps_read past modify_length.
2. At the top of every action-loop iteration, bound _len
against (target_length - output_offset). This single
bound covers the target_data write in all four modes.
3. In each mode, bound the source-side read range:
- SOURCE_READ: output_offset + _len <= source_length
- TARGET_READ: modify_offset + _len <= modify_length - 12
- SOURCE_COPY: source_offset (post-offset-add) + _len
<= source_length
- TARGET_COPY: target_offset (post-offset-add) + _len
<= target_length
Adds samples/tasks/bps_patch/bps_patch_bounds_test as a
regression test, registered in
.github/workflows/Linux-samples-tasks.yml as an ASan-enabled
step. Five cases covering OOB-write rejection, OOB-read
rejection, legitimate operations, and the exact-capacity
boundary.
Test follows the existing samples/tasks pattern (verbatim
copy of the post-fix predicates with maintenance-contract
comment, ASan as the discriminator). Verified to fire
under ASan when the bounds are removed: heap-buffer-overflow
WRITE of size 1 at offset 4 of a 4-byte allocation in
apply_action_loop. With the bounds the test passes clean.
…se_info_build_query_enum Add strlcpy_append, a bound-checked replacement for the unsafe idiom that's widespread in this codebase: _len += strlcpy(s + _len, src, len - _len); That looks like a self-bounding chain but isn't. strlcpy returns strlen(source) regardless of how much was actually written, so on truncation _len overshoots len and the next (len - _len) underflows to a huge size_t, driving the next strlcpy into an OOB-write or memcpy of attacker-influenced size. This is the same primitive that drove the shader- wildcard fix (commit bac6894). strlcpy_append takes (s, len, *pos, src), checks the truncation condition correctly, and on overflow: - leaves s NUL-terminated at len - 1 (strlcpy's contract) - clamps *pos to len - 1 so subsequent calls in a chain short-circuit (also returning -1) - returns -1 to the caller The clamp is what makes chained use safe with a single check at the end of the chain. Apply to database_info_build_query_enum (database_info.c). That function builds DB query strings of the shape {'KEY':"PATH"} (or numeric variants without the quotes, or {'developer':glob('*PATH*')}) via 22 switch cases, each of which was a hand-unrolled chain of s[++_len] = '...' character writes followed by a path strlcpy and another short s[++_len] = '...' chain to close the brace. None of those writes was bounded against len. A caller passing a buffer smaller than the prefix length got OOB-writes from the prefix; a caller whose buffer was just barely enough for prefix + path got the trailing s[_len]='"';s[++_len]='}' writes off the end (up to 3 bytes OOB). All callers in tree pass query[4096] from the menu displaylist, so practical buffers are large -- but the pattern is wrong and the path component is attacker- influenced when scanning content metadata. Side fix in DATABASE_QUERY_ENTRY_BBFC_RATING: pre-rewrite the case wrote s[++_len] = '"' after the path strlcpy instead of s[_len] = '"', leaving the strlcpy's NUL terminator embedded in the query. BBFC rating searches have been silently returning nothing since this code was written. Fixed while rewriting. Adds libretro-common/samples/string/strlcpy_append/ strlcpy_append_test as a regression test, registered in .github/workflows/Linux-libretro-common-samples.yml's RUN_TARGETS allowlist (which builds with ASan + UBSan default since the cdfs/chd commit). Eight cases covering the contract: basic append, chained append, exact-fit boundary, wide-margin truncation, chain short-circuiting after truncation, empty source, NULL/zero-length args, and *pos already at or past len. Test follows the existing libretro-common/samples pattern (heap-allocated destinations sized to exactly the legal capacity so any reintroduction of the unbounded chain is flagged by ASan). Verified to fire under ASan when the helper is replaced with the unsafe naive idiom: heap- buffer-overflow WRITE of size 2 at offset 26 of an 8-byte allocation in test_chain_short_circuits_after_truncation. With the safe helper all 8 cases pass clean. Verified database_info_build_query_enum produces identical output for all 21 enum values at len=4096 (the production caller's buffer size); also verified that at len=8 the function returns -1 with no OOB write, which is the behaviour the unsafe naive replacement would have failed at.
Apply strlcpy_append (commit 78c52ab) to seven sites flagged in the strlcpy-misuse audit, all sharing the same shape: _len = strlcpy(dst, src1, sizeof(dst)); dst[_len] = c1; dst[++_len] = c2; ... strlcpy(dst + _len, src2, sizeof(dst) - _len); None of these had a bound on _len. When src1 was longer than sizeof(dst) - 1, strlcpy returned strlen(src1) >= sizeof(dst) and: - the dst[_len], dst[++_len], ... character writes ran off the end of dst (stack buffer overflow); - the trailing strlcpy got len - _len which underflowed size_t, producing an unbounded copy. Sites: menu/drivers/materialui.c:8090 - "Draw message box". dst is msg[NAME_MAX_LENGTH] which is 128 on Xbox1, 3DS, PSP, PS2, GameCube, Wii, WiiU, PS3, Emscripten and 256 elsewhere. src1 is menu_st->input_dialog_kb_label[256] (always 256 regardless of platform). On the small- NAME_MAX_LENGTH platforms a long label drove _len up to ~255 and the chain wrote up to 127 bytes past the stack frame. Reachable from any input dialog. tasks/task_http.c:495 and tasks/task_http_emscripten.c:312 - download status string. dst is tmp[NAME_MAX_LENGTH]; src1 is the localized "Downloading" string. Same primitive on small-NAME_MAX_LENGTH platforms when a translation pushed the prefix close to the cap. menu/cbs/menu_cbs_sublabel.c:2164 - core backup CRC sublabel. The chain wrote 8 character bytes ("00000000") plus a NUL after the localized "Backup CRC: " prefix. dst is the caller's s/len; if a translation pushed the prefix close to len, the 9 byte writes ran past the caller's buffer. steam/steam.c:449/471/493 - rich-presence content strings. Three near-identical cases (CONTENT_SYSTEM, CONTENT_CORE, CONTENT_SYSTEM_CORE). Each builds "LABEL (CONTENT)" or "LABEL (N/A)" with hand-unrolled writes of " (", "N/A", "/", "A", ")", "\0" at offsets _len + N. dst is content[PATH_MAX_LENGTH] (512 on small platforms, 4096 elsewhere) and the per-character write offsets reached _len + 12 on the SYSTEM_CORE variant; long labels OOB- wrote up to 12 bytes past content. Replace each chain with a sequence of strlcpy_append calls on a single rolling cursor, with the helper handling the bound check at every step. No functional change at the buffer-fits-comfortably end; on small buffers the new code truncates cleanly instead of stack-overflowing. No new tests -- the strlcpy_append regression test from commit 78c52ab covers the helper's contract; these are just additional users.
Apply strlcpy_append (commit 78c52ab) to three further sites identified in the wider strlcpy-misuse sweep, all with the same shape that drove the prior commits c41e955 / 78c52ab / e446242: _len = strlcpy(buf, src1, sizeof(buf)); _len += strlcpy(buf + _len, src2, sizeof(buf) - _len); ... (more steps) ... That chain is unsafe because strlcpy returns strlen(source) regardless of truncation; on overflow _len overshoots sizeof(buf) and the next sizeof - _len underflows size_t, producing an unbounded copy that writes past the buffer. Sites: gfx/drivers/gl2.c:4549 - GL device-info string assembly. gl->device_str is 128 bytes; the chain appends `vendor`, ' ', and `renderer` (all from glGetString). Some drivers report long vendor/renderer combinations. Not attacker- reachable in the usual threat model but the pattern is still wrong, and the `device_str[_len]=' '; device_str [++_len]='\0'` separator chain in the middle was the short-chain shape from commit e446242. menu/menu_displaylist.c:2767 - create_string_list_rdb_entry _string. Builds out_lbl (NAME_MAX_LENGTH bytes) from label + "|" + actual_string + "|" + path, plus tmp (128 bytes) from desc + ": " + actual_string. The strings come from RetroArch database (.rdb) entries; a malicious .rdb could supply long values to drive the chain underflow. Modest threat model -- users typically use the RetroArch-shipped databases, not custom ones -- but the pattern is the same one we've fixed elsewhere. command.c:1119 - GET_STATUS network reply. Builds the reply for the network command interface from the paused/ playing state, system_id or library_name, content basename, and CRC. reply is 8192 bytes which fits any realistic content, but ROM basenames can legally be any length and the chain has 8 steps; a long enough basename drives the underflow. Replace each chain with sequences of strlcpy_append calls on a single rolling cursor. Same correctness story as e446242: helper handles the bound check at every step, truncation is silent and contained, no behaviour change for the buffer-fits-comfortably case. No new tests -- the strlcpy_append regression test from 78c52ab covers the helper's contract; these are additional users. Triaged but not fixed in this commit (safe by construction, flagged for the record): network/cloud_sync/webdav.c:401 - 16-call chain, but the function pre-calculates the exact required length via strlen() over the same inputs and mallocs that exact size before the chain runs. Single-threaded. retroarch.c:8769 - 20-call chain for SIMD feature names into a 128-byte buffer. Total max ~120 bytes; tight but no real CPU has all SIMD bits set so practical max is well under cap. gfx/video_shader_parse.c:1252, configuration.c:6961/7167, frontend/drivers/platform_emscripten.c:620 - classifier false positives (each _len is per-buffer reused, not accumulated cross-buffer).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )