Subject: fix for vulnerability CVE-2010-0402 for OpenTTD 0.6.0 - 0.6.3 (Denial of service via improperly validated commands) From: OpenTTD developer team Origin: backport, https://github.com/OpenTTD/OpenTTD/commit/1f28e23 https://github.com/OpenTTD/OpenTTD/commit/a929ab0 https://github.com/OpenTTD/OpenTTD/commit/0f65601 https://github.com/OpenTTD/OpenTTD/commit/75d4bc9 https://github.com/OpenTTD/OpenTTD/commit/2a5ddd0 https://github.com/OpenTTD/OpenTTD/commit/5ecf2f7 https://github.com/OpenTTD/OpenTTD/commit/2365a4d Bug: https://github.com/OpenTTD/OpenTTD/issues/3748 https://github.com/OpenTTD/OpenTTD/issues/3747 In multiple places in-game commands 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 only in-game so the attacker must have access to the server: his IP must not be banned, he must know the password if it has been set and the server must not be full. The one root cause of this problem is a generalised “pool” system that uses size_t to store/access indices, but has typed validation checks to check indices. Some of these checks use 16 bit integers, which means that a 32 bit “command parameter” gets silently (by the compiler) truncated to 16 bits before being checked after which the 32 bit number is used to access items in the pool. For example item 0 exists, the pool has that one item and the command parameter is 65536. The validation function would be checking 0, as the high 16 bits of the parameter would be (silently) discarded, and as such the check returns that it is a valid item. This then results in trying to get item 65536 which is way outside of the valid range of the pool. However, it is not solely confined to pooled items; there are a few cases where a not-yet-validated value is used. Attached are patches for all vulnerable versions since 0.6.0. Note that this is in essence a partial backport of trunk r19657, r19656, r19655, r19654, r19637, r19633 and r19621. The “in essence” is because those patches change the whole handling of command parameters which is 90% changing to conform to the new policy or standard and 10% actually fixes the crashes. Therefor we chose to make one minimalistic patch for each of the stable series since 0.6 that solves all issues we are aware of, without ending up with a ten times larger diff with many unrelated unneeded changes. Also earlier releases were more vulnerable than the later ones due to improvements in the pool system and some rewrites. Due to the time needed for assembling these patches and because we are unaware of any Linux distribution shipping OpenTTD from before the 0.6 series in a still supported release we have not made patches for those. They are vulnerable though. This fix maintains network compatability of patched and unpatched OpenTTD’s except the cases where the unpatched version would crash, but then the crash would be the cause of the disconnection and not that they are incompatible on that part of the network communication. Index: src/terraform_cmd.cpp =================================================================== --- src/terraform_cmd.cpp (revision 19678) +++ src/terraform_cmd.cpp (working copy) @@ -365,7 +365,7 @@ oldh = TileHeight(p1); /* compute new height */ - h = oldh + p2; + h = oldh + (int8)p2; /* Check range of destination height */ if (h > MAX_TILE_HEIGHT) return_cmd_error((oldh == 0) ? STR_1003_ALREADY_AT_SEA_LEVEL : STR_1004_TOO_HIGH); Index: src/rail_cmd.cpp =================================================================== --- src/rail_cmd.cpp (revision 19678) +++ src/rail_cmd.cpp (working copy) @@ -305,8 +305,8 @@ CommandCost CmdBuildSingleRail(TileIndex tile, uint32 flags, uint32 p1, uint32 p2) { Slope tileh; - RailType railtype = (RailType)p1; - Track track = (Track)p2; + RailType railtype = (RailType)GB(p1, 0, 4); + Track track = (Track)GB(p2, 0, 3); TrackBits trackbit; CommandCost cost(EXPENSES_CONSTRUCTION); CommandCost ret; @@ -438,12 +438,12 @@ */ CommandCost CmdRemoveSingleRail(TileIndex tile, uint32 flags, uint32 p1, uint32 p2) { - Track track = (Track)p2; + Track track = (Track)GB(p2, 0, 3); TrackBits trackbit; CommandCost cost(EXPENSES_CONSTRUCTION, _price.remove_rail ); bool crossing = false; - if (!ValParamTrackOrientation((Track)p2)) return CMD_ERROR; + if (!ValParamTrackOrientation(track)) return CMD_ERROR; trackbit = TrackToTrackBits(track); /* Need to read tile owner now because it may change when the rail is removed @@ -734,6 +734,7 @@ Slope tileh; /* check railtype and valid direction for depot (0 through 3), 4 in total */ + p1 = GB(p1, 0, 4); if (!ValParamRailtype((RailType)p1)) return CMD_ERROR; tileh = GetTileSlope(tile, NULL); @@ -984,10 +985,9 @@ bool semaphores = HasBit(p2, 4); bool remove = HasBit(p2, 5); bool autofill = HasBit(p2, 6); - Trackdir trackdir = TrackToTrackdir(track); byte signal_density = GB(p2, 24, 8); - if (p1 >= MapSize()) return CMD_ERROR; + if (p1 >= MapSize() || !ValParamTrackOrientation(track)) return CMD_ERROR; end_tile = p1; if (signal_density == 0 || signal_density > 20) return CMD_ERROR; @@ -997,6 +997,7 @@ * since the original amount will be too dense (shorter tracks) */ signal_density *= 2; + Trackdir trackdir = TrackToTrackdir(track); if (CmdFailed(ValidateAutoDrag(&trackdir, tile, end_tile))) return CMD_ERROR; track = TrackdirToTrack(trackdir); /* trackdir might have changed, keep track in sync */ @@ -1178,7 +1179,7 @@ CommandCost CmdConvertRail(TileIndex tile, uint32 flags, uint32 p1, uint32 p2) { CommandCost cost(EXPENSES_CONSTRUCTION); - RailType totype = (RailType)p2; + RailType totype = (RailType)GB(p2, 0, 4); if (!ValParamRailtype(totype)) return CMD_ERROR; if (p1 >= MapSize()) return CMD_ERROR; Index: src/group_cmd.cpp =================================================================== --- src/group_cmd.cpp (revision 19678) +++ src/group_cmd.cpp (working copy) @@ -88,7 +88,7 @@ */ CommandCost CmdCreateGroup(TileIndex tile, uint32 flags, uint32 p1, uint32 p2) { - VehicleType vt = (VehicleType)p1; + VehicleType vt = (VehicleType)GB(p1, 0, 3); if (!IsPlayerBuildableVehicleType(vt)) return CMD_ERROR; if (!Group::CanAllocateItem()) return CMD_ERROR; @@ -114,6 +114,7 @@ */ CommandCost CmdDeleteGroup(TileIndex tile, uint32 flags, uint32 p1, uint32 p2) { + p1 = GB(p1, 0, 16); if (!IsValidGroupID(p1)) return CMD_ERROR; Group *g = GetGroup(p1); @@ -176,6 +177,7 @@ */ CommandCost CmdRenameGroup(TileIndex tile, uint32 flags, uint32 p1, uint32 p2) { + p1 = GB(p1, 0, 16); if (!IsValidGroupID(p1) || StrEmpty(_cmd_text)) return CMD_ERROR; Group *g = GetGroup(p1); @@ -206,6 +208,8 @@ */ CommandCost CmdAddVehicleGroup(TileIndex tile, uint32 flags, uint32 p1, uint32 p2) { + p1 = GB(p1, 0, 16); + p2 = GB(p2, 0, 16); GroupID new_g = p1; if (!IsValidVehicleID(p2) || (!IsValidGroupID(new_g) && !IsDefaultGroupID(new_g))) return CMD_ERROR; @@ -253,12 +257,12 @@ */ CommandCost CmdAddSharedVehicleGroup(TileIndex tile, uint32 flags, uint32 p1, uint32 p2) { - VehicleType type = (VehicleType)p2; + p1 = GB(p1, 0, 16); + VehicleType type = (VehicleType)GB(p2, 0, 3); if (!IsValidGroupID(p1) || !IsPlayerBuildableVehicleType(type)) return CMD_ERROR; if (flags & DC_EXEC) { Vehicle *v; - VehicleType type = (VehicleType)p2; GroupID id_g = p1; /* Find the first front engine which belong to the group id_g @@ -290,7 +294,8 @@ */ CommandCost CmdRemoveAllVehiclesGroup(TileIndex tile, uint32 flags, uint32 p1, uint32 p2) { - VehicleType type = (VehicleType)p2; + p1 = GB(p1, 0, 16); + VehicleType type = (VehicleType)GB(p2, 0, 3); if (!IsValidGroupID(p1) || !IsPlayerBuildableVehicleType(type)) return CMD_ERROR; Group *g = GetGroup(p1); @@ -327,6 +332,7 @@ */ CommandCost CmdSetGroupReplaceProtection(TileIndex tile, uint32 flags, uint32 p1, uint32 p2) { + p1 = GB(p1, 0, 16); if (!IsValidGroupID(p1)) return CMD_ERROR; Group *g = GetGroup(p1); Index: src/station_cmd.cpp =================================================================== --- src/station_cmd.cpp (revision 19678) +++ src/station_cmd.cpp (working copy) @@ -1320,6 +1320,7 @@ */ CommandCost CmdBuildRoadStop(TileIndex tile, uint32 flags, uint32 p1, uint32 p2) { + p1 = GB(p1, 0, 2); bool type = HasBit(p2, 0); bool is_drive_through = HasBit(p2, 1); bool build_over_road = is_drive_through && IsNormalRoadTile(tile); @@ -2638,6 +2639,7 @@ */ CommandCost CmdRenameStation(TileIndex tile, uint32 flags, uint32 p1, uint32 p2) { + p1 = GB(p1, 0, 16); if (!IsValidStationID(p1) || StrEmpty(_cmd_text)) return CMD_ERROR; Station *st = GetStation(p1); Index: src/ship_cmd.cpp =================================================================== --- src/ship_cmd.cpp (revision 19678) +++ src/ship_cmd.cpp (working copy) @@ -810,6 +810,7 @@ UnitID unit_num; Engine *e; + p1 = GB(p1, 0, 16); if (!IsEngineBuildable(p1, VEH_SHIP, _current_player)) return_cmd_error(STR_SHIP_NOT_AVAILABLE); value = EstimateShipCost(p1); Index: src/order_cmd.cpp =================================================================== --- src/order_cmd.cpp (revision 19678) +++ src/order_cmd.cpp (working copy) @@ -173,7 +173,7 @@ v = GetVehicle(veh); - if (!CheckOwnership(v->owner)) return CMD_ERROR; + if (!v->IsPrimaryVehicle() || !CheckOwnership(v->owner)) return CMD_ERROR; /* Check if the inserted order is to the correct destination (owner, type), * and has the correct flags if any */ @@ -455,15 +455,15 @@ CommandCost CmdDeleteOrder(TileIndex tile, uint32 flags, uint32 p1, uint32 p2) { Vehicle *v, *u; - VehicleID veh_id = p1; - VehicleOrderID sel_ord = p2; + VehicleID veh_id = GB(p1, 0, 16); + VehicleOrderID sel_ord = GB(p2, 0, 8); Order *order; if (!IsValidVehicleID(veh_id)) return CMD_ERROR; v = GetVehicle(veh_id); - if (!CheckOwnership(v->owner)) return CMD_ERROR; + if (!v->IsPrimaryVehicle() || !CheckOwnership(v->owner)) return CMD_ERROR; /* If we did not select an order, we maybe want to de-clone the orders */ if (sel_ord >= v->num_orders) @@ -535,14 +535,14 @@ CommandCost CmdSkipToOrder(TileIndex tile, uint32 flags, uint32 p1, uint32 p2) { Vehicle *v; - VehicleID veh_id = p1; - VehicleOrderID sel_ord = p2; + VehicleID veh_id = GB(p1, 0, 16); + VehicleOrderID sel_ord = GB(p2, 0, 8); if (!IsValidVehicleID(veh_id)) return CMD_ERROR; v = GetVehicle(veh_id); - if (!CheckOwnership(v->owner) || sel_ord == v->cur_order_index || + if (!v->IsPrimaryVehicle() || !CheckOwnership(v->owner) || sel_ord == v->cur_order_index || sel_ord >= v->num_orders || v->num_orders < 2) return CMD_ERROR; if (flags & DC_EXEC) { @@ -579,14 +579,14 @@ */ CommandCost CmdMoveOrder(TileIndex tile, uint32 flags, uint32 p1, uint32 p2) { - VehicleID veh = p1; + VehicleID veh = GB(p1, 0, 16); VehicleOrderID moving_order = GB(p2, 0, 16); VehicleOrderID target_order = GB(p2, 16, 16); if (!IsValidVehicleID(veh)) return CMD_ERROR; Vehicle *v = GetVehicle(veh); - if (!CheckOwnership(v->owner)) return CMD_ERROR; + if (!v->IsPrimaryVehicle() || !CheckOwnership(v->owner)) return CMD_ERROR; /* Don't make senseless movements */ if (moving_order >= v->num_orders || target_order >= v->num_orders || @@ -675,7 +675,7 @@ v = GetVehicle(veh); - if (!CheckOwnership(v->owner)) return CMD_ERROR; + if (!v->IsPrimaryVehicle() || !CheckOwnership(v->owner)) return CMD_ERROR; /* Is it a valid order? */ if (sel_ord >= v->num_orders) return CMD_ERROR; @@ -753,7 +753,7 @@ dst = GetVehicle(veh_dst); - if (!CheckOwnership(dst->owner)) return CMD_ERROR; + if (!dst->IsPrimaryVehicle() || !CheckOwnership(dst->owner)) return CMD_ERROR; switch (p2) { case CO_SHARE: { @@ -764,7 +764,7 @@ src = GetVehicle(veh_src); /* Sanity checks */ - if (!CheckOwnership(src->owner) || dst->type != src->type || dst == src) + if (!src->IsPrimaryVehicle() || !CheckOwnership(src->owner) || dst->type != src->type || dst == src) return CMD_ERROR; /* Trucks can't share orders with busses (and visa versa) */ @@ -811,7 +811,7 @@ src = GetVehicle(veh_src); /* Sanity checks */ - if (!CheckOwnership(src->owner) || dst->type != src->type || dst == src) + if (!src->IsPrimaryVehicle() || !CheckOwnership(src->owner) || dst->type != src->type || dst == src) return CMD_ERROR; /* Trucks can't copy all the orders from busses (and visa versa) */ @@ -886,11 +886,12 @@ CargoID cargo = GB(p2, 0, 8); byte subtype = GB(p2, 8, 8); + if (cargo >= NUM_CARGO) return CMD_ERROR; if (!IsValidVehicleID(veh)) return CMD_ERROR; v = GetVehicle(veh); - if (!CheckOwnership(v->owner)) return CMD_ERROR; + if (!v->IsPrimaryVehicle() || !CheckOwnership(v->owner)) return CMD_ERROR; order = GetVehicleOrder(v, order_number); if (order == NULL) return CMD_ERROR; @@ -1033,12 +1034,13 @@ VehicleOrderID cur_ord = GB(p2, 0, 16); uint16 serv_int = GB(p2, 16, 16); + p1 = GB(p1, 0, 16); if (!IsValidVehicleID(p1)) return CMD_ERROR; v = GetVehicle(p1); /* Check the vehicle type and ownership, and if the service interval and order are in range */ - if (!CheckOwnership(v->owner)) return CMD_ERROR; + if (!v->IsPrimaryVehicle() || !CheckOwnership(v->owner)) return CMD_ERROR; if (serv_int != GetServiceIntervalClamped(serv_int) || cur_ord >= v->num_orders) return CMD_ERROR; if (flags & DC_EXEC) { Index: src/waypoint.cpp =================================================================== --- src/waypoint.cpp (revision 19678) +++ src/waypoint.cpp (working copy) @@ -364,6 +364,7 @@ */ CommandCost CmdRenameWaypoint(TileIndex tile, uint32 flags, uint32 p1, uint32 p2) { + p1 = GB(p1, 0, 16); Waypoint *wp; if (!IsValidWaypointID(p1)) return CMD_ERROR; Index: src/train_cmd.cpp =================================================================== --- src/train_cmd.cpp (revision 19678) +++ src/train_cmd.cpp (working copy) @@ -765,6 +765,7 @@ */ CommandCost CmdBuildRailVehicle(TileIndex tile, uint32 flags, uint32 p1, uint32 p2) { + p1 = GB(p1, 0, 16); /* Check if the engine-type is valid (for the player) */ if (!IsEngineBuildable(p1, VEH_TRAIN, _current_player)) return_cmd_error(STR_RAIL_VEHICLE_NOT_AVAILABLE); Index: src/tunnelbridge_cmd.cpp =================================================================== --- src/tunnelbridge_cmd.cpp (revision 19678) +++ src/tunnelbridge_cmd.cpp (working copy) @@ -218,7 +218,7 @@ break; case TRANSPORT_RAIL: - railtype = (RailType)GB(p2, 8, 8); + railtype = (RailType)GB(p2, 8, 4); if (!ValParamRailtype(railtype)) return CMD_ERROR; break; @@ -467,7 +467,7 @@ _build_tunnel_endtile = 0; if (transport_type == TRANSPORT_RAIL) { - if (!ValParamRailtype((RailType)p1)) return CMD_ERROR; + if (!ValParamRailtype((RailType)GB(p1, 0, 4))) return CMD_ERROR; } else { const RoadTypes rts = (RoadTypes)GB(p1, 0, 3); if (!AreValidRoadTypes(rts) || !HasRoadTypesAvail(_current_player, rts)) return CMD_ERROR; Index: src/water_cmd.cpp =================================================================== --- src/water_cmd.cpp (revision 19678) +++ src/water_cmd.cpp (working copy) @@ -348,7 +348,7 @@ int y; int sx, sy; - if (p1 >= MapSize()) return CMD_ERROR; + if (p1 >= MapSize() || p2 > 2) return CMD_ERROR; /* Outside of the editor you can only build canals, not oceans */ if (p2 != 0 && _game_mode != GM_EDITOR) return CMD_ERROR; Index: src/economy.cpp =================================================================== --- src/economy.cpp (revision 19678) +++ src/economy.cpp (working copy) @@ -1503,7 +1503,6 @@ SetBit(v->vehicle_flags, VF_CARGO_UNLOADING); continue; } - GoodsEntry *ge = &st->goods[v->cargo_type]; const CargoList::List *cargos = v->cargo.Packets(); @@ -1889,6 +1888,7 @@ { Player *p; CommandCost cost(EXPENSES_OTHER); + p1 = GB(p1, 0, 8); /* Check if buying shares is allowed (protection against modified clients) */ /* Cannot buy own shares */ @@ -1938,6 +1938,7 @@ { Player *p; Money cost; + p1 = GB(p1, 0, 8); /* Check if selling shares is allowed (protection against modified clients) */ /* Cannot sell own shares */ @@ -1975,6 +1976,7 @@ */ CommandCost CmdBuyCompany(TileIndex tile, uint32 flags, uint32 p1, uint32 p2) { + p1 = GB(p1, 0, 8); Player *p; PlayerID pid = (PlayerID)p1; Index: src/rail.cpp =================================================================== --- src/rail.cpp (revision 19678) +++ src/rail.cpp (working copy) @@ -186,7 +186,7 @@ bool ValParamRailtype(const RailType rail) { - return HasRailtypeAvail(_current_player, rail); + return rail < RAILTYPE_END && HasRailtypeAvail(_current_player, rail); } RailType GetBestRailtype(const PlayerID p) Index: src/roadveh_cmd.cpp =================================================================== --- src/roadveh_cmd.cpp (revision 19678) +++ src/roadveh_cmd.cpp (working copy) @@ -172,6 +172,7 @@ UnitID unit_num; Engine *e; + p1 = GB(p1, 0, 16); if (!IsEngineBuildable(p1, VEH_ROAD, _current_player)) return_cmd_error(STR_ROAD_VEHICLE_NOT_AVAILABLE); cost = EstimateRoadVehCost(p1); Index: src/vehicle.cpp =================================================================== --- src/vehicle.cpp (revision 19678) +++ src/vehicle.cpp (working copy) @@ -1763,6 +1763,7 @@ CommandCost cost; VehicleType vehicle_type = (VehicleType)GB(p1, 0, 8); + if (!IsPlayerBuildableVehicleType(vehicle_type)) return CMD_ERROR; if (!IsDepotTile(tile) || !IsTileOwner(tile, _current_player)) return CMD_ERROR; /* Get the list of vehicles in the depot */ @@ -1826,8 +1827,10 @@ CommandCost cost, total_cost(EXPENSES_NEW_VEHICLES); uint32 build_argument = 2; + p1 = GB(p1, 0, 16); if (!IsValidVehicleID(p1)) return CMD_ERROR; v = GetVehicle(p1); + if (!v->IsPrimaryVehicle()) return CMD_ERROR; v_front = v; w = NULL; w_front = NULL; @@ -2162,7 +2165,7 @@ } break; - default: NOT_REACHED(); break; + default: return 0; } if ((n + 100) < *length_of_array) { @@ -2395,11 +2398,12 @@ { Vehicle *v; + p1 = GB(p1, 0, 16); if (!IsValidVehicleID(p1) || StrEmpty(_cmd_text)) return CMD_ERROR; v = GetVehicle(p1); - if (!CheckOwnership(v->owner)) return CMD_ERROR; + if (!v->IsPrimaryVehicle() || !CheckOwnership(v->owner)) return CMD_ERROR; if (!IsUniqueVehicleName(_cmd_text)) return_cmd_error(STR_NAME_MUST_BE_UNIQUE); @@ -2425,11 +2429,12 @@ Vehicle* v; uint16 serv_int = GetServiceIntervalClamped(p2); /* Double check the service interval from the user-input */ + p1 = GB(p1, 0, 16); if (serv_int != p2 || !IsValidVehicleID(p1)) return CMD_ERROR; v = GetVehicle(p1); - if (!CheckOwnership(v->owner)) return CMD_ERROR; + if (!v->IsPrimaryVehicle() || !CheckOwnership(v->owner)) return CMD_ERROR; if (flags & DC_EXEC) { v->service_interval = serv_int; Index: src/industry_cmd.cpp =================================================================== --- src/industry_cmd.cpp (revision 19678) +++ src/industry_cmd.cpp (working copy) @@ -1624,8 +1624,11 @@ */ CommandCost CmdBuildIndustry(TileIndex tile, uint32 flags, uint32 p1, uint32 p2) { - const IndustrySpec *indspec = GetIndustrySpec(GB(p1, 0, 16)); + IndustryType it = GB(p1, 0, 16); + if (it >= NUM_INDUSTRYTYPES) return CMD_ERROR; + const IndustrySpec *indspec = GetIndustrySpec(it); + /* Check if the to-be built/founded industry is available for this climate. */ if (!indspec->enabled) { return CMD_ERROR; @@ -1648,7 +1651,7 @@ * because parameter evaluation order is not guaranteed in the c++ standard */ tile = RandomTile(); - const Industry *ind = CreateNewIndustryHelper(tile, p1, flags, indspec, RandomRange(indspec->num_table), p2); + const Industry *ind = CreateNewIndustryHelper(tile, it, flags, indspec, RandomRange(indspec->num_table), p2); if (ind != NULL) { SetDParam(0, indspec->name); if (indspec->new_industry_text > STR_LAST_STRINGID) { @@ -1675,7 +1678,7 @@ if (--num < 0) num = indspec->num_table - 1; } while (!CheckIfIndustryTilesAreFree(tile, itt[num], num, p1)); - if (CreateNewIndustryHelper(tile, p1, flags, indspec, num, p2) == NULL) return CMD_ERROR; + if (CreateNewIndustryHelper(tile, it, flags, indspec, num, p2) == NULL) return CMD_ERROR; } return CommandCost(EXPENSES_OTHER, indspec->GetConstructionCost()); Index: src/aircraft_cmd.cpp =================================================================== --- src/aircraft_cmd.cpp (revision 19678) +++ src/aircraft_cmd.cpp (working copy) @@ -276,6 +276,7 @@ */ CommandCost CmdBuildAircraft(TileIndex tile, uint32 flags, uint32 p1, uint32 p2) { + p1 = GB(p1, 0, 16); if (!IsEngineBuildable(p1, VEH_AIRCRAFT, _current_player)) return_cmd_error(STR_AIRCRAFT_NOT_AVAILABLE); const AircraftVehicleInfo *avi = AircraftVehInfo(p1);