Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix command handlers, and the command packet handler #4745

Closed
DorpsGek opened this issue Aug 25, 2011 · 5 comments
Closed

Fix command handlers, and the command packet handler #4745

DorpsGek opened this issue Aug 25, 2011 · 5 comments
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay

Comments

@DorpsGek
Copy link
Member

monoid opened the ticket and wrote:

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.

Attachments

Reported version: trunk
Operating system: All


This issue was imported from FlySpray: https://bugs.openttd.org/task/4745
@DorpsGek
Copy link
Member Author

michi_cc wrote:

How is a call to whatever address is stored directly after _callback_table not exploitable to crash a client/server?


This comment was imported from FlySpray: https://bugs.openttd.org/task/4745#comment10234

@DorpsGek
Copy link
Member Author

michi_cc wrote:

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).


This comment was imported from FlySpray: https://bugs.openttd.org/task/4745#comment10235

@DorpsGek
Copy link
Member Author

monoid wrote:

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.


This comment was imported from FlySpray: https://bugs.openttd.org/task/4745#comment10238

@DorpsGek
Copy link
Member Author

michi_cc wrote:

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).


This comment was imported from FlySpray: https://bugs.openttd.org/task/4745#comment10240

@DorpsGek
Copy link
Member Author

Rubidium closed the ticket.

Reason for closing: Fixed

In r22845. Thanks for the patch


This comment was imported from FlySpray: https://bugs.openttd.org/task/4745

@DorpsGek DorpsGek added Core flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay labels Apr 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay
Projects
None yet
Development

No branches or pull requests

1 participant