(svn r2308) - Fix: enforce server-only and/or offline commands by giving them flags in the process table. This also fixes bug "[ 1190944 ] Many commands not checked for security"

- CodeChange: move ValParamRailtype() to check rail type from command.h to vehicle.h where it is better suited.

6 files changed, 241 insertions, 204 deletions

diff --git a/command.c b/command.c
index 7eae3c4fd..7d9e2460b 100644
--- a/command.c
+++ b/command.c
@@ -154,146 +154,146 @@ DEF_COMMAND(CmdRemoveSignalTrack);
 DEF_COMMAND(CmdReplaceVehicle);
 
 /* The master command table */
-static CommandProc * const _command_proc_table[] = {
-	CmdBuildRailroadTrack,				/* 0  */
-	CmdRemoveRailroadTrack,				/* 1  */
-	CmdBuildSingleRail,						/* 2  */
-	CmdRemoveSingleRail,					/* 3  */
-	CmdLandscapeClear,						/* 4  */
-	CmdBuildBridge,								/* 5  */
-	CmdBuildRailroadStation,			/* 6  */
-	CmdBuildTrainDepot,						/* 7  */
-	CmdBuildSingleSignal,					/* 8  */
-	CmdRemoveSingleSignal,				/* 9  */
-	CmdTerraformLand,							/* 10 */
-	CmdPurchaseLandArea,					/* 11 */
-	CmdSellLandArea,							/* 12 */
-	CmdBuildTunnel,								/* 13 */
-	CmdRemoveFromRailroadStation,	/* 14 */
-	CmdConvertRail,								/* 15 */
-	CmdBuildTrainWaypoint,				/* 16 */
-	CmdRenameWaypoint,						/* 17 */
-	CmdRemoveTrainWaypoint,				/* 18 */
-	NULL,                         /* 19 */
-	NULL,													/* 20 */
-	CmdBuildRoadStop,							/* 21 */
-	NULL,													/* 22 */
-	CmdBuildLongRoad,							/* 23 */
-	CmdRemoveLongRoad,						/* 24 */
-	CmdBuildRoad,									/* 25 */
-	CmdRemoveRoad,								/* 26 */
-	CmdBuildRoadDepot,						/* 27 */
-	NULL,													/* 28 */
-	CmdBuildAirport,							/* 29 */
-	CmdBuildDock,									/* 30 */
-	CmdBuildShipDepot,						/* 31 */
-	CmdBuildBuoy,									/* 32 */
-	CmdPlantTree,									/* 33 */
-	CmdBuildRailVehicle,					/* 34 */
-	CmdMoveRailVehicle,						/* 35 */
-	CmdStartStopTrain,						/* 36 */
-	NULL,													/* 37 */
-	CmdSellRailWagon,							/* 38 */
-	CmdSendTrainToDepot,		  	  /* 39 */
-	CmdForceTrainProceed,					/* 40 */
-	CmdReverseTrainDirection,			/* 41 */
-
-	CmdModifyOrder,								/* 42 */
-	CmdSkipOrder,									/* 43 */
-	CmdDeleteOrder,								/* 44 */
-	CmdInsertOrder,								/* 45 */
-
-	CmdChangeTrainServiceInt,			/* 46 */
-
-	CmdBuildIndustry,							/* 47 */
-	CmdBuildCompanyHQ,						/* 48 */
-	CmdSetPlayerFace,							/* 49 */
-	CmdSetPlayerColor,						/* 50 */
-
-	CmdIncreaseLoan,							/* 51 */
-	CmdDecreaseLoan,							/* 52 */
-
-	CmdWantEnginePreview,					/* 53 */
-
-	CmdNameVehicle,								/* 54 */
-	CmdRenameEngine,							/* 55 */
-
-	CmdChangeCompanyName,					/* 56 */
-	CmdChangePresidentName,				/* 57 */
-
-	CmdRenameStation,							/* 58 */
-
-	CmdSellAircraft,							/* 59 */
-	CmdStartStopAircraft,					/* 60 */
-
-	CmdBuildAircraft,							/* 61 */
-	CmdSendAircraftToHangar,			/* 62 */
-	CmdChangeAircraftServiceInt,	/* 63 */
-	CmdRefitAircraft,							/* 64 */
-
-	CmdPlaceSign,									/* 65 */
-	CmdRenameSign,								/* 66 */
-
-	CmdBuildRoadVeh,							/* 67 */
-	CmdStartStopRoadVeh,					/* 68 */
-	CmdSellRoadVeh,								/* 69 */
-	CmdSendRoadVehToDepot,				/* 70 */
-	CmdTurnRoadVeh,								/* 71 */
-	CmdChangeRoadVehServiceInt,		/* 72 */
-
-	CmdPause,											/* 73 <-- TODO: check/enforce by server */
-
-	CmdBuyShareInCompany,					/* 74 */
-	CmdSellShareInCompany,				/* 75 */
-	CmdBuyCompany,								/* 76 */
-
-	CmdBuildTown,									/* 77 <-- offline */
-	NULL,													/* 78 */
-	NULL,													/* 79 */
-	CmdRenameTown,								/* 80 <-- TODO: check/enforce by server */
-	CmdDoTownAction,							/* 81 */
-
-	CmdSetRoadDriveSide,					/* 82 <-- TODO: check/enforce by server */
-	NULL,              						/* 83 */
-	NULL,													/* 84 */
-	CmdChangeDifficultyLevel,			/* 85 <-- TODO: check/enforce by server */
-
-	CmdStartStopShip,							/* 86 */
-	CmdSellShip,									/* 87 */
-	CmdBuildShip,									/* 88 */
-	CmdSendShipToDepot,						/* 89 */
-	CmdChangeShipServiceInt,			/* 90 */
-	CmdRefitShip,									/* 91 */
-
-	NULL,            							/* 92 */
-	NULL,                         /* 93 */
-	NULL,             						/* 94 */
-	NULL,                         /* 95 */
-	NULL,													/* 96 */
-	NULL,                 				/* 97 */
-	NULL,                					/* 98 */
-
-	CmdCloneOrder,								/* 99 */
-
-	CmdClearArea,									/* 100 */
-	NULL,                         /* 101 */
-
-	CmdMoneyCheat,								/* 102 <-- offline (debug) */
-	CmdBuildCanal,								/* 103 */
-	CmdPlayerCtrl,								/* 104 */
-
-	CmdLevelLand,									/* 105 */
-
-	CmdRefitRailVehicle,					/* 106 */
-	CmdRestoreOrderIndex,					/* 107 */
-	CmdBuildLock,									/* 108 */
-	NULL,           							/* 109 */
-	CmdBuildSignalTrack,					/* 110 */
-	CmdRemoveSignalTrack,					/* 111 */
-	NULL,               					/* 112 */
-	CmdGiveMoney,									/* 113 */
-	CmdChangePatchSetting,				/* 114 <-- TODO: check/enforce by server */
-	CmdReplaceVehicle,						/* 115 */
+static const Command _command_proc_table[] = {
+	{CmdBuildRailroadTrack,                  0}, /*   0 */
+	{CmdRemoveRailroadTrack,                 0}, /*   1 */
+	{CmdBuildSingleRail,                     0}, /*   2 */
+	{CmdRemoveSingleRail,                    0}, /*   3 */
+	{CmdLandscapeClear,                      0}, /*   4 */
+	{CmdBuildBridge,                         0}, /*   5 */
+	{CmdBuildRailroadStation,                0}, /*   6 */
+	{CmdBuildTrainDepot,                     0}, /*   7 */
+	{CmdBuildSingleSignal,                   0}, /*   8 */
+	{CmdRemoveSingleSignal,                  0}, /*   9 */
+	{CmdTerraformLand,                       0}, /*  10 */
+	{CmdPurchaseLandArea,                    0}, /*  11 */
+	{CmdSellLandArea,                        0}, /*  12 */
+	{CmdBuildTunnel,                         0}, /*  13 */
+	{CmdRemoveFromRailroadStation,           0}, /*  14 */
+	{CmdConvertRail,                         0}, /*  15 */
+	{CmdBuildTrainWaypoint,                  0}, /*  16 */
+	{CmdRenameWaypoint,                      0}, /*  17 */
+	{CmdRemoveTrainWaypoint,                 0}, /*  18 */
+	{NULL,                                   0}, /*  19 */
+	{NULL,                                   0}, /*  20 */
+	{CmdBuildRoadStop,                       0}, /*  21 */
+	{NULL,                                   0}, /*  22 */
+	{CmdBuildLongRoad,                       0}, /*  23 */
+	{CmdRemoveLongRoad,                      0}, /*  24 */
+	{CmdBuildRoad,                           0}, /*  25 */
+	{CmdRemoveRoad,                          0}, /*  26 */
+	{CmdBuildRoadDepot,                      0}, /*  27 */
+	{NULL,                                   0}, /*  28 */
+	{CmdBuildAirport,                        0}, /*  29 */
+	{CmdBuildDock,                           0}, /*  30 */
+	{CmdBuildShipDepot,                      0}, /*  31 */
+	{CmdBuildBuoy,                           0}, /*  32 */
+	{CmdPlantTree,                           0}, /*  33 */
+	{CmdBuildRailVehicle,                    0}, /*  34 */
+	{CmdMoveRailVehicle,                     0}, /*  35 */
+	{CmdStartStopTrain,                      0}, /*  36 */
+	{NULL,                                   0}, /*  37 */
+	{CmdSellRailWagon,                       0}, /*  38 */
+	{CmdSendTrainToDepot,                    0}, /*  39 */
+	{CmdForceTrainProceed,                   0}, /*  40 */
+	{CmdReverseTrainDirection,               0}, /*  41 */
+
+	{CmdModifyOrder,                         0}, /*  42 */
+	{CmdSkipOrder,                           0}, /*  43 */
+	{CmdDeleteOrder,                         0}, /*  44 */
+	{CmdInsertOrder,                         0}, /*  45 */
+
+	{CmdChangeTrainServiceInt,               0}, /*  46 */
+
+	{CmdBuildIndustry,                       0}, /*  47 */
+	{CmdBuildCompanyHQ,                      0}, /*  48 */
+	{CmdSetPlayerFace,                       0}, /*  49 */
+	{CmdSetPlayerColor,                      0}, /*  50 */
+
+	{CmdIncreaseLoan,                        0}, /*  51 */
+	{CmdDecreaseLoan,                        0}, /*  52 */
+
+	{CmdWantEnginePreview,                   0}, /*  53 */
+
+	{CmdNameVehicle,                         0}, /*  54 */
+	{CmdRenameEngine,                        0}, /*  55 */
+
+	{CmdChangeCompanyName,                   0}, /*  56 */
+	{CmdChangePresidentName,                 0}, /*  57 */
+
+	{CmdRenameStation,                       0}, /*  58 */
+
+	{CmdSellAircraft,                        0}, /*  59 */
+	{CmdStartStopAircraft,                   0}, /*  60 */
+
+	{CmdBuildAircraft,                       0}, /*  61 */
+	{CmdSendAircraftToHangar,                0}, /*  62 */
+	{CmdChangeAircraftServiceInt,            0}, /*  63 */
+	{CmdRefitAircraft,                       0}, /*  64 */
+
+	{CmdPlaceSign,                           0}, /*  65 */
+	{CmdRenameSign,                          0}, /*  66 */
+
+	{CmdBuildRoadVeh,                        0}, /*  67 */
+	{CmdStartStopRoadVeh,                    0}, /*  68 */
+	{CmdSellRoadVeh,                         0}, /*  69 */
+	{CmdSendRoadVehToDepot,                  0}, /*  70 */
+	{CmdTurnRoadVeh,                         0}, /*  71 */
+	{CmdChangeRoadVehServiceInt,             0}, /*  72 */
+
+	{CmdPause,                      CMD_SERVER}, /*  73 */
+
+	{CmdBuyShareInCompany,                   0}, /*  74 */
+	{CmdSellShareInCompany,                  0}, /*  75 */
+	{CmdBuyCompany,                          0}, /*  76 */
+
+	{CmdBuildTown,                 CMD_OFFLINE}, /*  77 */
+	{NULL,                                   0}, /*  78 */
+	{NULL,                                   0}, /*  79 */
+	{CmdRenameTown,                 CMD_SERVER}, /*  80 */
+	{CmdDoTownAction,                        0}, /*  81 */
+
+	{CmdSetRoadDriveSide,           CMD_SERVER}, /*  82 */
+	{NULL,                                   0}, /*  83 */
+	{NULL,                                   0}, /*  84 */
+	{CmdChangeDifficultyLevel,      CMD_SERVER}, /*  85 */
+
+	{CmdStartStopShip,                       0}, /*  86 */
+	{CmdSellShip,                            0}, /*  87 */
+	{CmdBuildShip,                           0}, /*  88 */
+	{CmdSendShipToDepot,                     0}, /*  89 */
+	{CmdChangeShipServiceInt,                0}, /*  90 */
+	{CmdRefitShip,                           0}, /*  91 */
+
+	{NULL,                                   0}, /*  92 */
+	{NULL,                                   0}, /*  93 */
+	{NULL,                                   0}, /*  94 */
+	{NULL,                                   0}, /*  95 */
+	{NULL,                                   0}, /*  96 */
+	{NULL,                                   0}, /*  97 */
+	{NULL,                                   0}, /*  98 */
+
+	{CmdCloneOrder,                          0}, /*  99 */
+
+	{CmdClearArea,                           0}, /* 100 */
+	{NULL,                                   0}, /* 101 */
+
+	{CmdMoneyCheat,                CMD_OFFLINE}, /* 102 */
+	{CmdBuildCanal,                          0}, /* 103 */
+	{CmdPlayerCtrl,                          0}, /* 104 */
+
+	{CmdLevelLand,                           0}, /* 105 */
+
+	{CmdRefitRailVehicle,                    0}, /* 106 */
+	{CmdRestoreOrderIndex,                   0}, /* 107 */
+	{CmdBuildLock,                           0}, /* 108 */
+	{NULL,                                   0}, /* 109 */
+	{CmdBuildSignalTrack,                    0}, /* 110 */
+	{CmdRemoveSignalTrack,                   0}, /* 111 */
+	{NULL,                                   0}, /* 112 */
+	{CmdGiveMoney,                           0}, /* 113 */
+	{CmdChangePatchSetting,         CMD_SERVER}, /* 114 */
+	{CmdReplaceVehicle,                      0}, /* 115 */
 };
 
 /* This function range-checks a cmd, and checks if the cmd is not NULL */
@@ -301,12 +301,14 @@ bool IsValidCommand(uint cmd)
 {
 	cmd = cmd & 0xFF;
 
-	if (cmd >= lengthof(_command_proc_table) || _command_proc_table[cmd] == NULL)
+	if (cmd >= lengthof(_command_proc_table) || _command_proc_table[cmd].proc == NULL)
 		return false;
 
 	return true;
 }
 
+byte GetCommandFlags(uint cmd) {return _command_proc_table[cmd & 0xFF].flags;}
+
 int32 DoCommandByTile(TileIndex tile, uint32 p1, uint32 p2, uint32 flags, uint procc)
 {
 	return DoCommand(TileX(tile) * 16, TileY(tile) * 16, p1, p2, flags, procc);
@@ -323,7 +325,7 @@ int32 DoCommand(int x, int y, uint32 p1, uint32 p2, uint32 flags, uint procc)
 	/* Do not even think about executing out-of-bounds tile-commands */
 	if (TILE_FROM_XY(x,y) > MapSize()) return CMD_ERROR;
 
-	proc = _command_proc_table[procc];
+	proc = _command_proc_table[procc].proc;
 
 	if (_docommand_recursive == 0) {
 		_error_message = INVALID_STRING_ID;
@@ -412,7 +414,7 @@ bool DoCommandP(TileIndex tile, uint32 p1, uint32 p2, CommandCallback *callback,
 
 	// get pointer to command handler
 	assert((cmd & 0xFF) < lengthof(_command_proc_table));
-	proc = _command_proc_table[cmd & 0xFF];
+	proc = _command_proc_table[cmd & 0xFF].proc;
 
 	// Some commands have a different output in dryrun than the realrun
 	//  e.g.: if you demolish a whole town, the dryrun would say okay.
diff --git a/command.h b/command.h
index 2a44b9176..8bb649d28 100644
--- a/command.h
+++ b/command.h
@@ -159,6 +159,19 @@ enum {
 	CMD_SHOW_NO_ERROR = 0x2000,
 };
 
+/** Command flags for the command table
+ * @see _command_proc_table
+ */
+enum {
+	CMD_SERVER  = 0x1, /// the command can only be initiated by the server
+	CMD_OFFLINE = 0x2, /// the command cannot be executed in a multiplayer game; single-player only
+};
+
+typedef struct Command {
+	CommandProc *proc;
+	byte flags;
+} Command;
+
 //#define return_cmd_error(errcode) do { _error_message=(errcode); return CMD_ERROR; } while(0)
 #define return_cmd_error(errcode) do { return CMD_ERROR | (errcode); } while (0)
 
@@ -178,8 +191,7 @@ int32 DoCommand(int x, int y, uint32 p1, uint32 p2, uint32 flags, uint procc);
 int32 DoCommandByTile(TileIndex tile, uint32 p1, uint32 p2, uint32 flags, uint procc);
 
 bool IsValidCommand(uint cmd);
+byte GetCommandFlags(uint cmd);
 int32 GetAvailableMoneyForCommand(void);
 
-/* Validate functions for rail building */
-static inline bool ValParamRailtype(uint32 rail) { return rail <= GetPlayer(_current_player)->max_railtype;}
 #endif /* COMMAND_H */
diff --git a/network.h b/network.h
index 87bdd6306..752cb6945 100644
--- a/network.h
+++ b/network.h
@@ -92,13 +92,13 @@ typedef struct NetworkPlayerInfo {
 } NetworkPlayerInfo;
 
 typedef struct NetworkClientInfo {
-	uint16 client_index;														// Index of the client (same as ClientState->index)
-	char client_name[NETWORK_CLIENT_NAME_LENGTH];		// Name of the client
-	byte client_lang;																// The language of the client
-	byte client_playas;															// As which player is this client playing
-	uint32 client_ip;																// IP-address of the client (so he can be banned)
-	uint16 join_date;																// Gamedate the player has joined
-	char unique_id[NETWORK_NAME_LENGTH];						// Every play sends an unique id so we can indentify him
+	uint16 client_index;                          /// Index of the client (same as ClientState->index)
+	char client_name[NETWORK_CLIENT_NAME_LENGTH]; /// Name of the client
+	byte client_lang;                             /// The language of the client
+	byte client_playas;                           /// As which player is this client playing (PlayerID)
+	uint32 client_ip;                             /// IP-address of the client (so he can be banned)
+	uint16 join_date;                             /// Gamedate the player has joined
+	char unique_id[NETWORK_NAME_LENGTH];          /// Every play sends an unique id so we can indentify him
 } NetworkClientInfo;
 
 typedef struct NetworkGameList {
diff --git a/network_data.h b/network_data.h
index cc927150c..8550cd632 100644
--- a/network_data.h
+++ b/network_data.h
@@ -33,14 +33,14 @@ typedef struct Packet {
 
 typedef struct CommandPacket {
 	struct CommandPacket *next;
-	byte player;
-	uint32 cmd;
-	uint32 p1;
-	uint32 p2;
-	uint32 tile; // Always make it uint32, so it is bigmap compatible
-	uint32 dp[20]; // decode_params
-	uint32 frame; // In which frame must this packet be executed?
-	byte callback;
+	byte player;   /// player that is executing the command (PlayerID)
+	uint32 cmd;    /// command being executed
+	uint32 p1;     /// parameter p1
+	uint32 p2;     /// parameter p2
+	uint32 tile;   /// tile command being executed on ; always make it uint32, so it is bigmap compatible (TileIndex)
+	uint32 dp[20]; /// _decode_parameters (for sending strings, etc.)
+	uint32 frame;  /// the frame in which this packet is executed
+	byte callback; /// any callback function executed upon successful completion of the command
 } CommandPacket;
 
 typedef enum {
diff --git a/network_server.c b/network_server.c
index 35a5f81a9..0adb11037 100644
--- a/network_server.c
+++ b/network_server.c
@@ -775,14 +775,40 @@ DEF_SERVER_RECEIVE_COMMAND(PACKET_CLIENT_MAP_OK)
 	}
 }
 
+static inline const char* GetPlayerIP(const NetworkClientInfo *ci) {return inet_ntoa(*(struct in_addr *)&ci->client_ip);}
+
+/** Enforce the command flags.
+ * Eg a server-only command can only be executed by a server, etc.
+ * @param *cp the commandpacket that is going to be checked
+ * @param *ci client information for debugging output to console
+ */
+static bool CheckCommandFlags(const CommandPacket *cp, const NetworkClientInfo *ci)
+{
+	byte flags = GetCommandFlags(cp->cmd);
+
+	if (flags & CMD_SERVER && ci->client_index != NETWORK_SERVER_INDEX) {
+		IConsolePrintF(_iconsole_color_error, "WARNING: server only command from player %d (IP: %s), kicking...", ci->client_playas, GetPlayerIP(ci));
+		return false;
+	}
+
+	if (flags & CMD_OFFLINE) {
+		IConsolePrintF(_iconsole_color_error, "WARNING: offline only command from player %d (IP: %s), kicking...", ci->client_playas, GetPlayerIP(ci));
+		return false;
+	}
+	return true;
+}
+
+/** The client has done a command and wants us to handle it
+ * @param *cs the connected client that has sent the command
+ * @param *p the packet in which the command was sent
+ */
 DEF_SERVER_RECEIVE_COMMAND(PACKET_CLIENT_COMMAND)
 {
-	// The client has done a command and wants us to handle it
-	uint i;
-	byte callback;
 	NetworkClientState *new_cs;
-	NetworkClientInfo *ci;
+	const NetworkClientInfo *ci;
 	char *dparam_char;
+	uint i;
+	byte callback;
 
 	CommandPacket *cp = malloc(sizeof(CommandPacket));
 
@@ -798,9 +824,10 @@ DEF_SERVER_RECEIVE_COMMAND(PACKET_CLIENT_COMMAND)
 	cp->p1     = NetworkRecv_uint32(cs, p);
 	cp->p2     = NetworkRecv_uint32(cs, p);
 	cp->tile   = NetworkRecv_uint32(cs, p);
-	/* We are going to send them byte by byte, because dparam is misused
-	    for chars (if it is used), and else we have a BigEndian / LittleEndian
-	    problem.. we should fix the misuse of dparam... -- TrueLight */
+	/** @todo We are going to send dparams byte by byte, because dparam is misused
+	 * for charstrings (if it is used), and else we have a Big/Little Endian
+	 * problem.. we should fix the misuse of dparam... -- TrueLight
+	 */
 	dparam_char = (char *)&cp->dp[0];
 	for (i = 0; i < lengthof(cp->dp) * 4; i++) {
 		*dparam_char = NetworkRecv_uint8(cs, p);
@@ -809,50 +836,47 @@ DEF_SERVER_RECEIVE_COMMAND(PACKET_CLIENT_COMMAND)
 
 	callback = NetworkRecv_uint8(cs, p);
 
-	if (cs->quited)
-		return;
+	if (cs->quited) return;
+
+	ci = DEREF_CLIENT_INFO(cs);
 
 	/* Check if cp->cmd is valid */
 	if (!IsValidCommand(cp->cmd)) {
+		IConsolePrintF(_iconsole_color_error, "WARNING: invalid command from player %d (IP: %s).", ci->client_playas, GetPlayerIP(ci));
 		SEND_COMMAND(PACKET_SERVER_ERROR)(cs, NETWORK_ERROR_NOT_EXPECTED);
 		return;
 	}
 
-	ci = DEREF_CLIENT_INFO(cs);
-	// Only CMD_PLAYER_CTRL is always allowed, for the rest, playas needs
-	//  to match the player in the packet
-	if (!(cp->cmd == CMD_PLAYER_CTRL && cp->p1 == 0) && ci->client_playas-1 != cp->player) {
+	if (!CheckCommandFlags(cp, ci)) {
+		SEND_COMMAND(PACKET_SERVER_ERROR)(cs, NETWORK_ERROR_KICKED);
+		return;
+	}
+
+	/** Only CMD_PLAYER_CTRL is always allowed, for the rest, playas needs
+	 * to match the player in the packet. If it doesn't, the client has done
+	 * something pretty naughty (or a bug), and will be kicked
+	 */
+	if (!(cp->cmd == CMD_PLAYER_CTRL && cp->p1 == 0) && ci->client_playas - 1 != cp->player) {
 		IConsolePrintF(_iconsole_color_error, "WARNING: player %d (IP: %s) tried to execute a command as player %d, kicking...",
-									 ci->client_playas - 1, inet_ntoa(*(struct in_addr *)&ci->client_ip), cp->player);
-		// The player did a command with the wrong player_id.. bad!!
+									 ci->client_playas, GetPlayerIP(ci), cp->player);
 		SEND_COMMAND(PACKET_SERVER_ERROR)(cs, NETWORK_ERROR_PLAYER_MISMATCH);
 		return;
 	}
-	switch (cp->cmd) {
-		/* Player_ctrl is not always allowed */
-		case CMD_PLAYER_CTRL:
-		{
-			/* cp->p1 == 0, is allowed */
-			if (cp->p1 == 0) {
-				// UGLY! p2 is mis-used to get the client-id in CmdPlayerCtrl
-				cp->p2 = cs - _clients;
-			} else {
-			/* The rest are cheats */
-				SEND_COMMAND(PACKET_SERVER_ERROR)(cs, NETWORK_ERROR_CHEATER);
-				return;
-			}
-		} break;
-
-		/* Don't allow those commands if server == advertising (considered cheating) */
-		case CMD_MONEY_CHEAT:
-		{
-			if (_network_advertise) {
-				SEND_COMMAND(PACKET_SERVER_ERROR)(cs, NETWORK_ERROR_CHEATER);
-				return;
-			}
-		} break;
-	}
 
+	/** @todo CMD_PLAYER_CTRL with p1 = 0 announces a new player to the server. To give the
+	 * player the correct ID, the server injects p2 and executes the command. Any other p1
+	 * is prohibited. Pretty ugly and should be redone together with its function.
+	 * @see CmdPlayerCtrl() players.c:655
+	 */
+	if (cp->cmd == CMD_PLAYER_CTRL) {
+		if (cp->p1 != 0) {
+			SEND_COMMAND(PACKET_SERVER_ERROR)(cs, NETWORK_ERROR_CHEATER);
+			return;
+		}
+
+		// XXX - UGLY! p2 is mis-used to get the client-id in CmdPlayerCtrl
+		cp->p2 = cs - _clients;
+	}
 
 	// The frame can be executed in the same frame as the next frame-packet
 	//  That frame just before that frame is saved in _frame_counter_max
@@ -865,11 +889,7 @@ DEF_SERVER_RECEIVE_COMMAND(PACKET_CLIENT_COMMAND)
 		if (new_cs->status > STATUS_AUTH) {
 			// Callbacks are only send back to the client who sent them in the
 			//  first place. This filters that out.
-			if (new_cs != cs)
-				cp->callback = 0;
-			else
-				cp->callback = callback;
-
+			cp->callback = (new_cs != cs) ? 0 : callback;
 			NetworkAddCommandQueue(new_cs, cp);
 		}
 	}
diff --git a/vehicle.h b/vehicle.h
index f0699b5a6..a760cbf2b 100644
--- a/vehicle.h
+++ b/vehicle.h
@@ -420,6 +420,9 @@ static inline Vehicle *GetFirstVehicleFromSharedList(Vehicle *v)
 	return u;
 }
 
+/* Validate functions for rail building */
+static inline bool ValParamRailtype(uint32 rail) { return rail <= GetPlayer(_current_player)->max_railtype;}
+
 // NOSAVE: Can be regenerated by inspecting the vehicles.
 VARDEF VehicleID _vehicle_position_hash[0x1000];