OpenTTD

Tasklist

FS#4745 - Fix command handlers, and the command packet handler

Attached to Project: OpenTTD
Opened by Matt D. (monoid) - Thursday, 25 August 2011, 15:16 GMT
Last edited by Remko Bijker (Rubidium) - Friday, 26 August 2011, 16:39 GMT
Type Patch
Category Core
Status Closed
Assigned To No-one
Operating System All
Severity Medium
Priority Normal
Reported Version trunk
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

This patch fixes issues in a few command handlers:

* CMD_SET_AUTOREPLACE: A group not owned by the current company can be added to the current company's autoreplace lists. This doesn't have any bad consequences, but is still an oversight in verification of the command parameters.

* CMD_INSERT_ORDER: Conditional orders' variables and comparators have an off-by-one error in their verifications. This allows invalid orders to be inserted, leading to a NOT_REACHED in ProcessConditionalOrder. This is definitely exploitable.

* CMD_DEPOT_SELL_ALL_VEHICLES: The sell_command is retrieved using GetCmdSellVeh on the user-provided vehicle_type before vehicle_type itself is verified. No real bad consequences, but this verification should be moved to before this.

It also fixes an off-by-one error in the verification of the callback parameter in command packets. This doesn't have any bad consequences, as the usage of the parameter in looking up the _callback_table should not be entirely out-of-bounds, nor does the value retrieved get used by the server or any clients in a exploitable fashion.
   fixcmds.diff (2.7 KiB)
This task depends upon

Closed by  Remko Bijker (Rubidium)
Friday, 26 August 2011, 16:39 GMT
Reason for closing:  Fixed
Additional comments about closing:  In r22845. Thanks for the patch
Comment by Michael Lutz (michi_cc) - Thursday, 25 August 2011, 19:21 GMT
How is a call to whatever address is stored directly after _callback_table not exploitable to crash a client/server?
Comment by Michael Lutz (michi_cc) - Thursday, 25 August 2011, 19:50 GMT
Paste from IRC:

<michi_cc> Regarding the InsertOrder part of 4745, the problem itself is there since r12667 (i.e. the very first conditional order revision) even if the check in question was only added in r13752 (there wasn't any check before).
<michi_cc> So everything from 0.7.0 onwards is vulnerable. The lines in question weren't changed at all since r13752, so a single patch should apply to all revision.

<michi_cc> The wrong test for the callback is there since rForever (i.e. r942, merge network branch).
Comment by Matt D. (monoid) - Friday, 26 August 2011, 05:59 GMT
michi_cc: As far as I'm aware, the callback does not get called on the server; it is only used in comparisons when it needs to be converted back into a callback number via the _callback_table (which will most likely fail to match, and hence give 0 (network_command.cpp:337)). It also does not get used on other clients as the server only sends the callback number back to the originating client (network_command.cpp:248). However, the server could send the bad callback number to the client, which it would try to execute, probably leading to a crash.
Comment by Michael Lutz (michi_cc) - Friday, 26 August 2011, 10:54 GMT
Ah, yes, indeed. So using the callback to DoS clients with a specially crafted server is possible. DoS'ing the server is only possible in the very unlikely event that the callback table somehow is directly at the end of a memory page and the next page is unmapped (same applies to the sell vehicle command).

Loading...