Subject: fix for vulnerability CVE-2011-3342 for OpenTTD 0.5.0 - 0.5.3 (Buffer overflows in savegame loading)
From: OpenTTD developer team <info@openttd.org>
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;