mirror of
https://github.com/libretro/RetroArch.git
synced 2026-11-09 03:29:43 -10:00
menu/materialui: fix three heap-safety issues found during audit pass
Three independent issues found during a focused audit of menu/ drivers/materialui.c. One is a real heap overflow in the status-bar metadata builder driven by user-controllable input strings; one is an OOB write/read on power-user-sized playlist counts; one is a TOCTOU OOB read in the touch-tap handler. Same audit class as the recent xmb (9a27e66) and ozone (d56c728) commits, grouped together because they share an audit context and none is large enough to stand alone. * status-bar metadata: heap overflow via _len underflow in strlcpy chain (menu/drivers/materialui.c) materialui_render_process_entry_playlist_desktop builds the status-bar metadata string ("Core: <name> ⠀ <runtime> ⠀ <last played>") through a chain of six _len += strlcpy(buf + _len, src, sizeof(buf) - _len); calls. This pattern is NOT self-bounding: strlcpy returns strlen(src), not bytes-actually-written, so when any intermediate call truncates -- because the source is longer than the remaining buffer space -- _len overshoots sizeof(status_bar.str). The next iteration's `sizeof(...) - _len` underflows size_t to ~SIZE_MAX, the strlcpy treats the destination as essentially infinite, and writes proceed past status_bar.str into adjacent struct fields (runtime_fallback_str, last_played_fallback_str, then whatever follows in materialui_handle_t on the heap). Reachable via any path that produces a long source string: a crafted playlist entry with an outsize core_name, a long runtime log entry, or a verbose locale translation of MENU_ENUM_LABEL_VALUE_PLAYLIST_SUBLABEL_CORE. Convert the chain to strlcpy_append, the bound-checked replacement added in78c52aband applied across the codebase in25ade82/e446242. After truncation strlcpy_append clamps *pos to len-1 so the chain short-circuits cleanly instead of propagating an underflow. * playlist_selection[]: OOB write/read for users with too many playlists (menu/drivers/materialui.c) mui->playlist_selection is sized [NAME_MAX_LENGTH], which is the *file/dir name length cap* (128 on small builds, 256 on default) -- the wrong constant for what's actually a per-playlist-tab remembered-selection cache. materialui_navigation_set writes mui->playlist_selection[mui->playlist_selection_ptr] = selection; with no bound check on the index, where the index is the user's row in the playlists tab (also stored unchecked one branch above). A user with > NAME_MAX_LENGTH playlists -- not implausible for someone with FBNeo, MAME, and every console they own -- navigating to row N and then entering any playlist will OOB-write at offset N into adjacent struct fields and the heap beyond. The read site in materialui_populate_entries has the matching OOB read. Two-part fix. Replace NAME_MAX_LENGTH with a purpose-built MUI_PLAYLIST_SELECTION_MAX (1024 -- comfortable headroom for any plausible setup, ~8 KB cache on mui). Bound-check both the write and read against that constant. Selections in slots beyond MUI_PLAYLIST_SELECTION_MAX simply aren't remembered; falling through to the existing default-selection behaviour is preferable to a heap corruption. * materialui_pointer_up TAP path: OOB read on stale ptr (menu/drivers/materialui.c) The TAP-gesture branch does list = MENU_LIST_GET_SELECTION(menu_list, 0); if (!list) break; if (!(node = (materialui_node_t*)list->list[ptr].userdata)) break; with no bound check on ptr against list->size. ptr was set in a prior render frame's hit-test loop, where it was bounded by that frame's entries_end. Between render and event delivery the list can be repopulated -- search filter applied, navigation back/ forward, async list rebuild -- leaving ptr stale and possibly past the new list end. The neighbouring LONG_PRESS branch (line ~11199) and the swipe handler at materialui_pointer_up_swipe_horz_default (line ~10874) both already guard with `ptr < entries_end`; the TAP path missed the same defence. Add it.
This commit is contained in:
1 parent
14a6f8fe07
commit
d5e598aedc
1 file changed
+64
-27
+64
-27
@@ -151,6 +151,15 @@
|
||||
#define MUI_BATTERY_PERCENT_MAX_LENGTH 12
|
||||
#define MUI_TIMEDATE_MAX_LENGTH 255
|
||||
|
||||
/* Maximum number of playlist tabs whose last-selected
|
||||
* entry index can be cached in mui->playlist_selection[].
|
||||
* Indexed by the user's row in the playlists tab. Power
|
||||
* users with many cores can have hundreds of playlists --
|
||||
* 1024 covers any plausible setup with comfortable margin
|
||||
* while still bounding mui's heap footprint to ~8 KB for
|
||||
* this cache. */
|
||||
#define MUI_PLAYLIST_SELECTION_MAX 1024
|
||||
|
||||
/* Allow force enabling secondary thumbnail */
|
||||
#define MUI_FORCE_ENABLE_SECONDARY 0
|
||||
|
||||
@@ -677,7 +686,7 @@ typedef struct materialui_handle
|
||||
uint32_t flags;
|
||||
uint32_t context_generation;
|
||||
|
||||
size_t playlist_selection[NAME_MAX_LENGTH];
|
||||
size_t playlist_selection[MUI_PLAYLIST_SELECTION_MAX];
|
||||
size_t playlist_selection_ptr;
|
||||
uint8_t mainmenu_selection_ptr;
|
||||
uint8_t settings_selection_ptr;
|
||||
@@ -3742,28 +3751,36 @@ static bool materialui_render_process_entry_playlist_desktop(
|
||||
if (!last_played_str || !*last_played_str)
|
||||
last_played_str = mui->status_bar.last_played_fallback_str;
|
||||
|
||||
/* Generate metadata string */
|
||||
_len = strlcpy(mui->status_bar.str,
|
||||
msg_hash_to_str(MENU_ENUM_LABEL_VALUE_PLAYLIST_SUBLABEL_CORE),
|
||||
sizeof(mui->status_bar.str));
|
||||
_len += strlcpy(mui->status_bar.str + _len,
|
||||
" ",
|
||||
sizeof(mui->status_bar.str) - _len);
|
||||
_len += strlcpy(mui->status_bar.str + _len,
|
||||
core_name,
|
||||
sizeof(mui->status_bar.str) - _len);
|
||||
_len += strlcpy(mui->status_bar.str + _len,
|
||||
MUI_TICKER_SPACER,
|
||||
sizeof(mui->status_bar.str) - _len);
|
||||
_len += strlcpy(mui->status_bar.str + _len,
|
||||
runtime_str,
|
||||
sizeof(mui->status_bar.str) - _len);
|
||||
_len += strlcpy(mui->status_bar.str + _len,
|
||||
MUI_TICKER_SPACER,
|
||||
sizeof(mui->status_bar.str) - _len);
|
||||
strlcpy(mui->status_bar.str + _len,
|
||||
last_played_str,
|
||||
sizeof(mui->status_bar.str) - _len);
|
||||
/* Generate metadata string.
|
||||
*
|
||||
* Pre-conversion this used the unsafe pattern
|
||||
* _len += strlcpy(buf + _len, src, sizeof(buf) - _len)
|
||||
* which is NOT self-bounding: strlcpy returns
|
||||
* strlen(src), not bytes-actually-written, so on a
|
||||
* truncating call (long core_name from a crafted
|
||||
* playlist entry, long runtime / last-played strings,
|
||||
* verbose locale, ...) _len overshoots
|
||||
* sizeof(status_bar.str), the next len-_len subtraction
|
||||
* underflows size_t to ~SIZE_MAX, and the subsequent
|
||||
* strlcpy treats the destination as essentially
|
||||
* infinite. Adjacent struct fields
|
||||
* (runtime_fallback_str, last_played_fallback_str,
|
||||
* ...) get corrupted on the heap. */
|
||||
_len = 0;
|
||||
strlcpy_append(mui->status_bar.str, sizeof(mui->status_bar.str),
|
||||
&_len, msg_hash_to_str(MENU_ENUM_LABEL_VALUE_PLAYLIST_SUBLABEL_CORE));
|
||||
strlcpy_append(mui->status_bar.str, sizeof(mui->status_bar.str),
|
||||
&_len, " ");
|
||||
strlcpy_append(mui->status_bar.str, sizeof(mui->status_bar.str),
|
||||
&_len, core_name);
|
||||
strlcpy_append(mui->status_bar.str, sizeof(mui->status_bar.str),
|
||||
&_len, MUI_TICKER_SPACER);
|
||||
strlcpy_append(mui->status_bar.str, sizeof(mui->status_bar.str),
|
||||
&_len, runtime_str);
|
||||
strlcpy_append(mui->status_bar.str, sizeof(mui->status_bar.str),
|
||||
&_len, MUI_TICKER_SPACER);
|
||||
strlcpy_append(mui->status_bar.str, sizeof(mui->status_bar.str),
|
||||
&_len, last_played_str);
|
||||
|
||||
/* All metadata is cached */
|
||||
mui->flags |= MUI_FLAG_STATUSBAR_CACHED;
|
||||
@@ -9414,7 +9431,14 @@ static void materialui_navigation_set(void *data, bool scroll)
|
||||
return;
|
||||
|
||||
if (mui->flags & MUI_FLAG_IS_PLAYLIST)
|
||||
mui->playlist_selection[mui->playlist_selection_ptr] = selection;
|
||||
{
|
||||
/* Bound playlist_selection_ptr against the cache size:
|
||||
* users with > MUI_PLAYLIST_SELECTION_MAX playlists would
|
||||
* otherwise OOB-write into adjacent struct fields when
|
||||
* navigating any deep playlist. */
|
||||
if (mui->playlist_selection_ptr < MUI_PLAYLIST_SELECTION_MAX)
|
||||
mui->playlist_selection[mui->playlist_selection_ptr] = selection;
|
||||
}
|
||||
else if (mui->flags & MUI_FLAG_IS_PLAYLISTS_TAB)
|
||||
mui->playlist_selection_ptr = selection;
|
||||
else if (string_is_equal(mui->menu_title, msg_hash_to_str(MENU_ENUM_LABEL_VALUE_MAIN_MENU)))
|
||||
@@ -9830,8 +9854,9 @@ static void materialui_populate_entries(void *data, const char *path,
|
||||
if ( mui->flags & MUI_FLAG_IS_PLAYLIST
|
||||
&& !string_is_equal(label, MENU_ENUM_LABEL_LOAD_CONTENT_HISTORY_STR))
|
||||
{
|
||||
if ( remember_selection == MENU_REMEMBER_SELECTION_ALWAYS
|
||||
if ( (remember_selection == MENU_REMEMBER_SELECTION_ALWAYS
|
||||
|| remember_selection == MENU_REMEMBER_SELECTION_PLAYLISTS)
|
||||
&& mui->playlist_selection_ptr < MUI_PLAYLIST_SELECTION_MAX)
|
||||
menu_state_get_ptr()->selection_ptr = mui->playlist_selection[mui->playlist_selection_ptr];
|
||||
}
|
||||
else if (mui->flags & MUI_FLAG_IS_PLAYLISTS_TAB)
|
||||
@@ -11112,9 +11137,21 @@ static int materialui_pointer_up(void *userdata,
|
||||
}
|
||||
|
||||
/* Get node (entry) associated with current
|
||||
* pointer item */
|
||||
* pointer item.
|
||||
*
|
||||
* Bound-check ptr against the live list->size:
|
||||
* ptr was set by the render-frame hit-test loop,
|
||||
* which guarantees ptr < entries_end at the point
|
||||
* it ran -- but the list can be repopulated (search
|
||||
* filter, navigation, async list rebuild) before
|
||||
* this event handler executes, leaving ptr stale
|
||||
* and possibly past the new list end. The
|
||||
* LONG_PRESS case below and the swipe handler at
|
||||
* materialui_pointer_up_swipe_horz_default already
|
||||
* guard with `ptr < entries_end`; mirror that
|
||||
* defence here. */
|
||||
list = menu_list ? MENU_LIST_GET_SELECTION(menu_list, 0) : NULL;
|
||||
if (!list)
|
||||
if (!list || ptr >= list->size)
|
||||
break;
|
||||
|
||||
if (!(node = (materialui_node_t*)list->list[ptr].userdata))
|
||||
|
||||
Reference in new issue
Block a user