(svn r1741) - Fix: added IsVehicleIndex() so it's possible to protect GetVehicle() from reading an invalid vehicle index

- Fix: added check for v->type in some commands, which expects v to be a specific type

Checks like this is needed to protect network servers from people, who hack their clients to either cheat or crash the server

NOTE: if I made a mistake here it can make a function unreachable when it should be used. Here is one place to look if something weird happens

1 files changed, 32 insertions, 10 deletions

diff --git a/train_cmd.c b/train_cmd.c
index b0951b53e..9c176c8df 100644
--- a/train_cmd.c
+++ b/train_cmd.c
@@ -719,7 +719,10 @@ int32 CmdMoveRailVehicle(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 	Vehicle *src, *dst, *src_head, *dst_head;
 	bool is_loco;
 
+	if (!IsVehicleIndex(p1 & 0xFFFF)) return CMD_ERROR;
+
 	src = GetVehicle(p1 & 0xFFFF);
+
 	if (src->type != VEH_Train) return CMD_ERROR;
 
 	is_loco = !(RailVehInfo(src->engine_type)->flags & RVI_WAGON)
@@ -864,9 +867,11 @@ int32 CmdStartStopTrain(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 {
 	Vehicle *v;
 
+	if (!IsVehicleIndex(p1)) return CMD_ERROR;
+
 	v = GetVehicle(p1);
 
-	if (!CheckOwnership(v->owner))
+	if (v->type != VEH_Train || !CheckOwnership(v->owner))
 		return CMD_ERROR;
 
 	if (flags & DC_EXEC) {
@@ -888,13 +893,15 @@ int32 CmdSellRailWagon(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 	Vehicle *v, *first,*last;
 	int32 cost;
 
-	SET_EXPENSES_TYPE(EXPENSES_NEW_VEHICLES);
+	if (!IsVehicleIndex(p1)) return CMD_ERROR;
 
 	v = GetVehicle(p1);
 
 	if (v->type != VEH_Train || !CheckOwnership(v->owner))
 		return CMD_ERROR;
 
+	SET_EXPENSES_TYPE(EXPENSES_NEW_VEHICLES);
+
 	// get first vehicle in chain
 	first = v;
 	if (first->subtype != TS_Front_Engine) {
@@ -1172,9 +1179,11 @@ int32 CmdReverseTrainDirection(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 {
 	Vehicle *v;
 
+	if (!IsVehicleIndex(p1)) return CMD_ERROR;
+
 	v = GetVehicle(p1);
 
-	if (!CheckOwnership(v->owner))
+	if (v->type != VEH_Train || !CheckOwnership(v->owner))
 		return CMD_ERROR;
 
 	_error_message = STR_EMPTY;
@@ -1201,9 +1210,11 @@ int32 CmdForceTrainProceed(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 {
 	Vehicle *v;
 
+	if (!IsVehicleIndex(p1)) return CMD_ERROR;
+
 	v = GetVehicle(p1);
 
-	if (!CheckOwnership(v->owner))
+	if (v->type != VEH_Train || !CheckOwnership(v->owner))
 		return CMD_ERROR;
 
 	if (flags & DC_EXEC)
@@ -1225,12 +1236,15 @@ int32 CmdRefitRailVehicle(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 
 	p2 = p2 & 0xFF;
 
-	SET_EXPENSES_TYPE(EXPENSES_TRAIN_RUN);
+	if (!IsVehicleIndex(p1)) return CMD_ERROR;
 
 	v = GetVehicle(p1);
-	if (!CheckOwnership(v->owner) || ((CheckStoppedInDepot(v) < 0) && !(SkipStoppedInDepotCheck)))
+
+	if (v->type != VEH_Train || !CheckOwnership(v->owner) || ((CheckStoppedInDepot(v) < 0) && !(SkipStoppedInDepotCheck)))
 		return CMD_ERROR;
 
+	SET_EXPENSES_TYPE(EXPENSES_TRAIN_RUN);
+
 	cost = 0;
 	num = 0;
 
@@ -1341,10 +1355,14 @@ static TrainFindDepotData FindClosestTrainDepot(Vehicle *v)
 		 bit 2 = clear v->set_for_replacement */
 int32 CmdTrainGotoDepot(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 {
-	Vehicle *v = GetVehicle(p1);
+	Vehicle *v;
 	TrainFindDepotData tfdd;
 
-	if (!CheckOwnership(v->owner))
+	if (!IsVehicleIndex(p1)) return CMD_ERROR;
+
+	v = GetVehicle(p1);
+
+	if (v->type != VEH_Train || !CheckOwnership(v->owner))
 		return CMD_ERROR;
 
 	if (HASBIT(p2, 0)) v->set_for_replacement = true;
@@ -1387,9 +1405,13 @@ int32 CmdTrainGotoDepot(int x, int y, uint32 flags, uint32 p1, uint32 p2)
  */
 int32 CmdChangeTrainServiceInt(int x, int y, uint32 flags, uint32 p1, uint32 p2)
 {
-	Vehicle *v = GetVehicle(p1);
+	Vehicle *v;
+
+	if (!IsVehicleIndex(p1)) return CMD_ERROR;
 
-	if (!CheckOwnership(v->owner))
+	v = GetVehicle(p1);
+
+	if (v->type != VEH_Train || !CheckOwnership(v->owner))
 		return CMD_ERROR;
 
 	if (flags & DC_EXEC) {