Bug #2161
closedHeap buffer overflow in worklist_load() via unbounded wl_length from save file -- overwrites struct city fields including pointers (CVSS 8.8)
0%
Description
Reported by Tristan.
========================================================================
Heap Buffer Overflow in worklist_load() via Crafted Save File
========================================================================
CVSS 3.1: 8.8 (AV:L/AC:L/PR:N/UI:R/S:C/C:H/I:H/A:H)
CWE: CWE-122 (Heap-based Buffer Overflow)
Auth: None (open save file)
Version: commit 815c1f4 (master, 2026-07-24)
File: server/savegame/savegame3.c, worklist_load(), lines 966-1006
server/savegame/savegame2.c, worklist_load(), lines 790-830
Root Cause:
worklist_load() reads wl_length from the save file and uses it as
a loop bound without validating against MAX_LEN_WORKLIST (64). The
loop writes struct universal entries directly to pwl->entries[i],
overflowing when i >= 64.
pwl->length = secfile_lookup_int_default(file, 0, // line 982
"%s.wl_length", path_str);
// NO check: pwl->length <= MAX_LEN_WORKLISTfor (i = 0; i < pwl->length; i++) { // line 985
kind = secfile_lookup_str(file, "%s.wl_kind%d", path_str, i);
name = secfile_lookup_str_default(file, "-", "%s.wl_value%d",
path_str, i);
pwl->entries[i] = universal_by_rule_name(kind, name); // line
992: OOB write
}The wlist_max_length parameter is only used for a padding loop
(line 1002), NOT to cap pwl->length.
Heap Layout (struct city, common/city.h:399+):
struct worklist worklist; // entries64 -- THE OVERFLOW SOURCE
bv_city_options city_options; // overwritten first
enum city_wl_cancel_behavior wlcb;
struct unit_list *units_supported; // POINTER -- corruptible for RCE
int *counter_values; // POINTER -- corruptible
When wl_length > 64, struct universal values (enum kind + union
value with pointers) overwrite city_options, wlcb, units_supported,
and counter_values. Corrupting units_supported leads to controlled
dereference when the server iterates supported units.
Contrast with safe pattern:
worklist_append() in common/worklist.c:147 correctly checks
next_index >= MAX_LEN_WORKLIST. But worklist_load() bypasses
worklist_append() entirely.
Impact:
Heap corruption via crafted save file. A save file with
wl_length=200 overwrites 136 entries past the buffer, corrupting
pointers. Arbitrary code execution via controlled pointer
dereference. Affects single-player and multiplayer (shared saves).
Both savegame v2 and v3 formats affected. All platforms.
Files
Updated by Marko Lindqvist 12 days ago
- File 0053-savegame-Fix-Heap-Buffer-Overflow-in-worklist_load.patch 0053-savegame-Fix-Heap-Buffer-Overflow-in-worklist_load.patch added
- Status changed from New to In Review
- Assignee set to Marko Lindqvist
- Target version set to 3.2.6
Updated by Marko Lindqvist 11 days ago
- File 0005-savegame-Fix-Heap-Buffer-Overflow-in-worklist_load.patch 0005-savegame-Fix-Heap-Buffer-Overflow-in-worklist_load.patch added
- Patch for S3_1
Updated by Marko Lindqvist 11 days ago
- File 0002-savegame-Fix-Heap-Buffer-Overflow-in-worklist_load.patch 0002-savegame-Fix-Heap-Buffer-Overflow-in-worklist_load.patch added
Apply also to S3_0 and S2_6. Separate patch for S2_6.
Updated by Alina Lenk 10 days ago
For all branches except S2_6: AIUI the wlist_max_length is just for reading/ignoring all the remaining (empty) entries in the table for worklists that are shorter than the longest one. So, limiting it to MAX_LEN_WORKLIST can produce a lot of unused entry warnings.
For avoiding the vulnerability, we can limit the worklist length directly to MAX_LEN_WORKLIST as in S2_6; this would also make the code more obviously correct. We can leave wlist_max_length uncapped.
Of course, if a worklist is longer than (uncapped) wlist_max_length that might still be cause for a warning? That can only happen in main, S3_3 and S3_2 where wlist_max_length is read from the file; it shouldn't happen in S3_1 and S3_0 where it's computed as the maximum of the individual lengths.
I don't consider that a blocking issue if we want to get this out ASAP. Additional warnings on an already incorrect save file are barely a problem, but obvious correctness could be worth it.
Aside from that, it all looks good.
Updated by Marko Lindqvist 10 days ago
Hmm... Should it be "if (pwl->length > MAX_LEN_WORKLIST) { log error; pwl->length = MAX_LEN_WORKLIST; } else if (pwl->length > wlist_max_length) { log different error; }" ?
Updated by Alina Lenk 10 days ago
Could even drop the else; just two separate ifs, for the case where wlist_max_length is itself too big. Though they'd have to be the other way 'round since one of them changes the length. Of course, then you'd get two errors for a single overlong worklist in an otherwise intact save file... Probably fine either way.
Updated by Marko Lindqvist 10 days ago
Now you start to sound like you'd want the semantics of my original patch. ;-)
Updated by Alina Lenk 10 days ago
Well no; the original patch caps wlist_max_length at MAX_LEN_WORKLIST, so the case where it's still greater than that by the time we load a worklist would be irrelevant :P
But ultimately, the original patch does look correct; it just produces more unused entry warnings in the hypothetical case where someone recompiles the game with an increased MAX_LEN_WORKLIST, makes an overlong worklist, and then tries to load that in the regular version. So we are, as ever, just doing perfectionism ^^
Updated by Marko Lindqvist 10 days ago
Anyway, I think it's enough to get just (1) "over MAX_WORKLIST_LENGTH" error also when there are also (2) "over wlist_max_length" AND (3) "wlist_max_length over MAX_WORKLIST_LENGTH" error situations simultaneously. It's not uncommon that one parsing error hides another. You know that things are going wrong from one error message already.
Updated by Alina Lenk 10 days ago
Makes sense. +1 for the else if option, then.
Updated by Marko Lindqvist 10 days ago
- File 0002-savegame-Fix-Heap-Buffer-Overflow-in-worklist_load.patch 0002-savegame-Fix-Heap-Buffer-Overflow-in-worklist_load.patch added
- Status changed from In Review to In Progress
Updated by Marko Lindqvist 10 days ago
- File 0046-savegame-Fix-Heap-Buffer-Overflow-in-worklist_load.patch 0046-savegame-Fix-Heap-Buffer-Overflow-in-worklist_load.patch added
- Status changed from In Progress to In Review
Next iteration of patches ready for review.