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 about 1 month 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 about 1 month 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 about 1 month 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 about 1 month 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 about 1 month 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 about 1 month 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 about 1 month ago
Now you start to sound like you'd want the semantics of my original patch. ;-)
Updated by Alina Lenk about 1 month 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 about 1 month 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 about 1 month ago
Makes sense. +1 for the else if option, then.
Updated by Marko Lindqvist about 1 month 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 about 1 month 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.
Updated by Marko Lindqvist about 1 month ago
- Blocks Task #2165: Release 3.2.6 added
Updated by Marko Lindqvist about 1 month ago
- Blocks Task #2116: S3_3-alpha3 added