Subject: fix for vulnerability CVE-2011-3342 for OpenTTD 0.5.0 - 0.5.3 (Buffer overflows in savegame loading) From: OpenTTD developer team Origin: backport, https://github.com/OpenTTD/OpenTTD/commit/81074e0 https://github.com/OpenTTD/OpenTTD/commit/ef09794 Bug: https://github.com/OpenTTD/OpenTTD/issues/4748 https://github.com/OpenTTD/OpenTTD/issues/4717 In multiple places indices in savegames are not properly validated that allow (remote) attackers to cause a denial of service (crash) and possibly execute arbitrary code via unspecified vectors. The bug is exploitable by passing someone a modified savegame, be it via a file sharing site, or by running a server. In case of the server the user only has to be able to login into the server, which is easy to accomplish: set no server password and do not ban anyone. Then upon joining the server the savegame will be downloaded and subsequently loaded. Note that versions before 0.5.0 are vulnerable as well. However, these versions are over five years old and not supported anymore. Therefore no patches for earlier versions are provided. Before 0.3.5 it is not possible to exploit this bug via the internet as multiplayer over internet did not exist yet. Index: players.c =================================================================== --- players.c (revision 22703) +++ players.c (working copy) @@ -1298,6 +1298,7 @@ SlObject(&p->cur_economy, _player_economy_desc); // Write old economy entries. + if (p->num_valid_stat_ent > lengthof(p->old_economy)) SlErrorCorrupt("Too many old economy entries"); for (i = 0; i < p->num_valid_stat_ent; i++) { SlObject(&p->old_economy[i], _player_economy_desc); } Index: misc.c =================================================================== --- misc.c (revision 22703) +++ misc.c (working copy) @@ -299,7 +299,12 @@ int index; while ((index = SlIterateArray()) != -1) { + if (index >= 512) SlErrorCorrupt("Invalid old name index"); + if (SlGetFieldLength() > 32) SlErrorCorrupt("Invalid old name length"); + SlArray(_name_array[index],SlGetFieldLength(),SLE_UINT8); + /* Make sure the name is null terminated */ + _name_array[index][31] = '\0'; } } @@ -606,6 +611,8 @@ Cheat* cht = (Cheat*)&_cheats; uint count = SlGetFieldLength() / 2; uint i; + /* Cannot use lengthof because _cheats is of type Cheats, not Cheat */ + if (count > sizeof(_cheats) / sizeof(Cheat)) SlErrorCorrupt("Too many cheat values"); for (i = 0; i < count; i++) { cht[i].been_used = SlReadByte(); Index: saveload.c =================================================================== --- saveload.c (revision 22703) +++ saveload.c (working copy) @@ -143,6 +143,11 @@ longjmp(_sl.excpt, 0); } +void SlErrorCorrupt(const char *message) +{ + SlError(message); +} + /** Read in a single byte from file. If the temporary buffer is full, * flush it to its final destination * @return return the read byte from file Index: saveload.h =================================================================== --- saveload.h (revision 22703) +++ saveload.h (working copy) @@ -33,6 +33,7 @@ uint32 flags; } ChunkHandler; +NORETURN void SlErrorCorrupt(const char *message); typedef struct { byte null; } NullStruct;