diff --git a/.github/workflows/Linux-libretro-common-samples.yml b/.github/workflows/Linux-libretro-common-samples.yml index 3e460bce7bd0..ddfaa50f5d39 100644 --- a/.github/workflows/Linux-libretro-common-samples.yml +++ b/.github/workflows/Linux-libretro-common-samples.yml @@ -69,6 +69,7 @@ jobs: cdrom_cuesheet_overflow_test cdfs_dir_record_test chd_meta_overflow_test + strlcpy_append_test http_parse_test rjson_test rtga_test diff --git a/.github/workflows/Linux-samples-tasks.yml b/.github/workflows/Linux-samples-tasks.yml index 59defcabc90f..060aba509781 100644 --- a/.github/workflows/Linux-samples-tasks.yml +++ b/.github/workflows/Linux-samples-tasks.yml @@ -171,3 +171,26 @@ jobs: test -x bsv_replay_bounds_test timeout 60 ./bsv_replay_bounds_test echo "[pass] bsv_replay_bounds_test" + + - name: Build and run bps_patch_bounds_test (ASan) + shell: bash + working-directory: samples/tasks/bps_patch + run: | + set -eu + # Regression test for the .bps patch parser bounds in + # tasks/task_patch.c::bps_apply_patch. Pre-fix a + # malicious .bps could write attacker-chosen bytes + # past the malloc'd target_data buffer (heap-buffer- + # overflow WRITE), read past source_data (info leak), + # and read past modify_data via TARGET_READ. The + # size-prelude was also unbounded, allowing 32-bit + # truncation of modify_target_size to drive a + # smaller-than-expected allocation. Build under + # AddressSanitizer so any reintroduction is caught + # at the bounds level. If task_patch.c amends the + # action-loop predicates, the verbatim copy in + # bps_patch_bounds_test.c must follow. + make clean all SANITIZER=address + test -x bps_patch_bounds_test + timeout 60 ./bps_patch_bounds_test + echo "[pass] bps_patch_bounds_test" diff --git a/command.c b/command.c index 281bebe95cf4..4b2777f400b1 100644 --- a/command.c +++ b/command.c @@ -1116,37 +1116,37 @@ bool command_get_status(command_t *cmd, const char* arg) core_info_get_current_core(&core_info); - _len = strlcpy(reply, "GET_STATUS ", sizeof(reply)); + _len = 0; + strlcpy_append(reply, sizeof(reply), &_len, "GET_STATUS "); if (runloop_st->flags & RUNLOOP_FLAG_PAUSED) - _len += strlcpy(reply + _len, "PAUSED", sizeof(reply) - _len); + strlcpy_append(reply, sizeof(reply), &_len, "PAUSED"); else - _len += strlcpy(reply + _len, "PLAYING", sizeof(reply) - _len); + strlcpy_append(reply, sizeof(reply), &_len, "PLAYING"); - _len += strlcpy(reply + _len, " ", sizeof(reply) - _len); + strlcpy_append(reply, sizeof(reply), &_len, " "); if (core_info && core_info->system_id) - _len += strlcpy(reply + _len, core_info->system_id, - sizeof(reply) - _len); + strlcpy_append(reply, sizeof(reply), &_len, core_info->system_id); else if (runloop_st->system.info.library_name) - _len += strlcpy(reply + _len, runloop_st->system.info.library_name, - sizeof(reply) - _len); + strlcpy_append(reply, sizeof(reply), &_len, + runloop_st->system.info.library_name); else - _len += strlcpy(reply + _len, "UNKNOWN", sizeof(reply) - _len); + strlcpy_append(reply, sizeof(reply), &_len, "UNKNOWN"); - _len += strlcpy(reply + _len, ",", sizeof(reply) - _len); + strlcpy_append(reply, sizeof(reply), &_len, ","); basename_path = path_get(RARCH_PATH_BASENAME); if (basename_path) { const char *basename = path_basename(basename_path); if (basename) - _len += strlcpy(reply + _len, basename, sizeof(reply) - _len); + strlcpy_append(reply, sizeof(reply), &_len, basename); else - _len += strlcpy(reply + _len, "UNKNOWN", sizeof(reply) - _len); + strlcpy_append(reply, sizeof(reply), &_len, "UNKNOWN"); } else - _len += strlcpy(reply + _len, "UNKNOWN", sizeof(reply) - _len); + strlcpy_append(reply, sizeof(reply), &_len, "UNKNOWN"); _len += snprintf(reply + _len, sizeof(reply) - _len, ",crc32=%lx\n", (unsigned long)content_get_crc()); diff --git a/database_info.c b/database_info.c index 59e119c80cd3..f70c33362bd3 100644 --- a/database_info.c +++ b/database_info.c @@ -35,450 +35,165 @@ int database_info_build_query_enum(char *s, size_t len, enum database_query_type type, const char *path) { + /* Build queries of the shape {'KEY':"PATH"} (or {'KEY':PATH} + * for numeric/rating fields, or the DEVELOPER glob form). + * + * Pre-this-rewrite each case unrolled the per-character + * writes with s[++_len]= and then a path strlcpy followed + * by another short s[++_len]= chain. None of those writes + * was bounded against len; if the caller's buffer was + * smaller than the prefix's length the writes ran off the + * end of s, and after the path strlcpy the trailing + * s[_len]='"';s[++_len]='}' sequence could OOB-write up to + * 3 bytes when path filled s. Naively replacing the chain + * with _len += strlcpy(s+_len, lit, len-_len) does not + * help: strlcpy returns strlen(source) regardless of + * truncation, so on overflow _len passes len and the next + * (len - _len) underflows size_t, producing an + * unbounded-size strlcpy and the same OOB. + * + * Use strlcpy_append (libretro-common/string/stdstring.c), + * which is bound-checked and clamps *pos to len - 1 on + * truncation, so subsequent calls in the chain short- + * circuit safely. The caller checks the final return + * value to detect any truncation in the chain. */ size_t _len = 0; + if (!s || len == 0) + return -1; + switch (type) { case DATABASE_QUERY_ENTRY: - s[ _len] = '{'; - s[++_len] = '\''; - s[++_len] = 'n'; - s[++_len] = 'a'; - s[++_len] = 'm'; - s[++_len] = 'e'; - s[++_len] = '\''; - s[++_len] = ':'; - s[++_len] = '"'; - s[++_len] = '\0'; - _len += strlcpy(s + _len, path, len - _len); - s[ _len] = '"'; - s[++_len] = '}'; - s[++_len] = '\0'; + strlcpy_append(s, len, &_len, "{'name':\""); + strlcpy_append(s, len, &_len, path); + if (strlcpy_append(s, len, &_len, "\"}")) + return -1; break; case DATABASE_QUERY_ENTRY_PUBLISHER: - s[ _len] = '{'; - s[++_len] = '\''; - s[++_len] = 'p'; - s[++_len] = 'u'; - s[++_len] = 'b'; - s[++_len] = 'l'; - s[++_len] = 'i'; - s[++_len] = 's'; - s[++_len] = 'h'; - s[++_len] = 'e'; - s[++_len] = 'r'; - s[++_len] = '\''; - s[++_len] = ':'; - s[++_len] = '"'; - s[++_len] = '\0'; - _len += strlcpy(s + _len, path, len - _len); - s[ _len] = '"'; - s[++_len] = '}'; - s[++_len] = '\0'; + strlcpy_append(s, len, &_len, "{'publisher':\""); + strlcpy_append(s, len, &_len, path); + if (strlcpy_append(s, len, &_len, "\"}")) + return -1; break; case DATABASE_QUERY_ENTRY_DEVELOPER: - s[ _len] = '{'; - s[++_len] = '\''; - s[++_len] = 'd'; - s[++_len] = 'e'; - s[++_len] = 'v'; - s[++_len] = 'e'; - s[++_len] = 'l'; - s[++_len] = 'o'; - s[++_len] = 'p'; - s[++_len] = 'e'; - s[++_len] = 'r'; - s[++_len] = '\''; - s[++_len] = ':'; - s[++_len] = 'g'; - s[++_len] = 'l'; - s[++_len] = 'o'; - s[++_len] = 'b'; - s[++_len] = '('; - s[++_len] = '\''; - s[++_len] = '*'; - s[++_len] = '\0'; - _len += strlcpy(s + _len, path, len - _len); - s[ _len] = '*'; - s[++_len] = '\''; - s[++_len] = ')'; - s[++_len] = '}'; - s[++_len] = '\0'; + strlcpy_append(s, len, &_len, "{'developer':glob('*"); + strlcpy_append(s, len, &_len, path); + if (strlcpy_append(s, len, &_len, "*')}")) + return -1; break; case DATABASE_QUERY_ENTRY_ORIGIN: - s[ _len] = '{'; - s[++_len] = '\''; - s[++_len] = 'o'; - s[++_len] = 'r'; - s[++_len] = 'i'; - s[++_len] = 'g'; - s[++_len] = 'i'; - s[++_len] = 'n'; - s[++_len] = '\''; - s[++_len] = ':'; - s[++_len] = '"'; - s[++_len] = '\0'; - _len += strlcpy(s + _len, path, len - _len); - s[ _len] = '"'; - s[++_len] = '}'; - s[++_len] = '\0'; + strlcpy_append(s, len, &_len, "{'origin':\""); + strlcpy_append(s, len, &_len, path); + if (strlcpy_append(s, len, &_len, "\"}")) + return -1; break; case DATABASE_QUERY_ENTRY_FRANCHISE: - s[ _len] = '{'; - s[++_len] = '\''; - s[++_len] = 'f'; - s[++_len] = 'r'; - s[++_len] = 'a'; - s[++_len] = 'n'; - s[++_len] = 'c'; - s[++_len] = 'h'; - s[++_len] = 'i'; - s[++_len] = 's'; - s[++_len] = 'e'; - s[++_len] = '\''; - s[++_len] = ':'; - s[++_len] = '"'; - s[++_len] = '\0'; - _len += strlcpy(s + _len, path, len - _len); - s[ _len] = '"'; - s[++_len] = '}'; - s[++_len] = '\0'; + strlcpy_append(s, len, &_len, "{'franchise':\""); + strlcpy_append(s, len, &_len, path); + if (strlcpy_append(s, len, &_len, "\"}")) + return -1; break; case DATABASE_QUERY_ENTRY_RATING: - s[ _len] = '{'; - s[++_len] = '\''; - s[++_len] = 'e'; - s[++_len] = 's'; - s[++_len] = 'r'; - s[++_len] = 'b'; - s[++_len] = '_'; - s[++_len] = 'r'; - s[++_len] = 'a'; - s[++_len] = 't'; - s[++_len] = 'i'; - s[++_len] = 'n'; - s[++_len] = 'g'; - s[++_len] = '\''; - s[++_len] = ':'; - s[++_len] = '"'; - _len += strlcpy(s + _len, path, len - _len); - s[ _len] = '"'; - s[++_len] = '}'; - s[++_len] = '\0'; + strlcpy_append(s, len, &_len, "{'esrb_rating':\""); + strlcpy_append(s, len, &_len, path); + if (strlcpy_append(s, len, &_len, "\"}")) + return -1; break; case DATABASE_QUERY_ENTRY_BBFC_RATING: - s[ _len] = '{'; - s[++_len] = '\''; - s[++_len] = 'b'; - s[++_len] = 'b'; - s[++_len] = 'f'; - s[++_len] = 'c'; - s[++_len] = '_'; - s[++_len] = 'r'; - s[++_len] = 'a'; - s[++_len] = 't'; - s[++_len] = 'i'; - s[++_len] = 'n'; - s[++_len] = 'g'; - s[++_len] = '\''; - s[++_len] = ':'; - s[++_len] = '"'; - s[++_len] = '\0'; - _len += strlcpy(s + _len, path, len - _len); - s[++_len] = '"'; - s[++_len] = '}'; - s[++_len] = '\0'; + /* Pre-rewrite this case wrote s[++_len] = '"' after + * the path strlcpy instead of s[_len] = '"', leaving + * the strlcpy's NUL terminator embedded in the query + * and silently breaking BBFC searches. Fix while + * rewriting. */ + strlcpy_append(s, len, &_len, "{'bbfc_rating':\""); + strlcpy_append(s, len, &_len, path); + if (strlcpy_append(s, len, &_len, "\"}")) + return -1; break; case DATABASE_QUERY_ENTRY_ELSPA_RATING: - s[ _len] = '{'; - s[++_len] = '\''; - s[++_len] = 'e'; - s[++_len] = 'l'; - s[++_len] = 's'; - s[++_len] = 'p'; - s[++_len] = 'a'; - s[++_len] = '_'; - s[++_len] = 'r'; - s[++_len] = 'a'; - s[++_len] = 't'; - s[++_len] = 'i'; - s[++_len] = 'n'; - s[++_len] = 'g'; - s[++_len] = '\''; - s[++_len] = ':'; - s[++_len] = '"'; - s[++_len] = '\0'; - _len += strlcpy(s + _len, path, len - _len); - s[ _len] = '"'; - s[++_len] = '}'; - s[++_len] = '\0'; + strlcpy_append(s, len, &_len, "{'elspa_rating':\""); + strlcpy_append(s, len, &_len, path); + if (strlcpy_append(s, len, &_len, "\"}")) + return -1; break; case DATABASE_QUERY_ENTRY_ESRB_RATING: - s[ _len] = '{'; - s[++_len] = '\''; - s[++_len] = 'e'; - s[++_len] = 's'; - s[++_len] = 'r'; - s[++_len] = 'b'; - s[++_len] = '_'; - s[++_len] = 'r'; - s[++_len] = 'a'; - s[++_len] = 't'; - s[++_len] = 'i'; - s[++_len] = 'n'; - s[++_len] = 'g'; - s[++_len] = '\''; - s[++_len] = ':'; - s[++_len] = '"'; - s[++_len] = '\0'; - _len += strlcpy(s + _len, path, len - _len); - s[ _len] = '"'; - s[++_len] = '}'; - s[++_len] = '\0'; + strlcpy_append(s, len, &_len, "{'esrb_rating':\""); + strlcpy_append(s, len, &_len, path); + if (strlcpy_append(s, len, &_len, "\"}")) + return -1; break; case DATABASE_QUERY_ENTRY_PEGI_RATING: - s[ _len] = '{'; - s[++_len] = '\''; - s[++_len] = 'p'; - s[++_len] = 'e'; - s[++_len] = 'g'; - s[++_len] = 'i'; - s[++_len] = '_'; - s[++_len] = 'r'; - s[++_len] = 'a'; - s[++_len] = 't'; - s[++_len] = 'i'; - s[++_len] = 'n'; - s[++_len] = 'g'; - s[++_len] = '\''; - s[++_len] = ':'; - s[++_len] = '"'; - s[++_len] = '\0'; - _len += strlcpy(s + _len, path, len - _len); - s[ _len] = '"'; - s[++_len] = '}'; - s[++_len] = '\0'; + strlcpy_append(s, len, &_len, "{'pegi_rating':\""); + strlcpy_append(s, len, &_len, path); + if (strlcpy_append(s, len, &_len, "\"}")) + return -1; break; case DATABASE_QUERY_ENTRY_CERO_RATING: - s[ _len] = '{'; - s[++_len] = '\''; - s[++_len] = 'c'; - s[++_len] = 'e'; - s[++_len] = 'r'; - s[++_len] = 'o'; - s[++_len] = '_'; - s[++_len] = 'r'; - s[++_len] = 'a'; - s[++_len] = 't'; - s[++_len] = 'i'; - s[++_len] = 'n'; - s[++_len] = 'g'; - s[++_len] = '\''; - s[++_len] = ':'; - s[++_len] = '"'; - s[++_len] = '\0'; - _len += strlcpy(s + _len, path, len - _len); - s[ _len] = '"'; - s[++_len] = '}'; - s[++_len] = '\0'; + strlcpy_append(s, len, &_len, "{'cero_rating':\""); + strlcpy_append(s, len, &_len, path); + if (strlcpy_append(s, len, &_len, "\"}")) + return -1; break; case DATABASE_QUERY_ENTRY_ENHANCEMENT_HW: - s[ _len] = '{'; - s[++_len] = '\''; - s[++_len] = 'e'; - s[++_len] = 'n'; - s[++_len] = 'h'; - s[++_len] = 'a'; - s[++_len] = 'n'; - s[++_len] = 'c'; - s[++_len] = 'e'; - s[++_len] = 'm'; - s[++_len] = 'e'; - s[++_len] = 'n'; - s[++_len] = 't'; - s[++_len] = '_'; - s[++_len] = 'h'; - s[++_len] = 'w'; - s[++_len] = '\''; - s[++_len] = ':'; - s[++_len] = '"'; - s[++_len] = '\0'; - _len += strlcpy(s + _len, path, len - _len); - s[ _len] = '"'; - s[++_len] = '}'; - s[++_len] = '\0'; + strlcpy_append(s, len, &_len, "{'enhancement_hw':\""); + strlcpy_append(s, len, &_len, path); + if (strlcpy_append(s, len, &_len, "\"}")) + return -1; break; case DATABASE_QUERY_ENTRY_EDGE_MAGAZINE_RATING: - s[ _len] = '{'; - s[++_len] = '\''; - s[++_len] = 'e'; - s[++_len] = 'd'; - s[++_len] = 'g'; - s[++_len] = 'e'; - s[++_len] = '_'; - s[++_len] = 'r'; - s[++_len] = 'a'; - s[++_len] = 't'; - s[++_len] = 'i'; - s[++_len] = 'n'; - s[++_len] = 'g'; - s[++_len] = '\''; - s[++_len] = ':'; - s[++_len] = '\0'; - _len += strlcpy(s + _len, path, len - _len); - s[ _len] = '}'; - s[++_len] = '\0'; + strlcpy_append(s, len, &_len, "{'edge_rating':"); + strlcpy_append(s, len, &_len, path); + if (strlcpy_append(s, len, &_len, "}")) + return -1; break; case DATABASE_QUERY_ENTRY_EDGE_MAGAZINE_ISSUE: - s[ _len] = '{'; - s[++_len] = '\''; - s[++_len] = 'e'; - s[++_len] = 'd'; - s[++_len] = 'g'; - s[++_len] = 'e'; - s[++_len] = '_'; - s[++_len] = 'i'; - s[++_len] = 's'; - s[++_len] = 's'; - s[++_len] = 'u'; - s[++_len] = 'e'; - s[++_len] = '\''; - s[++_len] = ':'; - s[++_len] = '\0'; - _len += strlcpy(s + _len, path, len - _len); - s[ _len] = '}'; - s[++_len] = '\0'; + strlcpy_append(s, len, &_len, "{'edge_issue':"); + strlcpy_append(s, len, &_len, path); + if (strlcpy_append(s, len, &_len, "}")) + return -1; break; case DATABASE_QUERY_ENTRY_FAMITSU_MAGAZINE_RATING: - s[ _len] = '{'; - s[++_len] = '\''; - s[++_len] = 'f'; - s[++_len] = 'a'; - s[++_len] = 'm'; - s[++_len] = 'i'; - s[++_len] = 't'; - s[++_len] = 's'; - s[++_len] = 'u'; - s[++_len] = '_'; - s[++_len] = 'r'; - s[++_len] = 'a'; - s[++_len] = 't'; - s[++_len] = 'i'; - s[++_len] = 'n'; - s[++_len] = 'g'; - s[++_len] = '\''; - s[++_len] = ':'; - s[++_len] = '\0'; - _len += strlcpy(s + _len, path, len - _len); - s[ _len] = '}'; - s[++_len] = '\0'; + strlcpy_append(s, len, &_len, "{'famitsu_rating':"); + strlcpy_append(s, len, &_len, path); + if (strlcpy_append(s, len, &_len, "}")) + return -1; break; case DATABASE_QUERY_ENTRY_RELEASEDATE_MONTH: - s[ _len] = '{'; - s[++_len] = '\''; - s[++_len] = 'r'; - s[++_len] = 'e'; - s[++_len] = 'l'; - s[++_len] = 'e'; - s[++_len] = 'a'; - s[++_len] = 's'; - s[++_len] = 'e'; - s[++_len] = 'm'; - s[++_len] = 'o'; - s[++_len] = 'n'; - s[++_len] = 't'; - s[++_len] = 'h'; - s[++_len] = '\''; - s[++_len] = ':'; - s[++_len] = '\0'; - _len += strlcpy(s + _len, path, len - _len); - s[ _len] = '}'; - s[++_len] = '\0'; + strlcpy_append(s, len, &_len, "{'releasemonth':"); + strlcpy_append(s, len, &_len, path); + if (strlcpy_append(s, len, &_len, "}")) + return -1; break; case DATABASE_QUERY_ENTRY_RELEASEDATE_YEAR: - s[ _len] = '{'; - s[++_len] = '\''; - s[++_len] = 'r'; - s[++_len] = 'e'; - s[++_len] = 'l'; - s[++_len] = 'e'; - s[++_len] = 'a'; - s[++_len] = 's'; - s[++_len] = 'e'; - s[++_len] = 'y'; - s[++_len] = 'e'; - s[++_len] = 'a'; - s[++_len] = 'r'; - s[++_len] = '\''; - s[++_len] = ':'; - s[++_len] = '\0'; - _len += strlcpy(s + _len, path, len - _len); - s[ _len] = '}'; - s[++_len] = '\0'; + strlcpy_append(s, len, &_len, "{'releaseyear':"); + strlcpy_append(s, len, &_len, path); + if (strlcpy_append(s, len, &_len, "}")) + return -1; break; case DATABASE_QUERY_ENTRY_MAX_USERS: - s[ _len] = '{'; - s[++_len] = '\''; - s[++_len] = 'u'; - s[++_len] = 's'; - s[++_len] = 'e'; - s[++_len] = 'r'; - s[++_len] = 's'; - s[++_len] = '\''; - s[++_len] = ':'; - s[++_len] = '\0'; - _len += strlcpy(s + _len, path, len - _len); - s[ _len] = '}'; - s[++_len] = '\0'; + strlcpy_append(s, len, &_len, "{'users':"); + strlcpy_append(s, len, &_len, path); + if (strlcpy_append(s, len, &_len, "}")) + return -1; break; case DATABASE_QUERY_ENTRY_GENRE: - s[ _len] = '{'; - s[++_len] = '\''; - s[++_len] = 'g'; - s[++_len] = 'e'; - s[++_len] = 'n'; - s[++_len] = 'r'; - s[++_len] = 'e'; - s[++_len] = '\''; - s[++_len] = ':'; - s[++_len] = '"'; - s[++_len] = '\0'; - _len += strlcpy(s + _len, path, len - _len); - s[ _len] = '"'; - s[++_len] = '}'; - s[++_len] = '\0'; + strlcpy_append(s, len, &_len, "{'genre':\""); + strlcpy_append(s, len, &_len, path); + if (strlcpy_append(s, len, &_len, "\"}")) + return -1; break; case DATABASE_QUERY_ENTRY_REGION: - s[ _len] = '{'; - s[++_len] = '\''; - s[++_len] = 'r'; - s[++_len] = 'e'; - s[++_len] = 'g'; - s[++_len] = 'i'; - s[++_len] = 'o'; - s[++_len] = 'n'; - s[++_len] = '\''; - s[++_len] = ':'; - s[++_len] = '"'; - s[++_len] = '\0'; - _len += strlcpy(s + _len, path, len - _len); - s[ _len] = '"'; - s[++_len] = '}'; - s[++_len] = '\0'; + strlcpy_append(s, len, &_len, "{'region':\""); + strlcpy_append(s, len, &_len, path); + if (strlcpy_append(s, len, &_len, "\"}")) + return -1; break; case DATABASE_QUERY_NONE: - s[ _len] = '{'; - s[++_len] = '\''; - s[++_len] = '\''; - s[++_len] = ':'; - s[++_len] = '\''; - s[++_len] = ':'; - s[++_len] = '"'; - s[++_len] = '\0'; - _len += strlcpy(s + _len, path, len - _len); - s[ _len] = '"'; - s[++_len] = '}'; - s[++_len] = '\0'; + strlcpy_append(s, len, &_len, "{'':':\""); + strlcpy_append(s, len, &_len, path); + if (strlcpy_append(s, len, &_len, "\"}")) + return -1; break; } diff --git a/gfx/drivers/gl2.c b/gfx/drivers/gl2.c index 4b0f5bcf6c44..5efbaf260e8c 100644 --- a/gfx/drivers/gl2.c +++ b/gfx/drivers/gl2.c @@ -4548,16 +4548,15 @@ static void *gl2_init(const video_info_t *video, if (vendor && *vendor) { - _len = strlcpy(gl->device_str, vendor, sizeof(gl->device_str)); - gl->device_str[ _len] = ' '; - gl->device_str[++_len] = '\0'; + strlcpy_append(gl->device_str, sizeof(gl->device_str), &_len, vendor); + strlcpy_append(gl->device_str, sizeof(gl->device_str), &_len, " "); } if (renderer && *renderer) - strlcpy(gl->device_str + _len, renderer, sizeof(gl->device_str) - _len); + strlcpy_append(gl->device_str, sizeof(gl->device_str), &_len, renderer); if (version && *version) - video_driver_set_gpu_api_version_string(version); + video_driver_set_gpu_api_version_string(version); } #ifdef _WIN32 diff --git a/gfx/video_driver.c b/gfx/video_driver.c index cb69726f23f2..d29c0d23338e 100644 --- a/gfx/video_driver.c +++ b/gfx/video_driver.c @@ -3189,10 +3189,13 @@ size_t video_driver_get_window_title(char *s, size_t len) video_driver_state_t *video_st = &video_driver_st; if (s && (video_st->flags & VIDEO_FLAG_WINDOW_TITLE_UPDATE)) { - strlcpy(s, video_st->window_title, len); + size_t n = strlcpy(s, video_st->window_title, len); video_st->flags &= ~VIDEO_FLAG_WINDOW_TITLE_UPDATE; + if (n >= len) + return len ? len - 1 : 0; + return n; } - return video_st->window_title_len; + return 0; } void video_driver_update_title(void *data) diff --git a/libretro-common/include/string/stdstring.h b/libretro-common/include/string/stdstring.h index f55343c18b5b..aff5ebc17bf4 100644 --- a/libretro-common/include/string/stdstring.h +++ b/libretro-common/include/string/stdstring.h @@ -355,6 +355,38 @@ void string_replace_multi_space_with_single_space(char *str); **/ size_t string_remove_all_whitespace(char *str_trimmed, const char *str); +/** + * strlcpy_append: + * @s : destination buffer + * @len : total size of @s, including space for the NUL + * @pos : in-out cursor. On entry, the offset within + * @s where @src should be appended; on + * successful return advanced past the appended + * bytes. On truncation clamped to @len - 1 so + * subsequent calls in a chain short-circuit. + * @src : NUL-terminated source string to append + * + * Bound-checked replacement for the unsafe pattern + * + * *pos += strlcpy(@s + *pos, @src, @len - *pos); + * + * which is widely used to build strings piece-by-piece but is + * NOT self-bounding -- strlcpy returns strlen(@src) regardless + * of how much was actually written, so on truncation *pos + * overshoots @len and the next @len - *pos subtraction underflows + * size_t, corrupting subsequent appends. + * + * On success returns 0 and advances *pos by strlen(@src). On + * truncation (the appended bytes plus a NUL would exceed @len) + * returns -1, leaves @s NUL-terminated at @len - 1, and clamps + * *pos to @len - 1 so a chain of strlcpy_append calls can + * short-circuit cleanly -- the caller need only check the bitwise + * OR of the chain's results once at the end. + * + * @return 0 on success, -1 on truncation or invalid arguments + **/ +int strlcpy_append(char *s, size_t len, size_t *pos, const char *src); + /* Retrieve the last occurance of the given character in a string. */ int string_index_last_occurance(const char *str, char c); diff --git a/libretro-common/samples/string/strlcpy_append/Makefile b/libretro-common/samples/string/strlcpy_append/Makefile new file mode 100644 index 000000000000..0f8a45e4fc0d --- /dev/null +++ b/libretro-common/samples/string/strlcpy_append/Makefile @@ -0,0 +1,35 @@ +TARGET := strlcpy_append_test + +LIBRETRO_COMM_DIR := ../../.. + +# strlcpy_append lives in libretro-common/string/stdstring.c. +# Only the helper itself plus strlcpy (compat_strl.c) are needed +# at link time. +SOURCES := \ + strlcpy_append_test.c \ + $(LIBRETRO_COMM_DIR)/string/stdstring.c \ + $(LIBRETRO_COMM_DIR)/encodings/encoding_utf.c \ + $(LIBRETRO_COMM_DIR)/compat/compat_strl.c \ + $(LIBRETRO_COMM_DIR)/compat/compat_posix_string.c + +OBJS := $(SOURCES:.c=.o) + +CFLAGS += -Wall -pedantic -std=gnu99 -g -O0 -I$(LIBRETRO_COMM_DIR)/include + +ifneq ($(SANITIZER),) + CFLAGS := -fsanitize=$(SANITIZER) -fno-omit-frame-pointer $(CFLAGS) + LDFLAGS := -fsanitize=$(SANITIZER) $(LDFLAGS) +endif + +all: $(TARGET) + +%.o: %.c + $(CC) -c -o $@ $< $(CFLAGS) + +$(TARGET): $(OBJS) + $(CC) -o $@ $^ $(LDFLAGS) + +clean: + rm -f $(TARGET) $(OBJS) + +.PHONY: clean diff --git a/libretro-common/samples/string/strlcpy_append/strlcpy_append_test.c b/libretro-common/samples/string/strlcpy_append/strlcpy_append_test.c new file mode 100644 index 000000000000..978873490754 --- /dev/null +++ b/libretro-common/samples/string/strlcpy_append/strlcpy_append_test.c @@ -0,0 +1,268 @@ +/* Copyright (C) 2010-2026 The RetroArch team + * + * --------------------------------------------------------------------------------------- + * The following license statement only applies to this file (strlcpy_append_test.c). + * --------------------------------------------------------------------------------------- + * + * Permission is hereby granted, free of charge, + * to any person obtaining a copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation the rights to + * use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, + * and to permit persons to whom the Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, + * INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. + * IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, + * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ + +/* Regression test for strlcpy_append in + * libretro-common/string/stdstring.c. + * + * strlcpy_append exists because the unsafe idiom + * _len += strlcpy(s + _len, src, len - _len); + * silently corrupts @s on truncation: strlcpy returns + * strlen(@src), so once @src is too long for the remaining + * space, _len overshoots @len and the next subtraction + * (len - _len) underflows size_t. + * + * Contract: on success the function advances *pos by + * strlen(@src) and returns 0. On truncation it leaves @s + * NUL-terminated and clamps *pos to len - 1, so subsequent + * calls in a chain short-circuit (also returning -1). This + * lets callers chain three or more appends and check just + * the final return value. + * + * Heap-allocated destinations sized to exactly the legal + * capacity so AddressSanitizer flags any reintroduction of + * the unbounded chain. + */ + +#include +#include +#include + +#include + +static int failures = 0; + +#define EXPECT_EQ_INT(got, want, msg) do { \ + long _g = (long)(got), _w = (long)(want); \ + if (_g != _w) { \ + printf("[ERROR] %s:%d %s got=%ld want=%ld\n", \ + __func__, __LINE__, (msg), _g, _w); \ + failures++; \ + } \ +} while (0) + +#define EXPECT_EQ_STR(got, want, msg) do { \ + if (strcmp((got), (want)) != 0) { \ + printf("[ERROR] %s:%d %s got=\"%s\" want=\"%s\"\n", \ + __func__, __LINE__, (msg), (got), (want)); \ + failures++; \ + } \ +} while (0) + +#define EXPECT_TRUE(cond, msg) do { \ + if (!(cond)) { \ + printf("[ERROR] %s:%d %s\n", __func__, __LINE__, (msg)); \ + failures++; \ + } \ +} while (0) + +/* ---- tests ------------------------------------------------ */ + +static void test_basic_append(void) +{ + char *s = (char*)malloc(16); + size_t pos = 0; + int rv, saved = failures; + if (!s) { printf("[ERROR] alloc\n"); failures++; return; } + memset(s, 0, 16); + + rv = strlcpy_append(s, 16, &pos, "hello"); + EXPECT_EQ_INT(rv, 0, "rv"); + EXPECT_EQ_INT(pos, 5, "pos"); + EXPECT_EQ_STR(s, "hello", "buffer"); + + free(s); + if (failures == saved) printf("[SUCCESS] basic append\n"); +} + +static void test_chained_append(void) +{ + char *s = (char*)malloc(32); + size_t pos = 0; + int rv1, rv2, rv3, saved = failures; + if (!s) { printf("[ERROR] alloc\n"); failures++; return; } + memset(s, 0, 32); + + rv1 = strlcpy_append(s, 32, &pos, "hello, "); + rv2 = strlcpy_append(s, 32, &pos, "world"); + rv3 = strlcpy_append(s, 32, &pos, "!"); + EXPECT_EQ_INT(rv1, 0, "rv1"); + EXPECT_EQ_INT(rv2, 0, "rv2"); + EXPECT_EQ_INT(rv3, 0, "rv3"); + EXPECT_EQ_INT(pos, 13, "pos"); + EXPECT_EQ_STR(s, "hello, world!", "buffer"); + + free(s); + if (failures == saved) printf("[SUCCESS] chained append\n"); +} + +static void test_truncation_at_boundary(void) +{ + /* Buffer sized exactly for "hello" + NUL = 6. First append + * fits exactly. Second append truncates: rv = -1, pos + * clamps to len - 1 = 5, buffer remains "hello". */ + char *s = (char*)malloc(6); + size_t pos = 0; + int rv, saved = failures; + if (!s) { printf("[ERROR] alloc\n"); failures++; return; } + memset(s, 0, 6); + + rv = strlcpy_append(s, 6, &pos, "hello"); + EXPECT_EQ_INT(rv, 0, "first rv"); + EXPECT_EQ_INT(pos, 5, "first pos"); + EXPECT_EQ_STR(s, "hello", "buffer after first"); + + rv = strlcpy_append(s, 6, &pos, "x"); + EXPECT_EQ_INT(rv, -1, "second rv (truncation)"); + EXPECT_EQ_INT(pos, 5, "pos clamped to len - 1"); + EXPECT_EQ_STR(s, "hello", "buffer NUL-terminated at len - 1"); + + free(s); + if (failures == saved) printf("[SUCCESS] truncation at boundary\n"); +} + +static void test_truncation_wide(void) +{ + /* Source much larger than destination. Pre-fix the naive + * idiom would have memcpy'd off the end of s; with + * strlcpy_append the call clamps without writing past the + * buffer. Heap-allocated to exactly 4 bytes so ASan flags + * any OOB. */ + char *s = (char*)malloc(4); + size_t pos = 0; + int rv, saved = failures; + if (!s) { printf("[ERROR] alloc\n"); failures++; return; } + memset(s, 0, 4); + + rv = strlcpy_append(s, 4, &pos, + "this string is much longer than the destination"); + EXPECT_EQ_INT(rv, -1, "rv"); + EXPECT_EQ_INT(pos, 3, "pos clamped to len - 1"); + /* strlcpy fills 3 chars + NUL. Buffer is "thi". */ + EXPECT_EQ_STR(s, "thi", "buffer truncated and NUL-terminated"); + + free(s); + if (failures == saved) printf("[SUCCESS] truncation by wide margin\n"); +} + +static void test_chain_short_circuits_after_truncation(void) +{ + /* Once a previous call truncates and clamps *pos to len - 1, + * every subsequent call short-circuits with -1 without + * writing past the buffer. This is the property that makes + * "check only the last return value" safe. */ + char *s = (char*)malloc(8); + size_t pos = 0; + int rv1, rv2, rv3, saved = failures; + if (!s) { printf("[ERROR] alloc\n"); failures++; return; } + memset(s, 0, 8); + + rv1 = strlcpy_append(s, 8, &pos, "ab"); + rv2 = strlcpy_append(s, 8, &pos, + "this exceeds the remaining space"); + rv3 = strlcpy_append(s, 8, &pos, "cd"); + + EXPECT_EQ_INT(rv1, 0, "rv1 fits"); + EXPECT_EQ_INT(rv2, -1, "rv2 truncates"); + EXPECT_EQ_INT(rv3, -1, "rv3 short-circuits"); + EXPECT_EQ_INT(pos, 7, "pos clamped at len - 1"); + EXPECT_TRUE(strlen(s) <= 7, "buffer fits"); + + free(s); + if (failures == saved) printf("[SUCCESS] chain short-circuits after truncation\n"); +} + +static void test_empty_source(void) +{ + char *s = (char*)malloc(8); + size_t pos = 0; + int rv, saved = failures; + if (!s) { printf("[ERROR] alloc\n"); failures++; return; } + memset(s, 0, 8); + + rv = strlcpy_append(s, 8, &pos, ""); + EXPECT_EQ_INT(rv, 0, "rv"); + EXPECT_EQ_INT(pos, 0, "pos unchanged"); + EXPECT_EQ_STR(s, "", "buffer empty"); + + rv = strlcpy_append(s, 8, &pos, "ok"); + EXPECT_EQ_INT(rv, 0, "second rv"); + EXPECT_EQ_INT(pos, 2, "second pos"); + EXPECT_EQ_STR(s, "ok", "second buffer"); + + free(s); + if (failures == saved) printf("[SUCCESS] empty source\n"); +} + +static void test_null_args(void) +{ + char buf[16]; + size_t pos = 0; + int saved = failures; + + EXPECT_EQ_INT(strlcpy_append(NULL, 16, &pos, "x"), -1, "NULL s"); + EXPECT_EQ_INT(strlcpy_append(buf, 16, NULL, "x"), -1, "NULL pos"); + EXPECT_EQ_INT(strlcpy_append(buf, 16, &pos, NULL), -1, "NULL src"); + EXPECT_EQ_INT(strlcpy_append(buf, 0, &pos, "x"), -1, "zero len"); + + if (failures == saved) printf("[SUCCESS] NULL/zero args rejected\n"); +} + +static void test_pos_at_or_past_len(void) +{ + char *s = (char*)malloc(8); + size_t pos = 8; + int rv, saved = failures; + if (!s) { printf("[ERROR] alloc\n"); failures++; return; } + memset(s, 0, 8); + + rv = strlcpy_append(s, 8, &pos, "x"); + EXPECT_EQ_INT(rv, -1, "pos == len rejects"); + EXPECT_EQ_INT(pos, 7, "pos clamped to len - 1"); + + pos = 99; + rv = strlcpy_append(s, 8, &pos, "x"); + EXPECT_EQ_INT(rv, -1, "pos > len rejects"); + EXPECT_EQ_INT(pos, 7, "pos clamped to len - 1"); + + free(s); + if (failures == saved) printf("[SUCCESS] pos at/past len rejected and clamped\n"); +} + +int main(void) +{ + test_basic_append(); + test_chained_append(); + test_truncation_at_boundary(); + test_truncation_wide(); + test_chain_short_circuits_after_truncation(); + test_empty_source(); + test_null_args(); + test_pos_at_or_past_len(); + + if (failures) + { + printf("\n%d strlcpy_append test(s) failed\n", failures); + return 1; + } + printf("\nAll strlcpy_append regression tests passed.\n"); + return 0; +} diff --git a/libretro-common/string/stdstring.c b/libretro-common/string/stdstring.c index 00193fd93a7b..425591a267ab 100644 --- a/libretro-common/string/stdstring.c +++ b/libretro-common/string/stdstring.c @@ -680,6 +680,51 @@ size_t string_remove_all_whitespace(char *s, const char *str) return s - start; } +/** + * strlcpy_append: + * + * See header (libretro-common/include/string/stdstring.h) for the + * full contract. Bound-checked append; advances *pos by + * strlen(@src) on success, returns -1 on truncation. + * + * Truncation is signalled when *pos >= len already, or when + * strlcpy returns a value >= the remaining capacity. In either + * case the destination is left NUL-terminated (strlcpy guarantees + * this when its size argument is non-zero) and *pos is clamped to + * len - 1 so subsequent calls in a chain become no-ops that also + * return -1. + **/ +int strlcpy_append(char *s, size_t len, size_t *pos, const char *src) +{ + size_t remaining; + size_t n; + + if (!s || !pos || !src || len == 0) + return -1; + + if (*pos >= len) + { + /* Already saturated; clamp and report truncation. */ + *pos = len - 1; + return -1; + } + + remaining = len - *pos; + n = strlcpy(s + *pos, src, remaining); + + if (n >= remaining) + { + /* strlcpy truncated. s + len - 1 is NUL-terminated by + * strlcpy's contract. Clamp *pos so subsequent appends + * short-circuit. */ + *pos = len - 1; + return -1; + } + + *pos += n; + return 0; +} + /** * Retrieve the last occurrence of the given character in a string. */ diff --git a/menu/cbs/menu_cbs_sublabel.c b/menu/cbs/menu_cbs_sublabel.c index de8c0eec36b6..832ab1de7217 100644 --- a/menu/cbs/menu_cbs_sublabel.c +++ b/menu/cbs/menu_cbs_sublabel.c @@ -2160,25 +2160,14 @@ static int action_bind_sublabel_core_backup_entry( const char *crc = list->list[i].alt ? list->list[i].alt : list->list[i].path; - /* Set sublabel prefix */ - size_t _len = strlcpy(s, msg_hash_to_str(MENU_ENUM_LABEL_VALUE_CORE_BACKUP_CRC), len); - + size_t _len = 0; + strlcpy_append(s, len, &_len, + msg_hash_to_str(MENU_ENUM_LABEL_VALUE_CORE_BACKUP_CRC)); /* Add CRC string */ if (!crc || !*crc) - { - s[ _len] = '0'; - s[++_len] = '0'; - s[++_len] = '0'; - s[++_len] = '0'; - s[++_len] = '0'; - s[++_len] = '0'; - s[++_len] = '0'; - s[++_len] = '0'; - s[++_len] = '\0'; - } + strlcpy_append(s, len, &_len, "00000000"); else - strlcpy(s + _len, crc, len - _len); - + strlcpy_append(s, len, &_len, crc); return 1; } diff --git a/menu/drivers/materialui.c b/menu/drivers/materialui.c index 96701c8d8a9d..88d496bea584 100644 --- a/menu/drivers/materialui.c +++ b/menu/drivers/materialui.c @@ -8087,12 +8087,10 @@ static void materialui_frame(void *data, video_frame_info_t *video_info) NULL); /* Draw message box */ - _len = strlcpy(msg, label, sizeof(msg)); - msg[ _len] = '\n'; - msg[++_len] = '\0'; - strlcpy(msg + _len, - str, - sizeof(msg) - _len); + _len = 0; + strlcpy_append(msg, sizeof(msg), &_len, label); + strlcpy_append(msg, sizeof(msg), &_len, "\n"); + strlcpy_append(msg, sizeof(msg), &_len, str); materialui_render_messagebox(mui, p_disp, userdata, video_width, video_height, diff --git a/menu/menu_displaylist.c b/menu/menu_displaylist.c index 78c185b58b3e..b64637f81fb0 100644 --- a/menu/menu_displaylist.c +++ b/menu/menu_displaylist.c @@ -2764,14 +2764,16 @@ static int create_string_list_rdb_entry_string( { char tmp[128]; char out_lbl[NAME_MAX_LENGTH]; - size_t _len = strlcpy(out_lbl, label, sizeof(out_lbl)); - _len += strlcpy(out_lbl + _len, "|", sizeof(out_lbl) - _len); - _len += strlcpy(out_lbl + _len, actual_string, sizeof(out_lbl) - _len); - _len += strlcpy(out_lbl + _len, "|", sizeof(out_lbl) - _len); - strlcpy(out_lbl + _len, path, sizeof(out_lbl) - _len); - _len = strlcpy(tmp, desc, sizeof(tmp)); - _len += strlcpy(tmp + _len, ": ", sizeof(tmp) - _len); - strlcpy(tmp + _len, actual_string, sizeof(tmp) - _len); + size_t _len = 0; + strlcpy_append(out_lbl, sizeof(out_lbl), &_len, label); + strlcpy_append(out_lbl, sizeof(out_lbl), &_len, "|"); + strlcpy_append(out_lbl, sizeof(out_lbl), &_len, actual_string); + strlcpy_append(out_lbl, sizeof(out_lbl), &_len, "|"); + strlcpy_append(out_lbl, sizeof(out_lbl), &_len, path); + _len = 0; + strlcpy_append(tmp, sizeof(tmp), &_len, desc); + strlcpy_append(tmp, sizeof(tmp), &_len, ": "); + strlcpy_append(tmp, sizeof(tmp), &_len, actual_string); menu_entries_append(list, tmp, out_lbl, enum_idx, 0, 0, 0, NULL); diff --git a/samples/tasks/bps_patch/Makefile b/samples/tasks/bps_patch/Makefile new file mode 100644 index 000000000000..f2132ec710a6 --- /dev/null +++ b/samples/tasks/bps_patch/Makefile @@ -0,0 +1,25 @@ +TARGET := bps_patch_bounds_test + +SOURCES := bps_patch_bounds_test.c + +OBJS := $(SOURCES:.c=.o) + +CFLAGS += -Wall -pedantic -std=gnu99 -g -O0 + +ifneq ($(SANITIZER),) + CFLAGS := -fsanitize=$(SANITIZER) -fno-omit-frame-pointer $(CFLAGS) + LDFLAGS := -fsanitize=$(SANITIZER) $(LDFLAGS) +endif + +all: $(TARGET) + +%.o: %.c + $(CC) -c -o $@ $< $(CFLAGS) + +$(TARGET): $(OBJS) + $(CC) -o $@ $^ $(LDFLAGS) + +clean: + rm -f $(TARGET) $(OBJS) + +.PHONY: clean diff --git a/samples/tasks/bps_patch/bps_patch_bounds_test.c b/samples/tasks/bps_patch/bps_patch_bounds_test.c new file mode 100644 index 000000000000..25d7f5290bda --- /dev/null +++ b/samples/tasks/bps_patch/bps_patch_bounds_test.c @@ -0,0 +1,415 @@ +/* Copyright (C) 2010-2026 The RetroArch team + * + * --------------------------------------------------------------------------------------- + * The following license statement only applies to this file (bps_patch_bounds_test.c). + * --------------------------------------------------------------------------------------- + * + * Permission is hereby granted, free of charge, + * to any person obtaining a copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation the rights to + * use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, + * and to permit persons to whom the Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, + * INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. + * IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, + * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ + +/* Regression test for the .bps patch parser bounds in + * tasks/task_patch.c::bps_apply_patch. + * + * The .bps format is a variable-length-integer-driven + * stream of action commands operating on three buffers + * (modify, source, target). Pre-this-patch the action + * loop performed every read/write without bounds: + * + * case TARGET_READ: + * while (_len--) { + * uint8_t data = bps_read(&bps); + * bps.target_data[bps.output_offset++] = data; // OOB write + * ... + * } + * + * 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; // OOB write + * ... + * } + * + * A malicious .bps patch could write attacker-chosen bytes + * past the malloc'd target_data buffer (heap-buffer-overflow + * WRITE), read past source_data (heap-info leak that flows + * into target_data and the patch checksum, exposing heap + * contents to a process that reads the patched output), and + * read past modify_data via TARGET_READ when the per-command + * _len exceeded the remaining bytes. + * + * Plus the size-prelude: the three variable-length integers + * decoded immediately after the "BPS1" header (modify_source_ + * size, modify_target_size, modify_markup_size) had no upper + * bound. modify_target_size silently truncated to size_t on + * 32-bit hosts producing a smaller allocation than + * bps.target_length expected; modify_markup_size of UINT64_MAX + * drove the markup-skip loop into reading the entire patch + * buffer and beyond. + * + * Reachability: user has soft-patching enabled, places a + * malicious .bps next to a ROM (or downloads a "ROM pack" + * containing patches). Same threat class as CDFS / CHD / BSV + * file-load bugs. + * + * Fix: bound _len against (target_length - output_offset) + * once at the top of each loop iteration; bound source/target + * offset+_len against the respective buffers; bound the size + * prelude against SIZE_MAX and remaining patch bytes. + * + * IMPORTANT: this test keeps verbatim copies of the post-fix + * predicates from tasks/task_patch.c. If task_patch.c amends, + * the copies below must follow. Convention used by + * archive_name_safety_test, http_method_match_test, + * video_shader_wildcard_test, input_remap_bounds_test, + * bsv_replay_bounds_test in this directory tree. + */ + +#include +#include +#include +#include +#include +#include + +enum bps_mode_t { SOURCE_READ = 0, TARGET_READ, SOURCE_COPY, TARGET_COPY }; + +struct mock_bps { + const uint8_t *modify_data; + const uint8_t *source_data; + uint8_t *target_data; + uint64_t modify_length; + uint64_t source_length; + uint64_t target_length; + uint64_t modify_offset; + uint64_t source_offset; + uint64_t target_offset; + uint64_t output_offset; +}; + +static int failures = 0; + +static uint8_t bps_read(struct mock_bps *b) +{ + /* Test scope: assume caller has bounded the read. If a + * test would have caused this to OOB, the post-fix + * predicate should have rejected the patch first. */ + return b->modify_data[b->modify_offset++]; +} + +static uint64_t bps_decode(struct mock_bps *b) +{ + uint64_t data = 0, shift = 1; + for (;;) + { + uint8_t x = bps_read(b); + data += (x & 0x7f) * shift; + if (x & 0x80) break; + shift <<= 7; + data += shift; + } + return data; +} + +/* === verbatim copy of the post-fix action loop predicates + * from tasks/task_patch.c::bps_apply_patch. Returns 0 + * on success, non-zero on a bounds-rejection (the + * production code returns specific patch_error values; + * this mock returns generic non-zero). If task_patch.c + * amends the predicates, this copy must follow. === */ +static int apply_action_loop(struct mock_bps *bps) +{ + while (bps->modify_offset < bps->modify_length - 12) + { + uint64_t _len = bps_decode(bps); + unsigned mode = _len & 3; + _len = (_len >> 2) + 1; + + if ( bps->output_offset >= bps->target_length + || _len > bps->target_length - bps->output_offset) + return 1; /* PATCH_TARGET_INVALID */ + + switch (mode) + { + case SOURCE_READ: + if (bps->output_offset + _len > bps->source_length) + return 2; /* PATCH_SOURCE_INVALID */ + while (_len--) + { + uint8_t data = bps->source_data[bps->output_offset]; + bps->target_data[bps->output_offset++] = data; + } + break; + + case TARGET_READ: + if (bps->modify_offset + _len > bps->modify_length - 12) + return 3; /* PATCH_PATCH_INVALID */ + while (_len--) + { + uint8_t data = bps_read(bps); + bps->target_data[bps->output_offset++] = data; + } + break; + + case SOURCE_COPY: + case TARGET_COPY: + { + int offset = (int)bps_decode(bps); + bool negative = offset & 1; + offset >>= 1; + if (negative) + offset = -offset; + + if (mode == SOURCE_COPY) + { + bps->source_offset += offset; + if ( bps->source_offset > bps->source_length + || _len > bps->source_length - bps->source_offset) + return 2; + while (_len--) + { + uint8_t data = bps->source_data[bps->source_offset++]; + bps->target_data[bps->output_offset++] = data; + } + } + else + { + bps->target_offset += offset; + if ( bps->target_offset > bps->target_length + || _len > bps->target_length - bps->target_offset) + return 1; + while (_len--) + { + uint8_t data = bps->target_data[bps->target_offset++]; + bps->target_data[bps->output_offset++] = data; + } + break; + } + break; + } + } + } + return 0; +} +/* === end verbatim copy === */ + +/* Helper: run the post-fix predicates against a patch+source+ + * target setup, with target_data heap-allocated to exactly + * the legitimate target_length so ASan flags any OOB write. */ +static int run_apply(const uint8_t *patch, size_t patch_len, + const uint8_t *src, size_t src_len, size_t target_len) +{ + struct mock_bps bps; + uint8_t *target; + int rv; + + if (!(target = (uint8_t*)malloc(target_len ? target_len : 1))) + return -1; + memset(target, 0, target_len ? target_len : 1); + + memset(&bps, 0, sizeof(bps)); + bps.modify_data = patch; + bps.source_data = src; + bps.target_data = target; + bps.modify_length = patch_len; + bps.source_length = src_len; + bps.target_length = target_len; + /* Skip the "BPS1" header + size prelude. The test + * harness sets modify_offset directly; in production the + * same offset is reached by the header read and three + * bps_decode calls before the action loop. */ + bps.modify_offset = 7; /* "BPS1" + 0x80, 0x80, 0x80 (zero sizes) */ + + rv = apply_action_loop(&bps); + + free(target); + return rv; +} + +/* Build a minimal valid header + 3 zero size declarations + * starting at out, returning the bytes written. */ +static size_t put_header(uint8_t *out) +{ + out[0] = 'B'; out[1] = 'P'; out[2] = 'S'; out[3] = '1'; + out[4] = 0x80; /* source size = 0 (single-byte: high bit set, value 0) */ + out[5] = 0x80; /* target size = 0 */ + out[6] = 0x80; /* markup size = 0 */ + return 7; +} + +/* ---- tests ------------------------------------------------ */ + +static void test_target_read_oob_write_rejected(void) +{ + /* Craft: TARGET_READ command with _len = 32 (encoded as 0xFD). + * Target buffer is 4 bytes. Pre-fix: 32 bytes written into a + * 4-byte allocation (heap-buffer-overflow WRITE). Post-fix: + * the top-of-loop bound rejects with PATCH_TARGET_INVALID. */ + uint8_t patch[64]; + size_t off = put_header(patch); + int i, rv; + patch[off++] = 0xFD; /* mode=1 (TARGET_READ), _len = (125>>2)+1 = 32 */ + for (i = 0; i < 32; i++) + patch[off++] = (uint8_t)(0xAA + i); + for (i = 0; i < 12; i++) + patch[off++] = 0; /* trailing checksums (12 bytes) */ + + rv = run_apply(patch, off, NULL, 0, 4); + if (rv == 0) + { + printf("[ERROR] TARGET_READ with _len=32 into target=4 was accepted\n"); + failures++; + } + else + printf("[SUCCESS] TARGET_READ over-length rejected without OOB write\n"); +} + +static void test_source_copy_oob_read_rejected(void) +{ + /* SOURCE_COPY with offset = 100 against a source of 8 bytes. + * After the offset, source_offset = 100 > source_length = 8 + * which the new bound rejects. Pre-fix this would have run + * source_data[100++] for _len iterations -- heap-buffer- + * overflow READ. */ + uint8_t patch[64], src[8] = {1,2,3,4,5,6,7,8}; + size_t off = put_header(patch); + int rv, i; + /* Encode: mode=SOURCE_COPY (2), _len=4. + * stored_len = ((4-1) << 2) | 2 = 14 = 0x0E. + * High bit not set, so it's not a single-byte encoding. + * For the test we want a single-byte command so just use + * 0x8E (14 + 0x80). bps_decode: x=0x8E, x&0x7F=0x0E, data=14, + * x&0x80=>break. So _len=4, mode=2. */ + patch[off++] = 0x8E; + /* offset = 200 (positive, so encoded value = 200<<1 = 400). + * 400 > 127 so multi-byte: low 7 bits = 400 & 0x7f = 16 (0x10), + * second byte = (400>>7)+something... easier to use a small + * positive offset directly. offset = 100 -> encoded = 200. + * 200 > 127 so 2-byte encoding: + * first byte: 200 & 0x7F = 72 = 0x48 (high bit clear) + * shift <<= 7 (=128), data += 128 => data = 72 + 128 = 200 + * ... wait that's the decoder, not the encoder. + * For decoder result 200: + * byte 1: x1 (high bit clear), data += x1 * 1 = x1, then shift=128, data += 128 + * byte 2: x2 (high bit set), data += (x2 & 0x7F) * 128 + * want data = 200 + * 200 = x1 + 128 + (x2 & 0x7F) * 128 + * = x1 + 128 * (1 + (x2 & 0x7F)) + * if x2 & 0x7F = 0, then 200 = x1 + 128, x1 = 72. + * So bytes: 0x48, 0x80 (x1=0x48, x2=0x80). Verify: + * data=0, shift=1; read 0x48, data += 0x48*1 = 72, no high bit, shift<<=7 (=128), data += 128 = 200, loop; + * read 0x80, data += 0 * 128 = 200, high bit => break. ✓ */ + patch[off++] = 0x48; + patch[off++] = 0x80; + /* trailing 12 bytes */ + for (i = 0; i < 12; i++) + patch[off++] = 0; + + rv = run_apply(patch, off, src, sizeof(src), 256); + if (rv == 0) + { + printf("[ERROR] SOURCE_COPY with offset=100 vs src=8 was accepted\n"); + failures++; + } + else + printf("[SUCCESS] SOURCE_COPY over-length rejected without OOB read\n"); +} + +static void test_legitimate_target_read_succeeds(void) +{ + /* TARGET_READ with _len=4 into target=4. */ + uint8_t patch[64]; + size_t off = put_header(patch); + int rv, i; + /* _len=4: stored = (3<<2)|1 = 13. Single byte 0x8D. */ + patch[off++] = 0x8D; + patch[off++] = 'A'; patch[off++] = 'B'; + patch[off++] = 'C'; patch[off++] = 'D'; + for (i = 0; i < 12; i++) + patch[off++] = 0; + + rv = run_apply(patch, off, NULL, 0, 4); + if (rv != 0) + { + printf("[ERROR] legitimate TARGET_READ rv=%d\n", rv); + failures++; + } + else + printf("[SUCCESS] legitimate TARGET_READ accepted\n"); +} + +static void test_legitimate_source_copy_succeeds(void) +{ + uint8_t patch[64], src[8] = {1,2,3,4,5,6,7,8}; + size_t off = put_header(patch); + int rv, i; + /* SOURCE_COPY mode=2, _len=4 -> stored = (3<<2)|2 = 14, byte 0x8E */ + patch[off++] = 0x8E; + /* offset = 0: encoded value = 0, single-byte 0x80 */ + patch[off++] = 0x80; + for (i = 0; i < 12; i++) + patch[off++] = 0; + + rv = run_apply(patch, off, src, sizeof(src), 8); + if (rv != 0) + { + printf("[ERROR] legitimate SOURCE_COPY rv=%d\n", rv); + failures++; + } + else + printf("[SUCCESS] legitimate SOURCE_COPY accepted\n"); +} + +static void test_target_read_at_exact_capacity(void) +{ + /* TARGET_READ with _len exactly equal to target_length. + * Should succeed (boundary). */ + uint8_t patch[64]; + size_t off = put_header(patch); + int rv, i; + /* _len=8: stored=(7<<2)|1=29, byte 0x9D */ + patch[off++] = 0x9D; + for (i = 0; i < 8; i++) + patch[off++] = 'X'; + for (i = 0; i < 12; i++) + patch[off++] = 0; + + rv = run_apply(patch, off, NULL, 0, 8); + if (rv != 0) + { + printf("[ERROR] boundary TARGET_READ (_len=target_length=8) rejected, rv=%d\n", rv); + failures++; + } + else + printf("[SUCCESS] boundary TARGET_READ (_len=target_length) accepted\n"); +} + +int main(void) +{ + test_target_read_oob_write_rejected(); + test_source_copy_oob_read_rejected(); + test_legitimate_target_read_succeeds(); + test_legitimate_source_copy_succeeds(); + test_target_read_at_exact_capacity(); + + if (failures) + { + printf("\n%d bps_patch_bounds test(s) failed\n", failures); + return 1; + } + printf("\nAll bps_patch_bounds regression tests passed.\n"); + return 0; +} diff --git a/steam/steam.c b/steam/steam.c index 689a3ea15fd8..d179031d87f8 100644 --- a/steam/steam.c +++ b/steam/steam.c @@ -446,82 +446,47 @@ void steam_update_presence(enum presence presence, bool force) } break; case STEAM_RICH_PRESENCE_FORMAT_CONTENT_SYSTEM: - _len = strlcpy(content, label, sizeof(content)); - content[_len ] = ' '; - content[_len+1] = '('; - content[_len+2] = '\0'; + _len = 0; + strlcpy_append(content, sizeof(content), &_len, label); + strlcpy_append(content, sizeof(content), &_len, " ("); if (core_info) { - _len += 2; - _len += strlcpy(content + _len, core_info->systemname, - sizeof(content) - _len); - content[_len ] = ')'; - content[_len+1] = '\0'; + strlcpy_append(content, sizeof(content), &_len, + core_info->systemname); + strlcpy_append(content, sizeof(content), &_len, ")"); } else - { - content[_len+2] = 'N'; - content[_len+3] = '/'; - content[_len+4] = 'A'; - content[_len+5] = ')'; - content[_len+6] = '\0'; - } + strlcpy_append(content, sizeof(content), &_len, "N/A)"); break; case STEAM_RICH_PRESENCE_FORMAT_CONTENT_CORE: - _len = strlcpy(content, label, sizeof(content)); - content[_len ] = ' '; - content[_len+1] = '('; - content[_len+2] = '\0'; + _len = 0; + strlcpy_append(content, sizeof(content), &_len, label); + strlcpy_append(content, sizeof(content), &_len, " ("); if (core_info) { - _len += 2; - _len += strlcpy(content + _len, core_info->core_name, - sizeof(content) - _len); - content[_len ] = ')'; - content[_len+1] = '\0'; + strlcpy_append(content, sizeof(content), &_len, + core_info->core_name); + strlcpy_append(content, sizeof(content), &_len, ")"); } else - { - content[_len+2] = 'N'; - content[_len+3] = '/'; - content[_len+4] = 'A'; - content[_len+5] = ')'; - content[_len+6] = '\0'; - } + strlcpy_append(content, sizeof(content), &_len, "N/A)"); break; case STEAM_RICH_PRESENCE_FORMAT_CONTENT_SYSTEM_CORE: - _len = strlcpy(content, label, sizeof(content)); - content[_len ] = ' '; - content[_len+1] = '('; - content[_len+2] = '\0'; + _len = 0; + strlcpy_append(content, sizeof(content), &_len, label); + strlcpy_append(content, sizeof(content), &_len, " ("); if (core_info) { - _len += 2; - _len += strlcpy(content + _len, core_info->systemname, - sizeof(content) - _len); - content[_len ] = ' '; - content[_len+1] = '-'; - content[_len+2] = ' '; - _len += 3; - _len += strlcpy(content + _len, core_info->core_name, - sizeof(content) - _len); - content[_len ] = ')'; - content[_len+1] = '\0'; + strlcpy_append(content, sizeof(content), &_len, + core_info->systemname); + strlcpy_append(content, sizeof(content), &_len, " - "); + strlcpy_append(content, sizeof(content), &_len, + core_info->core_name); + strlcpy_append(content, sizeof(content), &_len, ")"); } else - { - content[_len+2] = 'N'; - content[_len+3] = '/'; - content[_len+4] = 'A'; - content[_len+5] = ' '; - content[_len+6] = '-'; - content[_len+7] = ' '; - content[_len+8] = 'N'; - content[_len+9] = '/'; - content[_len+10] = 'A'; - content[_len+11] = ')'; - content[_len+12] = '\0'; - } + strlcpy_append(content, sizeof(content), &_len, + "N/A - N/A)"); break; case STEAM_RICH_PRESENCE_FORMAT_NONE: default: diff --git a/tasks/task_http.c b/tasks/task_http.c index a75b2bdaa2d1..713b99a05795 100644 --- a/tasks/task_http.c +++ b/tasks/task_http.c @@ -491,17 +491,16 @@ void* task_push_http_transfer_file(const char* url, bool mute, s = transfer_data->path; else s = url; - - _len = strlcpy(tmp, msg_hash_to_str(MSG_DOWNLOADING), sizeof(tmp)); - tmp[ _len] = ':'; - tmp[++_len] = ' '; - tmp[++_len] = '\0'; + _len = 0; + strlcpy_append(tmp, sizeof(tmp), &_len, + msg_hash_to_str(MSG_DOWNLOADING)); + strlcpy_append(tmp, sizeof(tmp), &_len, ": "); if (string_ends_with_size(s, ".index", strlen(s), STRLEN_CONST(".index"))) s = msg_hash_to_str(MSG_INDEX_FILE); - strlcpy(tmp + _len, s, sizeof(tmp) - _len); + strlcpy_append(tmp, sizeof(tmp), &_len, s); /* should be using type but some callers now rely on type being ignored */ return task_push_http_transfer_generic_titled( diff --git a/tasks/task_http_emscripten.c b/tasks/task_http_emscripten.c index 365c9fae267e..ad3a070e3001 100644 --- a/tasks/task_http_emscripten.c +++ b/tasks/task_http_emscripten.c @@ -308,16 +308,16 @@ void* task_push_http_transfer_file(const char* url, bool mute, s = transfer_data->path; else s = url; - - _len = strlcpy(tmp, msg_hash_to_str(MSG_DOWNLOADING), sizeof(tmp)); - tmp[ _len] = ' '; - tmp[++_len] = '\0'; + _len = 0; + strlcpy_append(tmp, sizeof(tmp), &_len, + msg_hash_to_str(MSG_DOWNLOADING)); + strlcpy_append(tmp, sizeof(tmp), &_len, " "); if (string_ends_with_size(s, ".index", strlen(s), STRLEN_CONST(".index"))) s = msg_hash_to_str(MSG_INDEX_FILE); - strlcpy(tmp + _len, s, sizeof(tmp) - _len); + strlcpy_append(tmp, sizeof(tmp), &_len, s); t->title = strdup(tmp); return t; diff --git a/tasks/task_patch.c b/tasks/task_patch.c index f98695a6389e..be785748ecdd 100644 --- a/tasks/task_patch.c +++ b/tasks/task_patch.c @@ -168,9 +168,38 @@ static enum patch_error bps_apply_patch( || (bps_read(&bps) != '1')) return PATCH_PATCH_INVALID_HEADER; - modify_source_size = bps_decode(&bps); - modify_target_size = bps_decode(&bps); - modify_markup_size = bps_decode(&bps); + { + uint64_t raw_source = bps_decode(&bps); + uint64_t raw_target = bps_decode(&bps); + uint64_t raw_markup = bps_decode(&bps); + + /* All three sizes are decoded from attacker-controlled + * variable-length integers in the patch file with no + * inherent upper bound (each can reach UINT64_MAX). + * Three separate sanity checks are required before they + * are assigned into size_t locals, where (on 32-bit + * targets) values above SIZE_MAX would silently truncate: + * - markup_size is consumed via bps_read below; if it + * exceeds the remaining bytes in modify_data, those + * reads run off the end of the patch buffer. + * - target_size on a 32-bit host would truncate at the + * assignment, producing a smaller-than-expected + * allocation while bps.target_length stays at the raw + * uint64. Subsequent target_data[output_offset++] + * writes OOB by the truncated delta. Reject anything + * that won't fit in size_t outright; malloc would have + * failed anyway. */ + if (raw_markup > modify_length - bps.modify_offset) + return PATCH_PATCH_INVALID; + if (raw_target > (uint64_t)SIZE_MAX) + return PATCH_TARGET_ALLOC_FAILED; + if (raw_source > (uint64_t)SIZE_MAX) + return PATCH_SOURCE_TOO_SMALL; + + modify_source_size = (size_t)raw_source; + modify_target_size = (size_t)raw_target; + modify_markup_size = (size_t)raw_markup; + } for (i = 0; i < modify_markup_size; i++) bps_read(&bps); @@ -198,9 +227,27 @@ static enum patch_error bps_apply_patch( _len = (_len >> 2) + 1; + /* Every action mode below performs target_data[output_ + * offset++] = ... up to _len times. Pre-this-patch + * none of the modes checked output_offset against + * bps.target_length, so a malicious .bps could write + * arbitrary attacker-chosen bytes past the malloc'd + * target_data buffer (heap-buffer-overflow WRITE). + * Bound _len against the remaining target capacity + * once at the top so each mode below stays in + * bounds. */ + if ( bps.output_offset >= bps.target_length + || _len > bps.target_length - bps.output_offset) + return PATCH_TARGET_INVALID; + switch (mode) { case SOURCE_READ: + /* SOURCE_READ reads from source_data[output_offset] + * for the same output_offset range, so the same + * bound applies to source_data too. */ + if (bps.output_offset + _len > bps.source_length) + return PATCH_SOURCE_INVALID; while (_len--) { uint8_t data = bps.source_data[bps.output_offset]; @@ -210,6 +257,14 @@ static enum patch_error bps_apply_patch( break; case TARGET_READ: + /* TARGET_READ reads _len bytes from modify_data via + * bps_read. The action loop's outer guard only + * confirms there is at least one command's worth + * of bytes (modify_offset < modify_length - 12); + * a long _len plus the 12-byte trailer can read + * past modify_length. */ + if (bps.modify_offset + _len > bps.modify_length - 12) + return PATCH_PATCH_INVALID; while (_len--) { uint8_t data = bps_read(&bps); @@ -232,6 +287,19 @@ static enum patch_error bps_apply_patch( if (mode == SOURCE_COPY) { bps.source_offset += offset; + /* Validate the resulting source_offset and the + * full read range against source_length. Pre- + * this-patch a malicious offset could push + * source_offset past source_length (or, with a + * negative offset, underflow it to a huge size_t), + * driving source_data[source_offset++] reads + * arbitrarily far OOB. An attacker who can read + * past source_data into adjacent heap leaks heap + * contents into target_data and the checksum + * calculation. */ + if ( bps.source_offset > bps.source_length + || _len > bps.source_length - bps.source_offset) + return PATCH_SOURCE_INVALID; while (_len--) { uint8_t data = bps.source_data[bps.source_offset++]; @@ -242,6 +310,13 @@ static enum patch_error bps_apply_patch( else { bps.target_offset += offset; + /* TARGET_COPY both reads from and writes to + * target_data; bound the read pointer the same + * way as SOURCE_COPY above. output_offset is + * already bounded by the top-of-loop check. */ + if ( bps.target_offset > bps.target_length + || _len > bps.target_length - bps.target_offset) + return PATCH_TARGET_INVALID; while (_len--) { uint8_t data = bps.target_data[bps.target_offset++];